[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