[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi Ulf,

Here is a non RFC version of my patch-set to wait for card_busy before
starting sdio requests. It is the same as the RFC version of the set,
but this time it has been tested no hardware which actually needs this
and I can confirm now that this fixes wifi on that hardware.

This patch-set should also allow removing this dw_mmc specific fix:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040

As this patch-set fixes this problem in a generic manner.

Regards,

Hans

[PATCH 1/3] mmc: Add mmc_is_io_op helper function

[Hans de Goede]

Add a helper function to check if an opcode is a sd-io-rw-* opcode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mmc/core/sdio_ops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index 12a4d3a..5660c7f 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -12,6 +12,8 @@
 #ifndef _MMC_SDIO_OPS_H
 #define _MMC_SDIO_OPS_H
 
+#include <linux/mmc/sdio.h>
+
 int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
 int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
 	unsigned addr, u8 in, u8* out);
@@ -19,5 +21,10 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
 int sdio_reset(struct mmc_host *host);
 
+static inline bool mmc_is_io_op(u32 opcode)
+{
+	return opcode == SD_IO_RW_DIRECT || opcode == SD_IO_RW_EXTENDED;
+}
+
 #endif
 
-- 
2.4.3

[PATCH 2/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Some sdio wifi chips will not work properly if we try to start new
sdio-rw requests while the device is signalling that it is busy.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mmc/core/core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0520064..ced66b8 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -204,6 +204,23 @@ static void __mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 		return;
 	}
 
+	/*
+	 * For sdio rw commands we must wait for card busy otherwise some
+	 * sdio devices won't work properly.
+	 */
+	if (mmc_is_io_op(mrq->cmd->opcode) && host->ops->card_busy) {
+		int tries = 500; /* Wait aprox 500ms at maximum */
+
+		while (host->ops->card_busy(host) && --tries)
+			mmc_delay(1);
+
+		if (tries == 0) {
+			mrq->cmd->error = -EBUSY;
+			mmc_request_done(host, mrq);
+			return;
+		}
+	}
+
 	host->ops->request(host, mrq);
 }
 
-- 
2.4.3

[PATCH 3/3] mmc: sunxi: Add card busy detection

[Hans de Goede]

Some sdio wifi modules have not been working reliable with the sunxi-mmc
host code. This turns out to be caused by starting new io-rw commands while
the card signals that it is still busy processing a previous command.

This commit adds card-busy detection to the sunxi-mmc driver which together
with recent core changes to check card-busy before starting io-rw commands
fixes the wifi reliability issues on the Cubietruck and other sunxi boards
using sdio wifi.

Reported-by: Eugene K <sigintmailru@gmail.com>
Suggested-by: Eugene K <sigintmailru@gmail.com>
Cc: Eugene K <sigintmailru@gmail.com>
Cc: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Properly accredit Eugene K for coming up with the fix for this
Changes in v3:
-Add card-busy detection and let the core decide when to actually use
 card-busy the added detection
---
 drivers/mmc/host/sunxi-mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index a7b7a67..e6226cd 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -868,6 +868,13 @@ static void sunxi_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	spin_unlock_irqrestore(&host->lock, iflags);
 }
 
+static int sunxi_mmc_card_busy(struct mmc_host *mmc)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	return !!(mmc_readl(host, REG_STAS) & SDXC_CARD_DATA_BUSY);
+}
+
 static const struct of_device_id sunxi_mmc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-mmc", },
 	{ .compatible = "allwinner,sun5i-a13-mmc", },
@@ -882,6 +889,7 @@ static struct mmc_host_ops sunxi_mmc_ops = {
 	.get_cd		 = mmc_gpio_get_cd,
 	.enable_sdio_irq = sunxi_mmc_enable_sdio_irq,
 	.hw_reset	 = sunxi_mmc_hw_reset,
+	.card_busy	 = sunxi_mmc_card_busy,
 };
 
 static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
-- 
2.4.3

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Ulf Hansson]

On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Ulf,
>
> Here is a non RFC version of my patch-set to wait for card_busy before
> starting sdio requests. It is the same as the RFC version of the set,
> but this time it has been tested no hardware which actually needs this
> and I can confirm now that this fixes wifi on that hardware.

Great! Thanks, applied for next!

>
> This patch-set should also allow removing this dw_mmc specific fix:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>
> As this patch-set fixes this problem in a generic manner.

Care to send a patch to remove the above hack/fix?

Kind regards
Uffe

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi,

On 23-09-15 23:43, Ulf Hansson wrote:
> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Ulf,
>>
>> Here is a non RFC version of my patch-set to wait for card_busy before
>> starting sdio requests. It is the same as the RFC version of the set,
>> but this time it has been tested no hardware which actually needs this
>> and I can confirm now that this fixes wifi on that hardware.
>
> Great! Thanks, applied for next!

Great, thanks, I guess it is too late for this to go as a fix into
4.3-rcX (no worries if it is) ?

>> This patch-set should also allow removing this dw_mmc specific fix:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>
>> As this patch-set fixes this problem in a generic manner.
>
> Care to send a patch to remove the above hack/fix?

I do not have any hardware to test this.

I've added Doug the original author of that patch to the Cc.

Dough, can you test if with the patch set from this mail thread
(merged into mmc/next) this patch:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040

Is still necessary ? Since this patch-set fixes the same issue
in the mmc core I believe that this commit can be reverted now.

Thanks & Regards,

Hans

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Arend van Spriel]

