On 2015/8/21 22:19, Gabriele Paoloni wrote: > Hi Liviu > > Many Thanks for reviewing > >> -----Original Message----- >> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Liviu Dudau >> Sent: Friday, August 21, 2015 2:43 PM >> To: Gabriele Paoloni >> Cc: Lucas Stach; Wangzhou (B); Bjorn Helgaas; jingoohan1@xxxxxxxxx; >> Pratyush Anand; Arnd Bergmann; linux@xxxxxxxxxxxxxxxx; >> thomas.petazzoni@xxxxxxxxxxxxxxxxxx; Lorenzo Pieralisi; James Morse; >> jason@xxxxxxxxxxxxxx; robh@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; >> 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@xxxxxxxxxxxxxx] >>>> Sent: Thursday, August 20, 2015 9:27 AM >>>> To: Gabriele Paoloni >>>> Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1@xxxxxxxxx; Pratyush >> Anand; >>>> Arnd Bergmann; linux@xxxxxxxxxxxxxxxx; thomas.petazzoni@free- >>>> electrons.com; lorenzo.pieralisi@xxxxxxx; james.morse@xxxxxxx; >>>> Liviu.Dudau@xxxxxxx; jason@xxxxxxxxxxxxxx; robh@xxxxxxxxxx; linux- >>>> pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; >>>> devicetree@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxxx] >>>>>> Sent: Wednesday, August 19, 2015 4:37 PM >>>>>> To: Gabriele Paoloni >>>>>> Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1@xxxxxxxxx; Pratyush >>>> Anand; >>>>>> Arnd Bergmann; linux@xxxxxxxxxxxxxxxx; thomas.petazzoni@free- >>>>>> electrons.com; lorenzo.pieralisi@xxxxxxx; james.morse@xxxxxxx; >>>>>> Liviu.Dudau@xxxxxxx; jason@xxxxxxxxxxxxxx; robh@xxxxxxxxxx; >> linux- >>>>>> pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; >>>>>> devicetree@xxxxxxxxxxxxxxx; 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 >> >>> 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@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
