* [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
@ 2025-07-24 10:58 Ben Dooks
2025-07-29 13:51 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Ben Dooks @ 2025-07-24 10:58 UTC (permalink / raw)
To: qemu-block, philmd, bmeng.cn; +Cc: qemu-devel, Ben Dooks
The addition of specific handlers for mmc-spi for SEND_CSD and
SEND_CID has broken at least Linux and possibly also u-boot's
mmc-spi code.
It looks like when adding the code, it is checking for these
commands to not be in sd_standby_state but the check looks to
have been accidentally reversed (see below)
if (sd->state != sd_standby_state) {
return sd_invalid_state_for_cmd(sd, req);
}
Linux shows the following:
[ 0.293983] Waiting for root device /dev/mmcblk0...
[ 1.363071] mmc0: error -38 whilst initialising SD card
[ 2.418566] mmc0: error -38 whilst initialising SD card
Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
hw/sd/sd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 49fc79cf8a..e6c1ba7c5d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
/* CMD9 */
static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
{
- if (sd->state != sd_standby_state) {
+ if (sd->state == sd_standby_state) {
return sd_invalid_state_for_cmd(sd, req);
}
return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1336,7 +1336,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
/* CMD10 */
static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
{
- if (sd->state != sd_standby_state) {
+ if (sd->state == sd_standby_state) {
return sd_invalid_state_for_cmd(sd, req);
}
return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1345,7 +1345,7 @@ static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
{
- if (sd->state != sd_standby_state) {
+ if (sd->state == sd_standby_state) {
return sd_invalid_state_for_cmd(sd, req);
}
--
2.37.2.352.g3c44437643
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
2025-07-24 10:58 [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check Ben Dooks
@ 2025-07-29 13:51 ` Philippe Mathieu-Daudé
2025-07-29 14:06 ` Guenter Roeck
2025-07-29 15:06 ` Ben Dooks
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-07-29 13:51 UTC (permalink / raw)
To: Ben Dooks, qemu-block, bmeng.cn; +Cc: qemu-devel, Guenter Roeck
Hi Ben,
On 24/7/25 12:58, Ben Dooks wrote:
> The addition of specific handlers for mmc-spi for SEND_CSD and
> SEND_CID has broken at least Linux and possibly also u-boot's
> mmc-spi code.
>
> It looks like when adding the code, it is checking for these
> commands to not be in sd_standby_state but the check looks to
> have been accidentally reversed (see below)
>
> if (sd->state != sd_standby_state) {
> return sd_invalid_state_for_cmd(sd, req);
> }
>
> Linux shows the following:
>
> [ 0.293983] Waiting for root device /dev/mmcblk0...
> [ 1.363071] mmc0: error -38 whilst initialising SD card
> [ 2.418566] mmc0: error -38 whilst initialising SD card
>
> Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> hw/sd/sd.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 49fc79cf8a..e6c1ba7c5d 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
> /* CMD9 */
> static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
> {
> - if (sd->state != sd_standby_state) {
> + if (sd->state == sd_standby_state) {
This happens to work in your case by luck.
Since we switch to sd_sendingdata_state in sd_cmd_to_sendingdata(),
we need to check for sd_transfer_state -- the spec is wrong here! --.
Btw sd_cmd_to_sendingdata() already checks for that.
> return sd_invalid_state_for_cmd(sd, req);
> }
> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1336,7 +1336,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
> /* CMD10 */
> static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
> {
> - if (sd->state != sd_standby_state) {
> + if (sd->state == sd_standby_state) {
Ditto.
> return sd_invalid_state_for_cmd(sd, req);
> }
> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1345,7 +1345,7 @@ static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
>
> static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
> {
> - if (sd->state != sd_standby_state) {
> + if (sd->state == sd_standby_state) {
Nack, you just broke SD card implementation. Have a look at
the "4.8 Card State Transition Table" in the spec.
> return sd_invalid_state_for_cmd(sd, req);
> }
>
Regards,
Phil.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
2025-07-29 13:51 ` Philippe Mathieu-Daudé
@ 2025-07-29 14:06 ` Guenter Roeck
2025-07-29 15:06 ` Ben Dooks
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2025-07-29 14:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Ben Dooks, qemu-block, bmeng.cn, qemu-devel
On Tue, Jul 29, 2025 at 03:51:33PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Ben,
>
> On 24/7/25 12:58, Ben Dooks wrote:
> > The addition of specific handlers for mmc-spi for SEND_CSD and
> > SEND_CID has broken at least Linux and possibly also u-boot's
> > mmc-spi code.
> >
> > It looks like when adding the code, it is checking for these
> > commands to not be in sd_standby_state but the check looks to
> > have been accidentally reversed (see below)
> >
> > if (sd->state != sd_standby_state) {
> > return sd_invalid_state_for_cmd(sd, req);
> > }
> >
> > Linux shows the following:
> >
> > [ 0.293983] Waiting for root device /dev/mmcblk0...
> > [ 1.363071] mmc0: error -38 whilst initialising SD card
> > [ 2.418566] mmc0: error -38 whilst initialising SD card
> >
> > Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)")
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > hw/sd/sd.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 49fc79cf8a..e6c1ba7c5d 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd, SDRequest req)
> > /* CMD9 */
> > static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
> > {
> > - if (sd->state != sd_standby_state) {
> > + if (sd->state == sd_standby_state) {
>
> This happens to work in your case by luck.
>
> Since we switch to sd_sendingdata_state in sd_cmd_to_sendingdata(),
> we need to check for sd_transfer_state -- the spec is wrong here! --.
>
> Btw sd_cmd_to_sendingdata() already checks for that.
>
In my fix (the one I am carrying downstream) I have
- if (sd->state != sd_standby_state) {
+ if (sd->state != sd_transfer_state) {
in spi_cmd_SEND_CSD() and spi_cmd_SEND_CID(), together with
- return sd_r2_s;
+ return sd_r1;
in sd_cmd_SEND_STATUS().
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
2025-07-29 13:51 ` Philippe Mathieu-Daudé
2025-07-29 14:06 ` Guenter Roeck
@ 2025-07-29 15:06 ` Ben Dooks
2025-07-29 16:35 ` Guenter Roeck
1 sibling, 1 reply; 6+ messages in thread
From: Ben Dooks @ 2025-07-29 15:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-block, bmeng.cn, qemu-devel, Guenter Roeck
On 2025-07-29 14:51, Philippe Mathieu-Daudé wrote:
> Hi Ben,
>
> On 24/7/25 12:58, Ben Dooks wrote:
>> The addition of specific handlers for mmc-spi for SEND_CSD and
>> SEND_CID has broken at least Linux and possibly also u-boot's
>> mmc-spi code.
>>
>> It looks like when adding the code, it is checking for these
>> commands to not be in sd_standby_state but the check looks to
>> have been accidentally reversed (see below)
>>
>> if (sd->state != sd_standby_state) {
>> return sd_invalid_state_for_cmd(sd, req);
>> }
>>
>> Linux shows the following:
>>
>> [ 0.293983] Waiting for root device /dev/mmcblk0...
>> [ 1.363071] mmc0: error -38 whilst initialising SD card
>> [ 2.418566] mmc0: error -38 whilst initialising SD card
>>
>> Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID
>> handlers (CMD9 & CMD10)")
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> hw/sd/sd.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 49fc79cf8a..e6c1ba7c5d 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -1317,7 +1317,7 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState
>> *sd, SDRequest req)
>> /* CMD9 */
>> static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
>> {
>> - if (sd->state != sd_standby_state) {
>> + if (sd->state == sd_standby_state) {
>
> This happens to work in your case by luck.
>
> Since we switch to sd_sendingdata_state in sd_cmd_to_sendingdata(),
> we need to check for sd_transfer_state -- the spec is wrong here! --.
>
> Btw sd_cmd_to_sendingdata() already checks for that.
>
>
>> return sd_invalid_state_for_cmd(sd, req);
>> }
>> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd,
>> req),
>> @@ -1336,7 +1336,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState
>> *sd, SDRequest req)
>> /* CMD10 */
>> static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
>> {
>> - if (sd->state != sd_standby_state) {
>> + if (sd->state == sd_standby_state) {
>
> Ditto.
>
>> return sd_invalid_state_for_cmd(sd, req);
>> }
>> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd,
>> req),
>> @@ -1345,7 +1345,7 @@ static sd_rsp_type_t spi_cmd_SEND_CID(SDState
>> *sd, SDRequest req)
>> static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
>> {
>> - if (sd->state != sd_standby_state) {
>> + if (sd->state == sd_standby_state) {
>
> Nack, you just broke SD card implementation. Have a look at
> the "4.8 Card State Transition Table" in the spec.
>
>> return sd_invalid_state_for_cmd(sd, req);
>> }
>>
> Regards,
> Phil.
ok, so what is the correct fix? the sd-spi has been broken for some
time.
out of office at the moment so may not be able to test anything until
5th August
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
2025-07-29 15:06 ` Ben Dooks
@ 2025-07-29 16:35 ` Guenter Roeck
2025-07-31 21:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2025-07-29 16:35 UTC (permalink / raw)
To: Ben Dooks; +Cc: Philippe Mathieu-Daudé, qemu-block, bmeng.cn, qemu-devel
On Tue, Jul 29, 2025 at 04:06:33PM +0100, Ben Dooks wrote:
>
>
> On 2025-07-29 14:51, Philippe Mathieu-Daudé wrote:
> > Hi Ben,
> >
> > On 24/7/25 12:58, Ben Dooks wrote:
> > > The addition of specific handlers for mmc-spi for SEND_CSD and
> > > SEND_CID has broken at least Linux and possibly also u-boot's
> > > mmc-spi code.
> > >
> > > It looks like when adding the code, it is checking for these
> > > commands to not be in sd_standby_state but the check looks to
> > > have been accidentally reversed (see below)
> > >
> > > if (sd->state != sd_standby_state) {
> > > return sd_invalid_state_for_cmd(sd, req);
> > > }
> > >
> > > Linux shows the following:
> > >
> > > [ 0.293983] Waiting for root device /dev/mmcblk0...
> > > [ 1.363071] mmc0: error -38 whilst initialising SD card
> > > [ 2.418566] mmc0: error -38 whilst initialising SD card
> > >
> > > Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID
> > > handlers (CMD9 & CMD10)")
> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > > ---
> > > hw/sd/sd.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
...
>
> ok, so what is the correct fix? the sd-spi has been broken for some time.
>
FWIW, I use the patch below on top of upstream qemu. I have no idea if it
is correct, but it fixes the problem for me.
Guenter
---
From 87a2004005eb47758c524b54dd3fbc68a00e317f Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Thu, 24 Oct 2024 12:16:44 -0700
Subject: [PATCH] sd: Fix boot failures seen on sifive_u
sifive_u fails to boot from SD. This patch fixes the problem.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
hw/sd/sd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c275fdda2d..f5c44a4a86 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1520,7 +1520,7 @@ static sd_rsp_type_t emmc_cmd_SEND_EXT_CSD(SDState *sd, SDRequest req)
/* CMD9 */
static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
{
- if (sd->state != sd_standby_state) {
+ if (sd->state != sd_transfer_state) {
return sd_invalid_state_for_cmd(sd, req);
}
return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1539,7 +1539,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
/* CMD10 */
static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
{
- if (sd->state != sd_standby_state) {
+ if (sd->state != sd_transfer_state) {
return sd_invalid_state_for_cmd(sd, req);
}
return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
@@ -1592,7 +1592,7 @@ static sd_rsp_type_t sd_cmd_SEND_STATUS(SDState *sd, SDRequest req)
}
if (sd_is_spi(sd)) {
- return sd_r2_s;
+ return sd_r1;
}
return sd_req_rca_same(sd, req) ? sd_r1 : sd_r0;
--
2.45.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check
2025-07-29 16:35 ` Guenter Roeck
@ 2025-07-31 21:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-07-31 21:00 UTC (permalink / raw)
To: Guenter Roeck, Ben Dooks
Cc: qemu-block, bmeng.cn, qemu-devel, Peter Maydell, qemu-arm
On 29/7/25 18:35, Guenter Roeck wrote:
> On Tue, Jul 29, 2025 at 04:06:33PM +0100, Ben Dooks wrote:
>>
>>
>> On 2025-07-29 14:51, Philippe Mathieu-Daudé wrote:
>>> Hi Ben,
>>>
>>> On 24/7/25 12:58, Ben Dooks wrote:
>>>> The addition of specific handlers for mmc-spi for SEND_CSD and
>>>> SEND_CID has broken at least Linux and possibly also u-boot's
>>>> mmc-spi code.
>>>>
>>>> It looks like when adding the code, it is checking for these
>>>> commands to not be in sd_standby_state but the check looks to
>>>> have been accidentally reversed (see below)
>>>>
>>>> if (sd->state != sd_standby_state) {
>>>> return sd_invalid_state_for_cmd(sd, req);
>>>> }
>>>>
>>>> Linux shows the following:
>>>>
>>>> [ 0.293983] Waiting for root device /dev/mmcblk0...
>>>> [ 1.363071] mmc0: error -38 whilst initialising SD card
>>>> [ 2.418566] mmc0: error -38 whilst initialising SD card
>>>>
>>>> Fixes: da954d0e32444f122a4 ("hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID
>>>> handlers (CMD9 & CMD10)")
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>> ---
>>>> hw/sd/sd.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
> ...
>>
>> ok, so what is the correct fix? the sd-spi has been broken for some time.
I couldn't figure yet the problem with SEND_CSD and SEND_CID
*not* being in sd_standby_state, but I figured what was missing
with R2 return in SEND_STATUS. I'll post the series ASAP.
About "broken for some time", I was only testing the SPI mode with
the Gumstix machines, but we removed their support in commit a2ccff4d2bc
("hw/arm: Remove 'connex' and 'verdex' machines").
For older SPI testing, see:
https://lore.kernel.org/qemu-devel/2b825be1-bc57-49c2-d1c9-be83577e8ce1@amsat.org/
> FWIW, I use the patch below on top of upstream qemu. I have no idea if it
> is correct, but it fixes the problem for me.
>
> Guenter
>
> ---
> From 87a2004005eb47758c524b54dd3fbc68a00e317f Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Thu, 24 Oct 2024 12:16:44 -0700
> Subject: [PATCH] sd: Fix boot failures seen on sifive_u
>
> sifive_u fails to boot from SD. This patch fixes the problem.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> hw/sd/sd.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index c275fdda2d..f5c44a4a86 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1520,7 +1520,7 @@ static sd_rsp_type_t emmc_cmd_SEND_EXT_CSD(SDState *sd, SDRequest req)
> /* CMD9 */
> static sd_rsp_type_t spi_cmd_SEND_CSD(SDState *sd, SDRequest req)
> {
> - if (sd->state != sd_standby_state) {
> + if (sd->state != sd_transfer_state) {
> return sd_invalid_state_for_cmd(sd, req);
> }
> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1539,7 +1539,7 @@ static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
> /* CMD10 */
> static sd_rsp_type_t spi_cmd_SEND_CID(SDState *sd, SDRequest req)
> {
> - if (sd->state != sd_standby_state) {
> + if (sd->state != sd_transfer_state) {
> return sd_invalid_state_for_cmd(sd, req);
> }
> return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req),
> @@ -1592,7 +1592,7 @@ static sd_rsp_type_t sd_cmd_SEND_STATUS(SDState *sd, SDRequest req)
> }
>
> if (sd_is_spi(sd)) {
> - return sd_r2_s;
> + return sd_r1;
> }
>
> return sd_req_rca_same(sd, req) ? sd_r1 : sd_r0;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-31 21:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-24 10:58 [PATCH] hw/sd/sdcard: fix spi_cmd_SEND_CSD/CID state check Ben Dooks
2025-07-29 13:51 ` Philippe Mathieu-Daudé
2025-07-29 14:06 ` Guenter Roeck
2025-07-29 15:06 ` Ben Dooks
2025-07-29 16:35 ` Guenter Roeck
2025-07-31 21:00 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).