| Message ID | 20220511231810.4928-1-Sergey.Semin@baikalelectronics.ru |
|---|---|
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
On 5/12/22 2:18 AM, Serge Semin wrote: > Currently not all of the Port-specific capabilities listed in the > PORT_CMD-enumeration. Let's extend that set with the Cold Presence > Detection and Mechanical Presence Switch attached to the Port flags [1] so > to closeup the set of the platform-specific port-capabilities flags. Note > these flags are supposed to be set by the platform firmware if there is > one. Alternatively as we are about to do they can be set by means of the > OF properties. > > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DEV_MPS and fix the Your code has PORT_IRQ_DMPS instead... > comment there. In accordance with [2] that IRQ flag is supposed to > indicate the state of the signal coming from the Mechanical Presence > Switch. > > [1] Serial ATA AHCI 1.3.1 Specification, p.27 > [2] Serial ATA AHCI 1.3.1 Specification, p.7 > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/ata/ahci.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 7d834deefeb9..f501531bd1b3 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -138,7 +138,7 @@ enum { > PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */ > > PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */ > - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */ > + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */ > PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */ > PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */ > PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */ [...] MBR, Sergey
On 2022/05/12 16:26, Serge Semin wrote: > On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote: >> On 5/12/22 01:17, Serge Semin wrote: >>> Since all the clocks are retrieved by the method >>> ahci_platform_get_resources() there is no need for the LLD (glue) drivers >>> to be looking for some particular of them in the kernel clocks table >>> again. Instead we suggest to add a simple method returning a >>> device-specific clock with passed connection ID if it is managed to be >>> found. Otherwise the function will return NULL. Thus the glue-drivers >>> won't need to either manually touching the hpriv->clks array or calling >>> clk_get()-friends. The AHCI platform drivers will be able to use the new >>> function right after the ahci_platform_get_resources() method invocation >>> and up to the device removal. >>> >>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> >>> >>> --- >>> >>> Changelog v2: >>> - Fix some grammar mistakes in the method description. >>> --- >>> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++ >>> include/linux/ahci_platform.h | 3 +++ >>> 2 files changed, 30 insertions(+) >>> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c >>> index 3cff86c225fd..7ff6626fd569 100644 >>> --- a/drivers/ata/libahci_platform.c >>> +++ b/drivers/ata/libahci_platform.c >>> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) >>> } >>> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); >>> +/** >>> + * ahci_platform_find_clk - Find platform clock >>> + * @hpriv: host private area to store config values >>> + * @con_id: clock connection ID >>> + * >>> + * This function returns a pointer to the clock descriptor of the clock with >>> + * the passed ID. >>> + * >>> + * RETURNS: >>> + * Pointer to the clock descriptor on success otherwise NULL >>> + */ >>> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id) >>> +{ >>> + struct clk *clk = NULL; >>> + int i; >>> + >>> + for (i = 0; i < hpriv->n_clks; i++) { >>> + if (!strcmp(hpriv->clks[i].id, con_id)) { >>> + clk = hpriv->clks[i].clk; >>> + break; >>> + } >>> + } >>> + >>> + return clk; >>> +} >>> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk); >>> + >>> /** >>> * ahci_platform_enable_clks - Enable platform clocks >>> * @hpriv: host private area to store config values >>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h >>> index 49e5383d4222..fd964e6a68d6 100644 >>> --- a/include/linux/ahci_platform.h >>> +++ b/include/linux/ahci_platform.h >>> @@ -13,6 +13,7 @@ >>> #include <linux/compiler.h> >>> +struct clk; >>> struct device; >>> struct ata_port_info; >>> struct ahci_host_priv; >>> @@ -21,6 +22,8 @@ struct scsi_host_template; >>> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv); >>> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv); >>> +struct clk * >>> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id); >>> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); >>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); >>> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv); >> > >> Where is this function being used? > > It will be used in the DWC AHCI SATA driver and can be utilized in the > rest of the drivers to simplify the available clocks access. > BTW Damien asked the same question in v1. My response was the same. Please squash this patch together with the patch introducing the first use of this function. > > -Sergey > >> >> Cheers, >> >> Hannes >> -- >> Dr. Hannes Reinecke Kernel Storage Architect >> hare@suse.de +49 911 74053 688 >> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg >> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Tue, May 17, 2022 at 01:58:41PM -0500, Rob Herring wrote: > On Thu, May 12, 2022 at 02:17:48AM +0300, Serge Semin wrote: > > It's redundant to have the 'dma-coherent' property explicitly specified in > > the DT schema because it's a generic property described in the core > > DT-schema by which the property is always evaluated. > > It is not redundant. > > The core schema defines the property (as a boolean), but this schema > defines it being used in this binding. Otherwise, it won't be allowed. I thought that the generic properties like ranges, dma-ranges, etc including the dma-coherent one due to being defined in the dt-core schema are always evaluated. As such seeing the unevaluatedProperties property is set to false here, they can be used in the DT-nodes with no need to be explicitly specified in the DT node bindings. In addition to that I tested this assumption by dropping the dma-coherent property definition from the AHCI-common schema and executed the DT-bindings check procedure. No error has been spotted: > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml | grep dma-coherent > dma-coherent; > [fancer@mobilestation] kernel $ make -j8 DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml dt_binding_check > LINT Documentation/devicetree/bindings > DTEX Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > DTC Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > CHECK Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts | grep dma-coherent > dma-coherent; > [fancer@mobilestation] kernel $ echo $? > 0 Due to that here are a few backward questions: 1) Am I doing something wrong in the framework of the DT-bindings evaluation? Really I even tried to specify unknown property in the DT-bindings example like "bla-bla-bla;" and no evaluation error was printed. Anyway If what you are saying was correct I would have got an error during the DT-bindings evaluation, but as you can see there was none. 2) Am I wrong in thinking that the unevaluatedProperties setting concerns the generic properties defined in the DT-core schema? If it doesn't concern the generic properties then does it work for the $ref'ed schemas only? Getting back to the patch topic. We need to drop the dma-coherent property from the schema anyway. AHCI-specification doesn't regulate the DMA operations coherency. The dma-coherent property is more specific to the particular controller implementation mainly dependent on the platform settings. So I'll change the patch log, but get to keep the patch in the series. What do you think? -Sergey > > Rob
On Tue, May 17, 2022 at 03:04:11PM -0500, Rob Herring wrote: > On Thu, May 12, 2022 at 02:18:05AM +0300, Serge Semin wrote: > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI > > SATA controller except a few peculiarities and the platform environment > > requirements. In particular it can have one or two reference clocks to > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset > > control for the application clock domain. In addition to that the DMA > > interface of each port can be tuned up to work with the predefined maximum > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA > > device DT binding. > > > > Note the DWC AHCI SATA controller DT-schema has been created in a way so > > to be reused for the vendor-specific DT-schemas (see for example the > > "snps,dwc-ahci" compatible string binding). One of which we are about to > > introduce. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v2: > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob) > > --- > > .../bindings/ata/ahci-platform.yaml | 8 -- > > .../bindings/ata/snps,dwc-ahci.yaml | 123 ++++++++++++++++++ > > 2 files changed, 123 insertions(+), 8 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml > > index 6cad7e86f3bb..4b65966ec23b 100644 > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml > > @@ -30,8 +30,6 @@ select: > > - marvell,armada-3700-ahci > > - marvell,armada-8k-ahci > > - marvell,berlin2q-ahci > > - - snps,dwc-ahci > > - - snps,spear-ahci > > required: > > - compatible > > > > @@ -48,17 +46,11 @@ properties: > > - marvell,berlin2-ahci > > - marvell,berlin2q-ahci > > - const: generic-ahci > > - - items: > > - - enum: > > - - rockchip,rk3568-dwc-ahci > > - - const: snps,dwc-ahci > > - enum: > > - cavium,octeon-7130-ahci > > - hisilicon,hisi-ahci > > - ibm,476gtr-ahci > > - marvell,armada-3700-ahci > > - - snps,dwc-ahci > > - - snps,spear-ahci > > > > reg: > > minItems: 1 > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > > new file mode 100644 > > index 000000000000..a13fd77a451f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml > > @@ -0,0 +1,123 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Synopsys DWC AHCI SATA controller > > + > > +maintainers: > > + - Serge Semin <fancer.lancer@gmail.com> > > + > > +description: > > + This document defines device tree bindings for the Synopsys DWC > > + implementation of the AHCI SATA controller. > > + > > +allOf: > > + - $ref: ahci-common.yaml# > > + > > +properties: > > + compatible: > > + oneOf: > > + - description: Synopsys AHCI SATA-compatible devices > > + contains: > > + const: snps,dwc-ahci > > + - description: SPEAr1340 AHCI SATA device > > + const: snps,spear-ahci > > + - description: Rockhip RK3568 ahci controller > > + const: rockchip,rk3568-dwc-ahci > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + description: > > + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock > > + and embedded PHYs reference clock together with vendor-specific set > > + of clocks. > > + minItems: 1 > > + maxItems: 4 > > + > > + clock-names: > > + contains: > > + anyOf: > > + - description: Application AXI/AHB BIU clock source > > + enum: > > + - aclk > > + - sata > > + - description: SATA Ports reference clock > > + enum: > > + - ref > > + - sata_ref > > + > > + resets: > > + description: > > + At least basic core and application clock domains reset is normally > > + supported by the DWC AHCI SATA controller. Some platform specific > > + clocks can be also specified though. > > + > > + reset-names: > > + contains: > > + description: Core and application clock domains reset control > > + const: arst > > + > > +patternProperties: > > + "^sata-port@[0-9a-e]$": > > + type: object > > + > > + properties: > > + reg: > > + minimum: 0 > > + maximum: 7 > > + > > + snps,tx-ts-max: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: Maximal size of Tx DMA transactions in FIFO words > > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ] > > + > > + snps,rx-ts-max: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: Maximal size of Rx DMA transactions in FIFO words > > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ] > > + > > + additionalProperties: true > > You just defined a DT property called 'additionalProperties'. For this > reason, I prefer placing additionalProperties above 'properties'. Right. Thanks > > As mentioned the way 'sata-port' schemas are done here doesn't work. Please, turn your attention to the emailing thread where you mentioned it: "[PATCH v3 02/23] dt-bindings: ata: ahci-platform: Detach common AHCI bindings" https://lore.kernel.org/linux-ide/20220511231810.4928-3-Sergey.Semin@baikalelectronics.ru/ Before I get to the series re-development I need your confirmation whether what I understand regarding your suggestion was right. -Sergey > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + > > + sata@122f0000 { > > + compatible = "snps,dwc-ahci"; > > + reg = <0x122F0000 0x1ff>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>; > > + > > + clocks = <&clock1>, <&clock2>; > > + clock-names = "aclk", "ref"; > > + > > + phys = <&sata_phy>; > > + phy-names = "sata-phy"; > > + > > + ports-implemented = <0x1>; > > + > > + sata-port@0 { > > + reg = <0>; > > + > > + hba-fbscp; > > + snps,tx-ts-max = <512>; > > + snps,rx-ts-max = <512>; > > + }; > > + }; > > +... > > -- > > 2.35.1 > > > >
On Tue, May 17, 2022 at 03:13:32PM -0500, Rob Herring wrote: > On Thu, May 12, 2022 at 02:18:07AM +0300, Serge Semin wrote: > > Baikal-T1 AHCI controller is based on the DWC AHCI SATA IP-core v4.10a > > with the next specific settings: two SATA ports, cascaded CSR access based > > on two clock domains (APB and AXI), selectable source of the reference > > clock (though stable work is currently available from the external source > > only), two reset lanes for the application and SATA ports domains. Other > > than that the device is fully compatible with the generic DWC AHCI SATA > > bindings. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v2: > > - Rename 'syscon' property to 'baikal,bt1-syscon'. > > - Drop macro usage from the example node. > > --- > > .../bindings/ata/baikal,bt1-ahci.yaml | 127 ++++++++++++++++++ > > 1 file changed, 127 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > > > diff --git a/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > new file mode 100644 > > index 000000000000..7c2eae75434f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > @@ -0,0 +1,127 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/ata/baikal,bt1-ahci.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Baikal-T1 SoC AHCI SATA controller > > + > > +maintainers: > > + - Serge Semin <fancer.lancer@gmail.com> > > + > > +description: | > > + AHCI SATA controller embedded into the Baikal-T1 SoC is based on the > > + DWC AHCI SATA v4.10a IP-core. > > + > > +allOf: > > + - $ref: snps,dwc-ahci.yaml# > > + > > +properties: > > + compatible: > > + contains: > > + const: baikal,bt1-ahci > > + > > + clocks: > > + items: > > + - description: Peripheral APB bus clock source > > + - description: Application AXI BIU clock > > + - description: Internal SATA Ports reference clock > > + - description: External SATA Ports reference clock > > + > > + clock-names: > > + items: > > + - const: pclk > > + - const: aclk > > + - const: ref_int > > + - const: ref_ext > > + > > + resets: > > + items: > > + - description: Application AXI BIU domain reset > > + - description: SATA Ports clock domain reset > > + > > + reset-names: > > + items: > > + - const: arst > > + - const: ref > > + > > + baikal,bt1-syscon: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + Phandle reference to the CCU system controller. It is required to > > + switch between internal and external SATA reference clock sources. > > Seems like the CCU system ctrlr should be a clock provider that provides > 'ref' clock and then assigned-clocks can be used to pick internal vs. > external ref. By assigned-clocks do you mean using the "assigned-clock-parents" property? Does it mean creating additional clocks exported from the CCU controller, which could have got one of the two parental clocks? > > > + > > + ports-implemented: > > + maximum: 0x3 > > + > > +patternProperties: > > + "^sata-port@[0-9a-e]$": > > + type: object > > unevaluatedProperties: false > > and then a $ref to a sata-port schema. Can I set additional sata-port properties constraints afterwards? Like I've done for the reg, snps,tx-ts-max and snps,rx-ts-max properties here? > > > + > > + properties: > > + reg: > > + minimum: 0 > > + maximum: 1 > > + > > + snps,tx-ts-max: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Due to having AXI3 bus interface utilized the maximum Tx DMA > > + transaction size can't exceed 16 beats (AxLEN[3:0]). > > + minimum: 1 > > + maximum: 16 > > + > > + snps,rx-ts-max: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Due to having AXI3 bus interface utilized the maximum Rx DMA > > + transaction size can't exceed 16 beats (AxLEN[3:0]). > > That's not a per port limitation (even though it's a per port register)? > I think this should be implied by the compatible string. The snps,{rx,tx}-ts-max property is a per-port property. I'd better explicitly set the property limitation here rather than having the value implicitly clamped by hardware especially seeing the limitation is set by the formulae (CC_MSTR_BURST_LEN * M_HDATA_WIDTH/32)) / (M_HDATA_WIDTH/32), which consists of the IP-core synthesized parameters. > > Really, firmware should configure this IMO. We don't have comprehensive firmware setting these and generic HBA parameters. In our case dtb is the main platform firmware. -Sergey > > > + minimum: 1 > > + maximum: 16 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - resets > > + - baikal,bt1-syscon > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + sata@1f050000 { > > + compatible = "baikal,bt1-ahci", "snps,dwc-ahci"; > > + reg = <0x1f050000 0x2000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + interrupts = <0 64 4>; > > + > > + clocks = <&ccu_sys 1>, <&ccu_axi 2>, <&ccu_sys 0>, <&clk_sata>; > > + clock-names = "pclk", "aclk", "ref_int", "ref_ext"; > > + > > + resets = <&ccu_axi 2>, <&ccu_sys 0>; > > + reset-names = "arst", "ref"; > > + > > + baikal,bt1-syscon = <&syscon>; > > + > > + ports-implemented = <0x3>; > > + > > + sata-port@0 { > > + reg = <0>; > > + > > + snps,tx-ts-max = <4>; > > + snps,rx-ts-max = <4>; > > + }; > > + > > + sata-port@1 { > > + reg = <1>; > > + > > + snps,tx-ts-max = <4>; > > + snps,rx-ts-max = <4>; > > + }; > > + }; > > +... > > -- > > 2.35.1 > > > >
On Sat, May 21, 2022 at 12:22:48PM +0300, Serge Semin wrote: > On Tue, May 17, 2022 at 01:58:41PM -0500, Rob Herring wrote: > > On Thu, May 12, 2022 at 02:17:48AM +0300, Serge Semin wrote: > > > It's redundant to have the 'dma-coherent' property explicitly specified in > > > the DT schema because it's a generic property described in the core > > > DT-schema by which the property is always evaluated. > > > > > It is not redundant. > > > > The core schema defines the property (as a boolean), but this schema > > defines it being used in this binding. Otherwise, it won't be allowed. > > I thought that the generic properties like ranges, dma-ranges, etc > including the dma-coherent one due to being defined in the dt-core > schema are always evaluated. As such seeing the unevaluatedProperties > property is set to false here, they can be used in the DT-nodes with > no need to be explicitly specified in the DT node bindings. In > addition to that I tested this assumption by dropping the dma-coherent > property definition from the AHCI-common schema and executed the > DT-bindings check procedure. No error has been spotted: Those common properties are always applied, but not at the same time as a device binding. IOW, it's 2 schemas that are applied to an instance (node) independently. For things like 'reg', the common schema does type checks and the device schema does size (number of entries) checks. There a few things always allowed like 'status', and those are added to the device schema by the tools. > > > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml | grep dma-coherent > > dma-coherent; > > [fancer@mobilestation] kernel $ make -j8 DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml dt_binding_check > > LINT Documentation/devicetree/bindings > > DTEX Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > DTC Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > > CHECK Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts | grep dma-coherent > > dma-coherent; > > [fancer@mobilestation] kernel $ echo $? > > 0 > Due to that here are a few backward questions: > 1) Am I doing something wrong in the framework of the DT-bindings > evaluation? Really I even tried to specify unknown property in the > DT-bindings example like "bla-bla-bla;" and no evaluation error was > printed. Anyway If what you are saying was correct I would have got an > error during the DT-bindings evaluation, but as you can see there was > none. I think this is a known issue which has a pending fix. If a referenced schema has 'additionalProperties: true' in it, then the referring schema never has any unevaluated properties. The fix is pending because all the schema examples that start failing have to be fixed and in a base that people work on (i.e. rc1). > 2) Am I wrong in thinking that the unevaluatedProperties setting > concerns the generic properties defined in the DT-core schema? You are wrong as explained above. > If it > doesn't concern the generic properties then does it work for the > $ref'ed schemas only? Yes, except for the issue making it not work. > Getting back to the patch topic. We need to drop the dma-coherent > property from the schema anyway. AHCI-specification doesn't > regulate the DMA operations coherency. The dma-coherent property is > more specific to the particular controller implementation mainly > dependent on the platform settings. So I'll change the patch log, but > get to keep the patch in the series. What do you think? Intel wrote the spec, so they probably assume coherent. In DT, PPC is default coherent and Arm is default non-coherent. You'll need to add it to whatever specific device schemas need it if you remove it. Personally, I think it is fine where it is. dma-coherent is valid on any DMA capable device and it's not really a property of the device, but the system. If we could generically identify DMA capable devices, then dma-coherent would be allowed on them automatically. Rob
On Tue, May 24, 2022 at 09:57:58AM -0500, Rob Herring wrote: > On Sat, May 21, 2022 at 12:22:48PM +0300, Serge Semin wrote: > > On Tue, May 17, 2022 at 01:58:41PM -0500, Rob Herring wrote: > > > On Thu, May 12, 2022 at 02:17:48AM +0300, Serge Semin wrote: > > > > It's redundant to have the 'dma-coherent' property explicitly specified in > > > > the DT schema because it's a generic property described in the core > > > > DT-schema by which the property is always evaluated. > > > > > > > > It is not redundant. > > > > > > The core schema defines the property (as a boolean), but this schema > > > defines it being used in this binding. Otherwise, it won't be allowed. > > > > I thought that the generic properties like ranges, dma-ranges, etc > > including the dma-coherent one due to being defined in the dt-core > > schema are always evaluated. As such seeing the unevaluatedProperties > > property is set to false here, they can be used in the DT-nodes with > > no need to be explicitly specified in the DT node bindings. In > > addition to that I tested this assumption by dropping the dma-coherent > > property definition from the AHCI-common schema and executed the > > DT-bindings check procedure. No error has been spotted: > > Those common properties are always applied, but not at the same time as > a device binding. IOW, it's 2 schemas that are applied to an instance > (node) independently. For things like 'reg', the common schema does type > checks and the device schema does size (number of entries) checks. > > There a few things always allowed like 'status', and those are added to > the device schema by the tools. It makes sense now. Thanks for clarification. > > > > > > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml | grep dma-coherent > > > dma-coherent; > > > [fancer@mobilestation] kernel $ make -j8 DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml dt_binding_check > > > LINT Documentation/devicetree/bindings > > > DTEX Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts > > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > > DTC Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > > > CHECK Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb > > > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts | grep dma-coherent > > > dma-coherent; > > > [fancer@mobilestation] kernel $ echo $? > > > 0 > > Due to that here are a few backward questions: > > 1) Am I doing something wrong in the framework of the DT-bindings > > evaluation? Really I even tried to specify unknown property in the > > DT-bindings example like "bla-bla-bla;" and no evaluation error was > > printed. Anyway If what you are saying was correct I would have got an > > error during the DT-bindings evaluation, but as you can see there was > > none. > > I think this is a known issue which has a pending fix. If a referenced > schema has 'additionalProperties: true' in it, then the referring schema > never has any unevaluated properties. The fix is pending because all > the schema examples that start failing have to be fixed and in a base > that people work on (i.e. rc1). Ok. I see. Just to note in case if a non-related schema error is found the unknown property error is printed too. Like this: /.../ata/snps,dwc-ahci.example.dtb: sata@122f0000: interrupts: [[0, 115, 4], [0, 116, 4]] is too long From schema: /.../ata/snps,dwc-ahci.yaml /../ata/snps,dwc-ahci.example.dtb: sata@122f0000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'phys', 'phy-names', 'ports-implemented', 'bla-bla-bla' were unexpected) If I fix the interrupts-property error, the dt-schema check procedure will work just fine. > > > 2) Am I wrong in thinking that the unevaluatedProperties setting > > concerns the generic properties defined in the DT-core schema? > > You are wrong as explained above. > > > If it > > doesn't concern the generic properties then does it work for the > > $ref'ed schemas only? > > Yes, except for the issue making it not work. > > > Getting back to the patch topic. We need to drop the dma-coherent > > property from the schema anyway. AHCI-specification doesn't > > regulate the DMA operations coherency. The dma-coherent property is > > more specific to the particular controller implementation mainly > > dependent on the platform settings. So I'll change the patch log, but > > get to keep the patch in the series. What do you think? > > Intel wrote the spec, so they probably assume coherent. In DT, PPC is > default coherent and Arm is default non-coherent. > > You'll need to add it to whatever specific device schemas need it if you > remove it. Right. This is what I was going to add to the patch log. > Personally, I think it is fine where it is. dma-coherent is > valid on any DMA capable device and it's not really a property of the > device, but the system. Right. It is mainly the platform property. In particular the DMA coherency is determined by the system interconnect design. In our case the l1 and l2 caches are embedded into the CPU cores block while the DDR and other SoC peripheral devices/controllers are attached to the cores via a dedicated AXI3 interconnect bus, which has nothing to do with the caches. That's why none of the system devices are cache-coherent. > If we could generically identify DMA capable > devices, then dma-coherent would be allowed on them automatically. Got it. I'll drop this patch then. -Sergey > > Rob
On Tue, May 24, 2022 at 10:33:45AM -0500, Rob Herring wrote: > On Sun, May 22, 2022 at 11:49:31PM +0300, Serge Semin wrote: > > On Tue, May 17, 2022 at 03:13:32PM -0500, Rob Herring wrote: > > > On Thu, May 12, 2022 at 02:18:07AM +0300, Serge Semin wrote: > > > > Baikal-T1 AHCI controller is based on the DWC AHCI SATA IP-core v4.10a > > > > with the next specific settings: two SATA ports, cascaded CSR access based > > > > on two clock domains (APB and AXI), selectable source of the reference > > > > clock (though stable work is currently available from the external source > > > > only), two reset lanes for the application and SATA ports domains. Other > > > > than that the device is fully compatible with the generic DWC AHCI SATA > > > > bindings. > > > > > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > --- > > > > > > > > Changelog v2: > > > > - Rename 'syscon' property to 'baikal,bt1-syscon'. > > > > - Drop macro usage from the example node. > > > > --- > > > > .../bindings/ata/baikal,bt1-ahci.yaml | 127 ++++++++++++++++++ > > > > 1 file changed, 127 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > > > new file mode 100644 > > > > index 000000000000..7c2eae75434f > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml > > > > @@ -0,0 +1,127 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/ata/baikal,bt1-ahci.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Baikal-T1 SoC AHCI SATA controller > > > > + > > > > +maintainers: > > > > + - Serge Semin <fancer.lancer@gmail.com> > > > > + > > > > +description: | > > > > + AHCI SATA controller embedded into the Baikal-T1 SoC is based on the > > > > + DWC AHCI SATA v4.10a IP-core. > > > > + > > > > +allOf: > > > > + - $ref: snps,dwc-ahci.yaml# > > > > + > > > > +properties: > > > > + compatible: > > > > + contains: > > > > + const: baikal,bt1-ahci > > > > + > > > > + clocks: > > > > + items: > > > > + - description: Peripheral APB bus clock source > > > > + - description: Application AXI BIU clock > > > > + - description: Internal SATA Ports reference clock > > > > + - description: External SATA Ports reference clock > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: pclk > > > > + - const: aclk > > > > + - const: ref_int > > > > + - const: ref_ext > > > > + > > > > + resets: > > > > + items: > > > > + - description: Application AXI BIU domain reset > > > > + - description: SATA Ports clock domain reset > > > > + > > > > + reset-names: > > > > + items: > > > > + - const: arst > > > > + - const: ref > > > > + > > > > + baikal,bt1-syscon: > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > + description: > > > > + Phandle reference to the CCU system controller. It is required to > > > > + switch between internal and external SATA reference clock sources. > > > > > > > > Seems like the CCU system ctrlr should be a clock provider that provides > > > 'ref' clock and then assigned-clocks can be used to pick internal vs. > > > external ref. > > > > By assigned-clocks do you mean using the "assigned-clock-parents" > > property? > > Yes, I meant any of those properties. > > > Does it mean creating additional clocks exported from the > > CCU controller, which could have got one of the two parental clocks? > > Yes, I believe so. Ok. I hoped to avoid this since it isn't that easy... but seems like I have not choice now.( > > > > > > + > > > > + ports-implemented: > > > > + maximum: 0x3 > > > > + > > > > +patternProperties: > > > > + "^sata-port@[0-9a-e]$": > > > > + type: object > > > > > > unevaluatedProperties: false > > > > > > > > and then a $ref to a sata-port schema. > > > > Can I set additional sata-port properties constraints afterwards? Like > > I've done for the reg, snps,tx-ts-max and snps,rx-ts-max properties > > here? > > Yes. All the constraints are effectively ANDed together. Ok. > > > > > + > > > > + properties: > > > > + reg: > > > > + minimum: 0 > > > > + maximum: 1 > > > > + > > > > + snps,tx-ts-max: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: > > > > + Due to having AXI3 bus interface utilized the maximum Tx DMA > > > > + transaction size can't exceed 16 beats (AxLEN[3:0]). > > > > + minimum: 1 > > > > + maximum: 16 > > > > + > > > > + snps,rx-ts-max: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: > > > > + Due to having AXI3 bus interface utilized the maximum Rx DMA > > > > + transaction size can't exceed 16 beats (AxLEN[3:0]). > > > > > > > > That's not a per port limitation (even though it's a per port register)? > > > I think this should be implied by the compatible string. > > > > The snps,{rx,tx}-ts-max property is a per-port property. I'd better > > explicitly set the property limitation here rather than having the > > value implicitly clamped by hardware especially seeing the limitation > > is set by the formulae > > (CC_MSTR_BURST_LEN * M_HDATA_WIDTH/32)) / (M_HDATA_WIDTH/32), > > which consists of the IP-core synthesized parameters. > > I did not say use the h/w default. > > What I asking is do you have any need for this to be different per port? > Seems unlikely given it's just 1 bus interface for all ports IIRC. I > can't see why you would want to tune the performance per port to > anything but the max burst length. If you have no need, use the > compatible string to determine what to set the register value to. Well it's not what I need, it's about the way the system and AHCI interfaces are used for on the particular platform. The Tx/Rx DMA max burst length affects the system interconnect bus response latency (bus to which all the system devices are attached to: CPU cores, DDR controller, Ethernet, PCIe, SATA, etc). The higher the max-burst length the higher the latency for the other devices to start executing their IO ops. At the same time maximizing the burst length increases the corresponding device performance. Should there be a high priority storage (like system/swap SSD) and a low priority device (data hard drive) attached to the AHCI ports, I would rise the max burst length of the hi-prio device and decrease it for the other one. As such the high-priority traffic would flow with max speed, while the low-priority device would work slower than the other(s) giving a chance for the other devices to start their system bus transfers more frequently. All of that is the platform-specific settings. > > > > Really, firmware should configure this IMO. > > > > We don't have comprehensive firmware setting these and generic HBA parameters. > > In our case dtb is the main platform firmware. > > No u-boot? Aside with the default u-boot bootloader there are customers using their own boot-up software. In addition to that the controller can be hard-reset before being used in the kernel, which defaults all the registers state including the state of the PnDMACR one. -Sergey > > Rob
The main goal of this patchset was to add Baikal-T1 AHCI SATA specifics support into the kernel AHCI subsystem. On the way of doing that we figured out that mainly these specifics are actually DWC AHCI SATA controller features, but still there were some Baikal-T1 SoC platform peculiarities which we had to take into account. So the patchset introduces two AHCI SATA controllers support and one AHCI SATA driver with a series of preparation, optimization and cleanup patches. The series starts used to start with converting the legacy AHCI SATA controllers text-based DT-bindings to the DT-schema. But turned out that has already been done in kernel v5.17. So instead we suggest to improve the bindings usability by splitting up the AHCI DT bindings into two schemas: one common AHCI SATA controller yaml-file, which can be reused by any AHCI-compatible controller utilizing the kernel AHCI library functions, and DT-bindings for the generic AHCI SATA devices indicated by the "generic-ahci" compatible string and implemented in the ahci_platform.c driver. Note after doing that we had to fix the sata-common.yaml file SATA port IDs constraint. Then a series of generic preparations-cleanups goes. First of all it concerns the device-managed methods usage in the framework of the CSR space remapping and the clocks requesting and enabling. Note since the clocks handlers are requested and kept in the generic AHCI library it seemed a good idea to add an AHCI-platform generic method to find and get a particular clock handler from the pool of the requested ones. It was used later in the series in the DWC/Baikal-T1-specific code. Secondly we suggested to at least sanity check the number of SATA ports DT-sub-nodes before using it further. Thirdly the ports-implemented DT-property parsing was moved from the AHCI platform-driver to the AHCI-library so to be used by the non-generic AHCI drivers if required (DT-schema is accordingly fixed too). Finally due to having the shared-reset control support we had to add a new AHCI-resource getter flag - AHCI_PLATFORM_RST_TRIGGER, which indicated using a trigger-like reset control. For such platforms the controller reset will be performed by means of the reset_control_reset() and reset_control_rearm() methods. AHCI-library reset functions encapsulating the way the reset procedure is performed have been also added. After that goes a patches series with the platform-specific AHCI-capabilities initialization. The suggested functionality will be useful for the platforms with no BIOS, comprehensive bootloader/firmware installed. In that case the AHCI-related platform-specifics like drive staggered spin-up, mechanical presence switch attached or FIS-based switching capability usage, etc will be left uninitialized with no generic way to be indicated as available if required. We suggested to use the AHCI device tree node and its ports sub-nodes for that. AHCI-platform library will be responsible fo the corresponding DT-properties parsing and pre-initialization of the internal capability registers cache, which will be then flashed back to the corresponding CSR after HBA reset. Thus a supposed to be firmware-work will be done by means of the AHCI-library and the DT-data. A set of the preparations/cleanups required to be done before introducing the feature. First the DT-properties indicating the corresponding capability availability were described in the common AHCI DT-binding schema. Second we needed to add the enum items with the AHCI Port CMD fields, which hadn't been added so far. Thirdly we suggested to discard one of the port-map internal storage (force_port_map) in favor of re-using another one (save_port_map) in order to simplify the port-map initialization interface a bit by getting rid from a redundant variable. Finally after discarding the double AHCI-version read procedure and changing the __ahci_port_base() method prototype the platform firmware-specific caps initialization functionality was introduced. The main part of the series goes afterwards. A dedicated DWC AHCI SATA controller driver was introduced together with the corresponding DT-binding schema pre-patch. Note the driver built mode is activated synchronously with the generic AHCI-platform driver by default so automatically to be integrated into the kernel for the DWC AHCI-based platforms which relied on activating the generic AHCI SATA controller driver. Aside with the generic resources getting and AHCI-host initialization, the driver implements the DWC-specific setups. In particular it checks whether the platform capabilities activated by the firmware (see the functionality described above) are actually supported by the controller. It's done by means of the vendor-specific registers. Then it makes sure that the embedded 1ms timer interval, which is used for the DevSleep and CCC features, is correctly initialized based on the application clock rate. The last but not least the driver provides a way to tune the DMA-interface performance up by setting the Tx/Rx transactions maximum size up. The required values are specified by means of the "snps,tx-ts-max" and snps,rx-ts-max" DT-properties. Finally we suggest to extend the DWC AHCI SATA controller driver functionality with a way to add the DWC-AHCI-based platform-specific quirks. Indeed there are many DWC AHCI-based controllers and just a few of them are diverged too much to be handled by a dedicated AHCI-driver. The rest of them most likely can work well either with a generic version of the driver or require a simple normally platform-specific quirk to get up and running. Such platforms can define a platform-data in the DWC AHCI driver with a set of the controller-specific flags and initialization functions. Those functions will be called at the corresponding stages of the device probe/resume/remove procedures so to be performing the platform setups/cleanups. After the denoted above functionality is added we can finally introduce the Baikal-T1 AHCI SATA controller support into the DWC AHCI SATA driver. The controller is based on the DWC AHCI SATA IP-core v4.10a and can work well with the generic DWC AHCI driver. The only peculiarity of it is connected with the SATA Ports reference clock source. It can be supplied either from the internal SoC PLL or from the chip pads. Currently we have to prefer selecting the signal coming from the pads if the corresponding clock source is specified because the link doesn't get stably established when the internal clock signal is activated. In addition the platform has trigger-based reset signals so the corresponding flag must be passed to the generic AHCI-resource getter. Link: https://lore.kernel.org/linux-ide/20220324001628.13028-1-Sergey.Semin@baikalelectronics.ru/ Changelog v2: - Rebase from kernel v5.17 to v5.18-rc3. (@Rob) - Rebase onto the already available AHCI DT schema. As a result two more patches have been added. (@Rob) - Rename 'syscon' property to 'baikal,bt1-syscon'. (@Rob) - Replace min/max constraints of the snps,{tx,rx}-ts-max property with enum [ 1, 2, 4, ..., 1024 ]. (@Rob) - Use dlemoal/libata.git git tree for the LIBATA SATA AHCI SYNOPSYS DWC driver (@Damien). - Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_', from 'bt1_ahci_' to 'ahci_bt1_'. (@Damien) - Use LLDD term in place of 'glue-driver'. (@Damien) - Convert the ahci_platform_assert_rsts() method to returning int status (@Damien). - Drop the else word from the DT child_nodes value checking if-else-if statement (@Damien) and convert the after-else part into the ternary operator-based statement. - Convert to checking the error-case first in the devm_clk_bulk_get_all() method invocation. (@Damien) - Drop the rc variable initialization in the ahci_platform_get_resources() method. (@Damien) - Add comma and replace "channel" with "SATA port" in the reg property description of the sata-common.yaml schema. (@Damien) Link: https://lore.kernel.org/lkml/20220503200938.18027-1-Sergey.Semin@baikalelectronics.ru/ Changelog v3: - Replace Jens's email address with Damien's one in the list of the common DT schema maintainers. (@Damien) Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-ide@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Serge Semin (23): dt-bindings: ata: ahci-platform: Drop dma-coherent property declaration dt-bindings: ata: ahci-platform: Detach common AHCI bindings dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints dt-bindings: ata: sata: Extend number of SATA ports ata: libahci_platform: Explicitly set rc on devres_alloc failure ata: libahci_platform: Convert to using platform devm-ioremap methods ata: libahci_platform: Convert to using devm bulk clocks API ata: libahci_platform: Add function returning a clock-handle by id ata: libahci_platform: Sanity check the DT child nodes number ata: libahci_platform: Parse ports-implemented property in resources getter ata: libahci_platform: Introduce reset assertion/deassertion methods dt-bindings: ata: ahci: Add platform capability properties ata: libahci: Extend port-cmd flags set with port capabilities ata: libahci: Discard redundant force_port_map parameter ata: libahci: Don't read AHCI version twice in the save-config method ata: ahci: Convert __ahci_port_base to accepting hpriv as arguments ata: ahci: Introduce firmware-specific caps initialization dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema ata: ahci: Add DWC AHCI SATA controller support dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema ata: ahci-dwc: Add platform-specific quirks support ata: ahci-dwc: Add Baikal-T1 AHCI SATA interface support MAINTAINERS: Add maintainers for DWC AHCI SATA driver .../devicetree/bindings/ata/ahci-common.yaml | 188 +++++++ .../bindings/ata/ahci-platform.yaml | 89 +-- .../bindings/ata/baikal,bt1-ahci.yaml | 127 +++++ .../devicetree/bindings/ata/sata-common.yaml | 7 +- .../bindings/ata/snps,dwc-ahci.yaml | 123 ++++ MAINTAINERS | 9 + drivers/ata/Kconfig | 11 + drivers/ata/Makefile | 1 + drivers/ata/ahci.c | 4 +- drivers/ata/ahci.h | 21 +- drivers/ata/ahci_dwc.c | 526 ++++++++++++++++++ drivers/ata/ahci_mtk.c | 2 - drivers/ata/ahci_platform.c | 5 - drivers/ata/ahci_st.c | 3 - drivers/ata/libahci.c | 63 ++- drivers/ata/libahci_platform.c | 236 ++++++-- include/linux/ahci_platform.h | 8 +- 17 files changed, 1258 insertions(+), 165 deletions(-) create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml create mode 100644 drivers/ata/ahci_dwc.c