| Message ID | 20220706102148.5060-1-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] powerpc/pci: Add config option for using OF 'reg' for PCI domain | expand |
On 06/07/2022 07:21, Pali Rohár wrote: > [...] > Fix this issue and introduce a new option CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG. > When this option is disabled then powerpc kernel would assign PCI domains > in the similar way like it is doing kernel for other architectures, > starting from zero and also how it was done prior that commit. I found this sentence a bit weird, "in the similar way like it is doing kernel for other architectures", but other than that: Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com> Thanks for the improvement! Cheers, Guilherme > > This option is by default enabled for powernv and pseries platform for which > was that commit originally intended. > > With this change upgrading kernels from LTS 4.4 version does not change PCI > domain on smaller embedded platforms with fixed number of PCIe controllers. > And also ensure that PCI domain zero is present as before that commit. > > Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties") > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > Changes in v2: > * Enable CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG by default on powernv and pseries > --- > arch/powerpc/Kconfig | 11 +++++++++++ > arch/powerpc/kernel/pci-common.c | 4 ++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index f66084bc1dfe..053a88e84049 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -386,6 +386,17 @@ config PPC_OF_PLATFORM_PCI > depends on PCI > depends on PPC64 # not supported on 32 bits yet > > +config PPC_PCI_DOMAIN_FROM_OF_REG > + bool "Use OF reg property for PCI domain" > + depends on PCI > + default y if PPC_PSERIES || PPC_POWERNV > + help > + By default PCI domain for host bridge during its registration is > + chosen as the lowest unused PCI domain number. > + > + When this option is enabled then PCI domain can be determined > + also from lower bits of the OF / Device Tree 'reg' property. > + > config ARCH_SUPPORTS_UPROBES > def_bool y > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index 068410cd54a3..7f959df34833 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -74,7 +74,6 @@ void __init set_pci_dma_ops(const struct dma_map_ops *dma_ops) > static int get_phb_number(struct device_node *dn) > { > int ret, phb_id = -1; > - u32 prop_32; > u64 prop; > > /* > @@ -83,7 +82,8 @@ static int get_phb_number(struct device_node *dn) > * reading "ibm,opal-phbid", only present in OPAL environment. > */ > ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop); > - if (ret) { > + if (ret && IS_ENABLED(CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG)) { > + u32 prop_32; > ret = of_property_read_u32_index(dn, "reg", 1, &prop_32); > prop = prop_32; > }
On Friday 15 July 2022 11:55:04 Guilherme G. Piccoli wrote: > On 06/07/2022 07:21, Pali Rohár wrote: > > [...] > > Fix this issue and introduce a new option CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG. > > When this option is disabled then powerpc kernel would assign PCI domains > > in the similar way like it is doing kernel for other architectures, > > starting from zero and also how it was done prior that commit. > > I found this sentence a bit weird, "in the similar way like it is doing > kernel for other architectures", but other than that: If you have some idea how to improve commit description, let me know and I can change it. > Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > Thanks for the improvement! > Cheers, > > > Guilherme > > > > > > This option is by default enabled for powernv and pseries platform for which > > was that commit originally intended. > > > > With this change upgrading kernels from LTS 4.4 version does not change PCI > > domain on smaller embedded platforms with fixed number of PCIe controllers. > > And also ensure that PCI domain zero is present as before that commit. > > > > Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties") > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > Changes in v2: > > * Enable CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG by default on powernv and pseries > > --- > > arch/powerpc/Kconfig | 11 +++++++++++ > > arch/powerpc/kernel/pci-common.c | 4 ++-- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > index f66084bc1dfe..053a88e84049 100644 > > --- a/arch/powerpc/Kconfig > > +++ b/arch/powerpc/Kconfig > > @@ -386,6 +386,17 @@ config PPC_OF_PLATFORM_PCI > > depends on PCI > > depends on PPC64 # not supported on 32 bits yet > > > > +config PPC_PCI_DOMAIN_FROM_OF_REG > > + bool "Use OF reg property for PCI domain" > > + depends on PCI > > + default y if PPC_PSERIES || PPC_POWERNV > > + help > > + By default PCI domain for host bridge during its registration is > > + chosen as the lowest unused PCI domain number. > > + > > + When this option is enabled then PCI domain can be determined > > + also from lower bits of the OF / Device Tree 'reg' property. > > + > > config ARCH_SUPPORTS_UPROBES > > def_bool y > > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > > index 068410cd54a3..7f959df34833 100644 > > --- a/arch/powerpc/kernel/pci-common.c > > +++ b/arch/powerpc/kernel/pci-common.c > > @@ -74,7 +74,6 @@ void __init set_pci_dma_ops(const struct dma_map_ops *dma_ops) > > static int get_phb_number(struct device_node *dn) > > { > > int ret, phb_id = -1; > > - u32 prop_32; > > u64 prop; > > > > /* > > @@ -83,7 +82,8 @@ static int get_phb_number(struct device_node *dn) > > * reading "ibm,opal-phbid", only present in OPAL environment. > > */ > > ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop); > > - if (ret) { > > + if (ret && IS_ENABLED(CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG)) { > > + u32 prop_32; > > ret = of_property_read_u32_index(dn, "reg", 1, &prop_32); > > prop = prop_32; > > }
On 15/07/2022 14:11, Pali Rohár wrote: > [...] >> >> I found this sentence a bit weird, "in the similar way like it is doing >> kernel for other architectures", but other than that: > > If you have some idea how to improve commit description, let me know and > I can change it. > Oh, for example: "in similar way the kernel is doing for other architectures". The sentence idea is perfectly fine, it's just that it's a bit oddly constructed IMHO heh Cheers!
On Friday 15 July 2022 15:32:56 Guilherme G. Piccoli wrote: > On 15/07/2022 14:11, Pali Rohár wrote: > > [...] > >> > >> I found this sentence a bit weird, "in the similar way like it is doing > >> kernel for other architectures", but other than that: > > > > If you have some idea how to improve commit description, let me know and > > I can change it. > > > > Oh, for example: "in similar way the kernel is doing for other > architectures". The sentence idea is perfectly fine, it's just that it's > a bit oddly constructed IMHO heh > > Cheers! Ou, sorry. English is not my first nor second language. Michael, should I resend this patch series with Guilherme's suggestion for changing working in commit description? Or will you adjust it manually?
Pali Rohár <pali@kernel.org> writes: > Since commit 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on > device-tree properties"), powerpc kernel always fallback to PCI domain > assignment from OF / Device Tree 'reg' property of the PCI controller. > > In most cases 'reg' property is not zero and therefore there it cause that > PCI domain zero is not present in system anymore. > > PCI code for other Linux architectures use increasing assignment of the PCI > domain for individual controllers (assign the first free number), like it > was also for powerpc prior mentioned commit. Also it starts numbering > domains from zero. > > Upgrading powerpc kernels from LTS 4.4 version (which does not contain > mentioned commit) to new LTS versions brings a change in domain assignment. > > It can be problematic for embedded machines with single PCIe controller > where it is expected that PCIe card is connected on the domain zero. > Also it can be problematic as that commit changes PCIe domains in > multi-controller setup with fixed number of controller (without hotplug > support). > > Originally that change was intended for powernv and pservers and specially > server machines with more PCI domains or hot plug support. > > Fix this issue and introduce a new option CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG. As I said in my previous reply, I don't want a config option for this. Adding an option now would revert the behaviour back to the way it was, which has the potential to break things, as you described. Maybe we shouldn't have changed the numbering to begin with, but it's been 6 years, so it's too late to change it back. cheers
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index f66084bc1dfe..053a88e84049 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -386,6 +386,17 @@ config PPC_OF_PLATFORM_PCI depends on PCI depends on PPC64 # not supported on 32 bits yet +config PPC_PCI_DOMAIN_FROM_OF_REG + bool "Use OF reg property for PCI domain" + depends on PCI + default y if PPC_PSERIES || PPC_POWERNV + help + By default PCI domain for host bridge during its registration is + chosen as the lowest unused PCI domain number. + + When this option is enabled then PCI domain can be determined + also from lower bits of the OF / Device Tree 'reg' property. + config ARCH_SUPPORTS_UPROBES def_bool y diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 068410cd54a3..7f959df34833 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -74,7 +74,6 @@ void __init set_pci_dma_ops(const struct dma_map_ops *dma_ops) static int get_phb_number(struct device_node *dn) { int ret, phb_id = -1; - u32 prop_32; u64 prop; /* @@ -83,7 +82,8 @@ static int get_phb_number(struct device_node *dn) * reading "ibm,opal-phbid", only present in OPAL environment. */ ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop); - if (ret) { + if (ret && IS_ENABLED(CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG)) { + u32 prop_32; ret = of_property_read_u32_index(dn, "reg", 1, &prop_32); prop = prop_32; }
Since commit 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties"), powerpc kernel always fallback to PCI domain assignment from OF / Device Tree 'reg' property of the PCI controller. In most cases 'reg' property is not zero and therefore there it cause that PCI domain zero is not present in system anymore. PCI code for other Linux architectures use increasing assignment of the PCI domain for individual controllers (assign the first free number), like it was also for powerpc prior mentioned commit. Also it starts numbering domains from zero. Upgrading powerpc kernels from LTS 4.4 version (which does not contain mentioned commit) to new LTS versions brings a change in domain assignment. It can be problematic for embedded machines with single PCIe controller where it is expected that PCIe card is connected on the domain zero. Also it can be problematic as that commit changes PCIe domains in multi-controller setup with fixed number of controller (without hotplug support). Originally that change was intended for powernv and pservers and specially server machines with more PCI domains or hot plug support. Fix this issue and introduce a new option CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG. When this option is disabled then powerpc kernel would assign PCI domains in the similar way like it is doing kernel for other architectures, starting from zero and also how it was done prior that commit. This option is by default enabled for powernv and pseries platform for which was that commit originally intended. With this change upgrading kernels from LTS 4.4 version does not change PCI domain on smaller embedded platforms with fixed number of PCIe controllers. And also ensure that PCI domain zero is present as before that commit. Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties") Signed-off-by: Pali Rohár <pali@kernel.org> --- Changes in v2: * Enable CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG by default on powernv and pseries --- arch/powerpc/Kconfig | 11 +++++++++++ arch/powerpc/kernel/pci-common.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-)