Re: [PATCH v3 2/4] dt-bindings: phy: Add documentation for Airoha AN7581 USB PHY

[Christian Marangi <ansuelsmth@gmail.com>]

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@gmail.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@gmail.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)

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.

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

> > +
> > +  airoha,scu:
> > +    description: Phandle to the SCU syscon to configure the Serdes line.
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +  '#phy-cells':
> > +    description: Describe if the referred PHY is the USB 2.0 PHY or USB 3.0 PHY.
> > +    const: 1
> 
> Which is which here?
>

Mhh I think I didn't understand here. #phy-cells describe the
parameters to be used for phys property in a different node.

The current usage would be

<&usb0_phy PHY_TYPE_USB2> for USB 2.0
or <&usb0_phy PHY_TYPE_USB3> for USB 3.0

This node expose 2 PHY that can be referenced by the single parameters.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - airoha,usb2-monitor-clk-sel
> > +  - airoha,usb3-serdes
> > +  - airoha,scu
> > +  - '#phy-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/phy/airoha,an7581-usb-phy.h>
> > +    #include <dt-bindings/soc/airoha,scu-ssr.h>
> > +
> > +    phy@1fac0000 {
> > +        compatible = "airoha,an7581-usb-phy";
> > +        reg = <0x1fac0000 0x10000>;
> > +
> > +        airoha,usb2-monitor-clk-sel = <AIROHA_USB2_MONCLK_SEL1>;
> > +        airoha,usb3-serdes = <AIROHA_SCU_SERDES_USB1>;
> > +        airoha,scu = <&scu>;
> > +
> > +        #phy-cells = <1>;
> > +    };
> > +
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8085fdca7bcd..af23c590bbc6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -763,6 +763,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/spi/airoha,en7581-snand.yaml
> >  F:	drivers/spi/spi-airoha-snfi.c
> >  
> > +AIROHA USB PHY DRIVER
> > +M:	Christian Marangi <ansuelsmth@gmail.com>
> > +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yaml
> > +F:	include/dt-bindings/phy/airoha,an7581-usb-phy.h
> > +
> >  AIRSPY MEDIA DRIVER
> >  L:	linux-media@vger.kernel.org
> >  S:	Orphan
> > diff --git a/include/dt-bindings/phy/airoha,an7581-usb-phy.h b/include/dt-bindings/phy/airoha,an7581-usb-phy.h
> > new file mode 100644
> > index 000000000000..efbb0ae75e3a
> > --- /dev/null
> > +++ b/include/dt-bindings/phy/airoha,an7581-usb-phy.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +
> > +#ifndef _DT_BINDINGS_AIROHA_AN7581_USB_PHY_H_
> > +#define _DT_BINDINGS_AIROHA_AN7581_USB_PHY_H_
> > +
> > +#define AIROHA_USB2_MONCLK_SEL0                 0
> > +#define AIROHA_USB2_MONCLK_SEL1                 1
> > +#define AIROHA_USB2_MONCLK_SEL2                 2
> > +#define AIROHA_USB2_MONCLK_SEL3                 3
> 
> These definitions seem completely pointless. The property is called
> "airoha,usb2-monitor-clk-sel" so any use will look like
> "airoha,usb2-monitor-clk-sel = <3>;"
> That's more informative than the define is, since it doesn't even
> truncate "monitor". I'd just delete this header entirely and use the
> number. If you want the define in the driver to avoid magic numbers,
> just define it in the driver.
> 

Well yes the idea here is to not have to use magic numbers in DT and try
to use the naming in the SoC documentation since they are called
monclk_sel0, monclk_sel1 ...

Will drop.

> pw-bot: changes-requested
> 
> Cheers,
> Conor.
> 
> > +
> > +#endif
> > -- 
> > 2.51.0
> > 



-- 
	Ansuel