LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
@ 2019-07-17  2:28 Baolin Wang
  2019-07-17  6:07 ` Adrian Hunter
  2019-07-22 11:54 ` Ulf Hansson
  0 siblings, 2 replies; 10+ messages in thread
From: Baolin Wang @ 2019-07-17  2:28 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, zhang.lyra, orsonzhai
  Cc: baolin.wang, vincent.guittot, linux-mmc, linux-kernel

In sdhci_runtime_resume_host() function, we will always do software reset
for all, which will cause Spreadtrum host controller work abnormally after
resuming.

Thus for Spreadtrum platform that will not power down the SD/eMMC card during
runtime suspend, we should not do software reset for all. To fix this
issue, adding a specific reset operation that adds one condition to validate
the power mode to decide if we can do software reset for all or just reset
command and data lines.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changess from v3:
 - Use ios.power_mode to validate if the card is power down or not.

Changes from v2:
 - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().

Changes from v1:
 - Add a specific reset operation instead of changing the core to avoid
 affecting other hardware.
---
 drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
index 603a5d9..94f9726 100644
--- a/drivers/mmc/host/sdhci-sprd.c
+++ b/drivers/mmc/host/sdhci-sprd.c
@@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
 	return 1 << 31;
 }
 
+static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	/*
+	 * When try to reset controller after runtime suspend, we should not
+	 * reset for all if the SD/eMMC card is not power down, just reset
+	 * command and data lines instead. Otherwise will meet some strange
+	 * behaviors for Spreadtrum host controller.
+	 */
+	if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
+	    mmc->ios.power_mode == MMC_POWER_ON)
+		mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
+
+	sdhci_reset(host, mask);
+}
+
 static struct sdhci_ops sdhci_sprd_ops = {
 	.read_l = sdhci_sprd_readl,
 	.write_l = sdhci_sprd_writel,
@@ -381,7 +398,7 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
 	.get_max_clock = sdhci_sprd_get_max_clock,
 	.get_min_clock = sdhci_sprd_get_min_clock,
 	.set_bus_width = sdhci_set_bus_width,
-	.reset = sdhci_reset,
+	.reset = sdhci_sprd_reset,
 	.set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
 	.hw_reset = sdhci_sprd_hw_reset,
 	.get_max_timeout_count = sdhci_sprd_get_max_timeout_count,
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-17  2:28 [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming Baolin Wang
@ 2019-07-17  6:07 ` Adrian Hunter
  2019-07-22 11:54 ` Ulf Hansson
  1 sibling, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2019-07-17  6:07 UTC (permalink / raw)
  To: Baolin Wang, ulf.hansson, zhang.lyra, orsonzhai
  Cc: vincent.guittot, linux-mmc, linux-kernel

On 17/07/19 5:28 AM, Baolin Wang wrote:
> In sdhci_runtime_resume_host() function, we will always do software reset
> for all, which will cause Spreadtrum host controller work abnormally after
> resuming.
> 
> Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> runtime suspend, we should not do software reset for all. To fix this
> issue, adding a specific reset operation that adds one condition to validate
> the power mode to decide if we can do software reset for all or just reset
> command and data lines.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> Changess from v3:
>  - Use ios.power_mode to validate if the card is power down or not.
> 
> Changes from v2:
>  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> 
> Changes from v1:
>  - Add a specific reset operation instead of changing the core to avoid
>  affecting other hardware.
> ---
>  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> index 603a5d9..94f9726 100644
> --- a/drivers/mmc/host/sdhci-sprd.c
> +++ b/drivers/mmc/host/sdhci-sprd.c
> @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
>  	return 1 << 31;
>  }
>  
> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> +{
> +	struct mmc_host *mmc = host->mmc;
> +
> +	/*
> +	 * When try to reset controller after runtime suspend, we should not
> +	 * reset for all if the SD/eMMC card is not power down, just reset
> +	 * command and data lines instead. Otherwise will meet some strange
> +	 * behaviors for Spreadtrum host controller.
> +	 */
> +	if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> +	    mmc->ios.power_mode == MMC_POWER_ON)
> +		mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> +
> +	sdhci_reset(host, mask);
> +}
> +
>  static struct sdhci_ops sdhci_sprd_ops = {
>  	.read_l = sdhci_sprd_readl,
>  	.write_l = sdhci_sprd_writel,
> @@ -381,7 +398,7 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
>  	.get_max_clock = sdhci_sprd_get_max_clock,
>  	.get_min_clock = sdhci_sprd_get_min_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> -	.reset = sdhci_reset,
> +	.reset = sdhci_sprd_reset,
>  	.set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
>  	.hw_reset = sdhci_sprd_hw_reset,
>  	.get_max_timeout_count = sdhci_sprd_get_max_timeout_count,
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-17  2:28 [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming Baolin Wang
  2019-07-17  6:07 ` Adrian Hunter
@ 2019-07-22 11:54 ` Ulf Hansson
  2019-07-23  3:05   ` Baolin Wang
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2019-07-22 11:54 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Adrian Hunter, Chunyan Zhang, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
>
> In sdhci_runtime_resume_host() function, we will always do software reset
> for all, which will cause Spreadtrum host controller work abnormally after
> resuming.

What does "software reset for all" means?

>
> Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> runtime suspend, we should not do software reset for all.

Normally, sdhci hosts that enters runtime suspend doesn't power off
the card (there are some exceptions like PCI variants).

So, what's so special here and how does the reset come into play? I
don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
and nor doesn the callback from the sdhci-sprd.c variant doing it.

> To fix this
> issue, adding a specific reset operation that adds one condition to validate
> the power mode to decide if we can do software reset for all or just reset
> command and data lines.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changess from v3:
>  - Use ios.power_mode to validate if the card is power down or not.
>
> Changes from v2:
>  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
>
> Changes from v1:
>  - Add a specific reset operation instead of changing the core to avoid
>  affecting other hardware.
> ---
>  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> index 603a5d9..94f9726 100644
> --- a/drivers/mmc/host/sdhci-sprd.c
> +++ b/drivers/mmc/host/sdhci-sprd.c
> @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
>         return 1 << 31;
>  }
>
> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct mmc_host *mmc = host->mmc;
> +
> +       /*
> +        * When try to reset controller after runtime suspend, we should not
> +        * reset for all if the SD/eMMC card is not power down, just reset
> +        * command and data lines instead. Otherwise will meet some strange
> +        * behaviors for Spreadtrum host controller.
> +        */
> +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> +           mmc->ios.power_mode == MMC_POWER_ON)
> +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;

Can sdhci_sprd_reset() be called when the host is runtime suspended?
That sounds like a bug to me, no?

> +
> +       sdhci_reset(host, mask);
> +}
> +
>  static struct sdhci_ops sdhci_sprd_ops = {
>         .read_l = sdhci_sprd_readl,
>         .write_l = sdhci_sprd_writel,
> @@ -381,7 +398,7 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
>         .get_max_clock = sdhci_sprd_get_max_clock,
>         .get_min_clock = sdhci_sprd_get_min_clock,
>         .set_bus_width = sdhci_set_bus_width,
> -       .reset = sdhci_reset,
> +       .reset = sdhci_sprd_reset,
>         .set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
>         .hw_reset = sdhci_sprd_hw_reset,
>         .get_max_timeout_count = sdhci_sprd_get_max_timeout_count,
> --
> 1.7.9.5
>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-22 11:54 ` Ulf Hansson
@ 2019-07-23  3:05   ` Baolin Wang
  2019-07-23  3:21     ` Chunyan Zhang
  2019-07-23 12:38     ` Ulf Hansson
  0 siblings, 2 replies; 10+ messages in thread
From: Baolin Wang @ 2019-07-23  3:05 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Adrian Hunter, Chunyan Zhang, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

Hi Ulf,

On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> > In sdhci_runtime_resume_host() function, we will always do software reset
> > for all, which will cause Spreadtrum host controller work abnormally after
> > resuming.
>
> What does "software reset for all" means?

The SD host controller specification defines 3 types software reset:
software reset for data line, software reset for command line and
software reset for all.
Software reset for all means this reset affects the entire Host
controller except for the card detection circuit.

>
> >
> > Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> > runtime suspend, we should not do software reset for all.
>
> Normally, sdhci hosts that enters runtime suspend doesn't power off
> the card (there are some exceptions like PCI variants).

Yes, same as our controller.

>
> So, what's so special here and how does the reset come into play? I
> don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> and nor doesn the callback from the sdhci-sprd.c variant doing it.

In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
issue software reset for all.

>
> > To fix this
> > issue, adding a specific reset operation that adds one condition to validate
> > the power mode to decide if we can do software reset for all or just reset
> > command and data lines.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > Changess from v3:
> >  - Use ios.power_mode to validate if the card is power down or not.
> >
> > Changes from v2:
> >  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> >
> > Changes from v1:
> >  - Add a specific reset operation instead of changing the core to avoid
> >  affecting other hardware.
> > ---
> >  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> > index 603a5d9..94f9726 100644
> > --- a/drivers/mmc/host/sdhci-sprd.c
> > +++ b/drivers/mmc/host/sdhci-sprd.c
> > @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> >         return 1 << 31;
> >  }
> >
> > +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +       struct mmc_host *mmc = host->mmc;
> > +
> > +       /*
> > +        * When try to reset controller after runtime suspend, we should not
> > +        * reset for all if the SD/eMMC card is not power down, just reset
> > +        * command and data lines instead. Otherwise will meet some strange
> > +        * behaviors for Spreadtrum host controller.
> > +        */
> > +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> > +           mmc->ios.power_mode == MMC_POWER_ON)
> > +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
>
> Can sdhci_sprd_reset() be called when the host is runtime suspended?

When host tries to runtime resume in sdhci_runtime_resume_host(), it
will call reset operation to do software reset.

> That sounds like a bug to me, no?

Since our controller will meet some strange behaviors if we do
software reset for all in sdhci_runtime_resume_host(), and try to
avoid changing the core logic of sdhci_runtime_resume_host() used by
other hardware controllers, thus I introduced a specific reset ops and
added some condition to make sure we just do software reset command
and data lines from runtime suspend state.

>
> > +
> > +       sdhci_reset(host, mask);
> > +}
> > +
> >  static struct sdhci_ops sdhci_sprd_ops = {
> >         .read_l = sdhci_sprd_readl,
> >         .write_l = sdhci_sprd_writel,
> > @@ -381,7 +398,7 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> >         .get_max_clock = sdhci_sprd_get_max_clock,
> >         .get_min_clock = sdhci_sprd_get_min_clock,
> >         .set_bus_width = sdhci_set_bus_width,
> > -       .reset = sdhci_reset,
> > +       .reset = sdhci_sprd_reset,
> >         .set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
> >         .hw_reset = sdhci_sprd_hw_reset,
> >         .get_max_timeout_count = sdhci_sprd_get_max_timeout_count,
> > --
> > 1.7.9.5
> >
>
> Kind regards
> Uffe



-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-23  3:05   ` Baolin Wang
@ 2019-07-23  3:21     ` Chunyan Zhang
  2019-07-23  3:30       ` Baolin Wang
  2019-07-23 12:38     ` Ulf Hansson
  1 sibling, 1 reply; 10+ messages in thread
From: Chunyan Zhang @ 2019-07-23  3:21 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Ulf Hansson, Adrian Hunter, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Tue, 23 Jul 2019 at 11:05, Baolin Wang <baolin.wang@linaro.org> wrote:
>
> Hi Ulf,
>
> On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> > >
> > > In sdhci_runtime_resume_host() function, we will always do software reset
> > > for all, which will cause Spreadtrum host controller work abnormally after
> > > resuming.
> >
> > What does "software reset for all" means?
>
> The SD host controller specification defines 3 types software reset:
> software reset for data line, software reset for command line and
> software reset for all.
> Software reset for all means this reset affects the entire Host
> controller except for the card detection circuit.
>
> >
> > >
> > > Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> > > runtime suspend, we should not do software reset for all.
> >
> > Normally, sdhci hosts that enters runtime suspend doesn't power off
> > the card (there are some exceptions like PCI variants).
>
> Yes, same as our controller.
>
> >
> > So, what's so special here and how does the reset come into play? I
> > don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> > and nor doesn the callback from the sdhci-sprd.c variant doing it.
>
> In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
> issue software reset for all.
>
> >
> > > To fix this
> > > issue, adding a specific reset operation that adds one condition to validate
> > > the power mode to decide if we can do software reset for all or just reset
> > > command and data lines.
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > Changess from v3:
> > >  - Use ios.power_mode to validate if the card is power down or not.
> > >
> > > Changes from v2:
> > >  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> > >
> > > Changes from v1:
> > >  - Add a specific reset operation instead of changing the core to avoid
> > >  affecting other hardware.
> > > ---
> > >  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> > > index 603a5d9..94f9726 100644
> > > --- a/drivers/mmc/host/sdhci-sprd.c
> > > +++ b/drivers/mmc/host/sdhci-sprd.c
> > > @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> > >         return 1 << 31;
> > >  }
> > >
> > > +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> > > +{
> > > +       struct mmc_host *mmc = host->mmc;
> > > +
> > > +       /*
> > > +        * When try to reset controller after runtime suspend, we should not
> > > +        * reset for all if the SD/eMMC card is not power down, just reset
> > > +        * command and data lines instead. Otherwise will meet some strange
> > > +        * behaviors for Spreadtrum host controller.
> > > +        */
> > > +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> > > +           mmc->ios.power_mode == MMC_POWER_ON)
> > > +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> >
> > Can sdhci_sprd_reset() be called when the host is runtime suspended?
>
> When host tries to runtime resume in sdhci_runtime_resume_host(), it
> will call reset operation to do software reset.
>
> > That sounds like a bug to me, no?
>
> Since our controller will meet some strange behaviors if we do
> software reset for all in sdhci_runtime_resume_host(), and try to
> avoid changing the core logic of sdhci_runtime_resume_host() used by
> other hardware controllers, thus I introduced a specific reset ops and
> added some condition to make sure we just do software reset command
> and data lines from runtime suspend state.

I can make a verification on sprd's SC9863A, but that would take a
little time, since I need to make sd card registered with sdhci-sprd.c
first :)

Thanks,
Chunyan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-23  3:21     ` Chunyan Zhang
@ 2019-07-23  3:30       ` Baolin Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Baolin Wang @ 2019-07-23  3:30 UTC (permalink / raw)
  To: Chunyan Zhang
  Cc: Ulf Hansson, Adrian Hunter, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Tue, 23 Jul 2019 at 11:21, Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Tue, 23 Jul 2019 at 11:05, Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> > Hi Ulf,
> >
> > On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> > > >
> > > > In sdhci_runtime_resume_host() function, we will always do software reset
> > > > for all, which will cause Spreadtrum host controller work abnormally after
> > > > resuming.
> > >
> > > What does "software reset for all" means?
> >
> > The SD host controller specification defines 3 types software reset:
> > software reset for data line, software reset for command line and
> > software reset for all.
> > Software reset for all means this reset affects the entire Host
> > controller except for the card detection circuit.
> >
> > >
> > > >
> > > > Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> > > > runtime suspend, we should not do software reset for all.
> > >
> > > Normally, sdhci hosts that enters runtime suspend doesn't power off
> > > the card (there are some exceptions like PCI variants).
> >
> > Yes, same as our controller.
> >
> > >
> > > So, what's so special here and how does the reset come into play? I
> > > don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> > > and nor doesn the callback from the sdhci-sprd.c variant doing it.
> >
> > In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
> > issue software reset for all.
> >
> > >
> > > > To fix this
> > > > issue, adding a specific reset operation that adds one condition to validate
> > > > the power mode to decide if we can do software reset for all or just reset
> > > > command and data lines.
> > > >
> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > ---
> > > > Changess from v3:
> > > >  - Use ios.power_mode to validate if the card is power down or not.
> > > >
> > > > Changes from v2:
> > > >  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> > > >
> > > > Changes from v1:
> > > >  - Add a specific reset operation instead of changing the core to avoid
> > > >  affecting other hardware.
> > > > ---
> > > >  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> > > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> > > > index 603a5d9..94f9726 100644
> > > > --- a/drivers/mmc/host/sdhci-sprd.c
> > > > +++ b/drivers/mmc/host/sdhci-sprd.c
> > > > @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> > > >         return 1 << 31;
> > > >  }
> > > >
> > > > +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> > > > +{
> > > > +       struct mmc_host *mmc = host->mmc;
> > > > +
> > > > +       /*
> > > > +        * When try to reset controller after runtime suspend, we should not
> > > > +        * reset for all if the SD/eMMC card is not power down, just reset
> > > > +        * command and data lines instead. Otherwise will meet some strange
> > > > +        * behaviors for Spreadtrum host controller.
> > > > +        */
> > > > +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> > > > +           mmc->ios.power_mode == MMC_POWER_ON)
> > > > +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> > >
> > > Can sdhci_sprd_reset() be called when the host is runtime suspended?
> >
> > When host tries to runtime resume in sdhci_runtime_resume_host(), it
> > will call reset operation to do software reset.
> >
> > > That sounds like a bug to me, no?
> >
> > Since our controller will meet some strange behaviors if we do
> > software reset for all in sdhci_runtime_resume_host(), and try to
> > avoid changing the core logic of sdhci_runtime_resume_host() used by
> > other hardware controllers, thus I introduced a specific reset ops and
> > added some condition to make sure we just do software reset command
> > and data lines from runtime suspend state.
>
> I can make a verification on sprd's SC9863A, but that would take a
> little time, since I need to make sd card registered with sdhci-sprd.c
> first :)

Great, you can try it on your board. Thanks.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-23  3:05   ` Baolin Wang
  2019-07-23  3:21     ` Chunyan Zhang
@ 2019-07-23 12:38     ` Ulf Hansson
  2019-07-24  2:21       ` Baolin Wang
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2019-07-23 12:38 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Adrian Hunter, Chunyan Zhang, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Tue, 23 Jul 2019 at 05:05, Baolin Wang <baolin.wang@linaro.org> wrote:
>
> Hi Ulf,
>
> On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> > >
> > > In sdhci_runtime_resume_host() function, we will always do software reset
> > > for all, which will cause Spreadtrum host controller work abnormally after
> > > resuming.
> >
> > What does "software reset for all" means?
>
> The SD host controller specification defines 3 types software reset:
> software reset for data line, software reset for command line and
> software reset for all.
> Software reset for all means this reset affects the entire Host
> controller except for the card detection circuit.

Thanks for clarifying, please update the changelog accordingly.

>
> >
> > >
> > > Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> > > runtime suspend, we should not do software reset for all.
> >
> > Normally, sdhci hosts that enters runtime suspend doesn't power off
> > the card (there are some exceptions like PCI variants).
>
> Yes, same as our controller.
>
> >
> > So, what's so special here and how does the reset come into play? I
> > don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> > and nor doesn the callback from the sdhci-sprd.c variant doing it.
>
> In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
> issue software reset for all.

Aha, I didn't read the code carefully enough. Apologize for the noise.

>
> >
> > > To fix this
> > > issue, adding a specific reset operation that adds one condition to validate
> > > the power mode to decide if we can do software reset for all or just reset
> > > command and data lines.
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > Changess from v3:
> > >  - Use ios.power_mode to validate if the card is power down or not.
> > >
> > > Changes from v2:
> > >  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> > >
> > > Changes from v1:
> > >  - Add a specific reset operation instead of changing the core to avoid
> > >  affecting other hardware.
> > > ---
> > >  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> > > index 603a5d9..94f9726 100644
> > > --- a/drivers/mmc/host/sdhci-sprd.c
> > > +++ b/drivers/mmc/host/sdhci-sprd.c
> > > @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> > >         return 1 << 31;
> > >  }
> > >
> > > +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> > > +{
> > > +       struct mmc_host *mmc = host->mmc;
> > > +
> > > +       /*
> > > +        * When try to reset controller after runtime suspend, we should not
> > > +        * reset for all if the SD/eMMC card is not power down, just reset
> > > +        * command and data lines instead. Otherwise will meet some strange
> > > +        * behaviors for Spreadtrum host controller.
> > > +        */
> > > +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> > > +           mmc->ios.power_mode == MMC_POWER_ON)
> > > +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> >
> > Can sdhci_sprd_reset() be called when the host is runtime suspended?
>
> When host tries to runtime resume in sdhci_runtime_resume_host(), it
> will call reset operation to do software reset.

Right, I see that now, thanks for clarifying.

However, there are still some weird things going on in
sdhci_runtime_resume_host(). Like why is host->ops->enable_dma()
called first, directly from sdhci_runtime_resume_host(), then again in
sdhci_do_reset(), after host->ops->reset() has been called. Looks like
the first call to ->enable_dma() doesn't make sense?

>
> > That sounds like a bug to me, no?
>
> Since our controller will meet some strange behaviors if we do
> software reset for all in sdhci_runtime_resume_host(), and try to
> avoid changing the core logic of sdhci_runtime_resume_host() used by
> other hardware controllers, thus I introduced a specific reset ops and
> added some condition to make sure we just do software reset command
> and data lines from runtime suspend state.

I understand, but perhaps it would become more clear if
sdhci_runtime_resume_host() is re-factored a bit. Maybe the caller can
give it some new parameter to let it decide if a SDHCI_RESET_ALL shall
be done or not.

>
> >
> > > +
> > > +       sdhci_reset(host, mask);
> > > +}
> > > +
> > >  static struct sdhci_ops sdhci_sprd_ops = {
> > >         .read_l = sdhci_sprd_readl,
> > >         .write_l = sdhci_sprd_writel,
> > > @@ -381,7 +398,7 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> > >         .get_max_clock = sdhci_sprd_get_max_clock,
> > >         .get_min_clock = sdhci_sprd_get_min_clock,
> > >         .set_bus_width = sdhci_set_bus_width,
> > > -       .reset = sdhci_reset,
> > > +       .reset = sdhci_sprd_reset,
> > >         .set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
> > >         .hw_reset = sdhci_sprd_hw_reset,
> > >         .get_max_timeout_count = sdhci_sprd_get_max_timeout_count,
> > > --
> > > 1.7.9.5
> > >

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-23 12:38     ` Ulf Hansson
@ 2019-07-24  2:21       ` Baolin Wang
  2019-07-24 12:55         ` Adrian Hunter
  0 siblings, 1 reply; 10+ messages in thread
From: Baolin Wang @ 2019-07-24  2:21 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Adrian Hunter, Chunyan Zhang, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Tue, 23 Jul 2019 at 20:39, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 23 Jul 2019 at 05:05, Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> > Hi Ulf,
> >
> > On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> > > >
> > > > In sdhci_runtime_resume_host() function, we will always do software reset
> > > > for all, which will cause Spreadtrum host controller work abnormally after
> > > > resuming.
> > >
> > > What does "software reset for all" means?
> >
> > The SD host controller specification defines 3 types software reset:
> > software reset for data line, software reset for command line and
> > software reset for all.
> > Software reset for all means this reset affects the entire Host
> > controller except for the card detection circuit.
>
> Thanks for clarifying, please update the changelog accordingly.

Sure, sorry for confusing.

>
> >
> > >
> > > >
> > > > Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> > > > runtime suspend, we should not do software reset for all.
> > >
> > > Normally, sdhci hosts that enters runtime suspend doesn't power off
> > > the card (there are some exceptions like PCI variants).
> >
> > Yes, same as our controller.
> >
> > >
> > > So, what's so special here and how does the reset come into play? I
> > > don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> > > and nor doesn the callback from the sdhci-sprd.c variant doing it.
> >
> > In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
> > issue software reset for all.
>
> Aha, I didn't read the code carefully enough. Apologize for the noise.

No worries :)

> >
> > >
> > > > To fix this
> > > > issue, adding a specific reset operation that adds one condition to validate
> > > > the power mode to decide if we can do software reset for all or just reset
> > > > command and data lines.
> > > >
> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > ---
> > > > Changess from v3:
> > > >  - Use ios.power_mode to validate if the card is power down or not.
> > > >
> > > > Changes from v2:
> > > >  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> > > >
> > > > Changes from v1:
> > > >  - Add a specific reset operation instead of changing the core to avoid
> > > >  affecting other hardware.
> > > > ---
> > > >  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> > > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> > > > index 603a5d9..94f9726 100644
> > > > --- a/drivers/mmc/host/sdhci-sprd.c
> > > > +++ b/drivers/mmc/host/sdhci-sprd.c
> > > > @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> > > >         return 1 << 31;
> > > >  }
> > > >
> > > > +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> > > > +{
> > > > +       struct mmc_host *mmc = host->mmc;
> > > > +
> > > > +       /*
> > > > +        * When try to reset controller after runtime suspend, we should not
> > > > +        * reset for all if the SD/eMMC card is not power down, just reset
> > > > +        * command and data lines instead. Otherwise will meet some strange
> > > > +        * behaviors for Spreadtrum host controller.
> > > > +        */
> > > > +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> > > > +           mmc->ios.power_mode == MMC_POWER_ON)
> > > > +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> > >
> > > Can sdhci_sprd_reset() be called when the host is runtime suspended?
> >
> > When host tries to runtime resume in sdhci_runtime_resume_host(), it
> > will call reset operation to do software reset.
>
> Right, I see that now, thanks for clarifying.
>
> However, there are still some weird things going on in
> sdhci_runtime_resume_host(). Like why is host->ops->enable_dma()
> called first, directly from sdhci_runtime_resume_host(), then again in
> sdhci_do_reset(), after host->ops->reset() has been called. Looks like
> the first call to ->enable_dma() doesn't make sense?

