[PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Pali Rohár]

Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
function and allow to build it as module") PCIe controller driver for
Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
allows dynamic binding and unbinding of PCIe controller device.

Kernel PCI subsystem assigns by default dynamically allocated PCI domain
number (starting from zero) for this PCIe controller every time when device
is bound. So PCI domain changes after every unbind / bind operation.

Alternative way for assigning PCI domain number is to use static allocated
numbers defined in Device Tree. This option has requirement that every PCI
controller in system must have defined PCI bus number in Device Tree.

Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in
Device Tree.

After this change PCI domain on Armada 37xx is always zero, even after
repeated unbind and bind operations.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module")
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 7a2df148c6a3..f02058ef5364 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -495,6 +495,7 @@
 					<0 0 0 2 &pcie_intc 1>,
 					<0 0 0 3 &pcie_intc 2>,
 					<0 0 0 4 &pcie_intc 3>;
+			linux,pci-domain = <0>;
 			max-link-speed = <2>;
 			phys = <&comphy1 0>;
 			pcie_intc: interrupt-controller {
-- 
2.20.1

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Rob Herring]

On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
>
> Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> function and allow to build it as module") PCIe controller driver for
> Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> allows dynamic binding and unbinding of PCIe controller device.
>
> Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> number (starting from zero) for this PCIe controller every time when device
> is bound. So PCI domain changes after every unbind / bind operation.

PCI host bridges as a module are relatively new, so seems likely a bug to me.

> Alternative way for assigning PCI domain number is to use static allocated
> numbers defined in Device Tree. This option has requirement that every PCI
> controller in system must have defined PCI bus number in Device Tree.

That seems entirely pointless from a DT point of view with a single PCI bridge.

> Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in
> Device Tree.
>
> After this change PCI domain on Armada 37xx is always zero, even after
> repeated unbind and bind operations.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module")
> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index 7a2df148c6a3..f02058ef5364 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -495,6 +495,7 @@
>                                         <0 0 0 2 &pcie_intc 1>,
>                                         <0 0 0 3 &pcie_intc 2>,
>                                         <0 0 0 4 &pcie_intc 3>;
> +                       linux,pci-domain = <0>;
>                         max-link-speed = <2>;
>                         phys = <&comphy1 0>;
>                         pcie_intc: interrupt-controller {
> --
> 2.20.1
>

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Pali Rohár]

On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > function and allow to build it as module") PCIe controller driver for
> > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > allows dynamic binding and unbinding of PCIe controller device.
> >
> > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > number (starting from zero) for this PCIe controller every time when device
> > is bound. So PCI domain changes after every unbind / bind operation.
> 
> PCI host bridges as a module are relatively new, so seems likely a bug to me.

Why a bug? It is there since 5.10 and it is working.

> > Alternative way for assigning PCI domain number is to use static allocated
> > numbers defined in Device Tree. This option has requirement that every PCI
> > controller in system must have defined PCI bus number in Device Tree.
> 
> That seems entirely pointless from a DT point of view with a single PCI bridge.

If domain id is not specified in DT then kernel uses counter and assigns
counter++. So it is not pointless if we want to have stable domain id.

> > Armada 37xx has only one PCIe controller, so assign for it PCI domain 0 in
> > Device Tree.
> >
> > After this change PCI domain on Armada 37xx is always zero, even after
> > repeated unbind and bind operations.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Fixes: 526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module")
> > ---
> >  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > index 7a2df148c6a3..f02058ef5364 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > @@ -495,6 +495,7 @@
> >                                         <0 0 0 2 &pcie_intc 1>,
> >                                         <0 0 0 3 &pcie_intc 2>,
> >                                         <0 0 0 4 &pcie_intc 3>;
> > +                       linux,pci-domain = <0>;
> >                         max-link-speed = <2>;
> >                         phys = <&comphy1 0>;
> >                         pcie_intc: interrupt-controller {
> > --
> > 2.20.1
> >

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Marek Behun]

