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

Mon Feb 28 06:35:01 PST 2022

On 28/02/2022 15:09, Frank Wunderlich wrote:
> Hi,
> 
> looks like i'm not the right person to convert the binding. I have not enough knowledge about yaml, bindings, drivers, ...
> 
> i will try you suggestions for me as a lerning-by-doing approach, but will drop it from the series, same for part2 as it depends on first.
> 
> 
> 
>> Gesendet: Montag, 28. Februar 2022 um 13:38 Uhr
>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski at canonical.com>
> 
>> On 28/02/2022 13:19, Frank Wunderlich wrote:
> 
>>>> 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.
> 
> Afaik the generic-ahci should be defined optional with one of the others needed, but afaik this will duplicate the list i had.
> 
> so i end up in a struct like this
> 
> compatible:
>   oneOf:
>     - enum:
>       - brcm,iproc-ahci
>       - cavium,octeon-7130-ahci
>       - hisilicon,hisi-ahci
>       - ibm,476gtr-ahci
>       - marvell,armada-3700-ahci
>       - marvell,armada-380-ahci
>       - snps,dwc-ahci
>       - snps,spear-ahci
>     - items:
>       - const: generic-ahci
>       - enum:
>         - brcm,iproc-ahci
>         - cavium,octeon-7130-ahci
>         - hisilicon,hisi-ahci
>         - ibm,476gtr-ahci
>         - marvell,armada-3700-ahci
>         - marvell,armada-380-ahci
>         - snps,dwc-ahci
>         - snps,spear-ahci

That could be one way, but instead I propose to have only second part
(so enum + generic-ahci) for all compatibles mentioned in
ahci_platform.c, which do not customize the driver behavior for these
compatibles..

> 
> 
>>>>> + 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.
> 
> interrupts/phys is plural of interrupt/phy, so it suggests it can be more than 1.

There is no property named "interrupt" or "phy". There is only
"interrupts", "phys", "gpios", "clocks" etc. regardless whether there is
one or multiple items. This is not specific to bindings, but all DTS
have this.

> 
> as i said i do not know every driver with all possibilities, so i started with the min-items as we need at least 1 interrupt/phy, but yes if there are any limits then they should be added, but this needs extensive knowledge about the drivers/hardware, i don't have.

But the sources of the driver and DTS are available, why not using them?

> 
>>>>> + 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.
> 
> so type should be
> 
> $ref: '/schemas/types.yaml#/definitions/uint32'

Yes. See example schema.

> 
> ?
> 
> it's the only one i've found with u32 looking like a type
> 
> found in Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.yaml

This part I do not understand.

> 
>>>
>>>>> +
>>>>> + 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.
> 
> "Please check all DTS and drivers." this is impossible for me as doing this as hobby with still limited time ;(

I believe that information - how many resets - is in general findable
via driver sources. In all bindings conversions, if we do not have such
information, we need to try to look it up.

>>>
>>>>> +
>>>>> + 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.
> 
> so i need to do try-and-error, setting maxItems to 1, make checks, if failing look in driver, increase, ...

Why try-and-error? "git grep" works here...

> 
> but this is maybe not the right way to do as dts can contain errors which should not modify the binding.

True, but having stricter limit, even if not fully correct, is better
than having too loose limit. Stricter can be always loosened. Loose
requirement cannot be made stricter.

> 
>>> 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']
> 
> i try this, but imho it's better to drop the Patch as i'm no expert in this, don't know the HW/drivers enough and this will delay the dts patch too much.
> I thought i can help getting this (simple looking) txt converted to yaml.
> Seems the binding needs to be done by someone who knows the drivers more than me.
> 
> regards Frank

Best regards,
Krzysztof