On 09/24/2015 11:19 AM, Hans de Goede wrote:
> Hi,
>
> On 23-09-15 23:43, Ulf Hansson wrote:
>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi Ulf,
>>>
>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>> starting sdio requests. It is the same as the RFC version of the set,
>>> but this time it has been tested no hardware which actually needs this
>>> and I can confirm now that this fixes wifi on that hardware.
>>
>> Great! Thanks, applied for next!
>
> Great, thanks, I guess it is too late for this to go as a fix into
> 4.3-rcX (no worries if it is) ?
>
>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>
>>>
>>> As this patch-set fixes this problem in a generic manner.
>>
>> Care to send a patch to remove the above hack/fix?
>
> I do not have any hardware to test this.
>
> I've added Doug the original author of that patch to the Cc.
>
> Dough, can you test if with the patch set from this mail thread
> (merged into mmc/next) this patch:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>
>
> Is still necessary ? Since this patch-set fixes the same issue
> in the mmc core I believe that this commit can be reverted now.

I have a chromebook which has this hack/patch (more or less) in 
chromeos. I can try and backport your patches and see how it goes.

Regards,
Arend

> Thanks & Regards,
>
> Hans

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Doug Anderson]

Hi,

On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 23-09-15 23:43, Ulf Hansson wrote:
>>
>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Ulf,
>>>
>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>> starting sdio requests. It is the same as the RFC version of the set,
>>> but this time it has been tested no hardware which actually needs this
>>> and I can confirm now that this fixes wifi on that hardware.
>>
>>
>> Great! Thanks, applied for next!
>
>
> Great, thanks, I guess it is too late for this to go as a fix into
> 4.3-rcX (no worries if it is) ?
>
>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>
>>> As this patch-set fixes this problem in a generic manner.
>>
>>
>> Care to send a patch to remove the above hack/fix?
>
>
> I do not have any hardware to test this.
>
> I've added Doug the original author of that patch to the Cc.
>
> Dough, can you test if with the patch set from this mail thread
> (merged into mmc/next) this patch:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>
> Is still necessary ? Since this patch-set fixes the same issue
> in the mmc core I believe that this commit can be reverted now.

I'll try to find some time in the next few days to test, but I'm not
terribly hopeful we can just revert the patch because:

1. Only one of the two callers of dw_mci_wait_while_busy() is handled
by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
something in the CMD register without going through the normal MMC
path.  This is used exclusively to update the clock registers in
dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
weird / awkward to me that you need to use the CMD register to update
clock settings in dw_mmc, but c'est la vie.

2. If I remember correctly, we ran into other instances where non-SDIO
cards needed the busy check.  It wasn't terribly common, but I think I
ran into this when stress testing, but only on a few cards.  The patch
referenced here only seems to check for SDIO commands.  As I
understand it, to be correct, it should check for all data commands
(other than stop or voltage change commands).  The Designware Databook
makes no reference to only needing the wait for SDIO commands.


...of course, it's always possible that some of the things I saw above
will no longer happen with all the other fixes we've done in the
meantime (turning on voltages at the right time, adding the right
delays, etc).


Note that I've hardly looked at sdhci at all, but on SDHCI is this
handled by the "SDHCI_DATA_INHIBIT" bits?


-Doug

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Jaehoon Chung]

Hi,

On 09/25/2015 01:04 AM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>
>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Ulf,
>>>>
>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>> but this time it has been tested no hardware which actually needs this
>>>> and I can confirm now that this fixes wifi on that hardware.
>>>
>>>
>>> Great! Thanks, applied for next!
>>
>>
>> Great, thanks, I guess it is too late for this to go as a fix into
>> 4.3-rcX (no worries if it is) ?
>>
>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>
>>>> As this patch-set fixes this problem in a generic manner.
>>>
>>>
>>> Care to send a patch to remove the above hack/fix?
>>
>>
>> I do not have any hardware to test this.
>>
>> I've added Doug the original author of that patch to the Cc.
>>
>> Dough, can you test if with the patch set from this mail thread
>> (merged into mmc/next) this patch:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>
>> Is still necessary ? Since this patch-set fixes the same issue
>> in the mmc core I believe that this commit can be reverted now.
> 
> I'll try to find some time in the next few days to test, but I'm not
> terribly hopeful we can just revert the patch because:
> 
> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
> something in the CMD register without going through the normal MMC
> path.  This is used exclusively to update the clock registers in
> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
> weird / awkward to me that you need to use the CMD register to update
> clock settings in dw_mmc, but c'est la vie.
> 
> 2. If I remember correctly, we ran into other instances where non-SDIO
> cards needed the busy check.  It wasn't terribly common, but I think I
> ran into this when stress testing, but only on a few cards.  The patch
> referenced here only seems to check for SDIO commands.  As I
> understand it, to be correct, it should check for all data commands
> (other than stop or voltage change commands).  The Designware Databook
> makes no reference to only needing the wait for SDIO commands.

I agreed absolutely for Doug's comment.

Best Regards,
Jaehoon Chung

> 
> 
> ...of course, it's always possible that some of the things I saw above
> will no longer happen with all the other fixes we've done in the
> meantime (turning on voltages at the right time, adding the right
> delays, etc).
> 
> 
> Note that I've hardly looked at sdhci at all, but on SDHCI is this
> handled by the "SDHCI_DATA_INHIBIT" bits?
> 
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi,

