[PATCH v7 3/6] PCI: designware: Add ARM64 support

Gabriele Paoloni gabriele.paoloni at huawei.com
Mon Aug 24 01:28:38 PDT 2015 


> -----Original Message-----
> From: Wangzhou (B)
> Sent: Monday, August 24, 2015 7:26 AM
> To: Gabriele Paoloni
> Cc: Liviu Dudau; Lucas Stach; 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 2015/8/21 22:19, Gabriele Paoloni wrote:
> > 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 :)
> >
> 
> How about we pass pp->busn->start to pci_create_root_bus as root bus
> number?
> We get pp->busn->start from resource list(res) whose elements come from
> of_pci_get_host_bridge_resources.
> 
> If there is a bus-range item in dts, root bus number will get from it.
> Oppositely, root bus number will be default value(0x0).
> 
> Thanks,
> Zhou

Sounds good to me. So we can remove root_bus_nr

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