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

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.

quoted

Gesendet: Montag, 28. Februar 2022 um 13:38 Uhr
Von: "Krzysztof Kozlowski" [off-list ref]

quoted

On 28/02/2022 13:19, Frank Wunderlich wrote:

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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.

quoted

quoted

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@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