Message-ID: <20251015-backlash-overtime-4c636f12b165@spud>
Date: Wed, 15 Oct 2025 09:59:42 +0100
From: Conor Dooley <conor@...nel.org>
To: Roy Luo <royluo@...gle.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, 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>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Peter Griffin <peter.griffin@...aro.org>,
	André Draszik <andre.draszik@...aro.org>,
	Tudor Ambarus <tudor.ambarus@...aro.org>,
	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-usb@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v3 1/4] dt-bindings: usb: dwc3: Add Google Tensor G5 DWC3

On Tue, Oct 14, 2025 at 05:50:17PM -0700, Roy Luo wrote:
> On Tue, Oct 14, 2025 at 1:22 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >
> > On 14/10/2025 03:40, Roy Luo wrote:
> > > On Fri, Oct 10, 2025 at 5:09 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > >>
> > >> On 10/10/2025 22:16, Roy Luo wrote:
> > >>> Document the device tree bindings for the DWC3 USB controller found in
> > >>> Google Tensor SoCs, starting with the G5 generation.
> > >>>
> > >>> The Tensor G5 silicon represents a complete architectural departure from
> > >>> previous generations (like gs101), including entirely new clock/reset
> > >>> schemes, top-level wrapper and register interface. Consequently,
> > >>> existing Samsung/Exynos DWC3 USB bindings are incompatible, necessitating
> > >>> this new device tree binding.
> > >>>
> > >>> The USB controller on Tensor G5 is based on Synopsys DWC3 IP and features
> > >>> Dual-Role Device single port with hibernation support.
> > >>
> > >> You still mix, completely unnecessarily, subsystems. For Greg this is
> > >> actually even undesired, but regardless don't do this for any cases
> > >> because it just makes everything slower or more difficult to apply.
> > >>
> > >> Really, think how maintainers should deal with your patches.
> > >>
> > >
> > > Understood, I will separate the patches into two distinct series: one for
> > > the controller and one for the PHY.
> > > Appreciate the feedback and the explanation.
> > >
> > >>>
> > >>> Signed-off-by: Roy Luo <royluo@...gle.com>
> > >>> ---
> > >>>  .../bindings/usb/google,gs5-dwc3.yaml         | 141 ++++++++++++++++++
> > >>>  1 file changed, 141 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/usb/google,gs5-dwc3.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/usb/google,gs5-dwc3.yaml b/Documentation/devicetree/bindings/usb/google,gs5-dwc3.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..6fadea7f41e8
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/usb/google,gs5-dwc3.yaml
> > >>> @@ -0,0 +1,141 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > >>> +# Copyright (c) 2025, Google LLC
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/usb/google,gs5-dwc3.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: Google Tensor Series (G5+) DWC3 USB SoC Controller
> > >>> +
> > >>> +maintainers:
> > >>> +  - Roy Luo <royluo@...gle.com>
> > >>> +
> > >>> +description:
> > >>> +  Describes the DWC3 USB controller block implemented on Google Tensor SoCs,
> > >>> +  starting with the G5 generation. Based on Synopsys DWC3 IP, the controller
> > >>> +  features Dual-Role Device single port with hibernation add-on.
> > >>> +
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    const: google,gs5-dwc3
> > >>> +
> > >>> +  reg:
> > >>> +    items:
> > >>> +      - description: Core DWC3 IP registers.
> > >>> +      - description: USB host controller configuration registers.
> > >>> +      - description: USB custom interrrupts control registers.
> > >>> +
> > >>> +  reg-names:
> > >>> +    items:
> > >>> +      - const: dwc3_core
> > >>> +      - const: host_cfg
> > >>> +      - const: usbint_cfg
> > >>> +
> > >>> +  interrupts:
> > >>> +    items:
> > >>> +      - description: Core DWC3 interrupt.
> > >>> +      - description: High speed power management event for remote wakeup from hibernation.
> > >>> +      - description: Super speed power management event for remote wakeup from hibernation.
> > >>
> > >> Wrap at 80 (see coding style) or just shorten these.
> > >
> > > Ack, will fix it in the next patch.
> > >
> > >>
> > >>> +
> > >>> +  interrupt-names:
> > >>> +    items:
> > >>> +      - const: dwc_usb3
> > >>
> > >> So just "core"?
> > >
> > > I'd prefer to stick to "dwc_usb3" as that's
> > > 1. more expressive by referring to the underlying IP name,
> >
> >
> > But that's completely redundant name.
> >
> > > 2. consistent with established dwc3 bindings such as
> > >     Documentation/devicetree/bindings/usb/snps,dwc3.yaml,
> >
> > If you use only one interrupt. You don't use one interrupt here.
> 
> After looking into it further, I found that the interrupt name "dwc_usb3"
> must be used here to adhere to the interrupt naming defined in
> "snps,dwc3.yaml".

