[PATCH 0/3] mmc: tmio: Fix reset operation

[PATCH 0/3] mmc: tmio: Fix reset operation

[Niklas Söderlund]

Hi,

While looking at the Renesas BSP kernel I found patches which improves 
the state of the hardware at probe and after runtime resume. This is 
tested on R-Car H3 ES2.

Patch 1/3 ensures the IRQ mask have the default value according the 
datasheet (Rev 0.15) in case the bootloader have changed it. Patch 2/3 
moves code around to prepare for 3/3 which improves the hardware reset 
by also resetting the SCC which was not previously reset.

Patches from Hayakawa-san have small changes done by me needed after 
they where rebased to mmc/next.

Masaharu Hayakawa (2):
  mmc: tmio: Add initial setting of interrupt mask register
  mmc: tmio: Fix reset operation

Niklas Söderlund (1):
  mmc: tmio: move tmio_mmc_hw_reset()

 drivers/mmc/host/tmio_mmc.h      |  1 +
 drivers/mmc/host/tmio_mmc_core.c | 27 ++++++++++++++++-----------
 2 files changed, 17 insertions(+), 11 deletions(-)

-- 
2.17.0

[PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

The initial value of the interrupt mask register may be different from
the H/W manual at the startup of the kernel by setting of initial program.
Since the error interrupts may be unmasked, the driver sets initial value.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/mmc/host/tmio_mmc.h      | 1 +
 drivers/mmc/host/tmio_mmc_core.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index e7d651352dc90fb8..bfb76b2963f80894 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -102,6 +102,7 @@
 
 /* Define some IRQ masks */
 /* This is the mask used at reset by the chip */
+#define TMIO_MASK_INIT          0x8b7f031d /* H/W initial value */
 #define TMIO_MASK_ALL           0x837f031d
 #define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND)
 #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND)
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 3080299303045adc..2ede81476daf76ce 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	tmio_mmc_clk_stop(_host);
 	tmio_mmc_reset(_host);
 
+	sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT);
 	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
 	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
 
-- 
2.17.0

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Hi Niklas,

On Thu, Jun 07, 2018 at 01:22:50AM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> The initial value of the interrupt mask register may be different from
> the H/W manual at the startup of the kernel by setting of initial program.

s/initial program/bootloader/ ?

> Since the error interrupts may be unmasked, the driver sets initial value.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/mmc/host/tmio_mmc.h      | 1 +
>  drivers/mmc/host/tmio_mmc_core.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index e7d651352dc90fb8..bfb76b2963f80894 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -102,6 +102,7 @@
>  
>  /* Define some IRQ masks */
>  /* This is the mask used at reset by the chip */
> +#define TMIO_MASK_INIT          0x8b7f031d /* H/W initial value */

known on R-Car 2+ only!

>  #define TMIO_MASK_ALL           0x837f031d
>  #define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND)
>  #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND)
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 3080299303045adc..2ede81476daf76ce 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>  	tmio_mmc_clk_stop(_host);
>  	tmio_mmc_reset(_host);
>  
> +	sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT);

So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
Maybe we should even move it to renesas_sdhi_core.c, but I am not too
sure about that.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2018-06-09 20:15:52 +0200, Wolfram Sang wrote:
> Hi Niklas,
> 
> On Thu, Jun 07, 2018 at 01:22:50AM +0200, Niklas S�derlund wrote:
> > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > 
> > The initial value of the interrupt mask register may be different from
> > the H/W manual at the startup of the kernel by setting of initial program.
> 
> s/initial program/bootloader/ ?

Sounds much better.

> 
> > Since the error interrupts may be unmasked, the driver sets initial value.
> > 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/mmc/host/tmio_mmc.h      | 1 +
> >  drivers/mmc/host/tmio_mmc_core.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index e7d651352dc90fb8..bfb76b2963f80894 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -102,6 +102,7 @@
> >  
> >  /* Define some IRQ masks */
> >  /* This is the mask used at reset by the chip */
> > +#define TMIO_MASK_INIT          0x8b7f031d /* H/W initial value */
> 
> known on R-Car 2+ only!

