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

On 6/24/20 5:55 AM, Florinel Iordache wrote:

quoted

-----Original Message-----
From: Florian Fainelli <f.fainelli@gmail.com>
Sent: Tuesday, June 23, 2020 1:21 AM
To: Florinel Iordache <redacted>; davem@davemloft.net;
netdev@vger.kernel.org; andrew@lunn.ch; hkallweit1@gmail.com;
linux@armlinux.org.uk
Cc: devicetree@vger.kernel.org; linux-doc@vger.kernel.org;
robh+dt@kernel.org; mark.rutland@arm.com; kuba@kernel.org;
corbet@lwn.net; shawnguo@kernel.org; Leo Li [off-list ref]; Madalin
Bucur (OSS) [off-list ref]; Ioana Ciornei
[off-list ref]; linux-kernel@vger.kernel.org
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:

quoted

Add ethernet backplane device tree bindings

Signed-off-by: Florinel Iordache <redacted>
---

[snip]

quoted

+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"

It does not seem to me that the register interface is going to be
anything but generic, therefore using SoC specific compatible strings
would be a safer and forward looking approach. If a generic/fall back
compatibility string can be added, it can be added later on, that is
much less problematic than the opposite.

quoted

quoted

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

Then I believe you need to advertise that with maxItems property to
denote how many.

quoted

quoted

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

I understand that part, my point was more than these properties are
generic properties, therefore it should not be necessary to provide a
description, and their definition belongs in the common properties
binding. If the common binding does not have a definition for those
(that is, in a YAML way), then please add them there.
-- 
Florian