On Fri, Mar 01, 2013 at 03:13:34PM +0000, Rob Herring wrote:
> On 03/01/2013 06:23 AM, Andrew Murray wrote:
> > This patch factors out common implementations patterns to reduce overall kernel
> > code and provide a means for host bridge drivers to directly obtain struct
> > resources from the DT's ranges property without relying on architecture specific
> > DT handling. This will make it easier to write archiecture independent host bridge
> > drivers and mitigate against further duplication of DT parsing code.
> >
> > This patch can be used in the following way:
> >
> > struct of_pci_range_iter iter;
> > for_each_of_pci_range(&iter, np) {
> >
> > //directly access properties of the address range, e.g.:
> > //iter.pci_space, iter.pci_addr, iter.cpu_addr, iter.size or
> > //iter.flags
> >
> > //alternatively obtain a struct resource, e.g.:
> > //struct resource res;
> > //range_iter_fill_resource(iter, np, res);
> > }
> >
> > Additionally the implementation takes care of adjacent ranges and merges them
> > into a single range (as was the case with powerpc and microblaze).
> >
> > The modifications to microblaze, mips and powerpc have not been tested.
> >
> > v2:
> > This follows on from suggestions made by Grant Likely
> > (marc.info/?l=linux-kernel&m=136079602806328)
> >
> > Signed-off-by: Andrew Murray <Andrew.Murray@xxxxxxx>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> > ---
> > arch/microblaze/pci/pci-common.c | 100 +++++++++++--------------------------
> > arch/mips/pci/pci.c | 44 ++++-------------
> > arch/powerpc/kernel/pci-common.c | 93 ++++++++++-------------------------
> > drivers/of/address.c | 54 ++++++++++++++++++++
> > include/linux/of_address.h | 30 +++++++++++
> > 5 files changed, 151 insertions(+), 170 deletions(-)
>
> The thing is that this still leaves pci_process_bridge_OF_ranges
> basically identical for microblaze and powerpc which is really what
> needs to be moved out to common code. Obviously, struct pci_controller
> vs. struct pci_sys_data on ARM is an issue, but they all have
> fundamentally the same data.
Yes it does. To make things worse struct pci_controller is duplicated and
pretty much identical between microblaze and powerpc. There is good scope
for getting rid of lots of code here :).
>
> All these common fields should be in a common PCI controller struct.
> Perhaps introducing this with just what you need would work. Depending
> how invasive moving those fields to a new struct is, you could have a
> wrapper that just copies/translates the fields to the arch specific struct.
Yes I see how this would be a good approach. Though my concern would be how
quirks are handled - if microblaze has the same quirks as powerpc then you'll
see the same duplicated code between those two architectures. Or you'd see
the architecture code pick apart the common pci controller struct... I'll
investigate and see what can be done.
A lack of an accepted way to parse DT ranges on ARM is blocking Thierry,
Thomas and Jingoo from upstreaming their drivers - do you think there is some
middle ground or temporary solution for these drivers?
>
> There's also things like ioremap of the i/o range. ARM uses a fixed
> virtual address, so we need to do something different. Just returning
> the i/o cpu_addr and moving the ioremap out of this function would solve
> that.
Yes I've noticed this wasn't quite right. I'm not quite sure how this fits
in with the DT. I guess the DT ranges would contain 0 for the PCI address
and a physical address which represents the host bridges I/O range. You
would then use these two addresses as inputs to pci_ioremap_io - and then
set the start address of the struct resource to 0 and pass to
pci_add_resource_offset with io_offset set to 0 - does this seem correct for
ARM?
Andrew Murray
>
> Rob
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
