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

[Joakim Zhang <qiangqing.zhang@nxp.com>]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2021年7月20日 6:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> 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 <linux-imx@nxp.com>;
> 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:
> > 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
> >   DTEX    Documentation/devicetree/bindings/net/fsl,fec.example.dts
> >   DTC
> Documentation/devicetree/bindings/net/fsl,fec.example.dt.yaml
> >   CHECK
> Documentation/devicetree/bindings/net/fsl,fec.example.dt.yaml
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  .../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.

> >
> > 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&amp;data=04%7C01
> %7Cqia
> >
> +ngqing.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686
> ea1d3b
> >
> +c2b4c6fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknow
> n%7CTWFp
> >
> +bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6M
> >
> +n0%3D%7C1000&amp;sdata=MG%2F9aKVQ8quuLiYxtC%2F7zqZTY7mIHur4G
> D7tsPtfq%
> > +2Bg%3D&amp;reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cqia
> ngqing
> >
> +.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686ea1d3b
> c2b4c6
> >
> +fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknown%7CT
> WFpbGZsb3
> >
> +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%
> >
> +7C1000&amp;sdata=7vf8UYQ7c%2BlaJxzAzhzvkPPvbaX3UWVowCtDtPNO9p
> Q%3D&amp
> > +;reserved=0
> > +
> > +title: Freescale Fast Ethernet Controller (FEC)
> > +
> > +maintainers:
> > +  - Joakim Zhang <qiangqing.zhang@nxp.com>
> > +
> > +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.
> > +      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
> > +      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
> 	
> 
> > Currently, only i.mx7 uses "int1" and "int2". They correspond to
> 
> Sounds like another constraint under an if/then schema.
> 
> > +      tx/rx queues 1 and 2. "int0" will be used for queue 0 and ENET_MII
> interrupts.
> > +      For imx6sx, "int0" handles all 3 queues and ENET_MII. "pps" is for
> the pulse
> > +      per second interrupt associated with 1588 precision time
> protocol(PTP).
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 5
> > +    description:
> > +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for register
> accessing.
> > +      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
> > +      RGMII TXC clock or RMII reference clock. It depends on board
> design,
> > +      the clock is required if RGMII TXC and RMII reference clock source
> from
> > +      SOC internal PLL.
> > +      The "enet_out"(option), output clock for external device, like supply
> clock
> > +      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.

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

> 
> > +
> > +  fsl,num-rx-queues:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      The property is valid for enet-avb IP, which supports hw multi
> queues.
> > +      Should specify the rx queue number, otherwise set rx queue number
> to 1.
> 
> constraints?
Will add.

> 
> > +
> > +  fsl,magic-packet:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      If present, indicates that the hardware supports waking up via magic
> packet.
> > +
> > +  fsl,err006687-workaround-present:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      If present indicates that the system has the hardware workaround
> for
> > +      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

> 
> > +      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.
> > +
> > +  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).
> > +
> > +  phy-reset-post-delay:
> > +    deprecated: true
> > +    description:
> > +      Post reset delay in milliseconds. If present then a delay of
> phy-reset-post-delay
> > +      milliseconds will be observed after the phy-reset-gpios has been
> toggled.
> > +      Can be omitted thus no delay is observed. Delay is in range of 1ms to
> 1000ms.
> > +      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.
> > +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
....

