[PATCH RFC/RFT] mmc: tmio: always restore irq register

[PATCH RFC/RFT] mmc: tmio: always restore irq register

[Wolfram Sang]

Currently, only SDHI on R-Car Gen2+ reinitializes the irq register
during reset but it should be done on all instances. We can move it from
the SDHI driver to the TMIO core, because we now have the
'sd_irq_mask_all' variable which carries the proper value to use. That
also means we can remove the initialization from tmio_mmc_probe()
because it calls tmio_mmc_reset(), too. We only move that
tmio_mmc_reset() call there a little to ensure 'sd_irq_mask_all' is
properly set.

Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Shimoda-san, I think this is the implementation of what we discussed. It
passes my tests on a Renesas H3 ES2.0. I'd be happy if you or the BSP
team could run their additional checks with this patch. Thank you and
kind regards!

 drivers/mmc/host/renesas_sdhi_core.c |  2 --
 drivers/mmc/host/tmio_mmc_core.c     | 11 ++++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index d36181b6f687..635bf31a6735 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -588,8 +588,6 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 		renesas_sdhi_scc_reset(host, priv);
 	}
 
-	sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK, TMIO_MASK_ALL_RCAR2);
-
 	if (sd_ctrl_read16(host, CTL_VERSION) >= SDHI_VER_GEN3_SD) {
 		val = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT);
 		val |= CARD_OPT_EXTOP;
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 0c474d78b186..bcd26056d47a 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -192,6 +192,9 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
 	if (host->reset)
 		host->reset(host);
 
+	host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(host, CTL_IRQ_MASK);
+	tmio_mmc_disable_mmc_irqs(host, host->sdcard_irq_mask_all);
+
 	tmio_mmc_set_bus_width(host, host->mmc->ios.bus_width);
 
 	if (host->pdata->flags & TMIO_MMC_SDIO_IRQ) {
@@ -1176,13 +1179,11 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	if (pdata->flags & TMIO_MMC_SDIO_IRQ)
 		_host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
 
-	_host->set_clock(_host, 0);
-	tmio_mmc_reset(_host);
-
-	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
 	if (!_host->sdcard_irq_mask_all)
 		_host->sdcard_irq_mask_all = TMIO_MASK_ALL;
-	tmio_mmc_disable_mmc_irqs(_host, _host->sdcard_irq_mask_all);
+
+	_host->set_clock(_host, 0);
+	tmio_mmc_reset(_host);
 
 	if (_host->native_hotplug)
 		tmio_mmc_enable_mmc_irqs(_host,
-- 
2.30.0

RE: [PATCH RFC/RFT] mmc: tmio: always restore irq register

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Thursday, April 8, 2021 12:31 AM
> 
> Currently, only SDHI on R-Car Gen2+ reinitializes the irq register
> during reset but it should be done on all instances. We can move it from
> the SDHI driver to the TMIO core, because we now have the
> 'sd_irq_mask_all' variable which carries the proper value to use. That
> also means we can remove the initialization from tmio_mmc_probe()
> because it calls tmio_mmc_reset(), too. We only move that
> tmio_mmc_reset() call there a little to ensure 'sd_irq_mask_all' is
> properly set.

Yes, this is my expectation. However...

> Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Shimoda-san, I think this is the implementation of what we discussed. It
> passes my tests on a Renesas H3 ES2.0. I'd be happy if you or the BSP
> team could run their additional checks with this patch. Thank you and
> kind regards!
> 
>  drivers/mmc/host/renesas_sdhi_core.c |  2 --
>  drivers/mmc/host/tmio_mmc_core.c     | 11 ++++++-----
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index d36181b6f687..635bf31a6735 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -588,8 +588,6 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  		renesas_sdhi_scc_reset(host, priv);
>  	}
> 
> -	sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK, TMIO_MASK_ALL_RCAR2);
> -
>  	if (sd_ctrl_read16(host, CTL_VERSION) >= SDHI_VER_GEN3_SD) {
>  		val = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT);
>  		val |= CARD_OPT_EXTOP;
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 0c474d78b186..bcd26056d47a 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -192,6 +192,9 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
>  	if (host->reset)
>  		host->reset(host);
> 
> +	host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(host, CTL_IRQ_MASK);
> +	tmio_mmc_disable_mmc_irqs(host, host->sdcard_irq_mask_all);
> +

This code could not resolve my concern. This code still read
CTL_IRQ_MASK at first. So, if the register value is incorrect
(when "host->reset" didn't exist), the sdcard_irq_mask value
will be not expected value.

So, I'm thinking we should write CTL_IRQ_MASK with sdcard_irq_mask_all
by using sd_ctrl_write32_as_16_and_16() instead of using
tmio_mmc_disable_mmc_irqs() at first.

Best regards,
Yoshihiro Shimoda

Re: [PATCH RFC/RFT] mmc: tmio: always restore irq register

[Wolfram Sang]

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

Hi Shimoda-san,

> This code could not resolve my concern. This code still read
> CTL_IRQ_MASK at first. So, if the register value is incorrect
> (when "host->reset" didn't exist), the sdcard_irq_mask value
> will be not expected value.

Geez, I forgot (again) that SD_RESET will not reset the irq register :(
I will initialize the register and the cache variable in reset() and
will resend in a few minutes after the tests.

Thanks!

   Wolfram

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