| Message ID | 1515506513-31961-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Reset SD cards attached to legacy-API controllers | expand |
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;" Since none of this comments are important, I can add a patch in my series previous to convert to SDBus. > + > + /* Since we're still using the legacy SD API the card is not plugged > + * into any bus, and we must reset it manually. > + */ > + device_reset(DEVICE(s->sd)); > +} > + > static void ssi_sd_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -260,10 +281,11 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data) > k->transfer = ssi_sd_transfer; > k->cs_polarity = SSI_CS_LOW; > dc->vmsd = &vmstate_ssi_sd; > + dc->reset = ssi_sd_reset; > } > > static const TypeInfo ssi_sd_info = { > - .name = "ssi-sd", > + .name = TYPE_SSI_SD, > .parent = TYPE_SSI_SLAVE, > .instance_size = sizeof(ssi_sd_state), > .class_init = ssi_sd_class_init, >
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
On 01/09/2018 01:28 PM, Peter Maydell wrote: > 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; 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!
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; + memset(s->cmdarg, 0, sizeof(s->cmdarg)); + memset(s->response, 0, sizeof(s->response)); + s->arglen = 0; + s->response_pos = 0; + s->stopping = 0; + + /* Since we're still using the legacy SD API the card is not plugged + * into any bus, and we must reset it manually. + */ + device_reset(DEVICE(s->sd)); +} + static void ssi_sd_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -260,10 +281,11 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data) k->transfer = ssi_sd_transfer; k->cs_polarity = SSI_CS_LOW; dc->vmsd = &vmstate_ssi_sd; + dc->reset = ssi_sd_reset; } static const TypeInfo ssi_sd_info = { - .name = "ssi-sd", + .name = TYPE_SSI_SD, .parent = TYPE_SSI_SLAVE, .instance_size = sizeof(ssi_sd_state), .class_init = ssi_sd_class_init,
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(-) -- 2.7.4