diff mbox series

[PATCHv2,09/21] mmc: sdhci-of-dwcmshc: add reset call back for rockchip Socs

Message ID 20220504213251.264819-10-sebastian.reichel@collabora.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Basic RK3588 Support | expand

Commit Message

Sebastian Reichel May 4, 2022, 9:32 p.m. UTC
From: Yifeng Zhao <yifeng.zhao@rock-chips.com>

The reset function build in the SDHCI will not reset the logic
circuit related to the tuning function, which may cause data
reading errors. Resetting the complete SDHCI controller through
the reset controller fixes the issue.

Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
[rebase, use optional variant of reset getter]
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Ulf Hansson May 6, 2022, 8:52 a.m. UTC | #1
On Wed, 4 May 2022 at 23:33, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> From: Yifeng Zhao <yifeng.zhao@rock-chips.com>
>
> The reset function build in the SDHCI will not reset the logic
> circuit related to the tuning function, which may cause data
> reading errors. Resetting the complete SDHCI controller through
> the reset controller fixes the issue.
>
> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> [rebase, use optional variant of reset getter]
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

I think this needs a corresponding update of the DT docs. Otherwise
this looks good to me.

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index bac874ab0b33..3a1b5ba36405 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/reset.h>
>  #include <linux/sizes.h>
>
>  #include "sdhci-pltfm.h"
> @@ -63,6 +64,7 @@
>  struct rk3568_priv {
>         /* Rockchip specified optional clocks */
>         struct clk_bulk_data rockchip_clks[RK3568_MAX_CLKS];
> +       struct reset_control *reset;
>         u8 txclk_tapnum;
>  };
>
> @@ -255,6 +257,21 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>         sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>  }
>
> +static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> +       struct rk35xx_priv *priv = dwc_priv->priv;
> +
> +       if (mask & SDHCI_RESET_ALL && priv->reset) {
> +               reset_control_assert(priv->reset);
> +               udelay(1);
> +               reset_control_deassert(priv->reset);
> +       }
> +
> +       sdhci_reset(host, mask);
> +}
> +
>  static const struct sdhci_ops sdhci_dwcmshc_ops = {
>         .set_clock              = sdhci_set_clock,
>         .set_bus_width          = sdhci_set_bus_width,
> @@ -269,7 +286,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk3568_ops = {
>         .set_bus_width          = sdhci_set_bus_width,
>         .set_uhs_signaling      = dwcmshc_set_uhs_signaling,
>         .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
> -       .reset                  = sdhci_reset,
> +       .reset                  = rk35xx_sdhci_reset,
>         .adma_write_desc        = dwcmshc_adma_write_desc,
>  };
>
> @@ -292,6 +309,13 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
>         int err;
>         struct rk3568_priv *priv = dwc_priv->priv;
>
> +       priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> +       if (IS_ERR(priv->reset)) {
> +               err = PTR_ERR(priv->reset);
> +               dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> +               return err;
> +       }
> +
>         priv->rockchip_clks[0].id = "axi";
>         priv->rockchip_clks[1].id = "block";
>         priv->rockchip_clks[2].id = "timer";
> --
> 2.35.1
>
Sebastian Reichel May 6, 2022, 9:18 a.m. UTC | #2
Hi,

On Fri, May 06, 2022 at 10:52:42AM +0200, Ulf Hansson wrote:
> On Wed, 4 May 2022 at 23:33, Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > From: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> >
> > The reset function build in the SDHCI will not reset the logic
> > circuit related to the tuning function, which may cause data
> > reading errors. Resetting the complete SDHCI controller through
> > the reset controller fixes the issue.
> >
> > Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> > [rebase, use optional variant of reset getter]
> > Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> I think this needs a corresponding update of the DT docs. Otherwise
> this looks good to me.

I do have 'resets' and 'reset-names' properties in the rk3588s.dtsi
for the sdhci interface and 'make dtbs_check' did not complain about
anything but missing 'arm,sdei-1.0' compatible for the rk3588 EVB
(sdei binding has not yet been converted to yaml). Thus I assume the
resets property is inferred from somewhere?

-- Sebastian

> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index bac874ab0b33..3a1b5ba36405 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/reset.h>
> >  #include <linux/sizes.h>
> >
> >  #include "sdhci-pltfm.h"
> > @@ -63,6 +64,7 @@
> >  struct rk3568_priv {
> >         /* Rockchip specified optional clocks */
> >         struct clk_bulk_data rockchip_clks[RK3568_MAX_CLKS];
> > +       struct reset_control *reset;
> >         u8 txclk_tapnum;
> >  };
> >
> > @@ -255,6 +257,21 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> >         sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
> >  }
> >
> > +static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +       struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > +       struct rk35xx_priv *priv = dwc_priv->priv;
> > +
> > +       if (mask & SDHCI_RESET_ALL && priv->reset) {
> > +               reset_control_assert(priv->reset);
> > +               udelay(1);
> > +               reset_control_deassert(priv->reset);
> > +       }
> > +
> > +       sdhci_reset(host, mask);
> > +}
> > +
> >  static const struct sdhci_ops sdhci_dwcmshc_ops = {
> >         .set_clock              = sdhci_set_clock,
> >         .set_bus_width          = sdhci_set_bus_width,
> > @@ -269,7 +286,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk3568_ops = {
> >         .set_bus_width          = sdhci_set_bus_width,
> >         .set_uhs_signaling      = dwcmshc_set_uhs_signaling,
> >         .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
> > -       .reset                  = sdhci_reset,
> > +       .reset                  = rk35xx_sdhci_reset,
> >         .adma_write_desc        = dwcmshc_adma_write_desc,
> >  };
> >
> > @@ -292,6 +309,13 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
> >         int err;
> >         struct rk3568_priv *priv = dwc_priv->priv;
> >
> > +       priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> > +       if (IS_ERR(priv->reset)) {
> > +               err = PTR_ERR(priv->reset);
> > +               dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> > +               return err;
> > +       }
> > +
> >         priv->rockchip_clks[0].id = "axi";
> >         priv->rockchip_clks[1].id = "block";
> >         priv->rockchip_clks[2].id = "timer";
> > --
> > 2.35.1
> >
Ulf Hansson May 6, 2022, 12:37 p.m. UTC | #3
On Fri, 6 May 2022 at 11:18, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Fri, May 06, 2022 at 10:52:42AM +0200, Ulf Hansson wrote:
> > On Wed, 4 May 2022 at 23:33, Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > >
> > > From: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> > >
> > > The reset function build in the SDHCI will not reset the logic
> > > circuit related to the tuning function, which may cause data
> > > reading errors. Resetting the complete SDHCI controller through
> > > the reset controller fixes the issue.
> > >
> > > Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> > > [rebase, use optional variant of reset getter]
> > > Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> >
> > I think this needs a corresponding update of the DT docs. Otherwise
> > this looks good to me.
>
> I do have 'resets' and 'reset-names' properties in the rk3588s.dtsi
> for the sdhci interface and 'make dtbs_check' did not complain about
> anything but missing 'arm,sdei-1.0' compatible for the rk3588 EVB
> (sdei binding has not yet been converted to yaml). Thus I assume the
> resets property is inferred from somewhere?

I don't think it should, but I may be wrong.

How about if you extend the example in the DT doc with a reset
property, will that cause the DT tools to complain?

Kind regards
Uffe

>
> -- Sebastian
>
> >
> > Kind regards
> > Uffe
> >
> > > ---
> > >  drivers/mmc/host/sdhci-of-dwcmshc.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > index bac874ab0b33..3a1b5ba36405 100644
> > > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/reset.h>
> > >  #include <linux/sizes.h>
> > >
> > >  #include "sdhci-pltfm.h"
> > > @@ -63,6 +64,7 @@
> > >  struct rk3568_priv {
> > >         /* Rockchip specified optional clocks */
> > >         struct clk_bulk_data rockchip_clks[RK3568_MAX_CLKS];
> > > +       struct reset_control *reset;
> > >         u8 txclk_tapnum;
> > >  };
> > >
> > > @@ -255,6 +257,21 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> > >         sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
> > >  }
> > >
> > > +static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> > > +{
> > > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +       struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > > +       struct rk35xx_priv *priv = dwc_priv->priv;
> > > +
> > > +       if (mask & SDHCI_RESET_ALL && priv->reset) {
> > > +               reset_control_assert(priv->reset);
> > > +               udelay(1);
> > > +               reset_control_deassert(priv->reset);
> > > +       }
> > > +
> > > +       sdhci_reset(host, mask);
> > > +}
> > > +
> > >  static const struct sdhci_ops sdhci_dwcmshc_ops = {
> > >         .set_clock              = sdhci_set_clock,
> > >         .set_bus_width          = sdhci_set_bus_width,
> > > @@ -269,7 +286,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk3568_ops = {
> > >         .set_bus_width          = sdhci_set_bus_width,
> > >         .set_uhs_signaling      = dwcmshc_set_uhs_signaling,
> > >         .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
> > > -       .reset                  = sdhci_reset,
> > > +       .reset                  = rk35xx_sdhci_reset,
> > >         .adma_write_desc        = dwcmshc_adma_write_desc,
> > >  };
> > >
> > > @@ -292,6 +309,13 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
> > >         int err;
> > >         struct rk3568_priv *priv = dwc_priv->priv;
> > >
> > > +       priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> > > +       if (IS_ERR(priv->reset)) {
> > > +               err = PTR_ERR(priv->reset);
> > > +               dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> > > +               return err;
> > > +       }
> > > +
> > >         priv->rockchip_clks[0].id = "axi";
> > >         priv->rockchip_clks[1].id = "block";
> > >         priv->rockchip_clks[2].id = "timer";
> > > --
> > > 2.35.1
> > >
Ulf Hansson May 31, 2022, 1:38 p.m. UTC | #4
On Wed, 4 May 2022 at 23:33, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> From: Yifeng Zhao <yifeng.zhao@rock-chips.com>
>
> The reset function build in the SDHCI will not reset the logic
> circuit related to the tuning function, which may cause data
> reading errors. Resetting the complete SDHCI controller through
> the reset controller fixes the issue.
>
> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> [rebase, use optional variant of reset getter]
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Queued for v5.20 on the devel branch, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index bac874ab0b33..3a1b5ba36405 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/reset.h>
>  #include <linux/sizes.h>
>
>  #include "sdhci-pltfm.h"
> @@ -63,6 +64,7 @@
>  struct rk3568_priv {
>         /* Rockchip specified optional clocks */
>         struct clk_bulk_data rockchip_clks[RK3568_MAX_CLKS];
> +       struct reset_control *reset;
>         u8 txclk_tapnum;
>  };
>
> @@ -255,6 +257,21 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>         sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>  }
>
> +static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> +       struct rk35xx_priv *priv = dwc_priv->priv;
> +
> +       if (mask & SDHCI_RESET_ALL && priv->reset) {
> +               reset_control_assert(priv->reset);
> +               udelay(1);
> +               reset_control_deassert(priv->reset);
> +       }
> +
> +       sdhci_reset(host, mask);
> +}
> +
>  static const struct sdhci_ops sdhci_dwcmshc_ops = {
>         .set_clock              = sdhci_set_clock,
>         .set_bus_width          = sdhci_set_bus_width,
> @@ -269,7 +286,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk3568_ops = {
>         .set_bus_width          = sdhci_set_bus_width,
>         .set_uhs_signaling      = dwcmshc_set_uhs_signaling,
>         .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
> -       .reset                  = sdhci_reset,
> +       .reset                  = rk35xx_sdhci_reset,
>         .adma_write_desc        = dwcmshc_adma_write_desc,
>  };
>
> @@ -292,6 +309,13 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
>         int err;
>         struct rk3568_priv *priv = dwc_priv->priv;
>
> +       priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> +       if (IS_ERR(priv->reset)) {
> +               err = PTR_ERR(priv->reset);
> +               dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> +               return err;
> +       }
> +
>         priv->rockchip_clks[0].id = "axi";
>         priv->rockchip_clks[1].id = "block";
>         priv->rockchip_clks[2].id = "timer";
> --
> 2.35.1
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index bac874ab0b33..3a1b5ba36405 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -15,6 +15,7 @@ 
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/reset.h>
 #include <linux/sizes.h>
 
 #include "sdhci-pltfm.h"
@@ -63,6 +64,7 @@ 
 struct rk3568_priv {
 	/* Rockchip specified optional clocks */
 	struct clk_bulk_data rockchip_clks[RK3568_MAX_CLKS];
+	struct reset_control *reset;
 	u8 txclk_tapnum;
 };
 
@@ -255,6 +257,21 @@  static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
 	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
 }
 
