[v4,2/3] dt-bindings: phy: Add i.MX8Q HSIO SerDes PHY binding

Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
Introduce one HSIO configuration 'fsl,hsio-cfg', which need be set at
initialization according to board design.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 .../bindings/phy/fsl,imx8qm-hsio.yaml         | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml

On Thu, 09 May 2024 13:56:20 +0800, Richard Zhu wrote:
> Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
> Introduce one HSIO configuration 'fsl,hsio-cfg', which need be set at
> initialization according to board design.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  .../bindings/phy/fsl,imx8qm-hsio.yaml         | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml:54:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml:54:22: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1715234181-672-3-git-send-email-hongxing.zhu@nxp.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

On Thu, May 09, 2024 at 01:56:20PM +0800, Richard Zhu wrote:
> Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
> Introduce one HSIO configuration 'fsl,hsio-cfg', which need be set at
> initialization according to board design.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  .../bindings/phy/fsl,imx8qm-hsio.yaml         | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> new file mode 100644
> index 000000000000..e8648cd9fea6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/fsl,imx8qm-hsio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX8QM SoC series HSIO SERDES PHY
> +
> +maintainers:
> +  - Richard Zhu <hongxing.zhu@nxp.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx8qm-hsio
> +      - fsl,imx8qxp-hsio
> +  reg:
> +    minItems: 4
> +    maxItems: 4

Need to define what is each entry:

items:
  - description: ...
  - description: ...
  - description: ...
  - description: ...


> +
> +  "#phy-cells":
> +    const: 3
> +    description:
> +      The first defines lane index.
> +      The second defines the type of the PHY refer to the include phy.h.
> +      The third defines the controller index, indicated which controller
> +      is bound to the lane.
> +
> +  reg-names:
> +    items:
> +      - const: reg
> +      - const: phy
> +      - const: ctrl
> +      - const: misc
> +
> +  clocks:
> +    minItems: 5
> +    maxItems: 14
> +
> +  clock-names:
> +    minItems: 5
> +    maxItems: 14
> +
> +  fsl,hsio-cfg:
> +    description: Refer macro HSIO_CFG* include/dt-bindings/phy/phy-imx8-pcie.h.

I can't, it's not in this patch. But it should be.

Please say something about what this is for, not just refer to header.

> +    $ref: /schemas/types.yaml#/definitions/uint32

       maximum: 3

> +
> +  fsl,refclk-pad-mode:
> +    description:
> +      Specifies the mode of the refclk pad used. INPUT(PHY refclock is
> +      provided externally via the refclk pad) or OUTPUT(PHY refclock is
> +      derived from SoC internal source and provided on the refclk pad).
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ "input", "output" ]

default?

Really, this could just be a boolean for the non-default mode.

> +
> +  power-domains:
> +    minItems: 1
> +    maxItems: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - "#phy-cells"
> +  - clocks
> +  - clock-names
> +  - fsl,hsio-cfg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx8qxp-hsio
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: pclk0
> +            - const: apb_pclk0
> +            - const: phy0_crr
> +            - const: ctl0_crr
> +            - const: misc_crr
> +        power-domains:
> +          minItems: 1

Should be maxItems? min is already 1.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx8qm-hsio
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: pclk0
> +            - const: pclk1
> +            - const: apb_pclk0
> +            - const: apb_pclk1
> +            - const: pclk2
> +            - const: epcs_tx
> +            - const: epcs_rx
> +            - const: apb_pclk2
> +            - const: phy0_crr
> +            - const: phy1_crr
> +            - const: ctl0_crr
> +            - const: ctl1_crr
> +            - const: ctl2_crr
> +            - const: misc_crr
> +        power-domains:
> +          minItems: 2
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8-clock.h>
> +    #include <dt-bindings/clock/imx8-lpcg.h>
> +    #include <dt-bindings/firmware/imx/rsrc.h>
> +    #include <dt-bindings/phy/phy-imx8-pcie.h>
> +
> +    hsio_phy@5f1a0000 {

phy@...

> +        compatible = "fsl,imx8qxp-hsio";
> +        reg = <0x5f1a0000 0x10000>,
> +              <0x5f120000 0x10000>,
> +              <0x5f140000 0x10000>,
> +              <0x5f160000 0x10000>;
> +        reg-names = "reg", "phy", "ctrl", "misc";
> +        clocks = <&phyx1_lpcg IMX_LPCG_CLK_0>,
> +                 <&phyx1_lpcg IMX_LPCG_CLK_4>,
> +                 <&phyx1_crr1_lpcg IMX_LPCG_CLK_4>,
> +                 <&pcieb_crr3_lpcg IMX_LPCG_CLK_4>,
> +                 <&misc_crr5_lpcg IMX_LPCG_CLK_4>;
> +        clock-names = "pclk0", "apb_pclk0", "phy0_crr", "ctl0_crr", "misc_crr";
> +        power-domains = <&pd IMX_SC_R_SERDES_1>;
> +        #phy-cells = <3>;
> +        fsl,hsio-cfg = <IMX8Q_HSIO_CFG_PCIEB>;
> +        fsl,refclk-pad-mode = "input";
> +    };
> +...
> -- 
> 2.37.1
>

