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 <hdego...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
