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 >
