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
