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

[Philippe Mathieu-Daudé]

On 01/09/2018 01:28 PM, Peter Maydell wrote:
> On 9 January 2018 at 16:25, Philippe Mathieu-Daudé <address@hidden> 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: address@hidden
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> ---
>>>  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;

And we can now drop the assignation in realize()

>>> +    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.

Got it!