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

From: Joakim Zhang
Date: Tue Jul 20 2021 - 05:58:39 EST

Next message: Greg KH: "Re: ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback"
Previous message: Paul Fertser: "Re: [PATCH v2 0/3] net/ncsi: Add NCSI Intel OEM command to keep PHY link up"
In reply to: Rob Herring: "Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to yaml"
Next in thread: Rob Herring: "Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to yaml"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 2021年7月20日 6:45
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> bruno.thomsen@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > .../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&data=04%7C01
> %7Cqia
> >
> +ngqing.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686
> ea1d3b
> >
> +c2b4c6fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknow
> n%7CTWFp
> >
> +bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6M
> >
> +n0%3D%7C1000&sdata=MG%2F9aKVQ8quuLiYxtC%2F7zqZTY7mIHur4G
> D7tsPtfq%
> > +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
> >
> +.zhang%40nxp.com%7Cf54454be267b426e60d008d94b06dfc7%7C686ea1d3b
> c2b4c6
> >
> +fa92cd99c5c301635%7C0%7C0%7C637623315180063755%7CUnknown%7CT
> WFpbGZsb3
> >
> +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%
> >
> +7C1000&sdata=7vf8UYQ7c%2BlaJxzAzhzvkPPvbaX3UWVowCtDtPNO9p
> Q%3D&amp
> > +;reserved=0
> > +
> > +title: Freescale Fast Ethernet Controller (FEC)
> > +
> > +maintainers:
> > + - Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > +
> > +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
> >
> >

Next message: Greg KH: "Re: ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback"
Previous message: Paul Fertser: "Re: [PATCH v2 0/3] net/ncsi: Add NCSI Intel OEM command to keep PHY link up"
In reply to: Rob Herring: "Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to yaml"
Next in thread: Rob Herring: "Re: [PATCH V1 1/3] dt-bindings: net: fec: convert fsl,*fec bindings to yaml"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]