[v4,17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema

Synopsys AHCI SATA controller is mainly compatible with the generic AHCI
SATA controller except a few peculiarities and the platform environment
requirements. In particular it can have one or two reference clocks to
feed up its AXI/AHB interface and SATA PHYs domain and at least one reset
control for the application clock domain. In addition to that the DMA
interface of each port can be tuned up to work with the predefined maximum
data chunk size. Note unlike generic AHCI controller DWC AHCI can't have
more than 8 ports. All of that is reflected in the new DWC AHCI SATA
device DT binding.

Note the DWC AHCI SATA controller DT-schema has been created in a way so
to be reused for the vendor-specific DT-schemas (see for example the
"snps,dwc-ahci" compatible string binding). One of which we are about to
introduce.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- Replace min/max constraints of the snps,{tx,rx}-ts-max property with
  enum [ 1, 2, 4, ..., 1024 ]. (@Rob)

Changelog v4:
- Decrease the "additionalProperties" property identation otherwise it's
  percieved as the node property instead of the key one. (@Rob)
- Use the ahci-port properties definition from the AHCI common schema
  in order to extend it with DWC AHCI SATA port properties. (@Rob)
- Remove the Hannes' rb tag since the patch content has changed.
---
 .../bindings/ata/ahci-platform.yaml           |   8 --
 .../bindings/ata/snps,dwc-ahci.yaml           | 129 ++++++++++++++++++
 2 files changed, 129 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml

On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:
> Synopsys AHCI SATA controller is mainly compatible with the generic AHCI
> SATA controller except a few peculiarities and the platform environment
> requirements. In particular it can have one or two reference clocks to
> feed up its AXI/AHB interface and SATA PHYs domain and at least one reset
> control for the application clock domain. In addition to that the DMA
> interface of each port can be tuned up to work with the predefined maximum
> data chunk size. Note unlike generic AHCI controller DWC AHCI can't have
> more than 8 ports. All of that is reflected in the new DWC AHCI SATA
> device DT binding.
> 
> Note the DWC AHCI SATA controller DT-schema has been created in a way so
> to be reused for the vendor-specific DT-schemas (see for example the
> "snps,dwc-ahci" compatible string binding). One of which we are about to
> introduce.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Changelog v2:
> - Replace min/max constraints of the snps,{tx,rx}-ts-max property with
>   enum [ 1, 2, 4, ..., 1024 ]. (@Rob)
> 
> Changelog v4:
> - Decrease the "additionalProperties" property identation otherwise it's
>   percieved as the node property instead of the key one. (@Rob)
> - Use the ahci-port properties definition from the AHCI common schema
>   in order to extend it with DWC AHCI SATA port properties. (@Rob)
> - Remove the Hannes' rb tag since the patch content has changed.
> ---
>  .../bindings/ata/ahci-platform.yaml           |   8 --
>  .../bindings/ata/snps,dwc-ahci.yaml           | 129 ++++++++++++++++++
>  2 files changed, 129 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> index e19cf9828e68..7dc2a2e8f598 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> @@ -30,8 +30,6 @@ select:
>            - marvell,armada-3700-ahci
>            - marvell,armada-8k-ahci
>            - marvell,berlin2q-ahci
> -          - snps,dwc-ahci
> -          - snps,spear-ahci
>    required:
>      - compatible
>  
> @@ -48,17 +46,11 @@ properties:
>                - marvell,berlin2-ahci
>                - marvell,berlin2q-ahci
>            - const: generic-ahci
> -      - items:
> -          - enum:
> -              - rockchip,rk3568-dwc-ahci
> -          - const: snps,dwc-ahci
>        - enum:
>            - cavium,octeon-7130-ahci
>            - hisilicon,hisi-ahci
>            - ibm,476gtr-ahci
>            - marvell,armada-3700-ahci
> -          - snps,dwc-ahci
> -          - snps,spear-ahci
>  
>    reg:
>      minItems: 1
> diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> new file mode 100644
> index 000000000000..af78f6c9b857
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DWC AHCI SATA controller
> +
> +maintainers:
> +  - Serge Semin <fancer.lancer@gmail.com>
> +
> +description:
> +  This document defines device tree bindings for the Synopsys DWC
> +  implementation of the AHCI SATA controller.
> +
> +allOf:
> +  - $ref: ahci-common.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: Synopsys AHCI SATA-compatible devices
> +        contains:
> +          const: snps,dwc-ahci
> +      - description: SPEAr1340 AHCI SATA device
> +        const: snps,spear-ahci
> +      - description: Rockhip RK3568 ahci controller
> +        const: rockchip,rk3568-dwc-ahci

This is never true because there is a fallback. We should keep what we 
had before.


> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock
> +      and embedded PHYs reference clock together with vendor-specific set
> +      of clocks.
> +    minItems: 1
> +    maxItems: 4
> +
> +  clock-names:
> +    contains:
> +      anyOf:
> +        - description: Application AXI/AHB BIU clock source
> +          enum:
> +            - aclk
> +            - sata
> +        - description: SATA Ports reference clock
> +          enum:
> +            - ref
> +            - sata_ref
> +
> +  resets:
> +    description:
> +      At least basic core and application clock domains reset is normally
> +      supported by the DWC AHCI SATA controller. Some platform specific
> +      clocks can be also specified though.

s/clocks/resets/ ?

This allows any number of resets which isn't great. I think this schema 
should just be the 'simple' cases where there's only 1 reset and 1 
clock (or how many the DWC block actually has if you have that info). 
More complicated cases get there own schema.

> +
> +  reset-names:
> +    contains:
> +      description: Core and application clock domains reset control
> +      const: arst
> +
> +patternProperties:
> +  "^sata-port@[0-9a-e]$":
> +    $ref: '#/$defs/dwc-ahci-port'
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +$defs:
> +  dwc-ahci-port:
> +    $ref: /schemas/ata/ahci-common.yaml#/$defs/ahci-port
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 7
> +
> +      snps,tx-ts-max:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Maximal size of Tx DMA transactions in FIFO words
> +        enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]
> +
> +      snps,rx-ts-max:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Maximal size of Rx DMA transactions in FIFO words
> +        enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/ata/ahci.h>
> +
> +    sata@122f0000 {
> +      compatible = "snps,dwc-ahci";
> +      reg = <0x122F0000 0x1ff>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
> +
> +      clocks = <&clock1>, <&clock2>;
> +      clock-names = "aclk", "ref";
> +
> +      phys = <&sata_phy>;
> +      phy-names = "sata-phy";
> +
> +      ports-implemented = <0x1>;
> +
> +      sata-port@0 {
> +        reg = <0>;
> +
> +        hba-port-cap = <HBA_PORT_FBSCP>;
> +
> +        snps,tx-ts-max = <512>;
> +        snps,rx-ts-max = <512>;
> +      };
> +    };
> +...
> -- 
> 2.35.1
> 
>