On 24-09-15 18:04, Doug Anderson wrote:
> Hi,
>
> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>
>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Ulf,
>>>>
>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>> but this time it has been tested no hardware which actually needs this
>>>> and I can confirm now that this fixes wifi on that hardware.
>>>
>>>
>>> Great! Thanks, applied for next!
>>
>>
>> Great, thanks, I guess it is too late for this to go as a fix into
>> 4.3-rcX (no worries if it is) ?
>>
>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>
>>>> As this patch-set fixes this problem in a generic manner.
>>>
>>>
>>> Care to send a patch to remove the above hack/fix?
>>
>>
>> I do not have any hardware to test this.
>>
>> I've added Doug the original author of that patch to the Cc.
>>
>> Dough, can you test if with the patch set from this mail thread
>> (merged into mmc/next) this patch:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>
>> Is still necessary ? Since this patch-set fixes the same issue
>> in the mmc core I believe that this commit can be reverted now.
>
> I'll try to find some time in the next few days to test, but I'm not
> terribly hopeful we can just revert the patch because:
>
> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
> something in the CMD register without going through the normal MMC
> path.  This is used exclusively to update the clock registers in
> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
> weird / awkward to me that you need to use the CMD register to update
> clock settings in dw_mmc, but c'est la vie.

I would not expect the card to signal busy when trying to change clocks
though, so I do not think this will really be a problem.

> 2. If I remember correctly, we ran into other instances where non-SDIO
> cards needed the busy check.  It wasn't terribly common, but I think I
> ran into this when stress testing, but only on a few cards.

Hmm, that would be a problem yes.

> The patch referenced here only seems to check for SDIO commands.  As I
> understand it, to be correct, it should check for all data commands
> (other than stop or voltage change commands).

But that is not what the patch does, it actually waits for all commands,
including non data commands. An earlier attempt of mine to fix the sdio
wifi issues with the sunxi driver copied your approach, and I actually
got reports of regressions with using normal micro-sd memory cards
from several people testing that patch.

And if you're right that we should wait for all data commands, then
I wonder if this is a designware thing (I believe the allwinner
mmc controller is designware derived) or a generic mmc / sdio thing ?

> The Designware Databook
> makes no reference to only needing the wait for SDIO commands.

Yet your commit message references problems with sdio wifi cards, and
on sunxi we've only been seeing this problem with sdio wifi cards / sdio
commands.

> ...of course, it's always possible that some of the things I saw above
> will no longer happen with all the other fixes we've done in the
> meantime (turning on voltages at the right time, adding the right
> delays, etc).
>
>
> Note that I've hardly looked at sdhci at all, but on SDHCI is this
> handled by the "SDHCI_DATA_INHIBIT" bits?

Regards,

Hans

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Jaehoon Chung]

Hi, Hans.

On 09/25/2015 04:53 PM, Hans de Goede wrote:
> Hi,
> 
> On 24-09-15 18:04, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>>
>>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi Ulf,
>>>>>
>>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>>> but this time it has been tested no hardware which actually needs this
>>>>> and I can confirm now that this fixes wifi on that hardware.
>>>>
>>>>
>>>> Great! Thanks, applied for next!
>>>
>>>
>>> Great, thanks, I guess it is too late for this to go as a fix into
>>> 4.3-rcX (no worries if it is) ?
>>>
>>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>>
>>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>
>>>>> As this patch-set fixes this problem in a generic manner.
>>>>
>>>>
>>>> Care to send a patch to remove the above hack/fix?
>>>
>>>
>>> I do not have any hardware to test this.
>>>
>>> I've added Doug the original author of that patch to the Cc.
>>>
>>> Dough, can you test if with the patch set from this mail thread
>>> (merged into mmc/next) this patch:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>
>>> Is still necessary ? Since this patch-set fixes the same issue
>>> in the mmc core I believe that this commit can be reverted now.
>>
>> I'll try to find some time in the next few days to test, but I'm not
>> terribly hopeful we can just revert the patch because:
>>
>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>> something in the CMD register without going through the normal MMC
>> path.  This is used exclusively to update the clock registers in
>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>> weird / awkward to me that you need to use the CMD register to update
>> clock settings in dw_mmc, but c'est la vie.
> 
> I would not expect the card to signal busy when trying to change clocks
> though, so I do not think this will really be a problem.

No. It shouldn't be occurred any problem.
But according to designware TRM, it needs to check whether card is busy or not, before updating clock.
I think even if problem will not occur, it doesn't mean this code is useless.

> 
>> 2. If I remember correctly, we ran into other instances where non-SDIO
>> cards needed the busy check.  It wasn't terribly common, but I think I
>> ran into this when stress testing, but only on a few cards.
> 
> Hmm, that would be a problem yes.
> 
>> The patch referenced here only seems to check for SDIO commands.  As I
>> understand it, to be correct, it should check for all data commands
>> (other than stop or voltage change commands).
> 
> But that is not what the patch does, it actually waits for all commands,
> including non data commands. An earlier attempt of mine to fix the sdio
> wifi issues with the sunxi driver copied your approach, and I actually
> got reports of regressions with using normal micro-sd memory cards
> from several people testing that patch.

I can't see any problem reported at mailing list.
Could you share more information what regressions issue?

Best Regards,
Jaehoon Chung

