[PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Doug Anderson]

Bing Zhao at Marvell found a problem with dw_mmc where interrupts
weren't firing sometimes.  He tracked it down to a read-modify-write
problem with the INTMASK.  These patches fix the problem.

Note: I've picked up a > 1-year old series here to make another
attempt at landing it upstream.  These patches have been in shipping
Chromebooks for the last year.  Note that v3 to v4 has no changes
other than a rebase and a small commit message update.

The first two patches extend the "init_card()" mechanism of MMC core
to actually be called for all card types, not just SDIO.  That could
be applied any time and should fix at least one longstanding bug
(untested).

The third patch is a cleanup patch to use init_card() to move things
around a bit so we don't need to handle SDIO cards in such a strange
place.  On earlier versions of this patch Seungwon brought up a few
points which I have _not_ addressed.  See
<https://patchwork.kernel.org/patch/3049071/>.  Other than talk of
cards with out of band interrupts maybe being able to gate their
clocks, he wanted to use MMC_QUIRK_BROKEN_CLK_GATING.  I didn't do
that because of the ordering of init_card() and when the quirks are
set.  Some users of init_card() like pandora_wl1251_init_card() rely
on it being called very early in the process.
pandora_wl1251_init_card() hardcodes a vendor and device and thus need
to be called super early.  On the other hand the code that adds quirks
_reads_ the vendor and device.  It can't possibly move before
init_card().  If folks are willing to take an additional host op of
init_card_late() I can certainly go that way, though.

The fourth patch is (I think) reviewed and ready to go assuming the
other two land.

Changes in v5:
- Split fixup to pandora_wl1251_init_card() into its own patch.

Changes in v3:
- Add fixup to pandora_wl1251_init_card().

Changes in v2:
- mmc core change new for this version.
- Fixed "|" to "&".
- intmask_lock renamed to irq_lock

Doug Anderson (4):
  ARM: OMAP2+: Make sure pandora_wl1251_init_card() applies to SDIO only
  mmc: core: Support the optional init_card() callback for MMC and SD
  mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts
  mmc: dw_mmc: Protect read-modify-write of INTMASK with a lock

 arch/arm/mach-omap2/board-omap3pandora.c | 14 +++---
 drivers/mmc/core/mmc.c                   |  6 +++
 drivers/mmc/core/sd.c                    |  7 ++-
 drivers/mmc/host/dw_mmc.c                | 80 +++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h                |  1 +
 include/linux/mmc/dw_mmc.h               |  6 +++
 6 files changed, 74 insertions(+), 40 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH v5 2/4] mmc: core: Support the optional init_card() callback for MMC and SD

[Doug Anderson]

In (3fcb027 ARM: MXC: mxcmmc: work around a bug in the SDHC busy line
handling) the optional init_card() callback was added.  According to
the original change it was "for now only called from
mmc_sdio_init_card()".

This callback really ought to be called from the SD and MMC init
functions as well.  One current user of this callback
(mxcmci_init_card) will not work as expected if you insert an SDIO
card, then eject it and put a normal SD card in.  Specifically the
normal SD card will not get to run with 4-bit data.

I'd like to use the init_card() callback to handle a similar quirk on
dw_mmc when using SDIO Interrupts (the "low power" feature of the card
needs to be disabled), so that will add a second user of the function.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
Changes in v5:
- Split fixup to pandora_wl1251_init_card() into its own patch.

Changes in v3:
- Add fixup to pandora_wl1251_init_card().

Changes in v2:
- mmc core change new for this version.

 drivers/mmc/core/mmc.c | 6 ++++++
 drivers/mmc/core/sd.c  | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 02ad792..4a21d66 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1297,6 +1297,12 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	}
 
 	/*
+	 * Call the optional HC's init_card function to handle quirks.
+	 */
+	if (host->ops->init_card)
+		host->ops->init_card(host, card);
+
+	/*
 	 * For native busses:  set card RCA and quit open drain mode.
 	 */
 	if (!mmc_host_is_spi(host)) {
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index d90a6de..29fccdc 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -933,6 +933,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	}
 
 	/*
+	 * Call the optional HC's init_card function to handle quirks.
+	 */
+	if (host->ops->init_card)
+		host->ops->init_card(host, card);
+
+	/*
 	 * For native busses:  get card RCA and quit open drain mode.
 	 */
 	if (!mmc_host_is_spi(host)) {
@@ -1271,4 +1277,3 @@ err:
 
 	return err;
 }
-
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Doug Anderson]

In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO
interrupts are used) I added code that disabled the low power mode of
dw_mmc when SDIO interrupts are used.  That code worked but always
felt a little hacky because we ended up disabling low power as a side
effect of the first enable_sdio_irq() call.  That wouldn't be so bad
except that disabling low power involves a complicated process of
writing to the CMD/CMDARG registers and that extra process makes it
difficult to cleanly the read-modify-write race in
dw_mci_enable_sdio_irq() (see future patch in the series).

Change the code to take advantage of the init_card() callback of the
mmc core to do this right at bootup.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v3: None
Changes in v2:
- Fixed "|" to "&".

 drivers/mmc/host/dw_mmc.c | 69 ++++++++++++++++++++++++-----------------------
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67c0451..ae10a02 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -27,6 +27,7 @@
 #include <linux/stat.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
+#include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/sd.h>
@@ -926,7 +927,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
 
 		/* enable clock; only low power if no SDIO */
 		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
-		if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->sdio_id)))
+		if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags))
 			clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
 		mci_writel(host, CLKENA, clk_en_a);
 
@@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	return present;
 }
 
