| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0b2143ca-ead7-c8fa-2e80-a94222af51ca@linaro.org>
Date: Wed, 31 May 2023 20:29:49 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Stanley Chang <stanley_chang@...ltek.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Alan Stern <stern@...land.harvard.edu>,
Flavio Suligoi <f.suligoi@...m.it>,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
Douglas Anderson <dianders@...omium.org>,
Matthias Kaehlcke <mka@...omium.org>,
Ray Chi <raychi@...gle.com>,
Michael Grzeschik <m.grzeschik@...gutronix.de>,
linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the
Realtek SoC USB 2.0/3.0 PHY
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@...ltek.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@...ltek.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@...13e14 {
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@...14 {
> + 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@...ltek.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
Powered by blists - more mailing lists