[2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

  Document the M31 USB2 phy present in IPQ5332

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
  

On Wed, 07 Jun 2023 16:26:06 +0530, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,m31.example.dtb: hs_m31phy@5b00: status:0: 'ok' is not one of ['okay', 'disabled', 'reserved']
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/14f60578e2935c0844537eab162af3afa52ffe39.1686126439.git.quic_varada@quicinc.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 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332

Full stop.

> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> new file mode 100644
> index 0000000..8ad4ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +

Drop stray blank lines.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm M31 USB PHY

What is M31?

> +
> +maintainers:
> +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> +  - Varadarajan Narayanan <quic_varada@quicinc.org>
> +
> +description:
> +  This file contains documentation for the USB M31 PHY found in Qualcomm

Drop redundant parts ("This file contains documentation for the").

> +  IPQ5018, IPQ5332 SoCs.
> +
> +properties:
> +  compatible:
> +    oneOf:

That's not oneOf.

> +      - items:

No items.

> +          - enum:
> +              - qcom,m31-usb-hsphy

I am confused what's this. If m31 is coming from some IP block provider,
then you are using wrong vendor prefix.
https://www.m31tech.com/download_file/M31_USB.pdf


> +              - qcom,ipq5332-m31-usb-hsphy

This confuses me even more. IPQ m31?

> +
> +  reg:
> +    description:
> +      Offset and length of the M31 PHY register set

Drop description, obvious.

> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: m31usb_phy_base
> +      - const: qscratch_base

Drop "_base" from both.

> +
> +  phy_type:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - utmi
> +              - ulpi

This does not belong to phy, but to USB node.

> +
> +  resets:
> +    maxItems: 1
> +    description:
> +      List of phandles and reset pairs, one for each entry in reset-names.

Drop useless description.

> +
> +  reset-names:
> +    items:
> +      - const:
> +          usb2_phy_reset

Drop entire reset-names.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> +    hs_m31phy_0: hs_m31phy@5b00 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, no underscores in node names.

> +        compatible = "qcom,m31-usb-hsphy";
> +        reg = <0x5b000 0x3000>,

This is not what your unit address is saying.

> +            <0x08af8800 0x400>;

Align it.

> +        reg-names = "m31usb_phy_base",
> +                "qscratch_base";

Align it.

> +        phy_type = "utmi";
> +        resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +        reset-names = "usb2_phy_reset";
> +
> +        status = "ok";

Drop.

> +    };

Best regards,
Krzysztof
  

On Wed, Jun 07, 2023 at 08:31:50PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Document the M31 USB2 phy present in IPQ5332
>
> Full stop.

Ok.

> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > new file mode 100644
> > index 0000000..8ad4ba4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
>
> Drop stray blank lines.

Ok.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm M31 USB PHY
>
> What is M31?

M31 is an external IP integrated into IPQ5332 SoC.

> > +
> > +maintainers:
> > +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > +  - Varadarajan Narayanan <quic_varada@quicinc.org>
> > +
> > +description:
> > +  This file contains documentation for the USB M31 PHY found in Qualcomm
>
> Drop redundant parts ("This file contains documentation for the").

Ok.

> > +  IPQ5018, IPQ5332 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
>
> That's not oneOf.

Ok.

> > +      - items:
>
> No items.

Ok.

> > +          - enum:
> > +              - qcom,m31-usb-hsphy
>
> I am confused what's this. If m31 is coming from some IP block provider,
> then you are using wrong vendor prefix.
> https://www.m31tech.com/download_file/M31_USB.pdf
>
>
> > +              - qcom,ipq5332-m31-usb-hsphy
>
> This confuses me even more. IPQ m31?

Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
Will that be acceptable?

> > +
> > +  reg:
> > +    description:
> > +      Offset and length of the M31 PHY register set
>
> Drop description, obvious.

Ok.

> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: m31usb_phy_base
> > +      - const: qscratch_base
>
> Drop "_base" from both.

Ok. Will drop qscratch_base. This is in the controller space.
Should not come here.

> > +
> > +  phy_type:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - utmi
> > +              - ulpi
>
> This does not belong to phy, but to USB node.

This is used by the driver to set a bit during phy init. Hence
have it as a replication of the USB node's entry. If this is not
permissible, is there some way to get this from the USB node,
or any other alternative mechanism?

> > +
> > +  resets:
> > +    maxItems: 1
> > +    description:
> > +      List of phandles and reset pairs, one for each entry in reset-names.
>
> Drop useless description.

Ok.

> > +
> > +  reset-names:
> > +    items:
> > +      - const:
> > +          usb2_phy_reset
>
> Drop entire reset-names.

Ok.

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > +    hs_m31phy_0: hs_m31phy@5b00 {
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> Also, no underscores in node names.

Will change this as usbphy0:hs_m31phy@7b000

> > +        compatible = "qcom,m31-usb-hsphy";
> > +        reg = <0x5b000 0x3000>,
>
> This is not what your unit address is saying.

Will change to 0x0007b000.

> > +            <0x08af8800 0x400>;
>
> Align it.

Will drop this. This is in the controller space.

> > +        reg-names = "m31usb_phy_base",
> > +                "qscratch_base";
>
> Align it.

Ok.

> > +        phy_type = "utmi";
> > +        resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > +        reset-names = "usb2_phy_reset";
> > +
> > +        status = "ok";
>
> Drop.

Ok. Hopefully will get an alternative for the phy_type.

Thanks
Varada

> > +    };
>
> Best regards,
> Krzysztof
>
  

