[coreboot] patch: resource map proposal.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Aug 4 22:50:50 CEST 2008


On 04.08.2008 20:27, ron minnich wrote:
> On Mon, Aug 4, 2008 at 11:11 AM, Carl-Daniel Hailfinger wrote:
>
>   
>>> #define pci_write_config32(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
>>>       
>
> Yeah I can do that. I think it is more confusing but I'm not picky.
> I'd rather see the raw form as a reminder.
>   

I can't argue with that one.

> But if the other form is preferred it takes me about 2 seconds to change :-)
>   

Peter already gave you an Ack for the patch (see his comment about
compilation, though).
If you remove the pci_write_config part of the patch and/or replace it
with a macro, the patch is also
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

I do have some minor comments, but feel free to commit without
addressing them. Cut-and-paste reduced to the areas I want to comment on.

> +/**
> + * setup a resource map. 
> + * for PCRM entries, add a pci device offset, and a pci "OR value offset"
>   

"for PCIRM entries,..."
"OR value offset is not entirely clear.


> + * for IO8 and IO32 entries, add an io base offset. 
> + * This function combines a bunch of seperate functions that were scattered
> + * throughout v2. It may be awkward but it does allow for one rmap for 
> + * all settings, which is handy. See include/arch/x86/cpu.h for usage on 
> + * how to set up a resource map. 
> + *
> + * @param rm The resource map
> + * @param max The map size
> + * @param offset_dev pci device offset. This can be useful on e.g. k8
> + *        we have a number of similar devices which need the same setups
> + *        we can use one map for more than one device. NOTE: 
> + *        offset_dev IS NOT ASSUMED TO BE OFFSET BY FN (i.e. it is not << 3)
>   

* offset_dev IS NOT ASSUMED TO BE SHIFTED BY FN (i.e. it is not << 3)


> + * @param offset_pciio added to the OR value for setting up PCI IO
> + * @param offset_io offset from the io base in the resource map
> + */
> +
> +void setup_resource_map_x_offset(const rmap *rm, u32 max,
> +                                 u32 offset_dev, u32 offset_pciio, 
> +                                 u32 offset_io)
>   

I had some trouble understanding the comments, but after rereading them
they look pretty ok.

> /* pci config map */
> struct pcm

struct pcicm would be a bit more readable.

> struct io8

Maybe struct io8_modify or struct io8_change? Not urgent.

> /* convenience initializer */
> #define PCM(abus,adev,afn,areg,aand,aor) {.type = PCIRM, {.pcm ={.bus=abus,.dev=adev,.fn=afn,.reg=areg,.and=aand,.or=aor}}}
> #define EIO8(aport, aand, aor) {.type=IO8, {.io8 = {.port = aport, .and = aand, .or = aor}}}
> #define EIO32(aport, aand, aaor) {.type = IO32, {.io32 = {.port = aport, .and = aand, .or = aor}}}

Consistent naming, please. If "E" stands for initializer, please use
EPCIRM instead of PCM.

Overall, I like the patch and want to see it in the tree. Your work is
very valuable. I hope you didn't get the impression that I want to stall
you.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list