Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to yaml

On Tue, Jul 20, 2021 at 3:58 AM Joakim Zhang [off-list ref] wrote:

Hi Rob,

quoted

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: 2021年7月20日 6:45
To: Joakim Zhang <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; shawnguo@kernel.org;
s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
bruno.thomsen@gmail.com; dl-linux-imx [off-list ref];
netdev@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to
yaml

On Fri, Jul 16, 2021 at 06:29:09PM +0800, Joakim Zhang wrote:

quoted

In order to automate the verification of DT nodes convert fsl-fec.txt
to fsl,fec.yaml, and pass binding check with below command.

$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- dt_binding_check

DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/fsl,fec.yaml

quoted

  DTEX    Documentation/devicetree/bindings/net/fsl,fec.example.dts
  DTC

Documentation/devicetree/bindings/net/fsl,fec.example.dt.yaml

quoted

  CHECK

Documentation/devicetree/bindings/net/fsl,fec.example.dt.yaml

quoted

Signed-off-by: Joakim Zhang <redacted>
---
 .../devicetree/bindings/net/fsl,fec.yaml      | 213 ++++++++++++++++++
 .../devicetree/bindings/net/fsl-fec.txt       |  95 --------
 2 files changed, 213 insertions(+), 95 deletions(-)  create mode
100644 Documentation/devicetree/bindings/net/fsl,fec.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt

Since the networking maintainers can't wait for actual binding reviews, you get
to send a follow-up patch...

OK.

quoted

quoted

diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml

b/Documentation/devicetree/bindings/net/fsl,fec.yaml
new file mode 100644
index 000000000000..7fa11f6622b1

--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml

@@ -0,0 +1,213 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fschemas%2Fnet%2Ffsl%2Cfec.yaml%23&data=04%7C01
%7Cqia

quoted

+ngqing.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686
ea1d3b

quoted

+c2b4c6fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknow
n%7CTWFp

quoted

+bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
I6M

quoted

+n0%3D%7C1000&sdata=MG%2F9aKVQ8quuLiYxtC%2F7zqZTY7mIHur4G
D7tsPtfq%

quoted

+2Bg%3D&reserved=0
+$schema:
+https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi

+cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=04%7C01%7Cqia
ngqing

quoted

+.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686ea1d3b
c2b4c6

quoted

+fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknown%7CT
WFpbGZsb3

quoted

+d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
%3D%

quoted

+7C1000&sdata=7vf8UYQ7c%2BlaJxzAzhzvkPPvbaX3UWVowCtDtPNO9p
Q%3D&amp

quoted

+;reserved=0
+
+title: Freescale Fast Ethernet Controller (FEC)
+
+maintainers:
+  - Joakim Zhang [off-list ref]
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - fsl,imx25-fec
+          - fsl,imx27-fec
+          - fsl,imx28-fec
+          - fsl,imx6q-fec
+          - fsl,mvf600-fec
+      - items:
+          - enum:
+              - fsl,imx53-fec
+              - fsl,imx6sl-fec
+          - const: fsl,imx25-fec
+      - items:
+          - enum:
+              - fsl,imx35-fec
+              - fsl,imx51-fec
+          - const: fsl,imx27-fec
+      - items:
+          - enum:
+              - fsl,imx6ul-fec
+              - fsl,imx6sx-fec
+          - const: fsl,imx6q-fec
+      - items:
+          - enum:
+              - fsl,imx7d-fec
+          - const: fsl,imx6sx-fec
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 4
+
+  interrupt-names:
+    description:
+      Names of the interrupts listed in interrupts property in the same

order.

quoted

+      The defaults if not specified are
+      __Number of interrupts__   __Default__
+            1                       "int0"
+            2                       "int0", "pps"
+            3                       "int0", "int1", "int2"
+            4                       "int0", "int1", "int2", "pps"
+      The order may be changed as long as they correspond to the

interrupts

quoted

+      property.

Why? None of the existing dts files do that and there is no reason to support
random order. You can do this:

oneOf:
  - minItems: 1
    items:
      - const: int0
      - const: pps
  - minItems: 3
    items:
      - const: int0
      - const: int1
      - const: int2
      - const: pps

How about below?
    oneOf:
      - items:
          - const: int0
      - items:
          - const: int0
          - const: pps
      - items:
          - const: int0
          - const: int1
          - const: int2
      - items:
          - const: int0
          - const: int1
          - const: int2
          - const: pps

What I wrote for you is the same thing, just more concise.

quoted

quoted

Currently, only i.mx7 uses "int1" and "int2". They correspond to

Sounds like another constraint under an if/then schema.

quoted

+      tx/rx queues 1 and 2. "int0" will be used for queue 0 and ENET_MII

interrupts.

quoted

