Re: [PATCHv1 2/5] dt-bindings: ata: ahci: add RK3588 AHCI controller

[Serge Semin]

Hi Sebastian

On Thu, Apr 13, 2023 at 08:23:42PM +0200, Sebastian Reichel wrote:
> Just like RK3568, the RK3588 has a DWC based AHCI controller.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
> FWIW IDK what exactly the ASIC clock is. The TRM does not provide any
> details unfortunately. It is required for functional SATA, though.
> ---
>  .../devicetree/bindings/ata/snps,dwc-ahci-common.yaml       | 6 ++++--
>  Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml    | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci-common.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci-common.yaml
> index c1457910520b..0df8f49431eb 100644
> --- a/Documentation/devicetree/bindings/ata/snps,dwc-ahci-common.yaml
> +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci-common.yaml
> @@ -31,11 +31,11 @@ properties:
>        PM-alive clock, RxOOB detection clock, embedded PHYs reference (Rx/Tx)
>        clock, etc.
>      minItems: 1
> -    maxItems: 4
> +    maxItems: 5
>  
>    clock-names:
>      minItems: 1
> -    maxItems: 4
> +    maxItems: 5
>      items:
>        oneOf:
>          - description: Application APB/AHB/AXI BIU clock
> @@ -50,6 +50,8 @@ properties:
>            const: rxoob
>          - description: SATA Ports reference clock
>            const: ref

> +        - description: Rockchip ASIC clock
> +          const: asic

Actually it's a standard DW SATA AHCI PHY-interface clock (named as
clk_asicN in the DW SATA AHCI HW manual). So feel free to add it to the
clock-names array with the description (taken from the manual): "PHY
Transmit Clock". The manual also says that the clock is generated by
the PHY0 for clocking Port0 Link and Transport Layers (TX clock
domain): 37.5 MHz, 75 MHz, 150 MHz, 300 MHz, or 600 MHz.

Similarly there is another clocks source "clk_rbcN". It's "PHY Receive
Clock" which is used to receive data from the PHYn. It can be also
added to the clock-names property under the name "rbc".

Note 1. Please add the suggested names to the property constraint
above the "ref" name definition. The later clock is mainly relevant to
the attached PHY rather than to the SATA AHCI controller itself.

Note 2. "rxoob", "asic" and "rbc" clocks are defined as "clk_rxoobN",
"clk_asicN" and "clk_rbcN" which means they are supposed to be defined
(if relevant) for each available SATA port. So in general they should
have been defined in the port sub-nodes clocks/clock-names properties.

Note 3. Note natively DW SATA AHCI doesn't have any PIPE interface (or
anything being called as PIPE). Instead it provides a PMA-interface
which is directly connected to a Synopsys SATA xG PHY with no
intermediate coders (PCS). Like this:

+---------+     +--------+
|         | PMA | Snps DW| SATA
| DW SATA |<--->| SATA xG|<---->
|  AHCI   | I/F |  PHY   |
|         |     |        |
+---------+     +--------+

In that case the DW SATA AHCI IP-core is supposed to be synthesized
with the particular Synopsys PHY type specified in the parameter
PHY_INTERFACE_TYPE. If a non-standard PHY is connected (like in your
case) PHY_INTERFACE_TYPE is supposed to be set to zero thus providing
a wide set of the PMA-interface configs which otherwise would have
been pre-defined with the Synopsys PHY-specific values. So judging by
the clock names in your patches and the way the DT-nodes are designed
Rockchip SATA AHCI controller diagram must be looking like this:

+---------+      +-------+       +--------+
|         | PIPE | some  | Rx/Tx | PMA/PMD| SATA
| DW SATA |<---->|  PCS  |<----->| NANEng |<---->
|  AHCI   |former|       |       |  PHY   |
|         |DW PMA|       |       |        |
+---------+      +-------+       +--------+

In the former case (DW SATA AHCI with Synopsys SATA xG PHY attached)
all the clocks "pmalive", "rbc", "asic" and "rxoob" are generated by
the Synopsys PHYs itself so there is no need in having them explicitly
defined in the system. In your case AFAICS a non-standard PCS+PHY
setup is utilized and the clocks are generated by a system-wide unit -
CRU.

>  
>    resets:
>      description:
> diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> index 5afa4b57ce20..c6a0d6c8b62c 100644
> --- a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> @@ -23,9 +23,11 @@ properties:
>          const: snps,dwc-ahci
>        - description: SPEAr1340 AHCI SATA device
>          const: snps,spear-ahci

> -      - description: Rockhip RK3568 AHCI controller
> +      - description: Rockhip AHCI controller
>          items:
> -          - const: rockchip,rk3568-dwc-ahci
> +          - enum:
> +              - rockchip,rk3568-dwc-ahci
> +              - rockchip,rk3588-dwc-ahci
>            - const: snps,dwc-ahci

What about moving all that in a dedicated YAML-file in order to define a
more comprehensive schema with actual "clocks", "clock-names",
ports-specific properties constraints? (see the way it's done for
Baikal-T1 SATA AHCI in ata/baikal,bt1-ahci.yaml).

Please note in that case you'll either need to drop the generic
fallback compatible (it's not like it would have been much useful
anyway) from your and RK3568 SATA DT-nodes, or define the
"select: properties: compatible: ..." property in the generic
DW SATA AHCI DT-schema, in order to prevent the generic schema being
automatically applied to the your SATA DT-nodes.

-Serge(y)

>  
>  patternProperties:
> -- 
> 2.39.2
>