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

Hello,

quoted

quoted

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

quoted

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

quoted

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