> 
> And if you're right that we should wait for all data commands, then
> I wonder if this is a designware thing (I believe the allwinner
> mmc controller is designware derived) or a generic mmc / sdio thing ?
> 
>> The Designware Databook
>> makes no reference to only needing the wait for SDIO commands.
> 
> Yet your commit message references problems with sdio wifi cards, and
> on sunxi we've only been seeing this problem with sdio wifi cards / sdio
> commands.
> 
>> ...of course, it's always possible that some of the things I saw above
>> will no longer happen with all the other fixes we've done in the
>> meantime (turning on voltages at the right time, adding the right
>> delays, etc).
>>
>>
>> Note that I've hardly looked at sdhci at all, but on SDHCI is this
>> handled by the "SDHCI_DATA_INHIBIT" bits?
> 
> Regards,
> 
> Hans
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi,

On 25-09-15 11:37, Jaehoon Chung wrote:
> Hi, Hans.
>
> On 09/25/2015 04:53 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 24-09-15 18:04, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>>>
>>>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>> Hi Ulf,
>>>>>>
>>>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>>>> but this time it has been tested no hardware which actually needs this
>>>>>> and I can confirm now that this fixes wifi on that hardware.
>>>>>
>>>>>
>>>>> Great! Thanks, applied for next!
>>>>
>>>>
>>>> Great, thanks, I guess it is too late for this to go as a fix into
>>>> 4.3-rcX (no worries if it is) ?
>>>>
>>>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>>>
>>>>>>
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>
>>>>>> As this patch-set fixes this problem in a generic manner.
>>>>>
>>>>>
>>>>> Care to send a patch to remove the above hack/fix?
>>>>
>>>>
>>>> I do not have any hardware to test this.
>>>>
>>>> I've added Doug the original author of that patch to the Cc.
>>>>
>>>> Dough, can you test if with the patch set from this mail thread
>>>> (merged into mmc/next) this patch:
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>
>>>> Is still necessary ? Since this patch-set fixes the same issue
>>>> in the mmc core I believe that this commit can be reverted now.
>>>
>>> I'll try to find some time in the next few days to test, but I'm not
>>> terribly hopeful we can just revert the patch because:
>>>
>>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>>> something in the CMD register without going through the normal MMC
>>> path.  This is used exclusively to update the clock registers in
>>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>>> weird / awkward to me that you need to use the CMD register to update
>>> clock settings in dw_mmc, but c'est la vie.
>>
>> I would not expect the card to signal busy when trying to change clocks
>> though, so I do not think this will really be a problem.
>
> No. It shouldn't be occurred any problem.
> But according to designware TRM, it needs to check whether card is busy or not, before updating clock.
> I think even if problem will not occur, it doesn't mean this code is useless.
>
>>
>>> 2. If I remember correctly, we ran into other instances where non-SDIO
>>> cards needed the busy check.  It wasn't terribly common, but I think I
>>> ran into this when stress testing, but only on a few cards.
>>
>> Hmm, that would be a problem yes.
>>
>>> The patch referenced here only seems to check for SDIO commands.  As I
>>> understand it, to be correct, it should check for all data commands
>>> (other than stop or voltage change commands).
>>
>> But that is not what the patch does, it actually waits for all commands,
>> including non data commands. An earlier attempt of mine to fix the sdio
>> wifi issues with the sunxi driver copied your approach, and I actually
>> got reports of regressions with using normal micro-sd memory cards
>> from several people testing that patch.
>
> I can't see any problem reported at mailing list.
> Could you share more information what regressions issue?

IIRC people where hitting the timeout in the code to wait for the card-busy.

Now that I think about this, this may have been caused by waiting
for card-busy while sending a stop.

Regards,

Hans



>
> Best Regards,
> Jaehoon Chung
>
>>
>> And if you're right that we should wait for all data commands, then
>> I wonder if this is a designware thing (I believe the allwinner
>> mmc controller is designware derived) or a generic mmc / sdio thing ?
>>
>>> The Designware Databook
>>> makes no reference to only needing the wait for SDIO commands.
>>
>> Yet your commit message references problems with sdio wifi cards, and
>> on sunxi we've only been seeing this problem with sdio wifi cards / sdio
>> commands.
>>
>>> ...of course, it's always possible that some of the things I saw above
>>> will no longer happen with all the other fixes we've done in the
>>> meantime (turning on voltages at the right time, adding the right
>>> delays, etc).
>>>
>>>
>>> Note that I've hardly looked at sdhci at all, but on SDHCI is this
>>> handled by the "SDHCI_DATA_INHIBIT" bits?
>>
>> Regards,
>>
>> Hans
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Jaehoon Chung]

