Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Message-ID: <trinity-ac45bde6-392d-4810-8aad-9a06d2bcd85a-1646050780475@3c-app-gmx-bs53>
From:   Frank Wunderlich <frank-w@public-files.de>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc:     Frank Wunderlich <linux@fw-web.de>,
        Rob Herring <robh+dt@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Jens Axboe <axboe@kernel.dk>, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org, Heiko Stuebner <heiko@sntech.de>,
        Peter Geis <pgwipeout@gmail.com>,
        Michael Riesch <michael.riesch@wolfvision.net>,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Aw: Re: [PATCH v3 1/3] dt-bindings: Convert ahci-platform DT
 bindings to yaml
Content-Type: text/plain; charset=UTF-8
Date:   Mon, 28 Feb 2022 13:19:40 +0100
Importance: normal
Sensitivity: Normal
In-Reply-To: <4c3303f5-7af5-9974-7bea-b7f0d6c7ef53@canonical.com>
References: <20220227182800.275572-1-linux@fw-web.de>
 <20220227182800.275572-2-linux@fw-web.de>
 <4c3303f5-7af5-9974-7bea-b7f0d6c7ef53@canonical.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Krzysztof,

thanks for first review.

> Gesendet: Montag, 28. Februar 2022 um 11:08 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@canonical.com>

> On 27/02/2022 19:27, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>

> You missed devicetree mailing list (corrupted address).

sorry, devicetree ML was To, but forget the Cc-Header (prepared addresses =
in coverletter)

> > imho all errors should be fixed in the dts not in the yaml...

> > -- reg               : <registers mapping>
> > -
> > -Please note that when using "generic-ahci" you must also specify a So=
C specific
> > -compatible:
> > -	compatible =3D "manufacturer,soc-model-ahci", "generic-ahci";
...
> > +properties:
> > +  compatible:
> > +    contains:
> > +      enum:
> > +        - brcm,iproc-ahci
> > +        - cavium,octeon-7130-ahci
> > +        - generic-ahci
> > +        - hisilicon,hisi-ahci
> > +        - ibm,476gtr-ahci
> > +        - marvell,armada-380-ahci
> > +        - marvell,armada-3700-ahci
>
> Order entries alphabetically.

ok

> > +        - snps,dwc-ahci
> > +        - snps,spear-ahci
>
> You converted the TXT bindings explicitly, but you missed the comment
> just below the 'reg' about generic-ahci. The generic-ahci never comes al=
one.

How should this comment be added? description above/below the compatible-p=
roperty?
Sorry for dumb questions...this is my first yaml ;)

e.g.

properties:
  compatible:
    description:
      Please note that when using "generic-ahci" you must also specify a S=
oC specific
      compatible:
         compatible =3D "manufacturer,soc-model-ahci", "generic-ahci";
    contains:
      enum:

> The snps,dwc-ahci could come, although history shows that Synapsys
> blocks are commonly re-used and they should have specific compatible.
> Current users still have single snps,dwc-ahci, so it could be fixed a
> bit later.
>
> On the other hand, I expect to fix all the DTS in the same series where
> the bindings are corrected.

i don't know the marvell/broadcom-hw so i cannot fix them.
Just converted the txt to check my rockchip sata nodes and to be more
future-proof (no more exceptions like the marvell/broadcom)

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 3
>
> Items should be described. Next or this patch could add also clock-names=
.

i was told to drop clock-names (same for interrupt-names and reset-names) =
from dts
and in txt it was not there so have not added it

https://patchwork.kernel.org/comment/24755956/

> > +
> > +  interrupts:
> > +    minItems: 1
>
> You mean maxItems?

no, minItems, as interrupts suggests 1+ (same for phys)

> > +
> > +  ahci-supply:
> > +    description:
> > +      regulator for AHCI controller
> > +
> > +  dma-coherent:
> > +    description:
> > +      Present if dma operations are coherent
>
> Skip description, it's obvious. Just 'true'.

ok, took this from the txt

> > +
> > +  phy-supply:
> > +    description:
> > +      regulator for PHY power
> > +
> > +  phys:
> > +    minItems: 1
>
> maxItems?
> > +
> > +  phy-names:
> > +    minItems: 1
>
> Describe the items.
>
> > +
> > +  ports-implemented:
> > +    description:
> > +      Mask that indicates which ports that the HBA supports
> > +      are available for software to use. Useful if PORTS_IMPL
> > +      is not programmed by the BIOS, which is true with
> > +      some embedded SoCs.
> > +    minItems: 1
>
> You need a type and maxItems.

what will be the type of a mask?

> > +
> > +  resets:
> > +    minItems: 1
>
> maxItems?

if there is a known maximum....

> > +
> > +  target-supply:
> > +    description:
> > +      regulator for SATA target power
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +patternProperties:
> > +  "^sata-port@[0-9]+$":
>
> You limit number of ports to 10. On purpose? What about 0xa? 0xb?

oh, right, there can be hexadecimal...
thought this is only true for the main-node (address) and have only seen @=
0, @1 and @2

> > +    type: object
> > +    description:
> > +      Subnode with configuration of the Ports.
> > +
> > +    properties:
> > +      reg:
> > +        maxItems: 1
> > +
> > +      phys:
> > +        minItems: 1
>
> maxItems? Why do you put everywhere minItems? Are several phys really
> expected?

name suggests that it can be more than 1. i know from usb subsystem (dwc3 =
usb3) that a device can have more than one phy, and because in the txt the=
re are no ranges i set everywhere MinItems to 1 with open end as i do not =
know all possibilities. Anything else will be trial and error...for all pr=
operties

> > +
> > +      target-supply:
> > +        description:
> > +          regulator for SATA target power
> > +
> > +    required:
> > +      - reg
> > +
> > +    anyOf:
> > +      - required: [ phys ]
> > +      - required: [ target-supply ]
> > +
> > +allOf:
> > +- $ref: "sata-common.yaml#"
>
> This goes before properties.
>
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +        sata@ffe08000 {
>
> Wrong indentation. It starts just below |

will fix it

> > +               compatible =3D "snps,spear-ahci";
> > +               reg =3D <0xffe08000 0x1000>;
> > +               interrupts =3D <115>;
> > +        };
> > +  - |
> > +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +        #include <dt-bindings/clock/berlin2q.h>
> > +        sata@f7e90000 {
> > +                compatible =3D "marvell,berlin2q-achi", "generic-ahci=
";
>
> This clearly won't pass your checks. I don't think you run
> dt_binding_check. You must test your bindings first.

i had them tested ...needed to add the includes...after that the dt_bindin=
gs_check was without errors/warnings

these are the commands i used:

ARCH=3Darm64 CROSS_COMPILE=3Daarch64-linux-gnu- make dt_binding_check DT_S=
CHEMA_FILES=3DDocumentation/devicetree/bindings/ata/ahci-platform.yaml
ARCH=3Darm64 CROSS_COMPILE=3Daarch64-linux-gnu- make dtbs_check DT_SCHEMA_=
FILES=3DDocumentation/devicetree/bindings/ata/ahci-platform.yaml

> Best regards,
> Krzysztof

regards Frank