On Thu, 15 Apr 2021 10:36:40 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:  
> > >
> > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > function and allow to build it as module") PCIe controller driver for
> > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > allows dynamic binding and unbinding of PCIe controller device.
> > >
> > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > number (starting from zero) for this PCIe controller every time when device
> > > is bound. So PCI domain changes after every unbind / bind operation.  
> > 
> > PCI host bridges as a module are relatively new, so seems likely a bug to me.  
> 
> Why a bug? It is there since 5.10 and it is working.
> 
> > > Alternative way for assigning PCI domain number is to use static allocated
> > > numbers defined in Device Tree. This option has requirement that every PCI
> > > controller in system must have defined PCI bus number in Device Tree.  
> > 
> > That seems entirely pointless from a DT point of view with a single PCI bridge.  
> 
> If domain id is not specified in DT then kernel uses counter and assigns
> counter++. So it is not pointless if we want to have stable domain id.

What Rob is trying to say is that
- the bug is that kernel assigns counter++
- device-tree should not be used to fix problems with how kernel does
  things
- if a device has only one PCIe controller, it is pointless to define
  it's pci-domain. If there were multiple controllers, then it would
  make sense, but there is only one

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Rob Herring]

On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Thu, 15 Apr 2021 10:36:40 +0200
> Pali Rohár <pali@kernel.org> wrote:
>
> > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > > function and allow to build it as module") PCIe controller driver for
> > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > > allows dynamic binding and unbinding of PCIe controller device.
> > > >
> > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > > number (starting from zero) for this PCIe controller every time when device
> > > > is bound. So PCI domain changes after every unbind / bind operation.
> > >
> > > PCI host bridges as a module are relatively new, so seems likely a bug to me.
> >
> > Why a bug? It is there since 5.10 and it is working.

I mean historically, the PCI subsystem didn't even support host
bridges as a module. They weren't even proper drivers and it was all
arch specific code. Most of the host bridge drivers are still built-in
only. This seems like a small detail that was easily overlooked.
unbind is not a well tested path.

> > > > Alternative way for assigning PCI domain number is to use static allocated
> > > > numbers defined in Device Tree. This option has requirement that every PCI
> > > > controller in system must have defined PCI bus number in Device Tree.
> > >
> > > That seems entirely pointless from a DT point of view with a single PCI bridge.
> >
> > If domain id is not specified in DT then kernel uses counter and assigns
> > counter++. So it is not pointless if we want to have stable domain id.
>
> What Rob is trying to say is that
> - the bug is that kernel assigns counter++
> - device-tree should not be used to fix problems with how kernel does
>   things
> - if a device has only one PCIe controller, it is pointless to define
>   it's pci-domain. If there were multiple controllers, then it would
>   make sense, but there is only one

Yes. I think what we want here is a domain bitmap rather than a
counter and we assign the lowest free bit. That could also allow for
handling a mixture of fixed domain numbers and dynamically assigned
ones.

You could create scenarios where the numbers change on you, but it
wouldn't be any different than say plugging in USB serial adapters.
You get the same ttyUSBx device when you re-attach unless there's been
other ttyUSBx devices attached/detached.

Rob

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Pali Rohár]

On Thursday 15 April 2021 10:13:17 Rob Herring wrote:
> On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Thu, 15 Apr 2021 10:36:40 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >
> > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > > > function and allow to build it as module") PCIe controller driver for
> > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > > > allows dynamic binding and unbinding of PCIe controller device.
> > > > >
> > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > > > number (starting from zero) for this PCIe controller every time when device
> > > > > is bound. So PCI domain changes after every unbind / bind operation.
> > > >
> > > > PCI host bridges as a module are relatively new, so seems likely a bug to me.
> > >
> > > Why a bug? It is there since 5.10 and it is working.
> 
> I mean historically, the PCI subsystem didn't even support host
> bridges as a module. They weren't even proper drivers and it was all
> arch specific code. Most of the host bridge drivers are still built-in
> only. This seems like a small detail that was easily overlooked.
> unbind is not a well tested path.

Ok! Just to note that during my testing I have not spotted any issue.