I am mot sure, since our host did not supply enable_dma() operation.
This logic was used by some other hardware and worked well, I am not
sure if it can reveal some issues if we change the logic here.

Adrian, could you help to explain why we put enable_dma() in front of
software reset?

>
> >
> > > That sounds like a bug to me, no?
> >
> > Since our controller will meet some strange behaviors if we do
> > software reset for all in sdhci_runtime_resume_host(), and try to
> > avoid changing the core logic of sdhci_runtime_resume_host() used by
> > other hardware controllers, thus I introduced a specific reset ops and
> > added some condition to make sure we just do software reset command
> > and data lines from runtime suspend state.
>
> I understand, but perhaps it would become more clear if
> sdhci_runtime_resume_host() is re-factored a bit. Maybe the caller can
> give it some new parameter to let it decide if a SDHCI_RESET_ALL shall
> be done or not.

Yes, sounds reasonable, but need change other host drivers which
issued the sdhci_runtime_resume_host().

Adrian, if you also agree with Ulf's suggestion, then I will post new
patches to add a parameter to decide the reset mode. Thanks.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-24  2:21       ` Baolin Wang
@ 2019-07-24 12:55         ` Adrian Hunter
  2019-07-25  3:05           ` Baolin Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2019-07-24 12:55 UTC (permalink / raw)
  To: Baolin Wang, Ulf Hansson
  Cc: Chunyan Zhang, Orson Zhai, Vincent Guittot, linux-mmc,
	Linux Kernel Mailing List

On 24/07/19 5:21 AM, Baolin Wang wrote:
> On Tue, 23 Jul 2019 at 20:39, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Tue, 23 Jul 2019 at 05:05, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>
>>> Hi Ulf,
>>>
>>> On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>>
>>>>> In sdhci_runtime_resume_host() function, we will always do software reset
>>>>> for all, which will cause Spreadtrum host controller work abnormally after
>>>>> resuming.
>>>>
>>>> What does "software reset for all" means?
>>>
>>> The SD host controller specification defines 3 types software reset:
>>> software reset for data line, software reset for command line and
>>> software reset for all.
>>> Software reset for all means this reset affects the entire Host
>>> controller except for the card detection circuit.
>>
>> Thanks for clarifying, please update the changelog accordingly.
> 
> Sure, sorry for confusing.
> 
>>
>>>
>>>>
>>>>>
>>>>> Thus for Spreadtrum platform that will not power down the SD/eMMC card during
>>>>> runtime suspend, we should not do software reset for all.
>>>>
>>>> Normally, sdhci hosts that enters runtime suspend doesn't power off
>>>> the card (there are some exceptions like PCI variants).
>>>
>>> Yes, same as our controller.
>>>
>>>>
>>>> So, what's so special here and how does the reset come into play? I
>>>> don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
>>>> and nor doesn the callback from the sdhci-sprd.c variant doing it.
>>>
>>> In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
>>> issue software reset for all.
>>
>> Aha, I didn't read the code carefully enough. Apologize for the noise.
> 
> No worries :)
> 
>>>
>>>>
>>>>> To fix this
>>>>> issue, adding a specific reset operation that adds one condition to validate
>>>>> the power mode to decide if we can do software reset for all or just reset
>>>>> command and data lines.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changess from v3:
>>>>>  - Use ios.power_mode to validate if the card is power down or not.
>>>>>
>>>>> Changes from v2:
>>>>>  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
>>>>>
>>>>> Changes from v1:
>>>>>  - Add a specific reset operation instead of changing the core to avoid
>>>>>  affecting other hardware.
>>>>> ---
>>>>>  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
>>>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
>>>>> index 603a5d9..94f9726 100644
>>>>> --- a/drivers/mmc/host/sdhci-sprd.c
>>>>> +++ b/drivers/mmc/host/sdhci-sprd.c
>>>>> @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
>>>>>         return 1 << 31;
>>>>>  }
>>>>>
>>>>> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
>>>>> +{
>>>>> +       struct mmc_host *mmc = host->mmc;
>>>>> +
>>>>> +       /*
>>>>> +        * When try to reset controller after runtime suspend, we should not
>>>>> +        * reset for all if the SD/eMMC card is not power down, just reset
>>>>> +        * command and data lines instead. Otherwise will meet some strange
>>>>> +        * behaviors for Spreadtrum host controller.
>>>>> +        */
>>>>> +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
>>>>> +           mmc->ios.power_mode == MMC_POWER_ON)
>>>>> +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
>>>>
>>>> Can sdhci_sprd_reset() be called when the host is runtime suspended?
>>>
>>> When host tries to runtime resume in sdhci_runtime_resume_host(), it
>>> will call reset operation to do software reset.
>>
>> Right, I see that now, thanks for clarifying.
>>
>> However, there are still some weird things going on in
>> sdhci_runtime_resume_host(). Like why is host->ops->enable_dma()
>> called first, directly from sdhci_runtime_resume_host(), then again in
>> sdhci_do_reset(), after host->ops->reset() has been called. Looks like
>> the first call to ->enable_dma() doesn't make sense?
> 
> I am mot sure, since our host did not supply enable_dma() operation.
> This logic was used by some other hardware and worked well, I am not
> sure if it can reveal some issues if we change the logic here.
> 
> Adrian, could you help to explain why we put enable_dma() in front of
> software reset?

No reason I can see.  But if you add a parameter to avoid a full reset, then
the ->enable_dma will be needed in that case.

> 
>>
>>>
>>>> That sounds like a bug to me, no?
>>>
>>> Since our controller will meet some strange behaviors if we do
>>> software reset for all in sdhci_runtime_resume_host(), and try to
>>> avoid changing the core logic of sdhci_runtime_resume_host() used by
>>> other hardware controllers, thus I introduced a specific reset ops and
>>> added some condition to make sure we just do software reset command
>>> and data lines from runtime suspend state.
>>
>> I understand, but perhaps it would become more clear if
>> sdhci_runtime_resume_host() is re-factored a bit. Maybe the caller can
>> give it some new parameter to let it decide if a SDHCI_RESET_ALL shall
>> be done or not.
> 
> Yes, sounds reasonable, but need change other host drivers which
> issued the sdhci_runtime_resume_host().
> 
> Adrian, if you also agree with Ulf's suggestion, then I will post new
> patches to add a parameter to decide the reset mode. Thanks.

Sounds fine.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming
  2019-07-24 12:55         ` Adrian Hunter