+static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
+	struct rk35xx_priv *priv = dwc_priv->priv;
+
+	if (mask & SDHCI_RESET_ALL && priv->reset) {
+		reset_control_assert(priv->reset);
+		udelay(1);
+		reset_control_deassert(priv->reset);
+	}
+
+	sdhci_reset(host, mask);
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
 	.set_clock		= sdhci_set_clock,
 	.set_bus_width		= sdhci_set_bus_width,
@@ -269,7 +286,7 @@  static const struct sdhci_ops sdhci_dwcmshc_rk3568_ops = {
 	.set_bus_width		= sdhci_set_bus_width,
 	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
 	.get_max_clock		= sdhci_pltfm_clk_get_max_clock,
-	.reset			= sdhci_reset,
+	.reset			= rk35xx_sdhci_reset,
 	.adma_write_desc	= dwcmshc_adma_write_desc,
 };
 
@@ -292,6 +309,13 @@  static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
 	int err;
 	struct rk3568_priv *priv = dwc_priv->priv;
 
+	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
+	if (IS_ERR(priv->reset)) {
+		err = PTR_ERR(priv->reset);
+		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
+		return err;
+	}
+
 	priv->rockchip_clks[0].id = "axi";
 	priv->rockchip_clks[1].id = "block";
 	priv->rockchip_clks[2].id = "timer";