diff mbox series

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

Message ID 20230724-th1520-emmc-v2-1-132ed2e2171e@baylibre.com
State New
Headers show
Series RISC-V: Add basic eMMC support for BeagleV Ahead | expand

Commit Message

Drew Fustini Aug. 5, 2023, 3:14 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski Aug. 7, 2023, 6:29 a.m. UTC | #1
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
Drew Fustini Aug. 16, 2023, 10:26 p.m. UTC | #2
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?

Thank you,
Drew
Krzysztof Kozlowski Aug. 19, 2023, 1:06 p.m. UTC | #3
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
diff mbox series

Patch

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
+
+  thead,pull-up:
+    description: True if pull-up, false if pull-down
+    type: boolean
+
 
 required:
   - compatible