Re: [PATCH v2 03/11] dt-bindings: clock: en7523: add Documentation for Airoha AN7581 SCU SSR

Rob Herring
2025-03-21T22:37:36Z

On Thu, Mar 20, 2025 at 02:00:26PM +0100, Christian Marangi wrote:
> The Airoha AN7581 SoC have in the SCU register space particular
> address that control how some peripheral are configured.
>
> These are toggeled in the System Status Register and are used to
> toggle Serdes port for USB 3.0 mode or HSGMII, USB 3.0 mode or PCIe2
> or setup port for PCIe mode or Ethrnet mode (HSGMII/USXGMII).
>
> Modes are mutually exclusive and selecting one mode cause the
> other feature to not work (example a mode in USB 3.0 cause PCIe
> port 2 to not work) This depends also on what is physically
> connected to the Hardware and needs to correctly reflect the
> System Status Register bits.
>
> Special care is needed for PCIe port 0 in 2 line mode that
> requires both WiFi1 and WiFi2 Serdes port set to PCIe0 2 Line
> mode.
>
> Expose these configuration as an enum of strings in the SCU node and
> also add dt-bindings header to reference each serdes port in DT.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> .../bindings/clock/airoha,en7523-scu.yaml | 101 ++++++++++++++++--
> MAINTAINERS | 7 ++
> include/dt-bindings/soc/airoha,scu-ssr.h | 11 ++
> 3 files changed, 110 insertions(+), 9 deletions(-)
> create mode 100644 include/dt-bindings/soc/airoha,scu-ssr.h
>
> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> index fe2c5c1baf43..637ce0e06619 100644
> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> @@ -9,6 +9,7 @@ title: EN7523 Clock
> maintainers:
> - Felix Fietkau <nbd@nbd.name>
> - John Crispin <nbd@nbd.name>
> + - Christian Marangi <ansuelsmth@gmail.com>
>
> description: |
> This node defines the System Control Unit of the EN7523 SoC,
> @@ -26,6 +27,23 @@ description: |
>
> The clocks are provided inside a system controller node.
>
> + The System Control Unit may also set different mode for the Serdes ports
> + present on the SoC.
> +
> + These are toggeled in the System Status Register and are used to
> + toggle Serdes port for USB 3.0 mode or HSGMII, USB 3.0 mode or PCIe2
> + or setup port for PCIe mode or Ethernet mode (HSGMII/USXGMII).
> +
> + Modes are mutually exclusive and selecting one mode cause the
> + other feature to not work (example a mode in USB 3.0 cause PCIe
> + port 2 to not work) This depends also on what is physically
> + connected to the Hardware and needs to correctly reflect the
> + System Status Register bits.
> +
> + Special care is needed for PCIe port 0 in 2 line mode that
> + requires both WiFi1 and WiFi2 Serdes port set to PCIe0 2 Line
> + mode.
> +
> properties:
> compatible:
> items:
> @@ -49,6 +67,40 @@ properties:
> description: ID of the controller reset line
> const: 1
>
> + airoha,serdes-wifi1:
> + description: Configure the WiFi1 Serdes port
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - pcie0_x2
> + - pcie0_x1
> + - ethernet
> + default: pcie0_x1
> +
> + airoha,serdes-wifi2:
> + description: Configure the WiFi2 Serdes port
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - pcie0_x2
> + - pcie1_x1
> + - ethernet
> + default: pcie1_x1
> +
> + airoha,serdes-usb1:
> + description: Configure the USB1 Serdes port
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - usb3
> + - ethernet
> + default: usb3
> +
> + airoha,serdes-usb2:
> + description: Configure the USB2 Serdes port
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - usb3
> + - pcie2_x1
> + default: usb3

Couldn't you make this a phy provider and use the mode flags in the
phy cells?

> +
> required:
> - compatible
> - reg
> @@ -64,6 +116,12 @@ allOf:
> reg:
> minItems: 2
>
> + airoha,serdes-wifi1: false
> + airoha,serdes-wifi2: false
> +
> + airoha,serdes-usb1: false
> + airoha,serdes-usb2: false
> +
> '#reset-cells': false
>
> - if:
> @@ -75,6 +133,24 @@ allOf:
> reg:
> maxItems: 1
>
> + - if:
> + properties:
> + airoha,serdes-wifi1:
> + const: pcie0_x2

This is also true if airoha,serdes-wifi1 is not present. Probably not
what you intended.

> + then:
> + properties:
> + airoha,serdes-wifi2:
> + const: pcie0_x2
> +
> + - if:
> + properties:
> + airoha,serdes-wifi2:
> + const: pcie0_x2
> + then:
> + properties:
> + airoha,serdes-wifi1:
> + const: pcie0_x2
> +
> additionalProperties: false