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

[Guenter Roeck <linux@roeck-us.net>]

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