[{"id":2912972,"web_url":"http://patchwork.ozlabs.org/comment/2912972/","msgid":"<20220614222754.GA2830345-robh@kernel.org>","list_archive_url":null,"date":"2022-06-14T22:27:54","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":62529,"url":"http://patchwork.ozlabs.org/api/people/62529/","name":"Rob Herring (Arm)","email":"robh@kernel.org"},"content":"On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> SATA controller except a few peculiarities and the platform environment\n> requirements. In particular it can have one or two reference clocks to\n> feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> control for the application clock domain. In addition to that the DMA\n> interface of each port can be tuned up to work with the predefined maximum\n> data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> device DT binding.\n> \n> Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> to be reused for the vendor-specific DT-schemas (see for example the\n> \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> introduce.\n> \n> Signed-off-by: Serge Semin \n> \n> ---\n> \n> Changelog v2:\n> - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> \n> Changelog v4:\n> - Decrease the \"additionalProperties\" property identation otherwise it's\n> percieved as the node property instead of the key one. (@Rob)\n> - Use the ahci-port properties definition from the AHCI common schema\n> in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> - Remove the Hannes' rb tag since the patch content has changed.\n> ---\n> .../bindings/ata/ahci-platform.yaml | 8 --\n> .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> 2 files changed, 129 insertions(+), 8 deletions(-)\n> create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> \n> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> index e19cf9828e68..7dc2a2e8f598 100644\n> --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> @@ -30,8 +30,6 @@ select:\n> - marvell,armada-3700-ahci\n> - marvell,armada-8k-ahci\n> - marvell,berlin2q-ahci\n> - - snps,dwc-ahci\n> - - snps,spear-ahci\n> required:\n> - compatible\n> \n> @@ -48,17 +46,11 @@ properties:\n> - marvell,berlin2-ahci\n> - marvell,berlin2q-ahci\n> - const: generic-ahci\n> - - items:\n> - - enum:\n> - - rockchip,rk3568-dwc-ahci\n> - - const: snps,dwc-ahci\n> - enum:\n> - cavium,octeon-7130-ahci\n> - hisilicon,hisi-ahci\n> - ibm,476gtr-ahci\n> - marvell,armada-3700-ahci\n> - - snps,dwc-ahci\n> - - snps,spear-ahci\n> \n> reg:\n> minItems: 1\n> diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> new file mode 100644\n> index 000000000000..af78f6c9b857\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> @@ -0,0 +1,129 @@\n> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> +%YAML 1.2\n> +---\n> +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> +\n> +title: Synopsys DWC AHCI SATA controller\n> +\n> +maintainers:\n> + - Serge Semin \n> +\n> +description:\n> + This document defines device tree bindings for the Synopsys DWC\n> + implementation of the AHCI SATA controller.\n> +\n> +allOf:\n> + - $ref: ahci-common.yaml#\n> +\n> +properties:\n> + compatible:\n> + oneOf:\n> + - description: Synopsys AHCI SATA-compatible devices\n> + contains:\n> + const: snps,dwc-ahci\n> + - description: SPEAr1340 AHCI SATA device\n> + const: snps,spear-ahci\n> + - description: Rockhip RK3568 ahci controller\n> + const: rockchip,rk3568-dwc-ahci\n\nThis is never true because there is a fallback. We should keep what we \nhad before.\n\n\n> +\n> + reg:\n> + maxItems: 1\n> +\n> + interrupts:\n> + maxItems: 1\n> +\n> + clocks:\n> + description:\n> + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock\n> + and embedded PHYs reference clock together with vendor-specific set\n> + of clocks.\n> + minItems: 1\n> + maxItems: 4\n> +\n> + clock-names:\n> + contains:\n> + anyOf:\n> + - description: Application AXI/AHB BIU clock source\n> + enum:\n> + - aclk\n> + - sata\n> + - description: SATA Ports reference clock\n> + enum:\n> + - ref\n> + - sata_ref\n> +\n> + resets:\n> + description:\n> + At least basic core and application clock domains reset is normally\n> + supported by the DWC AHCI SATA controller. Some platform specific\n> + clocks can be also specified though.\n\ns/clocks/resets/ ?\n\nThis allows any number of resets which isn't great. I think this schema \nshould just be the 'simple' cases where there's only 1 reset and 1 \nclock (or how many the DWC block actually has if you have that info). \nMore complicated cases get there own schema.\n\n> +\n> + reset-names:\n> + contains:\n> + description: Core and application clock domains reset control\n> + const: arst\n> +\n> +patternProperties:\n> + \"^sata-port@[0-9a-e]$\":\n> + $ref: '#/$defs/dwc-ahci-port'\n> +\n> + unevaluatedProperties: false\n> +\n> +required:\n> + - compatible\n> + - reg\n> + - interrupts\n> +\n> +unevaluatedProperties: false\n> +\n> +$defs:\n> + dwc-ahci-port:\n> + $ref: /schemas/ata/ahci-common.yaml#/$defs/ahci-port\n> +\n> + properties:\n> + reg:\n> + minimum: 0\n> + maximum: 7\n> +\n> + snps,tx-ts-max:\n> + $ref: /schemas/types.yaml#/definitions/uint32\n> + description: Maximal size of Tx DMA transactions in FIFO words\n> + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> +\n> + snps,rx-ts-max:\n> + $ref: /schemas/types.yaml#/definitions/uint32\n> + description: Maximal size of Rx DMA transactions in FIFO words\n> + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> +\n> +examples:\n> + - |\n> + #include \n> + #include \n> +\n> + sata@122f0000 {\n> + compatible = \"snps,dwc-ahci\";\n> + reg = <0x122F0000 0x1ff>;\n> + #address-cells = <1>;\n> + #size-cells = <0>;\n> +\n> + interrupts = ;\n> +\n> + clocks = <&clock1>, <&clock2>;\n> + clock-names = \"aclk\", \"ref\";\n> +\n> + phys = <&sata_phy>;\n> + phy-names = \"sata-phy\";\n> +\n> + ports-implemented = <0x1>;\n> +\n> + sata-port@0 {\n> + reg = <0>;\n> +\n> + hba-port-cap = ;\n> +\n> + snps,tx-ts-max = <512>;\n> + snps,rx-ts-max = <512>;\n> + };\n> + };\n> +...\n> -- \n> 2.35.1\n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LN37f6Blkz9vDZ\n\tfor ; Wed, 15 Jun 2022 08:31:26 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S229969AbiFNWbU (ORCPT );\n Tue, 14 Jun 2022 18:31:20 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:52498 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S1350908AbiFNW22 (ORCPT\n ); Tue, 14 Jun 2022 18:28:28 -0400","from mail-io1-f50.google.com (mail-io1-f50.google.com\n [209.85.166.50])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E97CF4AE37;\n Tue, 14 Jun 2022 15:27:57 -0700 (PDT)","by mail-io1-f50.google.com with SMTP id h8so10893707iof.11;\n Tue, 14 Jun 2022 15:27:57 -0700 (PDT)","from robh.at.kernel.org ([64.188.179.251])\n by smtp.gmail.com with ESMTPSA id\n s4-20020a056e02020400b002d3da8e4af5sm6212682ilr.23.2022.06.14.15.27.55\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Tue, 14 Jun 2022 15:27:56 -0700 (PDT)","(nullmailer pid 2841909 invoked by uid 1000);\n Tue, 14 Jun 2022 22:27:54 -0000"],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=ijnM6qqps/61JEPhDY+exeWmv8F27GG2ZRzYNTTkDdI=;\n b=YluoaUQFF5biecY8bQJWsyqOTFmtP7gVIV/xDz/hCPB60wg9F4wKIH+Je12VIrsnqr\n AT/bANsbnkX77La+0zsYP2NVs4LQzE6Ew8gNhUYHByI8vG/uaSUDTb+vy3LNPIPRauNr\n tavW1Xhwq2wp6r6cTMwipCt53IAm1HQ2Bpip8QJ4SyLJYRFcHSyUPsnJdU4ydAr6UfML\n BXw5UFfU3HtBDr/HwE3bgd4S23KGPL1SH1ZMhsaVR/mjx5CQl6xZeAYCkNadXz+5xEnn\n ESsH4TgAW2lGSp42ym4KtPIUmm0NDJpnhwx1AaC6+vR82uJ1iM9vZqzCudXk8UBd0POp\n sxwQ==","X-Gm-Message-State":"AOAM532YwRGC45fuTX0xpFeF1TAMiO5j7NIGbev3U13cBsTY1der70Tu\n cpEM6595PuwMAPGpOXZ53Q==","X-Google-Smtp-Source":"\n ABdhPJzbTj6BQ/OtQQ/H0TzS/7EgQ4bX0DXg7IDkhc9vrhCiPRjg/U1m4E4hYebaesNmCw3R6N/RTA==","X-Received":"by 2002:a05:6638:d4d:b0:333:1aea:4b9f with SMTP id\n d13-20020a0566380d4d00b003331aea4b9fmr3988177jak.149.1655245676867;\n Tue, 14 Jun 2022 15:27:56 -0700 (PDT)","Date":"Tue, 14 Jun 2022 16:27:54 -0600","From":"Rob Herring ","To":"Serge Semin ","Cc":"Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Serge Semin ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220614222754.GA2830345-robh@kernel.org>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>","X-Spam-Status":"No, score=-1.2 required=5.0 tests=BAYES_00,\n FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,\n HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,\n SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2915197,"web_url":"http://patchwork.ozlabs.org/comment/2915197/","msgid":"<20220617193744.av27axznbogademt@mobilestation>","list_archive_url":null,"date":"2022-06-17T19:37:44","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > SATA controller except a few peculiarities and the platform environment\n> > requirements. In particular it can have one or two reference clocks to\n> > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > control for the application clock domain. In addition to that the DMA\n> > interface of each port can be tuned up to work with the predefined maximum\n> > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > device DT binding.\n> > \n> > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > to be reused for the vendor-specific DT-schemas (see for example the\n> > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > introduce.\n> > \n> > Signed-off-by: Serge Semin \n> > \n> > ---\n> > \n> > Changelog v2:\n> > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > \n> > Changelog v4:\n> > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > percieved as the node property instead of the key one. (@Rob)\n> > - Use the ahci-port properties definition from the AHCI common schema\n> > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > - Remove the Hannes' rb tag since the patch content has changed.\n> > ---\n> > .../bindings/ata/ahci-platform.yaml | 8 --\n> > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > \n> > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > index e19cf9828e68..7dc2a2e8f598 100644\n> > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > @@ -30,8 +30,6 @@ select:\n> > - marvell,armada-3700-ahci\n> > - marvell,armada-8k-ahci\n> > - marvell,berlin2q-ahci\n> > - - snps,dwc-ahci\n> > - - snps,spear-ahci\n> > required:\n> > - compatible\n> > \n> > @@ -48,17 +46,11 @@ properties:\n> > - marvell,berlin2-ahci\n> > - marvell,berlin2q-ahci\n> > - const: generic-ahci\n> > - - items:\n> > - - enum:\n> > - - rockchip,rk3568-dwc-ahci\n> > - - const: snps,dwc-ahci\n> > - enum:\n> > - cavium,octeon-7130-ahci\n> > - hisilicon,hisi-ahci\n> > - ibm,476gtr-ahci\n> > - marvell,armada-3700-ahci\n> > - - snps,dwc-ahci\n> > - - snps,spear-ahci\n> > \n> > reg:\n> > minItems: 1\n> > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > new file mode 100644\n> > index 000000000000..af78f6c9b857\n> > --- /dev/null\n> > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > @@ -0,0 +1,129 @@\n> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > +%YAML 1.2\n> > +---\n> > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > +\n> > +title: Synopsys DWC AHCI SATA controller\n> > +\n> > +maintainers:\n> > + - Serge Semin \n> > +\n> > +description:\n> > + This document defines device tree bindings for the Synopsys DWC\n> > + implementation of the AHCI SATA controller.\n> > +\n> > +allOf:\n> > + - $ref: ahci-common.yaml#\n> > +\n> > +properties:\n> > + compatible:\n> > + oneOf:\n> > + - description: Synopsys AHCI SATA-compatible devices\n> > + contains:\n> > + const: snps,dwc-ahci\n> > + - description: SPEAr1340 AHCI SATA device\n> > + const: snps,spear-ahci\n> > + - description: Rockhip RK3568 ahci controller\n> > + const: rockchip,rk3568-dwc-ahci\n> \n\n> This is never true because there is a fallback. We should keep what we \n> had before.\n\nCould you be more specific what you meant? I don't see\n\"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\nstring so modification is correct in that case.\n\nMy idea was to have the compatible strings with the required generic\nfallback \"snps,dwc-ahci\" for all new devices thus identifying the\ncontroller IP-core origin. But later you said \"The generic IP block\nfallbacks have proven to be useless.\" I do agree that functionally it\nisn't that often used, but in some cases it can be handy for instance\nto implement quirks in the generic code or use the fallback as an\nadditional info regarding the IP-core origin/version. So if I were you\nI wouldn't be that strict about dropping the generic IP-core fallback\nidentifier. It's much easier to have it specified from the very\nbeginning than adding it after it has been declared as not required.\n\n> \n> \n> > +\n> > + reg:\n> > + maxItems: 1\n> > +\n> > + interrupts:\n> > + maxItems: 1\n> > +\n> > + clocks:\n> > + description:\n> > + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock\n> > + and embedded PHYs reference clock together with vendor-specific set\n> > + of clocks.\n> > + minItems: 1\n> > + maxItems: 4\n> > +\n> > + clock-names:\n> > + contains:\n> > + anyOf:\n> > + - description: Application AXI/AHB BIU clock source\n> > + enum:\n> > + - aclk\n> > + - sata\n> > + - description: SATA Ports reference clock\n> > + enum:\n> > + - ref\n> > + - sata_ref\n> > +\n> > + resets:\n> > + description:\n> > + At least basic core and application clock domains reset is normally\n> > + supported by the DWC AHCI SATA controller. Some platform specific\n> > + clocks can be also specified though.\n> \n\n> s/clocks/resets/ ?\n\nRight, but only in the reference to \"platform specific clocks\" -> \"... resets\".\n\n> \n> This allows any number of resets which isn't great. I think this schema \n> should just be the 'simple' cases where there's only 1 reset and 1 \n> clock (or how many the DWC block actually has if you have that info). \n> More complicated cases get there own schema.\n\nDWC SATA reference manual claims there can be resets implemented to\neach clock domain.\n1) PM-clk <- PM-rst - PM keep-alive clock/reset.\n2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.\n3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to\nnumber of ports <= 8.)\n4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to\nnumber of ports <= 8.)\n5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up\nto number of ports <= 8.)\n\nSo to speak the IP-core can be equipped with up to 26 clocks and\nresets. Should we be more strict we would have needed to define the\nproperties with all the names above and permit up to 26 clocks/resets\nitems. (Do you want it to be done?). In our case for instance there\nis \"aclk\" and a single common \"ref\" clock for all 3, 4 and 5 domain\n(clock 1 is missing).\n\n-Sergey\n\n> \n> > +\n> > + reset-names:\n> > + contains:\n> > + description: Core and application clock domains reset control\n> > + const: arst\n> > +\n> > +patternProperties:\n> > + \"^sata-port@[0-9a-e]$\":\n> > + $ref: '#/$defs/dwc-ahci-port'\n> > +\n> > + unevaluatedProperties: false\n> > +\n> > +required:\n> > + - compatible\n> > + - reg\n> > + - interrupts\n> > +\n> > +unevaluatedProperties: false\n> > +\n> > +$defs:\n> > + dwc-ahci-port:\n> > + $ref: /schemas/ata/ahci-common.yaml#/$defs/ahci-port\n> > +\n> > + properties:\n> > + reg:\n> > + minimum: 0\n> > + maximum: 7\n> > +\n> > + snps,tx-ts-max:\n> > + $ref: /schemas/types.yaml#/definitions/uint32\n> > + description: Maximal size of Tx DMA transactions in FIFO words\n> > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> > +\n> > + snps,rx-ts-max:\n> > + $ref: /schemas/types.yaml#/definitions/uint32\n> > + description: Maximal size of Rx DMA transactions in FIFO words\n> > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> > +\n> > +examples:\n> > + - |\n> > + #include \n> > + #include \n> > +\n> > + sata@122f0000 {\n> > + compatible = \"snps,dwc-ahci\";\n> > + reg = <0x122F0000 0x1ff>;\n> > + #address-cells = <1>;\n> > + #size-cells = <0>;\n> > +\n> > + interrupts = ;\n> > +\n> > + clocks = <&clock1>, <&clock2>;\n> > + clock-names = \"aclk\", \"ref\";\n> > +\n> > + phys = <&sata_phy>;\n> > + phy-names = \"sata-phy\";\n> > +\n> > + ports-implemented = <0x1>;\n> > +\n> > + sata-port@0 {\n> > + reg = <0>;\n> > +\n> > + hba-port-cap = ;\n> > +\n> > + snps,tx-ts-max = <512>;\n> > + snps,rx-ts-max = <512>;\n> > + };\n> > + };\n> > +...\n> > -- \n> > 2.35.1\n> > \n> >","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=lRHxRuKy;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LPq815qDYz9sFx\n\tfor ; Sat, 18 Jun 2022 05:37:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S230011AbiFQThv (ORCPT );\n Fri, 17 Jun 2022 15:37:51 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:50198 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S232923AbiFQThv (ORCPT\n ); Fri, 17 Jun 2022 15:37:51 -0400","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n [IPv6:2a00:1450:4864:20::12e])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D621A1A396;\n Fri, 17 Jun 2022 12:37:48 -0700 (PDT)","by mail-lf1-x12e.google.com with SMTP id s6so8323510lfo.13;\n Fri, 17 Jun 2022 12:37:48 -0700 (PDT)","from mobilestation ([95.79.189.214])\n by smtp.gmail.com with ESMTPSA id\n t20-20020a195f14000000b00478e9b136f6sm736345lfb.221.2022.06.17.12.37.45\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Fri, 17 Jun 2022 12:37:46 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=x6g2E/RC2MEAPi9nkuXxVBMo/zSoleb8Sr+XS8bc7nY=;\n b=lRHxRuKyvB3h2Dydu/M1IrvAnR65RPyOWTev2+f/IskR1FFJvZ8tJ9zQfchrwc4aev\n Y78mVSfKnUI6/KEURRqL+8SNswpqKnIriBKMFd1nrfjdxYrnGZ9nSA+ZAItmUZPku1yi\n sFUtQ5xblDZwh/OpVScdAtdrzH1ESEUitQyqJcUrMeT1S3VtAwH2O9WcgjKIJvmpe4X7\n 3/+pIwBKPJ1ZNGqRCQa8IOrJdAD9Tb3leHdgfb/uOxCR4ftmo8IfIY1MBXOuMk7OvTUg\n oIrGi6DUv0CVINp9NFcAvS5Av7uhJPr9eDZBFw5S2GPyqe7XiCBcKduGcGrFPBMNWFit\n +g/Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=x6g2E/RC2MEAPi9nkuXxVBMo/zSoleb8Sr+XS8bc7nY=;\n b=1RD9lBnmO43/4a/cbnHdGPhDElcl5vh6Sj0fgPnacd6S7dDZOVHxZ3/I29F/XVS4Ud\n 4uUq6Sy/62OHWJCyrjOxvn93O/BKv/D9mgrKqUTwZoLmYHSzzDdhHKlXhhW3UvZkUEeK\n g/WNR3JYE3Idmb2oRFyXkOC/A8m6wHs0TjnagzBK3mbOh7QRZZ3HRsDgec4M1q3kRML3\n wDB/20WHtUe2sQSQkUrkewDOAxYfl3WvAXk9STXIXW+BZEZMBCu+mi+aZaWmlvAghxU1\n Hwkbkno//kCTonTYras55s86WIARMw9/lGmDB4Ua72hyHQ5264jjVjGNniv76VVr93f6\n px2Q==","X-Gm-Message-State":"AJIora/v+Ze5yuNl6XyfemA8rZsQ7Dl0M3M5KHmRDUjqErmVW4M0FerO\n lXSwApVPP882x9gzEljj3ZQ=","X-Google-Smtp-Source":"\n AGRyM1svq4kcgguvPttsqYx1cM3J6lm3ymbjR8zCKQl8Ad0btvpQoEj3i3drD8OxsUPkhDgWDU8+xA==","X-Received":"by 2002:ac2:5c1b:0:b0:478:fc5d:9ddf with SMTP id\n r27-20020ac25c1b000000b00478fc5d9ddfmr6427860lfp.635.1655494666992;\n Fri, 17 Jun 2022 12:37:46 -0700 (PDT)","Date":"Fri, 17 Jun 2022 22:37:44 +0300","From":"Serge Semin ","To":"Rob Herring ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220617193744.av27axznbogademt@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220614222754.GA2830345-robh@kernel.org>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2921440,"web_url":"http://patchwork.ozlabs.org/comment/2921440/","msgid":"<20220628121051.lvmoyz4gmexijoeo@mobilestation>","list_archive_url":null,"date":"2022-06-28T12:10:51","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"Rob,\nI am very much waiting for your response to the notes below.\n\n-Sergey\n\nOn Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:\n> On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > > SATA controller except a few peculiarities and the platform environment\n> > > requirements. In particular it can have one or two reference clocks to\n> > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > > control for the application clock domain. In addition to that the DMA\n> > > interface of each port can be tuned up to work with the predefined maximum\n> > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > > device DT binding.\n> > > \n> > > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > > to be reused for the vendor-specific DT-schemas (see for example the\n> > > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > > introduce.\n> > > \n> > > Signed-off-by: Serge Semin \n> > > \n> > > ---\n> > > \n> > > Changelog v2:\n> > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > > \n> > > Changelog v4:\n> > > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > > percieved as the node property instead of the key one. (@Rob)\n> > > - Use the ahci-port properties definition from the AHCI common schema\n> > > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > > - Remove the Hannes' rb tag since the patch content has changed.\n> > > ---\n> > > .../bindings/ata/ahci-platform.yaml | 8 --\n> > > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > \n> > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > index e19cf9828e68..7dc2a2e8f598 100644\n> > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > @@ -30,8 +30,6 @@ select:\n> > > - marvell,armada-3700-ahci\n> > > - marvell,armada-8k-ahci\n> > > - marvell,berlin2q-ahci\n> > > - - snps,dwc-ahci\n> > > - - snps,spear-ahci\n> > > required:\n> > > - compatible\n> > > \n> > > @@ -48,17 +46,11 @@ properties:\n> > > - marvell,berlin2-ahci\n> > > - marvell,berlin2q-ahci\n> > > - const: generic-ahci\n> > > - - items:\n> > > - - enum:\n> > > - - rockchip,rk3568-dwc-ahci\n> > > - - const: snps,dwc-ahci\n> > > - enum:\n> > > - cavium,octeon-7130-ahci\n> > > - hisilicon,hisi-ahci\n> > > - ibm,476gtr-ahci\n> > > - marvell,armada-3700-ahci\n> > > - - snps,dwc-ahci\n> > > - - snps,spear-ahci\n> > > \n> > > reg:\n> > > minItems: 1\n> > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > new file mode 100644\n> > > index 000000000000..af78f6c9b857\n> > > --- /dev/null\n> > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > @@ -0,0 +1,129 @@\n> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > > +%YAML 1.2\n> > > +---\n> > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > > +\n> > > +title: Synopsys DWC AHCI SATA controller\n> > > +\n> > > +maintainers:\n> > > + - Serge Semin \n> > > +\n> > > +description:\n> > > + This document defines device tree bindings for the Synopsys DWC\n> > > + implementation of the AHCI SATA controller.\n> > > +\n> > > +allOf:\n> > > + - $ref: ahci-common.yaml#\n> > > +\n> > > +properties:\n> > > + compatible:\n> > > + oneOf:\n> > > + - description: Synopsys AHCI SATA-compatible devices\n> > > + contains:\n> > > + const: snps,dwc-ahci\n> > > + - description: SPEAr1340 AHCI SATA device\n> > > + const: snps,spear-ahci\n> > > + - description: Rockhip RK3568 ahci controller\n> > > + const: rockchip,rk3568-dwc-ahci\n> > \n> \n> > This is never true because there is a fallback. We should keep what we \n> > had before.\n> \n> Could you be more specific what you meant? I don't see\n> \"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\n> string so modification is correct in that case.\n> \n> My idea was to have the compatible strings with the required generic\n> fallback \"snps,dwc-ahci\" for all new devices thus identifying the\n> controller IP-core origin. But later you said \"The generic IP block\n> fallbacks have proven to be useless.\" I do agree that functionally it\n> isn't that often used, but in some cases it can be handy for instance\n> to implement quirks in the generic code or use the fallback as an\n> additional info regarding the IP-core origin/version. So if I were you\n> I wouldn't be that strict about dropping the generic IP-core fallback\n> identifier. It's much easier to have it specified from the very\n> beginning than adding it after it has been declared as not required.\n> \n> > \n> > \n> > > +\n> > > + reg:\n> > > + maxItems: 1\n> > > +\n> > > + interrupts:\n> > > + maxItems: 1\n> > > +\n> > > + clocks:\n> > > + description:\n> > > + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock\n> > > + and embedded PHYs reference clock together with vendor-specific set\n> > > + of clocks.\n> > > + minItems: 1\n> > > + maxItems: 4\n> > > +\n> > > + clock-names:\n> > > + contains:\n> > > + anyOf:\n> > > + - description: Application AXI/AHB BIU clock source\n> > > + enum:\n> > > + - aclk\n> > > + - sata\n> > > + - description: SATA Ports reference clock\n> > > + enum:\n> > > + - ref\n> > > + - sata_ref\n> > > +\n> > > + resets:\n> > > + description:\n> > > + At least basic core and application clock domains reset is normally\n> > > + supported by the DWC AHCI SATA controller. Some platform specific\n> > > + clocks can be also specified though.\n> > \n> \n> > s/clocks/resets/ ?\n> \n> Right, but only in the reference to \"platform specific clocks\" -> \"... resets\".\n> \n> > \n> > This allows any number of resets which isn't great. I think this schema \n> > should just be the 'simple' cases where there's only 1 reset and 1 \n> > clock (or how many the DWC block actually has if you have that info). \n> > More complicated cases get there own schema.\n> \n> DWC SATA reference manual claims there can be resets implemented to\n> each clock domain.\n> 1) PM-clk <- PM-rst - PM keep-alive clock/reset.\n> 2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.\n> 3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to\n> number of ports <= 8.)\n> 4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to\n> number of ports <= 8.)\n> 5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up\n> to number of ports <= 8.)\n> \n> So to speak the IP-core can be equipped with up to 26 clocks and\n> resets. Should we be more strict we would have needed to define the\n> properties with all the names above and permit up to 26 clocks/resets\n> items. (Do you want it to be done?). In our case for instance there\n> is \"aclk\" and a single common \"ref\" clock for all 3, 4 and 5 domain\n> (clock 1 is missing).\n> \n> -Sergey\n> \n> > \n> > > +\n> > > + reset-names:\n> > > + contains:\n> > > + description: Core and application clock domains reset control\n> > > + const: arst\n> > > +\n> > > +patternProperties:\n> > > + \"^sata-port@[0-9a-e]$\":\n> > > + $ref: '#/$defs/dwc-ahci-port'\n> > > +\n> > > + unevaluatedProperties: false\n> > > +\n> > > +required:\n> > > + - compatible\n> > > + - reg\n> > > + - interrupts\n> > > +\n> > > +unevaluatedProperties: false\n> > > +\n> > > +$defs:\n> > > + dwc-ahci-port:\n> > > + $ref: /schemas/ata/ahci-common.yaml#/$defs/ahci-port\n> > > +\n> > > + properties:\n> > > + reg:\n> > > + minimum: 0\n> > > + maximum: 7\n> > > +\n> > > + snps,tx-ts-max:\n> > > + $ref: /schemas/types.yaml#/definitions/uint32\n> > > + description: Maximal size of Tx DMA transactions in FIFO words\n> > > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> > > +\n> > > + snps,rx-ts-max:\n> > > + $ref: /schemas/types.yaml#/definitions/uint32\n> > > + description: Maximal size of Rx DMA transactions in FIFO words\n> > > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024 ]\n> > > +\n> > > +examples:\n> > > + - |\n> > > + #include \n> > > + #include \n> > > +\n> > > + sata@122f0000 {\n> > > + compatible = \"snps,dwc-ahci\";\n> > > + reg = <0x122F0000 0x1ff>;\n> > > + #address-cells = <1>;\n> > > + #size-cells = <0>;\n> > > +\n> > > + interrupts = ;\n> > > +\n> > > + clocks = <&clock1>, <&clock2>;\n> > > + clock-names = \"aclk\", \"ref\";\n> > > +\n> > > + phys = <&sata_phy>;\n> > > + phy-names = \"sata-phy\";\n> > > +\n> > > + ports-implemented = <0x1>;\n> > > +\n> > > + sata-port@0 {\n> > > + reg = <0>;\n> > > +\n> > > + hba-port-cap = ;\n> > > +\n> > > + snps,tx-ts-max = <512>;\n> > > + snps,rx-ts-max = <512>;\n> > > + };\n> > > + };\n> > > +...\n> > > -- \n> > > 2.35.1\n> > > \n> > >","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=Xd/dySpM;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LXNjJ119bz9sGC\n\tfor ; Tue, 28 Jun 2022 22:11:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S1345246AbiF1MK7 (ORCPT );\n Tue, 28 Jun 2022 08:10:59 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:41224 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S1345243AbiF1MK6 (ORCPT\n ); Tue, 28 Jun 2022 08:10:58 -0400","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n [IPv6:2a00:1450:4864:20::136])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7A40234671;\n Tue, 28 Jun 2022 05:10:55 -0700 (PDT)","by mail-lf1-x136.google.com with SMTP id g4so21877558lfv.9;\n Tue, 28 Jun 2022 05:10:55 -0700 (PDT)","from mobilestation ([95.79.140.178])\n by smtp.gmail.com with ESMTPSA id\n d10-20020a0565123d0a00b0048110fd06c4sm1226744lfv.53.2022.06.28.05.10.52\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Tue, 28 Jun 2022 05:10:53 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=iUThF36MhWxuNcZJwCw3U7E8s2+/JCGpjeHLpyuRyW8=;\n b=Xd/dySpMVit9WWKQWCdfdQIQXOA/CZOlRW3pa/3Td+dvAI6XZO8/e/vDrgVvDdEAyz\n i8jLqoQFQBuV/nmWAB6ra9Xsh3RZk7zNXYAHSliuxVvJif9otblt23E3IQT7vsB/mgnR\n +Y5bJw4vg/roMe9Nsmf8EV/W8pppRVE4ey3nZ7aw9nFrZnOLuWdyPkB+8ZqZUJeoT84H\n Zo5pVNPjA8GOBBf6/2sI3TV7KsAseYFvVlA4GQE0bHGhF3dM5TXJBRTdgbNfEoClZCkw\n 20OPRmRk3reRoptd17LRg4Fn9pt1akUxcs2o5Jr1ct8Bd4c93RtK3fMwOFRIDmnBP708\n IhWA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=iUThF36MhWxuNcZJwCw3U7E8s2+/JCGpjeHLpyuRyW8=;\n b=gA+Q7EKTPAPhucFTETgBAAJdCJtmdWlhD9Ml64PohoptwzKven2y0Hrx8GWDkGCxpD\n wu5TQKumjQj0z+mL0p/XvvgE4AFnggfR+aK+PE2roTHaQk1INANYxse3r2sj+5Y19LY+\n YUltVQzfTRJU1ldQxMJpql0acAANUuNHEL8FiP90DLyLQoUGfmbTwTl27VRQRt/bHSWW\n 71xWX4wnqXB/SXA0JORTC4OkKkIlKkeEN9C3RWdPnccsoi5qvgiYeFa2Y5wj3rR6z9la\n 8VDF0ArzjQQFH5blas4DE3apN7doelw2FTjXLqy58AbVrk2M/jq0HokSIueXlOYvA7t2\n DypQ==","X-Gm-Message-State":"AJIora9WOV9I/KjBL6NapZJrfUxIxfbXjdqhR6UzDfNKJNBhbffCYXnd\n pQzI1EzgRLKwHHRsT9Cn6xg=","X-Google-Smtp-Source":"\n AGRyM1vmsdX5x0hVJqJrjzWJ6sMBsDWCRMjzatlF1fSHMyHidNtTWBGtv3RwPzCuJ+XsR+/Q4Kzm7w==","X-Received":"by 2002:a05:6512:6d5:b0:480:fd16:42c5 with SMTP id\n u21-20020a05651206d500b00480fd1642c5mr10854410lff.183.1656418253793;\n Tue, 28 Jun 2022 05:10:53 -0700 (PDT)","Date":"Tue, 28 Jun 2022 15:10:51 +0300","From":"Serge Semin ","To":"Rob Herring ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220628121051.lvmoyz4gmexijoeo@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>\n <20220617193744.av27axznbogademt@mobilestation>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220617193744.av27axznbogademt@mobilestation>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2927850,"web_url":"http://patchwork.ozlabs.org/comment/2927850/","msgid":"<20220706223642.GC572635-robh@kernel.org>","list_archive_url":null,"date":"2022-07-06T22:36:42","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":62529,"url":"http://patchwork.ozlabs.org/api/people/62529/","name":"Rob Herring (Arm)","email":"robh@kernel.org"},"content":"On Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:\n> On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > > SATA controller except a few peculiarities and the platform environment\n> > > requirements. In particular it can have one or two reference clocks to\n> > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > > control for the application clock domain. In addition to that the DMA\n> > > interface of each port can be tuned up to work with the predefined maximum\n> > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > > device DT binding.\n> > > \n> > > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > > to be reused for the vendor-specific DT-schemas (see for example the\n> > > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > > introduce.\n> > > \n> > > Signed-off-by: Serge Semin \n> > > \n> > > ---\n> > > \n> > > Changelog v2:\n> > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > > \n> > > Changelog v4:\n> > > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > > percieved as the node property instead of the key one. (@Rob)\n> > > - Use the ahci-port properties definition from the AHCI common schema\n> > > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > > - Remove the Hannes' rb tag since the patch content has changed.\n> > > ---\n> > > .../bindings/ata/ahci-platform.yaml | 8 --\n> > > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > \n> > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > index e19cf9828e68..7dc2a2e8f598 100644\n> > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > @@ -30,8 +30,6 @@ select:\n> > > - marvell,armada-3700-ahci\n> > > - marvell,armada-8k-ahci\n> > > - marvell,berlin2q-ahci\n> > > - - snps,dwc-ahci\n> > > - - snps,spear-ahci\n> > > required:\n> > > - compatible\n> > > \n> > > @@ -48,17 +46,11 @@ properties:\n> > > - marvell,berlin2-ahci\n> > > - marvell,berlin2q-ahci\n> > > - const: generic-ahci\n> > > - - items:\n> > > - - enum:\n> > > - - rockchip,rk3568-dwc-ahci\n> > > - - const: snps,dwc-ahci\n> > > - enum:\n> > > - cavium,octeon-7130-ahci\n> > > - hisilicon,hisi-ahci\n> > > - ibm,476gtr-ahci\n> > > - marvell,armada-3700-ahci\n> > > - - snps,dwc-ahci\n> > > - - snps,spear-ahci\n> > > \n> > > reg:\n> > > minItems: 1\n> > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > new file mode 100644\n> > > index 000000000000..af78f6c9b857\n> > > --- /dev/null\n> > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > @@ -0,0 +1,129 @@\n> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > > +%YAML 1.2\n> > > +---\n> > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > > +\n> > > +title: Synopsys DWC AHCI SATA controller\n> > > +\n> > > +maintainers:\n> > > + - Serge Semin \n> > > +\n> > > +description:\n> > > + This document defines device tree bindings for the Synopsys DWC\n> > > + implementation of the AHCI SATA controller.\n> > > +\n> > > +allOf:\n> > > + - $ref: ahci-common.yaml#\n> > > +\n> > > +properties:\n> > > + compatible:\n> > > + oneOf:\n> > > + - description: Synopsys AHCI SATA-compatible devices\n> > > + contains:\n> > > + const: snps,dwc-ahci\n> > > + - description: SPEAr1340 AHCI SATA device\n> > > + const: snps,spear-ahci\n> > > + - description: Rockhip RK3568 ahci controller\n> > > + const: rockchip,rk3568-dwc-ahci\n> > \n> \n> > This is never true because there is a fallback. We should keep what we \n> > had before.\n> \n> Could you be more specific what you meant? I don't see\n> \"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\n> string so modification is correct in that case.\n\nSpear does not, just rockchip:\n\narch/arm64/boot/dts/rockchip/rk3568.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\narch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\narch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n\nSo the 3rd entry is never true.\n\n> My idea was to have the compatible strings with the required generic\n> fallback \"snps,dwc-ahci\" for all new devices thus identifying the\n> controller IP-core origin. But later you said \"The generic IP block\n> fallbacks have proven to be useless.\" I do agree that functionally it\n> isn't that often used, but in some cases it can be handy for instance\n> to implement quirks in the generic code or use the fallback as an\n> additional info regarding the IP-core origin/version. So if I were you\n> I wouldn't be that strict about dropping the generic IP-core fallback\n> identifier. It's much easier to have it specified from the very\n> beginning than adding it after it has been declared as not required.\n\nI wish they were useful, but experience has shown they are not.\n\n> > > + reg:\n> > > + maxItems: 1\n> > > +\n> > > + interrupts:\n> > > + maxItems: 1\n> > > +\n> > > + clocks:\n> > > + description:\n> > > + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock\n> > > + and embedded PHYs reference clock together with vendor-specific set\n> > > + of clocks.\n> > > + minItems: 1\n> > > + maxItems: 4\n> > > +\n> > > + clock-names:\n> > > + contains:\n> > > + anyOf:\n> > > + - description: Application AXI/AHB BIU clock source\n> > > + enum:\n> > > + - aclk\n> > > + - sata\n> > > + - description: SATA Ports reference clock\n> > > + enum:\n> > > + - ref\n> > > + - sata_ref\n> > > +\n> > > + resets:\n> > > + description:\n> > > + At least basic core and application clock domains reset is normally\n> > > + supported by the DWC AHCI SATA controller. Some platform specific\n> > > + clocks can be also specified though.\n> > \n> \n> > s/clocks/resets/ ?\n> \n> Right, but only in the reference to \"platform specific clocks\" -> \"... resets\".\n> \n> > \n> > This allows any number of resets which isn't great. I think this schema \n> > should just be the 'simple' cases where there's only 1 reset and 1 \n> > clock (or how many the DWC block actually has if you have that info). \n> > More complicated cases get there own schema.\n> \n> DWC SATA reference manual claims there can be resets implemented to\n> each clock domain.\n> 1) PM-clk <- PM-rst - PM keep-alive clock/reset.\n> 2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.\n> 3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to\n> number of ports <= 8.)\n> 4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to\n> number of ports <= 8.)\n> 5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up\n> to number of ports <= 8.)\n> \n> So to speak the IP-core can be equipped with up to 26 clocks and\n> resets. Should we be more strict we would have needed to define the\n> properties with all the names above and permit up to 26 clocks/resets\n> items. (Do you want it to be done?). In our case for instance there\n> is \"aclk\" and a single common \"ref\" clock for all 3, 4 and 5 domain\n> (clock 1 is missing).\n\nI imagine most implementations just tie most clocks together.\n\nI guess there's not going to be much new SATA h/w, so perhaps fine \nas-is.\n\nRob","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LdZm147tnz9s09\n\tfor ; Thu, 7 Jul 2022 09:01:21 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S233610AbiGFXBT (ORCPT );\n Wed, 6 Jul 2022 19:01:19 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:51586 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S229768AbiGFXBS (ORCPT\n ); Wed, 6 Jul 2022 19:01:18 -0400","from mail-oa1-f42.google.com (mail-oa1-f42.google.com\n [209.85.160.42])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AE5611C12F;\n Wed, 6 Jul 2022 16:01:17 -0700 (PDT)","by mail-oa1-f42.google.com with SMTP id\n 586e51a60fabf-1013ecaf7e0so23158082fac.13;\n Wed, 06 Jul 2022 16:01:17 -0700 (PDT)","from robh.at.kernel.org ([2607:fb90:5fe0:199f:283a:d3be:fc5c:70cd])\n by smtp.gmail.com with ESMTPSA id\n i21-20020a056808031500b00338869042fdsm468712oie.32.2022.07.06.16.01.14\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Wed, 06 Jul 2022 16:01:16 -0700 (PDT)","(nullmailer pid 660709 invoked by uid 1000);\n Wed, 06 Jul 2022 22:36:42 -0000"],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=ftg3nkiw4YMCBhVm4GjYQbi4EAw65M6tPEzJj7RvX3Q=;\n b=GanXydllfwsuwWs2wqfVjm1V7yclrBnEVLu3vo8lOT8FedPXQhHswVE78Z/fXP06yl\n U8v2UYPfheWAyFC09z/Y7ph8N7UyD5DRH+Svdyeq8Df7gUOybyyThlljLexVdYnXQL9l\n oHZTV3uTcPrKTAZfZFhDM2QWBi0QH+TbBkXvmN/K7eKFe43eAKK7rQ+edg5r6vQnIe0b\n J0hsKC/wl6Pdw6AYU9CuCK6vDR7hJZ6zMmIngkAlZOFzizLxHZRtfGZAWuArfQ2y4Jnr\n aVwQ4F+3hpMfbUbmr7dV49SViKz5ln74855YhOYotC5/wK7AaI52vUKjX5S/z4ppwXJN\n GeDA==","X-Gm-Message-State":"AJIora8xF1xrcWsfTQic+rHFcL2SmGcfUHH/FvLjJgIt2mM/TDF11gzv\n 6gHzO049NZ/uskW26inoDw==","X-Google-Smtp-Source":"\n AGRyM1v1MWfpmm6E5fV9mEVufA395d+r9AZ9GUjfky5sGrak0Tl8WiMFGE2c/r0+HUr1dAdE29X2dw==","X-Received":"by 2002:a05:6871:1d0:b0:101:59af:c6b7 with SMTP id\n q16-20020a05687101d000b0010159afc6b7mr679263oad.89.1657148476851;\n Wed, 06 Jul 2022 16:01:16 -0700 (PDT)","Date":"Wed, 6 Jul 2022 16:36:42 -0600","From":"Rob Herring ","To":"Serge Semin ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220706223642.GC572635-robh@kernel.org>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>\n <20220617193744.av27axznbogademt@mobilestation>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220617193744.av27axznbogademt@mobilestation>","X-Spam-Status":"No, score=-1.2 required=5.0 tests=BAYES_00,\n FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,\n HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,\n RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=no autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2928440,"web_url":"http://patchwork.ozlabs.org/comment/2928440/","msgid":"<20220707152539.tktdo4qnvwormkqk@mobilestation>","list_archive_url":null,"date":"2022-07-07T15:25:39","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Wed, Jul 06, 2022 at 04:36:42PM -0600, Rob Herring wrote:\n> On Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:\n> > On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> > > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > > > SATA controller except a few peculiarities and the platform environment\n> > > > requirements. In particular it can have one or two reference clocks to\n> > > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > > > control for the application clock domain. In addition to that the DMA\n> > > > interface of each port can be tuned up to work with the predefined maximum\n> > > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > > > device DT binding.\n> > > > \n> > > > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > > > to be reused for the vendor-specific DT-schemas (see for example the\n> > > > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > > > introduce.\n> > > > \n> > > > Signed-off-by: Serge Semin \n> > > > \n> > > > ---\n> > > > \n> > > > Changelog v2:\n> > > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > > > \n> > > > Changelog v4:\n> > > > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > > > percieved as the node property instead of the key one. (@Rob)\n> > > > - Use the ahci-port properties definition from the AHCI common schema\n> > > > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > > > - Remove the Hannes' rb tag since the patch content has changed.\n> > > > ---\n> > > > .../bindings/ata/ahci-platform.yaml | 8 --\n> > > > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > > > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > \n> > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > index e19cf9828e68..7dc2a2e8f598 100644\n> > > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > @@ -30,8 +30,6 @@ select:\n> > > > - marvell,armada-3700-ahci\n> > > > - marvell,armada-8k-ahci\n> > > > - marvell,berlin2q-ahci\n> > > > - - snps,dwc-ahci\n> > > > - - snps,spear-ahci\n> > > > required:\n> > > > - compatible\n> > > > \n> > > > @@ -48,17 +46,11 @@ properties:\n> > > > - marvell,berlin2-ahci\n> > > > - marvell,berlin2q-ahci\n> > > > - const: generic-ahci\n> > > > - - items:\n> > > > - - enum:\n> > > > - - rockchip,rk3568-dwc-ahci\n> > > > - - const: snps,dwc-ahci\n> > > > - enum:\n> > > > - cavium,octeon-7130-ahci\n> > > > - hisilicon,hisi-ahci\n> > > > - ibm,476gtr-ahci\n> > > > - marvell,armada-3700-ahci\n> > > > - - snps,dwc-ahci\n> > > > - - snps,spear-ahci\n> > > > \n> > > > reg:\n> > > > minItems: 1\n> > > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > new file mode 100644\n> > > > index 000000000000..af78f6c9b857\n> > > > --- /dev/null\n> > > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > @@ -0,0 +1,129 @@\n> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > > > +%YAML 1.2\n> > > > +---\n> > > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > > > +\n> > > > +title: Synopsys DWC AHCI SATA controller\n> > > > +\n> > > > +maintainers:\n> > > > + - Serge Semin \n> > > > +\n> > > > +description:\n> > > > + This document defines device tree bindings for the Synopsys DWC\n> > > > + implementation of the AHCI SATA controller.\n> > > > +\n> > > > +allOf:\n> > > > + - $ref: ahci-common.yaml#\n> > > > +\n> > > > +properties:\n> > > > + compatible:\n> > > > + oneOf:\n> > > > + - description: Synopsys AHCI SATA-compatible devices\n> > > > + contains:\n> > > > + const: snps,dwc-ahci\n> > > > + - description: SPEAr1340 AHCI SATA device\n> > > > + const: snps,spear-ahci\n> > > > + - description: Rockhip RK3568 ahci controller\n> > > > + const: rockchip,rk3568-dwc-ahci\n> > > \n> > \n> > > This is never true because there is a fallback. We should keep what we \n> > > had before.\n> > \n> > Could you be more specific what you meant? I don't see\n> > \"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\n> > string so modification is correct in that case.\n> \n\n> Spear does not, just rockchip:\n> \n> arch/arm64/boot/dts/rockchip/rk3568.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> \n> So the 3rd entry is never true.\n\nThen I'll have to split the schema up into two bindings:\n1. snps,dwc-ahci-common.yaml: generic DW SATA AHCI properties and no \"compatible\"\nproperty constraint since you said fallback was useless.\n2. snps,dwc-ahci.yaml: generic DW SATA AHCI DT-schema with\ncompetibles: (\"snps,dwc-ahci\"), (\"snps,spear-ahci\"),\n(\"rockchip,rk3568-dwc-ahci\",\"snps,dwc-ahci\").\n\nAre you ok with this?\n\nBTW if we had the fallback required the splitting up couldn't have\nbeen needed.\n\n> \n> > My idea was to have the compatible strings with the required generic\n> > fallback \"snps,dwc-ahci\" for all new devices thus identifying the\n> > controller IP-core origin. But later you said \"The generic IP block\n> > fallbacks have proven to be useless.\" I do agree that functionally it\n> > isn't that often used, but in some cases it can be handy for instance\n> > to implement quirks in the generic code or use the fallback as an\n> > additional info regarding the IP-core origin/version. So if I were you\n> > I wouldn't be that strict about dropping the generic IP-core fallback\n> > identifier. It's much easier to have it specified from the very\n> > beginning than adding it after it has been declared as not required.\n> \n> I wish they were useful, but experience has shown they are not.\n\nSo what to do with the generic fallback compatibles then? Please\nanswer to the next questions so I would correct all my currently\nstashed patches in accordance with it.\n\n1) Do you want all the new DT-binding schemas refusing to have the\nfallback compatibles except for the nodes which bindings have already\nbeen defined that way?\n\n2) What if a device IP-core has some versioning, but it's either\nnot auto-detectable at runtime or can be auto-detected but starting\nfrom some IP-core version? Do we need it being specified in addition\nto the vendor-specific compatible string?\n\n3) The same as 2), but shall it have a generic version-less fallback\ncompatible string too?\n\n4) The same as 2), but what if it concerns a device which driver\nrelies on the versioning?\n\n5) The same as 2), but what if it concerns the device which currently\ndoesn't have a driver relying on the IP-core version?\n\n6) What if we don't have the generic fallback compatible string\nrequired, but at some point a kernel would need it to\nimplement a version/IP-core-specific quirk? If we had the generic\nfallback specified in dts the older systems would have been supported\nout-of-box, otherwise the firmware update would also needed.\n\nIMO having the IP-core version + generic compatibles give many\nbenefits and it's much easier to have them required from the very\nbeginning instead of adding afterwards when then a need arises.\n\n-Sergey\n\n> \n> > > > + reg:\n> > > > + maxItems: 1\n> > > > +\n> > > > + interrupts:\n> > > > + maxItems: 1\n> > > > +\n> > > > + clocks:\n> > > > + description:\n> > > > + Basic DWC AHCI SATA clock sources like application AXI/AHB BIU clock\n> > > > + and embedded PHYs reference clock together with vendor-specific set\n> > > > + of clocks.\n> > > > + minItems: 1\n> > > > + maxItems: 4\n> > > > +\n> > > > + clock-names:\n> > > > + contains:\n> > > > + anyOf:\n> > > > + - description: Application AXI/AHB BIU clock source\n> > > > + enum:\n> > > > + - aclk\n> > > > + - sata\n> > > > + - description: SATA Ports reference clock\n> > > > + enum:\n> > > > + - ref\n> > > > + - sata_ref\n> > > > +\n> > > > + resets:\n> > > > + description:\n> > > > + At least basic core and application clock domains reset is normally\n> > > > + supported by the DWC AHCI SATA controller. Some platform specific\n> > > > + clocks can be also specified though.\n> > > \n> > \n> > > s/clocks/resets/ ?\n> > \n> > Right, but only in the reference to \"platform specific clocks\" -> \"... resets\".\n> > \n> > > \n> > > This allows any number of resets which isn't great. I think this schema \n> > > should just be the 'simple' cases where there's only 1 reset and 1 \n> > > clock (or how many the DWC block actually has if you have that info). \n> > > More complicated cases get there own schema.\n> > \n> > DWC SATA reference manual claims there can be resets implemented to\n> > each clock domain.\n> > 1) PM-clk <- PM-rst - PM keep-alive clock/reset.\n> > 2) aclk/hclk <- aresetn/hresetn - AXI/AHB clock domain/reset.\n> > 3) rbc*_clk <- rbc*_rst - PHY Receive Clock domain/reset. (Up to\n> > number of ports <= 8.)\n> > 4) asic*_clk <- asic*_rst - PHY Transmit Clock domain/reset. (Up to\n> > number of ports <= 8.)\n> > 5) rxoob*_clk <- rxoob*_rst - RxOOB Detection Clock domain/reset. (Up\n> > to number of ports <= 8.)\n> > \n> > So to speak the IP-core can be equipped with up to 26 clocks and\n> > resets. Should we be more strict we would have needed to define the\n> > properties with all the names above and permit up to 26 clocks/resets\n> > items. (Do you want it to be done?). In our case for instance there\n> > is \"aclk\" and a single common \"ref\" clock for all 3, 4 and 5 domain\n> > (clock 1 is missing).\n> \n\n> I imagine most implementations just tie most clocks together.\n> \n> I guess there's not going to be much new SATA h/w, so perhaps fine \n> as-is.\n\nDo you mean to keep the \"resets\" property with no num-of-items constraints\nthen?\n\n-Sergey\n\n> \n> Rob","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=iLcRUJze;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4Lf0bw4PGjz9s0w\n\tfor ; Fri, 8 Jul 2022 01:25:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S235576AbiGGPZq (ORCPT );\n Thu, 7 Jul 2022 11:25:46 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:48434 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S235122AbiGGPZp (ORCPT\n ); Thu, 7 Jul 2022 11:25:45 -0400","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n [IPv6:2a00:1450:4864:20::12f])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BCF2B2CC95;\n Thu, 7 Jul 2022 08:25:43 -0700 (PDT)","by mail-lf1-x12f.google.com with SMTP id d12so8397327lfq.12;\n Thu, 07 Jul 2022 08:25:43 -0700 (PDT)","from mobilestation ([95.79.140.178])\n by smtp.gmail.com with ESMTPSA id\n o1-20020a05651205c100b0048459950d65sm1352593lfo.306.2022.07.07.08.25.40\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Thu, 07 Jul 2022 08:25:41 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=4z3J663gkFkbp472Bx8Crc/uidLLLx8KpS8uotKO5LA=;\n b=iLcRUJzeI4c2gYOsmAYZ5/Dhcz85wIaP3s5pEOdCBIIVKzFTP/ogqV+Mnh5tiM0JpQ\n qwMvF5G7Fd39dTWmpr91gPPAsMRyJWMec//lNDQ/gvJE3sk0WVZxUBRm1GxNngw4jDcF\n gDXE9smqoqNZSJoPNUxCnkV0PeasTtsm7PVEWpsug/21zRWSIsomNYLKat6NjDJBkoWl\n krEYoeRZrSS4BcgEHGLqnHzwl4m9B0LTsJC4oY7T8RVWoCrbVM0yh1hiFzfxfT79U6Wr\n 0hsgZC4cttaqDDamuPpVI3yWXTtlG91/YrIzBcX1H4PUC7X3/dvcBp7mp60eWfvldEY8\n K6hQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=4z3J663gkFkbp472Bx8Crc/uidLLLx8KpS8uotKO5LA=;\n b=2s3Q9vakp8oD3MSSf29oDkpKZQ/vvTJUIzwvUiCfbuDLAoPsZMaQQQzvtpUAvnXl0F\n Wc4+c7RpRWgDcd8FgVbSUDEsWZ4sotApXoIfbTaEjEAdW2MlC7IOTRfdTNmX4KOhMah2\n 67KiGu6Sm+QOLo2HZEetJROf7L1H4ApXd8GNfa3gb4Dds7yFxwW8AC7yyhrUZu9rEoTK\n B1amnhoJa11ZatoHTED2ThkIGtSGoUMoy/11lm7mWKDzEDzIOZ3PXa8F+kHKmiEK0Y4q\n TEJRS6ESBo7fqsE0yCS9UNk1ERzXPnPhd8bO5cESpju0nDhR0wtMcEmCDS7/1zYOzuee\n 87eQ==","X-Gm-Message-State":"AJIora87z5g5dGCUq5/vb12NN9LFYGkxFRZttPe6LBqgBE9rQRWSmlYQ\n HcVwQ9PaQxbTgEI0rJn7xG4=","X-Google-Smtp-Source":"\n AGRyM1sB79BSnFqpgp1BveiIrl7G8QFiRp/78fOZt4UfoGYkhtPFEwIc/2uZIadvSM/Uv0DLI1omrg==","X-Received":"by 2002:a05:6512:3b83:b0:47f:7419:5e58 with SMTP id\n g3-20020a0565123b8300b0047f74195e58mr29966749lfv.302.1657207541912;\n Thu, 07 Jul 2022 08:25:41 -0700 (PDT)","Date":"Thu, 7 Jul 2022 18:25:39 +0300","From":"Serge Semin ","To":"Rob Herring ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220707152539.tktdo4qnvwormkqk@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>\n <20220617193744.av27axznbogademt@mobilestation>\n <20220706223642.GC572635-robh@kernel.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220706223642.GC572635-robh@kernel.org>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2931387,"web_url":"http://patchwork.ozlabs.org/comment/2931387/","msgid":"<20220712201342.GM1823936-robh@kernel.org>","list_archive_url":null,"date":"2022-07-12T20:13:42","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":62529,"url":"http://patchwork.ozlabs.org/api/people/62529/","name":"Rob Herring (Arm)","email":"robh@kernel.org"},"content":"On Thu, Jul 07, 2022 at 06:25:39PM +0300, Serge Semin wrote:\n> On Wed, Jul 06, 2022 at 04:36:42PM -0600, Rob Herring wrote:\n> > On Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:\n> > > On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> > > > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > > > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > > > > SATA controller except a few peculiarities and the platform environment\n> > > > > requirements. In particular it can have one or two reference clocks to\n> > > > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > > > > control for the application clock domain. In addition to that the DMA\n> > > > > interface of each port can be tuned up to work with the predefined maximum\n> > > > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > > > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > > > > device DT binding.\n> > > > > \n> > > > > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > > > > to be reused for the vendor-specific DT-schemas (see for example the\n> > > > > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > > > > introduce.\n> > > > > \n> > > > > Signed-off-by: Serge Semin \n> > > > > \n> > > > > ---\n> > > > > \n> > > > > Changelog v2:\n> > > > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > > > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > > > > \n> > > > > Changelog v4:\n> > > > > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > > > > percieved as the node property instead of the key one. (@Rob)\n> > > > > - Use the ahci-port properties definition from the AHCI common schema\n> > > > > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > > > > - Remove the Hannes' rb tag since the patch content has changed.\n> > > > > ---\n> > > > > .../bindings/ata/ahci-platform.yaml | 8 --\n> > > > > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > > > > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > > > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > \n> > > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > index e19cf9828e68..7dc2a2e8f598 100644\n> > > > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > @@ -30,8 +30,6 @@ select:\n> > > > > - marvell,armada-3700-ahci\n> > > > > - marvell,armada-8k-ahci\n> > > > > - marvell,berlin2q-ahci\n> > > > > - - snps,dwc-ahci\n> > > > > - - snps,spear-ahci\n> > > > > required:\n> > > > > - compatible\n> > > > > \n> > > > > @@ -48,17 +46,11 @@ properties:\n> > > > > - marvell,berlin2-ahci\n> > > > > - marvell,berlin2q-ahci\n> > > > > - const: generic-ahci\n> > > > > - - items:\n> > > > > - - enum:\n> > > > > - - rockchip,rk3568-dwc-ahci\n> > > > > - - const: snps,dwc-ahci\n> > > > > - enum:\n> > > > > - cavium,octeon-7130-ahci\n> > > > > - hisilicon,hisi-ahci\n> > > > > - ibm,476gtr-ahci\n> > > > > - marvell,armada-3700-ahci\n> > > > > - - snps,dwc-ahci\n> > > > > - - snps,spear-ahci\n> > > > > \n> > > > > reg:\n> > > > > minItems: 1\n> > > > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > new file mode 100644\n> > > > > index 000000000000..af78f6c9b857\n> > > > > --- /dev/null\n> > > > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > @@ -0,0 +1,129 @@\n> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > > > > +%YAML 1.2\n> > > > > +---\n> > > > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > > > > +\n> > > > > +title: Synopsys DWC AHCI SATA controller\n> > > > > +\n> > > > > +maintainers:\n> > > > > + - Serge Semin \n> > > > > +\n> > > > > +description:\n> > > > > + This document defines device tree bindings for the Synopsys DWC\n> > > > > + implementation of the AHCI SATA controller.\n> > > > > +\n> > > > > +allOf:\n> > > > > + - $ref: ahci-common.yaml#\n> > > > > +\n> > > > > +properties:\n> > > > > + compatible:\n> > > > > + oneOf:\n> > > > > + - description: Synopsys AHCI SATA-compatible devices\n> > > > > + contains:\n> > > > > + const: snps,dwc-ahci\n> > > > > + - description: SPEAr1340 AHCI SATA device\n> > > > > + const: snps,spear-ahci\n> > > > > + - description: Rockhip RK3568 ahci controller\n> > > > > + const: rockchip,rk3568-dwc-ahci\n> > > > \n> > > \n> > > > This is never true because there is a fallback. We should keep what we \n> > > > had before.\n> > > \n> > > Could you be more specific what you meant? I don't see\n> > > \"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\n> > > string so modification is correct in that case.\n> > \n> \n> > Spear does not, just rockchip:\n> > \n> > arch/arm64/boot/dts/rockchip/rk3568.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > \n> > So the 3rd entry is never true.\n> \n> Then I'll have to split the schema up into two bindings:\n> 1. snps,dwc-ahci-common.yaml: generic DW SATA AHCI properties and no \"compatible\"\n> property constraint since you said fallback was useless.\n> 2. snps,dwc-ahci.yaml: generic DW SATA AHCI DT-schema with\n> competibles: (\"snps,dwc-ahci\"), (\"snps,spear-ahci\"),\n> (\"rockchip,rk3568-dwc-ahci\",\"snps,dwc-ahci\").\n> \n> Are you ok with this?\n\nYes.\n\n> BTW if we had the fallback required the splitting up couldn't have\n> been needed.\n\nWe generally end up needing a split like this anyways.\n\n\n> > > My idea was to have the compatible strings with the required generic\n> > > fallback \"snps,dwc-ahci\" for all new devices thus identifying the\n> > > controller IP-core origin. But later you said \"The generic IP block\n> > > fallbacks have proven to be useless.\" I do agree that functionally it\n> > > isn't that often used, but in some cases it can be handy for instance\n> > > to implement quirks in the generic code or use the fallback as an\n> > > additional info regarding the IP-core origin/version. So if I were you\n> > > I wouldn't be that strict about dropping the generic IP-core fallback\n> > > identifier. It's much easier to have it specified from the very\n> > > beginning than adding it after it has been declared as not required.\n> > \n> > I wish they were useful, but experience has shown they are not.\n> \n> So what to do with the generic fallback compatibles then? Please\n> answer to the next questions so I would correct all my currently\n> stashed patches in accordance with it.\n> \n> 1) Do you want all the new DT-binding schemas refusing to have the\n> fallback compatibles except for the nodes which bindings have already\n> been defined that way?\n\nYes. I wouldn't go quite as far as 'refusing'. I'm okay with a fallback \nin cases that are simple enough to actually work without platform \nspecific code. As soon as the clocks, resets, phys, etc. aren't \nstandard, that goes out the window. Based on experience, that pretty \nmuch never happens except on the IP vendor's FPGA.\n\n\n> 2) What if a device IP-core has some versioning, but it's either\n> not auto-detectable at runtime or can be auto-detected but starting\n> from some IP-core version? Do we need it being specified in addition\n> to the vendor-specific compatible string?\n\nBy the time you are probing the device, you know the specific SoC and \ncan just set a version variable easily. Why have a string to parse that \ndoesn't work for version comparisons (e.g GT/GE/LT).\n\nAlso, what if you don't know the exact IP version? Maybe you can guess \nthat it is at least at certain version based on knowing the features, so \nyou set that version. Would you really want to put that guess in DT when \nlater on you might need to change it?\n\n> 3) The same as 2), but shall it have a generic version-less fallback\n> compatible string too?\n\nIf the device can function without the version specific compatible.\n\n> 4) The same as 2), but what if it concerns a device which driver\n> relies on the versioning?\n> \n> 5) The same as 2), but what if it concerns the device which currently\n> doesn't have a driver relying on the IP-core version?\n\nAgain, let the driver set the version based on the platform specific \ncompatible.\n\n> 6) What if we don't have the generic fallback compatible string\n> required, but at some point a kernel would need it to\n> implement a version/IP-core-specific quirk? If we had the generic\n> fallback specified in dts the older systems would have been supported\n> out-of-box, otherwise the firmware update would also needed.\n\nAgain, when you start probing the device, you already know the specific \nplatform implementation. From that, you can easily imply the IP vendor \nand version. No DT change needed.\n\n> IMO having the IP-core version + generic compatibles give many\n> benefits and it's much easier to have them required from the very\n> beginning instead of adding afterwards when then a need arises.\n\nCertainly adding afterwards is broken. That's why we insist on SoC \nspecific compatibles. Adding them when we have some platform specific \nquirk doesn't work.\n\nRob","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LjBm21LZCz9s07\n\tfor ; Wed, 13 Jul 2022 06:13:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S230166AbiGLUNx (ORCPT );\n Tue, 12 Jul 2022 16:13:53 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:54242 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S233911AbiGLUNs (ORCPT\n ); Tue, 12 Jul 2022 16:13:48 -0400","from mail-io1-f46.google.com (mail-io1-f46.google.com\n [209.85.166.46])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 71AC3B7EB;\n Tue, 12 Jul 2022 13:13:45 -0700 (PDT)","by mail-io1-f46.google.com with SMTP id n68so8991192iod.3;\n Tue, 12 Jul 2022 13:13:45 -0700 (PDT)","from robh.at.kernel.org ([64.188.179.248])\n by smtp.gmail.com with ESMTPSA id\n w16-20020a92d2d0000000b002dc29c112a7sm4093666ilg.69.2022.07.12.13.13.43\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Tue, 12 Jul 2022 13:13:43 -0700 (PDT)","(nullmailer pid 2288992 invoked by uid 1000);\n Tue, 12 Jul 2022 20:13:42 -0000"],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=FD3iOLQdN91bZcY+dc8yYBURhvU2viDaJpl9SU9IenU=;\n b=Hvqc30j3isKxScnoo69gFhZFQ25/hWn1rLtgcRiNQ5QuOdZScb95qmnKnDdD3gKbX4\n X8tX5yi/hdSfP3LX92FqYFN8OsCmC9bd56sViGd3hDMN487ZGMtfvrCXAmzEgA6STlcS\n mI/YmuG+ZwiPAfPjYdQ/BMr35nMGthjOP6xb7HDM5450/VnOpEnVhHmPcJQhRZo8jYCC\n IREvfflBcyNZ129ZFzXTILINXjFTRUnvgyozrOaKnqiyA6HM6VWIXfGzfSZdSj4g6GcB\n uU6qEOXPT0KHQFbUfxW5G8/B40gtevvZrvn17NDFzGIsCya2veXwt5Zd8QU742hzttWq\n X7mw==","X-Gm-Message-State":"AJIora+AtJ/UwE2fTPUAVhkJT39WCet/sPi44mYVOeaP0yEjAAH6MK9A\n wbP8N0bxEV2FptzMMgN/6A==","X-Google-Smtp-Source":"\n AGRyM1vtymMdnzuF/JqdCbpN9B/QBFZTTbR503UfN7lSeYp249vszXWJ6d/7ED8IkcKRO8O40J2jdA==","X-Received":"by 2002:a05:6638:272c:b0:33f:6fe4:b76f with SMTP id\n m44-20020a056638272c00b0033f6fe4b76fmr2999123jav.284.1657656824314;\n Tue, 12 Jul 2022 13:13:44 -0700 (PDT)","Date":"Tue, 12 Jul 2022 14:13:42 -0600","From":"Rob Herring ","To":"Serge Semin ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220712201342.GM1823936-robh@kernel.org>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>\n <20220617193744.av27axznbogademt@mobilestation>\n <20220706223642.GC572635-robh@kernel.org>\n <20220707152539.tktdo4qnvwormkqk@mobilestation>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220707152539.tktdo4qnvwormkqk@mobilestation>","X-Spam-Status":"No, score=-1.2 required=5.0 tests=BAYES_00,\n FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,\n HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,\n SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2931403,"web_url":"http://patchwork.ozlabs.org/comment/2931403/","msgid":"<20220712204352.5qqk5wbvpe4iihrv@mobilestation>","list_archive_url":null,"date":"2022-07-12T20:43:52","subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Tue, Jul 12, 2022 at 02:13:42PM -0600, Rob Herring wrote:\n> On Thu, Jul 07, 2022 at 06:25:39PM +0300, Serge Semin wrote:\n> > On Wed, Jul 06, 2022 at 04:36:42PM -0600, Rob Herring wrote:\n> > > On Fri, Jun 17, 2022 at 10:37:44PM +0300, Serge Semin wrote:\n> > > > On Tue, Jun 14, 2022 at 04:27:54PM -0600, Rob Herring wrote:\n> > > > > On Fri, Jun 10, 2022 at 11:17:55AM +0300, Serge Semin wrote:\n> > > > > > Synopsys AHCI SATA controller is mainly compatible with the generic AHCI\n> > > > > > SATA controller except a few peculiarities and the platform environment\n> > > > > > requirements. In particular it can have one or two reference clocks to\n> > > > > > feed up its AXI/AHB interface and SATA PHYs domain and at least one reset\n> > > > > > control for the application clock domain. In addition to that the DMA\n> > > > > > interface of each port can be tuned up to work with the predefined maximum\n> > > > > > data chunk size. Note unlike generic AHCI controller DWC AHCI can't have\n> > > > > > more than 8 ports. All of that is reflected in the new DWC AHCI SATA\n> > > > > > device DT binding.\n> > > > > > \n> > > > > > Note the DWC AHCI SATA controller DT-schema has been created in a way so\n> > > > > > to be reused for the vendor-specific DT-schemas (see for example the\n> > > > > > \"snps,dwc-ahci\" compatible string binding). One of which we are about to\n> > > > > > introduce.\n> > > > > > \n> > > > > > Signed-off-by: Serge Semin \n> > > > > > \n> > > > > > ---\n> > > > > > \n> > > > > > Changelog v2:\n> > > > > > - Replace min/max constraints of the snps,{tx,rx}-ts-max property with\n> > > > > > enum [ 1, 2, 4, ..., 1024 ]. (@Rob)\n> > > > > > \n> > > > > > Changelog v4:\n> > > > > > - Decrease the \"additionalProperties\" property identation otherwise it's\n> > > > > > percieved as the node property instead of the key one. (@Rob)\n> > > > > > - Use the ahci-port properties definition from the AHCI common schema\n> > > > > > in order to extend it with DWC AHCI SATA port properties. (@Rob)\n> > > > > > - Remove the Hannes' rb tag since the patch content has changed.\n> > > > > > ---\n> > > > > > .../bindings/ata/ahci-platform.yaml | 8 --\n> > > > > > .../bindings/ata/snps,dwc-ahci.yaml | 129 ++++++++++++++++++\n> > > > > > 2 files changed, 129 insertions(+), 8 deletions(-)\n> > > > > > create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > > \n> > > > > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > > index e19cf9828e68..7dc2a2e8f598 100644\n> > > > > > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml\n> > > > > > @@ -30,8 +30,6 @@ select:\n> > > > > > - marvell,armada-3700-ahci\n> > > > > > - marvell,armada-8k-ahci\n> > > > > > - marvell,berlin2q-ahci\n> > > > > > - - snps,dwc-ahci\n> > > > > > - - snps,spear-ahci\n> > > > > > required:\n> > > > > > - compatible\n> > > > > > \n> > > > > > @@ -48,17 +46,11 @@ properties:\n> > > > > > - marvell,berlin2-ahci\n> > > > > > - marvell,berlin2q-ahci\n> > > > > > - const: generic-ahci\n> > > > > > - - items:\n> > > > > > - - enum:\n> > > > > > - - rockchip,rk3568-dwc-ahci\n> > > > > > - - const: snps,dwc-ahci\n> > > > > > - enum:\n> > > > > > - cavium,octeon-7130-ahci\n> > > > > > - hisilicon,hisi-ahci\n> > > > > > - ibm,476gtr-ahci\n> > > > > > - marvell,armada-3700-ahci\n> > > > > > - - snps,dwc-ahci\n> > > > > > - - snps,spear-ahci\n> > > > > > \n> > > > > > reg:\n> > > > > > minItems: 1\n> > > > > > diff --git a/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > > new file mode 100644\n> > > > > > index 000000000000..af78f6c9b857\n> > > > > > --- /dev/null\n> > > > > > +++ b/Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml\n> > > > > > @@ -0,0 +1,129 @@\n> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)\n> > > > > > +%YAML 1.2\n> > > > > > +---\n> > > > > > +$id: http://devicetree.org/schemas/ata/snps,dwc-ahci.yaml#\n> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#\n> > > > > > +\n> > > > > > +title: Synopsys DWC AHCI SATA controller\n> > > > > > +\n> > > > > > +maintainers:\n> > > > > > + - Serge Semin \n> > > > > > +\n> > > > > > +description:\n> > > > > > + This document defines device tree bindings for the Synopsys DWC\n> > > > > > + implementation of the AHCI SATA controller.\n> > > > > > +\n> > > > > > +allOf:\n> > > > > > + - $ref: ahci-common.yaml#\n> > > > > > +\n> > > > > > +properties:\n> > > > > > + compatible:\n> > > > > > + oneOf:\n> > > > > > + - description: Synopsys AHCI SATA-compatible devices\n> > > > > > + contains:\n> > > > > > + const: snps,dwc-ahci\n> > > > > > + - description: SPEAr1340 AHCI SATA device\n> > > > > > + const: snps,spear-ahci\n> > > > > > + - description: Rockhip RK3568 ahci controller\n> > > > > > + const: rockchip,rk3568-dwc-ahci\n> > > > > \n> > > > \n> > > > > This is never true because there is a fallback. We should keep what we \n> > > > > had before.\n> > > > \n> > > > Could you be more specific what you meant? I don't see\n> > > > \"snps,spear-ahci\" and \"rockchip,rk3568-dwc-ahci\" used with the fallback\n> > > > string so modification is correct in that case.\n> > > \n> > \n> > > Spear does not, just rockchip:\n> > > \n> > > arch/arm64/boot/dts/rockchip/rk3568.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > > arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > > arch/arm64/boot/dts/rockchip/rk356x.dtsi: compatible = \"rockchip,rk3568-dwc-ahci\", \"snps,dwc-ahci\";\n> > > \n> > > So the 3rd entry is never true.\n> > \n> > Then I'll have to split the schema up into two bindings:\n> > 1. snps,dwc-ahci-common.yaml: generic DW SATA AHCI properties and no \"compatible\"\n> > property constraint since you said fallback was useless.\n> > 2. snps,dwc-ahci.yaml: generic DW SATA AHCI DT-schema with\n> > competibles: (\"snps,dwc-ahci\"), (\"snps,spear-ahci\"),\n> > (\"rockchip,rk3568-dwc-ahci\",\"snps,dwc-ahci\").\n> > \n> > Are you ok with this?\n> \n\n> Yes.\n> \n> > BTW if we had the fallback required the splitting up couldn't have\n> > been needed.\n> \n> We generally end up needing a split like this anyways.\n\nOk. I'll split it up into two schemas then.\n\n> \n> \n> > > > My idea was to have the compatible strings with the required generic\n> > > > fallback \"snps,dwc-ahci\" for all new devices thus identifying the\n> > > > controller IP-core origin. But later you said \"The generic IP block\n> > > > fallbacks have proven to be useless.\" I do agree that functionally it\n> > > > isn't that often used, but in some cases it can be handy for instance\n> > > > to implement quirks in the generic code or use the fallback as an\n> > > > additional info regarding the IP-core origin/version. So if I were you\n> > > > I wouldn't be that strict about dropping the generic IP-core fallback\n> > > > identifier. It's much easier to have it specified from the very\n> > > > beginning than adding it after it has been declared as not required.\n> > > \n> > > I wish they were useful, but experience has shown they are not.\n> > \n> > So what to do with the generic fallback compatibles then? Please\n> > answer to the next questions so I would correct all my currently\n> > stashed patches in accordance with it.\n> > \n> > 1) Do you want all the new DT-binding schemas refusing to have the\n> > fallback compatibles except for the nodes which bindings have already\n> > been defined that way?\n> \n> Yes. I wouldn't go quite as far as 'refusing'. I'm okay with a fallback \n> in cases that are simple enough to actually work without platform \n> specific code. As soon as the clocks, resets, phys, etc. aren't \n> standard, that goes out the window. Based on experience, that pretty \n> much never happens except on the IP vendor's FPGA.\n> \n> \n> > 2) What if a device IP-core has some versioning, but it's either\n> > not auto-detectable at runtime or can be auto-detected but starting\n> > from some IP-core version? Do we need it being specified in addition\n> > to the vendor-specific compatible string?\n> \n> By the time you are probing the device, you know the specific SoC and \n> can just set a version variable easily. Why have a string to parse that \n> doesn't work for version comparisons (e.g GT/GE/LT).\n> \n> Also, what if you don't know the exact IP version? Maybe you can guess \n> that it is at least at certain version based on knowing the features, so \n> you set that version. Would you really want to put that guess in DT when \n> later on you might need to change it?\n> \n> > 3) The same as 2), but shall it have a generic version-less fallback\n> > compatible string too?\n> \n> If the device can function without the version specific compatible.\n> \n> > 4) The same as 2), but what if it concerns a device which driver\n> > relies on the versioning?\n> > \n> > 5) The same as 2), but what if it concerns the device which currently\n> > doesn't have a driver relying on the IP-core version?\n> \n> Again, let the driver set the version based on the platform specific \n> compatible.\n> \n> > 6) What if we don't have the generic fallback compatible string\n> > required, but at some point a kernel would need it to\n> > implement a version/IP-core-specific quirk? If we had the generic\n> > fallback specified in dts the older systems would have been supported\n> > out-of-box, otherwise the firmware update would also needed.\n> \n> Again, when you start probing the device, you already know the specific \n> platform implementation. From that, you can easily imply the IP vendor \n> and version. No DT change needed.\n> \n> > IMO having the IP-core version + generic compatibles give many\n> > benefits and it's much easier to have them required from the very\n> > beginning instead of adding afterwards when then a need arises.\n> \n> Certainly adding afterwards is broken. That's why we insist on SoC \n> specific compatibles. Adding them when we have some platform specific \n> quirk doesn't work.\n\nGot it. Thanks for the very thorough clarification. I'll fix my patches\nin accordance with the described requirements.\n\n-Serge(y)\n\n> \n> Rob","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=bonbs0Fi;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LjCR36FnZz9s07\n\tfor ; Wed, 13 Jul 2022 06:44:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S233514AbiGLUoG (ORCPT );\n Tue, 12 Jul 2022 16:44:06 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:48850 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S229700AbiGLUoA (ORCPT\n ); Tue, 12 Jul 2022 16:44:00 -0400","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n [IPv6:2a00:1450:4864:20::12f])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 68D271C12A;\n Tue, 12 Jul 2022 13:43:57 -0700 (PDT)","by mail-lf1-x12f.google.com with SMTP id n18so14199509lfq.1;\n Tue, 12 Jul 2022 13:43:57 -0700 (PDT)","from mobilestation ([95.79.140.178])\n by smtp.gmail.com with ESMTPSA id\n s25-20020a056512203900b0047f6b4a53cdsm2367110lfs.172.2022.07.12.13.43.54\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Tue, 12 Jul 2022 13:43:54 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=2QnPPtTmS0obTiVf4sj7DHwqK9Ykca6pBt674ZHo4Fk=;\n b=bonbs0FiIHsy2/T68V0/LFwWZbzJhceXVnOJFoo7BVj/O+YipcUWTm2yO5JvoGNfME\n 7hFJloRmvOB9OQSMLEHZB+0JfPN4ltOGJH6rVGv/tHXXZxfU1P4apf0AbhkpI6ILzgio\n CFE/umhCAHrSuK0fkZpeis8bQSaKSV2reEvODEuYTXmi3Z0qEPkApPYavLNbOjXUvfX4\n RSfQp/zZxicW75Et1p3VlEWB+Ib4l4U7lHhnA+OcHIV65UY1FHXgJEpqoUoWTucJHkEQ\n PFji8cNI69KE2y1VfUyFyON5gXfKtplgQeiyQhRMlmQZ/TXosuV2wsDUI79DjdBz/n3L\n XW3Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=2QnPPtTmS0obTiVf4sj7DHwqK9Ykca6pBt674ZHo4Fk=;\n b=gUL6Xgpy2Fqf48hhsJ1d+bf0iL2LVqZUZDbs6mvttstEL83QqFs8i9ufyfIOMmHU77\n PSGSiiHdCPFRGrXxm5oqmWj2NvWzzLQ5edtqrnWXG19iYiMnAS/w91Lulk4NFGSA7SfV\n PrZ0TSBvhcz9mh0SxqXsMCgi0bKhPmatvLjv5Jl+MsgFqVhPTJ9gevQPXmtgk+fHwkAH\n 8RI/cCzqcZOsanwxUE6bG7iyEn8NLGaydz1MgUW2x/lPjt4FqEkaxwA/09d3hAahJkT4\n /Uzdz0kRWAruEFWYrFGF+wbJJqxWaQDVo3L3QIRD1EYduCKp2Q2XvC6M4YthjhPiOLlZ\n 45xg==","X-Gm-Message-State":"AJIora/2j7KsRbEfxuH60/CeTB23GJf3B7NlD4zVwynddJi++BPjyo3R\n tT8iPAJSTmY5iYOtAP7tAjAyH3SNSYtT4Q==","X-Google-Smtp-Source":"\n AGRyM1uY7aFXwoG4RBfq2dxZv4bo11fHrF9y6JIfmSp3mucML+Rr81HyiatY6fBZGFXXcuY9XIAsog==","X-Received":"by 2002:a05:6512:3e01:b0:489:c6e7:71b9 with SMTP id\n i1-20020a0565123e0100b00489c6e771b9mr14036749lfv.605.1657658635543;\n Tue, 12 Jul 2022 13:43:55 -0700 (PDT)","Date":"Tue, 12 Jul 2022 23:43:52 +0300","From":"Serge Semin ","To":"Rob Herring ","Cc":"Serge Semin ,\n Damien Le Moal ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Krzysztof Kozlowski ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,\n devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA\n controller DT schema","Message-ID":"<20220712204352.5qqk5wbvpe4iihrv@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-18-Sergey.Semin@baikalelectronics.ru>\n <20220614222754.GA2830345-robh@kernel.org>\n <20220617193744.av27axznbogademt@mobilestation>\n <20220706223642.GC572635-robh@kernel.org>\n <20220707152539.tktdo4qnvwormkqk@mobilestation>\n <20220712201342.GM1823936-robh@kernel.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220712201342.GM1823936-robh@kernel.org>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}}]