@ 2019-07-25  3:05           ` Baolin Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Baolin Wang @ 2019-07-25  3:05 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Ulf Hansson, Chunyan Zhang, Orson Zhai, Vincent Guittot,
	linux-mmc, Linux Kernel Mailing List

On Wed, 24 Jul 2019 at 20:56, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 24/07/19 5:21 AM, Baolin Wang wrote:
> > On Tue, 23 Jul 2019 at 20:39, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>
> >> On Tue, 23 Jul 2019 at 05:05, Baolin Wang <baolin.wang@linaro.org> wrote:
> >>>
> >>> Hi Ulf,
> >>>
> >>> On Mon, 22 Jul 2019 at 19:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>>
> >>>> On Wed, 17 Jul 2019 at 04:29, Baolin Wang <baolin.wang@linaro.org> wrote:
> >>>>>
> >>>>> In sdhci_runtime_resume_host() function, we will always do software reset
> >>>>> for all, which will cause Spreadtrum host controller work abnormally after
> >>>>> resuming.
> >>>>
> >>>> What does "software reset for all" means?
> >>>
> >>> The SD host controller specification defines 3 types software reset:
> >>> software reset for data line, software reset for command line and
> >>> software reset for all.
> >>> Software reset for all means this reset affects the entire Host
> >>> controller except for the card detection circuit.
> >>
> >> Thanks for clarifying, please update the changelog accordingly.
> >
> > Sure, sorry for confusing.
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Thus for Spreadtrum platform that will not power down the SD/eMMC card during
> >>>>> runtime suspend, we should not do software reset for all.
> >>>>
> >>>> Normally, sdhci hosts that enters runtime suspend doesn't power off
> >>>> the card (there are some exceptions like PCI variants).
> >>>
> >>> Yes, same as our controller.
> >>>
> >>>>
> >>>> So, what's so special here and how does the reset come into play? I
> >>>> don't see sdhci doing a reset in sdhci_runtime_suspend|resume_host()
> >>>> and nor doesn the callback from the sdhci-sprd.c variant doing it.
> >>>
> >>> In sdhci_runtime_resume_host(), it will issue sdhci_init(host, 0) to
> >>> issue software reset for all.
> >>
> >> Aha, I didn't read the code carefully enough. Apologize for the noise.
> >
> > No worries :)
> >
> >>>
> >>>>
> >>>>> To fix this
> >>>>> issue, adding a specific reset operation that adds one condition to validate
> >>>>> the power mode to decide if we can do software reset for all or just reset
> >>>>> command and data lines.
> >>>>>
> >>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >>>>> ---
> >>>>> Changess from v3:
> >>>>>  - Use ios.power_mode to validate if the card is power down or not.
> >>>>>
> >>>>> Changes from v2:
> >>>>>  - Simplify the sdhci_sprd_reset() by issuing sdhci_reset().
> >>>>>
> >>>>> Changes from v1:
> >>>>>  - Add a specific reset operation instead of changing the core to avoid
> >>>>>  affecting other hardware.
> >>>>> ---
> >>>>>  drivers/mmc/host/sdhci-sprd.c |   19 ++++++++++++++++++-
> >>>>>  1 file changed, 18 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
> >>>>> index 603a5d9..94f9726 100644
> >>>>> --- a/drivers/mmc/host/sdhci-sprd.c
> >>>>> +++ b/drivers/mmc/host/sdhci-sprd.c
> >>>>> @@ -373,6 +373,23 @@ static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host)
> >>>>>         return 1 << 31;
> >>>>>  }
> >>>>>
> >>>>> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> >>>>> +{
> >>>>> +       struct mmc_host *mmc = host->mmc;
> >>>>> +
> >>>>> +       /*
> >>>>> +        * When try to reset controller after runtime suspend, we should not
> >>>>> +        * reset for all if the SD/eMMC card is not power down, just reset
> >>>>> +        * command and data lines instead. Otherwise will meet some strange
> >>>>> +        * behaviors for Spreadtrum host controller.
> >>>>> +        */
> >>>>> +       if (host->runtime_suspended && (mask & SDHCI_RESET_ALL) &&
> >>>>> +           mmc->ios.power_mode == MMC_POWER_ON)
> >>>>> +               mask = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> >>>>
> >>>> Can sdhci_sprd_reset() be called when the host is runtime suspended?
> >>>
> >>> When host tries to runtime resume in sdhci_runtime_resume_host(), it
> >>> will call reset operation to do software reset.
> >>
> >> Right, I see that now, thanks for clarifying.
> >>
> >> However, there are still some weird things going on in
> >> sdhci_runtime_resume_host(). Like why is host->ops->enable_dma()
> >> called first, directly from sdhci_runtime_resume_host(), then again in
> >> sdhci_do_reset(), after host->ops->reset() has been called. Looks like
> >> the first call to ->enable_dma() doesn't make sense?
> >
> > I am mot sure, since our host did not supply enable_dma() operation.
> > This logic was used by some other hardware and worked well, I am not
> > sure if it can reveal some issues if we change the logic here.
> >
> > Adrian, could you help to explain why we put enable_dma() in front of
> > software reset?
>
> No reason I can see.  But if you add a parameter to avoid a full reset, then
> the ->enable_dma will be needed in that case.

OK. I'll keep it.

>
> >
> >>
> >>>
> >>>> That sounds like a bug to me, no?
> >>>
> >>> Since our controller will meet some strange behaviors if we do
> >>> software reset for all in sdhci_runtime_resume_host(), and try to
> >>> avoid changing the core logic of sdhci_runtime_resume_host() used by
> >>> other hardware controllers, thus I introduced a specific reset ops and
> >>> added some condition to make sure we just do software reset command
> >>> and data lines from runtime suspend state.
> >>
> >> I understand, but perhaps it would become more clear if
> >> sdhci_runtime_resume_host() is re-factored a bit. Maybe the caller can
> >> give it some new parameter to let it decide if a SDHCI_RESET_ALL shall
> >> be done or not.
> >
> > Yes, sounds reasonable, but need change other host drivers which
> > issued the sdhci_runtime_resume_host().
> >
> > Adrian, if you also agree with Ulf's suggestion, then I will post new
> > patches to add a parameter to decide the reset mode. Thanks.
>
> Sounds fine.

OK. Thanks for your input.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-07-25  3:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17  2:28 [PATCH v4] mmc: host: sdhci-sprd: Fix the incorrect soft reset operation when runtime resuming Baolin Wang
2019-07-17  6:07 ` Adrian Hunter
2019-07-22 11:54 ` Ulf Hansson
2019-07-23  3:05   ` Baolin Wang
2019-07-23  3:21     ` Chunyan Zhang
2019-07-23  3:30       ` Baolin Wang
2019-07-23 12:38     ` Ulf Hansson
2019-07-24  2:21       ` Baolin Wang
2019-07-24 12:55         ` Adrian Hunter
2019-07-25  3:05           ` Baolin Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).