Hi Krzysztof, Thank you the review comments. On Mon, Dec 19, 2022 at 04:40:35PM +0100, Krzysztof Kozlowski wrote: > On 19/12/2022 15:24, Piyush Malgujar wrote: > > From: Jayanthi Annadurai <jannadurai@xxxxxxxxxxx> > > > > Subject: use final prefix matching the file, so "cdns,sdhci:" > > > Add support for SD6 controller support > > Full stop. > > > > > Signed-off-by: Jayanthi Annadurai <jannadurai@xxxxxxxxxxx> > > Signed-off-by: Piyush Malgujar <pmalgujar@xxxxxxxxxxx> > > --- > > .../devicetree/bindings/mmc/cdns,sdhci.yaml | 33 +++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..2043e78ccd5f708a01e87fd96ec410418fcd539f 100644 > > --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > @@ -4,7 +4,7 @@ > > $id: http://devicetree.org/schemas/mmc/cdns,sdhci.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Cadence SD/SDIO/eMMC Host Controller (SD4HC) > > +title: Cadence SD/SDIO/eMMC Host Controller (SD4HC, SD6HC) > > > > maintainers: > > - Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> > > @@ -19,6 +19,7 @@ properties: > > - microchip,mpfs-sd4hc > > - socionext,uniphier-sd4hc > > - const: cdns,sd4hc > > + - const: cdns,sd6hc > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > > ... because it does not really make sense. Why do you require SD6HC as > fallback? I think you meant enum. > Yes, that's correct. I will change it to enum. > > > > reg: > > maxItems: 1 > > @@ -111,6 +112,34 @@ properties: > > minimum: 0 > > maximum: 0x7f > > > > + cdns,iocell_input_delay: > > No underscores. Use proper units in name suffix: > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml > > > > + description: Delay in ps across the input IO cells > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > Ditto... and so on - all of the fields. > > > + > > + cdns,iocell_output_delay: > > + description: Delay in ps across the output IO cells > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,delay_element: > > + description: Delay element in ps used for calculating phy timings > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,read_dqs_cmd_delay: > > + description: Command delay used in HS200 tuning > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,tune_val_start: > > + description: Staring value of data delay used in HS200 tuning > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,tune_val_step: > > + description: Incremental value of data delay used in HS200 tuning > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,max_tune_iter: > > + description: Maximum number of iterations to complete the HS200 tuning process > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > Why these three are properties of DT? > These tuning parameters are added here so to make them custom configurable for different boards. > > + > > required: > > - compatible > > - reg > > @@ -122,7 +151,7 @@ unevaluatedProperties: false > > examples: > > - | > > emmc: mmc@5a000000 { > > - compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc"; > > + compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc", "cdns,sd6hc"; > > This is confusing. I don't understand it. It requires much more > explanation in your commit msg. > > > reg = <0x5a000000 0x400>; > > interrupts = <0 78 4>; > > clocks = <&clk 4>; > > Best regards, > Krzysztof > Rest of the comments will be taken care in V2. Thanks, Piyush
