On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> On Tue, 12 Feb 2013 18:30:11 +0000, Arnd Bergmann wrote:
> > On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> > > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> > > new file mode 100644
> > > index 0000000..3ad563f
> > > --- /dev/null
> > > +++ b/drivers/pci/host/Makefile
> > > @@ -0,0 +1,4 @@
> > > +obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
> > > +CFLAGS_pci-mvebu.o += \
> > > + -I$(srctree)/arch/arm/plat-orion/include \
> > > + -I$(srctree)/arch/arm/mach-mvebu/include
> >
> > This does not seem like a good idea to me. We should not include
> > architecture specific directories from a driver directory.
> >
> > What are the header files you need here?
>
> From the patch itself:
>
> +#include <plat/pcie.h>
> +#include <mach/addr-map.h>
>
> <plat/pcie.h> is needed for a few PCIe functions shared with earlier
> families of Marvell SoC. My plan is that once this PCI driver gets
> accepted, I work on migrating the earlier Marvell SoC families to using
> this PCI driver, and therefore those functions would ultimately move in
> the driver in drivers/pci/host/, which would remove the <plat/pcie.h>.
Hmm, although it is a bit unusual, I would actually propose duplicating
that code for now, and merging a copy into the new driver right away.
This gets rid of the header file dependency and lets you just delete
the old file when the other platforms are converted.
> The <mach/addr-map.h> is here to access the address decoding windows
> allocation/free API. And for this, there is no other long term plan
> than having an API provided by the platform code in arch/arm/, and used
> by drivers. Some other drivers may have to use this API as well in the
> future.
This is harder to do, but I'm sure we can find a solution. At least
the addr-map.c code has no other dependencies than the plat/addr-map.h
header, so we are fairly free to move it elsehwere.
I can think of several approaches that I'd prefer over your approach,
although I have to admit that none of them makes me really happy:
a) arch/arm/common/mv-addr-map.c and arch/arm/include/asm/mach/mv-addr-map.h
I assume that Russell will object to this one, but I let him speak for
himself
b) drivers/bus/<something>
This would make a lot more sense if we followed the scheme I explained
in my discussion to Jason Gunthorpe, where we basically treat this
bus as a parent node in the device tree for anything that can get remapped.
Without that change, it feels a little misplaced
c) drivers/misc/
I usually object to anything put in here and would certainly prefer the
previous one over this.
> Note that I have been careful to use CFLAGS_pci-mvebu.o, so that those
> include paths only apply to *this* driver. I added a separate dummy
> driver in drivers/pci/host/, and verified that those include paths are
> not used when building this other driver. So those special CFLAGS are
> still compatible with the multiplatform kernel.
I understand this, but I think it would be better not to set a precedent
for doing this.
>
> > > +static void mvebu_pcie_setup_io_window(struct mvebu_pcie_port
> > > *port,
> > > + int enable)
> > > +{
> > > + unsigned long iobase, iolimit;
> > > +
> > > + if (port->bridge.iolimit < port->bridge.iobase)
> > > + return;
> > > +
> > > + iolimit = 0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
> > > + (port->bridge.iolimitupper << 16);
> > > + iobase = ((port->bridge.iobase & 0xF0) << 8) |
> > > + (port->bridge.iobaseupper << 16);
> >
> > I don't understand this code with the masks and shifts. Could you
> > add a comment here for readers like me?
>
> Sure, will do.
>
> It basically comes from the PCI-to-PCI bridge specification, which
> explains how the I/O address and I/O limit is split into two 16 bits
> registers, with those bizarre shifts and hardcoded values. I'll put a
> reference to the relevant section of the PCI-to-PCI bridge
> specification here.
Ok, I see. Thanks for the explanation.
> > > + /* Get the I/O and memory ranges from DT */
> > > + while ((range = of_pci_process_ranges(np, &res, range)) !=
> > > NULL) {
> > > + if (resource_type(&res) == IORESOURCE_IO) {
> > > + memcpy(&pcie->io, &res, sizeof(res));
> > > + memcpy(&pcie->realio, &res, sizeof(res));
> > > + pcie->io.name = "I/O";
> > > + pcie->realio.start &= 0xFFFFF;
> > > + pcie->realio.end &= 0xFFFFF;
> > > + }
> >
> > The bit masking seems fishy here. What exactly are you doing,
> > does this just assume you have a 1MB window at most?
>
> Basically, I have two resources for the I/O:
>
> * One described in the DT, from 0xC0000000 to 0xC00FFFFF which will be
> used to create the address decoding windows for the I/O regions of
> the different PCIe interfaces. The PCI I/O virtual address 0xffe00000
> will be mapped to those physical addresses. Those address decoding
> windows are configured with the special "remap" mechanism that
> ensures that if an access is made at 0xC0000000 + offset, it will
> appear on the PCI bus as an I/O access at address "offset".
Right, I got this from reading the code. Unfortunately the of_pci_process_ranges
functions from your earlier patch makes this a little confusing as it
marks this resource as IORESOURCE_IO when in reality it is the IORESOURCE_MEM
resource that describes where in MMIO space the I/O window is.
> * One covering the low addresses 0x0 -> 0xFFFFF (pcie->realio), which
> is used to tell the Linux PCI subsystem from which address range it
> should assign I/O addresses.
>
> > Maybe something like
> >
> > pcie->realio.start = 0;
> > pcie->realio.end = pcie->io.end - pcie->io.start;
>
> Indeed, that would result in the same values. If you find it clearer,
> I'm fine with it.
Ideally we would read that from the ranges property as well, since it
does not necessarily start at zero, although any other value would
be a bit silly.
I definitely prefer the version I suggested over your version. On second
thought I would make this
pcie->realio.start = PCIBIOS_MIN_IO;
pcie->realio.end = min(pcie->io.end - pcie->io.start, IO_SPACE_LIMIT);
to ensure that we are inside of the limits.
>
> > > +static int mvebu_pcie_init(void)
> > > +{
> > > + return platform_driver_probe(&mvebu_pcie_driver,
> > > + mvebu_pcie_probe);
> > > +}
> > > +
> > > +subsys_initcall(mvebu_pcie_init);
> >
> > You don't have to do it, but I wonder if this could be a module
> > with unload support instead.
>
> This has already been discussed in the review of PATCHv2. Please see
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/145580.html.
Ok.
Arnd
--
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