> > > > > Alternative way for assigning PCI domain number is to use static allocated
> > > > > numbers defined in Device Tree. This option has requirement that every PCI
> > > > > controller in system must have defined PCI bus number in Device Tree.
> > > >
> > > > That seems entirely pointless from a DT point of view with a single PCI bridge.
> > >
> > > If domain id is not specified in DT then kernel uses counter and assigns
> > > counter++. So it is not pointless if we want to have stable domain id.
> >
> > What Rob is trying to say is that
> > - the bug is that kernel assigns counter++
> > - device-tree should not be used to fix problems with how kernel does
> >   things
> > - if a device has only one PCIe controller, it is pointless to define
> >   it's pci-domain. If there were multiple controllers, then it would
> >   make sense, but there is only one
> 
> Yes. I think what we want here is a domain bitmap rather than a
> counter and we assign the lowest free bit. That could also allow for
> handling a mixture of fixed domain numbers and dynamically assigned
> ones.

Currently this code is implemented in pci_bus_find_domain_nr() function.
IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB
of memory. I'm not sure if it is fine or some other tree-based structure
for allocated domain numbers is needed.

> You could create scenarios where the numbers change on you, but it
> wouldn't be any different than say plugging in USB serial adapters.
> You get the same ttyUSBx device when you re-attach unless there's been
> other ttyUSBx devices attached/detached.

This should be fine for most scenarios. Dynamically attaching /
detaching PCI domain is not such common action...

Will you implement this new feature?

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Andrew Lunn]

> Currently this code is implemented in pci_bus_find_domain_nr() function.
> IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB
> of memory. I'm not sure if it is fine or some other tree-based structure
> for allocated domain numbers is needed.

Hi Pali

Have a look at lib/idr.c

     Andrew

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Pali Rohár]

On Saturday 17 April 2021 17:19:38 Andrew Lunn wrote:
> > Currently this code is implemented in pci_bus_find_domain_nr() function.
> > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB
> > of memory. I'm not sure if it is fine or some other tree-based structure
> > for allocated domain numbers is needed.
> 
> Hi Pali
> 
> Have a look at lib/idr.c
> 
>      Andrew

Great! So number allocation is already implemented in kernel (via radix trees).

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Rob Herring]

On Sat, Apr 17, 2021 at 9:49 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 15 April 2021 10:13:17 Rob Herring wrote:
> > On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Thu, 15 Apr 2021 10:36:40 +0200
> > > Pali Rohár <pali@kernel.org> wrote:
> > >
> > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > > > > function and allow to build it as module") PCIe controller driver for
> > > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > > > > allows dynamic binding and unbinding of PCIe controller device.
> > > > > >
> > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > > > > number (starting from zero) for this PCIe controller every time when device
> > > > > > is bound. So PCI domain changes after every unbind / bind operation.
> > > > >
> > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me.
> > > >
> > > > Why a bug? It is there since 5.10 and it is working.
> >
> > I mean historically, the PCI subsystem didn't even support host
> > bridges as a module. They weren't even proper drivers and it was all
> > arch specific code. Most of the host bridge drivers are still built-in
> > only. This seems like a small detail that was easily overlooked.
> > unbind is not a well tested path.
>
> Ok! Just to note that during my testing I have not spotted any issue.
>
> > > > > > Alternative way for assigning PCI domain number is to use static allocated
> > > > > > numbers defined in Device Tree. This option has requirement that every PCI
> > > > > > controller in system must have defined PCI bus number in Device Tree.
> > > > >
> > > > > That seems entirely pointless from a DT point of view with a single PCI bridge.
> > > >
> > > > If domain id is not specified in DT then kernel uses counter and assigns
> > > > counter++. So it is not pointless if we want to have stable domain id.
> > >
> > > What Rob is trying to say is that
> > > - the bug is that kernel assigns counter++
> > > - device-tree should not be used to fix problems with how kernel does
> > >   things
> > > - if a device has only one PCIe controller, it is pointless to define
> > >   it's pci-domain. If there were multiple controllers, then it would
> > >   make sense, but there is only one
> >
> > Yes. I think what we want here is a domain bitmap rather than a
> > counter and we assign the lowest free bit. That could also allow for
> > handling a mixture of fixed domain numbers and dynamically assigned
> > ones.
>
> Currently this code is implemented in pci_bus_find_domain_nr() function.
> IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB
> of memory. I'm not sure if it is fine or some other tree-based structure
> for allocated domain numbers is needed.

