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