On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:
> On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:
> > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI
> > SATA controller except a few peculiarities and the platform environment
> > requirements. In particular it can have one or two reference clocks to
> > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset
> > control for the application clock domain. In addition to that the DMA
> > interface of each port can be tuned up to work with the predefined maximum
> > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have
> > more than 8 ports. All of that is reflected in the new DWC AHCI SATA
> > device DT binding.
> > 
> > Note the DWC AHCI SATA controller DT-schema has been created in a way so
> > to be reused for the vendor-specific DT-schemas (see for example the
> > "snps,dwc-ahci" compatible string binding). One of which we are about to
> > introduce.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Changelog v2:
> > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with
> >   enum [ 1, 2, 4, ..., 1024 ]. (@Rob)
> > 
> > Changelog v4:
> > - Decrease the "additionalProperties" property identation otherwise it's
> >   percieved as the node property instead of the key one. (@Rob)
> > - Use the ahci-port properties definition from the AHCI common schema
> >   in order to extend it with DWC AHCI SATA port properties. (@Rob)
> > - Remove the Hannes' rb tag since the patch content has changed.
> > ---
> >  .../bindings/ata/ahci-platform.yaml           |   8 --
> >  .../bindings/ata/snps,dwc-ahci.yaml           | 129 ++++++++++++++++++
> >  2 files changed, 129 insertions(+), 8 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > index e19cf9828e68..7dc2a2e8f598 100644
> > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > @@ -30,8 +30,6 @@ select:
> >            - marvell,armada-3700-ahci
> >            - marvell,armada-8k-ahci
> >            - marvell,berlin2q-ahci
> > -          - snps,dwc-ahci
> > -          - snps,spear-ahci
> >    required:
> >      - compatible
> >  
> > @@ -48,17 +46,11 @@ properties:
> >                - marvell,berlin2-ahci
> >                - marvell,berlin2q-ahci
> >            - const: generic-ahci
> > -      - items:
> > -          - enum:
> > -              - rockchip,rk3568-dwc-ahci
> > -          - const: snps,dwc-ahci
> >        - enum:
> >            - cavium,octeon-7130-ahci
> >            - hisilicon,hisi-ahci
> >            - ibm,476gtr-ahci
> >            - marvell,armada-3700-ahci
> > -          - snps,dwc-ahci
> > -          - snps,spear-ahci
> >  
> >    reg:
> >      minItems: 1
> > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > new file mode 100644
> > index 000000000000..af78f6c9b857
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DWC AHCI SATA controller
> > +
> > +maintainers:
> > +  - Serge Semin <fancer.lancer@gmail.com>
> > +
> > +description:
> > +  This document defines device tree bindings for the Synopsys DWC
> > +  implementation of the AHCI SATA controller.
> > +
> > +allOf:
> > +  - $ref: ahci-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description: Synopsys AHCI SATA-compatible devices
> > +        contains:
> > +          const: snps,dwc-ahci
> > +      - description: SPEAr1340 AHCI SATA device
> > +        const: snps,spear-ahci
> > +      - description: Rockhip RK3568 ahci controller
> > +        const: rockchip,rk3568-dwc-ahci
> 

