[v3,5/6] 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   | 24 +++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)
  

On 27/02/2023 19:31, Piyush Malgujar wrote:
> 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   | 24 +++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..0dba17c4f17f82c8ae68e46225ed72418e8361ff 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

I see here rather random set of changes in each version of this patch.
This does not really make sense. You are saying that existing (!!!)
mpfs-sd4hc is compatible with sd6hc. I think you wanted oneOf here, but
not sure. Can you explain what is your intention? Your commit msg is
just one line saying the same as subject, so not really helpful.


Best regards,
Krzysztof
  

On Mon, Feb 27, 2023 at 10:31:50AM -0800, Piyush Malgujar wrote:
> From: Jayanthi Annadurai <jannadurai@marvell.com>
> 
> Add support for SD6 controller support.

On what h/w?

> 
> Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---
>  .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 24 +++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..0dba17c4f17f82c8ae68e46225ed72418e8361ff 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

Other than FPGA implementations IP vendor compatible strings are pretty 
much useless. Define a compatible for your h/w.

>  
>    reg:
>      maxItems: 1
> @@ -111,6 +113,24 @@ properties:
>      minimum: 0
>      maximum: 0x7f
>  
> +  cdns,iocell-input-delay-ps:
> +    description: Delay in ps across the input IO cells
> +
> +  cdns,iocell-output-delay-ps:
> +    description: Delay in ps across the output IO cells
> +
> +  cdns,delay-element-ps:
> +    description: Delay element in ps used for calculating phy timings
> +
> +  cdns,read-dqs-cmd-delay-ps:
> +    description: Command delay used in HS200 tuning
> +
> +  cdns,tune-val-start-ps:
> +    description: Staring value of data delay used in HS200 tuning
> +
> +  cdns,tune-val-step-ps:
> +    description: Incremental value of data delay used in HS200 tuning

Wouldn't any controller implementation need these possibly? IIRC, we 
have some common properties for this. If not, survey what we do have and 
come up with something common. Or you can imply all this from the h/w 
specific compatible you are going to add.

Rob
  

Hi Krzysztof,

Thanks for the review comments.

On Tue, Feb 28, 2023 at 11:53:51AM +0100, Krzysztof Kozlowski wrote:
> On 27/02/2023 19:31, Piyush Malgujar wrote:
> > 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   | 24 +++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> > index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..0dba17c4f17f82c8ae68e46225ed72418e8361ff 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
> 
> I see here rather random set of changes in each version of this patch.
> This does not really make sense. You are saying that existing (!!!)
> mpfs-sd4hc is compatible with sd6hc. I think you wanted oneOf here, but
> not sure. Can you explain what is your intention? Your commit msg is
> just one line saying the same as subject, so not really helpful.
> 
> 
> Best regards,
> Krzysztof
>

Yes thank you, it should be oneOf as sd6hc is exclusive. I will correct it in the
next version.

Best Regards,
Piyush
  

Hi Rob,

Thanks for the review comments.

On Tue, Feb 28, 2023 at 09:46:48AM -0600, Rob Herring wrote:
> On Mon, Feb 27, 2023 at 10:31:50AM -0800, Piyush Malgujar wrote:
> > From: Jayanthi Annadurai <jannadurai@marvell.com>
> > 
> > Add support for SD6 controller support.
> 
> On what h/w?
> 

This has been used and tested on Marvell CN10K series SOCs.

> > 
> > Signed-off-by: Jayanthi Annadurai <jannadurai@marvell.com>
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---
> >  .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 24 +++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> > index 8b1a0fdcb5e3e2e8b87d8d7678e37f3dad447fc1..0dba17c4f17f82c8ae68e46225ed72418e8361ff 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
> 
> Other than FPGA implementations IP vendor compatible strings are pretty 
> much useless. Define a compatible for your h/w.
> 

ok, will use Marvell specific string here. 

> >  
> >    reg:
> >      maxItems: 1
> > @@ -111,6 +113,24 @@ properties:
> >      minimum: 0
> >      maximum: 0x7f
> >  
> > +  cdns,iocell-input-delay-ps:
> > +    description: Delay in ps across the input IO cells
> > +
> > +  cdns,iocell-output-delay-ps:
> > +    description: Delay in ps across the output IO cells
> > +
> > +  cdns,delay-element-ps:
> > +    description: Delay element in ps used for calculating phy timings
> > +
> > +  cdns,read-dqs-cmd-delay-ps:
> > +    description: Command delay used in HS200 tuning
> > +
> > +  cdns,tune-val-start-ps:
> > +    description: Staring value of data delay used in HS200 tuning
> > +
> > +  cdns,tune-val-step-ps:
> > +    description: Incremental value of data delay used in HS200 tuning
> 
> Wouldn't any controller implementation need these possibly? IIRC, we 
> have some common properties for this. If not, survey what we do have and 
> come up with something common. Or you can imply all this from the h/w 
> specific compatible you are going to add.
> 

These parameters are for sd6 cadence controller, will add sd6 in the property
string so to imply it's specific to sd6hc.

> Rob

Thanks,
Piyush