RE: [PATCH v4 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Stanley Chang[昌育德]]

Hi Krzysztof,

> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - realtek,rtd1295-usb2phy
> > +          - realtek,rtd1312c-usb2phy
> > +          - realtek,rtd1315e-usb2phy
> > +          - realtek,rtd1319-usb2phy
> > +          - realtek,rtd1319d-usb2phy
> > +          - realtek,rtd1395-usb2phy
> > +          - realtek,rtd1395-usb2phy-2port
> > +          - realtek,rtd1619-usb2phy
> > +          - realtek,rtd1619b-usb2phy
> > +      - const: realtek,usb2phy
> 
> That's not what your driver is saying... This patchset has random set of
> changes.
> 
> I suggest to drop "realtek,usb2phy".

Okay, I will remove it.

> > +
> > +  reg:
> > +    items:
> > +      - description: PHY data registers
> > +      - description: PHY control registers
> > +
> > +  "#phy-cells":
> > +    const: 0
> > +
> > +  nvmem-cells:
> > +    maxItems: 2
> > +    description:
> > +      Phandles to nvmem cell that contains the trimming data.
> > +      If unspecified, default value is used.
> > +
> > +  nvmem-cell-names:
> > +    items:
> > +      - const: usb-dc-cal
> > +      - const: usb-dc-dis
> > +    description:
> > +      The following names, which correspond to each nvmem-cells.
> > +      usb-dc-cal is the driving level for each phy specified via efuse.
> > +      usb-dc-dis is the disconnection level for each phy specified via efuse.
> > +
> > +  realtek,inverse-hstx-sync-clock:
> > +    description:
> > +      For one of the phys of RTD1619b SoC, the synchronous clock of the
> > +      high-speed tx must be inverted. So this property is used to set the
> > +      inverted clock.
> 
> Drop last sentence, it is redundant.

Okay

> > +    type: boolean
> > +
> > +  realtek,driving-level:
> > +    description:
> > +      Each board or port may have a different driving capability. This
> > +      will adjust the driving level value if the value is not the default.
> 
> I don't understand it. What is "driving capability" and if each port can have it
> different, why do you need property for this?

Sorry. I didn't express myself clearly.
"Driving capability" refers to the magnitude of the Dp/Dm output swing.
For a different board or port, the original magnitude maybe not meet the specification. 
In this situation we can adjust the value to meet the specification.

> You mention some default - why it is not expressed as "default: xx"?

The default is mean hardware default value.
I can add the default value.

> What do the values mean?

The description can be modified to:
    description:
      Control the magnitude of High speed Dp/Dm output swing.
      For a different board or port, the original magnitude maybe not meet
      the specification. In this situation we can adjust the value to meet
      the specification.
    $ref: /schemas/types.yaml#/definitions/uint32
    default: 8
    minimum: 0
    maximum: 31

> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 31
> > +
> > +  realtek,driving-compensate:
> > +    description:
> > +      For RTD1315e SoC, the driving level can be adjusted by reading the
> > +      efuse table. Therefore, this property provides drive compensation
> for
> > +      different boards with different drive capabilities.
> 
> if driving level can be read from nvmem, why do you have realtek,driving-level
> in the first place? Don't you miss here some allOf making this constrained per
> variant?
> 
> "Therefore" means "for that reason" or "as a consequence". How is this
> property a consequence of reading driving level from efuse? Is it then mutually
> exclusive with "realtek,driving-level"? But your schema does not express it.

No, it's not that complicated.
In our case, not all ICs have programming efuse.
The value "realtek,deiving-level" is for non-programmed efuse ICs.

In the programmed efuse IC, we use the value of efuse to instead of "realtek,driving-level". 
If the magnitude of High speed Dp/Dm output swing still not meet the specification,
then we can set the value "driving-compensate" to meet the specification.

The description can be modified to:
  realtek,driving-compensate:
    description:
      For RTD1315e SoC, the driving level can be adjusted by reading the
      efuse table. This property provides drive compensation.
      If the magnitude of High speed Dp/Dm output swing still not meet the
      specification, then we can set this value to meet the specification.
    $ref: /schemas/types.yaml#/definitions/int32
    minimum: -8
    maximum: 8

> > +    $ref: /schemas/types.yaml#/definitions/int32
> > +    minimum: -8
> > +    maximum: 8
> > +
> > +  realtek,disconnection-compensate:
> > +    description:
> > +      This adjusts the disconnection level compensation for the different
> > +      boards with different disconnection level.
> > +    $ref: /schemas/types.yaml#/definitions/int32
> > +    minimum: -8
> > +    maximum: 8
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#phy-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    usb_port0_usb2phy: usb-phy@13214 {
> > +        compatible = "realtek,rtd1319d-usb2phy", "realtek,usb2phy";
> > +        reg = <0x13214 0x4>, <0x28280 0x4>;
> > +        #phy-cells = <0>;
> > +
> > +        realtek,driving-level = <0xe>;
> 
> Why this example is so empty? You have at least 6 more properties which
> should be shown here.

I will add more examples in here.
examples:
  - |
    usb_port0_usb2phy: usb-phy@13214 {
        compatible = "realtek,rtd1319d-usb2phy";
        reg = <0x13214 0x4>, <0x28280 0x4>;
        #phy-cells = <0>;
        nvmem-cells = <&otp_usb_port0_dc_cal>, <&otp_usb_port0_dc_dis>;
        nvmem-cell-names = "usb-dc-cal", "usb-dc-dis";

        realtek,driving-level = <0xe>;
    };

  - |
    port0_usb2phy: usb-phy@13214 {
        compatible = "realtek,rtd1619b-usb2phy";
        reg = <0x13214 0x4>, <0x28280 0x4>;
        #phy-cells = <0>;
        nvmem-cells = <&otp_usb_port0_dc_cal>, <&otp_usb_port0_dc_dis>;
        nvmem-cell-names = "usb-dc-cal", "usb-dc-dis";

        realtek,inverse-hstx-sync-clock;
        realtek,driving-level = <0xa>;
        realtek,driving-compensate = <(-1)>;
        realtek,disconnection-compensate = <(-1)>;
    };

    port1_usb2phy: usb-phy@13c14 {
        compatible = "realtek,rtd1619b-usb2phy";
        reg = <0x13c14 0x4>, <0x31280 0x4>;
        #phy-cells = <0>;
        nvmem-cells = <&otp_usb_port1_dc_cal>, <&otp_usb_port1_dc_dis>;
        nvmem-cell-names = "usb-dc-cal", "usb-dc-dis";

        realtek,driving-level = <8>;
        realtek,driving-compensate = <1>;
        realtek,disconnection-compensate = <0>;
    };

Thanks,
Stanley