No it's also know on Gen3 :-)

If you check the documents for the registers SD_INFO1_MASK and 
SD_INFO2_MASK they describe there respective initial values. If one 
combines them you get the same constant as documented here.

The tricky part is that sd_ctrl_write32_as_16_and_16() is used to write 
them so you have to take 16 bits from each register description as that 
is how the are written. After the fun bus shift is applied this ends up 
writing the lower 16 bits of the constant to 0x80 and the higher 
16 of it to 0x88.

> 
> >  #define TMIO_MASK_ALL           0x837f031d
> >  #define TMIO_MASK_READOP  (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND)
> >  #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND)
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> > index 3080299303045adc..2ede81476daf76ce 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
> >  	tmio_mmc_clk_stop(_host);
> >  	tmio_mmc_reset(_host);
> >  
> > +	sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT);
> 
> So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> sure about that.
> 
> Regards,
> 
>    Wolfram
> 



-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]


> > > +#define TMIO_MASK_INIT          0x8b7f031d /* H/W initial value */
> > 
> > known on R-Car 2+ only!
> 
> No it's also know on Gen3 :-)

Sure, 2+ means "R-Car 2 and later". Like with GPL2+. Sorry if this was
not clear. And it is still my main argument to do...

> > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> > Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> > sure about that.

... this.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2018-06-27 09:27:02 +0200, Wolfram Sang wrote:
> 
> > > > +#define TMIO_MASK_INIT          0x8b7f031d /* H/W initial value */
> > > 
> > > known on R-Car 2+ only!
> > 
> > No it's also know on Gen3 :-)
> 
> Sure, 2+ means "R-Car 2 and later". Like with GPL2+. Sorry if this was
> not clear. And it is still my main argument to do...

When you point it out it's clear, sorry I missed that.

> 
> > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> > > sure about that.
> 
> ... this.
> 

I will look into this and send a new version :-)

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]


> > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> > > > sure about that.
> > 
> > ... this.
> > 
> 
> I will look into this and send a new version :-)

Did you send V2 of this? I can't find it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Niklas Söderlund]

Hi Wolfram,

On 2018-09-16 16:48:07 +0200, Wolfram Sang wrote:
> 
> > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> > > > > sure about that.
> > > 
> > > ... this.
> > > 
> > 
> > I will look into this and send a new version :-)
> 
> Did you send V2 of this? I can't find it.
> 

No I have not yet posted a v2 of this series. Perhaps I should break 
this patch out and send separate? I have handled the comments for this 
but patch 3/3 in this series still requires more work, what do you 
think?

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Thu, Sep 20, 2018 at 12:02:27AM +0200, Niklas Söderlund wrote:
> Hi Wolfram,
> 
> On 2018-09-16 16:48:07 +0200, Wolfram Sang wrote:
> > 
> > > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think.
> > > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too
> > > > > > sure about that.
> > > > 
> > > > ... this.
> > > > 
> > > 
> > > I will look into this and send a new version :-)
> > 
> > Did you send V2 of this? I can't find it.
> > 
> 
> No I have not yet posted a v2 of this series. Perhaps I should break 
> this patch out and send separate? I have handled the comments for this 
> but patch 3/3 in this series still requires more work, what do you 
> think?

Yes, please.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] mmc: tmio: move tmio_mmc_hw_reset()

[Niklas Söderlund]

Avoid having to use forward declaration when in a later patch fixing the
reset operation by moving tmio_mmc_hw_reset() earlier in the source
file. No functional change in this change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/mmc/host/tmio_mmc_core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 2ede81476daf76ce..9f5793407552a23a 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -243,6 +243,14 @@ 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);
+
+	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,
@@ -769,14 +777,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);
-- 
2.17.0

