diff mbox series

[v3,2/3] mmc: tmio: fix reset operation

Message ID 20181031230554.1660-3-niklas.soderlund@ragnatech.se (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series mmc: tmio: fix reset operation | expand

Commit Message

Niklas Söderlund Oct. 31, 2018, 11:05 p.m. UTC 
From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

SD / MMC did not operate properly when suspend transition failed.
Because the SCC was not reset at resume, issue of the command failed.
Call the host specific reset function and reset the hardware in order to
add reset of SCC. This change also fixes tuning on some stubborn cards
on Gen2.

Based on work from Masaharu Hayakawa.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---
* Changes sine v1
- Merge tmio_mmc_reset() into tmio_mmc_hw_reset() as it's now the only
  caller.

* Changes since v2
- Rebased on mmc/next caused small refactoring of the code.
---
 drivers/mmc/host/tmio_mmc_core.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

Wolfram Sang Nov. 1, 2018, 7:02 p.m. UTC | #1 
On Thu, Nov 01, 2018 at 12:05:53AM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> SD / MMC did not operate properly when suspend transition failed.
> Because the SCC was not reset at resume, issue of the command failed.
> Call the host specific reset function and reset the hardware in order to
> add reset of SCC. This change also fixes tuning on some stubborn cards
> on Gen2.
> 
> Based on work from Masaharu Hayakawa.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Masahiro Yamada Nov. 2, 2018, 6:54 a.m. UTC | #2 
On Thu, Nov 1, 2018 at 8:53 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> SD / MMC did not operate properly when suspend transition failed.
> Because the SCC was not reset at resume, issue of the command failed.
> Call the host specific reset function and reset the hardware in order to
> add reset of SCC. This change also fixes tuning on some stubborn cards
> on Gen2.
>
> Based on work from Masaharu Hayakawa.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
> * Changes sine v1
> - Merge tmio_mmc_reset() into tmio_mmc_hw_reset() as it's now the only
>   caller.
>
> * Changes since v2
> - Rebased on mmc/next caused small refactoring of the code.
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 953562a12a0d6ebc..662161be03b6d52e 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -171,6 +171,18 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
>         }
>  }
>
> +static void tmio_mmc_hw_reset(struct mmc_host *mmc)
> +{
> +       struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +       host->reset(host);
> +
> +       tmio_mmc_abort_dma(host);
> +
> +       if (host->hw_reset)
> +               host->hw_reset(host);
> +}
> +
>  static void tmio_mmc_reset_work(struct work_struct *work)
>  {
>         struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
> @@ -209,7 +221,7 @@ static void tmio_mmc_reset_work(struct work_struct *work)
>
>         spin_unlock_irqrestore(&host->lock, flags);
>
> -       host->reset(host);
> +       tmio_mmc_hw_reset(host->mmc);
>
>         /* Ready for new calls */
>         host->mrq = NULL;



I see tmio_mmc_abort_dma() a few lines below.

If you add tmio_mmc_abort_dma() into tmio_mmc_hw_reset(),
you do not need to abort DMA twice, don't you?




        tmio_mmc_hw_reset(host->mmc);

        /* Ready for new calls */
        host->mrq = NULL;

        tmio_mmc_abort_dma(host);              /* <-- abort DMA again? */
        mmc_request_done(host->mmc, mrq);
}







> @@ -696,14 +708,6 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
>         return 0;
>  }
>
> -static void tmio_mmc_hw_reset(struct mmc_host *mmc)
> -{
> -       struct tmio_mmc_host *host = mmc_priv(mmc);
> -
> -       if (host->hw_reset)
> -               host->hw_reset(host);
> -}
> -
>  static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>         struct tmio_mmc_host *host = mmc_priv(mmc);
> @@ -1228,7 +1232,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>                 _host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
>
>         _host->set_clock(_host, 0);
> -       _host->reset(_host);
> +       tmio_mmc_hw_reset(mmc);


I think it is weird to call tmio_mmc_abort_dma()
before tmio_mmc_request_dma().





>         _host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
>         tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
> @@ -1329,7 +1333,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
>         struct tmio_mmc_host *host = dev_get_drvdata(dev);
>
>         tmio_mmc_clk_enable(host);
> -       host->reset(host);
> +       tmio_mmc_hw_reset(host->mmc);
>
>         if (host->clk_cache)
>                 host->set_clock(host, host->clk_cache);
> --
> 2.19.1
>


--
Best Regards
Masahiro Yamada
Niklas Söderlund Nov. 26, 2018, 4:53 p.m. UTC | #3 
Hi Yamada-san,

Thanks for your feedback.

