On Thu, Apr 12, 2012 at 02:44:54PM +1200, Alexey Korolev wrote: > On 04/04/12 15:31, Kevin O'Connor wrote: > > Agreed - the only thing it does is force a minimum size for memory bars as > > you pointed out in a previous email. As above, I did play with > > this a little more on Sunday. I also added in two patches from Gerd's > > series and made alignment handling more explicit. I'm including the > > series here if you're interested. Again, I think this should wait until > > after the 1.7.0 release. -Kevin > Hi Kevin, > > Sorry for late response, was quite busy. > I've reviewed and tried your patches. > > [Patches 1-4] are almost the same as I proposed in this series.
Yes. It's your patches tweaked slightly. > The noticeable differences are: > 1) instead of sorting entries at mapping stage your patches sort entries at > allocation stage. (no difference in behavior or readability so > any approach is fine for me) > 2) instead of storing pointer to bus resource which the bridge device > represents (this_bus), it is obtained from pci structure and array of > "busses". > - entry->this_bus->r[entry->type].base = entry->base; > + struct pci_bus *child_bus = &busses[entry->dev->secondary_bus]; > + child_bus->r[entry->type].base = addr; > Since "entry->this_bus" member is removed the information about whether the > resource is bridge region or PCI BAR is encoded inside > "entry->bar". > Value "-1" - means this is a bridge region, the positive values mean it is a > BAR. > IMHO 2) makes code less readable, at least a comment explaining the meaning > of negative value of PCI BAR is required. > I've found just cosmetic difference to my patches so please don't forget to > add my sign-off-by to them. Okay - will do. > [Patch 5] Track region alignment explicitly. Looks very good and safe. Should > reduce address range wastes. > > [Patch 6] Small patch to account resources of PCI bridges. > Looks fine. > May be instead of > + num_regions = 2; > I would add > #define PCI_BRIDGE_NUM_REGIONS 2 > ..... > + num_regions = PCI_BRIDGE_NUM_REGIONS; This was me playing with some of Gerd's patches. I think it would require additional changes before committing. In particular, the patch misses the ROM bar on bridges - I think maybe it should be changed to keep the same loop constraints but add a "if (bar > PCI_BRIDGE_NUM_REGIONS && bar < PCI_ROM_SLOT) continue;". > > [Patch 7] 64bit aware code. Patch is incomplete. It is not working. This was also me playing with one of Gerd's patches. It just makes the bar read/write code 64bit aware. It doesn't actually program them. The logic to do real 64bit allocations would require list merging. Is this something you have looked at? -Kevin
