| Message ID | 20240122173514.935742-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: marvell,prestera: Fix example PCI bus addressing | expand |
> -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Monday, January 22, 2024 7:35 PM > To: David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Miquel Raynal <miquel.raynal@bootlin.com> > Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: [EXT] [PATCH net-next] net: marvell,prestera: Fix example PCI bus > addressing > > External Email > > ---------------------------------------------------------------------- > The example for PCI devices has some addressing errors. 'reg' is written as if > the parent bus is PCI, but the default bus for examples is 1 address and size > cell. 'ranges' is defining config space with a size of 0. Generally, config space > should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these > issues by updating the values with made-up, but valid values. > > This was uncovered with recent dtschema changes. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > index 5ea8b73663a5..16ff892f7bbd 100644 > --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > @@ -78,8 +78,8 @@ examples: > pcie@0 { > #address-cells = <3>; > #size-cells = <2>; > - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; > - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; > + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; > + reg = <0x0 0x1000>; > device_type = "pci"; > > switch@0,0 { > -- > 2.43.0 > This yaml has a mix-up of device P/N (belonging to AC3, BC2) and PCIe IDs (belonging to AC3X, Aldrin2) Looks like a part of the yaml was updated, and another part was not There is a reference here of actual usage of prestera switch device: https://github.com/dentproject/linux/blob/dent-linux-5.15.y/arch/arm64/boot/dts/marvell/accton-as4564-26p.dts So actual ranges and reg could be used instead of made up ones. But the actual real life dts places the prestera at the top level of the dts, not under pci. I am not aware of any dts/dtsi using such kind of switch node under pcie node, similar to the example given in the yaml file, and did not manage to find any under latest linux-next for both arm and arm64 dts directories (please correct me here if I am wrong). So the question here is if this pci example really necessary for the prestera device, or can be removed altogether (which is what I think is best to do). Elad.
On Tue, Jan 23, 2024 at 09:59:46AM +0000, Elad Nachman wrote: > > > > -----Original Message----- > > From: Rob Herring <robh@kernel.org> > > Sent: Monday, January 22, 2024 7:35 PM > > To: David S. Miller <davem@davemloft.net>; Eric Dumazet > > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > > <pabeni@redhat.com>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Miquel Raynal <miquel.raynal@bootlin.com> > > Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: [EXT] [PATCH net-next] net: marvell,prestera: Fix example PCI bus > > addressing > > > > External Email > > > > ---------------------------------------------------------------------- > > The example for PCI devices has some addressing errors. 'reg' is written as if > > the parent bus is PCI, but the default bus for examples is 1 address and size > > cell. 'ranges' is defining config space with a size of 0. Generally, config space > > should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these > > issues by updating the values with made-up, but valid values. > > > > This was uncovered with recent dtschema changes. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > index 5ea8b73663a5..16ff892f7bbd 100644 > > --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > @@ -78,8 +78,8 @@ examples: > > pcie@0 { > > #address-cells = <3>; > > #size-cells = <2>; > > - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; > > - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; > > + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; > > + reg = <0x0 0x1000>; > > device_type = "pci"; > > > > switch@0,0 { > > -- > > 2.43.0 > > > > This yaml has a mix-up of device P/N (belonging to AC3, BC2) and PCIe > IDs (belonging to AC3X, Aldrin2) > Looks like a part of the yaml was updated, and another part was not > > There is a reference here of actual usage of prestera switch device: > https://github.com/dentproject/linux/blob/dent-linux-5.15.y/arch/arm64/boot/dts/marvell/accton-as4564-26p.dts That doesn't match upstream at all... > > So actual ranges and reg could be used instead of made up ones. > > But the actual real life dts places the prestera at the top level of > the dts, not under pci. > > I am not aware of any dts/dtsi using such kind of switch node under > pcie node, similar to the example given in the yaml file, and did not > manage to find any under latest linux-next for both arm and arm64 dts > directories (please correct me here if I am wrong). Don't know. It seems plausible. > > So the question here is if this pci example really necessary for the > prestera device, or can be removed altogether (which is what I think is best to do). Miquel's commit adding indicates such devices exist. Why would he add them otherwise? Anyways, I'm just fixing boilerplate to make the PCI bus properties valid. Has nothing to do with this Marvell device really. Rob
Hello, > > > The example for PCI devices has some addressing errors. 'reg' is written as if > > > the parent bus is PCI, but the default bus for examples is 1 address and size > > > cell. 'ranges' is defining config space with a size of 0. Generally, config space > > > should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these > > > issues by updating the values with made-up, but valid values. > > > > > > This was uncovered with recent dtschema changes. > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > --- > > > Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > index 5ea8b73663a5..16ff892f7bbd 100644 > > > --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > @@ -78,8 +78,8 @@ examples: > > > pcie@0 { > > > #address-cells = <3>; > > > #size-cells = <2>; > > > - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; > > > + reg = <0x0 0x1000>; > > > device_type = "pci"; > > > > > > switch@0,0 { > > > -- > > > 2.43.0 > > > > > > > This yaml has a mix-up of device P/N (belonging to AC3, BC2) and PCIe > > IDs (belonging to AC3X, Aldrin2) > > Looks like a part of the yaml was updated, and another part was not > > > > There is a reference here of actual usage of prestera switch device: > > https://github.com/dentproject/linux/blob/dent-linux-5.15.y/arch/arm64/boot/dts/marvell/accton-as4564-26p.dts > > That doesn't match upstream at all... Yes, the DTS there are not up to date. I actually took mine (see below) from: https://github.com/dentproject/linux/blob/dent-linux-5.15.105/arch/arm64/boot/dts/marvell/delta-tn48m.dts#L133 and fixed the Prestera representation (a root node does not make any sense). > > So actual ranges and reg could be used instead of made up ones. > > > > But the actual real life dts places the prestera at the top level of > > the dts, not under pci. > > > > I am not aware of any dts/dtsi using such kind of switch node under > > pcie node, similar to the example given in the yaml file, and did not > > manage to find any under latest linux-next for both arm and arm64 dts > > directories (please correct me here if I am wrong). > > Don't know. It seems plausible. The DT where this is used is public but not upstream, it was derived from the above link: https://github.com/miquelraynal/linux/blob/onie/syseeprom-public/arch/arm64/boot/dts/marvell/armada-7040-tn48m.dts#L316 > > So the question here is if this pci example really necessary for the > > prestera device, or can be removed altogether (which is what I think is best to do). > > Miquel's commit adding indicates such devices exist. Why would he add > them otherwise? > > Anyways, I'm just fixing boilerplate to make the PCI bus properties > valid. Has nothing to do with this Marvell device really. I can't remember why the example in the schema is slightly different (must have seen an update) but here is the exact diff I used to get it working. Maybe the reg/ranges are loose though, TBH I've always been a bit lost by PCI DT properties. + pci@0,0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + ranges; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x0 0x0>; + + switch@0,0 { + reg = <0x0 0x0 0x0 0x0 0x0>; + compatible = "pci11ab,c80c"; ... Would something like this work better for the example? FYI the pci@0,0 node is a child of CP11X_LABEL(pcie0): pcie@CP11X_PCIE0_BASE from armada-cp11x.dtsi (which is upstream). It defines the Prestera switch as attached through PCI on a TN48M Marvell based switch. There was a "whish" to get this DT upstream in the past but it needs to be updated a bit and no action like that was ever triggered. The reason why we want to describe it is that it exposes interesting NVMEM cells to the system (like MAC addresses). Thanks, Miquèl
On Thu, Jan 25, 2024 at 3:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > > > > The example for PCI devices has some addressing errors. 'reg' is written as if > > > > the parent bus is PCI, but the default bus for examples is 1 address and size > > > > cell. 'ranges' is defining config space with a size of 0. Generally, config space > > > > should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these > > > > issues by updating the values with made-up, but valid values. > > > > > > > > This was uncovered with recent dtschema changes. > > > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > --- > > > > Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > index 5ea8b73663a5..16ff892f7bbd 100644 > > > > --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > @@ -78,8 +78,8 @@ examples: > > > > pcie@0 { > > > > #address-cells = <3>; > > > > #size-cells = <2>; > > > > - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > > - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > > + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; > > > > + reg = <0x0 0x1000>; > > > > device_type = "pci"; > > > > > > > > switch@0,0 { > > > > -- > > > > 2.43.0 > > > > > > > > > > This yaml has a mix-up of device P/N (belonging to AC3, BC2) and PCIe > > > IDs (belonging to AC3X, Aldrin2) > > > Looks like a part of the yaml was updated, and another part was not > > > > > > There is a reference here of actual usage of prestera switch device: > > > https://github.com/dentproject/linux/blob/dent-linux-5.15.y/arch/arm64/boot/dts/marvell/accton-as4564-26p.dts > > > > That doesn't match upstream at all... > > Yes, the DTS there are not up to date. I actually took mine (see below) > from: > https://github.com/dentproject/linux/blob/dent-linux-5.15.105/arch/arm64/boot/dts/marvell/delta-tn48m.dts#L133 > and fixed the Prestera representation (a root node does not make any > sense). > > > > So actual ranges and reg could be used instead of made up ones. > > > > > > But the actual real life dts places the prestera at the top level of > > > the dts, not under pci. > > > > > > I am not aware of any dts/dtsi using such kind of switch node under > > > pcie node, similar to the example given in the yaml file, and did not > > > manage to find any under latest linux-next for both arm and arm64 dts > > > directories (please correct me here if I am wrong). > > > > Don't know. It seems plausible. > > The DT where this is used is public but not upstream, it was derived > from the above link: > https://github.com/miquelraynal/linux/blob/onie/syseeprom-public/arch/arm64/boot/dts/marvell/armada-7040-tn48m.dts#L316 > > > > So the question here is if this pci example really necessary for the > > > prestera device, or can be removed altogether (which is what I think is best to do). > > > > Miquel's commit adding indicates such devices exist. Why would he add > > them otherwise? > > > > Anyways, I'm just fixing boilerplate to make the PCI bus properties > > valid. Has nothing to do with this Marvell device really. > > I can't remember why the example in the schema is slightly different > (must have seen an update) but here is the exact diff I used to get it > working. Maybe the reg/ranges are loose though, TBH I've always been > a bit lost by PCI DT properties. Yeah, there aren't many examples to go on, but I'm trying to improve the schema to better constrain PCI nodes to be correct. > > + pci@0,0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; > + ranges; > + #address-cells = <3>; > + #size-cells = <2>; > + bus-range = <0x0 0x0>; > + > + switch@0,0 { > + reg = <0x0 0x0 0x0 0x0 0x0>; > + compatible = "pci11ab,c80c"; > ... > > Would something like this work better for the example? While this is valid on its own, it's not with the example template. The example has to have a host bridge node because the template is just the default bus addressing and there has to be translation to PCI addressing. To put it another way, we can only check 'reg' if the parent node is valid, but with the above the parent node is wrong. > FYI the pci@0,0 node is a child of > > CP11X_LABEL(pcie0): pcie@CP11X_PCIE0_BASE > > from armada-cp11x.dtsi (which is upstream). Right, that's the host bridge and then the root port node and then the device. Whether there's a root port or not is outside the scope of this binding, but if you want to show it that's fine. Rob
Hi Rob, robh@kernel.org wrote on Fri, 26 Jan 2024 09:12:08 -0600: > On Thu, Jan 25, 2024 at 3:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hello, > > > > > > > The example for PCI devices has some addressing errors. 'reg' is written as if > > > > > the parent bus is PCI, but the default bus for examples is 1 address and size > > > > > cell. 'ranges' is defining config space with a size of 0. Generally, config space > > > > > should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these > > > > > issues by updating the values with made-up, but valid values. > > > > > > > > > > This was uncovered with recent dtschema changes. > > > > > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > > --- > > > > > Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > > b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > > index 5ea8b73663a5..16ff892f7bbd 100644 > > > > > --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > > +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml > > > > > @@ -78,8 +78,8 @@ examples: > > > > > pcie@0 { > > > > > #address-cells = <3>; > > > > > #size-cells = <2>; > > > > > - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > > > - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; > > > > > + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; > > > > > + reg = <0x0 0x1000>; > > > > > device_type = "pci"; > > > > > > > > > > switch@0,0 { > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > > > This yaml has a mix-up of device P/N (belonging to AC3, BC2) and PCIe > > > > IDs (belonging to AC3X, Aldrin2) > > > > Looks like a part of the yaml was updated, and another part was not > > > > > > > > There is a reference here of actual usage of prestera switch device: > > > > https://github.com/dentproject/linux/blob/dent-linux-5.15.y/arch/arm64/boot/dts/marvell/accton-as4564-26p.dts > > > > > > That doesn't match upstream at all... > > > > Yes, the DTS there are not up to date. I actually took mine (see below) > > from: > > https://github.com/dentproject/linux/blob/dent-linux-5.15.105/arch/arm64/boot/dts/marvell/delta-tn48m.dts#L133 > > and fixed the Prestera representation (a root node does not make any > > sense). > > > > > > So actual ranges and reg could be used instead of made up ones. > > > > > > > > But the actual real life dts places the prestera at the top level of > > > > the dts, not under pci. > > > > > > > > I am not aware of any dts/dtsi using such kind of switch node under > > > > pcie node, similar to the example given in the yaml file, and did not > > > > manage to find any under latest linux-next for both arm and arm64 dts > > > > directories (please correct me here if I am wrong). > > > > > > Don't know. It seems plausible. > > > > The DT where this is used is public but not upstream, it was derived > > from the above link: > > https://github.com/miquelraynal/linux/blob/onie/syseeprom-public/arch/arm64/boot/dts/marvell/armada-7040-tn48m.dts#L316 > > > > > > So the question here is if this pci example really necessary for the > > > > prestera device, or can be removed altogether (which is what I think is best to do). > > > > > > Miquel's commit adding indicates such devices exist. Why would he add > > > them otherwise? > > > > > > Anyways, I'm just fixing boilerplate to make the PCI bus properties > > > valid. Has nothing to do with this Marvell device really. > > > > I can't remember why the example in the schema is slightly different > > (must have seen an update) but here is the exact diff I used to get it > > working. Maybe the reg/ranges are loose though, TBH I've always been > > a bit lost by PCI DT properties. > > Yeah, there aren't many examples to go on, but I'm trying to improve > the schema to better constrain PCI nodes to be correct. > > > > > + pci@0,0 { > > + device_type = "pci"; > > + reg = <0x0 0x0 0x0 0x0 0x0>; > > + ranges; > > + #address-cells = <3>; > > + #size-cells = <2>; > > + bus-range = <0x0 0x0>; > > + > > + switch@0,0 { > > + reg = <0x0 0x0 0x0 0x0 0x0>; > > + compatible = "pci11ab,c80c"; > > ... > > > > Would something like this work better for the example? > > While this is valid on its own, it's not with the example template. > The example has to have a host bridge node because the template is > just the default bus addressing and there has to be translation to PCI > addressing. To put it another way, we can only check 'reg' if the > parent node is valid, but with the above the parent node is wrong. > > > FYI the pci@0,0 node is a child of > > > > CP11X_LABEL(pcie0): pcie@CP11X_PCIE0_BASE > > > > from armada-cp11x.dtsi (which is upstream). > > Right, that's the host bridge and then the root port node and then the > device. Whether there's a root port or not is outside the scope of > this binding, but if you want to show it that's fine. I don't have a strong opinion on that, I was just giving as much information as I could. I prefer clean binding even though the examples are not perfectly matching the reality, if all examples follow the same pattern (which I believe is what you are currently working on). Thanks, Miquèl
diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.yaml b/Documentation/devicetree/bindings/net/marvell,prestera.yaml index 5ea8b73663a5..16ff892f7bbd 100644 --- a/Documentation/devicetree/bindings/net/marvell,prestera.yaml +++ b/Documentation/devicetree/bindings/net/marvell,prestera.yaml @@ -78,8 +78,8 @@ examples: pcie@0 { #address-cells = <3>; #size-cells = <2>; - ranges = <0x0 0x0 0x0 0x0 0x0 0x0>; - reg = <0x0 0x0 0x0 0x0 0x0 0x0>; + ranges = <0x02000000 0x0 0x100000 0x10000000 0x0 0x0>; + reg = <0x0 0x1000>; device_type = "pci"; switch@0,0 {
The example for PCI devices has some addressing errors. 'reg' is written as if the parent bus is PCI, but the default bus for examples is 1 address and size cell. 'ranges' is defining config space with a size of 0. Generally, config space should not be defined in 'ranges', only PCI memory and I/O spaces. Fix these issues by updating the values with made-up, but valid values. This was uncovered with recent dtschema changes. Signed-off-by: Rob Herring <robh@kernel.org> --- Documentation/devicetree/bindings/net/marvell,prestera.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)