On 09/25/2015 06:41 PM, Hans de Goede wrote:
> Hi,
> 
> On 25-09-15 11:37, Jaehoon Chung wrote:
>> Hi, Hans.
>>
>> On 09/25/2015 04:53 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 24-09-15 18:04, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>>>>
>>>>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>
>>>>>>> Hi Ulf,
>>>>>>>
>>>>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>>>>> but this time it has been tested no hardware which actually needs this
>>>>>>> and I can confirm now that this fixes wifi on that hardware.
>>>>>>
>>>>>>
>>>>>> Great! Thanks, applied for next!
>>>>>
>>>>>
>>>>> Great, thanks, I guess it is too late for this to go as a fix into
>>>>> 4.3-rcX (no worries if it is) ?
>>>>>
>>>>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>>>>
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>>
>>>>>>> As this patch-set fixes this problem in a generic manner.
>>>>>>
>>>>>>
>>>>>> Care to send a patch to remove the above hack/fix?
>>>>>
>>>>>
>>>>> I do not have any hardware to test this.
>>>>>
>>>>> I've added Doug the original author of that patch to the Cc.
>>>>>
>>>>> Dough, can you test if with the patch set from this mail thread
>>>>> (merged into mmc/next) this patch:
>>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>
>>>>> Is still necessary ? Since this patch-set fixes the same issue
>>>>> in the mmc core I believe that this commit can be reverted now.
>>>>
>>>> I'll try to find some time in the next few days to test, but I'm not
>>>> terribly hopeful we can just revert the patch because:
>>>>
>>>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>>>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>>>> something in the CMD register without going through the normal MMC
>>>> path.  This is used exclusively to update the clock registers in
>>>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>>>> weird / awkward to me that you need to use the CMD register to update
>>>> clock settings in dw_mmc, but c'est la vie.
>>>
>>> I would not expect the card to signal busy when trying to change clocks
>>> though, so I do not think this will really be a problem.
>>
>> No. It shouldn't be occurred any problem.
>> But according to designware TRM, it needs to check whether card is busy or not, before updating clock.
>> I think even if problem will not occur, it doesn't mean this code is useless.
>>
>>>
>>>> 2. If I remember correctly, we ran into other instances where non-SDIO
>>>> cards needed the busy check.  It wasn't terribly common, but I think I
>>>> ran into this when stress testing, but only on a few cards.
>>>
>>> Hmm, that would be a problem yes.
>>>
>>>> The patch referenced here only seems to check for SDIO commands.  As I
>>>> understand it, to be correct, it should check for all data commands
>>>> (other than stop or voltage change commands).
>>>
>>> But that is not what the patch does, it actually waits for all commands,
>>> including non data commands. An earlier attempt of mine to fix the sdio
>>> wifi issues with the sunxi driver copied your approach, and I actually
>>> got reports of regressions with using normal micro-sd memory cards
>>> from several people testing that patch.
>>
>> I can't see any problem reported at mailing list.
>> Could you share more information what regressions issue?
> 
> IIRC people where hitting the timeout in the code to wait for the card-busy.
> 
> Now that I think about this, this may have been caused by waiting
> for card-busy while sending a stop.

I understood what problem you said.
Well, but i don't accept your opinion yet.
Is that case reproduced with dwmmc controller? otherwise...sunxi driver?
(It seems that you mentioned the case of sunxi driver.)

Best Regards,
Jaehoon Chung

> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> And if you're right that we should wait for all data commands, then
>>> I wonder if this is a designware thing (I believe the allwinner
>>> mmc controller is designware derived) or a generic mmc / sdio thing ?
>>>
>>>> The Designware Databook
>>>> makes no reference to only needing the wait for SDIO commands.
>>>
>>> Yet your commit message references problems with sdio wifi cards, and
>>> on sunxi we've only been seeing this problem with sdio wifi cards / sdio
>>> commands.
>>>
>>>> ...of course, it's always possible that some of the things I saw above
>>>> will no longer happen with all the other fixes we've done in the
>>>> meantime (turning on voltages at the right time, adding the right
>>>> delays, etc).
>>>>
>>>>
>>>> Note that I've hardly looked at sdhci at all, but on SDHCI is this
>>>> handled by the "SDHCI_DATA_INHIBIT" bits?
>>>
>>> Regards,
>>>
>>> Hans
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi,

On 25-09-15 11:58, Jaehoon Chung wrote:
> On 09/25/2015 06:41 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 25-09-15 11:37, Jaehoon Chung wrote:
>>> Hi, Hans.
>>>
>>> On 09/25/2015 04:53 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 24-09-15 18:04, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>>>>>
>>>>>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>
>>>>>>>> Hi Ulf,
>>>>>>>>
>>>>>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>>>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>>>>>> but this time it has been tested no hardware which actually needs this
>>>>>>>> and I can confirm now that this fixes wifi on that hardware.
>>>>>>>
>>>>>>>
>>>>>>> Great! Thanks, applied for next!
>>>>>>
>>>>>>
>>>>>> Great, thanks, I guess it is too late for this to go as a fix into
>>>>>> 4.3-rcX (no worries if it is) ?
>>>>>>
>>>>>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>>>>>
>>>>>>>>
>>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>>>
>>>>>>>> As this patch-set fixes this problem in a generic manner.
>>>>>>>
>>>>>>>
>>>>>>> Care to send a patch to remove the above hack/fix?
>>>>>>
>>>>>>
>>>>>> I do not have any hardware to test this.
>>>>>>
>>>>>> I've added Doug the original author of that patch to the Cc.
>>>>>>
>>>>>> Dough, can you test if with the patch set from this mail thread
>>>>>> (merged into mmc/next) this patch:
>>>>>>
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>
>>>>>> Is still necessary ? Since this patch-set fixes the same issue
>>>>>> in the mmc core I believe that this commit can be reverted now.
>>>>>
>>>>> I'll try to find some time in the next few days to test, but I'm not
>>>>> terribly hopeful we can just revert the patch because:
>>>>>
>>>>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>>>>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>>>>> something in the CMD register without going through the normal MMC
>>>>> path.  This is used exclusively to update the clock registers in
>>>>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>>>>> weird / awkward to me that you need to use the CMD register to update
>>>>> clock settings in dw_mmc, but c'est la vie.
>>>>
>>>> I would not expect the card to signal busy when trying to change clocks
>>>> though, so I do not think this will really be a problem.
>>>
>>> No. It shouldn't be occurred any problem.
>>> But according to designware TRM, it needs to check whether card is busy or not, before updating clock.
>>> I think even if problem will not occur, it doesn't mean this code is useless.
>>>
>>>>
>>>>> 2. If I remember correctly, we ran into other instances where non-SDIO
>>>>> cards needed the busy check.  It wasn't terribly common, but I think I
>>>>> ran into this when stress testing, but only on a few cards.
>>>>
>>>> Hmm, that would be a problem yes.
>>>>
>>>>> The patch referenced here only seems to check for SDIO commands.  As I
>>>>> understand it, to be correct, it should check for all data commands
>>>>> (other than stop or voltage change commands).
>>>>
>>>> But that is not what the patch does, it actually waits for all commands,
>>>> including non data commands. An earlier attempt of mine to fix the sdio
>>>> wifi issues with the sunxi driver copied your approach, and I actually
>>>> got reports of regressions with using normal micro-sd memory cards
>>>> from several people testing that patch.
>>>
>>> I can't see any problem reported at mailing list.
>>> Could you share more information what regressions issue?
>>
>> IIRC people where hitting the timeout in the code to wait for the card-busy.
>>
>> Now that I think about this, this may have been caused by waiting
>> for card-busy while sending a stop.
>
> I understood what problem you said.
> Well, but i don't accept your opinion yet.
> Is that case reproduced with dwmmc controller? otherwise...sunxi driver?
> (It seems that you mentioned the case of sunxi driver.)

