| Message ID | 20240628070216.92609-52-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/sd/sdcard: Add eMMC support | expand |
On 6/28/24 9:01 AM, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > hw/sd/sd.c | 50 ++++++++++++++++++++------------------------------ > 1 file changed, 20 insertions(+), 30 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index bd7c7cf518..564e08709b 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1323,6 +1323,15 @@ 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) { > + return sd_invalid_state_for_cmd(sd, req); > + } > + return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), > + sd->csd, 16); > +} > + > static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req) > { > if (sd->state != sd_standby_state) { > @@ -1333,6 +1342,15 @@ 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) { > + return sd_invalid_state_for_cmd(sd, req); > + } > + return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), > + sd->cid, 16); > +} > + > static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req) > { > if (sd->state != sd_standby_state) { > @@ -1408,36 +1426,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req) > > switch (req.cmd) { > /* Basic commands (Class 0 and Class 1) */ > - case 9: /* CMD9: SEND_CSD */ > - rca = sd_req_get_rca(sd, req); > - switch (sd->state) { > - case sd_transfer_state: > - if (!sd_is_spi(sd)) { > - break; > - } > - return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), > - sd->csd, 16); > - > - default: > - break; > - } > - break; > - > - case 10: /* CMD10: SEND_CID */ > - rca = sd_req_get_rca(sd, req); > - switch (sd->state) { > - case sd_transfer_state: > - if (!sd_is_spi(sd)) { > - break; > - } > - return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), > - sd->cid, 16); > - > - default: > - break; > - } > - break; > - > case 12: /* CMD12: STOP_TRANSMISSION */ > switch (sd->state) { > case sd_sendingdata_state: > @@ -2288,6 +2276,8 @@ static const SDProto sd_proto_spi = { > [5] = {9, sd_spi, "IO_SEND_OP_COND", sd_cmd_optional}, > [6] = {10, sd_spi, "SWITCH_FUNCTION", sd_cmd_SWITCH_FUNCTION}, > [8] = {0, sd_spi, "SEND_IF_COND", sd_cmd_SEND_IF_COND}, > + [9] = {0, sd_spi, "SEND_CSD", spi_cmd_SEND_CSD}, > + [10] = {0, sd_spi, "SEND_CID", spi_cmd_SEND_CID}, > [34] = {10, sd_spi, "READ_SEC_CMD", sd_cmd_optional}, > [35] = {10, sd_spi, "WRITE_SEC_CMD", sd_cmd_optional}, > [36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
Hi, On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Cédric Le Goater <clg@redhat.com> > --- This patch results in: [ 5.976133] Waiting for root device /dev/mmcblk0... [ 6.501462] mmc0: error -38 whilst initialising SD card [ 7.557473] mmc0: error -38 whilst initialising SD card ... (repeated until session is aborted) when trying to boot Linux for sifive_u from sd card. The command used to boot the image is qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ -kernel arch/riscv/boot/Image \ -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ -bios default \ -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 earlycon" \ -nographic -monitor none Bisect log is attached. Guenter --- # bad: [fd1952d814da738ed107e05583b3e02ac11e88ff] Update version for v9.1.0 release # good: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release git bisect start 'HEAD' 'v9.0.0' # good: [2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219] hw/acpi/ich9: Remove dead code related to 'acpi_memory_hotplug' git bisect good 2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219 # bad: [76e375fc3c538bd6e4232314f693b56536a50b73] docs/qapidoc: add QMP highlighting to annotated qmp-example blocks git bisect bad 76e375fc3c538bd6e4232314f693b56536a50b73 # good: [720c0f3e6cf856fa62c06a8f0005d814684c30d9] hw/sd/sdcard: Register generic optional handlers (CMD11 and CMD20) git bisect good 720c0f3e6cf856fa62c06a8f0005d814684c30d9 # bad: [5915139aba1646220630596de30c673528e047c9] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging git bisect bad 5915139aba1646220630596de30c673528e047c9 # bad: [188569c10d5dc6996bde90ce25645083e9661ecb] target/i386/SEV: implement mask_cpuid_features git bisect bad 188569c10d5dc6996bde90ce25645083e9661ecb # bad: [8442e1625ba6723bee2c6d0fdb7207a3e27a2b05] hw/sd/sdcard: Add sd_acmd_SEND_SCR handler (ACMD51) git bisect bad 8442e1625ba6723bee2c6d0fdb7207a3e27a2b05 # bad: [96f3d00ac1680f978984314d24ae6f2f6f281fdc] hw/sd/sdcard: Add sd_cmd_PROGRAM_CSD handler (CMD27) git bisect bad 96f3d00ac1680f978984314d24ae6f2f6f281fdc # good: [1ec3cb893fa8883f5baf69850a4d0a97502bbad8] hw/sd/sdcard: Add sd_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) git bisect good 1ec3cb893fa8883f5baf69850a4d0a97502bbad8 # bad: [9318be060506be33a0fbf01198b0024fdeb28f39] hw/sd/sdcard: Add sd_cmd_GO_INACTIVE_STATE handler (CMD15) git bisect bad 9318be060506be33a0fbf01198b0024fdeb28f39 # bad: [030897e89d3dff8ef7efd5cc570612da4476734f] hw/sd/sdcard: Add sd_cmd_STOP_TRANSMISSION handler (CMD12) git bisect bad 030897e89d3dff8ef7efd5cc570612da4476734f # bad: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) git bisect bad da954d0e32444f122a41c24948d4d1c718bf66d4 # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)
Hi Guenter, On 23/10/24 19:24, Guenter Roeck wrote: > Hi, > > On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> --- > > This patch results in: > > [ 5.976133] Waiting for root device /dev/mmcblk0... > [ 6.501462] mmc0: error -38 whilst initialising SD card > [ 7.557473] mmc0: error -38 whilst initialising SD card > > ... (repeated until session is aborted) > > when trying to boot Linux for sifive_u from sd card. > The command used to boot the image is > > qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ > -kernel arch/riscv/boot/Image \ > -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ > -bios default \ > -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 earlycon" \ > -nographic -monitor none > > Bisect log is attached. > > Guenter > > --- > # bad: [fd1952d814da738ed107e05583b3e02ac11e88ff] Update version for v9.1.0 release > # good: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release > git bisect start 'HEAD' 'v9.0.0' > # good: [2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219] hw/acpi/ich9: Remove dead code related to 'acpi_memory_hotplug' > git bisect good 2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219 > # bad: [76e375fc3c538bd6e4232314f693b56536a50b73] docs/qapidoc: add QMP highlighting to annotated qmp-example blocks > git bisect bad 76e375fc3c538bd6e4232314f693b56536a50b73 > # good: [720c0f3e6cf856fa62c06a8f0005d814684c30d9] hw/sd/sdcard: Register generic optional handlers (CMD11 and CMD20) > git bisect good 720c0f3e6cf856fa62c06a8f0005d814684c30d9 > # bad: [5915139aba1646220630596de30c673528e047c9] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging > git bisect bad 5915139aba1646220630596de30c673528e047c9 > # bad: [188569c10d5dc6996bde90ce25645083e9661ecb] target/i386/SEV: implement mask_cpuid_features > git bisect bad 188569c10d5dc6996bde90ce25645083e9661ecb > # bad: [8442e1625ba6723bee2c6d0fdb7207a3e27a2b05] hw/sd/sdcard: Add sd_acmd_SEND_SCR handler (ACMD51) > git bisect bad 8442e1625ba6723bee2c6d0fdb7207a3e27a2b05 > # bad: [96f3d00ac1680f978984314d24ae6f2f6f281fdc] hw/sd/sdcard: Add sd_cmd_PROGRAM_CSD handler (CMD27) > git bisect bad 96f3d00ac1680f978984314d24ae6f2f6f281fdc > # good: [1ec3cb893fa8883f5baf69850a4d0a97502bbad8] hw/sd/sdcard: Add sd_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) > git bisect good 1ec3cb893fa8883f5baf69850a4d0a97502bbad8 > # bad: [9318be060506be33a0fbf01198b0024fdeb28f39] hw/sd/sdcard: Add sd_cmd_GO_INACTIVE_STATE handler (CMD15) > git bisect bad 9318be060506be33a0fbf01198b0024fdeb28f39 > # bad: [030897e89d3dff8ef7efd5cc570612da4476734f] hw/sd/sdcard: Add sd_cmd_STOP_TRANSMISSION handler (CMD12) > git bisect bad 030897e89d3dff8ef7efd5cc570612da4476734f > # bad: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) > git bisect bad da954d0e32444f122a41c24948d4d1c718bf66d4 > # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) I don't have access to my workstation, but looking at the patch, maybe the fix is simply: --- diff --git a/hw/sd/sd.c b/hw/sd/sd.c index a5d2d929a8a..1594d340a6e 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1567,7 +1567,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), @@ -1586,7 +1586,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), --- Is it possible for you to test this snippet? Regards, Phil.
On 10/23/24 20:27, Philippe Mathieu-Daudé wrote: > Hi Guenter, > > On 23/10/24 19:24, Guenter Roeck wrote: >> Hi, >> >> On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>> --- >> >> This patch results in: >> >> [ 5.976133] Waiting for root device /dev/mmcblk0... >> [ 6.501462] mmc0: error -38 whilst initialising SD card >> [ 7.557473] mmc0: error -38 whilst initialising SD card >> >> ... (repeated until session is aborted) >> >> when trying to boot Linux for sifive_u from sd card. >> The command used to boot the image is >> >> qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ >> -kernel arch/riscv/boot/Image \ >> -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ >> -bios default \ >> -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 earlycon" \ >> -nographic -monitor none >> >> Bisect log is attached. >> >> Guenter >> >> --- >> # bad: [fd1952d814da738ed107e05583b3e02ac11e88ff] Update version for v9.1.0 release >> # good: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release >> git bisect start 'HEAD' 'v9.0.0' >> # good: [2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219] hw/acpi/ich9: Remove dead code related to 'acpi_memory_hotplug' >> git bisect good 2529ea2d561ea9fe359fb19ebdcfeb8b6cddd219 >> # bad: [76e375fc3c538bd6e4232314f693b56536a50b73] docs/qapidoc: add QMP highlighting to annotated qmp-example blocks >> git bisect bad 76e375fc3c538bd6e4232314f693b56536a50b73 >> # good: [720c0f3e6cf856fa62c06a8f0005d814684c30d9] hw/sd/sdcard: Register generic optional handlers (CMD11 and CMD20) >> git bisect good 720c0f3e6cf856fa62c06a8f0005d814684c30d9 >> # bad: [5915139aba1646220630596de30c673528e047c9] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging >> git bisect bad 5915139aba1646220630596de30c673528e047c9 >> # bad: [188569c10d5dc6996bde90ce25645083e9661ecb] target/i386/SEV: implement mask_cpuid_features >> git bisect bad 188569c10d5dc6996bde90ce25645083e9661ecb >> # bad: [8442e1625ba6723bee2c6d0fdb7207a3e27a2b05] hw/sd/sdcard: Add sd_acmd_SEND_SCR handler (ACMD51) >> git bisect bad 8442e1625ba6723bee2c6d0fdb7207a3e27a2b05 >> # bad: [96f3d00ac1680f978984314d24ae6f2f6f281fdc] hw/sd/sdcard: Add sd_cmd_PROGRAM_CSD handler (CMD27) >> git bisect bad 96f3d00ac1680f978984314d24ae6f2f6f281fdc >> # good: [1ec3cb893fa8883f5baf69850a4d0a97502bbad8] hw/sd/sdcard: Add sd_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) >> git bisect good 1ec3cb893fa8883f5baf69850a4d0a97502bbad8 >> # bad: [9318be060506be33a0fbf01198b0024fdeb28f39] hw/sd/sdcard: Add sd_cmd_GO_INACTIVE_STATE handler (CMD15) >> git bisect bad 9318be060506be33a0fbf01198b0024fdeb28f39 >> # bad: [030897e89d3dff8ef7efd5cc570612da4476734f] hw/sd/sdcard: Add sd_cmd_STOP_TRANSMISSION handler (CMD12) >> git bisect bad 030897e89d3dff8ef7efd5cc570612da4476734f >> # bad: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) >> git bisect bad da954d0e32444f122a41c24948d4d1c718bf66d4 >> # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) > > I don't have access to my workstation, but looking at the patch, > maybe the fix is simply: > > --- > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index a5d2d929a8a..1594d340a6e 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1567,7 +1567,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), > @@ -1586,7 +1586,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), > --- > > Is it possible for you to test this snippet? > It must be related, but something else must be wrong. With the above, I get [ 4.355063] Run /sbin/init as init process ssi_sd: error: Unexpected response to cmd 13 [ 4.780139] mmc0: SPI card removed [ 4.785194] EXT4-fs (mmcblk0): shut down requested (2) [ 4.812689] Starting init: /sbin/init exists but couldn't execute it (error -5) [ 4.813248] Run /etc/init as init process [ 4.825799] init: attempt to access beyond end of device The state is always 4 when spi_cmd_SEND_CSD() and spi_cmd_SEND_CID() are called. With more debugging added: ssi_sd: error: Unexpected response to cmd 13 (arglen expected 4, got 16) Changing only one of the functions to check against sd_transfer_state doesn't help either; that brings back the repeated error -38. Guenter
Hi Guenter, On 24/10/24 01:04, Guenter Roeck wrote: > On 10/23/24 20:27, Philippe Mathieu-Daudé wrote: >> Hi Guenter, >> >> On 23/10/24 19:24, Guenter Roeck wrote: >>> Hi, >>> >>> On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>>> --- >>> >>> This patch results in: >>> >>> [ 5.976133] Waiting for root device /dev/mmcblk0... >>> [ 6.501462] mmc0: error -38 whilst initialising SD card >>> [ 7.557473] mmc0: error -38 whilst initialising SD card >>> >>> ... (repeated until session is aborted) >>> >>> when trying to boot Linux for sifive_u from sd card. >>> The command used to boot the image is >>> >>> qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ >>> -kernel arch/riscv/boot/Image \ >>> -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ >>> -bios default \ >>> -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 >>> earlycon" \ >>> -nographic -monitor none >>> # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] >>> hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) >> >> I don't have access to my workstation, but looking at the patch, >> maybe the fix is simply: >> >> --- >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> index a5d2d929a8a..1594d340a6e 100644 >> --- a/hw/sd/sd.c >> +++ b/hw/sd/sd.c >> @@ -1567,7 +1567,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), >> @@ -1586,7 +1586,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), >> --- >> >> Is it possible for you to test this snippet? >> > > It must be related, but something else must be wrong. With the above, I get > > [ 4.355063] Run /sbin/init as init process > ssi_sd: error: Unexpected response to cmd 13 > [ 4.780139] mmc0: SPI card removed > [ 4.785194] EXT4-fs (mmcblk0): shut down requested (2) > [ 4.812689] Starting init: /sbin/init exists but couldn't execute it > (error -5) > [ 4.813248] Run /etc/init as init process > [ 4.825799] init: attempt to access beyond end of device > > The state is always 4 when spi_cmd_SEND_CSD() and spi_cmd_SEND_CID() > are called. With more debugging added: > > ssi_sd: error: Unexpected response to cmd 13 (arglen expected 4, got 16) > > Changing only one of the functions to check against sd_transfer_state > doesn't help either; that brings back the repeated error -38. Looking at commit 807f6adac37 ("hw/sd/sdcard: Add sd_cmd_SEND_STATUS handler (CMD13)"), this should fix: -- >8 -- @@ -1639,7 +1639,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; --- But -- why the commit msg didn't mention the spec fix -- the commit looks correct to me. We might be missing smth from the spec. I'll have a look during soft freeze. Having a test such the one recently added in https://lore.kernel.org/qemu-devel/20241024082735.42324-3-thuth@redhat.com/ would help me ;) Regards, Phil.
On 10/24/24 10:53, Philippe Mathieu-Daudé wrote: > Hi Guenter, > > On 24/10/24 01:04, Guenter Roeck wrote: >> On 10/23/24 20:27, Philippe Mathieu-Daudé wrote: >>> Hi Guenter, >>> >>> On 23/10/24 19:24, Guenter Roeck wrote: >>>> Hi, >>>> >>>> On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>>>> --- >>>> >>>> This patch results in: >>>> >>>> [ 5.976133] Waiting for root device /dev/mmcblk0... >>>> [ 6.501462] mmc0: error -38 whilst initialising SD card >>>> [ 7.557473] mmc0: error -38 whilst initialising SD card >>>> >>>> ... (repeated until session is aborted) >>>> >>>> when trying to boot Linux for sifive_u from sd card. >>>> The command used to boot the image is >>>> >>>> qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ >>>> -kernel arch/riscv/boot/Image \ >>>> -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ >>>> -bios default \ >>>> -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 earlycon" \ >>>> -nographic -monitor none > > >>>> # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) >>> >>> I don't have access to my workstation, but looking at the patch, >>> maybe the fix is simply: >>> >>> --- >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >>> index a5d2d929a8a..1594d340a6e 100644 >>> --- a/hw/sd/sd.c >>> +++ b/hw/sd/sd.c >>> @@ -1567,7 +1567,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), >>> @@ -1586,7 +1586,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), >>> --- >>> >>> Is it possible for you to test this snippet? >>> >> >> It must be related, but something else must be wrong. With the above, I get >> >> [ 4.355063] Run /sbin/init as init process >> ssi_sd: error: Unexpected response to cmd 13 >> [ 4.780139] mmc0: SPI card removed >> [ 4.785194] EXT4-fs (mmcblk0): shut down requested (2) >> [ 4.812689] Starting init: /sbin/init exists but couldn't execute it (error -5) >> [ 4.813248] Run /etc/init as init process >> [ 4.825799] init: attempt to access beyond end of device >> >> The state is always 4 when spi_cmd_SEND_CSD() and spi_cmd_SEND_CID() >> are called. With more debugging added: >> >> ssi_sd: error: Unexpected response to cmd 13 (arglen expected 4, got 16) >> >> Changing only one of the functions to check against sd_transfer_state >> doesn't help either; that brings back the repeated error -38. > > Looking at commit 807f6adac37 ("hw/sd/sdcard: Add sd_cmd_SEND_STATUS > handler (CMD13)"), this should fix: > Yes, it does fix the problem, together with the sd_transfer_state changes above. I noticed that sd_cmd_SEND_CSD() and sd_cmd_SEND_CID() also check against sd_standby_state. Is that wrong as well ? > -- >8 -- > @@ -1639,7 +1639,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; > --- > > But -- why the commit msg didn't mention the spec fix -- the commit > looks correct to me. We might be missing smth from the spec. I'll > have a look during soft freeze. Having a test such the one recently > added in https://lore.kernel.org/qemu-devel/20241024082735.42324-3-thuth@redhat.com/ > would help me ;) > I'll see if I can add one to git@github.com:groeck/linux-test-downloads.git. Thanks, Guenter
On 10/24/24 12:13, Guenter Roeck wrote: > On 10/24/24 10:53, Philippe Mathieu-Daudé wrote: >> Hi Guenter, >> >> On 24/10/24 01:04, Guenter Roeck wrote: >>> On 10/23/24 20:27, Philippe Mathieu-Daudé wrote: >>>> Hi Guenter, >>>> >>>> On 23/10/24 19:24, Guenter Roeck wrote: >>>>> Hi, >>>>> >>>>> On Fri, Jun 28, 2024 at 09:01:27AM +0200, Philippe Mathieu-Daudé wrote: >>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>>>>> --- >>>>> >>>>> This patch results in: >>>>> >>>>> [ 5.976133] Waiting for root device /dev/mmcblk0... >>>>> [ 6.501462] mmc0: error -38 whilst initialising SD card >>>>> [ 7.557473] mmc0: error -38 whilst initialising SD card >>>>> >>>>> ... (repeated until session is aborted) >>>>> >>>>> when trying to boot Linux for sifive_u from sd card. >>>>> The command used to boot the image is >>>>> >>>>> qemu-system-riscv64 -M sifive_u -m 512M -no-reboot \ >>>>> -kernel arch/riscv/boot/Image \ >>>>> -snapshot -drive file=rootfs.ext2,format=raw,if=sd \ >>>>> -bios default \ >>>>> -append "root=/dev/mmcblk0 rootwait console=ttySIF0,115200 earlycon" \ >>>>> -nographic -monitor none >> >> >>>>> # first bad commit: [da954d0e32444f122a41c24948d4d1c718bf66d4] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10) >>>> >>>> I don't have access to my workstation, but looking at the patch, >>>> maybe the fix is simply: >>>> >>>> --- >>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >>>> index a5d2d929a8a..1594d340a6e 100644 >>>> --- a/hw/sd/sd.c >>>> +++ b/hw/sd/sd.c >>>> @@ -1567,7 +1567,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), >>>> @@ -1586,7 +1586,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), >>>> --- >>>> >>>> Is it possible for you to test this snippet? >>>> >>> >>> It must be related, but something else must be wrong. With the above, I get >>> >>> [ 4.355063] Run /sbin/init as init process >>> ssi_sd: error: Unexpected response to cmd 13 >>> [ 4.780139] mmc0: SPI card removed >>> [ 4.785194] EXT4-fs (mmcblk0): shut down requested (2) >>> [ 4.812689] Starting init: /sbin/init exists but couldn't execute it (error -5) >>> [ 4.813248] Run /etc/init as init process >>> [ 4.825799] init: attempt to access beyond end of device >>> >>> The state is always 4 when spi_cmd_SEND_CSD() and spi_cmd_SEND_CID() >>> are called. With more debugging added: >>> >>> ssi_sd: error: Unexpected response to cmd 13 (arglen expected 4, got 16) >>> >>> Changing only one of the functions to check against sd_transfer_state >>> doesn't help either; that brings back the repeated error -38. >> >> Looking at commit 807f6adac37 ("hw/sd/sdcard: Add sd_cmd_SEND_STATUS >> handler (CMD13)"), this should fix: >> > > Yes, it does fix the problem, together with the sd_transfer_state changes above. > > I noticed that sd_cmd_SEND_CSD() and sd_cmd_SEND_CID() also check against > sd_standby_state. Is that wrong as well ? > Never mind; those are correct. Guenter
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index bd7c7cf518..564e08709b 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1323,6 +1323,15 @@ 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) { + return sd_invalid_state_for_cmd(sd, req); + } + return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), + sd->csd, 16); +} + static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req) { if (sd->state != sd_standby_state) { @@ -1333,6 +1342,15 @@ 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) { + return sd_invalid_state_for_cmd(sd, req); + } + return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), + sd->cid, 16); +} + static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req) { if (sd->state != sd_standby_state) { @@ -1408,36 +1426,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req) switch (req.cmd) { /* Basic commands (Class 0 and Class 1) */ - case 9: /* CMD9: SEND_CSD */ - rca = sd_req_get_rca(sd, req); - switch (sd->state) { - case sd_transfer_state: - if (!sd_is_spi(sd)) { - break; - } - return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), - sd->csd, 16); - - default: - break; - } - break; - - case 10: /* CMD10: SEND_CID */ - rca = sd_req_get_rca(sd, req); - switch (sd->state) { - case sd_transfer_state: - if (!sd_is_spi(sd)) { - break; - } - return sd_cmd_to_sendingdata(sd, req, sd_req_get_address(sd, req), - sd->cid, 16); - - default: - break; - } - break; - case 12: /* CMD12: STOP_TRANSMISSION */ switch (sd->state) { case sd_sendingdata_state: @@ -2288,6 +2276,8 @@ static const SDProto sd_proto_spi = { [5] = {9, sd_spi, "IO_SEND_OP_COND", sd_cmd_optional}, [6] = {10, sd_spi, "SWITCH_FUNCTION", sd_cmd_SWITCH_FUNCTION}, [8] = {0, sd_spi, "SEND_IF_COND", sd_cmd_SEND_IF_COND}, + [9] = {0, sd_spi, "SEND_CSD", spi_cmd_SEND_CSD}, + [10] = {0, sd_spi, "SEND_CID", spi_cmd_SEND_CID}, [34] = {10, sd_spi, "READ_SEC_CMD", sd_cmd_optional}, [35] = {10, sd_spi, "WRITE_SEC_CMD", sd_cmd_optional}, [36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sd/sd.c | 50 ++++++++++++++++++++------------------------------ 1 file changed, 20 insertions(+), 30 deletions(-)