> This is never true because there is a fallback. We should keep what we 
> had before.

Could you be more specific what you meant? I don't see
"snps,spear-ahci" and "rockchip,rk3568-dwc-ahci" used with the fallback
string so modification is correct in that case.

My idea was to have the compatible strings with the required generic
fallback "snps,dwc-ahci" for all new devices thus identifying the
controller IP-core origin. But later you said "The generic IP block
fallbacks have proven to be useless." I do agree that functionally it
isn't that often used, but in some cases it can be handy for instance
to implement quirks in the generic code or use the fallback as an
additional info regarding the IP-core origin/version. So if I were you
I wouldn't be that strict about dropping the generic IP-core fallback
identifier. It's much easier to have it specified from the very
beginning than adding it after it has been declared as not required.

> 
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description:
> > +      Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock
> > +      and embedded PHYs reference clock together with vendor-specific set
> > +      of clocks.
> > +    minItems: 1
> > +    maxItems: 4
> > +
> > +  clock-names:
> > +    contains:
> > +      anyOf:
> > +        - description: Application AXI/AHB BIU clock source
> > +          enum:
> > +            - aclk
> > +            - sata
> > +        - description: SATA Ports reference clock
> > +          enum:
> > +            - ref
> > +            - sata_ref
> > +
> > +  resets:
> > +    description:
> > +      At least basic core and application clock domains reset is normally
> > +      supported by the DWC AHCI SATA controller. Some platform specific
> > +      clocks can be also specified though.
> 

> s/clocks/resets/ ?

Right, but only in the reference to "platform specific clocks" -> "... resets".

> 
> This allows any number of resets which isn't great. I think this schema 
> should just be the 'simple' cases where there's only 1 reset and 1 
> clock (or how many the DWC block actually has if you have that info). 
> More complicated cases get there own schema.

DWC SATA reference manual claims there can be resets implemented to
each clock domain.
1) PM-clk <- PM-rst - PM keep-alive clock/reset.
2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.
3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to
number of ports <= 8.)
4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to
number of ports <= 8.)
5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up
to number of ports <= 8.)

So to speak the IP-core can be equipped with up to 26 clocks and
resets. Should we be more strict we would have needed to define the
properties with all the names above and permit up to 26 clocks/resets
items. (Do you want it to be done?). In our case for instance there
is "aclk" and a single common "ref" clock for all 3, 4 and 5 domain
(clock 1 is missing).

-Sergey

> 
> > +
> > +  reset-names:
> > +    contains:
> > +      description: Core and application clock domains reset control
> > +      const: arst
> > +
> > +patternProperties:
> > +  "^sata-port@[0-9a-e]$":
> > +    $ref: '#/$defs/dwc-ahci-port'
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +$defs:
> > +  dwc-ahci-port:
> > +    $ref: /schemas/ata/ahci-common.yaml#/$defs/ahci-port
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 7
> > +
> > +      snps,tx-ts-max:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: Maximal size of Tx DMA transactions in FIFO words
> > +        enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]
> > +
> > +      snps,rx-ts-max:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: Maximal size of Rx DMA transactions in FIFO words
> > +        enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/ata/ahci.h>
> > +
> > +    sata@122f0000 {
> > +      compatible = "snps,dwc-ahci";
> > +      reg = <0x122F0000 0x1ff>;
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +      clocks = <&clock1>, <&clock2>;
> > +      clock-names = "aclk", "ref";
> > +
> > +      phys = <&sata_phy>;
> > +      phy-names = "sata-phy";
> > +
> > +      ports-implemented = <0x1>;
> > +
> > +      sata-port@0 {
> > +        reg = <0>;
> > +
> > +        hba-port-cap = <HBA_PORT_FBSCP>;
> > +
> > +        snps,tx-ts-max = <512>;
> > +        snps,rx-ts-max = <512>;
> > +      };
> > +    };
> > +...
> > -- 
> > 2.35.1
> > 
> >