I was talking about the sunxi driver only. A few weeks back I did a
patch for the sunxi driver which copied the dw-mmc fix, and some users
testing that saw issues accessing certain micro-sd memory cards.

Regards,

Hans

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Jaehoon Chung]

On 09/25/2015 07:06 PM, Hans de Goede wrote:
> Hi,
> 
> On 25-09-15 11:58, Jaehoon Chung wrote:
>> On 09/25/2015 06:41 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 25-09-15 11:37, Jaehoon Chung wrote:
>>>> Hi, Hans.
>>>>
>>>> On 09/25/2015 04:53 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 24-09-15 18:04, Doug Anderson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Sep 24, 2015 at 2:19 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 23-09-15 23:43, Ulf Hansson wrote:
>>>>>>>>
>>>>>>>> On 22 September 2015 at 17:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Ulf,
>>>>>>>>>
>>>>>>>>> Here is a non RFC version of my patch-set to wait for card_busy before
>>>>>>>>> starting sdio requests. It is the same as the RFC version of the set,
>>>>>>>>> but this time it has been tested no hardware which actually needs this
>>>>>>>>> and I can confirm now that this fixes wifi on that hardware.
>>>>>>>>
>>>>>>>>
>>>>>>>> Great! Thanks, applied for next!
>>>>>>>
>>>>>>>
>>>>>>> Great, thanks, I guess it is too late for this to go as a fix into
>>>>>>> 4.3-rcX (no worries if it is) ?
>>>>>>>
>>>>>>>>> This patch-set should also allow removing this dw_mmc specific fix:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>>>>
>>>>>>>>> As this patch-set fixes this problem in a generic manner.
>>>>>>>>
>>>>>>>>
>>>>>>>> Care to send a patch to remove the above hack/fix?
>>>>>>>
>>>>>>>
>>>>>>> I do not have any hardware to test this.
>>>>>>>
>>>>>>> I've added Doug the original author of that patch to the Cc.
>>>>>>>
>>>>>>> Dough, can you test if with the patch set from this mail thread
>>>>>>> (merged into mmc/next) this patch:
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/dw_mmc.c?id=0bdbd0e88cf6b603a2196418672715b0890fb040
>>>>>>>
>>>>>>> Is still necessary ? Since this patch-set fixes the same issue
>>>>>>> in the mmc core I believe that this commit can be reverted now.
>>>>>>
>>>>>> I'll try to find some time in the next few days to test, but I'm not
>>>>>> terribly hopeful we can just revert the patch because:
>>>>>>
>>>>>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>>>>>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>>>>>> something in the CMD register without going through the normal MMC
>>>>>> path.  This is used exclusively to update the clock registers in
>>>>>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>>>>>> weird / awkward to me that you need to use the CMD register to update
>>>>>> clock settings in dw_mmc, but c'est la vie.
>>>>>
>>>>> I would not expect the card to signal busy when trying to change clocks
>>>>> though, so I do not think this will really be a problem.
>>>>
>>>> No. It shouldn't be occurred any problem.
>>>> But according to designware TRM, it needs to check whether card is busy or not, before updating clock.
>>>> I think even if problem will not occur, it doesn't mean this code is useless.
>>>>
>>>>>
>>>>>> 2. If I remember correctly, we ran into other instances where non-SDIO
>>>>>> cards needed the busy check.  It wasn't terribly common, but I think I
>>>>>> ran into this when stress testing, but only on a few cards.
>>>>>
>>>>> Hmm, that would be a problem yes.
>>>>>
>>>>>> The patch referenced here only seems to check for SDIO commands.  As I
>>>>>> understand it, to be correct, it should check for all data commands
>>>>>> (other than stop or voltage change commands).
>>>>>
>>>>> But that is not what the patch does, it actually waits for all commands,
>>>>> including non data commands. An earlier attempt of mine to fix the sdio
>>>>> wifi issues with the sunxi driver copied your approach, and I actually
>>>>> got reports of regressions with using normal micro-sd memory cards
>>>>> from several people testing that patch.
>>>>
>>>> I can't see any problem reported at mailing list.
>>>> Could you share more information what regressions issue?
>>>
>>> IIRC people where hitting the timeout in the code to wait for the card-busy.
>>>
>>> Now that I think about this, this may have been caused by waiting
>>> for card-busy while sending a stop.
>>
>> I understood what problem you said.
>> Well, but i don't accept your opinion yet.
>> Is that case reproduced with dwmmc controller? otherwise...sunxi driver?
>> (It seems that you mentioned the case of sunxi driver.)
> 
> I was talking about the sunxi driver only. A few weeks back I did a
> patch for the sunxi driver which copied the dw-mmc fix, and some users
> testing that saw issues accessing certain micro-sd memory cards.

