| Message ID | 20230921-th1520-mmc-v1-1-49f76c274fb3@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: Add eMMC support for TH1520 boards | expand |
Hey Drew, On Thu, Sep 21, 2023 at 06:49:48PM -0700, Drew Fustini wrote: > Add compatible value for the T-Head TH1520 dwcmshc controller and add > thead,phy-pull-up property. > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > --- > Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml > index a43eb837f8da..46b768d46712 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,9 @@ properties: > description: Specify the number of delay for tx sampling. > $ref: /schemas/types.yaml#/definitions/uint8 > > + thead,phy-pull-up: > + description: Enable weak pull-up on PHY pads > + type: boolean Why is the weak pull-up required? How would the dts author know if they need to use this property? Thanks, Conor. > > required: > - compatible > > -- > 2.34.1 >
On Fri, Sep 22, 2023 at 10:57:36AM +0100, Conor Dooley wrote: > Hey Drew, > > On Thu, Sep 21, 2023 at 06:49:48PM -0700, Drew Fustini wrote: > > Add compatible value for the T-Head TH1520 dwcmshc controller and add > > thead,phy-pull-up property. > > > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > > --- > > Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml > > index a43eb837f8da..46b768d46712 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,9 @@ properties: > > description: Specify the number of delay for tx sampling. > > $ref: /schemas/types.yaml#/definitions/uint8 > > > > + thead,phy-pull-up: > > + description: Enable weak pull-up on PHY pads > > + type: boolean > > Why is the weak pull-up required? How would the dts author know if they > need to use this property? This is a good question, and I don't have a good reason beyond it is what the vendor SDK was doing. There are only two boards right now using the TH1520 that I know of. Both the LPi4a [1] and the Ahead [2] have the pull-up property set on all the mmc controller nodes their downstream device trees. Rob suggested on #devicetree that it would be simpler to just enable it in the driver and disable support when needed. I like this idea as it will simplify this binding patch and the code in the driver patch. Thanks, Drew [1] https://git.beagleboard.org/beaglev-ahead/BeagleBoard-DeviceTrees/-/blob/v5.10.x-ti-unified/src/riscv/light-beagle-ref.dts [2] https://github.com/revyos/thead-kernel/blob/lpi4a/arch/riscv/boot/dts/thead/light-lpi4a-ref.dts
On Fri, Sep 22, 2023 at 11:18 PM Drew Fustini <dfustini@baylibre.com> wrote: > > On Fri, Sep 22, 2023 at 10:57:36AM +0100, Conor Dooley wrote: > > Hey Drew, > > > > On Thu, Sep 21, 2023 at 06:49:48PM -0700, Drew Fustini wrote: > > > Add compatible value for the T-Head TH1520 dwcmshc controller and add > > > thead,phy-pull-up property. > > > > > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > > > --- > > > Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml > > > index a43eb837f8da..46b768d46712 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,9 @@ properties: > > > description: Specify the number of delay for tx sampling. > > > $ref: /schemas/types.yaml#/definitions/uint8 > > > > > > + thead,phy-pull-up: > > > + description: Enable weak pull-up on PHY pads > > > + type: boolean > > > > Why is the weak pull-up required? How would the dts author know if they > > need to use this property? > > This is a good question, and I don't have a good reason beyond it is > what the vendor SDK was doing. > > There are only two boards right now using the TH1520 that I know of. > Both the LPi4a [1] and the Ahead [2] have the pull-up property set on > all the mmc controller nodes their downstream device trees. > > Rob suggested on #devicetree that it would be simpler to just enable it > in the driver and disable support when needed. I like this idea as it > will simplify this binding patch and the code in the driver patch. It's for the PHY debug. You could directly remove them, or keep it with no-pull-up flag, then no pull-up flag in the dts for default.. static void sdhci_phy_1_8v_init_no_pull(struct sdhci_host *host) { uint32_t val; sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R); sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R); sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R); val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R); val &= ~(1 << 4); sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R); val = sdhci_readw(host, PHY_CMDPAD_CNFG_R); sdhci_writew(host, val | 1, PHY_CMDPAD_CNFG_R); val = sdhci_readw(host, PHY_DATAPAD_CNFG_R); sdhci_writew(host, val | 1, PHY_DATAPAD_CNFG_R); val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R); sdhci_writew(host, val | 1, PHY_RSTNPAD_CNFG_R); val = sdhci_readw(host, PHY_STBPAD_CNFG_R); sdhci_writew(host, val | 1, PHY_STBPAD_CNFG_R); val = sdhci_readb(host, PHY_DLL_CTRL_R); sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R); } static void sdhci_phy_3_3v_init_no_pull(struct sdhci_host *host) { uint32_t val; sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R); sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R); sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R); val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R); val &= ~(1 << 4); sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R); val = sdhci_readw(host, PHY_CMDPAD_CNFG_R); sdhci_writew(host, val | 2, PHY_CMDPAD_CNFG_R); val = sdhci_readw(host, PHY_DATAPAD_CNFG_R); sdhci_writew(host, val | 2, PHY_DATAPAD_CNFG_R); val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R); sdhci_writew(host, val | 2, PHY_RSTNPAD_CNFG_R); val = sdhci_readw(host, PHY_STBPAD_CNFG_R); sdhci_writew(host, val | 2, PHY_STBPAD_CNFG_R); val = sdhci_readb(host, PHY_DLL_CTRL_R); sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R); } > > Thanks, > Drew > > [1] https://git.beagleboard.org/beaglev-ahead/BeagleBoard-DeviceTrees/-/blob/v5.10.x-ti-unified/src/riscv/light-beagle-ref.dts > [2] https://github.com/revyos/thead-kernel/blob/lpi4a/arch/riscv/boot/dts/thead/light-lpi4a-ref.dts
On Sat, Sep 23, 2023 at 04:10:11PM +0800, Guo Ren wrote: > On Fri, Sep 22, 2023 at 11:18 PM Drew Fustini <dfustini@baylibre.com> wrote: > > > > On Fri, Sep 22, 2023 at 10:57:36AM +0100, Conor Dooley wrote: > > > Hey Drew, > > > > > > On Thu, Sep 21, 2023 at 06:49:48PM -0700, Drew Fustini wrote: > > > > Add compatible value for the T-Head TH1520 dwcmshc controller and add > > > > thead,phy-pull-up property. > > > > > > > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > > > > --- > > > > Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml > > > > index a43eb837f8da..46b768d46712 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,9 @@ properties: > > > > description: Specify the number of delay for tx sampling. > > > > $ref: /schemas/types.yaml#/definitions/uint8 > > > > > > > > + thead,phy-pull-up: > > > > + description: Enable weak pull-up on PHY pads > > > > + type: boolean > > > > > > Why is the weak pull-up required? How would the dts author know if they > > > need to use this property? > > > > This is a good question, and I don't have a good reason beyond it is > > what the vendor SDK was doing. > > > > There are only two boards right now using the TH1520 that I know of. > > Both the LPi4a [1] and the Ahead [2] have the pull-up property set on > > all the mmc controller nodes their downstream device trees. > > > > Rob suggested on #devicetree that it would be simpler to just enable it > > in the driver and disable support when needed. I like this idea as it > > will simplify this binding patch and the code in the driver patch. > It's for the PHY debug. You could directly remove them, or keep it > with no-pull-up flag, then no pull-up flag in the dts for default.. Thank you for explaining that the purpose of the pull-up DT property was for PHY debug. I will plan to remove the pull-up DT property in the next version of this patch series and remove sdhci_phy_1_8v_init_no_pull() and sdhci_phy_3_3v_init_no_pull() from my sdhci-of-dwcmshc.c patch. I will make th1520_phy_1_8v_init() and th1520_phy_3_3v_init() always set WEAKPULL_EN = 1 (PULLUP) for CMDPAD_CNFG, DATAPAD_CNFG, RSTNPAD_CNFG and set WEAKPULL_EN = 2 (PULLDOWN) for STBPAD_CNFG. Thanks, Drew
diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml index a43eb837f8da..46b768d46712 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,9 @@ properties: description: Specify the number of delay for tx sampling. $ref: /schemas/types.yaml#/definitions/uint8 + thead,phy-pull-up: + description: Enable weak pull-up on PHY pads + type: boolean required: - compatible
Add compatible value for the T-Head TH1520 dwcmshc controller and add thead,phy-pull-up property. Signed-off-by: Drew Fustini <dfustini@baylibre.com> --- Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 ++++ 1 file changed, 4 insertions(+)