It's an atomic_t but then shortened (potentially) to an 'int'. Surely
we don't need 8k (or 2^31) host bridges? Seems like we could start
with 64 and bump it as needed. Or the idr route is another option if
that works. We'd need to get the lowest free value and be able to
reserve values (when specified by firmware).

> > You could create scenarios where the numbers change on you, but it
> > wouldn't be any different than say plugging in USB serial adapters.
> > You get the same ttyUSBx device when you re-attach unless there's been
> > other ttyUSBx devices attached/detached.
>
> This should be fine for most scenarios. Dynamically attaching /
> detaching PCI domain is not such common action...
>
> Will you implement this new feature?

Yes, after I find a DT binding co-maintainer.

Rob

Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero

[Pali Rohár]

On Friday 23 April 2021 10:33:38 Rob Herring wrote:
> On Sat, Apr 17, 2021 at 9:49 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 15 April 2021 10:13:17 Rob Herring wrote:
> > > On Thu, Apr 15, 2021 at 3:45 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > >
> > > > On Thu, 15 Apr 2021 10:36:40 +0200
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > > On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > > > > > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > > > > > function and allow to build it as module") PCIe controller driver for
> > > > > > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > > > > > allows dynamic binding and unbinding of PCIe controller device.
> > > > > > >
> > > > > > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > > > > > number (starting from zero) for this PCIe controller every time when device
> > > > > > > is bound. So PCI domain changes after every unbind / bind operation.
> > > > > >
> > > > > > PCI host bridges as a module are relatively new, so seems likely a bug to me.
> > > > >
> > > > > Why a bug? It is there since 5.10 and it is working.
> > >
> > > I mean historically, the PCI subsystem didn't even support host
> > > bridges as a module. They weren't even proper drivers and it was all
> > > arch specific code. Most of the host bridge drivers are still built-in
> > > only. This seems like a small detail that was easily overlooked.
> > > unbind is not a well tested path.
> >
> > Ok! Just to note that during my testing I have not spotted any issue.
> >
> > > > > > > Alternative way for assigning PCI domain number is to use static allocated
> > > > > > > numbers defined in Device Tree. This option has requirement that every PCI
> > > > > > > controller in system must have defined PCI bus number in Device Tree.
> > > > > >
> > > > > > That seems entirely pointless from a DT point of view with a single PCI bridge.
> > > > >
> > > > > If domain id is not specified in DT then kernel uses counter and assigns
> > > > > counter++. So it is not pointless if we want to have stable domain id.
> > > >
> > > > What Rob is trying to say is that
> > > > - the bug is that kernel assigns counter++
> > > > - device-tree should not be used to fix problems with how kernel does
> > > >   things
> > > > - if a device has only one PCIe controller, it is pointless to define
> > > >   it's pci-domain. If there were multiple controllers, then it would
> > > >   make sense, but there is only one
> > >
> > > Yes. I think what we want here is a domain bitmap rather than a
> > > counter and we assign the lowest free bit. That could also allow for
> > > handling a mixture of fixed domain numbers and dynamically assigned
> > > ones.
> >
> > Currently this code is implemented in pci_bus_find_domain_nr() function.
> > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB
> > of memory. I'm not sure if it is fine or some other tree-based structure
> > for allocated domain numbers is needed.
> 
> It's an atomic_t but then shortened (potentially) to an 'int'. Surely
> we don't need 8k (or 2^31) host bridges? Seems like we could start
> with 64 and bump it as needed. Or the idr route is another option if
> that works. We'd need to get the lowest free value and be able to
> reserve values (when specified by firmware).

Seems that idr_alloc() supports both required operations as you can ask
idr_alloc() for allocating specific id (if is available).

> > > You could create scenarios where the numbers change on you, but it
> > > wouldn't be any different than say plugging in USB serial adapters.
> > > You get the same ttyUSBx device when you re-attach unless there's been
> > > other ttyUSBx devices attached/detached.
> >
> > This should be fine for most scenarios. Dynamically attaching /
> > detaching PCI domain is not such common action...
> >
> > Will you implement this new feature?
> 
> Yes, after I find a DT binding co-maintainer.
> 
> Rob