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

[Miquel Raynal]

Hi Rob,

robh@xxxxxxxxxx wrote on Fri, 26 Jan 2024 09:12:08 -0600:

> On Thu, Jan 25, 2024 at 3:03 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > >  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