Re: [PATCH v42 51/98] hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.