-/*
- * Disable lower power mode.
- *
- * Low power mode will stop the card clock when idle.  According to the
- * description of the CLKENA register we should disable low power mode
- * for SDIO cards if we need SDIO interrupts to work.
- *
- * This function is fast if low power mode is already disabled.
- */
-static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
+static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
 {
+	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
-	u32 clk_en_a;
-	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
 
-	clk_en_a = mci_readl(host, CLKENA);
+	/*
+	 * Low power mode will stop the card clock when idle.  According to the
+	 * description of the CLKENA register we should disable low power mode
+	 * for SDIO cards if we need SDIO interrupts to work.
+	 */
+	if (mmc->caps & MMC_CAP_SDIO_IRQ) {
+		const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+		u32 clk_en_a_old;
+		u32 clk_en_a;
 
-	if (clk_en_a & clken_low_pwr) {
-		mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
-		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
-			     SDMMC_CMD_PRV_DAT_WAIT, 0);
+		clk_en_a_old = mci_readl(host, CLKENA);
+
+		if (card->type == MMC_TYPE_SDIO ||
+		    card->type == MMC_TYPE_SD_COMBO) {
+			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			clk_en_a = clk_en_a_old & ~clken_low_pwr;
+		} else {
+			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			clk_en_a = clk_en_a_old | clken_low_pwr;
+		}
+
+		if (clk_en_a != clk_en_a_old) {
+			mci_writel(host, CLKENA, clk_en_a);
+			mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+				     SDMMC_CMD_PRV_DAT_WAIT, 0);
+		}
 	}
 }
 
@@ -1277,21 +1288,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 
 	/* Enable/disable Slot Specific SDIO interrupt */
 	int_mask = mci_readl(host, INTMASK);
-	if (enb) {
-		/*
-		 * Turn off low power mode if it was enabled.  This is a bit of
-		 * a heavy operation and we disable / enable IRQs a lot, so
-		 * we'll leave low power mode disabled and it will get
-		 * re-enabled again in dw_mci_setup_bus().
-		 */
-		dw_mci_disable_low_power(slot);
-
-		mci_writel(host, INTMASK,
-			   (int_mask | SDMMC_INT_SDIO(slot->sdio_id)));
-	} else {
-		mci_writel(host, INTMASK,
-			   (int_mask & ~SDMMC_INT_SDIO(slot->sdio_id)));
-	}
+	if (enb)
+		int_mask |= SDMMC_INT_SDIO(slot->sdio_id);
+	else
+		int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
+	mci_writel(host, INTMASK, int_mask);
 }
 
 static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
@@ -1337,7 +1338,7 @@ static const struct mmc_host_ops dw_mci_ops = {
 	.execute_tuning		= dw_mci_execute_tuning,
 	.card_busy		= dw_mci_card_busy,
 	.start_signal_voltage_switch = dw_mci_switch_voltage,
-
+	.init_card		= dw_mci_init_card,
 };
 
 static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq)
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 0d0f7a2..d27d4c0 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -244,6 +244,7 @@ struct dw_mci_slot {
 	unsigned long		flags;
 #define DW_MMC_CARD_PRESENT	0
 #define DW_MMC_CARD_NEED_INIT	1
+#define DW_MMC_CARD_NO_LOW_PWR	2
 	int			id;
 	int			sdio_id;
 };
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v5 4/4] mmc: dw_mmc: Protect read-modify-write of INTMASK with a lock

[Doug Anderson]

