> -----Original Message----- > From: Wangzhou (B) > Sent: Monday, August 24, 2015 7:26 AM > To: Gabriele Paoloni > Cc: Liviu Dudau; Lucas Stach; 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 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 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@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
