[PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY
Krzysztof Kozlowski
krzysztof.kozlowski at linaro.org
Wed May 31 11:29:49 PDT 2023
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 at realtek.com>
> ---
> 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 at realtek.com>
> +
> +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 at 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 at 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 at realtek.com>
> +
> +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
More information about the linux-phy
mailing list