Designware needs to check other MMC/SD card before sending command. :)
I will maintain Doug's patch for dwmmc controller.
If similar issue is occurred, then i will analyze codes at that time.
Maybe, your comment helps to me at that time.

Best Regards,
Jaehoon Chung

> 
> Regards,
> 
> Hans
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Doug Anderson]

Hi,

I think Jaehoon has already responded to much of this, but...

On Fri, Sep 25, 2015 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>> something in the CMD register without going through the normal MMC
>> path.  This is used exclusively to update the clock registers in
>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>> weird / awkward to me that you need to use the CMD register to update
>> clock settings in dw_mmc, but c'est la vie.
>
>
> I would not expect the card to signal busy when trying to change clocks
> though, so I do not think this will really be a problem.

I just can't quite remember what problem I was seeing.  Let's see...
I'll comment out that and see if I can find any errors.

OK, I commented it out and ran a bit of stress testing for the 4
devices / 4 cards sitting at my desk.  I saw no issues.  I also ran
some of the MMC tests that have caused me problems in the past and saw
no problems.  I also looked back at
<https://chromium-review.googlesource.com/#/c/244850/> where this
landed in our tree and I see that my comment is:

> Actually, while this works I may need to extend it to also be used for mci_send_cmd().

That indicates no problems on my end without the check before updating
clocks.  ...but, oh, I see why this is.  It was even posted upstream!
:)

https://patchwork.kernel.org/patch/5858221/

I said:

> Sorry for the quick spin.  After I posted this I re-read all the
> messages and it looks like Addy has an actual SD card (not SDIO) that
> is also asserting busy.  He's seeing it assert busy around the clock
> update.

 ...so I guess the answer here is that I personally haven't seen any
problems, but adding this check in mci_send_cmd() shouldn't hurt,
should be safe, and might avoid some problems.  Note that it's
possible that Addy was seeing some other bug somewhere that simply
resulted in the "busy" line being asserted, but technically the
Designware databook recommends waiting for "not busy" before updating
clocks IIRC.


>> 2. If I remember correctly, we ran into other instances where non-SDIO
>> cards needed the busy check.  It wasn't terribly common, but I think I
>> ran into this when stress testing, but only on a few cards.
>
>
> Hmm, that would be a problem yes.

So perhaps it would be good to update your patch to check for all data commands?


>> The patch referenced here only seems to check for SDIO commands.  As I
>> understand it, to be correct, it should check for all data commands
>> (other than stop or voltage change commands).
>
>
> But that is not what the patch does, it actually waits for all commands,
> including non data commands. An earlier attempt of mine to fix the sdio
> wifi issues with the sunxi driver copied your approach, and I actually
> got reports of regressions with using normal micro-sd memory cards
> from several people testing that patch.

You're saying that my patch waits for all commands including non-data
commands.  I'm pretty sure that's not true.  It relies on a whole
bunch of other logic in dw_mmc that figures out that we have a data
command (and that logic also looks for stop commands).  Specifically
my patches keys off SDMMC_CMD_PRV_DAT_WAIT.  Looking how that gets set
in dw_mci_prepare_command():
* We don't set it for the various "stop" commands
* Else we set it for all commands with cmd->data, except "send status"


> And if you're right that we should wait for all data commands, then
> I wonder if this is a designware thing (I believe the allwinner
> mmc controller is designware derived) or a generic mmc / sdio thing ?

It seems hard to believe it would be Designware specific.  If I
understand correctly, "busy" is signaled by the card holding the data
lines low.  ...so if a normal SD card was really asserting busy then
you'd better not send a command.


>> The Designware Databook
>> makes no reference to only needing the wait for SDIO commands.
>
>
> Yet your commit message references problems with sdio wifi cards, and
> on sunxi we've only been seeing this problem with sdio wifi cards / sdio
> commands.

Yup.  Though I did some amount in parallel, I definitely focused on
SDIO problems first before focusing on UHS SD cards.  ...so my primary
focus was on SDIO here but I tried to code it in a generic way so it
would be useful for all data commands (since it seemed like it could
technically affect them).


-Doug

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Hans de Goede]

Hi,

