Messages in this thread | | | From | Frank Wunderlich <> | Subject | Aw: Re: [PATCH v3 1/3] dt-bindings: Convert ahci-platform DT bindings to yaml | Date | Mon, 28 Feb 2022 13:19:40 +0100 |
| |
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 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 ;)
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/
> > + > > + 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 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
> > + > > + 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 = "snps,spear-ahci"; > > + reg = <0xffe08000 0x1000>; > > + interrupts = <115>; > > + }; > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/berlin2q.h> > > + sata@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
> Best regards, > Krzysztof
regards Frank
| |