| Message ID | 20210407153126.37285-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC/RFT] mmc: tmio: always restore irq register | expand |
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
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
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,
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(-)