Re: [PATCH 2/3] mmc: tmio: move tmio_mmc_hw_reset()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Thu, Jun 07, 2018 at 01:22:51AM +0200, Niklas Söderlund wrote:
> Avoid having to use forward declaration when in a later patch fixing the
> reset operation by moving tmio_mmc_hw_reset() earlier in the source
> file. No functional change in this change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I'd merge patches 2+3, but I'll leave that to Ulf...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] mmc: tmio: Fix reset operation

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

SD / MMC did not operate properly when suspend transition failed.
Because the SCC was not reset at resume, issue of the command failed.
Changed tmio_mmc_reset() to tmio_mmc_hw_reset() in order to add reset
of SCC to tmio_mmc_host_runtime_resume().

On runtime power management resume, the host clock needs to be
enabled before calling tmio_mmc_reset. If the mmc device has a power
domain entry, the host clock is enabled via genpd_runtime_resume,
running before tmio_mmc_host_runtime_resume. If the mmc device has
no power domain entry, however, genpd_runtime_resume is not called.
This patch changes tmio_mmc_host_runtime_resume to enable the host
clock before calling tmio_mmc_reset.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: small fixes when rebase to mmc/next]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/mmc/host/tmio_mmc_core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 9f5793407552a23a..da10b26d8f2d80e7 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -247,6 +247,10 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
 
+	tmio_mmc_reset(host);
+
+	tmio_mmc_abort_dma(host);
+
 	if (host->hw_reset)
 		host->hw_reset(host);
 }
@@ -289,7 +293,7 @@ static void tmio_mmc_reset_work(struct work_struct *work)
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
-	tmio_mmc_reset(host);
+	tmio_mmc_hw_reset(host->mmc);
 
 	/* Ready for new calls */
 	host->mrq = NULL;
@@ -1271,7 +1275,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 		_host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
 
 	tmio_mmc_clk_stop(_host);
-	tmio_mmc_reset(_host);
+	tmio_mmc_hw_reset(mmc);
 
 	sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT);
 	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
@@ -1372,8 +1376,8 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
 {
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
 
-	tmio_mmc_reset(host);
 	tmio_mmc_clk_enable(host);
+	tmio_mmc_hw_reset(host->mmc);
 
 	if (host->clk_cache)
 		tmio_mmc_set_clock(host, host->clk_cache);
-- 
2.17.0

Re: [PATCH 3/3] mmc: tmio: Fix reset operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

On Thu, Jun 07, 2018 at 01:22:52AM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> SD / MMC did not operate properly when suspend transition failed.
> Because the SCC was not reset at resume, issue of the command failed.
> Changed tmio_mmc_reset() to tmio_mmc_hw_reset() in order to add reset
> of SCC to tmio_mmc_host_runtime_resume().

So, my gut feeling is that this change, in general, seems OK. However,
the naming (tmio_mmc_reset and tmio_mmc_hw_reset) becomes confusing and
unneeded. tmio_mmc_reset now only gets called in tmio_mmc_hw_reset, so
we can merge the two functions into one to reduce the confusion.

Because we add stuff to the function which gets pointed to by
mmc_ops->hw_reset, it makes sense to me to double check when the MMC
core calls this pointer and if our additions still make sense there.
I'd think so, but better safe than sorry.

> On runtime power management resume, the host clock needs to be
> enabled before calling tmio_mmc_reset. If the mmc device has a power
> domain entry, the host clock is enabled via genpd_runtime_resume,
> running before tmio_mmc_host_runtime_resume. If the mmc device has
> no power domain entry, however, genpd_runtime_resume is not called.
> This patch changes tmio_mmc_host_runtime_resume to enable the host
> clock before calling tmio_mmc_reset.

I think this makes sense but should be a seperate patch. Or maybe even
something slightly different? Because we have a patch in the BSP [1] for
which I ever wondered in what case it was needed. But seeing the above,
it might make sense to add the clock enablement to the reset routine?
Niklas, do you have time to check that?

Thanks,

   Wolfram

[1] 940904fe4ea8 ("mmc: tmio: Add SDCLK enable after reset")


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]