Best Regards,
Joakim Zhang
> > +
> > +examples:
> > +  - |
> > +    ethernet@83fec000 {
> > +      compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> > +      reg = <0x83fec000 0x4000>;
> > +      interrupts = <87>;
> > +      phy-mode = "mii";
> > +      phy-reset-gpios = <&gpio2 14 0>;
> > +      phy-supply = <&reg_fec_supply>;
> > +    };
> > +
> > +    ethernet@83fed000 {
> > +      compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> > +      reg = <0x83fed000 0x4000>;
> > +      interrupts = <87>;
> > +      phy-mode = "mii";
> > +      phy-reset-gpios = <&gpio2 14 0>;
> > +      phy-supply = <&reg_fec_supply>;
> > +      phy-handle = <&ethphy0>;
> > +
> > +      mdio {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ethphy0: ethernet-phy@0 {
> > +          compatible = "ethernet-phy-ieee802.3-c22";
> > +          reg = <0>;
> > +        };
> > +      };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
> > b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > deleted file mode 100644
> > index 9b543789cd52..000000000000
> > --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> > +++ /dev/null
> > @@ -1,95 +0,0 @@
> > -* Freescale Fast Ethernet Controller (FEC)
> > -
> > -Required properties:
> > -- compatible : Should be "fsl,<soc>-fec"
> > -- reg : Address and length of the register set for the device
> > -- interrupts : Should contain fec interrupt
> > -- phy-mode : See ethernet.txt file in the same directory
> > -
> > -Optional properties:
> > -- phy-supply : regulator that powers the Ethernet PHY.
> > -- phy-handle : phandle to the PHY device connected to this device.
> > -- fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
> > -  Use instead of phy-handle.
> > -- fsl,num-tx-queues : The property is valid for enet-avb IP, which
> > supports
> > -  hw multi queues. Should specify the tx queue number, otherwise set
> > tx queue
> > -  number to 1.
> > -- fsl,num-rx-queues : The property is valid for enet-avb IP, which
> > supports
> > -  hw multi queues. Should specify the rx queue number, otherwise set
> > rx queue
> > -  number to 1.
> > -- fsl,magic-packet : If present, indicates that the hardware supports
> > waking
> > -  up via magic packet.
> > -- fsl,err006687-workaround-present: If present indicates that the
> > system has
> > -  the hardware workaround for ERR006687 applied and does not need a
> > software
> > -  workaround.
> > -- fsl,stop-mode: register bits of stop mode control, the format is
> > -		 <&gpr req_gpr req_bit>.
> > -		 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.
> > - -interrupt-names:  names of the interrupts listed in interrupts
> > property in
> > -  the same order. 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
> > -  property. Currently, only i.mx7 uses "int1" and "int2". They
> > correspond to
> > -  tx/rx queues 1 and 2. "int0" will be used for queue 0 and ENET_MII
> interrupts.
> > -  For imx6sx, "int0" handles all 3 queues and ENET_MII. "pps" is for
> > the pulse
> > -  per second interrupt associated with 1588 precision time protocol(PTP).
> > -
> > -Optional subnodes:
> > -- mdio : specifies the mdio bus in the FEC, used as a container for
> > phy nodes
> > -  according to phy.txt in the same directory
> > -
> > -Deprecated optional properties:
> > -	To avoid these, create a phy node according to phy.txt in the same
> > -	directory, and point the fec's "phy-handle" property to it. Then use
> > -	the phy's reset binding, again described by phy.txt.
> > -- phy-reset-gpios : Should specify the gpio for phy reset
> > -- phy-reset-duration : 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 : If present then the reset sequence using
> > the GPIO
> > -  specified in the "phy-reset-gpios" property is reversed (H=reset
> > state,
> > -  L=operation state).
> > -- phy-reset-post-delay : Post reset delay in milliseconds. If present
> > then
> > -  a delay of phy-reset-post-delay milliseconds will be observed after
> > the
> > -  phy-reset-gpios has been toggled. Can be omitted thus no delay is
> > -  observed. Delay is in range of 1ms to 1000ms. Other delays are invalid.
> > -
> > -Example:
> > -
> > -ethernet@83fec000 {
> > -	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> > -	reg = <0x83fec000 0x4000>;
> > -	interrupts = <87>;
> > -	phy-mode = "mii";
> > -	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */
> > -	local-mac-address = [00 04 9F 01 1B B9];
> > -	phy-supply = <&reg_fec_supply>;
> > -};
> > -
> > -Example with phy specified:
> > -
> > -ethernet@83fec000 {
> > -	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> > -	reg = <0x83fec000 0x4000>;
> > -	interrupts = <87>;
> > -	phy-mode = "mii";
> > -	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */
> > -	local-mac-address = [00 04 9F 01 1B B9];
> > -	phy-supply = <&reg_fec_supply>;
> > -	phy-handle = <&ethphy>;
> > -	mdio {
> > -	        clock-frequency = <5000000>;
> > -		ethphy: ethernet-phy@6 {
> > -			compatible = "ethernet-phy-ieee802.3-c22";
> > -			reg = <6>;
> > -			max-speed = <100>;
> > -		};
> > -	};
> > -};
> > --
> > 2.17.1
> >
> >