Re: [Qemu-devel] [Qemu-arm] [PATCH 3/4] hw/sd/ssi-sd: Reset SD card on controller reset

[Peter Maydell <peter.maydell@linaro.org>]

On 9 January 2018 at 16:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 01/09/2018 11:01 AM, Peter Maydell wrote:
>> Since ssi-sd is still using the legacy SD card API, the SD
>> card created by sd_init() is not plugged into any bus. This
>> means that the controller has to reset it manually.
>>
>> Failing to do this mostly didn't affect the guest since the
>> guest typically does a programmed SD card reset as part of
>> its SD controller driver initialization, but meant that
>> migration failed because it's only in sd_reset() that we
>> set up the wpgrps_size field.
>>
>> In the case of sd-ssi, we have to implement an entire
>> reset function since there wasn't one previously, and
>> that requires a QOM cast macro that got omitted when this
>> device was QOMified.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/sd/ssi-sd.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
>> index 24001dc..30d2a87 100644
>> --- a/hw/sd/ssi-sd.c
>> +++ b/hw/sd/ssi-sd.c
>> @@ -50,6 +50,9 @@ typedef struct {
>>      SDState *sd;
>>  } ssi_sd_state;
>>
>> +#define TYPE_SSI_SD "ssi-sd"
>> +#define SSI_SD(obj) OBJECT_CHECK(ssi_sd_state, (obj), TYPE_SSI_SD)
>> +
>>  /* State word bits.  */
>>  #define SSI_SDR_LOCKED          0x0001
>>  #define SSI_SDR_WP_ERASE        0x0002
>> @@ -251,6 +254,24 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
>>      }
>>  }
>>
>> +static void ssi_sd_reset(DeviceState *dev)
>> +{
>> +    ssi_sd_state *s = SSI_SD(dev);
>> +
>> +    s->mode = SSI_SD_CMD;
>
>> +    s->cmd = 0;
>
> Not necessary/useful since s->mode = SSI_SD_CMD.
>
>> +    memset(s->cmdarg, 0, sizeof(s->cmdarg));
>> +    memset(s->response, 0, sizeof(s->response));
>
> This might be cleaner to move it in ssi_sd_transfer() case SSI_SD_CMD.
>
>> +    s->arglen = 0;
>
> Not necessary/useful since s->mode = SSI_SD_CMD.
>
>> +    s->response_pos = 0;
>
> This might be safer to move this in ssi_sd_transfer() case SSI_SD_CMD.
>
>> +    s->stopping = 0;
>
> This might be cleaner to move it in ssi_sd_transfer() entry "Special
> case else s->stopping = 0;"

I felt it was easier to reason about both (a) whether the reset
function was correct and (b) what the state of the device might
be at any point if the reset function just cleared everything
back to the state it's in when the device is freshly created.

thanks
-- PMM