In order to create a more sophisticated AHCI controller DT bindings let's
divide the already available generic AHCI platform YAML schema into the
platform part and a set of the common AHCI properties. The former part
will be used to evaluate the AHCI DT nodes mainly compatible with the
generic AHCI controller while the later schema will be used for more
thorough AHCI DT nodes description. For instance such YAML schemas design
will be useful for our DW AHCI SATA controller derivative with four clock
sources, two reset lines, one system controller reference and specific
max Rx/Tx DMA xfers size constraints.
Note the phys and target-supply property requirement is preserved in the
generic AHCI platform bindings because some platforms can lack of the
explicitly specified PHYs or target device power regulators.
Signed-off-by: Serge Semin <[email protected]>
---
Folks, I don't really see why the phys/target-supply requirement has been
added to the generic AHCI DT schema in the first place. Probably just to
imply some meaning for the sub-nodes definition. Anyway in one of the
further patches I am adding the DW AHCI SATA controller DT bindings which
won't require having these properties specified in the sub-nodes, but will
describe additional port-specific properties. That's why I get to keep the
constraints in the ahci-platform.yaml schema instead of moving them to the
common schema.
Changelog v2:
- This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
Changelog v3:
- Replace Jens's email address with Damien's one in the list of the
schema maintainers. (@Damien)
---
.../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
.../bindings/ata/ahci-platform.yaml | 68 +---------
2 files changed, 123 insertions(+), 62 deletions(-)
create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
new file mode 100644
index 000000000000..620042ca12e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common Properties for Serial ATA AHCI controllers
+
+maintainers:
+ - Hans de Goede <[email protected]>
+ - Damien Le Moal <[email protected]>
+
+description:
+ This document defines device tree properties for a common AHCI SATA
+ controller implementation. It's hardware interface is supposed to
+ conform to the technical standard defined by Intel (see Serial ATA
+ Advanced Host Controller Interface specification for details). The
+ document doesn't constitute a DT-node binding by itself but merely
+ defines a set of common properties for the AHCI-compatible devices.
+
+select: false
+
+allOf:
+ - $ref: sata-common.yaml#
+
+properties:
+ reg:
+ description:
+ Generic AHCI registers space conforming to the Serial ATA AHCI
+ specification.
+
+ reg-names:
+ description: CSR space IDs
+
+ interrupts:
+ description:
+ Generic AHCI state change interrupt. Can be implemented either as a
+ single line attached to the controller as a set of the dedicated signals
+ for the global and particular port events.
+
+ clocks:
+ description:
+ List of all the reference clocks connected to the controller.
+
+ clock-names:
+ description: Reference clocks IDs
+
+ resets:
+ description:
+ List of the reset control lines to reset the controller clock
+ domains.
+
+ reset-names:
+ description: Reset line IDs
+
+ power-domains:
+ description:
+ List of the power domain the AHCI controller being a part of.
+
+ ahci-supply:
+ description: Power regulator for AHCI controller
+
+ target-supply:
+ description: Power regulator for SATA target device
+
+ phy-supply:
+ description: Power regulator for SATA PHY
+
+ phys:
+ description: Reference to the SATA PHY node
+ maxItems: 1
+
+ phy-names:
+ maxItems: 1
+
+ ports-implemented:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ description:
+ Mask that indicates which ports the HBA supports. Useful if PI is not
+ programmed by the BIOS, which is true for some embedded SoC's.
+ maximum: 0x1f
+
+patternProperties:
+ "^sata-port@[0-9a-f]+$":
+ type: object
+ description:
+ It is optionally possible to describe the ports as sub-nodes so
+ to enable each port independently when dealing with multiple PHYs.
+
+ properties:
+ reg:
+ description: AHCI SATA port identifier
+ maxItems: 1
+
+ phys:
+ description: Individual AHCI SATA port PHY
+ maxItems: 1
+
+ phy-names:
+ description: AHCI SATA port PHY ID
+ maxItems: 1
+
+ target-supply:
+ description: Power regulator for SATA port target device
+
+ required:
+ - reg
+
+ additionalProperties: true
+
+required:
+ - reg
+ - interrupts
+
+additionalProperties: true
+
+...
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
index 9304e4731965..76075d3c8987 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
@@ -36,8 +36,7 @@ select:
- compatible
allOf:
- - $ref: "sata-common.yaml#"
-
+ - $ref: "ahci-common.yaml#"
properties:
compatible:
@@ -69,90 +68,35 @@ properties:
maxItems: 1
clocks:
- description:
- Clock IDs array as required by the controller.
minItems: 1
maxItems: 3
clock-names:
- description:
- Names of clocks corresponding to IDs in the clock property.
minItems: 1
maxItems: 3
interrupts:
maxItems: 1
- ahci-supply:
- description:
- regulator for AHCI controller
-
- phy-supply:
- description:
- regulator for PHY power
-
- phys:
- description:
- List of all PHYs on this controller
- maxItems: 1
-
- phy-names:
- description:
- Name specifier for the PHYs
- maxItems: 1
-
- ports-implemented:
- $ref: '/schemas/types.yaml#/definitions/uint32'
- description: |
- Mask that indicates which ports that the HBA supports
- are available for software to use. Useful if PORTS_IMPL
- is not programmed by the BIOS, which is true with
- some embedded SoCs.
- maximum: 0x1f
-
power-domains:
maxItems: 1
resets:
maxItems: 1
- target-supply:
- description:
- regulator for SATA target power
-
-required:
- - compatible
- - reg
- - interrupts
-
patternProperties:
"^sata-port@[0-9a-f]+$":
type: object
- additionalProperties: false
- description:
- Subnode with configuration of the Ports.
-
- properties:
- reg:
- maxItems: 1
-
- phys:
- maxItems: 1
-
- phy-names:
- maxItems: 1
-
- target-supply:
- description:
- regulator for SATA target power
-
- required:
- - reg
anyOf:
- required: [ phys ]
- required: [ target-supply ]
+required:
+ - compatible
+ - reg
+ - interrupts
+
unevaluatedProperties: false
examples:
--
2.35.1
On Thu, May 12, 2022 at 08:19:34AM +0200, Hannes Reinecke wrote:
> On 5/12/22 01:17, Serge Semin wrote:
> > In order to create a more sophisticated AHCI controller DT bindings let's
> > divide the already available generic AHCI platform YAML schema into the
> > platform part and a set of the common AHCI properties. The former part
> > will be used to evaluate the AHCI DT nodes mainly compatible with the
> > generic AHCI controller while the later schema will be used for more
> > thorough AHCI DT nodes description. For instance such YAML schemas design
> > will be useful for our DW AHCI SATA controller derivative with four clock
> > sources, two reset lines, one system controller reference and specific
> > max Rx/Tx DMA xfers size constraints.
> >
> > Note the phys and target-supply property requirement is preserved in the
> > generic AHCI platform bindings because some platforms can lack of the
> > explicitly specified PHYs or target device power regulators.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Folks, I don't really see why the phys/target-supply requirement has been
> > added to the generic AHCI DT schema in the first place. Probably just to
> > imply some meaning for the sub-nodes definition. Anyway in one of the
> > further patches I am adding the DW AHCI SATA controller DT bindings which
> > won't require having these properties specified in the sub-nodes, but will
> > describe additional port-specific properties. That's why I get to keep the
> > constraints in the ahci-platform.yaml schema instead of moving them to the
> > common schema.
> >
> > Changelog v2:
> > - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
> >
> > Changelog v3:
> > - Replace Jens's email address with Damien's one in the list of the
> > schema maintainers. (@Damien)
> > ---
> > .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> > .../bindings/ata/ahci-platform.yaml | 68 +---------
> > 2 files changed, 123 insertions(+), 62 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > new file mode 100644
> > index 000000000000..620042ca12e7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common Properties for Serial ATA AHCI controllers
> > +
> > +maintainers:
> > + - Hans de Goede <[email protected]>
> > + - Damien Le Moal <[email protected]>
> > +
> > +description:
> > + This document defines device tree properties for a common AHCI SATA
> > + controller implementation. It's hardware interface is supposed to
> > + conform to the technical standard defined by Intel (see Serial ATA
> > + Advanced Host Controller Interface specification for details). The
> > + document doesn't constitute a DT-node binding by itself but merely
> > + defines a set of common properties for the AHCI-compatible devices.
> > +
> > +select: false
> > +
> > +allOf:
> > + - $ref: sata-common.yaml#
> > +
> > +properties:
> > + reg:
> > + description:
> > + Generic AHCI registers space conforming to the Serial ATA AHCI
> > + specification.
> > +
> > + reg-names:
> > + description: CSR space IDs
> > +
> > + interrupts:
> > + description:
> > + Generic AHCI state change interrupt. Can be implemented either as a
> > + single line attached to the controller as a set of the dedicated signals
> > + for the global and particular port events.
> > +
> > + clocks:
> > + description:
> > + List of all the reference clocks connected to the controller.
> > +
> > + clock-names:
> > + description: Reference clocks IDs
> > +
> > + resets:
> > + description:
> > + List of the reset control lines to reset the controller clock
> > + domains.
> > +
> > + reset-names:
> > + description: Reset line IDs
> > +
> > + power-domains:
> > + description:
> > + List of the power domain the AHCI controller being a part of.
> > +
> > + ahci-supply:
> > + description: Power regulator for AHCI controller
> > +
> > + target-supply:
> > + description: Power regulator for SATA target device
> > +
> > + phy-supply:
> > + description: Power regulator for SATA PHY
> > +
> > + phys:
> > + description: Reference to the SATA PHY node
> > + maxItems: 1
> > +
> > + phy-names:
> > + maxItems: 1
> > +
> > + ports-implemented:
> > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > + description:
> > + Mask that indicates which ports the HBA supports. Useful if PI is not
> > + programmed by the BIOS, which is true for some embedded SoC's.
> > + maximum: 0x1f
> > +
> > +patternProperties:
> > + "^sata-port@[0-9a-f]+$":
> > + type: object
> > + description:
> > + It is optionally possible to describe the ports as sub-nodes so
> > + to enable each port independently when dealing with multiple PHYs.
> > +
> > + properties:
> > + reg:
> > + description: AHCI SATA port identifier
> > + maxItems: 1
> > +
> > + phys:
> > + description: Individual AHCI SATA port PHY
> > + maxItems: 1
> > +
> > + phy-names:
> > + description: AHCI SATA port PHY ID
> > + maxItems: 1
> > +
> > + target-supply:
> > + description: Power regulator for SATA port target device
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: true
> > +
> > +required:
> > + - reg
> > + - interrupts
> > +
> > +additionalProperties: true
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > index 9304e4731965..76075d3c8987 100644
> > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > @@ -36,8 +36,7 @@ select:
> > - compatible
> > allOf:
> > - - $ref: "sata-common.yaml#"
> > -
> > + - $ref: "ahci-common.yaml#"
>
> What happened to 'sata-common.yaml' ?
Nothing. It's still relevant for some devices.
> Not needed anymore? Included via other means?
Included by the SATA-compatible devices. Mainly that schema is relevant
to the devices which aren't AHCI-compatible.
>
> Please clarify.
sata-common.yaml is a common schema for the SATA devices (including
AHCI-devices), while ahci-common.yaml is a common schema for the
AHCI-compatible devices. So the later is more restrictive, than the
former one. The SATA DT-indings can be used by the
AHCI-incompatible devices for instance by the ones handled in the
drivers drivers/ata/sata_*.c. The AHCI DT-schema can be used by the
AHCI-enabled platforms like described in the LLDDs drivers/ata/ahci_*.
This means if your device is based on AHCI, then its bindings should
refer to the ahci-common.yaml schema and specify the platform-specific
DT-bindings: new properties and common properties constraints. If it
isn't AHCI-compatible but is a Serial ATA device, then it's bindings
should refer to the sata-common.yaml schema (which is much less
restrictive and defines just some small set of the properties).
For instance the brcm,sata-brcm.yaml DT-bindings can be converted to
using the ahci-common.yaml schema instead of the sata-common.yaml
schema, while the renesas,rcar-sata.yaml bindings can refer to the
sata-common.yaml schema thus for instance restricting the DT-node name
to be 'sata' for which in the current RCAR SATA bindings there is no
relevant constraints.
-Sergey
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> [email protected] +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
> HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer
On 5/12/22 01:17, Serge Semin wrote:
> In order to create a more sophisticated AHCI controller DT bindings let's
> divide the already available generic AHCI platform YAML schema into the
> platform part and a set of the common AHCI properties. The former part
> will be used to evaluate the AHCI DT nodes mainly compatible with the
> generic AHCI controller while the later schema will be used for more
> thorough AHCI DT nodes description. For instance such YAML schemas design
> will be useful for our DW AHCI SATA controller derivative with four clock
> sources, two reset lines, one system controller reference and specific
> max Rx/Tx DMA xfers size constraints.
>
> Note the phys and target-supply property requirement is preserved in the
> generic AHCI platform bindings because some platforms can lack of the
> explicitly specified PHYs or target device power regulators.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Folks, I don't really see why the phys/target-supply requirement has been
> added to the generic AHCI DT schema in the first place. Probably just to
> imply some meaning for the sub-nodes definition. Anyway in one of the
> further patches I am adding the DW AHCI SATA controller DT bindings which
> won't require having these properties specified in the sub-nodes, but will
> describe additional port-specific properties. That's why I get to keep the
> constraints in the ahci-platform.yaml schema instead of moving them to the
> common schema.
>
> Changelog v2:
> - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
>
> Changelog v3:
> - Replace Jens's email address with Damien's one in the list of the
> schema maintainers. (@Damien)
> ---
> .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> .../bindings/ata/ahci-platform.yaml | 68 +---------
> 2 files changed, 123 insertions(+), 62 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> new file mode 100644
> index 000000000000..620042ca12e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common Properties for Serial ATA AHCI controllers
> +
> +maintainers:
> + - Hans de Goede <[email protected]>
> + - Damien Le Moal <[email protected]>
> +
> +description:
> + This document defines device tree properties for a common AHCI SATA
> + controller implementation. It's hardware interface is supposed to
> + conform to the technical standard defined by Intel (see Serial ATA
> + Advanced Host Controller Interface specification for details). The
> + document doesn't constitute a DT-node binding by itself but merely
> + defines a set of common properties for the AHCI-compatible devices.
> +
> +select: false
> +
> +allOf:
> + - $ref: sata-common.yaml#
> +
> +properties:
> + reg:
> + description:
> + Generic AHCI registers space conforming to the Serial ATA AHCI
> + specification.
> +
> + reg-names:
> + description: CSR space IDs
> +
> + interrupts:
> + description:
> + Generic AHCI state change interrupt. Can be implemented either as a
> + single line attached to the controller as a set of the dedicated signals
> + for the global and particular port events.
> +
> + clocks:
> + description:
> + List of all the reference clocks connected to the controller.
> +
> + clock-names:
> + description: Reference clocks IDs
> +
> + resets:
> + description:
> + List of the reset control lines to reset the controller clock
> + domains.
> +
> + reset-names:
> + description: Reset line IDs
> +
> + power-domains:
> + description:
> + List of the power domain the AHCI controller being a part of.
> +
> + ahci-supply:
> + description: Power regulator for AHCI controller
> +
> + target-supply:
> + description: Power regulator for SATA target device
> +
> + phy-supply:
> + description: Power regulator for SATA PHY
> +
> + phys:
> + description: Reference to the SATA PHY node
> + maxItems: 1
> +
> + phy-names:
> + maxItems: 1
> +
> + ports-implemented:
> + $ref: '/schemas/types.yaml#/definitions/uint32'
> + description:
> + Mask that indicates which ports the HBA supports. Useful if PI is not
> + programmed by the BIOS, which is true for some embedded SoC's.
> + maximum: 0x1f
> +
> +patternProperties:
> + "^sata-port@[0-9a-f]+$":
> + type: object
> + description:
> + It is optionally possible to describe the ports as sub-nodes so
> + to enable each port independently when dealing with multiple PHYs.
> +
> + properties:
> + reg:
> + description: AHCI SATA port identifier
> + maxItems: 1
> +
> + phys:
> + description: Individual AHCI SATA port PHY
> + maxItems: 1
> +
> + phy-names:
> + description: AHCI SATA port PHY ID
> + maxItems: 1
> +
> + target-supply:
> + description: Power regulator for SATA port target device
> +
> + required:
> + - reg
> +
> + additionalProperties: true
> +
> +required:
> + - reg
> + - interrupts
> +
> +additionalProperties: true
> +
> +...
> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> index 9304e4731965..76075d3c8987 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> @@ -36,8 +36,7 @@ select:
> - compatible
>
> allOf:
> - - $ref: "sata-common.yaml#"
> -
> + - $ref: "ahci-common.yaml#"
>
What happened to 'sata-common.yaml' ?
Not needed anymore? Included via other means?
Please clarify.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Thu, May 12, 2022 at 02:17:49AM +0300, Serge Semin wrote:
> In order to create a more sophisticated AHCI controller DT bindings let's
> divide the already available generic AHCI platform YAML schema into the
> platform part and a set of the common AHCI properties. The former part
> will be used to evaluate the AHCI DT nodes mainly compatible with the
> generic AHCI controller while the later schema will be used for more
> thorough AHCI DT nodes description. For instance such YAML schemas design
> will be useful for our DW AHCI SATA controller derivative with four clock
> sources, two reset lines, one system controller reference and specific
> max Rx/Tx DMA xfers size constraints.
>
> Note the phys and target-supply property requirement is preserved in the
> generic AHCI platform bindings because some platforms can lack of the
> explicitly specified PHYs or target device power regulators.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Folks, I don't really see why the phys/target-supply requirement has been
> added to the generic AHCI DT schema in the first place. Probably just to
> imply some meaning for the sub-nodes definition. Anyway in one of the
> further patches I am adding the DW AHCI SATA controller DT bindings which
> won't require having these properties specified in the sub-nodes, but will
> describe additional port-specific properties. That's why I get to keep the
> constraints in the ahci-platform.yaml schema instead of moving them to the
> common schema.
>
> Changelog v2:
> - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
>
> Changelog v3:
> - Replace Jens's email address with Damien's one in the list of the
> schema maintainers. (@Damien)
> ---
> .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> .../bindings/ata/ahci-platform.yaml | 68 +---------
> 2 files changed, 123 insertions(+), 62 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> new file mode 100644
> index 000000000000..620042ca12e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common Properties for Serial ATA AHCI controllers
> +
> +maintainers:
> + - Hans de Goede <[email protected]>
> + - Damien Le Moal <[email protected]>
> +
> +description:
> + This document defines device tree properties for a common AHCI SATA
> + controller implementation. It's hardware interface is supposed to
> + conform to the technical standard defined by Intel (see Serial ATA
> + Advanced Host Controller Interface specification for details). The
> + document doesn't constitute a DT-node binding by itself but merely
> + defines a set of common properties for the AHCI-compatible devices.
> +
> +select: false
> +
> +allOf:
> + - $ref: sata-common.yaml#
> +
> +properties:
> + reg:
> + description:
> + Generic AHCI registers space conforming to the Serial ATA AHCI
> + specification.
> +
> + reg-names:
> + description: CSR space IDs
> +
> + interrupts:
> + description:
> + Generic AHCI state change interrupt. Can be implemented either as a
> + single line attached to the controller as a set of the dedicated signals
> + for the global and particular port events.
> +
> + clocks:
> + description:
> + List of all the reference clocks connected to the controller.
> +
> + clock-names:
> + description: Reference clocks IDs
> +
> + resets:
> + description:
> + List of the reset control lines to reset the controller clock
> + domains.
> +
> + reset-names:
> + description: Reset line IDs
> +
> + power-domains:
> + description:
> + List of the power domain the AHCI controller being a part of.
There's not really any point in listing all the above properties here,
because they all have to be listed in the device specific schemas.
> +
> + ahci-supply:
> + description: Power regulator for AHCI controller
> +
> + target-supply:
> + description: Power regulator for SATA target device
> +
> + phy-supply:
> + description: Power regulator for SATA PHY
> +
> + phys:
> + description: Reference to the SATA PHY node
> + maxItems: 1
> +
> + phy-names:
> + maxItems: 1
> +
> + ports-implemented:
> + $ref: '/schemas/types.yaml#/definitions/uint32'
> + description:
> + Mask that indicates which ports the HBA supports. Useful if PI is not
> + programmed by the BIOS, which is true for some embedded SoC's.
> + maximum: 0x1f
The AHCI spec says there's a max of 32 ports, not 5.
https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1-3-1.pdf
> +
> +patternProperties:
> + "^sata-port@[0-9a-f]+$":
> + type: object
> + description:
> + It is optionally possible to describe the ports as sub-nodes so
> + to enable each port independently when dealing with multiple PHYs.
> +
> + properties:
> + reg:
> + description: AHCI SATA port identifier
> + maxItems: 1
> +
> + phys:
> + description: Individual AHCI SATA port PHY
> + maxItems: 1
> +
> + phy-names:
> + description: AHCI SATA port PHY ID
> + maxItems: 1
> +
> + target-supply:
> + description: Power regulator for SATA port target device
> +
> + required:
> + - reg
> +
> + additionalProperties: true
If device specific bindings can add their own properties (as this
allows), then all the common sata-port properties needs to be its own
schema document. That way the device binding can reference it, define
extra properties and set 'unevaluatedProperties: false'.
If other properties aren't allowed, then you can just change this to
false. That's what we had before this change.
> +
> +required:
> + - reg
> + - interrupts
> +
> +additionalProperties: true
> +
> +...
> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> index 9304e4731965..76075d3c8987 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> @@ -36,8 +36,7 @@ select:
> - compatible
>
> allOf:
> - - $ref: "sata-common.yaml#"
> -
> + - $ref: "ahci-common.yaml#"
>
> properties:
> compatible:
> @@ -69,90 +68,35 @@ properties:
> maxItems: 1
>
> clocks:
> - description:
> - Clock IDs array as required by the controller.
> minItems: 1
> maxItems: 3
>
> clock-names:
> - description:
> - Names of clocks corresponding to IDs in the clock property.
> minItems: 1
> maxItems: 3
>
> interrupts:
> maxItems: 1
>
> - ahci-supply:
> - description:
> - regulator for AHCI controller
> -
> - phy-supply:
> - description:
> - regulator for PHY power
> -
> - phys:
> - description:
> - List of all PHYs on this controller
> - maxItems: 1
> -
> - phy-names:
> - description:
> - Name specifier for the PHYs
> - maxItems: 1
> -
> - ports-implemented:
> - $ref: '/schemas/types.yaml#/definitions/uint32'
> - description: |
> - Mask that indicates which ports that the HBA supports
> - are available for software to use. Useful if PORTS_IMPL
> - is not programmed by the BIOS, which is true with
> - some embedded SoCs.
> - maximum: 0x1f
> -
> power-domains:
> maxItems: 1
>
> resets:
> maxItems: 1
>
> - target-supply:
> - description:
> - regulator for SATA target power
> -
> -required:
> - - compatible
> - - reg
> - - interrupts
> -
> patternProperties:
> "^sata-port@[0-9a-f]+$":
> type: object
> - additionalProperties: false
> - description:
> - Subnode with configuration of the Ports.
> -
> - properties:
> - reg:
> - maxItems: 1
> -
> - phys:
> - maxItems: 1
> -
> - phy-names:
> - maxItems: 1
> -
> - target-supply:
> - description:
> - regulator for SATA target power
> -
> - required:
> - - reg
>
> anyOf:
> - required: [ phys ]
> - required: [ target-supply ]
>
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> unevaluatedProperties: false
>
> examples:
> --
> 2.35.1
>
>
On Tue, May 17, 2022 at 02:10:55PM -0500, Rob Herring wrote:
> On Thu, May 12, 2022 at 02:17:49AM +0300, Serge Semin wrote:
> > In order to create a more sophisticated AHCI controller DT bindings let's
> > divide the already available generic AHCI platform YAML schema into the
> > platform part and a set of the common AHCI properties. The former part
> > will be used to evaluate the AHCI DT nodes mainly compatible with the
> > generic AHCI controller while the later schema will be used for more
> > thorough AHCI DT nodes description. For instance such YAML schemas design
> > will be useful for our DW AHCI SATA controller derivative with four clock
> > sources, two reset lines, one system controller reference and specific
> > max Rx/Tx DMA xfers size constraints.
> >
> > Note the phys and target-supply property requirement is preserved in the
> > generic AHCI platform bindings because some platforms can lack of the
> > explicitly specified PHYs or target device power regulators.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Folks, I don't really see why the phys/target-supply requirement has been
> > added to the generic AHCI DT schema in the first place. Probably just to
> > imply some meaning for the sub-nodes definition. Anyway in one of the
> > further patches I am adding the DW AHCI SATA controller DT bindings which
> > won't require having these properties specified in the sub-nodes, but will
> > describe additional port-specific properties. That's why I get to keep the
> > constraints in the ahci-platform.yaml schema instead of moving them to the
> > common schema.
> >
> > Changelog v2:
> > - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
> >
> > Changelog v3:
> > - Replace Jens's email address with Damien's one in the list of the
> > schema maintainers. (@Damien)
> > ---
> > .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> > .../bindings/ata/ahci-platform.yaml | 68 +---------
> > 2 files changed, 123 insertions(+), 62 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > new file mode 100644
> > index 000000000000..620042ca12e7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common Properties for Serial ATA AHCI controllers
> > +
> > +maintainers:
> > + - Hans de Goede <[email protected]>
> > + - Damien Le Moal <[email protected]>
> > +
> > +description:
> > + This document defines device tree properties for a common AHCI SATA
> > + controller implementation. It's hardware interface is supposed to
> > + conform to the technical standard defined by Intel (see Serial ATA
> > + Advanced Host Controller Interface specification for details). The
> > + document doesn't constitute a DT-node binding by itself but merely
> > + defines a set of common properties for the AHCI-compatible devices.
> > +
> > +select: false
> > +
> > +allOf:
> > + - $ref: sata-common.yaml#
> > +
> > +properties:
> > + reg:
> > + description:
> > + Generic AHCI registers space conforming to the Serial ATA AHCI
> > + specification.
> > +
> > + reg-names:
> > + description: CSR space IDs
> > +
> > + interrupts:
> > + description:
> > + Generic AHCI state change interrupt. Can be implemented either as a
> > + single line attached to the controller as a set of the dedicated signals
> > + for the global and particular port events.
> > +
> > + clocks:
> > + description:
> > + List of all the reference clocks connected to the controller.
> > +
> > + clock-names:
> > + description: Reference clocks IDs
> > +
> > + resets:
> > + description:
> > + List of the reset control lines to reset the controller clock
> > + domains.
> > +
> > + reset-names:
> > + description: Reset line IDs
> > +
> > + power-domains:
> > + description:
> > + List of the power domain the AHCI controller being a part of.
>
> There's not really any point in listing all the above properties here,
> because they all have to be listed in the device specific schemas.
I agree with dropping the reset, clocks and power-related properties,
but it would be good to somehow signify that at least one IRQ is
required. Is it possible to somehow set such constraint with open
upper bound? If currently it isn't what about setting minItems: 1 (one
generic IRQ) and maxItems: 32 (in case of the per-port IRQs platform)?
Regarding the reg and reg-names properties. Some constraints are added
in one of the next patches of this series (you have already noticed
that).
>
> > +
> > + ahci-supply:
> > + description: Power regulator for AHCI controller
> > +
> > + target-supply:
> > + description: Power regulator for SATA target device
> > +
> > + phy-supply:
> > + description: Power regulator for SATA PHY
> > +
> > + phys:
> > + description: Reference to the SATA PHY node
> > + maxItems: 1
> > +
> > + phy-names:
> > + maxItems: 1
> > +
> > + ports-implemented:
> > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > + description:
> > + Mask that indicates which ports the HBA supports. Useful if PI is not
> > + programmed by the BIOS, which is true for some embedded SoC's.
> > + maximum: 0x1f
>
> The AHCI spec says there's a max of 32 ports, not 5.
>
> https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1-3-1.pdf
Right. The maximum constraint is dropped in the patch:
[PATCH v3 03/23] dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints
>
> > +
> > +patternProperties:
> > + "^sata-port@[0-9a-f]+$":
> > + type: object
> > + description:
> > + It is optionally possible to describe the ports as sub-nodes so
> > + to enable each port independently when dealing with multiple PHYs.
> > +
> > + properties:
> > + reg:
> > + description: AHCI SATA port identifier
> > + maxItems: 1
> > +
> > + phys:
> > + description: Individual AHCI SATA port PHY
> > + maxItems: 1
> > +
> > + phy-names:
> > + description: AHCI SATA port PHY ID
> > + maxItems: 1
> > +
> > + target-supply:
> > + description: Power regulator for SATA port target device
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: true
>
> If device specific bindings can add their own properties (as this
> allows), then all the common sata-port properties needs to be its own
> schema document. That way the device binding can reference it, define
> extra properties and set 'unevaluatedProperties: false'.
>
Could you please be more specific the way it is supposed to look? We
have already got the sata-port@.* object defined in the sata-common.yaml
super-schema. Here I just redefine it with more specific properties.
Is it ok if I moved the sata-port@.* properties in the "definitions"
section of the sata-common.yaml and ahci-common.yaml schema and
re-used them right in the common bindings and, if required, in the
device-specific schema?
Please confirm that the next schema hierarchy is what you were talking
about and it will be ok in this case:
> Documentation/devicetree/bindings/ata/sata-common.yaml:
...
+ patternProperties:
+ "^sata-port@[0-9a-e]$":
+ $ref: '#/definitions/sata-port'
+
+ definitions:
+ sata-port:
+ type: object
+
+ properties:
+ reg:
+ minimum: 0
+
+ additionalProperties: true
> Documentation/devicetree/bindings/ata/ahci-common.yaml:
...
+ patternProperties:
+ "^sata-port@[0-9a-e]$":
+ $ref: '#/definitions/ahci-port'
+
+ definitions:
+ ahci-port:
+ $ref: /schemas/ata/sata-common.yaml#/definitions/sata-port
+ properties:
+ reg:
+ minimum: 0
+ maximum: 31
...
+
+ additionalProperties: true
> Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml:
...
+ patternProperties:
+ "^sata-port@[0-9a-e]$":
+ $ref: /schemas/ata/ahci-common.yaml#/definitions/ahci-port
+ properties:
+ reg:
+ minimum: 0
+ maximum: 7
+
+ snps,tx-ts-max:
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ snps,rx-ts-max:
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ unevaluatedProperties: true
So what do you think about the schemas hierarchy above?
> If other properties aren't allowed, then you can just change this to
> false. That's what we had before this change.
Before this change the schemas design was the same except it was intended
that the sub-schemas can't extend the sata-port node properties set
(ahci-platform.yaml had additional properties disallowed),
while both the ahci-platform.yaml and parental sata-common.yaml
schemas have sata-port object definition.
In my case for just a lucky coincident it turned out that each sub-schemas
has the sata-port object properties set extended with properties and
new constraints. Anyway if what I suggested in the previous paragraph
is ok for you, I'll update the patch that way.
-Sergey
>
> > +
> > +required:
> > + - reg
> > + - interrupts
> > +
> > +additionalProperties: true
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > index 9304e4731965..76075d3c8987 100644
> > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > @@ -36,8 +36,7 @@ select:
> > - compatible
> >
> > allOf:
> > - - $ref: "sata-common.yaml#"
> > -
> > + - $ref: "ahci-common.yaml#"
> >
> > properties:
> > compatible:
> > @@ -69,90 +68,35 @@ properties:
> > maxItems: 1
> >
> > clocks:
> > - description:
> > - Clock IDs array as required by the controller.
> > minItems: 1
> > maxItems: 3
> >
> > clock-names:
> > - description:
> > - Names of clocks corresponding to IDs in the clock property.
> > minItems: 1
> > maxItems: 3
> >
> > interrupts:
> > maxItems: 1
> >
> > - ahci-supply:
> > - description:
> > - regulator for AHCI controller
> > -
> > - phy-supply:
> > - description:
> > - regulator for PHY power
> > -
> > - phys:
> > - description:
> > - List of all PHYs on this controller
> > - maxItems: 1
> > -
> > - phy-names:
> > - description:
> > - Name specifier for the PHYs
> > - maxItems: 1
> > -
> > - ports-implemented:
> > - $ref: '/schemas/types.yaml#/definitions/uint32'
> > - description: |
> > - Mask that indicates which ports that the HBA supports
> > - are available for software to use. Useful if PORTS_IMPL
> > - is not programmed by the BIOS, which is true with
> > - some embedded SoCs.
> > - maximum: 0x1f
> > -
> > power-domains:
> > maxItems: 1
> >
> > resets:
> > maxItems: 1
> >
> > - target-supply:
> > - description:
> > - regulator for SATA target power
> > -
> > -required:
> > - - compatible
> > - - reg
> > - - interrupts
> > -
> > patternProperties:
> > "^sata-port@[0-9a-f]+$":
> > type: object
> > - additionalProperties: false
> > - description:
> > - Subnode with configuration of the Ports.
> > -
> > - properties:
> > - reg:
> > - maxItems: 1
> > -
> > - phys:
> > - maxItems: 1
> > -
> > - phy-names:
> > - maxItems: 1
> > -
> > - target-supply:
> > - description:
> > - regulator for SATA target power
> > -
> > - required:
> > - - reg
> >
> > anyOf:
> > - required: [ phys ]
> > - required: [ target-supply ]
> >
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > +
> > unevaluatedProperties: false
> >
> > examples:
> > --
> > 2.35.1
> >
> >
On Sun, May 22, 2022 at 06:02:47PM +0300, Serge Semin wrote:
> On Tue, May 17, 2022 at 02:10:55PM -0500, Rob Herring wrote:
> > On Thu, May 12, 2022 at 02:17:49AM +0300, Serge Semin wrote:
> > > In order to create a more sophisticated AHCI controller DT bindings let's
> > > divide the already available generic AHCI platform YAML schema into the
> > > platform part and a set of the common AHCI properties. The former part
> > > will be used to evaluate the AHCI DT nodes mainly compatible with the
> > > generic AHCI controller while the later schema will be used for more
> > > thorough AHCI DT nodes description. For instance such YAML schemas design
> > > will be useful for our DW AHCI SATA controller derivative with four clock
> > > sources, two reset lines, one system controller reference and specific
> > > max Rx/Tx DMA xfers size constraints.
> > >
> > > Note the phys and target-supply property requirement is preserved in the
> > > generic AHCI platform bindings because some platforms can lack of the
> > > explicitly specified PHYs or target device power regulators.
> > >
> > > Signed-off-by: Serge Semin <[email protected]>
> > >
> > > ---
> > >
> > > Folks, I don't really see why the phys/target-supply requirement has been
> > > added to the generic AHCI DT schema in the first place. Probably just to
> > > imply some meaning for the sub-nodes definition. Anyway in one of the
> > > further patches I am adding the DW AHCI SATA controller DT bindings which
> > > won't require having these properties specified in the sub-nodes, but will
> > > describe additional port-specific properties. That's why I get to keep the
> > > constraints in the ahci-platform.yaml schema instead of moving them to the
> > > common schema.
> > >
> > > Changelog v2:
> > > - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
> > >
> > > Changelog v3:
> > > - Replace Jens's email address with Damien's one in the list of the
> > > schema maintainers. (@Damien)
> > > ---
> > > .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> > > .../bindings/ata/ahci-platform.yaml | 68 +---------
> > > 2 files changed, 123 insertions(+), 62 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > > new file mode 100644
> > > index 000000000000..620042ca12e7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > > @@ -0,0 +1,117 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Common Properties for Serial ATA AHCI controllers
> > > +
> > > +maintainers:
> > > + - Hans de Goede <[email protected]>
> > > + - Damien Le Moal <[email protected]>
> > > +
> > > +description:
> > > + This document defines device tree properties for a common AHCI SATA
> > > + controller implementation. It's hardware interface is supposed to
> > > + conform to the technical standard defined by Intel (see Serial ATA
> > > + Advanced Host Controller Interface specification for details). The
> > > + document doesn't constitute a DT-node binding by itself but merely
> > > + defines a set of common properties for the AHCI-compatible devices.
> > > +
> > > +select: false
> > > +
> > > +allOf:
> > > + - $ref: sata-common.yaml#
> > > +
> > > +properties:
> > > + reg:
> > > + description:
> > > + Generic AHCI registers space conforming to the Serial ATA AHCI
> > > + specification.
> > > +
> > > + reg-names:
> > > + description: CSR space IDs
> > > +
> > > + interrupts:
> > > + description:
> > > + Generic AHCI state change interrupt. Can be implemented either as a
> > > + single line attached to the controller as a set of the dedicated signals
> > > + for the global and particular port events.
> > > +
> > > + clocks:
> > > + description:
> > > + List of all the reference clocks connected to the controller.
> > > +
> > > + clock-names:
> > > + description: Reference clocks IDs
> > > +
> > > + resets:
> > > + description:
> > > + List of the reset control lines to reset the controller clock
> > > + domains.
> > > +
> > > + reset-names:
> > > + description: Reset line IDs
> > > +
> > > + power-domains:
> > > + description:
> > > + List of the power domain the AHCI controller being a part of.
> >
>
> > There's not really any point in listing all the above properties here,
> > because they all have to be listed in the device specific schemas.
>
> I agree with dropping the reset, clocks and power-related properties,
> but it would be good to somehow signify that at least one IRQ is
> required. Is it possible to somehow set such constraint with open
> upper bound? If currently it isn't what about setting minItems: 1 (one
> generic IRQ) and maxItems: 32 (in case of the per-port IRQs platform)?
required:
- interrupts
>
> Regarding the reg and reg-names properties. Some constraints are added
> in one of the next patches of this series (you have already noticed
> that).
>
> >
> > > +
> > > + ahci-supply:
> > > + description: Power regulator for AHCI controller
> > > +
> > > + target-supply:
> > > + description: Power regulator for SATA target device
> > > +
> > > + phy-supply:
> > > + description: Power regulator for SATA PHY
> > > +
> > > + phys:
> > > + description: Reference to the SATA PHY node
> > > + maxItems: 1
> > > +
> > > + phy-names:
> > > + maxItems: 1
> > > +
> > > + ports-implemented:
> > > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > > + description:
> > > + Mask that indicates which ports the HBA supports. Useful if PI is not
> > > + programmed by the BIOS, which is true for some embedded SoC's.
> > > + maximum: 0x1f
> >
>
> > The AHCI spec says there's a max of 32 ports, not 5.
> >
> > https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1-3-1.pdf
>
> Right. The maximum constraint is dropped in the patch:
> [PATCH v3 03/23] dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints
>
> >
> > > +
> > > +patternProperties:
> > > + "^sata-port@[0-9a-f]+$":
> > > + type: object
> > > + description:
> > > + It is optionally possible to describe the ports as sub-nodes so
> > > + to enable each port independently when dealing with multiple PHYs.
> > > +
> > > + properties:
> > > + reg:
> > > + description: AHCI SATA port identifier
> > > + maxItems: 1
> > > +
> > > + phys:
> > > + description: Individual AHCI SATA port PHY
> > > + maxItems: 1
> > > +
> > > + phy-names:
> > > + description: AHCI SATA port PHY ID
> > > + maxItems: 1
> > > +
> > > + target-supply:
> > > + description: Power regulator for SATA port target device
> > > +
> > > + required:
> > > + - reg
> > > +
> > > + additionalProperties: true
> >
>
> > If device specific bindings can add their own properties (as this
> > allows), then all the common sata-port properties needs to be its own
> > schema document. That way the device binding can reference it, define
> > extra properties and set 'unevaluatedProperties: false'.
> >
>
> Could you please be more specific the way it is supposed to look? We
> have already got the sata-port@.* object defined in the sata-common.yaml
> super-schema. Here I just redefine it with more specific properties.
If you want an example, see spi-peripheral-props.yaml and the change
that introduced it.
If properties are defined in multiple locations, we have to be able to
combine all those schemas to a single (logical, not single file) schema
to apply it. That's the only way all the disjoint properties can be
evaluated.
> Is it ok if I moved the sata-port@.* properties in the "definitions"
> section of the sata-common.yaml and ahci-common.yaml schema and
> re-used them right in the common bindings and, if required, in the
> device-specific schema?
Yes, I guess. There's not really any advantage to doing that. A separate
schema file is only a small amount of boilerplate.
> Please confirm that the next schema hierarchy is what you were talking
> about and it will be ok in this case:
>
> > Documentation/devicetree/bindings/ata/sata-common.yaml:
> ...
> + patternProperties:
> + "^sata-port@[0-9a-e]$":
> + $ref: '#/definitions/sata-port'
> +
> + definitions:
'$defs' is preferred over 'definitions'.
> + sata-port:
> + type: object
> +
> + properties:
> + reg:
> + minimum: 0
That's the default.
> +
> + additionalProperties: true
Drop this.
>
> > Documentation/devicetree/bindings/ata/ahci-common.yaml:
> ...
> + patternProperties:
> + "^sata-port@[0-9a-e]$":
> + $ref: '#/definitions/ahci-port'
> +
> + definitions:
> + ahci-port:
> + $ref: /schemas/ata/sata-common.yaml#/definitions/sata-port
> + properties:
> + reg:
> + minimum: 0
> + maximum: 31
> ...
> +
> + additionalProperties: true
Drop this.
>
> > Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml:
> ...
> + patternProperties:
> + "^sata-port@[0-9a-e]$":
> + $ref: /schemas/ata/ahci-common.yaml#/definitions/ahci-port
> + properties:
> + reg:
> + minimum: 0
> + maximum: 7
> +
> + snps,tx-ts-max:
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + snps,rx-ts-max:
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + unevaluatedProperties: true
This needs to be false. And this should work as the $ref issue is only
for the top-level schema.
Rob
On Tue, May 24, 2022 at 10:19:14AM -0500, Rob Herring wrote:
> On Sun, May 22, 2022 at 06:02:47PM +0300, Serge Semin wrote:
> > On Tue, May 17, 2022 at 02:10:55PM -0500, Rob Herring wrote:
> > > On Thu, May 12, 2022 at 02:17:49AM +0300, Serge Semin wrote:
> > > > In order to create a more sophisticated AHCI controller DT bindings let's
> > > > divide the already available generic AHCI platform YAML schema into the
> > > > platform part and a set of the common AHCI properties. The former part
> > > > will be used to evaluate the AHCI DT nodes mainly compatible with the
> > > > generic AHCI controller while the later schema will be used for more
> > > > thorough AHCI DT nodes description. For instance such YAML schemas design
> > > > will be useful for our DW AHCI SATA controller derivative with four clock
> > > > sources, two reset lines, one system controller reference and specific
> > > > max Rx/Tx DMA xfers size constraints.
> > > >
> > > > Note the phys and target-supply property requirement is preserved in the
> > > > generic AHCI platform bindings because some platforms can lack of the
> > > > explicitly specified PHYs or target device power regulators.
> > > >
> > > > Signed-off-by: Serge Semin <[email protected]>
> > > >
> > > > ---
> > > >
> > > > Folks, I don't really see why the phys/target-supply requirement has been
> > > > added to the generic AHCI DT schema in the first place. Probably just to
> > > > imply some meaning for the sub-nodes definition. Anyway in one of the
> > > > further patches I am adding the DW AHCI SATA controller DT bindings which
> > > > won't require having these properties specified in the sub-nodes, but will
> > > > describe additional port-specific properties. That's why I get to keep the
> > > > constraints in the ahci-platform.yaml schema instead of moving them to the
> > > > common schema.
> > > >
> > > > Changelog v2:
> > > > - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
> > > >
> > > > Changelog v3:
> > > > - Replace Jens's email address with Damien's one in the list of the
> > > > schema maintainers. (@Damien)
> > > > ---
> > > > .../devicetree/bindings/ata/ahci-common.yaml | 117 ++++++++++++++++++
> > > > .../bindings/ata/ahci-platform.yaml | 68 +---------
> > > > 2 files changed, 123 insertions(+), 62 deletions(-)
> > > > create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > > > new file mode 100644
> > > > index 000000000000..620042ca12e7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > > > @@ -0,0 +1,117 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/ata/ahci-common.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Common Properties for Serial ATA AHCI controllers
> > > > +
> > > > +maintainers:
> > > > + - Hans de Goede <[email protected]>
> > > > + - Damien Le Moal <[email protected]>
> > > > +
> > > > +description:
> > > > + This document defines device tree properties for a common AHCI SATA
> > > > + controller implementation. It's hardware interface is supposed to
> > > > + conform to the technical standard defined by Intel (see Serial ATA
> > > > + Advanced Host Controller Interface specification for details). The
> > > > + document doesn't constitute a DT-node binding by itself but merely
> > > > + defines a set of common properties for the AHCI-compatible devices.
> > > > +
> > > > +select: false
> > > > +
> > > > +allOf:
> > > > + - $ref: sata-common.yaml#
> > > > +
> > > > +properties:
> > > > + reg:
> > > > + description:
> > > > + Generic AHCI registers space conforming to the Serial ATA AHCI
> > > > + specification.
> > > > +
> > > > + reg-names:
> > > > + description: CSR space IDs
> > > > +
> > > > + interrupts:
> > > > + description:
> > > > + Generic AHCI state change interrupt. Can be implemented either as a
> > > > + single line attached to the controller as a set of the dedicated signals
> > > > + for the global and particular port events.
> > > > +
> > > > + clocks:
> > > > + description:
> > > > + List of all the reference clocks connected to the controller.
> > > > +
> > > > + clock-names:
> > > > + description: Reference clocks IDs
> > > > +
> > > > + resets:
> > > > + description:
> > > > + List of the reset control lines to reset the controller clock
> > > > + domains.
> > > > +
> > > > + reset-names:
> > > > + description: Reset line IDs
> > > > +
> > > > + power-domains:
> > > > + description:
> > > > + List of the power domain the AHCI controller being a part of.
> > >
> >
> > > There's not really any point in listing all the above properties here,
> > > because they all have to be listed in the device specific schemas.
> >
> > I agree with dropping the reset, clocks and power-related properties,
> > but it would be good to somehow signify that at least one IRQ is
> > required. Is it possible to somehow set such constraint with open
> > upper bound? If currently it isn't what about setting minItems: 1 (one
> > generic IRQ) and maxItems: 32 (in case of the per-port IRQs platform)?
>
> required:
> - interrupts
Got it. Thanks. On a second thought specifying maxItems: 32 would be
more descriptive.
>
> >
> > Regarding the reg and reg-names properties. Some constraints are added
> > in one of the next patches of this series (you have already noticed
> > that).
> >
> > >
> > > > +
> > > > + ahci-supply:
> > > > + description: Power regulator for AHCI controller
> > > > +
> > > > + target-supply:
> > > > + description: Power regulator for SATA target device
> > > > +
> > > > + phy-supply:
> > > > + description: Power regulator for SATA PHY
> > > > +
> > > > + phys:
> > > > + description: Reference to the SATA PHY node
> > > > + maxItems: 1
> > > > +
> > > > + phy-names:
> > > > + maxItems: 1
> > > > +
> > > > + ports-implemented:
> > > > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > > > + description:
> > > > + Mask that indicates which ports the HBA supports. Useful if PI is not
> > > > + programmed by the BIOS, which is true for some embedded SoC's.
> > > > + maximum: 0x1f
> > >
> >
> > > The AHCI spec says there's a max of 32 ports, not 5.
> > >
> > > https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1-3-1.pdf
> >
> > Right. The maximum constraint is dropped in the patch:
> > [PATCH v3 03/23] dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints
> >
> > >
> > > > +
> > > > +patternProperties:
> > > > + "^sata-port@[0-9a-f]+$":
> > > > + type: object
> > > > + description:
> > > > + It is optionally possible to describe the ports as sub-nodes so
> > > > + to enable each port independently when dealing with multiple PHYs.
> > > > +
> > > > + properties:
> > > > + reg:
> > > > + description: AHCI SATA port identifier
> > > > + maxItems: 1
> > > > +
> > > > + phys:
> > > > + description: Individual AHCI SATA port PHY
> > > > + maxItems: 1
> > > > +
> > > > + phy-names:
> > > > + description: AHCI SATA port PHY ID
> > > > + maxItems: 1
> > > > +
> > > > + target-supply:
> > > > + description: Power regulator for SATA port target device
> > > > +
> > > > + required:
> > > > + - reg
> > > > +
> > > > + additionalProperties: true
> > >
> >
> > > If device specific bindings can add their own properties (as this
> > > allows), then all the common sata-port properties needs to be its own
> > > schema document. That way the device binding can reference it, define
> > > extra properties and set 'unevaluatedProperties: false'.
> > >
> >
> > Could you please be more specific the way it is supposed to look? We
> > have already got the sata-port@.* object defined in the sata-common.yaml
> > super-schema. Here I just redefine it with more specific properties.
>
> If you want an example, see spi-peripheral-props.yaml and the change
> that introduced it.
>
> If properties are defined in multiple locations, we have to be able to
> combine all those schemas to a single (logical, not single file) schema
> to apply it. That's the only way all the disjoint properties can be
> evaluated.
Hm, why do you refer to the cdns,qspi-nor-peripheral-props.yaml and
samsung,spi-peripheral-props.yaml schema from the common
spi-peripheral-props.yaml schema? In that case you permit having the
vendor-specific properties used in all controllers. It doesn't seem
right. Isn't it would be more natural to create a generic-to-private
hierarchy? Like this:
> spi-peripheral-props.yaml:
+ properties:
+ ...
> cdns,qspi-nor-peripheral-props.yaml:
+ allOf:
+ - $ref: spi-peripheral-props.yaml#
+ properties:
+ ...
> samsung,spi-peripheral-props.yaml:
+ allOf:
+ - $ref: spi-peripheral-props.yaml#
+ properties:
+ ...
Especially seeing you left the generic peripheral-props schema opened for
the additional properties (additionalProperties: true). Afterwards the Cdns
and Samsung SPI DT-schemas would refer to these peripheral props schemas
in the sub-nodes. Like this:
> cdns,qspi-nor.yaml:
+ ...
+ patternProperties:
+ "^.*@[0-9a-f]+$":
+ type: object
+ $ref: spi-peripheral-props.yaml
+ ...
>
> > Is it ok if I moved the sata-port@.* properties in the "definitions"
> > section of the sata-common.yaml and ahci-common.yaml schema and
> > re-used them right in the common bindings and, if required, in the
> > device-specific schema?
>
> Yes, I guess. There's not really any advantage to doing that. A separate
> schema file is only a small amount of boilerplate.
IMO having the common ports definitions in the same schema file as the
corresponding DT-bindings is more readable. You don't have to
open additional files, switch between tabs in order to get to the
referred sub-schema. In addition splitting that much coherent parts
isn't good from the maintainability point of view either.
>
> > Please confirm that the next schema hierarchy is what you were talking
> > about and it will be ok in this case:
> >
> > > Documentation/devicetree/bindings/ata/sata-common.yaml:
> > ...
> > + patternProperties:
> > + "^sata-port@[0-9a-e]$":
> > + $ref: '#/definitions/sata-port'
> > +
> > + definitions:
>
> '$defs' is preferred over 'definitions'.
Ok.
>
> > + sata-port:
> > + type: object
> > +
> > + properties:
> > + reg:
> > + minimum: 0
>
> That's the default.
>
> > +
> > + additionalProperties: true
>
> Drop this.
Ok.
>
> >
> > > Documentation/devicetree/bindings/ata/ahci-common.yaml:
> > ...
> > + patternProperties:
> > + "^sata-port@[0-9a-e]$":
> > + $ref: '#/definitions/ahci-port'
> > +
> > + definitions:
> > + ahci-port:
> > + $ref: /schemas/ata/sata-common.yaml#/definitions/sata-port
> > + properties:
> > + reg:
> > + minimum: 0
> > + maximum: 31
> > ...
> > +
> > + additionalProperties: true
>
> Drop this.
>
> >
> > > Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml:
> > ...
> > + patternProperties:
> > + "^sata-port@[0-9a-e]$":
> > + $ref: /schemas/ata/ahci-common.yaml#/definitions/ahci-port
> > + properties:
> > + reg:
> > + minimum: 0
> > + maximum: 7
> > +
> > + snps,tx-ts-max:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > + snps,rx-ts-max:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > + unevaluatedProperties: true
>
> This needs to be false.
Right. Incorrectly copy-pasted it.
> And this should work as the $ref issue is only
> for the top-level schema.
Do you mean that this will work for the schemas referring the
snps,dwc-ahci.yaml schema only? I suppose the vendor-specific schemas
still can extend it by re-designing the snps,dwc-ahci.yaml DT-binding in
the same way as the generic SATA/AHCI schemas.
-Sergey
>
> Rob
On Fri, May 27, 2022 at 01:10:57PM +0300, Serge Semin wrote:
> On Tue, May 24, 2022 at 10:19:14AM -0500, Rob Herring wrote:
> > On Sun, May 22, 2022 at 06:02:47PM +0300, Serge Semin wrote:
> > > On Tue, May 17, 2022 at 02:10:55PM -0500, Rob Herring wrote:
> > > > On Thu, May 12, 2022 at 02:17:49AM +0300, Serge Semin wrote:
> > > > > In order to create a more sophisticated AHCI controller DT bindings let's
> > > > > divide the already available generic AHCI platform YAML schema into the
> > > > > platform part and a set of the common AHCI properties. The former part
> > > > > will be used to evaluate the AHCI DT nodes mainly compatible with the
> > > > > generic AHCI controller while the later schema will be used for more
> > > > > thorough AHCI DT nodes description. For instance such YAML schemas design
> > > > > will be useful for our DW AHCI SATA controller derivative with four clock
> > > > > sources, two reset lines, one system controller reference and specific
> > > > > max Rx/Tx DMA xfers size constraints.
> > > > >
> > > > > Note the phys and target-supply property requirement is preserved in the
> > > > > generic AHCI platform bindings because some platforms can lack of the
> > > > > explicitly specified PHYs or target device power regulators.
[...]
> > > > > +patternProperties:
> > > > > + "^sata-port@[0-9a-f]+$":
> > > > > + type: object
> > > > > + description:
> > > > > + It is optionally possible to describe the ports as sub-nodes so
> > > > > + to enable each port independently when dealing with multiple PHYs.
> > > > > +
> > > > > + properties:
> > > > > + reg:
> > > > > + description: AHCI SATA port identifier
> > > > > + maxItems: 1
> > > > > +
> > > > > + phys:
> > > > > + description: Individual AHCI SATA port PHY
> > > > > + maxItems: 1
> > > > > +
> > > > > + phy-names:
> > > > > + description: AHCI SATA port PHY ID
> > > > > + maxItems: 1
> > > > > +
> > > > > + target-supply:
> > > > > + description: Power regulator for SATA port target device
> > > > > +
> > > > > + required:
> > > > > + - reg
> > > > > +
> > > > > + additionalProperties: true
> > > >
> > >
> > > > If device specific bindings can add their own properties (as this
> > > > allows), then all the common sata-port properties needs to be its own
> > > > schema document. That way the device binding can reference it, define
> > > > extra properties and set 'unevaluatedProperties: false'.
> > > >
> > >
> > > Could you please be more specific the way it is supposed to look? We
> > > have already got the sata-port@.* object defined in the sata-common.yaml
> > > super-schema. Here I just redefine it with more specific properties.
> >
>
> > If you want an example, see spi-peripheral-props.yaml and the change
> > that introduced it.
> >
> > If properties are defined in multiple locations, we have to be able to
> > combine all those schemas to a single (logical, not single file) schema
> > to apply it. That's the only way all the disjoint properties can be
> > evaluated.
>
> Hm, why do you refer to the cdns,qspi-nor-peripheral-props.yaml and
> samsung,spi-peripheral-props.yaml schema from the common
> spi-peripheral-props.yaml schema? In that case you permit having the
> vendor-specific properties used in all controllers. It doesn't seem
> right. Isn't it would be more natural to create a generic-to-private
> hierarchy? Like this:
It's not 'used in all controllers' but used in all devices. But yes, it
does mean a device node could have any of the properties.
The schema for the device must have all possible properties in its
schema either directly or via $ref's. There's not a way to say if the
parent controller is X, then apply these controller child device
properties.
> > spi-peripheral-props.yaml:
> + properties:
> + ...
>
> > cdns,qspi-nor-peripheral-props.yaml:
> + allOf:
> + - $ref: spi-peripheral-props.yaml#
> + properties:
> + ...
>
> > samsung,spi-peripheral-props.yaml:
Who would apply/$ref this in your schema?
> + allOf:
> + - $ref: spi-peripheral-props.yaml#
> + properties:
> + ...
>
> Especially seeing you left the generic peripheral-props schema opened for
> the additional properties (additionalProperties: true). Afterwards the Cdns
> and Samsung SPI DT-schemas would refer to these peripheral props schemas
> in the sub-nodes. Like this:
> > cdns,qspi-nor.yaml:
> + ...
> + patternProperties:
> + "^.*@[0-9a-f]+$":
> + type: object
> + $ref: spi-peripheral-props.yaml
> + ...
This is the pattern that simply doesn't work. "^.*@[0-9a-f]+$" is
entirely independent of a device schema and there's not 1 schema that
has all possible properties.
We could at least limit nodes to allow one set of controller specific
properties (but not necessarily the correct one):
allOf:
- $ref: spi-peripheral-props.yaml#
- oneOf:
- $ref: samsung,spi-peripheral-props.yaml#
- $ref: cdns,qspi-nor-peripheral-props.yaml#
And then in each of the above schemas we need:
anyOf:
- required: [ vendor,prop1 ]
- required: [ vendor,prop2 ]
- ... for all the controller specific properties
The last part keeps the vendor specific schema from being true if none
of the properties are present.
> > > Is it ok if I moved the sata-port@.* properties in the "definitions"
> > > section of the sata-common.yaml and ahci-common.yaml schema and
> > > re-used them right in the common bindings and, if required, in the
> > > device-specific schema?
> >
>
> > Yes, I guess. There's not really any advantage to doing that. A separate
> > schema file is only a small amount of boilerplate.
>
> IMO having the common ports definitions in the same schema file as the
> corresponding DT-bindings is more readable. You don't have to
> open additional files, switch between tabs in order to get to the
> referred sub-schema. In addition splitting that much coherent parts
> isn't good from the maintainability point of view either.
>
> >
> > > Please confirm that the next schema hierarchy is what you were talking
> > > about and it will be ok in this case:
> > >
> > > > Documentation/devicetree/bindings/ata/sata-common.yaml:
> > > ...
> > > + patternProperties:
> > > + "^sata-port@[0-9a-e]$":
> > > + $ref: '#/definitions/sata-port'
> > > +
> > > + definitions:
> >
>
> > '$defs' is preferred over 'definitions'.
>
> Ok.
>
> >
> > > + sata-port:
> > > + type: object
> > > +
> > > + properties:
> > > + reg:
> > > + minimum: 0
> >
>
> > That's the default.
> >
> > > +
> > > + additionalProperties: true
> >
> > Drop this.
>
> Ok.
>
> >
> > >
> > > > Documentation/devicetree/bindings/ata/ahci-common.yaml:
> > > ...
> > > + patternProperties:
> > > + "^sata-port@[0-9a-e]$":
> > > + $ref: '#/definitions/ahci-port'
> > > +
> > > + definitions:
> > > + ahci-port:
> > > + $ref: /schemas/ata/sata-common.yaml#/definitions/sata-port
> > > + properties:
> > > + reg:
> > > + minimum: 0
> > > + maximum: 31
> > > ...
> > > +
> > > + additionalProperties: true
> >
> > Drop this.
> >
> > >
> > > > Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml:
> > > ...
> > > + patternProperties:
> > > + "^sata-port@[0-9a-e]$":
> > > + $ref: /schemas/ata/ahci-common.yaml#/definitions/ahci-port
> > > + properties:
> > > + reg:
> > > + minimum: 0
> > > + maximum: 7
> > > +
> > > + snps,tx-ts-max:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > + snps,rx-ts-max:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > + unevaluatedProperties: true
> >
>
> > This needs to be false.
>
> Right. Incorrectly copy-pasted it.
>
> > And this should work as the $ref issue is only
> > for the top-level schema.
>
> Do you mean that this will work for the schemas referring the
> snps,dwc-ahci.yaml schema only? I suppose the vendor-specific schemas
> still can extend it by re-designing the snps,dwc-ahci.yaml DT-binding in
> the same way as the generic SATA/AHCI schemas.
I mean it doesn't have the bug in dtschema preventing
unevaluatedProperties from fully working correctly.
Rob