Message-ID: <20251030-unbridle-ribbon-5c8543b83b71@spud>
Date: Thu, 30 Oct 2025 18:44:23 +0000
From: Conor Dooley <conor@...nel.org>
To: Christian Marangi <ansuelsmth@...il.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>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	linux-arm-kernel@...ts.infradead.org, linux-phy@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] dt-bindings: phy: Add documentation for Airoha
 AN7581 USB PHY

On Wed, Oct 29, 2025 at 08:11:10PM +0100, Christian Marangi wrote:
> On Wed, Oct 29, 2025 at 06:35:51PM +0000, Conor Dooley wrote:
> > On Wed, Oct 29, 2025 at 07:24:04PM +0100, Christian Marangi wrote:
> > > On Wed, Oct 29, 2025 at 06:07:22PM +0000, Conor Dooley wrote:
> > > > On Wed, Oct 29, 2025 at 06:37:10PM +0100, Christian Marangi wrote:
> > > > > Add documentation for Airoha AN7581 USB PHY that describe the USB PHY
> > > > > for the USB controller.
> > > > > 
> > > > > Airoha AN7581 SoC support a maximum of 2 USB port. The USB 2.0 mode is
> > > > > always supported. The USB 3.0 mode is optional and depends on the Serdes
> > > > > mode currently configured on the system for the relevant USB port.
> > > > > 
> > > > > The first USB port on the SoC can be both used for USB 3.0 operation or
> > > > > Ethernet (HSGMII).
> > > > > The second USB port on the SoC can be both used for USB 3.0 operation or
> > > > > for an additional PCIe line.
> > > > > 
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > > > > ---
> > > > > 
> > > > > For DT maintainers, in v2 there were some comments, hope the new
> > > > > description and names of the property better clarify the usage and
> > > > > why they are needed.
> > > > > 
> > > > >  .../bindings/phy/airoha,an7581-usb-phy.yaml   | 76 +++++++++++++++++++
> > > > >  MAINTAINERS                                   |  7 ++
> > > > >  .../dt-bindings/phy/airoha,an7581-usb-phy.h   | 11 +++
> > > > >  3 files changed, 94 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yaml
> > > > >  create mode 100644 include/dt-bindings/phy/airoha,an7581-usb-phy.h
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yaml b/Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..5106685c124d
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yaml
> > > > > @@ -0,0 +1,76 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/phy/airoha,an7581-usb-phy.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Airoha AN7581 SoC USB PHY
> > > > > +
> > > > > +maintainers:
> > > > > +  - Christian Marangi <ansuelsmth@...il.com>
> > > > > +
> > > > > +description: >
> > > > > +  The Airoha AN7581 SoC USB PHY describes the USB PHY for the USB controller.
> > > > > +
> > > > > +  Airoha AN7581 SoC support a maximum of 2 USB port. The USB 2.0 mode is
> > > > > +  always supported. The USB 3.0 mode is optional and depends on the Serdes
> > > > > +  mode currently configured on the system for the relevant USB port.
> > > > > +
> > > > > +  The first USB port on the SoC can be both used for USB 3.0 operation or
> > > > > +  Ethernet (HSGMII).
> > > > > +  The second USB port on the SoC can be both used for USB 3.0 operation or
> > > > > +  for an additional PCIe line.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: airoha,an7581-usb-phy
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  airoha,usb2-monitor-clk-sel:
> > > > > +    description: Describe what oscillator across the available 4
> > > > > +      should be selected for USB 2.0 Slew Rate calibration.
> > > > 
> > > > Why's this being set in dt? What actually determines what oscillator
> > > > should be used? Do they have different performance characteristics?
> > > > How is someone meant to know which one to use?
> > > >
> > > 
> > > Hi Conor,
> > > 
> > > thanks a lot for the review.
> > > 
> > > The airoha,usb2-monitor-clk-sel is set in DT because it describe the HW
> > > and to what oscillator the PHY should be connected to.
> > > 
> > > There are 2 PHY at different register space. One PHY needs to calibrate
> > > for oscillator 1 and the other PHY for oscillator 2 or the PHY doesn't
> > > work for USB 2.0 (the calibration fails)
> > 
> > This implies that the oscillator is not part of the phy block, if
> > there's multiple competing for the same one. Why are these oscillators
> > not represented as clocks?
> >
> 
> This was also pointed out in the old revision. The oscillator is
> internal and there isn't a register to read the frequency or to
> enable/disable.

If it has come up twice, that's a sure sign that you're not explaining
how this property works correctly. Since it's more about suitability and
less on usage, put it in the commit message.

> The PHY register block have the same exact bits to select from 0 to 3.
> The HW internally is then connected to 1 or 2 basted on the physical PHY.
> 
> So yes they are part of the PHY block, just the register map is too
> ""liberal"" on the configuration.
> 
> I verified if the same oscillator could be used for both but it does end
> up in the port not working as the calibration fails.
> 
> > > The previous implementation used an index property but that was rejected
> > > as it wasn't descriptive of the HW.
> > > 
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [0, 1, 2, 3]
> > > > > +
> > > > > +  airoha,usb3-serdes:
> > > > > +    description: Describe what Serdes line is attached to the USB 3.0 port.
> > > > > +      Can be either Serdes USB1 or Serdes USB2.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [2, 3]
> > > > 
> > > > This is confusing. Why 2 and 3 for usb1 and usb2? What even is the
> > > > mapping? Is it 2:1/3:2 or 2:2/3:1?
> > > > 
> > > 
> > > AFAIK there isn't a way to directly reference dt-bindings.
> > 
> > Usually people put the name of the file in.
> > 
> 
> Yes on this I have seen mixed implementation with only the enum and some
> adding the link to it. I will add in the description.
> 
> > > 
> > > 2 and 3 are from include/dt-bindings/soc/airoha,scu-ssr.h
> > > 
> > > #define AIROHA_SCU_SERDES_USB1		2
> > > #define AIROHA_SCU_SERDES_USB2		3
> > 
> > Why overcomplicate like this? Just use 1 and 2. If that corresponds to
> > register settings of 2 and 3, do that conversion in the driver.
> > Properties are not shoving just register settings etc into, what happens,
> > for example, when the next soc comes along and these are actually usb1 is
> > 3 and usb2 is 4 there? All of a sudden, your property has different
> > meanings depending on which device is being used.
> > 
> 
> This is really just to identify the serdes line that can have multiple
> usage, they don't refer to a specific register.
> 
> The PCIe one are added as they can also be used for PCIe or Ethernet
> usage and in the future this feature will be implemented in the PCIe PHY.
> 
> It's ok to change it 1 but then I hope we won't fall in the monclk case
> where we end up with

What is the "monclk" case?

> airoha,usb3-serdes = <1>;
> 
> as that will be VERY confusing in DT.

Why would this be confusing for whatever "monclk" is?

I think the best argument for keeping it behaving how it does, is that
you've got pcie1 and pcie2 as potential options too.

Honestly, this whole patch comes across as doing weird or not needed
stuff, until the explanations arrive for why you're doing it this way a
few mails later. Just put the explanations for anything non-obvious like
this into either the property description (if usage is what's weird or
non-obvious) or into the commit message (if it's about suitability) and
we'll save a bunch of time in the future ;)


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

Powered by blists - more mailing lists