| 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: <69025bc9.5d0a0220.1f0440.deb9@mx.google.com>
Date: Wed, 29 Oct 2025 19:24:04 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Conor Dooley <conor@...nel.org>
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 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)
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@...c0000 {
> > + 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@...il.com>
> > +L: linux-arm-kernel@...ts.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@...r.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
Powered by blists - more mailing lists