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 <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 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