On Fri, May 10, 2024 at 10:38:49AM -0500, Rob Herring wrote:
> On Thu, May 09, 2024 at 01:56:20PM +0800, Richard Zhu wrote:
> > +  fsl,refclk-pad-mode:
> > +    description:
> > +      Specifies the mode of the refclk pad used. INPUT(PHY refclock is
> > +      provided externally via the refclk pad) or OUTPUT(PHY refclock is
> > +      derived from SoC internal source and provided on the refclk pad).
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ "input", "output" ]
> 
> default?
> 
> Really, this could just be a boolean for the non-default mode.

There's actually a third option, or at least there was in v1, unused.
The description in v1 was:

      It can be UNUSED(PHY
      refclock is derived from SoC internal source), INPUT(PHY refclock
      is provided externally via the refclk pad) or OUTPUT(PHY refclock
      is derived from SoC internal source and provided on the refclk pad).

I suggested that there should be 3 strings and not having the property
would mean unused. But all mention of unused seems to have vanished :/

On Fri, May 10, 2024 at 04:52:22PM +0100, Conor Dooley wrote:
> On Fri, May 10, 2024 at 10:38:49AM -0500, Rob Herring wrote:
> > On Thu, May 09, 2024 at 01:56:20PM +0800, Richard Zhu wrote:
> > > +  fsl,refclk-pad-mode:
> > > +    description:
> > > +      Specifies the mode of the refclk pad used. INPUT(PHY refclock is
> > > +      provided externally via the refclk pad) or OUTPUT(PHY refclock is
> > > +      derived from SoC internal source and provided on the refclk pad).
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +    enum: [ "input", "output" ]
> > 
> > default?
> > 
> > Really, this could just be a boolean for the non-default mode.
> 
> There's actually a third option, or at least there was in v1, unused.
> The description in v1 was:
> 
>       It can be UNUSED(PHY
>       refclock is derived from SoC internal source), INPUT(PHY refclock
>       is provided externally via the refclk pad) or OUTPUT(PHY refclock
>       is derived from SoC internal source and provided on the refclk pad).
> 
> I suggested that there should be 3 strings and not having the property
> would mean unused. But all mention of unused seems to have vanished :/

Yes, missed mention about unused.

Is it okay about "fsl,refclk-pad-mode not exist means unused ?

Of course, need mention in description. 


Frank

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2024年5月10日 23:39
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: conor@kernel.org; vkoul@kernel.org; kishon@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; Frank Li <frank.li@nxp.com>;
> conor+dt@kernel.org; linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; imx@lists.linux.dev
> Subject: Re: [PATCH v4 2/3] dt-bindings: phy: Add i.MX8Q HSIO SerDes PHY
> binding
> 
> On Thu, May 09, 2024 at 01:56:20PM +0800, Richard Zhu wrote:
> > Add i.MX8QM and i.MX8QXP HSIO SerDes PHY binding.
> > Introduce one HSIO configuration 'fsl,hsio-cfg', which need be set at
> > initialization according to board design.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  .../bindings/phy/fsl,imx8qm-hsio.yaml         | 142
> ++++++++++++++++++
> >  1 file changed, 142 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> > b/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> > new file mode 100644
> > index 000000000000..e8648cd9fea6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/fsl,imx8qm-hsio.yaml
> > @@ -0,0 +1,142 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fphy%2Ffsl%2Cimx8qm-hsio.yaml%23&data=05%7C
> 02%7
> >
> +Chongxing.zhu%40nxp.com%7C2a3e55cc15c4465304eb08dc71074c2f%7C68
> 6ea1d3
> >
> +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638509523353991074%7CUnkn
> own%7CTWF
> >
> +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6
> >
> +Mn0%3D%7C0%7C%7C%7C&sdata=fBXfU7NNPj57mnI3VXR3vR250oJ7kJrLNjY
> 5W6mL0sk
> > +%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C02%7Chongxing.z
> hu%
> >
> +40nxp.com%7C2a3e55cc15c4465304eb08dc71074c2f%7C686ea1d3bc2b4c6f
> a92cd9
> >
> +9c5c301635%7C0%7C0%7C638509523353999176%7CUnknown%7CTWFpbG
> Zsb3d8eyJWI
> >
> +joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0
> %7C%
> >
> +7C%7C&sdata=n01dVLoIaHimlyaQTU1hUwIUNvmno7YUVnrjkTZK5TQ%3D&res
> erved=0
> > +
> > +title: Freescale i.MX8QM SoC series HSIO SERDES PHY
> > +
> > +maintainers:
> > +  - Richard Zhu <hongxing.zhu@nxp.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx8qm-hsio
> > +      - fsl,imx8qxp-hsio
> > +  reg:
> > +    minItems: 4
> > +    maxItems: 4
> 
> Need to define what is each entry:
> 
> items:
>   - description: ...
>   - description: ...
>   - description: ...
>   - description: ...
> 
> 
> > +
> > +  "#phy-cells":
> > +    const: 3
> > +    description:
> > +      The first defines lane index.
> > +      The second defines the type of the PHY refer to the include phy.h.
> > +      The third defines the controller index, indicated which controller
> > +      is bound to the lane.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: reg
> > +      - const: phy
> > +      - const: ctrl
> > +      - const: misc
> > +
> > +  clocks:
> > +    minItems: 5
> > +    maxItems: 14
> > +
> > +  clock-names:
> > +    minItems: 5
> > +    maxItems: 14
> > +
> > +  fsl,hsio-cfg:
> > +    description: Refer macro HSIO_CFG*
> include/dt-bindings/phy/phy-imx8-pcie.h.
> 
> I can't, it's not in this patch. But it should be.
>
Hi Rob:
Thanks for your comments.
I would merge the first two of the patch-set into one patch later.

