On 01/09/2018 01:28 PM, Peter Maydell wrote:
> On 9 January 2018 at 16:25, Philippe Mathieu-Daudé <f4...@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-sta...@nongnu.org
>>> Signed-off-by: Peter Maydell <peter.mayd...@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;
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!