On 2018-11-02 15:54:17 +0900, Masahiro Yamada wrote:
> On Thu, Nov 1, 2018 at 8:53 AM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > SD / MMC did not operate properly when suspend transition failed.
> > Because the SCC was not reset at resume, issue of the command failed.
> > Call the host specific reset function and reset the hardware in order to
> > add reset of SCC. This change also fixes tuning on some stubborn cards
> > on Gen2.
> >
> > Based on work from Masaharu Hayakawa.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > ---
> > * Changes sine v1
> > - Merge tmio_mmc_reset() into tmio_mmc_hw_reset() as it's now the only
> >   caller.
> >
> > * Changes since v2
> > - Rebased on mmc/next caused small refactoring of the code.
> > ---
> >  drivers/mmc/host/tmio_mmc_core.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> > index 953562a12a0d6ebc..662161be03b6d52e 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -171,6 +171,18 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
> >         }
> >  }
> >
> > +static void tmio_mmc_hw_reset(struct mmc_host *mmc)
> > +{
> > +       struct tmio_mmc_host *host = mmc_priv(mmc);
> > +
> > +       host->reset(host);
> > +
> > +       tmio_mmc_abort_dma(host);
> > +
> > +       if (host->hw_reset)
> > +               host->hw_reset(host);
> > +}
> > +
> >  static void tmio_mmc_reset_work(struct work_struct *work)
> >  {
> >         struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
> > @@ -209,7 +221,7 @@ static void tmio_mmc_reset_work(struct work_struct *work)
> >
> >         spin_unlock_irqrestore(&host->lock, flags);
> >
> > -       host->reset(host);
> > +       tmio_mmc_hw_reset(host->mmc);
> >
> >         /* Ready for new calls */
> >         host->mrq = NULL;
> 
> 
> 
> I see tmio_mmc_abort_dma() a few lines below.
> 
> If you add tmio_mmc_abort_dma() into tmio_mmc_hw_reset(),
> you do not need to abort DMA twice, don't you?
> 
> 
> 
> 
>         tmio_mmc_hw_reset(host->mmc);
> 
>         /* Ready for new calls */
>         host->mrq = NULL;
> 
>         tmio_mmc_abort_dma(host);              /* <-- abort DMA again? */
>         mmc_request_done(host->mmc, mrq);
> }
> 

You are correct with this change the call to tmio_mmc_abort_dma() can be 
dropped here. Will do so and send out a new version. Thanks for pointing 
this out!

> 
> > @@ -696,14 +708,6 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> >         return 0;
> >  }
> >
> > -static void tmio_mmc_hw_reset(struct mmc_host *mmc)
> > -{
> > -       struct tmio_mmc_host *host = mmc_priv(mmc);
> > -
> > -       if (host->hw_reset)
> > -               host->hw_reset(host);
> > -}
> > -
> >  static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> >  {
> >         struct tmio_mmc_host *host = mmc_priv(mmc);
> > @@ -1228,7 +1232,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
> >                 _host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
> >
> >         _host->set_clock(_host, 0);
> > -       _host->reset(_host);
> > +       tmio_mmc_hw_reset(mmc);
> 
> 
> I think it is weird to call tmio_mmc_abort_dma()
> before tmio_mmc_request_dma().
> 
> 
> 
> 
> 
> >         _host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
> >         tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
> > @@ -1329,7 +1333,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
> >         struct tmio_mmc_host *host = dev_get_drvdata(dev);
> >
> >         tmio_mmc_clk_enable(host);
> > -       host->reset(host);
> > +       tmio_mmc_hw_reset(host->mmc);
> >
> >         if (host->clk_cache)
> >                 host->set_clock(host, host->clk_cache);
> > --
> > 2.19.1
> >
> 
> 
> --
> Best Regards
> Masahiro Yamada
diff mbox series

Patch

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 953562a12a0d6ebc..662161be03b6d52e 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -171,6 +171,18 @@  static void tmio_mmc_reset(struct tmio_mmc_host *host)
 	}
 }
 
+static void tmio_mmc_hw_reset(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	host->reset(host);
+
+	tmio_mmc_abort_dma(host);
+
+	if (host->hw_reset)
+		host->hw_reset(host);
+}
+
 static void tmio_mmc_reset_work(struct work_struct *work)
 {
 	struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
@@ -209,7 +221,7 @@  static void tmio_mmc_reset_work(struct work_struct *work)
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
-	host->reset(host);
+	tmio_mmc_hw_reset(host->mmc);
 
 	/* Ready for new calls */
 	host->mrq = NULL;
@@ -696,14 +708,6 @@  static int tmio_mmc_start_data(struct tmio_mmc_host *host,
 	return 0;
 }
 
-static void tmio_mmc_hw_reset(struct mmc_host *mmc)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (host->hw_reset)
-		host->hw_reset(host);
-}
-
 static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
@@ -1228,7 +1232,7 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 		_host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
 
 	_host->set_clock(_host, 0);
-	_host->reset(_host);
+	tmio_mmc_hw_reset(mmc);
 
 	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
 	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
@@ -1329,7 +1333,7 @@  int tmio_mmc_host_runtime_resume(struct device *dev)
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
 
 	tmio_mmc_clk_enable(host);
-	host->reset(host);
+	tmio_mmc_hw_reset(host->mmc);
 
 	if (host->clk_cache)
 		host->set_clock(host, host->clk_cache);