On Wed, Jul 06, 2022 at 04:36:42PM -0600, Rob Herring wrote:
> On Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:
> > On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:
> > > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:
> > > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI
> > > > SATA controller except a few peculiarities and the platform environment
> > > > requirements. In particular it can have one or two reference clocks to
> > > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset
> > > > control for the application clock domain. In addition to that the DMA
> > > > interface of each port can be tuned up to work with the predefined maximum
> > > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have
> > > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA
> > > > device DT binding.
> > > > 
> > > > Note the DWC AHCI SATA controller DT-schema has been created in a way so
> > > > to be reused for the vendor-specific DT-schemas (see for example the
> > > > "snps,dwc-ahci" compatible string binding). One of which we are about to
> > > > introduce.
> > > > 
> > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > 
> > > > ---
> > > > 
> > > > Changelog v2:
> > > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with
> > > >   enum [ 1, 2, 4, ..., 1024 ]. (@Rob)
> > > > 
> > > > Changelog v4:
> > > > - Decrease the "additionalProperties" property identation otherwise it's
> > > >   percieved as the node property instead of the key one. (@Rob)
> > > > - Use the ahci-port properties definition from the AHCI common schema
> > > >   in order to extend it with DWC AHCI SATA port properties. (@Rob)
> > > > - Remove the Hannes' rb tag since the patch content has changed.
> > > > ---
> > > >  .../bindings/ata/ahci-platform.yaml           |   8 --
> > > >  .../bindings/ata/snps,dwc-ahci.yaml           | 129 ++++++++++++++++++
> > > >  2 files changed, 129 insertions(+), 8 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > > > index e19cf9828e68..7dc2a2e8f598 100644
> > > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > > > @@ -30,8 +30,6 @@ select:
> > > >            - marvell,armada-3700-ahci
> > > >            - marvell,armada-8k-ahci
> > > >            - marvell,berlin2q-ahci
> > > > -          - snps,dwc-ahci
> > > > -          - snps,spear-ahci
> > > >    required:
> > > >      - compatible
> > > >  
> > > > @@ -48,17 +46,11 @@ properties:
> > > >                - marvell,berlin2-ahci
> > > >                - marvell,berlin2q-ahci
> > > >            - const: generic-ahci
> > > > -      - items:
> > > > -          - enum:
> > > > -              - rockchip,rk3568-dwc-ahci
> > > > -          - const: snps,dwc-ahci
> > > >        - enum:
> > > >            - cavium,octeon-7130-ahci
> > > >            - hisilicon,hisi-ahci
> > > >            - ibm,476gtr-ahci
> > > >            - marvell,armada-3700-ahci
> > > > -          - snps,dwc-ahci
> > > > -          - snps,spear-ahci
> > > >  
> > > >    reg:
> > > >      minItems: 1
> > > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > > > new file mode 100644
> > > > index 000000000000..af78f6c9b857
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
> > > > @@ -0,0 +1,129 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Synopsys DWC AHCI SATA controller
> > > > +
> > > > +maintainers:
> > > > +  - Serge Semin <fancer.lancer@gmail.com>
> > > > +
> > > > +description:
> > > > +  This document defines device tree bindings for the Synopsys DWC
> > > > +  implementation of the AHCI SATA controller.
> > > > +
> > > > +allOf:
> > > > +  - $ref: ahci-common.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - description: Synopsys AHCI SATA-compatible devices
> > > > +        contains:
> > > > +          const: snps,dwc-ahci
> > > > +      - description: SPEAr1340 AHCI SATA device
> > > > +        const: snps,spear-ahci
> > > > +      - description: Rockhip RK3568 ahci controller
> > > > +        const: rockchip,rk3568-dwc-ahci
> > > 
> > 
> > > This is never true because there is a fallback. We should keep what we 
> > > had before.
> > 
> > Could you be more specific what you meant? I don't see
> > "snps,spear-ahci" and "rockchip,rk3568-dwc-ahci" used with the fallback
> > string so modification is correct in that case.
> 

> Spear does not, just rockchip:
> 
> arch/arm64/boot/dts/rockchip/rk3568.dtsi:               compatible = "rockchip,rk3568-dwc-ahci", "snps,dwc-ahci";
> arch/arm64/boot/dts/rockchip/rk356x.dtsi:               compatible = "rockchip,rk3568-dwc-ahci", "snps,dwc-ahci";
> arch/arm64/boot/dts/rockchip/rk356x.dtsi:               compatible = "rockchip,rk3568-dwc-ahci", "snps,dwc-ahci";
> 
> So the 3rd entry is never true.

