Aw: Re: [PATCH v3 1/3] dt-bindings: Convert ahci-platform DT bindings to yaml

[Krzysztof Kozlowski]

On 28/02/2022 13:19, Frank Wunderlich wrote:
> Hi Krzysztof,
> 
> thanks for first review.
> 
>> Gesendet: Montag, 28. Februar 2022 um 11:08 Uhr
>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski at canonical.com>
> 
>> On 27/02/2022 19:27, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w at 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 SoC specific
>>> -compatible:
>>> -	compatible = "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 alone.
> 
> How should this comment be added? description above/below the compatible-property?
> Sorry for dumb questions...this is my first yaml ;)

No, this has to be oneOf. See for example
Documentation/devicetree/bindings/gpio/gpio-vf610.yaml or many other files.

> 
> e.g.
> 
> properties:
>   compatible:
>     description:
>       Please note that when using "generic-ahci" you must also specify a SoC specific
>       compatible:
>          compatible = "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/

OK, then let's skip them now. The clock items should be described if it
is possible.

> 
>>> +
>>> +  interrupts:
>>> +    minItems: 1
>>
>> You mean maxItems?
> 
> no, minItems, as interrupts suggests 1+ (same for phys)

You cannot have infinite number of interrupts... What suggests "1+"?
What does it mean "as interrupts suggests"? Do these hardware blocks
really have many interrupt lines?

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

`git grep ports-implemented` gives pretty straightfoward answer. All DTS
have u32 and driver also uses u32.

> 
>>> +
>>> +  resets:
>>> +    minItems: 1
>>
>> maxItems?
> 
> if there is a known maximum....

Must be. You cannot have infinite number of reset lines... Please check
all DTS and drivers. If there is public documentation, it also might be
useful.

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

What do you mean "name suggests"? Name of property? No, it does not
suggest that. Name is standard. Please check example schema and other
existing schema bindings to see how it is done. For example earlier
gpio-vf610.yaml is not bad.

> i know from usb subsystem (dwc3 usb3) that a device can have more than one phy, and because in the txt there 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 properties

The bindings need to be specific, so only properties which really,
really can have many unknown elements we could keep here some high
maxItems limit. In 99% of cases maxItems are clearly defined.

> 
>>> +
>>> +      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 at ffe08000 {
>>
>> Wrong indentation. It starts just below |
> 
> will fix it
> 
>>> +               compatible = "snps,spear-ahci";
>>> +               reg = <0xffe08000 0x1000>;
>>> +               interrupts = <115>;
>>> +        };
>>> +  - |
>>> +        #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +        #include <dt-bindings/clock/berlin2q.h>
>>> +        sata at f7e90000 {
>>> +                compatible = "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_bindings_check was without errors/warnings
> 
> these are the commands i used:
> 
> ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/ahci-platform.yaml
> ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/ahci-platform.yaml

Install dependencies (libyaml-dev) and you will see first error:
Documentation/devicetree/bindings/ata/ahci-platform.yaml:110:1:
[warning] wrong indentation: expected 2 but found 0 (indentation)

But the one I am thinking is indeed not visible by default. You would
need to run it like Rob's boot is running, so add DT_CHECKER_FLAGS=-m.
Then you see:

Documentation/devicetree/bindings/ata/ahci-platform.example.dt.yaml:0:0:
/example-1/sata at f7e90000: failed to match any schema with compatible:
['marvell,berlin2q-achi', 'generic-ahci']




Best regards,
Krzysztof