Best Regards
Richard Zhu
 
> Please say something about what this is for, not just refer to header.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
>        maximum: 3
> 
> > +
> > +  fsl,refclk-pad-mode:
> > +    description:
> > +      Specifies the mode of the refclk pad used. INPUT(PHY refclock is
> > +      provided externally via the refclk pad) or OUTPUT(PHY refclock is
> > +      derived from SoC internal source and provided on the refclk pad).
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ "input", "output" ]
> 
> default?
> 
> Really, this could just be a boolean for the non-default mode.
> 
> > +
> > +  power-domains:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - "#phy-cells"
> > +  - clocks
> > +  - clock-names
> > +  - fsl,hsio-cfg
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - fsl,imx8qxp-hsio
> > +    then:
> > +      properties:
> > +        clock-names:
> > +          items:
> > +            - const: pclk0
> > +            - const: apb_pclk0
> > +            - const: phy0_crr
> > +            - const: ctl0_crr
> > +            - const: misc_crr
> > +        power-domains:
> > +          minItems: 1
> 
> Should be maxItems? min is already 1.
> 
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - fsl,imx8qm-hsio
> > +    then:
> > +      properties:
> > +        clock-names:
> > +          items:
> > +            - const: pclk0
> > +            - const: pclk1
> > +            - const: apb_pclk0
> > +            - const: apb_pclk1
> > +            - const: pclk2
> > +            - const: epcs_tx
> > +            - const: epcs_rx
> > +            - const: apb_pclk2
> > +            - const: phy0_crr
> > +            - const: phy1_crr
> > +            - const: ctl0_crr
> > +            - const: ctl1_crr
> > +            - const: ctl2_crr
> > +            - const: misc_crr
> > +        power-domains:
> > +          minItems: 2
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8-clock.h>
> > +    #include <dt-bindings/clock/imx8-lpcg.h>
> > +    #include <dt-bindings/firmware/imx/rsrc.h>
> > +    #include <dt-bindings/phy/phy-imx8-pcie.h>
> > +
> > +    hsio_phy@5f1a0000 {
> 
> phy@...
> 
> > +        compatible = "fsl,imx8qxp-hsio";
> > +        reg = <0x5f1a0000 0x10000>,
> > +              <0x5f120000 0x10000>,
> > +              <0x5f140000 0x10000>,
> > +              <0x5f160000 0x10000>;
> > +        reg-names = "reg", "phy", "ctrl", "misc";
> > +        clocks = <&phyx1_lpcg IMX_LPCG_CLK_0>,
> > +                 <&phyx1_lpcg IMX_LPCG_CLK_4>,
> > +                 <&phyx1_crr1_lpcg IMX_LPCG_CLK_4>,
> > +                 <&pcieb_crr3_lpcg IMX_LPCG_CLK_4>,
> > +                 <&misc_crr5_lpcg IMX_LPCG_CLK_4>;
> > +        clock-names = "pclk0", "apb_pclk0", "phy0_crr", "ctl0_crr",
> "misc_crr";
> > +        power-domains = <&pd IMX_SC_R_SERDES_1>;
> > +        #phy-cells = <3>;
> > +        fsl,hsio-cfg = <IMX8Q_HSIO_CFG_PCIEB>;
> > +        fsl,refclk-pad-mode = "input";
> > +    };
> > +...
> > --
> > 2.37.1
> >