+      For imx6sx, "int0" handles all 3 queues and ENET_MII. "pps" is for

the pulse

quoted

+      per second interrupt associated with 1588 precision time

protocol(PTP).

quoted

+
+  clocks:
+    minItems: 2
+    maxItems: 5
+    description:
+      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for register

accessing.

quoted

+      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
+      The "ptp"(option), for IEEE1588 timer clock that requires the clock.
+      The "enet_clk_ref"(option), for MAC transmit/receiver reference

clock like

quoted

+      RGMII TXC clock or RMII reference clock. It depends on board

design,

quoted

+      the clock is required if RGMII TXC and RMII reference clock source

from

quoted

+      SOC internal PLL.
+      The "enet_out"(option), output clock for external device, like supply

clock

quoted

+      for PHY. The clock is required if PHY clock source from SOC.
+
+  clock-names:
+    minItems: 2
+    maxItems: 5
+    contains:
+      enum:
+      - ipg
+      - ahb
+      - ptp
+      - enet_clk_ref
+      - enet_out

This means clock-names contains one of these strings and then anything else is
valid.

s/contains/items/

OK.

quoted

quoted

+
+  phy-mode: true
+
+  phy-handle: true
+
+  fixed-link: true
+
+  local-mac-address: true
+
+  mac-address: true
+
+  phy-supply:
+    description:
+      Regulator that powers the Ethernet PHY.
+
+  fsl,num-tx-queues:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The property is valid for enet-avb IP, which supports hw multi

queues.

quoted

+      Should specify the tx queue number, otherwise set tx queue number

to 1.

constraints? 2^32 queues are valid?

Yes, the value should be limited to 1, 2, 3. Will add:
default: 1
minimum: 1
maximum: 3

Personally, I'd do 'enum: [ 1, 2, 3 ]' instead.

quoted

quoted

+
+  fsl,num-rx-queues:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The property is valid for enet-avb IP, which supports hw multi

queues.

quoted

+      Should specify the rx queue number, otherwise set rx queue number

to 1.

constraints?

Will add.

quoted

quoted

+
+  fsl,magic-packet:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If present, indicates that the hardware supports waking up via magic

packet.

quoted

+
+  fsl,err006687-workaround-present:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If present indicates that the system has the hardware workaround

for

quoted

+      ERR006687 applied and does not need a software workaround.
+
+  fsl,stop-mode:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Register bits of stop mode control, the format is <&gpr req_gpr

req_bit>.

So, maxItems: 3

Why? I think maxItems: 1

quoted

quoted

+      gpr is the phandle to general purpose register node.
+      req_gpr is the gpr register offset for ENET stop request.
+      req_bit is the gpr bit offset for ENET stop request.
+
+  mdio:
+    type: object
+    description:
+      Specifies the mdio bus in the FEC, used as a container for phy nodes.
+
+  # Deprecated optional properties:
+  # To avoid these, create a phy node according to ethernet-phy.yaml
+ in the same  # directory, and point the FEC's "phy-handle" property
+ to it. Then use  # the phy's reset binding, again described by

ethernet-phy.yaml.

quoted

+
+  phy-reset-gpios:
+    deprecated: true
+    description:
+      Should specify the gpio for phy reset.
+
+  phy-reset-duration:
+    deprecated: true
+    description:
+      Reset duration in milliseconds.  Should present only if property
+      "phy-reset-gpios" is available.  Missing the property will have the
+      duration be 1 millisecond.  Numbers greater than 1000 are invalid
+      and 1 millisecond will be used instead.
+
+  phy-reset-active-high:
+    deprecated: true
+    description:
+      If present then the reset sequence using the GPIO specified in the
+      "phy-reset-gpios" property is reversed (H=reset state, L=operation

state).

quoted

+
+  phy-reset-post-delay:
+    deprecated: true
+    description:
+      Post reset delay in milliseconds. If present then a delay of

phy-reset-post-delay

quoted

+      milliseconds will be observed after the phy-reset-gpios has been

toggled.

quoted

+      Can be omitted thus no delay is observed. Delay is in range of 1ms to

1000ms.

quoted

+      Other delays are invalid.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+# FIXME: We had better set additionalProperties to false to avoid
+invalid or at # least undocumented properties. However, PHY may have
+a deprecated option to # place PHY OF properties in the MAC node,
+such as Micrel PHY, and we can find # these boards which is based on

i.MX6QDL.

quoted

+additionalProperties: true

Why can't the dts files be updated? Can you point me to an example .dts?

Below dtsi under fec device node:
arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
arch/arm/boot/dts/imx6qdl-nitrogen6_max.dtsi
arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi

You either need to document all those deprecated properties or update
the dts files. I'd prefer the latter.

Rob