[RFC 0/3] mmc: host: tmio: sdio irq improvements

[RFC 0/3] mmc: host: tmio: sdio irq improvements

[Wolfram Sang]

Here is a small series with two minor improvements (patches 1+2) and one bigger
change (patch 3) for SDIO handling with TMIO/SDHI. It is marked as RFC because
I wonder why other drivers are largely not clearing interrupts before enabling
them (patch 3 again). I really think it should be done, but maybe I am missing
something?

Based on mmc/next and tested on Renesas R-Car H3 and M3-W.


Wolfram Sang (3):
  mmc: host: tmio: refactor calls to sdio irq
  mmc: host: tmio: SDIO_STATUS_QUIRK is rather SDIO_STATUS_SETBITS
  mmc: tmio: discard obsolete SDIO irqs before enabling irqs

 drivers/mmc/host/sh_mobile_sdhi.c |  6 ++----
 drivers/mmc/host/tmio_mmc_pio.c   | 18 +++++++++++++-----
 include/linux/mfd/tmio.h          |  2 +-
 3 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.11.0

[RFC 1/3] mmc: host: tmio: refactor calls to sdio irq

[Wolfram Sang]

tmio_mmc_sdio_irq() is not used as a seperate irq handler anymore, so we
can make it similar to the other irq helper functions, namely:

* only give the host as argument function which is what it really needs
* prefix function name with __

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 3ca97f3c307975..60dc43b4574117 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -709,9 +709,8 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host,
 	return false;
 }
 
-static void tmio_mmc_sdio_irq(int irq, void *devid)
+static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
 {
-	struct tmio_mmc_host *host = devid;
 	struct mmc_host *mmc = host->mmc;
 	struct tmio_mmc_data *pdata = host->pdata;
 	unsigned int ireg, status;
@@ -752,7 +751,7 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	if (__tmio_mmc_sdcard_irq(host, ireg, status))
 		return IRQ_HANDLED;
 
-	tmio_mmc_sdio_irq(irq, devid);
+	__tmio_mmc_sdio_irq(host);
 
 	return IRQ_HANDLED;
 }
-- 
2.11.0

[RFC 2/3] mmc: host: tmio: SDIO_STATUS_QUIRK is rather SDIO_STATUS_SETBITS

[Wolfram Sang]

QUIRK sounds like there is something wrong, but actually there are just
some bits which need to be 1. Rename it to be more clear.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 6 ++----
 drivers/mmc/host/tmio_mmc_pio.c   | 2 +-
 include/linux/mfd/tmio.h          | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 59db14b4827c33..1600fefc2c3e2c 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -641,10 +641,8 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	 */
 	mmc_data->flags |= TMIO_MMC_HAVE_CMD12_CTRL;
 
-	/*
-	 * All SDHI need SDIO_INFO1 reserved bit
-	 */
-	mmc_data->flags |= TMIO_MMC_SDIO_STATUS_QUIRK;
+	/* All SDHI have reserved bits */
+	mmc_data->flags |= TMIO_MMC_SDIO_STATUS_SETBITS;
 
 	ret = tmio_mmc_host_probe(host, mmc_data);
 	if (ret < 0)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 60dc43b4574117..d6033620a45c12 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -723,7 +723,7 @@ static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
 	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdio_irq_mask;
 
 	sdio_status = status & ~TMIO_SDIO_MASK_ALL;
-	if (pdata->flags & TMIO_MMC_SDIO_STATUS_QUIRK)
+	if (pdata->flags & TMIO_MMC_SDIO_STATUS_SETBITS)
 		sdio_status |= 6;
 
 	sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status);
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index fba44abd05ba1a..5a05a34cc1a292 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -97,7 +97,7 @@
 /*
  * Some controllers needs to set 1 on SDIO status reserved bits
  */
-#define TMIO_MMC_SDIO_STATUS_QUIRK	(1 << 8)
+#define TMIO_MMC_SDIO_STATUS_SETBITS	(1 << 8)
 
 /*
  * Some controllers have a 32-bit wide data port register
-- 
2.11.0

[RFC 3/3] mmc: tmio: discard obsolete SDIO irqs before enabling irqs

[Wolfram Sang]

Before enabling SDIO irqs, clear the status bit, so we discard old and
stale interrupts. Needed to get two wireless cards working.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index d6033620a45c12..ebe3e12f0083dd 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -134,12 +134,21 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	struct tmio_mmc_host *host = mmc_priv(mmc);
 
 	if (enable && !host->sdio_irq_enabled) {
+		u16 sdio_status;
+
 		/* Keep device active while SDIO irq is enabled */
 		pm_runtime_get_sync(mmc_dev(mmc));
-		host->sdio_irq_enabled = true;
 
+		host->sdio_irq_enabled = true;
 		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
 					~TMIO_SDIO_STAT_IOIRQ;
