[RFC,v2,1/4] dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support

Add compatible value for the T-Head TH1520 dwcmshc controller and
thead,io-fixed-1v8 and thead,pull-up properties.

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

On 05/08/2023 05:14, Drew Fustini wrote:
> Add compatible value for the T-Head TH1520 dwcmshc controller and
> thead,io-fixed-1v8 and thead,pull-up properties.
> 
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
>  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> index a43eb837f8da..57602c345cab 100644
> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> @@ -19,6 +19,7 @@ properties:
>        - rockchip,rk3568-dwcmshc
>        - rockchip,rk3588-dwcmshc
>        - snps,dwcmshc-sdhci
> +      - thead,th1520-dwcmshc
>  
>    reg:
>      maxItems: 1
> @@ -60,6 +61,14 @@ properties:
>      description: Specify the number of delay for tx sampling.
>      $ref: /schemas/types.yaml#/definitions/uint8
>  
> +  thead,io-fixed-1v8:
> +    description: SoC PHY pad is fixed 1.8V
> +    type: boolean

Isn't this duplicating existing properties for MMC modes with 1.8 V?

> +
> +  thead,pull-up:
> +    description: True if pull-up, false if pull-down

This explains me nothing. No clue what you are pulling and why do you
need it. Pin pulls should be done via pin controller, not MMC.

Anyway you should have here allOf:if:then (move the allOf: from top to
behind "required:") which will disallow these properties for other variants.

> +    type: boolean
> +
>  
>  required:
>    - compatible
> 

Best regards,
Krzysztof

On 17/08/2023 00:26, Drew Fustini wrote:
> On Mon, Aug 07, 2023 at 08:29:21AM +0200, Krzysztof Kozlowski wrote:
>> On 05/08/2023 05:14, Drew Fustini wrote:
>>> Add compatible value for the T-Head TH1520 dwcmshc controller and
>>> thead,io-fixed-1v8 and thead,pull-up properties.
>>>
>>> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> index a43eb837f8da..57602c345cab 100644
>>> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> @@ -19,6 +19,7 @@ properties:
>>>        - rockchip,rk3568-dwcmshc
>>>        - rockchip,rk3588-dwcmshc
>>>        - snps,dwcmshc-sdhci
>>> +      - thead,th1520-dwcmshc
>>>  
>>>    reg:
>>>      maxItems: 1
>>> @@ -60,6 +61,14 @@ properties:
>>>      description: Specify the number of delay for tx sampling.
>>>      $ref: /schemas/types.yaml#/definitions/uint8
>>>  
>>> +  thead,io-fixed-1v8:
>>> +    description: SoC PHY pad is fixed 1.8V
>>> +    type: boolean
>>
>> Isn't this duplicating existing properties for MMC modes with 1.8 V?
> 
> Thank you for reviewing. Yes, now that you mention it, I do see those
> properties now in mmc-controller.yaml. It seems like the existing
> mmc-ddr-1_8v property would be appropriate.
> 
>>
>>> +
>>> +  thead,pull-up:
>>> +    description: True if pull-up, false if pull-down
>>
>> This explains me nothing. No clue what you are pulling and why do you
>> need it. Pin pulls should be done via pin controller, not MMC.
> 
> Good point that my description is not helpful. The pull-up property
> determines whether certain phy registers are written to. I need to try
> to can get documentation on the phy so that I can better understand the
> details of the pull-up configuration in the phy registers.
> 
>>
>> Anyway you should have here allOf:if:then (move the allOf: from top to
>> behind "required:") which will disallow these properties for other variants.
> 
> I noticed that nvidia,tegra20-sdhci.yaml has several lines related to
> pull-up/down configuration:
> 
> 218   - if:
> 219       properties:
> 220         compatible:
> 221           contains:
> 222             const: nvidia,tegra210-sdhci
> 223     then:
> 224       properties:
> 225         pinctrl-names:
> 226           oneOf:
> 227             - items:
> 228                 - const: sdmmc-3v3
> 229                   description: pad configuration for 3.3 V
> 230                 - const: sdmmc-1v8
> 231                   description: pad configuration for 1.8 V
> 232                 - const: sdmmc-3v3-drv
> 233                   description: pull-up/down configuration for 3.3 V
> 234                 - const: sdmmc-1v8-drv
> 235                   description: pull-up/down configuration for 1.8 V
> 236             - items:
> 237                 - const: sdmmc-3v3-drv
> 238                   description: pull-up/down configuration for 3.3 V
> 239                 - const: sdmmc-1v8-drv
> 240                   description: pull-up/down configuration for 1.8 V
> 241             - items:
> 242                 - const: sdmmc-1v8-drv
> 243                   description: pull-up/down configuration for 1.8 V
> 
> Do you think creating something like that would be a good approach?

This depends. Does your driver implementation will make use of it? If
yes, then it makes sense.

Best regards,
Krzysztof