We're running into cases where our enabling of the SDIO interrupt in
dw_mmc doesn't actually take effect.  Specifically, adding patch like
this:

 +++ b/drivers/mmc/host/dw_mmc.c
 @@ -1076,6 +1076,9 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)

      mci_writel(host, INTMASK,
           (int_mask | SDMMC_INT_SDIO(slot->id)));
 +    int_mask = mci_readl(host, INTMASK);
 +    if (!(int_mask & SDMMC_INT_SDIO(slot->id)))
 +      dev_err(&mmc->class_dev, "failed to enable sdio irq\n");
    } else {

...actually triggers the error message.  That's because the
dw_mci_enable_sdio_irq() unsafely does a read-modify-write of the
INTMASK register.

We can't just use the standard host->lock since that lock is not irq
safe and mmc_signal_sdio_irq() (called from interrupt context) calls
dw_mci_enable_sdio_irq().  Add a new irq-safe lock to protect INTMASK.

An alternate solution to this is to punt mmc_signal_sdio_irq() to the
tasklet and then protect INTMASK modifications by the standard host
lock.  This seemed like a bit more of a high-latency change.

Reported-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
Changes in v5: None
Changes in v3: None
Changes in v2:
- intmask_lock renamed to irq_lock

 drivers/mmc/host/dw_mmc.c  | 13 +++++++++++++
 include/linux/mmc/dw_mmc.h |  6 ++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ae10a02..64ea042 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -759,6 +759,7 @@ disable:
 
 static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
 {
+	unsigned long irqflags;
 	int sg_len;
 	u32 temp;
 
@@ -795,9 +796,11 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
 	mci_writel(host, CTRL, temp);
 
 	/* Disable RX/TX IRQs, let DMA handle it */
+	spin_lock_irqsave(&host->irq_lock, irqflags);
 	temp = mci_readl(host, INTMASK);
 	temp  &= ~(SDMMC_INT_RXDR | SDMMC_INT_TXDR);
 	mci_writel(host, INTMASK, temp);
+	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 
 	host->dma_ops->start(host, sg_len);
 
@@ -806,6 +809,7 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
 
 static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
 {
+	unsigned long irqflags;
 	u32 temp;
 
 	data->error = -EINPROGRESS;
@@ -834,9 +838,12 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
 		host->part_buf_count = 0;
 
 		mci_writel(host, RINTSTS, SDMMC_INT_TXDR | SDMMC_INT_RXDR);
+
+		spin_lock_irqsave(&host->irq_lock, irqflags);
 		temp = mci_readl(host, INTMASK);
 		temp |= SDMMC_INT_TXDR | SDMMC_INT_RXDR;
 		mci_writel(host, INTMASK, temp);
+		spin_unlock_irqrestore(&host->irq_lock, irqflags);
 
 		temp = mci_readl(host, CTRL);
 		temp &= ~SDMMC_CTRL_DMA_ENABLE;
@@ -1284,8 +1291,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
+	unsigned long irqflags;
 	u32 int_mask;
 
+	spin_lock_irqsave(&host->irq_lock, irqflags);
+
 	/* Enable/disable Slot Specific SDIO interrupt */
 	int_mask = mci_readl(host, INTMASK);
 	if (enb)
@@ -1293,6 +1303,8 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	else
 		int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
 	mci_writel(host, INTMASK, int_mask);
+
+	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 }
 
 static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
@@ -2661,6 +2673,7 @@ int dw_mci_probe(struct dw_mci *host)
 	host->quirks = host->pdata->quirks;
 
 	spin_lock_init(&host->lock);
+	spin_lock_init(&host->irq_lock);
 	INIT_LIST_HEAD(&host->queue);
 
 	/*
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 42b724e..471fb31 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -106,6 +106,11 @@ struct mmc_data;
  * @cur_slot, @mrq and @state. These must always be updated
  * at the same time while holding @lock.
  *
+ * @irq_lock is an irq-safe spinlock protecting the INTMASK register
+ * to allow the interrupt handler to modify it directly.  Held for only long
+ * enough to read-modify-write INTMASK and no other locks are grabbed when
+ * holding this one.
+ *
  * The @mrq field of struct dw_mci_slot is also protected by @lock,
  * and must always be written at the same time as the slot is added to
  * @queue.
@@ -125,6 +130,7 @@ struct mmc_data;
  */
 struct dw_mci {
 	spinlock_t		lock;
+	spinlock_t		irq_lock;
 	void __iomem		*regs;
 
 	struct scatterlist	*sg;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Jaehoon Chung]

Hi, Doug.

On 12/03/2014 08:42 AM, Doug Anderson wrote:
> In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO
> interrupts are used) I added code that disabled the low power mode of
> dw_mmc when SDIO interrupts are used.  That code worked but always
> felt a little hacky because we ended up disabling low power as a side
> effect of the first enable_sdio_irq() call.  That wouldn't be so bad
> except that disabling low power involves a complicated process of
> writing to the CMD/CMDARG registers and that extra process makes it
> difficult to cleanly the read-modify-write race in
> dw_mci_enable_sdio_irq() (see future patch in the series).
> 
> Change the code to take advantage of the init_card() callback of the
> mmc core to do this right at bootup.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v5: None
> Changes in v3: None
> Changes in v2:
> - Fixed "|" to "&".
> 
>  drivers/mmc/host/dw_mmc.c | 69 ++++++++++++++++++++++++-----------------------
>  drivers/mmc/host/dw_mmc.h |  1 +
>  2 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 67c0451..ae10a02 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -27,6 +27,7 @@
>  #include <linux/stat.h>
>  #include <linux/delay.h>
>  #include <linux/irq.h>
> +#include <linux/mmc/card.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/mmc/sd.h>
> @@ -926,7 +927,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>  
>  		/* enable clock; only low power if no SDIO */
>  		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
> -		if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->sdio_id)))
> +		if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags))
>  			clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
>  		mci_writel(host, CLKENA, clk_en_a);
>  
> @@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	return present;
>  }
>  
> -/*
> - * Disable lower power mode.
> - *
> - * Low power mode will stop the card clock when idle.  According to the
> - * description of the CLKENA register we should disable low power mode
> - * for SDIO cards if we need SDIO interrupts to work.
> - *
> - * This function is fast if low power mode is already disabled.
> - */
> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>  {
> +	struct dw_mci_slot *slot = mmc_priv(mmc);
>  	struct dw_mci *host = slot->host;
> -	u32 clk_en_a;
> -	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>  
> -	clk_en_a = mci_readl(host, CLKENA);
> +	/*
> +	 * Low power mode will stop the card clock when idle.  According to the
> +	 * description of the CLKENA register we should disable low power mode
> +	 * for SDIO cards if we need SDIO interrupts to work.
> +	 */
> +	if (mmc->caps & MMC_CAP_SDIO_IRQ) {
> +		const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> +		u32 clk_en_a_old;
> +		u32 clk_en_a;
>  
> -	if (clk_en_a & clken_low_pwr) {
> -		mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> -		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> -			     SDMMC_CMD_PRV_DAT_WAIT, 0);
> +		clk_en_a_old = mci_readl(host, CLKENA);
> +
> +		if (card->type == MMC_TYPE_SDIO ||
> +		    card->type == MMC_TYPE_SD_COMBO) {
> +			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			clk_en_a = clk_en_a_old & ~clken_low_pwr;
> +		} else {

I wonder this point. When entered at this point?

MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't?

Best Regards,
Jaehoon Chung

> +			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			clk_en_a = clk_en_a_old | clken_low_pwr;
> +		}
> +
> +		if (clk_en_a != clk_en_a_old) {
> +			mci_writel(host, CLKENA, clk_en_a);
> +			mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> +				     SDMMC_CMD_PRV_DAT_WAIT, 0);
> +		}
>  	}
>  }
>  
> @@ -1277,21 +1288,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  
>  	/* Enable/disable Slot Specific SDIO interrupt */
>  	int_mask = mci_readl(host, INTMASK);
> -	if (enb) {
> -		/*
> -		 * Turn off low power mode if it was enabled.  This is a bit of
> -		 * a heavy operation and we disable / enable IRQs a lot, so
> -		 * we'll leave low power mode disabled and it will get
> -		 * re-enabled again in dw_mci_setup_bus().
> -		 */
> -		dw_mci_disable_low_power(slot);
> -
> -		mci_writel(host, INTMASK,
> -			   (int_mask | SDMMC_INT_SDIO(slot->sdio_id)));
> -	} else {
> -		mci_writel(host, INTMASK,
> -			   (int_mask & ~SDMMC_INT_SDIO(slot->sdio_id)));
> -	}
> +	if (enb)
> +		int_mask |= SDMMC_INT_SDIO(slot->sdio_id);
> +	else
> +		int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
> +	mci_writel(host, INTMASK, int_mask);
>  }
>  
>  static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> @@ -1337,7 +1338,7 @@ static const struct mmc_host_ops dw_mci_ops = {
>  	.execute_tuning		= dw_mci_execute_tuning,
>  	.card_busy		= dw_mci_card_busy,
>  	.start_signal_voltage_switch = dw_mci_switch_voltage,
> -
> +	.init_card		= dw_mci_init_card,
>  };
>  
>  static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq)
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 0d0f7a2..d27d4c0 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -244,6 +244,7 @@ struct dw_mci_slot {
>  	unsigned long		flags;
>  #define DW_MMC_CARD_PRESENT	0
>  #define DW_MMC_CARD_NEED_INIT	1
> +#define DW_MMC_CARD_NO_LOW_PWR	2
>  	int			id;
>  	int			sdio_id;
>  };
> 

