* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 7:53 ` Hans de Goede
0 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-25 7:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 7:53 ` Hans de Goede
@ 2015-09-25 9:37 ` Jaehoon Chung
-1 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 9:37 UTC (permalink / raw)
To: Hans de Goede, Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Alim Akhtar,
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 9:37 ` Jaehoon Chung
0 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 9:37 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 9:37 ` Jaehoon Chung
@ 2015-09-25 9:41 ` Hans de Goede
-1 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-25 9:41 UTC (permalink / raw)
To: Jaehoon Chung, Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Alim Akhtar
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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 9:41 ` Hans de Goede
0 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-25 9:41 UTC (permalink / raw)
To: linux-arm-kernel
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
>>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 9:41 ` Hans de Goede
@ 2015-09-25 9:58 ` Jaehoon Chung
-1 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 9:58 UTC (permalink / raw)
To: Hans de Goede, Jaehoon Chung, Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Alim Akhtar
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@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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 9:58 ` Jaehoon Chung
0 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 9:58 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 9:58 ` Jaehoon Chung
@ 2015-09-25 10:06 ` Hans de Goede
-1 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-25 10:06 UTC (permalink / raw)
To: Jaehoon Chung, Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Alim Akhtar
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 10:06 ` Hans de Goede
0 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-25 10:06 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 10:06 ` Hans de Goede
@ 2015-09-25 10:23 ` Jaehoon Chung
-1 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 10:23 UTC (permalink / raw)
To: Hans de Goede, Jaehoon Chung, Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Alim Akhtar
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 10:23 ` Jaehoon Chung
0 siblings, 0 replies; 36+ messages in thread
From: Jaehoon Chung @ 2015-09-25 10:23 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 7:53 ` Hans de Goede
@ 2015-09-25 16:14 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2015-09-25 16:14 UTC (permalink / raw)
To: Hans de Goede
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Seungwon Jeon,
Alim Akhtar, Jaehoon Chung, Addy Ke
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-25 16:14 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2015-09-25 16:14 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-25 16:14 ` Doug Anderson
@ 2015-09-29 12:58 ` Hans de Goede
-1 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-29 12:58 UTC (permalink / raw)
To: Doug Anderson
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Seungwon Jeon,
Alim Akhtar, Jaehoon Chung, Addy Ke
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-29 12:58 ` Hans de Goede
0 siblings, 0 replies; 36+ messages in thread
From: Hans de Goede @ 2015-09-29 12:58 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
2015-09-29 12:58 ` Hans de Goede
@ 2015-09-29 17:38 ` Doug Anderson
-1 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2015-09-29 17:38 UTC (permalink / raw)
To: Hans de Goede
Cc: Ulf Hansson, Chris Ball, Arend van Spriel, Maxime Ripard,
linux-mmc, linux-arm-kernel@lists.infradead.org, Seungwon Jeon,
Alim Akhtar, Jaehoon Chung, Addy Ke
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/3] mmc: Wait for card_busy before starting sdio requests
@ 2015-09-29 17:38 ` Doug Anderson
0 siblings, 0 replies; 36+ messages in thread
From: Doug Anderson @ 2015-09-29 17:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 36+ messages in thread