Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

From: Krzysztof Kozlowski
Date: Wed May 31 2023 - 14:30:01 EST

Next message: Jim Mattson: "Re: [PATCH v4] KVM: VMX: do not disable interception for MSR_IA32_SPEC_CTRL on eIBRS"
Previous message: Qais Yousef: "Re: [RESEND][PATCH v2 1/3] sched/tp: Add new tracepoint to track uclamp set from user-space"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY"
Next in thread: Stanley Chang: "[PATCH v2 2/3] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0/3.0 PHY"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 25/05/2023 04:26, Stanley Chang wrote:
> Two documentations explain the property about realtek USB PHY drivers.
>

Thank you for your patch. There is something to discuss/improve.

Actually a lot... The bindings are not suitable for review.

> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB 2.0/3.0
> controller. Added two drivers to drive the USB 2.0/3.0 PHY transceivers.
> For USB 3.0 transceivers, a driver phy-rtk-usb3 is provided.
> The driver phy-rtk-usb2 is used to support USB 2.0 transceivers.
>
> Signed-off-by: Stanley Chang <stanley_chang@xxxxxxxxxxx>
> ---
> v1 to v2 change:
> Add phy-cells for generic phy driver
> ---
> .../bindings/phy/realtek,usb2phy.yaml | 255 ++++++++++++++++++
> .../bindings/phy/realtek,usb3phy.yaml | 201 ++++++++++++++
> 2 files changed, 456 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> new file mode 100644
> index 000000000000..a2a69da0a163
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> @@ -0,0 +1,255 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb2phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB2 PHY
> +
> +maintainers:
> + - Stanley Chang <stanley_chang@xxxxxxxxxxx>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> + Realtek USB 2.0 PHY support the digital home center (DHC) RTD series SoCs.
> +
> +properties:
> + compatible:
> + enum:
> + - realtek,usb2phy

As pointed out by Conor, this does not make sense.

> + - realtek,rtd-usb2phy
> + - realtek,rtd1295-usb2phy
> + - realtek,rtd1395-usb2phy
> + - realtek,rtd1619-usb2phy
> + - realtek,rtd1319-usb2phy
> + - realtek,rtd1619b-usb2phy
> + - realtek,rtd1312c-usb2phy
> + - realtek,rtd1319d-usb2phy
> + - realtek,rtd1315e-usb2phy
> +
> + reg:
> + items:
> + - description: PHY data registers
> + - description: PHY control registers
> +
> + "#phy-cells":
> + const: 0
> +
> + realtek,usb:
> + description: The phandler of realtek dwc3 node

"phandler"? Except obvious typo, drop "The phandler of" and describe
what is it for.

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

Anyway, it shouldn't be here. No, no.

> +
> + realtek,mac:
> + description: The phandler of dwc3 node
> + $ref: /schemas/types.yaml#/definitions/phandle

NAK.

> +
> + realtek,usb_ctrl:
> + description: The phandler of usb power control node
> + $ref: /schemas/types.yaml#/definitions/phandle

NAK for similar reasons (nothing here justifies it existence). Also, do
not use underscores in node names.

> +
> + realtek,port-index:
> + description: The index of USB 2.0 PHY
> + $ref: /schemas/types.yaml#/definitions/uint32

No. No reason for this. You have reg.

> +
> + realtek,phyN:
> + description: The total amount of USB 2.0 PHY
> + $ref: /schemas/types.yaml#/definitions/uint32

No. Compatible defines it.

> +
> + phy0:
> + description: The child node of PHY for the parameter v1.

??? Open other phy bindings and use them as example.

> + type: object
> + properties:
> + realtek,phy-data-page0-size:
> + description: PHY data page 0 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,phy-data-page0-addr:
> + description: PHY data page 0 address
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,phy-data-page0-A00:
> + description: PHY data page 0 value
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,phy-data-page1-size:
> + description: PHY data page 1 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,phy-data-page1-addr:
> + description: PHY data page 1 address
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,phy-data-page1-A00:
> + description: PHY data page 1 value
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,phy-data-page2-size:
> + description: PHY data page 2 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,phy-data-page2-addr:
> + description: PHY data page 2 address
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,phy-data-page2-A00:
> + description: PHY data page 2 value
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> + realtek,do-toggle:
> + description: Do PHY parameter toggle when port status change
> + type: boolean
> +
> + realtek,check-efuse:
> + description: Enable to fix PHY parameter from reading otp table
> + type: boolean
> +
> + realtek,use-default-parameter:
> + description: Don't set parameter and use default value
> + type: boolean
> +
> + realtek,is-double-sensitivity-mode:
> + description: Enable double sensitivity mode
> + type: boolean
> +
> + realtek,ldo-page0-e4-compensate:
> + description: Adjust the PHY parameter for page 0 0xE4 for ldo mode
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,page0-e4-compensate:
> + description: Adjust the PHY parameter for page 0 0xE4
> + for efuse table v2
> + $ref: /schemas/types.yaml#/definitions/uint32

I don't understand what's all this for. Most of these descriptions do
not explain anything except duplicating name of property.

> +
> + phy0_data:
> + description: The child node of PHY for parameter v2.

Even more question marks. We are getting close to what the hell is this
binding?

