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@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