RE: [EXT] Re: [PATCH net-next v3 2/7] dt-bindings: net: add backplane dt bindings

[Florinel Iordache]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Sent: Tuesday, June 23, 2020 1:21 AM
> To: Florinel Iordache <florinel.iordache@xxxxxxx>; davem@xxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; andrew@xxxxxxx; hkallweit1@xxxxxxxxx;
> linux@xxxxxxxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; kuba@xxxxxxxxxx;
> corbet@xxxxxxx; shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Madalin
> Bucur (OSS) <madalin.bucur@xxxxxxxxxxx>; Ioana Ciornei
> <ioana.ciornei@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH net-next v3 2/7] dt-bindings: net: add backplane dt
> bindings
> 
> Caution: EXT Email
> 
> On 6/22/20 6:35 AM, Florinel Iordache wrote:
> > Add ethernet backplane device tree bindings
> >
> > Signed-off-by: Florinel Iordache <florinel.iordache@xxxxxxx>
> > ---
> 
> [snip]
> 
> > +properties:
> > +  $nodename:
> > +    pattern: "^serdes(@[a-f0-9]+)?$"
> > +
> > +  compatible:
> > +    oneOf:
> > +      - const: serdes-10g
> > +        description: SerDes module type of 10G
> 
> Since this appears to be memory mapped in your case, it does not sound like
> "serdes-10g" alone is going to be sufficient, should not we have a SoC specific
> compatible string if nothing else?

My intention was to make it generic enough to be used by any SerDes (Serializer/Deserializer) block.
So I was thinking that specifying serdes as HW block and the type: 10g (or 28g for example) should be enough.
I could add SoC specific (or family of SoC) to the compatible string
like for example Freescale/NXP QorIQ Soc: "fsl,ls1046a-serdes-10g" or "fsl,qoriq-serdes-10g"

> 
> > +
> > +  reg:
> > +    description:
> > +      Registers memory map offset and size for this serdes module
> > +
> > +  reg-names:
> > +    description:
> > +      Names of the register map given in "reg" node.
> 
> You would also need to describe how many of these two properties are
> expected.

Only one memory map is required since the memory maps for lanes are individually described
(as it is documented in serdes-lane.yaml).
I will specify this.

> 
> > +
> > +  little-endian:
> > +    description:
> > +      Specifies the endianness of serdes module
> > +      For complete definition see
> > +      Documentation/devicetree/bindings/common-properties.txt
> 
> This is redundant with the default binding then, and if it is not already in the
> common YAML binding, can you please add little-endian and native-endian
> added there?

The endianness of the serdes block must be specified as little-endian or big-endian.
The serdes endianness may be different than the cores endianness.
This is also the case for QorIQ LS1043/LS1046 platforms with ARM cores which
are little endian but serdes block is big endian.
So endianness must be specified in device tree in order for the driver to know how to access it.
This is the generic binding description (with an example below)
but for LS1046 platform for example we should put: big-endian
(as it is in the last patch: 0007-arm64-dts-add-serdes-and-mdio-description.patch
in file: /arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi )

> 
> > +
> > +examples:
> > +  - |
> > +    serdes1: serdes@1ea0000 {
> > +        compatible = "serdes-10g";
> > +        reg = <0x0 0x1ea0000 0 0x00002000>;
> > +        reg-names = "serdes", "serdes-10g";
> > +        little-endian;
> > +    };
> >
> 
> 
> --
> Florian

Thank you for feedback
Florinel.