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
