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