On 25-09-15 18:14, Doug Anderson wrote:
> Hi,
>
> I think Jaehoon has already responded to much of this, but...
>
> On Fri, Sep 25, 2015 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> 1. Only one of the two callers of dw_mci_wait_while_busy() is handled
>>> by your patch.  mci_send_cmd() is used internally in dw_mmc to throw
>>> something in the CMD register without going through the normal MMC
>>> path.  This is used exclusively to update the clock registers in
>>> dw_mmc.  I'm pretty sure this needs the wait, too.  It's always seemed
>>> weird / awkward to me that you need to use the CMD register to update
>>> clock settings in dw_mmc, but c'est la vie.
>>
>>
>> I would not expect the card to signal busy when trying to change clocks
>> though, so I do not think this will really be a problem.
>
> I just can't quite remember what problem I was seeing.  Let's see...
> I'll comment out that and see if I can find any errors.
>
> OK, I commented it out and ran a bit of stress testing for the 4
> devices / 4 cards sitting at my desk.  I saw no issues.  I also ran
> some of the MMC tests that have caused me problems in the past and saw
> no problems.  I also looked back at
> <https://chromium-review.googlesource.com/#/c/244850/> where this
> landed in our tree and I see that my comment is:
>
>> Actually, while this works I may need to extend it to also be used for mci_send_cmd().
>
> That indicates no problems on my end without the check before updating
> clocks.  ...but, oh, I see why this is.  It was even posted upstream!
> :)
>
> https://patchwork.kernel.org/patch/5858221/
>
> I said:
>
>> Sorry for the quick spin.  After I posted this I re-read all the
>> messages and it looks like Addy has an actual SD card (not SDIO) that
>> is also asserting busy.  He's seeing it assert busy around the clock
>> update.
>
>   ...so I guess the answer here is that I personally haven't seen any
> problems, but adding this check in mci_send_cmd() shouldn't hurt,
> should be safe, and might avoid some problems.  Note that it's
> possible that Addy was seeing some other bug somewhere that simply
> resulted in the "busy" line being asserted, but technically the
> Designware databook recommends waiting for "not busy" before updating
> clocks IIRC.

OK, so maybe we need to add a busy-check in the core before updating
the clocks, as you say below this stuff is likely not designware
specific ...

>>> 2. If I remember correctly, we ran into other instances where non-SDIO
>>> cards needed the busy check.  It wasn't terribly common, but I think I
>>> ran into this when stress testing, but only on a few cards.
>>
>>
>> Hmm, that would be a problem yes.
>
> So perhaps it would be good to update your patch to check for all data commands?

Agreed, since you seem to know this stuff much better then I do can you do
a follow-up patch expanding my patch to do the busy check / wait for
all data commands?

>>> The patch referenced here only seems to check for SDIO commands.  As I
>>> understand it, to be correct, it should check for all data commands
>>> (other than stop or voltage change commands).
>>
>>
>> But that is not what the patch does, it actually waits for all commands,
>> including non data commands. An earlier attempt of mine to fix the sdio
>> wifi issues with the sunxi driver copied your approach, and I actually
>> got reports of regressions with using normal micro-sd memory cards
>> from several people testing that patch.
>
> You're saying that my patch waits for all commands including non-data
> commands.  I'm pretty sure that's not true.

Ok I believe you, I was merely going by the commitdiff which adds
the checks unconditionally, but it may very well be that it is added
to a data only path.

> It relies on a whole
> bunch of other logic in dw_mmc that figures out that we have a data
> command (and that logic also looks for stop commands).  Specifically
> my patches keys off SDMMC_CMD_PRV_DAT_WAIT.  Looking how that gets set
> in dw_mci_prepare_command():
> * We don't set it for the various "stop" commands
> * Else we set it for all commands with cmd->data, except "send status"

Ack.

>> And if you're right that we should wait for all data commands, then
>> I wonder if this is a designware thing (I believe the allwinner
>> mmc controller is designware derived) or a generic mmc / sdio thing ?
>
> It seems hard to believe it would be Designware specific.  If I
> understand correctly, "busy" is signaled by the card holding the data
> lines low.  ...so if a normal SD card was really asserting busy then
> you'd better not send a command.

Agreed, so as said above, you seem to be more knowledgeable on this
then me, can you do a follow-up patch extending the check I added to
not just apply to mmc-io commands ?

>>> The Designware Databook
>>> makes no reference to only needing the wait for SDIO commands.
>>
>>
>> Yet your commit message references problems with sdio wifi cards, and
>> on sunxi we've only been seeing this problem with sdio wifi cards / sdio
>> commands.
>
> Yup.  Though I did some amount in parallel, I definitely focused on
> SDIO problems first before focusing on UHS SD cards.  ...so my primary
> focus was on SDIO here but I tried to code it in a generic way so it
> would be useful for all data commands (since it seemed like it could
> technically affect them).

Ok.

Regards,

Hans

[PATCH 0/3] mmc: Wait for card_busy before starting sdio requests

[Doug Anderson]

Hans,

On Tue, Sep 29, 2015 at 5:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> So perhaps it would be good to update your patch to check for all data
>> commands?
>
>
> Agreed, since you seem to know this stuff much better then I do can you do
> a follow-up patch expanding my patch to do the busy check / wait for
> all data commands?

Hmmm, what a strange thing to say about me.  ;)  I always kinda feel
like I'm bumbling around with most of this stuff.  If I've happened to
look at a particular issue in detail I know about it, but usually I
only know about the little bits that I've had to debug...  Really my
knowledge of SD/MMC/SDIO is limited to having to deal with dw_mmc
since all the SoCs I've worked with have had that controller.

I'll put it on my list to try to come up with a patch, but it might be
a while before I get to it since I've already god a whole lot of
"upstream wishlist" things that I haven't had time for.  My main work
keep promising to lighten up a little bit to pick some of these tasks
off.  If someone else is reading this and wants to take a crack at the
patch themselves, please don't wait for me.

-Doug