On 15/06/2023 07:27, Varadarajan Narayanan wrote:
>>> +          - enum:
>>> +              - qcom,m31-usb-hsphy
>>
>> I am confused what's this. If m31 is coming from some IP block provider,
>> then you are using wrong vendor prefix.
>> https://www.m31tech.com/download_file/M31_USB.pdf
>>
>>
>>> +              - qcom,ipq5332-m31-usb-hsphy
>>
>> This confuses me even more. IPQ m31?
> 
> Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> Will that be acceptable?

m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
device have some name/version/model? If it is not really known, then I
would just propose to go with qcom,ipq5332-usb-hsphy.

Skip generic compatible ("usb-hsphy") entirely.

And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
something similar with difference in the hyphen. Just use device
specific compatible thus device specific filename.

> 
>>> +
>>> +  reg:
>>> +    description:
>>> +      Offset and length of the M31 PHY register set
>>
>> Drop description, obvious.
> 
> Ok.
> 
>>> +    maxItems: 2
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: m31usb_phy_base
>>> +      - const: qscratch_base
>>
>> Drop "_base" from both.
> 
> Ok. Will drop qscratch_base. This is in the controller space.
> Should not come here.

Then drop reg-names entirely.

> 
>>> +
>>> +  phy_type:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - utmi
>>> +              - ulpi
>>
>> This does not belong to phy, but to USB node.
> 
> This is used by the driver to set a bit during phy init. Hence
> have it as a replication of the USB node's entry. If this is not
> permissible, is there some way to get this from the USB node,
> or any other alternative mechanism?

Shouldn't USB controller choose what type of PHY type it wants?

> 
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>>> +    hs_m31phy_0: hs_m31phy@5b00 {
>>
>> Node names should be generic. See also explanation and list of examples
>> in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> Also, no underscores in node names.
> 
> Will change this as usbphy0:hs_m31phy@7b000

This does not solve my comments. I did not write "label" but "node name".

Best regards,
Krzysztof
  

On Sat, Jun 17, 2023 at 10:48:41AM +0200, Krzysztof Kozlowski wrote:
> On 15/06/2023 07:27, Varadarajan Narayanan wrote:
> >>> +          - enum:
> >>> +              - qcom,m31-usb-hsphy
> >>
> >> I am confused what's this. If m31 is coming from some IP block provider,
> >> then you are using wrong vendor prefix.
> >> https://www.m31tech.com/download_file/M31_USB.pdf
> >>
> >>
> >>> +              - qcom,ipq5332-m31-usb-hsphy
> >>
> >> This confuses me even more. IPQ m31?
> >
> > Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> > Will that be acceptable?
>
> m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
> device have some name/version/model? If it is not really known, then I
> would just propose to go with qcom,ipq5332-usb-hsphy.
>
> Skip generic compatible ("usb-hsphy") entirely.

Ok.

> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> something similar with difference in the hyphen. Just use device
> specific compatible thus device specific filename.

qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
driver we are introducing is for UTMI. We would have to
modify phy-qcom-usb-hs.c to accomodate M31. Will that be
acceptable to phy-qcom-usb-hs.c owners/maintainers?

> >>> +
> >>> +  reg:
> >>> +    description:
> >>> +      Offset and length of the M31 PHY register set
> >>
> >> Drop description, obvious.
> >
> > Ok.
> >
> >>> +    maxItems: 2
> >>> +
> >>> +  reg-names:
> >>> +    items:
> >>> +      - const: m31usb_phy_base
> >>> +      - const: qscratch_base
> >>
> >> Drop "_base" from both.
> >
> > Ok. Will drop qscratch_base. This is in the controller space.
> > Should not come here.
>
> Then drop reg-names entirely.

Ok.

> >>> +
> >>> +  phy_type:
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - enum:
> >>> +              - utmi
> >>> +              - ulpi
> >>
> >> This does not belong to phy, but to USB node.
> >
> > This is used by the driver to set a bit during phy init. Hence
> > have it as a replication of the USB node's entry. If this is not
> > permissible, is there some way to get this from the USB node,
> > or any other alternative mechanism?
>
> Shouldn't USB controller choose what type of PHY type it wants?

Will remove this. IPQ5332 uses it in UTMI mode only.

> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> >>> +    hs_m31phy_0: hs_m31phy@5b00 {
> >>
> >> Node names should be generic. See also explanation and list of examples
> >> in DT specification:
> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >>
> >> Also, no underscores in node names.
> >
> > Will change this as usbphy0:hs_m31phy@7b000
>
> This does not solve my comments. I did not write "label" but "node name".

Sorry. will fix it.

Thanks
Varada
  

On 20/06/2023 11:32, Varadarajan Narayanan wrote:

> 
>> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
>> something similar with difference in the hyphen. Just use device
>> specific compatible thus device specific filename.
> 
> qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> driver we are introducing is for UTMI. We would have to
> modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> acceptable to phy-qcom-usb-hs.c owners/maintainers?

We don't talk about drivers here but bindings. Why would you need to
modify the driver when introducing new binding for different device?



Best regards,
Krzysztof
  

On Wed, Jun 21, 2023 at 10:43:38AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 11:32, Varadarajan Narayanan wrote:
>
> >
> >> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> >> something similar with difference in the hyphen. Just use device
> >> specific compatible thus device specific filename.
> >
> > qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> > driver we are introducing is for UTMI. We would have to
> > modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> > acceptable to phy-qcom-usb-hs.c owners/maintainers?
>
> We don't talk about drivers here but bindings. Why would you need to
> modify the driver when introducing new binding for different device?

Sorry. I misunderstood your feedback as "use the existing bindings".

Will name the bindings file as qcom,ipq5332-usb-hsphy.yaml and post
the next version.

Thanks
Varada