Re: [PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Doug Anderson]

Jaehoon,

On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> @@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>       return present;
>>  }
>>
>> -/*
>> - * Disable lower power mode.
>> - *
>> - * Low power mode will stop the card clock when idle.  According to the
>> - * description of the CLKENA register we should disable low power mode
>> - * for SDIO cards if we need SDIO interrupts to work.
>> - *
>> - * This function is fast if low power mode is already disabled.
>> - */
>> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
>> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>  {
>> +     struct dw_mci_slot *slot = mmc_priv(mmc);
>>       struct dw_mci *host = slot->host;
>> -     u32 clk_en_a;
>> -     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>
>> -     clk_en_a = mci_readl(host, CLKENA);
>> +     /*
>> +      * Low power mode will stop the card clock when idle.  According to the
>> +      * description of the CLKENA register we should disable low power mode
>> +      * for SDIO cards if we need SDIO interrupts to work.
>> +      */
>> +     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>> +             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>> +             u32 clk_en_a_old;
>> +             u32 clk_en_a;
>>
>> -     if (clk_en_a & clken_low_pwr) {
>> -             mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
>> -             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>> -                          SDMMC_CMD_PRV_DAT_WAIT, 0);
>> +             clk_en_a_old = mci_readl(host, CLKENA);
>> +
>> +             if (card->type == MMC_TYPE_SDIO ||
>> +                 card->type == MMC_TYPE_SD_COMBO) {
>> +                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> +                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>> +             } else {
>
> I wonder this point. When entered at this point?
>
> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't?

Right.  As I understand it:

* You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that
might have an MMC card, and SD card, or an SDIO card.  You still want
the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in
that case.

* If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some
problem where the SDIO interrupt is broken on your board.  That means
you're using polling mode to find interrupts.  As I understand it that
would allow you to use LOW_PWR since you don't need to detect SDIO
interrupts.  I know on Marvell WiFi modules I tested this seemed to
work OK, but I doubt anyone is really running a production device in
this way.


Basically I was trying to mirror the functionality of the old code as
closely as possible.  In the old code if you were using polling mode
then dw_mci_enable_sdio_irq() should never be called and you'd never
go out of low power mode.

-Doug

Re: [PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Jaehoon Chung]

Hi Doug

On 12/03/2014 09:36 AM, Doug Anderson wrote:
> Jaehoon,
> 
> On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> @@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>       return present;
>>>  }
>>>
>>> -/*
>>> - * Disable lower power mode.
>>> - *
>>> - * Low power mode will stop the card clock when idle.  According to the
>>> - * description of the CLKENA register we should disable low power mode
>>> - * for SDIO cards if we need SDIO interrupts to work.
>>> - *
>>> - * This function is fast if low power mode is already disabled.
>>> - */
>>> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
>>> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>  {
>>> +     struct dw_mci_slot *slot = mmc_priv(mmc);
>>>       struct dw_mci *host = slot->host;
>>> -     u32 clk_en_a;
>>> -     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>
>>> -     clk_en_a = mci_readl(host, CLKENA);
>>> +     /*
>>> +      * Low power mode will stop the card clock when idle.  According to the
>>> +      * description of the CLKENA register we should disable low power mode
>>> +      * for SDIO cards if we need SDIO interrupts to work.
>>> +      */
>>> +     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>>> +             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>> +             u32 clk_en_a_old;
>>> +             u32 clk_en_a;
>>>
>>> -     if (clk_en_a & clken_low_pwr) {
>>> -             mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
>>> -             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>> -                          SDMMC_CMD_PRV_DAT_WAIT, 0);
>>> +             clk_en_a_old = mci_readl(host, CLKENA);
>>> +
>>> +             if (card->type == MMC_TYPE_SDIO ||
>>> +                 card->type == MMC_TYPE_SD_COMBO) {
>>> +                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>> +                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>> +             } else {
>>
>> I wonder this point. When entered at this point?
>>
>> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't?
> 
> Right.  As I understand it:
> 
> * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that
> might have an MMC card, and SD card, or an SDIO card.  You still want
> the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in
> that case.

Ok, I understood your purpose.
Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card.
It's reasonable.
But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode.
And SDIO case should not enter at here.

> 
> * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some
> problem where the SDIO interrupt is broken on your board.  That means
> you're using polling mode to find interrupts.  As I understand it that
> would allow you to use LOW_PWR since you don't need to detect SDIO
> interrupts.  I know on Marvell WiFi modules I tested this seemed to
> work OK, but I doubt anyone is really running a production device in
> this way.

No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem.
Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card.
Then it would be check the twice whether card is SDIO or not.

Actually, i didn't test WiFi module with this patch. but concept is not problem.

Best Regards,
Jaehoon Chung

> 
> 
> Basically I was trying to mirror the functionality of the old code as
> closely as possible.  In the old code if you were using polling mode
> then dw_mci_enable_sdio_irq() should never be called and you'd never
> go out of low power mode.
> 
> -Doug
> 

Re: [PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Doug Anderson]

Jaehoon,

On Tue, Dec 2, 2014 at 5:06 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Doug
>
> On 12/03/2014 09:36 AM, Doug Anderson wrote:
>> Jaehoon,
>>
>> On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> @@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>>       return present;
>>>>  }
>>>>
>>>> -/*
>>>> - * Disable lower power mode.
>>>> - *
>>>> - * Low power mode will stop the card clock when idle.  According to the
>>>> - * description of the CLKENA register we should disable low power mode
>>>> - * for SDIO cards if we need SDIO interrupts to work.
>>>> - *
>>>> - * This function is fast if low power mode is already disabled.
>>>> - */
>>>> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
>>>> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>>  {
>>>> +     struct dw_mci_slot *slot = mmc_priv(mmc);
>>>>       struct dw_mci *host = slot->host;
>>>> -     u32 clk_en_a;
>>>> -     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>>
>>>> -     clk_en_a = mci_readl(host, CLKENA);
>>>> +     /*
>>>> +      * Low power mode will stop the card clock when idle.  According to the
>>>> +      * description of the CLKENA register we should disable low power mode
>>>> +      * for SDIO cards if we need SDIO interrupts to work.
>>>> +      */
>>>> +     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>>>> +             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>> +             u32 clk_en_a_old;
>>>> +             u32 clk_en_a;
>>>>
>>>> -     if (clk_en_a & clken_low_pwr) {
>>>> -             mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
>>>> -             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>>> -                          SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> +             clk_en_a_old = mci_readl(host, CLKENA);
>>>> +
>>>> +             if (card->type == MMC_TYPE_SDIO ||
>>>> +                 card->type == MMC_TYPE_SD_COMBO) {
>>>> +                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>> +                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>>> +             } else {
>>>
>>> I wonder this point. When entered at this point?
>>>
>>> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't?
>>
>> Right.  As I understand it:
>>
>> * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that
>> might have an MMC card, and SD card, or an SDIO card.  You still want
>> the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in
>> that case.
>
> Ok, I understood your purpose.
> Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card.
> It's reasonable.
> But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode.
> And SDIO case should not enter at here.

Are you sure?  What about if you plug in an SDIO card, then unplug it,
then plug in an SD card?

>> * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some
>> problem where the SDIO interrupt is broken on your board.  That means
>> you're using polling mode to find interrupts.  As I understand it that
>> would allow you to use LOW_PWR since you don't need to detect SDIO
>> interrupts.  I know on Marvell WiFi modules I tested this seemed to
>> work OK, but I doubt anyone is really running a production device in
>> this way.
>
> No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem.
> Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card.
> Then it would be check the twice whether card is SDIO or not.
>
> Actually, i didn't test WiFi module with this patch. but concept is not problem.

Yup, I think you were only thinking about the embedded case where you
know that an SDIO card will be plugged into the slot.  In the case
where you have an exposed slot it is certainly possible to expect
someone might plug in an SD card or an SDIO card.  ...or even a combo
card!

-Doug

Re: [PATCH v5 3/4] mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts

[Jaehoon Chung]

Doug.

On 12/03/2014 10:12 AM, Doug Anderson wrote:
> Jaehoon,
> 
> On Tue, Dec 2, 2014 at 5:06 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Doug
>>
>> On 12/03/2014 09:36 AM, Doug Anderson wrote:
>>> Jaehoon,
>>>
>>> On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>> @@ -1245,27 +1246,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>>>       return present;
>>>>>  }
>>>>>
>>>>> -/*
>>>>> - * Disable lower power mode.
>>>>> - *
>>>>> - * Low power mode will stop the card clock when idle.  According to the
>>>>> - * description of the CLKENA register we should disable low power mode
>>>>> - * for SDIO cards if we need SDIO interrupts to work.
>>>>> - *
>>>>> - * This function is fast if low power mode is already disabled.
>>>>> - */
>>>>> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
>>>>> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>>>  {
>>>>> +     struct dw_mci_slot *slot = mmc_priv(mmc);
>>>>>       struct dw_mci *host = slot->host;
>>>>> -     u32 clk_en_a;
>>>>> -     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>>>
>>>>> -     clk_en_a = mci_readl(host, CLKENA);
>>>>> +     /*
>>>>> +      * Low power mode will stop the card clock when idle.  According to the
>>>>> +      * description of the CLKENA register we should disable low power mode
>>>>> +      * for SDIO cards if we need SDIO interrupts to work.
>>>>> +      */
>>>>> +     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
>>>>> +             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>>>>> +             u32 clk_en_a_old;
>>>>> +             u32 clk_en_a;
>>>>>
>>>>> -     if (clk_en_a & clken_low_pwr) {
>>>>> -             mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
>>>>> -             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>>>>> -                          SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>>> +             clk_en_a_old = mci_readl(host, CLKENA);
>>>>> +
>>>>> +             if (card->type == MMC_TYPE_SDIO ||
>>>>> +                 card->type == MMC_TYPE_SD_COMBO) {
>>>>> +                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>>>>> +                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
>>>>> +             } else {
>>>>
>>>> I wonder this point. When entered at this point?
>>>>
>>>> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't?
>>>
>>> Right.  As I understand it:
>>>
>>> * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that
>>> might have an MMC card, and SD card, or an SDIO card.  You still want
>>> the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in
>>> that case.
>>
>> Ok, I understood your purpose.
>> Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card.
>> It's reasonable.
>> But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode.
>> And SDIO case should not enter at here.
> 
> Are you sure?  What about if you plug in an SDIO card, then unplug it,
> then plug in an SD card?

Oh.. :) I missed this case. thank you for pointing out.
I considered only non-removable case, it's stupid...

Best Regards,
Jaehoon Chung

> 
>>> * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some
>>> problem where the SDIO interrupt is broken on your board.  That means
>>> you're using polling mode to find interrupts.  As I understand it that
>>> would allow you to use LOW_PWR since you don't need to detect SDIO
>>> interrupts.  I know on Marvell WiFi modules I tested this seemed to
>>> work OK, but I doubt anyone is really running a production device in
>>> this way.
>>
>> No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem.
>> Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card.
>> Then it would be check the twice whether card is SDIO or not.
>>
>> Actually, i didn't test WiFi module with this patch. but concept is not problem.
> 
> Yup, I think you were only thinking about the embedded case where you
> know that an SDIO card will be plugged into the slot.  In the case
> where you have an exposed slot it is certainly possible to expect
> someone might plug in an SD card or an SDIO card.  ...or even a combo
> card!
> 
> -Doug
> 

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Ulf Hansson]

On 3 December 2014 at 00:42, Doug Anderson <dianders@chromium.org> wrote:
> Bing Zhao at Marvell found a problem with dw_mmc where interrupts
> weren't firing sometimes.  He tracked it down to a read-modify-write
> problem with the INTMASK.  These patches fix the problem.
>
> Note: I've picked up a > 1-year old series here to make another
> attempt at landing it upstream.  These patches have been in shipping
> Chromebooks for the last year.  Note that v3 to v4 has no changes
> other than a rebase and a small commit message update.
>
> The first two patches extend the "init_card()" mechanism of MMC core
> to actually be called for all card types, not just SDIO.  That could
> be applied any time and should fix at least one longstanding bug
> (untested).
>
> The third patch is a cleanup patch to use init_card() to move things
> around a bit so we don't need to handle SDIO cards in such a strange
> place.  On earlier versions of this patch Seungwon brought up a few
> points which I have _not_ addressed.  See
> <https://patchwork.kernel.org/patch/3049071/>.  Other than talk of
> cards with out of band interrupts maybe being able to gate their
> clocks, he wanted to use MMC_QUIRK_BROKEN_CLK_GATING.  I didn't do
> that because of the ordering of init_card() and when the quirks are
> set.  Some users of init_card() like pandora_wl1251_init_card() rely
> on it being called very early in the process.
> pandora_wl1251_init_card() hardcodes a vendor and device and thus need
> to be called super early.  On the other hand the code that adds quirks
> _reads_ the vendor and device.  It can't possibly move before
> init_card().  If folks are willing to take an additional host op of
> init_card_late() I can certainly go that way, though.
>
> The fourth patch is (I think) reviewed and ready to go assuming the
> other two land.

I have queued this up for 3.20. It was a bit hard to follow the
updated the revisions, please don't send patches "in-reply-to" for
future sets.

In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.

Kind regards
Uffe

>
> Changes in v5:
> - Split fixup to pandora_wl1251_init_card() into its own patch.
>
> Changes in v3:
> - Add fixup to pandora_wl1251_init_card().
>
> Changes in v2:
> - mmc core change new for this version.
> - Fixed "|" to "&".
> - intmask_lock renamed to irq_lock
>
> Doug Anderson (4):
>   ARM: OMAP2+: Make sure pandora_wl1251_init_card() applies to SDIO only
>   mmc: core: Support the optional init_card() callback for MMC and SD
>   mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts
>   mmc: dw_mmc: Protect read-modify-write of INTMASK with a lock
>
>  arch/arm/mach-omap2/board-omap3pandora.c | 14 +++---
>  drivers/mmc/core/mmc.c                   |  6 +++
>  drivers/mmc/core/sd.c                    |  7 ++-
>  drivers/mmc/host/dw_mmc.c                | 80 +++++++++++++++++++-------------
>  drivers/mmc/host/dw_mmc.h                |  1 +
>  include/linux/mmc/dw_mmc.h               |  6 +++
>  6 files changed, 74 insertions(+), 40 deletions(-)
>
> --
> 2.2.0.rc0.207.ga3a616c
>

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Doug Anderson]

Ulf,

On Fri, Dec 19, 2014 at 2:17 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 3 December 2014 at 00:42, Doug Anderson <dianders@chromium.org> wrote:
>> Bing Zhao at Marvell found a problem with dw_mmc where interrupts
>> weren't firing sometimes.  He tracked it down to a read-modify-write
>> problem with the INTMASK.  These patches fix the problem.
>>
>> Note: I've picked up a > 1-year old series here to make another
>> attempt at landing it upstream.  These patches have been in shipping
>> Chromebooks for the last year.  Note that v3 to v4 has no changes
>> other than a rebase and a small commit message update.
>>
>> The first two patches extend the "init_card()" mechanism of MMC core
>> to actually be called for all card types, not just SDIO.  That could
>> be applied any time and should fix at least one longstanding bug
>> (untested).
>>
>> The third patch is a cleanup patch to use init_card() to move things
>> around a bit so we don't need to handle SDIO cards in such a strange
>> place.  On earlier versions of this patch Seungwon brought up a few
>> points which I have _not_ addressed.  See
>> <https://patchwork.kernel.org/patch/3049071/>.  Other than talk of
>> cards with out of band interrupts maybe being able to gate their
>> clocks, he wanted to use MMC_QUIRK_BROKEN_CLK_GATING.  I didn't do
>> that because of the ordering of init_card() and when the quirks are
>> set.  Some users of init_card() like pandora_wl1251_init_card() rely
>> on it being called very early in the process.
>> pandora_wl1251_init_card() hardcodes a vendor and device and thus need
>> to be called super early.  On the other hand the code that adds quirks
>> _reads_ the vendor and device.  It can't possibly move before
>> init_card().  If folks are willing to take an additional host op of
>> init_card_late() I can certainly go that way, though.
>>
>> The fourth patch is (I think) reviewed and ready to go assuming the
>> other two land.
>
> I have queued this up for 3.20.

Thanks!


> It was a bit hard to follow the
> updated the revisions, please don't send patches "in-reply-to" for
> future sets.

Very strange.  I didn't send out anything in-reply-to other than what
git-send-email usually does.  I believe I had:

[0] - no in reply to.
 [1] - in reply to [0]
 [2] - in reply to [0]
 [3] - in reply to [0]
 [4] - in reply to [0]

Is there some other way you'd prefer?

Looking full headers in <https://patchwork.kernel.org/patch/5425241/>,
I confirm it is "in-reply-to"
"1417563767-32181-1-git-send-email-dianders@chromium.org".  Patchwork
doesn't keep cover letters, but you can see at
<http://www.spinics.net/lists/linux-mmc/msg29699.html>) that there is
no in-reply-to.

I'm more than happy to adjust my workflow if you can give me some
specifics.  Thanks!  :)


> In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.

Ah, maybe because it wasn't sent to linux-mmc?  I messed that up and
will try to do better in the future.  Sorry.  :(  You were in the To
line, though.  You can see at
<https://patchwork.kernel.org/patch/5425241/>.

Do you want me to repost it and CC linux-mmc with Tony's Ack?

---

Note: patchwork seems to find all my patches:

pwclient list -w dianders@chromium.org -p ""

5425241 New          [v5,1/4] ARM: OMAP2+: Make sure
pandora_wl1251_init_card() applies to SDIO only
5425291 New          [v5,1/4] ARM: OMAP2+: Make sure
pandora_wl1251_init_card() applies to SDIO only
5425311 New          [v5,1/4] ARM: OMAP2+: Make sure
pandora_wl1251_init_card() applies to SDIO only
5425231 New          [v5,2/4] mmc: core: Support the optional
init_card() callback for MMC and SD
5425301 New          [v5,2/4] mmc: core: Support the optional
init_card() callback for MMC and SD
5425271 New          [v5,3/4] mmc: dw_mmc: Cleanup disable of low
power mode w/ SDIO interrupts
5425281 New          [v5,3/4] mmc: dw_mmc: Cleanup disable of low
power mode w/ SDIO interrupts
5425251 New          [v5,4/4] mmc: dw_mmc: Protect read-modify-write
of INTMASK with a lock
5425261 New          [v5,4/4] mmc: dw_mmc: Protect read-modify-write
of INTMASK with a lock

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Ulf Hansson]

On 19 December 2014 at 20:02, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Fri, Dec 19, 2014 at 2:17 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 3 December 2014 at 00:42, Doug Anderson <dianders@chromium.org> wrote:
>>> Bing Zhao at Marvell found a problem with dw_mmc where interrupts
>>> weren't firing sometimes.  He tracked it down to a read-modify-write
>>> problem with the INTMASK.  These patches fix the problem.
>>>
>>> Note: I've picked up a > 1-year old series here to make another
>>> attempt at landing it upstream.  These patches have been in shipping
>>> Chromebooks for the last year.  Note that v3 to v4 has no changes
>>> other than a rebase and a small commit message update.
>>>
>>> The first two patches extend the "init_card()" mechanism of MMC core
>>> to actually be called for all card types, not just SDIO.  That could
>>> be applied any time and should fix at least one longstanding bug
>>> (untested).
>>>
>>> The third patch is a cleanup patch to use init_card() to move things
>>> around a bit so we don't need to handle SDIO cards in such a strange
>>> place.  On earlier versions of this patch Seungwon brought up a few
>>> points which I have _not_ addressed.  See
>>> <https://patchwork.kernel.org/patch/3049071/>.  Other than talk of
>>> cards with out of band interrupts maybe being able to gate their
>>> clocks, he wanted to use MMC_QUIRK_BROKEN_CLK_GATING.  I didn't do
>>> that because of the ordering of init_card() and when the quirks are
>>> set.  Some users of init_card() like pandora_wl1251_init_card() rely
>>> on it being called very early in the process.
>>> pandora_wl1251_init_card() hardcodes a vendor and device and thus need
>>> to be called super early.  On the other hand the code that adds quirks
>>> _reads_ the vendor and device.  It can't possibly move before
>>> init_card().  If folks are willing to take an additional host op of
>>> init_card_late() I can certainly go that way, though.
>>>
>>> The fourth patch is (I think) reviewed and ready to go assuming the
>>> other two land.
>>
>> I have queued this up for 3.20.
>
> Thanks!
>
>
>> It was a bit hard to follow the
>> updated the revisions, please don't send patches "in-reply-to" for
>> future sets.
>
> Very strange.  I didn't send out anything in-reply-to other than what
> git-send-email usually does.  I believe I had:
>
> [0] - no in reply to.
>  [1] - in reply to [0]
>  [2] - in reply to [0]
>  [3] - in reply to [0]
>  [4] - in reply to [0]

That's good. As long as there are no in-reply to previous versions of
patches/patchsets.

I am using gmails web-based client so it could very well be that it
does some magic, which I am not yet aware of.

>
> Is there some other way you'd prefer?
>
> Looking full headers in <https://patchwork.kernel.org/patch/5425241/>,
> I confirm it is "in-reply-to"
> "1417563767-32181-1-git-send-email-dianders@chromium.org".  Patchwork
> doesn't keep cover letters, but you can see at
> <http://www.spinics.net/lists/linux-mmc/msg29699.html>) that there is
> no in-reply-to.
>
> I'm more than happy to adjust my workflow if you can give me some
> specifics.  Thanks!  :)
>
>
>> In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.
>
> Ah, maybe because it wasn't sent to linux-mmc?  I messed that up and
> will try to do better in the future.  Sorry.  :(  You were in the To
> line, though.  You can see at
> <https://patchwork.kernel.org/patch/5425241/>.
>
> Do you want me to repost it and CC linux-mmc with Tony's Ack?

I suggest you have a look at my next branch and to verify that I
haven't screwed things up. If so, either I should drop the patches and
you make a resend or if it's possible to just send an incremental path
on top?

Kind regards
Uffe

>
> ---
>
> Note: patchwork seems to find all my patches:
>
> pwclient list -w dianders@chromium.org -p ""
>
> 5425241 New          [v5,1/4] ARM: OMAP2+: Make sure
> pandora_wl1251_init_card() applies to SDIO only
> 5425291 New          [v5,1/4] ARM: OMAP2+: Make sure
> pandora_wl1251_init_card() applies to SDIO only
> 5425311 New          [v5,1/4] ARM: OMAP2+: Make sure
> pandora_wl1251_init_card() applies to SDIO only
> 5425231 New          [v5,2/4] mmc: core: Support the optional
> init_card() callback for MMC and SD
> 5425301 New          [v5,2/4] mmc: core: Support the optional
> init_card() callback for MMC and SD
> 5425271 New          [v5,3/4] mmc: dw_mmc: Cleanup disable of low
> power mode w/ SDIO interrupts
> 5425281 New          [v5,3/4] mmc: dw_mmc: Cleanup disable of low
> power mode w/ SDIO interrupts
> 5425251 New          [v5,4/4] mmc: dw_mmc: Protect read-modify-write
> of INTMASK with a lock
> 5425261 New          [v5,4/4] mmc: dw_mmc: Protect read-modify-write
> of INTMASK with a lock

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Javier Martinez Canillas]

Hello Ulf,

On Tue, Dec 30, 2014 at 11:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 December 2014 at 20:02, Doug Anderson <dianders@chromium.org> wrote:
>>
>>> It was a bit hard to follow the
>>> updated the revisions, please don't send patches "in-reply-to" for
>>> future sets.
>>
>> Very strange.  I didn't send out anything in-reply-to other than what
>> git-send-email usually does.  I believe I had:
>>
>> [0] - no in reply to.
>>  [1] - in reply to [0]
>>  [2] - in reply to [0]
>>  [3] - in reply to [0]
>>  [4] - in reply to [0]
>
> That's good. As long as there are no in-reply to previous versions of
> patches/patchsets.
>
> I am using gmails web-based client so it could very well be that it
> does some magic, which I am not yet aware of.
>

I think that you were confused because Gmail's "conversation view"
groups emails in a thread not by the email subject but by the email
body.

So if a new revision of a series is sent and a patch did not change
from the previous version, Gmail will add it to the old patch thread
since the body is basically the same even when a version is present in
the subject (e.g: PATCH v2 foo).

I found that annoying as well so I disabled conversation view going to
Gmail's settings -> General -> Conversation view off

Hope it helps.

Best regards,
Javier

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Doug Anderson]

Ulf,

On Tue, Dec 30, 2014 at 2:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.
>>
>> Ah, maybe because it wasn't sent to linux-mmc?  I messed that up and
>> will try to do better in the future.  Sorry.  :(  You were in the To
>> line, though.  You can see at
>> <https://patchwork.kernel.org/patch/5425241/>.
>>
>> Do you want me to repost it and CC linux-mmc with Tony's Ack?
>
> I suggest you have a look at my next branch and to verify that I
> haven't screwed things up. If so, either I should drop the patches and
> you make a resend or if it's possible to just send an incremental path
> on top?

The patches that landed look good to me, but I think you might break
omap3pandora's WiFi until you land patch #1 in the series.  It has
Tony's Ack so I think he intends for you to land it in your tree.

[v5,1/4] ARM: OMAP2+: Make sure pandora_wl1251_init_card() applies to SDIO only

-Doug

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Tony Lindgren]

* Doug Anderson <dianders@chromium.org> [150102 09:09]:
> Ulf,
> 
> On Tue, Dec 30, 2014 at 2:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>> In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.
> >>
> >> Ah, maybe because it wasn't sent to linux-mmc?  I messed that up and
> >> will try to do better in the future.  Sorry.  :(  You were in the To
> >> line, though.  You can see at
> >> <https://patchwork.kernel.org/patch/5425241/>.
> >>
> >> Do you want me to repost it and CC linux-mmc with Tony's Ack?
> >
> > I suggest you have a look at my next branch and to verify that I
> > haven't screwed things up. If so, either I should drop the patches and
> > you make a resend or if it's possible to just send an incremental path
> > on top?
> 
> The patches that landed look good to me, but I think you might break
> omap3pandora's WiFi until you land patch #1 in the series.  It has
> Tony's Ack so I think he intends for you to land it in your tree.
> 
> [v5,1/4] ARM: OMAP2+: Make sure pandora_wl1251_init_card() applies to SDIO only

Yes please pick that one up too to avoid breakage. The changes to the
mach-omap2/board-*.c files are only minimal fixes, so it should not
conflict with anything I'll be applying.

Regards,

Tony

Re: [PATCH v5 0/4] Fixes for SDIO interrupts for dw_mmc

[Ulf Hansson]

On 2 January 2015 at 18:11, Tony Lindgren <tony@atomide.com> wrote:
> * Doug Anderson <dianders@chromium.org> [150102 09:09]:
>> Ulf,
>>
>> On Tue, Dec 30, 2014 at 2:29 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >>> In v5, I don't find a patch 1/4. Anyway, I have taken patch 2->4.
>> >>
>> >> Ah, maybe because it wasn't sent to linux-mmc?  I messed that up and
>> >> will try to do better in the future.  Sorry.  :(  You were in the To
>> >> line, though.  You can see at
>> >> <https://patchwork.kernel.org/patch/5425241/>.
>> >>
>> >> Do you want me to repost it and CC linux-mmc with Tony's Ack?
>> >
>> > I suggest you have a look at my next branch and to verify that I
>> > haven't screwed things up. If so, either I should drop the patches and
>> > you make a resend or if it's possible to just send an incremental path
>> > on top?
>>
>> The patches that landed look good to me, but I think you might break
>> omap3pandora's WiFi until you land patch #1 in the series.  It has
>> Tony's Ack so I think he intends for you to land it in your tree.
>>
>> [v5,1/4] ARM: OMAP2+: Make sure pandora_wl1251_init_card() applies to SDIO only
>
> Yes please pick that one up too to avoid breakage. The changes to the
> mach-omap2/board-*.c files are only minimal fixes, so it should not
> conflict with anything I'll be applying.

Tony, Doug,

I have picked it up from the arm patch tracker and applied it for my
next branch. I have put it prior the following patch:
"mmc: core: Support the optional init_card() callback for MMC and SD".

Kind regards
Uffe

>
> Regards,
>
> Tony