Then I'll have to split the schema up into two bindings:
1. snps,dwc-ahci-common.yaml: generic DW SATA AHCI properties and no "compatible"
property constraint since you said fallback was useless.
2. snps,dwc-ahci.yaml: generic DW SATA AHCI DT-schema with
competibles: ("snps,dwc-ahci"), ("snps,spear-ahci"),
("rockchip,rk3568-dwc-ahci","snps,dwc-ahci").

Are you ok with this?

BTW if we had the fallback required the splitting up couldn't have
been needed.

> 
> > My idea was to have the compatible strings with the required generic
> > fallback "snps,dwc-ahci" for all new devices thus identifying the
> > controller IP-core origin. But later you said "The generic IP block
> > fallbacks have proven to be useless." I do agree that functionally it
> > isn't that often used, but in some cases it can be handy for instance
> > to implement quirks in the generic code or use the fallback as an
> > additional info regarding the IP-core origin/version. So if I were you
> > I wouldn't be that strict about dropping the generic IP-core fallback
> > identifier. It's much easier to have it specified from the very
> > beginning than adding it after it has been declared as not required.
> 
> I wish they were useful, but experience has shown they are not.

So what to do with the generic fallback compatibles then? Please
answer to the next questions so I would correct all my currently
stashed patches in accordance with it.

1) Do you want all the new DT-binding schemas refusing to have the
fallback compatibles except for the nodes which bindings have already
been defined that way?

2) What if a device IP-core has some versioning, but it's either
not auto-detectable at runtime or can be auto-detected but starting
from some IP-core version? Do we need it being specified in addition
to the vendor-specific compatible string?

3) The same as 2), but shall it have a generic version-less fallback
compatible string too?

4) The same as 2), but what if it concerns a device which driver
relies on the versioning?

5) The same as 2), but what if it concerns the device which currently
doesn't have a driver relying on the IP-core version?

6) What if we don't have the generic fallback compatible string
required, but at some point a kernel would need it to
implement a version/IP-core-specific quirk? If we had the generic
fallback specified in dts the older systems would have been supported
out-of-box, otherwise the firmware update would also needed.

IMO having the IP-core version + generic compatibles give many
benefits and it's much easier to have them required from the very
beginning instead of adding afterwards when then a need arises.

-Sergey

> 
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    description:
> > > > +      Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock
> > > > +      and embedded PHYs reference clock together with vendor-specific set
> > > > +      of clocks.
> > > > +    minItems: 1
> > > > +    maxItems: 4
> > > > +
> > > > +  clock-names:
> > > > +    contains:
> > > > +      anyOf:
> > > > +        - description: Application AXI/AHB BIU clock source
> > > > +          enum:
> > > > +            - aclk
> > > > +            - sata
> > > > +        - description: SATA Ports reference clock
> > > > +          enum:
> > > > +            - ref
> > > > +            - sata_ref
> > > > +
> > > > +  resets:
> > > > +    description:
> > > > +      At least basic core and application clock domains reset is normally
> > > > +      supported by the DWC AHCI SATA controller. Some platform specific
> > > > +      clocks can be also specified though.
> > > 
> > 
> > > s/clocks/resets/ ?
> > 
> > Right, but only in the reference to "platform specific clocks" -> "... resets".
> > 
> > > 
> > > This allows any number of resets which isn't great. I think this schema 
> > > should just be the 'simple' cases where there's only 1 reset and 1 
> > > clock (or how many the DWC block actually has if you have that info). 
> > > More complicated cases get there own schema.
> > 
> > DWC SATA reference manual claims there can be resets implemented to
> > each clock domain.
> > 1) PM-clk <- PM-rst - PM keep-alive clock/reset.
> > 2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.
> > 3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to
> > number of ports <= 8.)
> > 4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to
> > number of ports <= 8.)
> > 5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up
> > to number of ports <= 8.)
> > 
> > So to speak the IP-core can be equipped with up to 26 clocks and
> > resets. Should we be more strict we would have needed to define the
> > properties with all the names above and permit up to 26 clocks/resets
> > items. (Do you want it to be done?). In our case for instance there
> > is "aclk" and a single common "ref" clock for all 3, 4 and 5 domain
> > (clock 1 is missing).
> 

> I imagine most implementations just tie most clocks together.
> 
> I guess there's not going to be much new SATA h/w, so perhaps fine 
> as-is.

Do you mean to keep the "resets" property with no num-of-items constraints
then?

-Sergey

> 
> Rob