Did you just chose to not read what Krzysztof said here? It must be used
only when that's the sole interrupt, which he stated is not the case for
your platform.

> This requirement stems from the device's corresponding glue driver
> utilizing a so-called "flattened" model (see [1] for context). This model
> causes the glue driver to probe an underlying "snps,dwc3" device.
> Consequently, the core DWC3 interrupt defined here is consumed by
> the driver handling the "snps,dwc3" device, making it mandatory to
> follow the interrupt naming established in "snps,dwc3.yaml".

I look at the binding and noticed that interrupt-names isn't even a
required property by snps,dwc3.yaml, and this comment about driver
behaviour likely isn't accurate given that the code in for host mode
(and the others are identical) is written so that it will grab the first
interrupt if the specific names it looks for are absent:
| static int dwc3_host_get_irq(struct dwc3 *dwc)
| {
| 	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
| 	int irq;
| 
| 	irq = platform_get_irq_byname_optional(dwc3_pdev, "host");
| 	if (irq > 0) {
| 		dwc3_host_fill_xhci_irq_res(dwc, irq, "host");
| 		goto out;
| 	}
| 
| 	if (irq == -EPROBE_DEFER)
| 		goto out;
| 
| 	irq = platform_get_irq_byname_optional(dwc3_pdev, "dwc_usb3");
| 	if (irq > 0) {
| 		dwc3_host_fill_xhci_irq_res(dwc, irq, "dwc_usb3");
| 		goto out;
| 	}
| 
| 	if (irq == -EPROBE_DEFER)
| 		goto out;
| 
| 	irq = platform_get_irq(dwc3_pdev, 0);
| 	if (irq > 0)
| 		dwc3_host_fill_xhci_irq_res(dwc, irq, NULL);
| 
| out:
| 	return irq;
| }

Since it grabs the first interrupt, as a fallback, in order to support
not having the interrupt-names property, the name of the first interrupt
ultimately doesn't matter at all to this driver (and likely any other
driver written in compliance with the bindings for the dwc3 core).

I'm not here to argue about what the name for the single interrupt
should be (keeping consistency with other devices might actually be
good), but ignoring what a maintainer says and the seemingly providing
an incorrect analysis is annoying. Did you perform the analysis on this
yourself, or did it perhaps come from Gemini?

Thanks,
Conor.

> Essentially, the interrupts defined here are a mix of vendor specific
> implementation (like "hs_pme", "ss_pme") and the DWC3 core in
> "snps,dwc3.yaml" ("dwc_usb3").
> 
> I don't know if there's a better way to express this implicit dependency
> of the core DWC3 interrupt except for documenting it in the binding
> description. Any advice would be welcome.
> 
> [1] https://lore.kernel.org/all/20250414-dwc3-refactor-v7-0-f015b358722d@oss.qualcomm.com/
> 
> Thanks,
> Roy Luo
> 
> >
> > >     Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml,
> > > unless you have a strong preference for the alternative naming.
> >
> > Such namings are discouraged, because they tell absolutely nothing.
> > Also, schematics or datasheets usually do not use them, either.
> >
> >
> > Best regards,
> > Krzysztof

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists