| Message ID | 20230608162238.50078-3-sebastian.reichel@collabora.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add RK3588 SATA support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Jun 08, 2023 at 06:22:35PM +0200, Sebastian Reichel wrote: > This adds Rockchip RK3588 AHCI binding. In order to narrow down the > allowed clocks without bloating the generic binding, the description > of Rockchip's AHCI controllers has been moved to its own file. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../bindings/ata/rockchip,dwc-ahci.yaml | 114 ++++++++++++++++++ > .../bindings/ata/snps,dwc-ahci.yaml | 17 ++- > 2 files changed, 125 insertions(+), 6 deletions(-) > create mode 100644 Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml > > diff --git a/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml > new file mode 100644 > index 000000000000..86da9bd594a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml > @@ -0,0 +1,114 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/ata/rockchip,dwc-ahci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Synopsys DWC AHCI SATA controller for Rockchip devices > + > +maintainers: > + - Serge Semin <fancer.lancer@gmail.com> > + > +description: > + This document defines device tree bindings for the Synopsys DWC > + implementation of the AHCI SATA controller found in Rockchip > + devices. > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - rockchip,rk3568-dwc-ahci > + - rockchip,rk3588-dwc-ahci > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - rockchip,rk3568-dwc-ahci > + - rockchip,rk3588-dwc-ahci > + - const: snps,dwc-ahci > + > + ports-implemented: > + const: 1 > + > +patternProperties: > + "^sata-port@[0-9a-e]$": > + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port > + What about adding the "reg" property constraints? Seeing the "ports-implemented" property is supposed to be "1" the "reg" property should be zero for all the Rockchip AHCI SATA controllers. Right? > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - ports-implemented > + > +allOf: > + - $ref: snps,dwc-ahci-common.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - rockchip,rk3588-dwc-ahci > + then: > + properties: > + clock-names: > + items: > + - const: sata > + - const: pmalive > + - const: rxoob > + - const: ref > + - const: asic The "clocks" property constraints could have been added too. Right? > + - if: > + properties: > + compatible: > + contains: > + enum: > + - rockchip,rk3568-dwc-ahci > + then: > + properties: > + clock-names: > + items: > + - const: sata > + - const: pmalive > + - const: rxoob ditto > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/ata/ahci.h> > + #include <dt-bindings/phy/phy.h> > + > + sata@fe210000 { > + compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"; > + reg = <0xfe210000 0x1000>; > + clocks = <&cru ACLK_SATA0>, <&cru CLK_PMALIVE0>, > + <&cru CLK_RXOOB0>, <&cru CLK_PIPEPHY0_REF>, > + <&cru CLK_PIPEPHY0_PIPE_ASIC_G>; > + clock-names = "sata", "pmalive", "rxoob", "ref", "asic"; > + interrupts = <GIC_SPI 273 IRQ_TYPE_LEVEL_HIGH 0>; > + ports-implemented = <0x1>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + sata-port@0 { > + reg = <0>; > + hba-port-cap = <HBA_PORT_FBSCP>; > + phys = <&combphy0_ps PHY_TYPE_SATA>; > + phy-names = "sata-phy"; > + snps,rx-ts-max = <32>; > + snps,tx-ts-max = <32>; > + }; > + }; > + > +... > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > index 5afa4b57ce20..55a4bdfa3d9a 100644 > --- a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > @@ -13,8 +13,14 @@ description: > This document defines device tree bindings for the generic Synopsys DWC > implementation of the AHCI SATA controller. > > -allOf: > - - $ref: snps,dwc-ahci-common.yaml# > +select: > + properties: > + compatible: > + enum: > + - snps,dwc-ahci > + - snps,spear-ahci > + required: > + - compatible Please don't move the allOf property. It's ok to have it defined above the object properties clause if it has just a few ref-statements. * Also make sure please that the "select" property is defined above the "allOf" schema composition. -Serge(y) > > properties: > compatible: > @@ -23,10 +29,6 @@ properties: > const: snps,dwc-ahci > - description: SPEAr1340 AHCI SATA device > const: snps,spear-ahci > - - description: Rockhip RK3568 AHCI controller > - items: > - - const: rockchip,rk3568-dwc-ahci > - - const: snps,dwc-ahci > > patternProperties: > "^sata-port@[0-9a-e]$": > @@ -39,6 +41,9 @@ required: > - reg > - interrupts > > +allOf: > + - $ref: snps,dwc-ahci-common.yaml# > + > unevaluatedProperties: false > > examples: > -- > 2.39.2 >
On 08/06/2023 18:22, Sebastian Reichel wrote: > This adds Rockchip RK3588 AHCI binding. In order to narrow down the > allowed clocks without bloating the generic binding, the description > of Rockchip's AHCI controllers has been moved to its own file. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- ... > + > +properties: > + compatible: > + items: > + - enum: > + - rockchip,rk3568-dwc-ahci > + - rockchip,rk3588-dwc-ahci > + - const: snps,dwc-ahci > + > + ports-implemented: > + const: 1 > + > +patternProperties: > + "^sata-port@[0-9a-e]$": > + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port > + > + unevaluatedProperties: false You should be able to skip this patternProperties entirely, because it comes from dwc-ahci-common -> ahci-common schema. Did you try the patch without it? > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - ports-implemented > + Best regards, Krzysztof
On Mon, Jun 12, 2023 at 10:24:06AM +0200, Krzysztof Kozlowski wrote: > On 08/06/2023 18:22, Sebastian Reichel wrote: > > This adds Rockchip RK3588 AHCI binding. In order to narrow down the > > allowed clocks without bloating the generic binding, the description > > of Rockchip's AHCI controllers has been moved to its own file. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > ... > > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - rockchip,rk3568-dwc-ahci > > + - rockchip,rk3588-dwc-ahci > > + - const: snps,dwc-ahci > > + > > + ports-implemented: > > + const: 1 > > + > > +patternProperties: > > + "^sata-port@[0-9a-e]$": > > + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port > > + > > + unevaluatedProperties: false > > You should be able to skip this patternProperties entirely, because it > comes from dwc-ahci-common -> ahci-common schema. Did you try the patch > without it? Please see my message about this. The "sata-port@[0-9a-e]$" sub-node bindings could be updated with the "reg" property constraint which, based on the "ports-implemented" property value, most likely is supposed to be always set to const: 1. -Serge(y) > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - ports-implemented > > + > > > Best regards, > Krzysztof >
On 12/06/2023 10:35, Serge Semin wrote: > On Mon, Jun 12, 2023 at 10:24:06AM +0200, Krzysztof Kozlowski wrote: >> On 08/06/2023 18:22, Sebastian Reichel wrote: >>> This adds Rockchip RK3588 AHCI binding. In order to narrow down the >>> allowed clocks without bloating the generic binding, the description >>> of Rockchip's AHCI controllers has been moved to its own file. >>> >>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >>> --- >> >> ... >> >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - rockchip,rk3568-dwc-ahci >>> + - rockchip,rk3588-dwc-ahci >>> + - const: snps,dwc-ahci >>> + >>> + ports-implemented: >>> + const: 1 >>> + >>> +patternProperties: >>> + "^sata-port@[0-9a-e]$": >>> + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port >>> + >>> + unevaluatedProperties: false >> > >> You should be able to skip this patternProperties entirely, because it >> comes from dwc-ahci-common -> ahci-common schema. Did you try the patch >> without it? > > Please see my message about this. The "sata-port@[0-9a-e]$" sub-node > bindings could be updated with the "reg" property constraint which, > based on the "ports-implemented" property value, most likely is > supposed to be always set to const: 1. Then anyway the pattern is wrong as it should be @1 always. Best regards, Krzysztof
On Mon, Jun 12, 2023 at 10:39:57AM +0200, Krzysztof Kozlowski wrote: > On 12/06/2023 10:35, Serge Semin wrote: > > On Mon, Jun 12, 2023 at 10:24:06AM +0200, Krzysztof Kozlowski wrote: > >> On 08/06/2023 18:22, Sebastian Reichel wrote: > >>> This adds Rockchip RK3588 AHCI binding. In order to narrow down the > >>> allowed clocks without bloating the generic binding, the description > >>> of Rockchip's AHCI controllers has been moved to its own file. > >>> > >>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > >>> --- > >> > >> ... > >> > >>> + > >>> +properties: > >>> + compatible: > >>> + items: > >>> + - enum: > >>> + - rockchip,rk3568-dwc-ahci > >>> + - rockchip,rk3588-dwc-ahci > >>> + - const: snps,dwc-ahci > >>> + > >>> + ports-implemented: > >>> + const: 1 > >>> + > >>> +patternProperties: > >>> + "^sata-port@[0-9a-e]$": > >>> + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port > >>> + > >>> + unevaluatedProperties: false > >> > > > >> You should be able to skip this patternProperties entirely, because it > >> comes from dwc-ahci-common -> ahci-common schema. Did you try the patch > >> without it? > > > > Please see my message about this. The "sata-port@[0-9a-e]$" sub-node > > bindings could be updated with the "reg" property constraint which, > > based on the "ports-implemented" property value, most likely is > > supposed to be always set to const: 1. > > Then anyway the pattern is wrong as it should be @1 always. * I miscalculated a bit, it should have been zero but in general the pattern-property is indeed redundant. As a conclusion the change should look like this: +properties: + compatible: + items: + - enum: + - rockchip,rk3568-dwc-ahci + - rockchip,rk3588-dwc-ahci + - const: snps,dwc-ahci + + ports-implemented: + const: 1 + + "sata-port@0": + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port + + properties: + reg: + const: 0 + + unevaluatedProperties: false + + ... Right? -Serge(y) > > Best regards, > Krzysztof >
On 12/06/2023 10:52, Serge Semin wrote: > On Mon, Jun 12, 2023 at 10:39:57AM +0200, Krzysztof Kozlowski wrote: >> On 12/06/2023 10:35, Serge Semin wrote: >>> On Mon, Jun 12, 2023 at 10:24:06AM +0200, Krzysztof Kozlowski wrote: >>>> On 08/06/2023 18:22, Sebastian Reichel wrote: >>>>> This adds Rockchip RK3588 AHCI binding. In order to narrow down the >>>>> allowed clocks without bloating the generic binding, the description >>>>> of Rockchip's AHCI controllers has been moved to its own file. >>>>> >>>>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >>>>> --- >>>> >>>> ... >>>> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - rockchip,rk3568-dwc-ahci >>>>> + - rockchip,rk3588-dwc-ahci >>>>> + - const: snps,dwc-ahci >>>>> + >>>>> + ports-implemented: >>>>> + const: 1 >>>>> + >>>>> +patternProperties: >>>>> + "^sata-port@[0-9a-e]$": >>>>> + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port >>>>> + >>>>> + unevaluatedProperties: false >>>> >>> >>>> You should be able to skip this patternProperties entirely, because it >>>> comes from dwc-ahci-common -> ahci-common schema. Did you try the patch >>>> without it? >>> > >>> Please see my message about this. The "sata-port@[0-9a-e]$" sub-node >>> bindings could be updated with the "reg" property constraint which, >>> based on the "ports-implemented" property value, most likely is >>> supposed to be always set to const: 1. >> >> Then anyway the pattern is wrong as it should be @1 always. > > * I miscalculated a bit, it should have been zero but in general > the pattern-property is indeed redundant. > > As a conclusion the change should look like this: > > +properties: > + compatible: > + items: > + - enum: > + - rockchip,rk3568-dwc-ahci > + - rockchip,rk3588-dwc-ahci > + - const: snps,dwc-ahci > + > + ports-implemented: > + const: 1 > + > + "sata-port@0": > + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port > + > + properties: > + reg: > + const: 0 > + > + unevaluatedProperties: false > + > + ... > > Right? Code is correct but as I said - probably meaningless. All other ports are also accepted via referenced schema. You would need to disallow other ports or switch to additionalProperties: false in top-level. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml new file mode 100644 index 000000000000..86da9bd594a7 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml @@ -0,0 +1,114 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/ata/rockchip,dwc-ahci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Synopsys DWC AHCI SATA controller for Rockchip devices + +maintainers: + - Serge Semin <fancer.lancer@gmail.com> + +description: + This document defines device tree bindings for the Synopsys DWC + implementation of the AHCI SATA controller found in Rockchip + devices. + +select: + properties: + compatible: + contains: + enum: + - rockchip,rk3568-dwc-ahci + - rockchip,rk3588-dwc-ahci + required: + - compatible + +properties: + compatible: + items: + - enum: + - rockchip,rk3568-dwc-ahci + - rockchip,rk3588-dwc-ahci + - const: snps,dwc-ahci + + ports-implemented: + const: 1 + +patternProperties: + "^sata-port@[0-9a-e]$": + $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port + + unevaluatedProperties: false + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - ports-implemented + +allOf: + - $ref: snps,dwc-ahci-common.yaml# + - if: + properties: + compatible: + contains: + enum: + - rockchip,rk3588-dwc-ahci + then: + properties: + clock-names: + items: + - const: sata + - const: pmalive + - const: rxoob + - const: ref + - const: asic + - if: + properties: + compatible: + contains: + enum: + - rockchip,rk3568-dwc-ahci + then: + properties: + clock-names: + items: + - const: sata + - const: pmalive + - const: rxoob + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rockchip,rk3588-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/ata/ahci.h> + #include <dt-bindings/phy/phy.h> + + sata@fe210000 { + compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"; + reg = <0xfe210000 0x1000>; + clocks = <&cru ACLK_SATA0>, <&cru CLK_PMALIVE0>, + <&cru CLK_RXOOB0>, <&cru CLK_PIPEPHY0_REF>, + <&cru CLK_PIPEPHY0_PIPE_ASIC_G>; + clock-names = "sata", "pmalive", "rxoob", "ref", "asic"; + interrupts = <GIC_SPI 273 IRQ_TYPE_LEVEL_HIGH 0>; + ports-implemented = <0x1>; + #address-cells = <1>; + #size-cells = <0>; + + sata-port@0 { + reg = <0>; + hba-port-cap = <HBA_PORT_FBSCP>; + phys = <&combphy0_ps PHY_TYPE_SATA>; + phy-names = "sata-phy"; + snps,rx-ts-max = <32>; + snps,tx-ts-max = <32>; + }; + }; + +... diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml index 5afa4b57ce20..55a4bdfa3d9a 100644 --- a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml @@ -13,8 +13,14 @@ description: This document defines device tree bindings for the generic Synopsys DWC implementation of the AHCI SATA controller. -allOf: - - $ref: snps,dwc-ahci-common.yaml# +select: + properties: + compatible: + enum: + - snps,dwc-ahci + - snps,spear-ahci + required: + - compatible properties: compatible: @@ -23,10 +29,6 @@ properties: const: snps,dwc-ahci - description: SPEAr1340 AHCI SATA device const: snps,spear-ahci - - description: Rockhip RK3568 AHCI controller - items: - - const: rockchip,rk3568-dwc-ahci - - const: snps,dwc-ahci patternProperties: "^sata-port@[0-9a-e]$": @@ -39,6 +41,9 @@ required: - reg - interrupts +allOf: + - $ref: snps,dwc-ahci-common.yaml# + unevaluatedProperties: false examples:
This adds Rockchip RK3588 AHCI binding. In order to narrow down the allowed clocks without bloating the generic binding, the description of Rockchip's AHCI controllers has been moved to its own file. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- .../bindings/ata/rockchip,dwc-ahci.yaml | 114 ++++++++++++++++++ .../bindings/ata/snps,dwc-ahci.yaml | 17 ++- 2 files changed, 125 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml