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 <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> > > > > > > --- > > > > > > 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 <hdegoede@xxxxxxxxxx> > > > + - Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> > > > + > > > +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