+
+		/* Clear obsolete interrupts before enabling */
+		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS) & ~TMIO_SDIO_MASK_ALL;
+		if (host->pdata->flags & TMIO_MMC_SDIO_STATUS_SETBITS)
+			sdio_status |= 6;
+		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status);
+
 		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 	} else if (!enable && host->sdio_irq_enabled) {
 		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
-- 
2.11.0

Re: [RFC 1/3] mmc: host: tmio: refactor calls to sdio irq

[Simon Horman]

On Thu, Jan 05, 2017 at 12:43:24PM +0100, Wolfram Sang wrote:
> tmio_mmc_sdio_irq() is not used as a seperate irq handler anymore, so we
> can make it similar to the other irq helper functions, namely:
> 
> * only give the host as argument function which is what it really needs
> * prefix function name with __
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I like that this avoids implicitly casting to and from void *.

Acked-by: Simon Horman <horms+renesas@verge.net.au>

Re: [RFC 2/3] mmc: host: tmio: SDIO_STATUS_QUIRK is rather SDIO_STATUS_SETBITS

[Simon Horman]

On Thu, Jan 05, 2017 at 12:43:25PM +0100, Wolfram Sang wrote:
> QUIRK sounds like there is something wrong, but actually there are just
> some bits which need to be 1. Rename it to be more clear.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 6 ++----
>  drivers/mmc/host/tmio_mmc_pio.c   | 2 +-
>  include/linux/mfd/tmio.h          | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 59db14b4827c33..1600fefc2c3e2c 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -641,10 +641,8 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>  	 */
>  	mmc_data->flags |= TMIO_MMC_HAVE_CMD12_CTRL;
>  
> -	/*
> -	 * All SDHI need SDIO_INFO1 reserved bit
> -	 */
> -	mmc_data->flags |= TMIO_MMC_SDIO_STATUS_QUIRK;
> +	/* All SDHI have reserved bits */
> +	mmc_data->flags |= TMIO_MMC_SDIO_STATUS_SETBITS;
>  
>  	ret = tmio_mmc_host_probe(host, mmc_data);
>  	if (ret < 0)
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 60dc43b4574117..d6033620a45c12 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -723,7 +723,7 @@ static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
>  	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdio_irq_mask;
>  
>  	sdio_status = status & ~TMIO_SDIO_MASK_ALL;
> -	if (pdata->flags & TMIO_MMC_SDIO_STATUS_QUIRK)
> +	if (pdata->flags & TMIO_MMC_SDIO_STATUS_SETBITS)
>  		sdio_status |= 6;
>  
>  	sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status);
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index fba44abd05ba1a..5a05a34cc1a292 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -97,7 +97,7 @@
>  /*
>   * Some controllers needs to set 1 on SDIO status reserved bits
>   */

Is the following update to the above appropriate?
In particular, is it some or all?

    * Controllers need to set 1 on SDIO status reserved bits

> -#define TMIO_MMC_SDIO_STATUS_QUIRK	(1 << 8)
> +#define TMIO_MMC_SDIO_STATUS_SETBITS	(1 << 8)
>  
>  /*
>   * Some controllers have a 32-bit wide data port register
> -- 
> 2.11.0
> 

Re: [RFC 3/3] mmc: tmio: discard obsolete SDIO irqs before enabling irqs

[Simon Horman]

On Thu, Jan 05, 2017 at 12:43:26PM +0100, Wolfram Sang wrote:
> Before enabling SDIO irqs, clear the status bit, so we discard old and
> stale interrupts. Needed to get two wireless cards working.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index d6033620a45c12..ebe3e12f0083dd 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -134,12 +134,21 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>  	struct tmio_mmc_host *host = mmc_priv(mmc);
>  
>  	if (enable && !host->sdio_irq_enabled) {
> +		u16 sdio_status;
> +
>  		/* Keep device active while SDIO irq is enabled */
>  		pm_runtime_get_sync(mmc_dev(mmc));
> -		host->sdio_irq_enabled = true;
>  
> +		host->sdio_irq_enabled = true;
>  		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
>  					~TMIO_SDIO_STAT_IOIRQ;
> +
> +		/* Clear obsolete interrupts before enabling */
> +		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS) & ~TMIO_SDIO_MASK_ALL;
> +		if (host->pdata->flags & TMIO_MMC_SDIO_STATUS_SETBITS)
> +			sdio_status |= 6;

Perhaps a #define would be an improvement over "6".

> +		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status);
> +
>  		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
>  	} else if (!enable && host->sdio_irq_enabled) {
>  		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
> -- 
> 2.11.0
> 

Re: [RFC 2/3] mmc: host: tmio: SDIO_STATUS_QUIRK is rather SDIO_STATUS_SETBITS

[Wolfram Sang]

> > --- a/include/linux/mfd/tmio.h
> > +++ b/include/linux/mfd/tmio.h
> > @@ -97,7 +97,7 @@
> >  /*
> >   * Some controllers needs to set 1 on SDIO status reserved bits
> >   */
> 
> Is the following update to the above appropriate?
> In particular, is it some or all?

Some.

>     * Controllers need to set 1 on SDIO status reserved bits

I updated this and the other comment to be more precise. Thanks!

Re: [RFC 3/3] mmc: tmio: discard obsolete SDIO irqs before enabling irqs

[Wolfram Sang]

> > +		if (host->pdata->flags & TMIO_MMC_SDIO_STATUS_SETBITS)
> > +			sdio_status |= 6;
> 
> Perhaps a #define would be an improvement over "6".

OK.