2024-01-22 18:10:39

by Rob Herring (Arm)

[permalink] [raw]
Subject: [PATCH net-next] net: marvell,prestera: Fix example PCI bus addressing

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 <[email protected]>
---
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



2024-01-23 10:07:41

by Elad Nachman

[permalink] [raw]
Subject: RE: [EXT] [PATCH net-next] net: marvell,prestera: Fix example PCI bus addressing



> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Monday, January 22, 2024 7:35 PM
> To: David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Miquel Raynal <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
> ---
> 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.

2024-01-23 22:43:40

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [EXT] [PATCH net-next] net: marvell,prestera: Fix example PCI bus addressing

On Tue, Jan 23, 2024 at 09:59:46AM +0000, Elad Nachman wrote:
>
>
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: Monday, January 22, 2024 7:35 PM
> > To: David S. Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> > <[email protected]>; Krzysztof Kozlowski
> > <[email protected]>; Conor Dooley <[email protected]>;
> > Miquel Raynal <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]
> > 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 <[email protected]>
> > ---
> > 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