linux-kernel - Re: [PATCH v5 1/2] dt-bindings: phy: google: Add Google Tensor G5 USB PHY

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251030-cunning-copper-jellyfish-1b5f3b@kuoka>
Date: Thu, 30 Oct 2025 08:37:48 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Roy Luo <royluo@...gle.com>
Cc: Vinod Koul <vkoul@...nel.org>, 
	Kishon Vijay Abraham I <kishon@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Peter Griffin <peter.griffin@...aro.org>, 
	André Draszik <andre.draszik@...aro.org>, Tudor Ambarus <tudor.ambarus@...aro.org>, 
	Doug Anderson <dianders@...gle.com>, Joy Chakraborty <joychakr@...gle.com>, 
	Naveen Kumar <mnkumar@...gle.com>, Badhri Jagan Sridharan <badhri@...gle.com>, 
	linux-phy@...ts.infradead.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v5 1/2] dt-bindings: phy: google: Add Google Tensor G5
 USB PHY

On Wed, Oct 29, 2025 at 09:40:31PM +0000, Roy Luo wrote:
> Document the device tree bindings for the USB PHY interfaces integrated
> with the DWC3 controller on Google Tensor SoCs, starting with G5
> generation. The USB PHY on Tensor G5 includes two integrated Synopsys
> PHY IPs: the eUSB 2.0 PHY IP and the USB 3.2/DisplayPort combo PHY IP.
> 
> Due to a complete architectural overhaul in the Google Tensor G5, the
> existing Samsung/Exynos USB PHY binding for older generations of Google
> silicons such as gs101 are no longer compatible, necessitating this new
> device tree binding.
> 
> Signed-off-by: Roy Luo <royluo@...gle.com>
> ---
>  .../bindings/phy/google,gs5-usb-phy.yaml      | 127 ++++++++++++++++++
>  1 file changed, 127 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/google,gs5-usb-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/google,gs5-usb-phy.yaml b/Documentation/devicetree/bindings/phy/google,gs5-usb-phy.yaml
> new file mode 100644
> index 000000000000..8a590036fbac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/google,gs5-usb-phy.yaml
> @@ -0,0 +1,127 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2025, Google LLC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/google,gs5-usb-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Tensor Series (G5+) USB PHY
> +
> +maintainers:
> +  - Roy Luo <royluo@...gle.com>
> +
> +description: |
> +  Describes the USB PHY interfaces integrated with the DWC3 USB controller on
> +  Google Tensor SoCs, starting with the G5 generation.
> +  Two specific PHY IPs from Synopsys are integrated, including eUSB 2.0 PHY IP
> +  and USB 3.2/DisplayPort combo PHY IP.
> +
> +properties:
> +  compatible:
> +    const: google,gs5-usb-phy
> +
> +  reg:
> +    items:
> +      - description: USB3.2/DisplayPort combo PHY core registers.
> +      - description: USB3.2/DisplayPort combo PHY Type-C Assist registers.
> +      - description: USB2 PHY configuration registers.
> +      - description: USB3.2/DisplayPort combo PHY top-level registers.
> +
> +  reg-names:
> +    items:
> +      - const: usb3_core
> +      - const: usb3_tca
> +      - const: usb2_cfg
> +      - const: usb3_top

These prefixes are redundant. Also, you are still referencing here
completely different devices. MMIO of IP blocks do not have size of 0xc
and they do not span over other blocks (0x0c410000 and then next one is
0x0c637000).


> +            reg = <0 0x0c410000 0 0x20000>,
> +                  <0 0x0c430000 0 0x1000>,
> +                  <0 0x0c450014 0 0xc>,
> +                  <0 0x0c637000 0 0xa0>;
> +
> +  "#phy-cells":
> +    description: |
> +      The phandle's argument in the PHY specifier selects one of the three
> +      following PHY interfaces.
> +      - 0 for USB high-speed.
> +      - 1 for USB super-speed.
> +      - 2 for DisplayPort.
> +    const: 1
> +
> +  clocks:
> +    minItems: 2
> +    items:
> +      - description: USB2 PHY clock.
> +      - description: USB2 PHY APB clock.
> +      - description: USB3.2/DisplayPort combo PHY clock.
> +      - description: USB3.2/DisplayPort combo PHY firmware clock.
> +    description:
> +      USB3 clocks are optional if the device is intended for USB2-only
> +      operation.

No, they are not. SoCs are not made that internal wires are being
disconnected when you solder it to a different board.

I stopped reviewing here.

You are making unusual, unexpected big changes after v4. At v4 you
received only few nits because the review process was about to finish.

Now you rewrite everything so you ask me to re-review from scratch.

> +
> +  clock-names:
> +    minItems: 2
> +    items:
> +      - const: usb2
> +      - const: usb2_apb
> +      - const: usb3
> +      - const: usb3_fw

Again, what's with the prefixes? apb is bus clock, so how you could have
bus clock for usb2 and usb3? This means you have two busses, so two
devices.

> +
> +  resets:
> +    minItems: 2
> +    items:
> +      - description: USB2 PHY reset.
> +      - description: USB2 PHY APB reset.
> +      - description: USB3.2/DisplayPort combo PHY reset.
> +    description:
> +      USB3 clocks are optional if the device is intended for USB2-only
> +      operation.
> +
> +  reset-names:
> +    minItems: 2
> +    items:
> +      - const: usb2
> +      - const: usb2_apb
> +      - const: usb3

And here?

This all looks like downstream code.

Best regards,
Krzysztof