> + type: object
> + properties:
> + realtek,page0-size:
> + description: PHY data page 0 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,page0-data-A00:
> + description: PHY data page 0 address and value
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> + realtek,page1-size:
> + description: PHY data page 1 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,page1-data-A00:
> + description: PHY data page 1 address and value
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> + realtek,page2-size:
> + description: PHY data page 2 size
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,page2-data-A00:
> + description: PHY data page 2 address and value
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> + realtek,do-toggle:
> + description: Do PHY parameter toggle when port status change
> + type: boolean
> +
> + realtek,do-toggle-driving:
> + description: Do PHY parameter toggle for driving when port
> + status change
> + type: boolean
> +
> + realtek,check-efuse:
> + description: Enable to fix PHY parameter from reading otp table
> + type: boolean
> +
> + realtek,use-default-parameter:
> + description: Don't set parameter and use default value
> + type: boolean
> +
> + realtek,is-double-sensitivity-mode:
> + description: Enable double sensitivity mode
> + type: boolean
> +
> + realtek,ldo-force-enable:
> + description: Force enable ldo mode
> + type: boolean
> +
> + realtek,ldo-page0-e4-compensate:
> + description: Adjust the PHY parameter for page0 0xE4 for ldo mode
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + realtek,page0-e4-compensate:
> + description: Adjust the PHY parameter for page0 0xE4
> + for efuse table v2
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> + - compatible
> + - reg
> + - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + dwc3_u3drd_usb2phy: dwc3_u3drd_usb2phy@98013e14 {

You must be joking with the node name.

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

Make it proper DTS, not some downstream unacceptable code.

> + compatible = "realtek,usb2phy";
> + reg = <0x98013e14 0x4>, <0x98058280 0x4>;
> + #phy-cells = <0>;
> + status = "okay";

Drop.

> + realtek,phyN = <1>;
> +
> + phy0 {

Why not phy9999?

> + realtek,phy-data-page0-size = <16>;
> + realtek,phy-data-page0-addr = /bits/ 8
> + <0xE0 0xE1 0xE2 0xE3 0xE4 0xE5 0xE6 0xE7 0xF0 0xF1

lowercase hex

> + 0xF2 0xF3 0xF4 0xF5 0xF6 0xF7>;
> + realtek,phy-data-page0-A00 = /bits/ 8
> + <0xE0 0x30 0x79 0x8D 0x6A 0x65 0x01 0x71 0xFC 0x8C
> + 0x00 0x11 0x9B 0x00 0x00 0x0A>;
> + realtek,phy-data-page0-B00 = /bits/ 8
> + <0x18 0x30 0x79 0x8D 0x6A 0x65 0x01 0x71 0xFC 0x8C
> + 0x00 0x11 0x9B 0x00 0x00 0x32>;
> + realtek,phy-data-page1-size = <8>;
> + realtek,phy-data-page1-addr = /bits/ 8
> + <0xE0 0xE1 0xE2 0xE3 0xE4 0xE5 0xE6 0xE7>;
> + realtek,phy-data-page1-A00 = /bits/ 8
> + <0x25 0xEF 0x60 0x44 0x00 0x0F 0x18 0xE3>;
> + realtek,phy-data-page2-size = <1>;
> + realtek,phy-data-page2-addr = /bits/ 8
> + <0xE0>;
> + realtek,phy-data-page2-A00 = /bits/ 8
> + <0x01>;
> + realtek,do-toggle;
> + realtek,check-efuse;
> + realtek,is-double-sensitivity-mode;
> + realtek,ldo-page0-e4-compensate = <(-2)>;
> + };
> + };
> +
> + - |
> + usb_port0_usb2phy: usb_port0_usb2phy@13214 {
> + compatible = "realtek,usb2phy";
> + reg = <0x13214 0x4>, <0x28280 0x4>;
> + #phy-cells = <0>;
> + realtek,usb = <&usb_port0>;
> + realtek,mac = <&port0_dwc3>;
> + realtek,usb_ctrl = <&usb_ctrl>;
> +
> + realtek,port-index = <0>;
> + realtek,phyN = <1>;
> + phy0_data {
> + realtek,page0-size = <16>;
> + realtek,page0-data-A00 = /* < addr data > */
> + <0xE0 0xA3>, <0xE4 0xB2>, <0xE5 0x4F>, <0xE6 0x42>;
> + realtek,page1-size = <8>;
> + realtek,page1-data-A00 = <0xE3 0x64>;
> + realtek,page2-size = <8>;
> + realtek,page2-data-A00 = <0xE7 0x45>;
> + realtek,do-toggle;
> + realtek.do-toggle-driving;
> + realtek,disconnect-driving-updated = <0x8>;
> + realtek,check-efuse;
> + realtek,is-double-sensitivity-mode;
> + realtek,ldo-force-enable;
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> new file mode 100644
> index 000000000000..2d2543acfb5d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> @@ -0,0 +1,201 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb3phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB3 PHY
> +
> +maintainers:
> + - Stanley Chang <stanley_chang@xxxxxxxxxxx>
> +
> +description: |
> + Realtek USB 3.0 PHY support the digital home center (DHC) RTD series SoCs.
> +
> +properties:
> + compatible:
> + enum:
> + - realtek,usb3phy
> + - realtek,rtd-usb3phy
> + - realtek,rtd1295-usb3phy
> + - realtek,rtd1619-usb3phy
> + - realtek,rtd1319-usb3phy
> + - realtek,rtd1619b-usb3phy
> + - realtek,rtd1319d-usb3phy

Does not make sense...
> +
> + reg:
> + maxItems: 1
> +
> + "#phy-cells":
> + const: 0

One huge NAK for these bindings. It looks like copy-paste from
downstream stuff which should never be sent as is to upstream. I am
sorry for being harsh, but amount of questions, coding and naming
styles, incorrect choices is just too big to handle in one review.

Best regards,
Krzysztof

Next message: Jim Mattson: "Re: [PATCH v4] KVM: VMX: do not disable interception for MSR_IA32_SPEC_CTRL on eIBRS"
Previous message: Qais Yousef: "Re: [RESEND][PATCH v2 1/3] sched/tp: Add new tracepoint to track uclamp set from user-space"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY"
Next in thread: Stanley Chang: "[PATCH v2 2/3] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0/3.0 PHY"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]