[3/5] dt-bindings: mmc: sdhci-cadence: SD6 support

  From: Jayanthi Annadurai <jannadurai@marvell.com>

Add support for SD6 controller support

Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 33 +++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)
  

On 19/12/2022 15:24, Piyush Malgujar wrote:
> From: Jayanthi Annadurai <jannadurai@marvell.com>
> 

Subject: use final prefix matching the file, so "cdns,sdhci:"

> Add support for SD6 controller support

Full stop.

> 
> Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---
>  .../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@socionext.com>
> @@ -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.

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

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

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@marvell.com>
> > 
> 
> Subject: use final prefix matching the file, so "cdns,sdhci:"
> 
> > Add support for SD6 controller support
> 
> Full stop.
> 
> > 
> > Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---
> >  .../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@socionext.com>
> > @@ -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
  

On 06/01/2023 17:48, Piyush Malgujar wrote:
> 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@marvell.com>
>>>
>>
>> Subject: use final prefix matching the file, so "cdns,sdhci:"
>>
>>> Add support for SD6 controller support
>>
>> Full stop.
>>
>>>
>>> Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
>>> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
>>> ---
>>>  .../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@socionext.com>
>>> @@ -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.

I understand why do you wanted to add them, but I am asking why these
are suitable for DT? DT  describes hardware, so what is here specific to
hardware which requires DT property?


Best regards,
Krzysztof
  

Hi Krzysztof,

On Sat, Jan 07, 2023 at 02:25:02PM +0100, Krzysztof Kozlowski wrote:
> On 06/01/2023 17:48, Piyush Malgujar wrote:
> > 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@marvell.com>
> >>>
> >>
> >> Subject: use final prefix matching the file, so "cdns,sdhci:"
> >>
> >>> Add support for SD6 controller support
> >>
> >> Full stop.
> >>
> >>>
> >>> Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
> >>> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> >>> ---
> >>>  .../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@socionext.com>
> >>> @@ -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.
> 
> I understand why do you wanted to add them, but I am asking why these
> are suitable for DT? DT  describes hardware, so what is here specific to
> hardware which requires DT property?
> 
> 

We have different values based on emmc devices populated on different boards and these
tuning parameters are used to program phy registers accordingly.

> Best regards,
> Krzysztof

Thanks,
Piyush>