diff mbox series

[v2] mmc: tmio: Add initial setting of interrupt mask register

Message ID 20180926150026.668-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State New, archived
Headers show
Series [v2] mmc: tmio: Add initial setting of interrupt mask register | expand

Commit Message

Niklas Söderlund Sept. 26, 2018, 3 p.m. UTC 
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 from the
bootloader. Since the error interrupts may be unmasked, the driver sets
initial value.

The initial value is only known for R-Car Gen2 and Gen3 platforms so
limit the initialization to those platforms.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---

* Changes since v1
- Limit the initialization to Gen2+ platforms by checking the
  TMIO_MMC_MIN_RCAR2 flag.
- Rename the constant for the initialization value to reflect it's only
  for R-Car Gen2+ platforms.
---
 drivers/mmc/host/tmio_mmc.h      | 1 +
 drivers/mmc/host/tmio_mmc_core.c | 4 ++++
 2 files changed, 5 insertions(+)

Comments

Simon Horman Oct. 1, 2018, 12:42 p.m. UTC | #1 
On Wed, Sep 26, 2018 at 05:00:26PM +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 from the
> bootloader. Since the error interrupts may be unmasked, the driver sets
> initial value.
> 
> The initial value is only known for R-Car Gen2 and Gen3 platforms so
> limit the initialization to those platforms.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Wolfram Sang Oct. 2, 2018, 10:54 p.m. UTC | #2 
On Wed, Sep 26, 2018 at 05:00:26PM +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 from the
> bootloader. Since the error interrupts may be unmasked, the driver sets
> initial value.
> 
> The initial value is only known for R-Car Gen2 and Gen3 platforms so
> limit the initialization to those platforms.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> ---
> 
> * Changes since v1
> - Limit the initialization to Gen2+ platforms by checking the
>   TMIO_MMC_MIN_RCAR2 flag.
> - Rename the constant for the initialization value to reflect it's only
>   for R-Car Gen2+ platforms.

Those changes are good! I wonder, though, if we shouldn't move the code
out of TMIO core into the SDHI core? This seems very Renesas specific.
What do you think?
Niklas Söderlund Oct. 5, 2018, 3:50 p.m. UTC | #3 
Hi Wolfram,

Thanks for your feedback.

On 2018-10-03 00:54:02 +0200, Wolfram Sang wrote:
> On Wed, Sep 26, 2018 at 05:00:26PM +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 from the
> > bootloader. Since the error interrupts may be unmasked, the driver sets
> > initial value.
> > 
> > The initial value is only known for R-Car Gen2 and Gen3 platforms so
> > limit the initialization to those platforms.
> > 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > ---
> > 
> > * Changes since v1
> > - Limit the initialization to Gen2+ platforms by checking the
> >   TMIO_MMC_MIN_RCAR2 flag.
> > - Rename the constant for the initialization value to reflect it's only
> >   for R-Car Gen2+ platforms.
> 
> Those changes are good! I wonder, though, if we shouldn't move the code
> out of TMIO core into the SDHI core? This seems very Renesas specific.
> What do you think?
> 

There are already checks for TMIO_MMC_MIN_RCAR2 inside 
tmio_mmc_host_probe(), but I agree with you it would be good if instead 
of adding to that start to move Renesas specific code out.

I did a quick test and it seems sane to move this to the end of 
renesas_sdhi_hw_reset(). Before I send a v3 of this what is your view?
Wolfram Sang Oct. 9, 2018, 11:53 p.m. UTC | #4 
Hi Niklas,

> There are already checks for TMIO_MMC_MIN_RCAR2 inside 
> tmio_mmc_host_probe(), but I agree with you it would be good if instead 
> of adding to that start to move Renesas specific code out.

Thanks!

> I did a quick test and it seems sane to move this to the end of 
> renesas_sdhi_hw_reset(). Before I send a v3 of this what is your view?

It seems a good place to me if it also gets called when probing the
device. From a quick glimpse, I see that this function ends up in
mmc_ops->hw_reset, but I haven't verified that the MMC core calls it
during probe. But you said you tested it...
Niklas Söderlund Oct. 10, 2018, 11:08 a.m. UTC | #5 
Hi Wolfram,

Thanks for our feedback.

On 2018-10-10 01:53:58 +0200, Wolfram Sang wrote:
> Hi Niklas,
> 
> > There are already checks for TMIO_MMC_MIN_RCAR2 inside 
> > tmio_mmc_host_probe(), but I agree with you it would be good if instead 
> > of adding to that start to move Renesas specific code out.
> 
> Thanks!
> 
> > I did a quick test and it seems sane to move this to the end of 
> > renesas_sdhi_hw_reset(). Before I send a v3 of this what is your view?
> 
> It seems a good place to me if it also gets called when probing the
> device. From a quick glimpse, I see that this function ends up in
> mmc_ops->hw_reset, but I haven't verified that the MMC core calls it
> during probe. But you said you tested it...
> 

You are correct, in mmc/next it is not called during probe. I see now I 
did my testing on-top of the reset series I'm currently reworking and 
then it's called at the right time to use the initial set irq mask for 
the _host->sdcard_irq_mask read out in tmio_mmc_host_probe().

I will move this patch back to the top of the reset series as it would 
IMHO create the least amount of code churn. Another option is to merge 
this version now and move it once the reset series is accepted.
Wolfram Sang Oct. 10, 2018, 11:39 a.m. UTC | #6 
> I will move this patch back to the top of the reset series as it would 
> IMHO create the least amount of code churn. Another option is to merge 
> this version now and move it once the reset series is accepted.

Less code churn sounds good to me.

Thanks, Niklas!
diff mbox series

Patch

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index a9972dc60c6fbb8c..00673cec47a4de13 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -99,6 +99,7 @@ 
 
 /* Define some IRQ masks */
 /* This is the mask used at reset by the chip */
+#define TMIO_MASK_INIT_RCAR2	0x8b7f031d /* Initial value for R-Car Gen2+ */
 #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 f05c3a622f090cd6..5aae7e2129400671 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1232,6 +1232,10 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	_host->set_clock(_host, 0);
 	tmio_mmc_reset(_host);
 
+	if (pdata->flags & TMIO_MMC_MIN_RCAR2)
+		sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK,
+					     TMIO_MASK_INIT_RCAR2);
+
 	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
 	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);