| Message ID | 20230123192735.21136-5-pmalgujar@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: mmc: sdhci-cadence: SD6 controller support | expand |
On 23/01/2023 20:27, Piyush Malgujar wrote: > From: Jayanthi Annadurai <jannadurai@marvell.com> > > Add support for SD6 controller support. This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. > > Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> > --- > .../devicetree/bindings/mmc/cdns,sdhci.yaml | 34 +++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..26ef2804aa9e17c583adaa906338ec7af8c4990b 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@socionext.com> > @@ -18,7 +18,9 @@ properties: > - enum: > - microchip,mpfs-sd4hc > - socionext,uniphier-sd4hc > - - const: cdns,sd4hc > + - enum: > + - cdns,sd4hc > + - cdns,sd6hc > > reg: > maxItems: 1 > @@ -111,6 +113,34 @@ properties: > minimum: 0 > maximum: 0x7f > > + cdns,iocell-input-delay: > + description: Delay in ps across the input IO cells Use proper unit suffix -ps, so ref wont' be needed. This comment was also ignored. > + $ref: "/schemas/types.yaml#/definitions/uint32" > + > + cdns,iocell-output-delay: > + description: Delay in ps across the output IO cells Ditto > + $ref: "/schemas/types.yaml#/definitions/uint32" > + > + cdns,delay-element: > + description: Delay element in ps used for calculating phy timings Ditto > + $ref: "/schemas/types.yaml#/definitions/uint32" > + > + cdns,read-dqs-cmd-delay: > + description: Command delay used in HS200 tuning What are the units? > + $ref: "/schemas/types.yaml#/definitions/uint32" Drop quotes (everywhere) > + > + cdns,tune-val-start: > + description: Staring value of data delay used in HS200 tuning Same problem - missing units. > + $ref: "/schemas/types.yaml#/definitions/uint32" > + I don't get why the feedback has to be repeated. It's a bit a waste of time, isn't it? Best regards, Krzysztof
Hi Krzysztof, Thank you for your review comments. On Mon, Jan 23, 2023 at 08:42:54PM +0100, Krzysztof Kozlowski wrote: > On 23/01/2023 20:27, Piyush Malgujar wrote: > > From: Jayanthi Annadurai <jannadurai@marvell.com> > > > > Add support for SD6 controller support. > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. > > Thank you. > > > > > Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com> > > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> > > --- > > .../devicetree/bindings/mmc/cdns,sdhci.yaml | 34 +++++++++++++++++-- > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..26ef2804aa9e17c583adaa906338ec7af8c4990b 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@socionext.com> > > @@ -18,7 +18,9 @@ properties: > > - enum: > > - microchip,mpfs-sd4hc > > - socionext,uniphier-sd4hc > > - - const: cdns,sd4hc > > + - enum: > > + - cdns,sd4hc > > + - cdns,sd6hc > > > > reg: > > maxItems: 1 > > @@ -111,6 +113,34 @@ properties: > > minimum: 0 > > maximum: 0x7f > > > > + cdns,iocell-input-delay: > > + description: Delay in ps across the input IO cells > > Use proper unit suffix -ps, so ref wont' be needed. > > This comment was also ignored. > > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,iocell-output-delay: > > + description: Delay in ps across the output IO cells > > Ditto > > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,delay-element: > > + description: Delay element in ps used for calculating phy timings > > Ditto > Yes, will add -ps and remove the $ref. > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > + cdns,read-dqs-cmd-delay: > > + description: Command delay used in HS200 tuning > > What are the units? > > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > Drop quotes (everywhere) > > > + > > + cdns,tune-val-start: > > + description: Staring value of data delay used in HS200 tuning > > Same problem - missing units. > These are integer values, will add in the description that these units are integer used in tuning. Also, will remove the $ref. > > + $ref: "/schemas/types.yaml#/definitions/uint32" > > + > > > I don't get why the feedback has to be repeated. It's a bit a waste of > time, isn't it? > > Best regards, > Krzysztof > Please let me know if with above changes, it will be good and acceptable for V3. Thanks, Piyush
On 20/02/2023 14:22, Piyush Malgujar wrote: >>> + >>> + cdns,read-dqs-cmd-delay: >>> + description: Command delay used in HS200 tuning >> >> What are the units? >> >>> + $ref: "/schemas/types.yaml#/definitions/uint32" >> >> Drop quotes (everywhere) >> >>> + >>> + cdns,tune-val-start: >>> + description: Staring value of data delay used in HS200 tuning >> >> Same problem - missing units. >> > > These are integer values, will add in the description that these units are integer used > in tuning. Also, will remove the $ref. All units are integer values. We don't talk about this. Delay is usually in time, thus I would assume here proper unit suffix. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..26ef2804aa9e17c583adaa906338ec7af8c4990b 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@socionext.com> @@ -18,7 +18,9 @@ properties: - enum: - microchip,mpfs-sd4hc - socionext,uniphier-sd4hc - - const: cdns,sd4hc + - enum: + - cdns,sd4hc + - cdns,sd6hc reg: maxItems: 1 @@ -111,6 +113,34 @@ properties: minimum: 0 maximum: 0x7f + cdns,iocell-input-delay: + description: Delay in ps across the input IO cells + $ref: "/schemas/types.yaml#/definitions/uint32" + + 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" + required: - compatible - reg