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@xxxxxxxxxxxx> wrote:
> >
> > From: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> >
> > ---
> > * 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
--
Regards,
Niklas Söderlund
