[PATCH v7 3/6] PCI: designware: Add ARM64 support
Gabriele Paoloni
gabriele.paoloni at huawei.com
Fri Aug 21 07:19:32 PDT 2015
Hi Liviu
Many Thanks for reviewing
> -----Original Message-----
> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
> owner at vger.kernel.org] On Behalf Of Liviu Dudau
> Sent: Friday, August 21, 2015 2:43 PM
> To: Gabriele Paoloni
> Cc: Lucas Stach; Wangzhou (B); Bjorn Helgaas; jingoohan1 at gmail.com;
> Pratyush Anand; Arnd Bergmann; linux at arm.linux.org.uk;
> thomas.petazzoni at free-electrons.com; Lorenzo Pieralisi; James Morse;
> jason at lakedaemon.net; robh at kernel.org; linux-pci at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org;
> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth)
> Subject: Re: [PATCH v7 3/6] PCI: designware: Add ARM64 support
>
> On Thu, Aug 20, 2015 at 12:10:24PM +0100, Gabriele Paoloni wrote:
> > Hi Lucas
> >
> > Again many thanks for explaining and for your time.
> >
> > I got your point now and I have dug a bit better in the PCI_DOMAINS
> code.
> >
> > However I have a question...see inline below
> >
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach at pengutronix.de]
> > > Sent: Thursday, August 20, 2015 9:27 AM
> > > To: Gabriele Paoloni
> > > Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1 at gmail.com; Pratyush
> Anand;
> > > Arnd Bergmann; linux at arm.linux.org.uk; thomas.petazzoni at free-
> > > electrons.com; lorenzo.pieralisi at arm.com; james.morse at arm.com;
> > > Liviu.Dudau at arm.com; jason at lakedaemon.net; robh at kernel.org; linux-
> > > pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > > devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
> > > qiuzhenfa; liudongdong (C); qiujiang; xuwei (O); Liguozhu (Kenneth)
> > > Subject: Re: [PATCH v7 3/6] PCI: designware: Add ARM64 support
> > >
> > > Hi Gab,
> > >
> > > Am Mittwoch, den 19.08.2015, 16:34 +0000 schrieb Gabriele Paoloni:
> > > > Hi Lucas
> > > >
> > > > First of all many thanks for the quick reply, really appreciated
> > > >
> > > > > -----Original Message-----
> > > > > From: Lucas Stach [mailto:l.stach at pengutronix.de]
> > > > > Sent: Wednesday, August 19, 2015 4:37 PM
> > > > > To: Gabriele Paoloni
> > > > > Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1 at gmail.com; Pratyush
> > > Anand;
> > > > > Arnd Bergmann; linux at arm.linux.org.uk; thomas.petazzoni at free-
> > > > > electrons.com; lorenzo.pieralisi at arm.com; james.morse at arm.com;
> > > > > Liviu.Dudau at arm.com; jason at lakedaemon.net; robh at kernel.org;
> linux-
> > > > > pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > > > > devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
> > > > > qiuzhenfa; liudongdong (C); qiujiang; xuwei (O); Liguozhu
> (Kenneth)
> > > > > Subject: Re: [PATCH v7 3/6] PCI: designware: Add ARM64 support
> > > > >
> > > > > Hi Gab,
> > > > >
> > > > > Am Mittwoch, den 19.08.2015, 15:16 +0000 schrieb Gabriele
> Paoloni:
> > > > > > Hi Lucas
> > > > > >
> > > > > > I have rewritten the patch to take into account multiple
> > > controllers.
> > > > > >
> > > > > > As you can see now there is a static var in
> dw_pcie_host_init()
> > > that
> > > > > tracks
> > > > > > the bus numbers used.
> > > > >
> > > > > This is wrong. The DT specifies the valid bus number range. You
> can
> > > not
> > > > > just assign the next free bus number to the root bus.
> > > >
> > > > I think this is what is being done in
> > > > http://lxr.free-
> electrons.com/source/arch/arm/kernel/bios32.c#L495
> > > > and currently designware assigns the root bus number in
> > > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie-
> > > designware.c#L730
> > > >
> > > You found the right spot. It works a bit different than I remember,
> but
> > > is less broken than your current code. :)
> > >
> > > It actually assigns the next instance a root bus number behind the
> > > current instances bus range. Please note the difference to your
> code
> > > which assigns the next free bus number, which may still lay within
> the
> > > current instances bus range.
>
> Hi Gabriele,
>
> >
> > Mmmm here I have done a mistake: in the current designware the number
> of hw
> > controllers is always 1
> > http://lxr.free-electrons.com/source/drivers/pci/host/pcie-
> designware.c#L510
> > So the loop in bios32.c does not work on multiple PCIe ports...
>
> Bios32.c is broken for multiple PCIe hosts for multiple reasons, this
> is just
> one of them. Hence the effort to get rid of it for maintained drivers.
As you can see we're pushing hard for this :)
>
> > However your comment about PCI_DOMAINS enlightened my mind and now I
> see
> > that when CONFIG_PCI_DOMAINS is defined we have the domains numbers
> > (tracked by __domain_nr).
>
> CONFIG_PCI_DOMAINS in itself doesn't do much without adding code to
> your driver.
> You could request a new domain for each controller with a special
> property to
> disable that behaviour in the dts, or you could enable
> CONFIG_PCI_DOMAINS_GENERIC
> which will give you a new domain automatically.
So far I see that for ARM and ARM64 CONFIG_PCI_DOMAINS_GENERIC defaults to
the same value as CONFIG_PCI_DOMAINS (see arch/arm/Kconfig, arch/arm64/Kconfig).
So I think this is how current designware based drivers get multiple PCIe
controller to work (they just enable CONFIG_PCI_DOMAINS)...
>
> > So (correct me if I am wrong) if we have 2 PCIe ports that do not
> specify
> > the "bus-range" property, both root ports will enumerate starting
> from
> > root_bus_nr = 0 and everything will still work ok.
>
> Correct.
>
> Best regards,
> Liviu
>
> >
> > So if my assumption is correct, I do not see why the orginal patch
> from Wang
> > Zhou is wrong.
> > The only point can be that assigning pp->root_bus_nr = 0 is not
> required
> > as pp memory is kzalloc'ed (an therefore init to zero).
> >
> >
> > >
> > > >
> > > > In general I agree with you but if you look at all the current
> > > drivers
> > > > based on designware none of them define the "bus-range" dtb
> property.
> > > > Therefore doing as you say would break the current driver when we
> > > have
> > > > multiple controllers...am I right?
> > > >
> > > The current _code_ works fine for multiple controllers. It's just
> that
> > > you must define the bus range property in the DTB if you want to
> enable
> > > multiple instances of one controller and support a kernel without
> PCI
> > > domains support. As all current implementations have only a single
> > > instance of the controller per SoC, or depend on PCI domain support,
> > > it's totally fine for them to not define the bus ranges property,
> as
> > > it's an optional property and it is well defined how things behave
> if
> > > it
> > > is absent. We absolutely need to keep that specified behavior.
> > >
> > > > If that is the case in order to fix this in the way you say I
> would
> > > need
> > > > to assign "bus-range" for all the PCIe drivers with multiple
> > > controllers:
> > > > in this case I would split the default range evenly (that is, if
> we
> > > have
> > > > two controllers I would define "bus-range" 0-127 and 128-255)
> > > >
> > > > If you think this solution is ok I can go for it. My only doubt
> was
> > > about
> > > > touching other vendors DTBs....
> > > >
> > > You don't need to touch any of the current DTBs, as they are fine
> with
> > > the default bus range behavior. You need to keep the specified
> behavior
> > > of the bus range property with the new code.
> > >
> > > Regards,
> > > Lucas
> > > >
> > > > >
> > > > > It is perfectly valid to have a bus range of 0x00-0x10 assigned
> to
> > > one
> > > > > instance and 0x50-0xff to the next instance. Additional with
> PCIe
> > > > > hotplug you may not use the full range of the bus numbers on
> one
> > > > > instance at the first scan, but only later populate more buses
> when
> > > > > more
> > > > > bridges are added to the tree.
> > > > >
> > > > > > Drivers that do not specify the bus range in the DTB set pp-
> > > > > >root_bus_nr = DW_ROOT_NR_UNDEFINED.
> > > > > > Designware will check if the flag is set and will use the
> > > automatic
> > > > > bus range
> > > > > > assignment.
> > > > >
> > > > > No, please lets get rid of this assignment altogether. The glue
> > > drivers
> > > > > have no business in assigning the bus range. Please remove the
> > > > > pp->root_bus_nr assignment from all the glue drivers.
> > > > >
> > > > > "bus range" is a generic DW PCIe property, so just parse the
> root
> > > bus
> > > > > number from the DT, it is handled the same way for all the DW
> based
> > > > > PCIe
> > > > > drivers. The bindings specifies that if the bus range property
> is
> > > > > missing the range is 0x00-0xff, so you can default to 0 as the
> root
> > > bus
> > > > > number in that case.
> > > > >
> > > > > Also I would think this conversion warrants a patch on its own
> and
> > > > > should not be mixed in the ARM64 support patch.
> > > > >
> > > > > Regards,
> > > > > Lucas
> > > > >
> > >
> > > --
> > > Pengutronix e.K. | Lucas Stach |
> > > Industrial Linux Solutions | http://www.pengutronix.de/ |
> >
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the linux-arm-kernel
mailing list