* [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate @ 2015-08-11 14:15 Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object Peter Maydell ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Peter Maydell @ 2015-08-11 14:15 UTC (permalink / raw) To: qemu-devel; +Cc: Markus Armbruster, patches This patchset updates the ancient pxa2xx_mmci device to something resembling modern standards for devices. In particular it makes it a proper sysbus device and switches to VMStateDescription structs. The major issue I have with this is in patch 1: I wanted the device to have a property so its users can set the BlockBackend* it should use. Unfortunately, DEFINE_PROP_DRIVE() is no good here, because setting a drive property results in a call to blk_attach_dev() which attaches the BlockBackend to this device. That then means that the call in sd_init() to attach the BlockBackend to the SD card object aborts. I needed a way to have a drive property which didn't mean "and this device claims the drive", and the best I could come up with was to use a pointer property. Suggestions for better approaches welcome. (The other SD controller devices are either also ancient non-QOM devices, or use drive_get_next() in the init function...) There are clearly further cleanup opportunities for this device, like making the sd callbacks into sysbus gpio input lines rather than having an ad-hoc pxa2xx_mmci_handlers() function to set them, but one thing at a time. Peter Maydell (3): hw/sd/pxa2xx_mmci: convert to SysBusDevice object hw/sd/pxa2xx_mmci: Convert to VMStateDescription hw/sd/pxa2xx_mmci: Add reset function hw/sd/pxa2xx_mmci.c | 261 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 159 insertions(+), 102 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-08-11 14:15 [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell @ 2015-08-11 14:15 ` Peter Maydell 2015-09-07 16:40 ` Markus Armbruster 2015-08-11 14:15 ` [Qemu-devel] [PATCH 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription Peter Maydell ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Peter Maydell @ 2015-08-11 14:15 UTC (permalink / raw) To: qemu-devel; +Cc: Markus Armbruster, patches Convert the pxa2xx_mmci device to be a sysbus device. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/sd/pxa2xx_mmci.c | 96 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 17 deletions(-) diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c index d1fe6d5..5b676c7 100644 --- a/hw/sd/pxa2xx_mmci.c +++ b/hw/sd/pxa2xx_mmci.c @@ -11,16 +11,23 @@ */ #include "hw/hw.h" +#include "hw/sysbus.h" #include "hw/arm/pxa.h" #include "hw/sd.h" #include "hw/qdev.h" -struct PXA2xxMMCIState { +#define TYPE_PXA2XX_MMCI "pxa2xx-mmci" +#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI) + +typedef struct PXA2xxMMCIState { + SysBusDevice parent_obj; + MemoryRegion iomem; qemu_irq irq; qemu_irq rx_dma; qemu_irq tx_dma; + void *blk; /* Should be a BlockBackend* */ SDState *card; uint32_t status; @@ -48,7 +55,7 @@ struct PXA2xxMMCIState { int resp_len; int cmdreq; -}; +} PXA2xxMMCIState; #define MMC_STRPCL 0x00 /* MMC Clock Start/Stop register */ #define MMC_STAT 0x04 /* MMC Status register */ @@ -474,31 +481,86 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem, BlockBackend *blk, qemu_irq irq, qemu_irq rx_dma, qemu_irq tx_dma) { - PXA2xxMMCIState *s; + DeviceState *dev; + SysBusDevice *sbd; + + dev = qdev_create(NULL, TYPE_PXA2XX_MMCI); + qdev_prop_set_ptr(dev, "drive", blk); + qdev_init_nofail(dev); + sbd = SYS_BUS_DEVICE(dev); + sysbus_mmio_map(sbd, 0, base); + sysbus_connect_irq(sbd, 0, irq); + sysbus_connect_irq(sbd, 1, rx_dma); + sysbus_connect_irq(sbd, 2, tx_dma); + return PXA2XX_MMCI(dev); +} - s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState)); - s->irq = irq; - s->rx_dma = rx_dma; - s->tx_dma = tx_dma; +void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly, + qemu_irq coverswitch) +{ + sd_set_cb(s->card, readonly, coverswitch); +} + +static void pxa2xx_mmci_instance_init(Object *obj) +{ + PXA2xxMMCIState *s = PXA2XX_MMCI(obj); + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); - memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s, + memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s, "pxa2xx-mmci", 0x00100000); - memory_region_add_subregion(sysmem, base, &s->iomem); + sysbus_init_mmio(sbd, &s->iomem); + sysbus_init_irq(sbd, &s->irq); + sysbus_init_irq(sbd, &s->rx_dma); + sysbus_init_irq(sbd, &s->tx_dma); + + register_savevm(NULL, "pxa2xx_mmci", 0, 0, + pxa2xx_mmci_save, pxa2xx_mmci_load, s); +} + +static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp) +{ + PXA2xxMMCIState *s = PXA2XX_MMCI(dev); /* Instantiate the actual storage */ - s->card = sd_init(blk, false); + s->card = sd_init(s->blk, false); if (s->card == NULL) { - exit(1); + error_setg(errp, "Could not initialize SD card with specified drive"); + return; } +} - register_savevm(NULL, "pxa2xx_mmci", 0, 0, - pxa2xx_mmci_save, pxa2xx_mmci_load, s); +static Property pxa2xx_mmci_properties[] = { + /* Note: pointer property 'drive' may remain NULL, thus no need + * for dc->cannot_instantiate_with_device_add_yet = true; + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because + * setting a 'drive' property results in a call to blk_attach_dev() + * attaching the BlockBackend to this device; that then means that + * the call in sd_init() to blk_attach_dev_nofail() which tries to + * attach the BlockBackend to the SD card object aborts. + */ + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), + DEFINE_PROP_END_OF_LIST(), +}; - return s; +static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = pxa2xx_mmci_realize; + dc->props = pxa2xx_mmci_properties; } -void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly, - qemu_irq coverswitch) +static const TypeInfo pxa2xx_mmci_info = { + .name = TYPE_PXA2XX_MMCI, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(PXA2xxMMCIState), + .class_init = pxa2xx_mmci_class_init, + .instance_init = pxa2xx_mmci_instance_init, +}; + +static void pxa2xx_mmci_register_types(void) { - sd_set_cb(s->card, readonly, coverswitch); + type_register_static(&pxa2xx_mmci_info); } + +type_init(pxa2xx_mmci_register_types) -- 1.9.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-08-11 14:15 ` [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object Peter Maydell @ 2015-09-07 16:40 ` Markus Armbruster 2015-09-07 16:42 ` Peter Maydell 0 siblings, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-09-07 16:40 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, patches Peter Maydell <peter.maydell@linaro.org> writes: > Convert the pxa2xx_mmci device to be a sysbus device. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/sd/pxa2xx_mmci.c | 96 +++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 79 insertions(+), 17 deletions(-) > > diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c > index d1fe6d5..5b676c7 100644 > --- a/hw/sd/pxa2xx_mmci.c > +++ b/hw/sd/pxa2xx_mmci.c > @@ -11,16 +11,23 @@ > */ > > #include "hw/hw.h" > +#include "hw/sysbus.h" > #include "hw/arm/pxa.h" > #include "hw/sd.h" > #include "hw/qdev.h" > > -struct PXA2xxMMCIState { > +#define TYPE_PXA2XX_MMCI "pxa2xx-mmci" > +#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI) > + > +typedef struct PXA2xxMMCIState { > + SysBusDevice parent_obj; > + > MemoryRegion iomem; > qemu_irq irq; > qemu_irq rx_dma; > qemu_irq tx_dma; > > + void *blk; /* Should be a BlockBackend* */ > SDState *card; > > uint32_t status; > @@ -48,7 +55,7 @@ struct PXA2xxMMCIState { > int resp_len; > > int cmdreq; > -}; > +} PXA2xxMMCIState; > > #define MMC_STRPCL 0x00 /* MMC Clock Start/Stop register */ > #define MMC_STAT 0x04 /* MMC Status register */ > @@ -474,31 +481,86 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem, > BlockBackend *blk, qemu_irq irq, > qemu_irq rx_dma, qemu_irq tx_dma) > { > - PXA2xxMMCIState *s; > + DeviceState *dev; > + SysBusDevice *sbd; > + > + dev = qdev_create(NULL, TYPE_PXA2XX_MMCI); > + qdev_prop_set_ptr(dev, "drive", blk); > + qdev_init_nofail(dev); > + sbd = SYS_BUS_DEVICE(dev); > + sysbus_mmio_map(sbd, 0, base); > + sysbus_connect_irq(sbd, 0, irq); > + sysbus_connect_irq(sbd, 1, rx_dma); > + sysbus_connect_irq(sbd, 2, tx_dma); > + return PXA2XX_MMCI(dev); > +} > > - s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState)); > - s->irq = irq; > - s->rx_dma = rx_dma; > - s->tx_dma = tx_dma; > +void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly, > + qemu_irq coverswitch) > +{ > + sd_set_cb(s->card, readonly, coverswitch); > +} > + > +static void pxa2xx_mmci_instance_init(Object *obj) > +{ > + PXA2xxMMCIState *s = PXA2XX_MMCI(obj); > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > > - memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s, > + memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s, > "pxa2xx-mmci", 0x00100000); > - memory_region_add_subregion(sysmem, base, &s->iomem); > + sysbus_init_mmio(sbd, &s->iomem); > + sysbus_init_irq(sbd, &s->irq); > + sysbus_init_irq(sbd, &s->rx_dma); > + sysbus_init_irq(sbd, &s->tx_dma); > + > + register_savevm(NULL, "pxa2xx_mmci", 0, 0, > + pxa2xx_mmci_save, pxa2xx_mmci_load, s); > +} > + > +static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp) > +{ > + PXA2xxMMCIState *s = PXA2XX_MMCI(dev); > > /* Instantiate the actual storage */ > - s->card = sd_init(blk, false); > + s->card = sd_init(s->blk, false); > if (s->card == NULL) { > - exit(1); > + error_setg(errp, "Could not initialize SD card with specified drive"); > + return; > } > +} > > - register_savevm(NULL, "pxa2xx_mmci", 0, 0, > - pxa2xx_mmci_save, pxa2xx_mmci_load, s); > +static Property pxa2xx_mmci_properties[] = { > + /* Note: pointer property 'drive' may remain NULL, thus no need > + * for dc->cannot_instantiate_with_device_add_yet = true; > + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because > + * setting a 'drive' property results in a call to blk_attach_dev() > + * attaching the BlockBackend to this device; that then means that > + * the call in sd_init() to blk_attach_dev_nofail() which tries to > + * attach the BlockBackend to the SD card object aborts. > + */ > + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), > + DEFINE_PROP_END_OF_LIST(), > +}; As far as I can tell, this problem is an artifact of our interface to the common sd-card code, namely sd_init(). sd_init() was made for the pre-qdev world: it creates and completely initializes the common SDState. In qdev, we do this in three separate steps: create, set properties, realize. Split up sd_init(), and the problem should go away. How? Well, code common to several related qdevs exists elsewhere. A simple example is serial.c. The serial qdevs embed a SerialState in their device state, have properties pointing into that sub-state, and call serial_realize_core() to realize that sub-state. We also use composition, i.e. do the common part as full-blown qdev, then wrap specialization qdevs around it. I doubt you need to be that fancy here. > > - return s; > +static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = pxa2xx_mmci_realize; > + dc->props = pxa2xx_mmci_properties; > } > > -void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly, > - qemu_irq coverswitch) > +static const TypeInfo pxa2xx_mmci_info = { > + .name = TYPE_PXA2XX_MMCI, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(PXA2xxMMCIState), > + .class_init = pxa2xx_mmci_class_init, > + .instance_init = pxa2xx_mmci_instance_init, > +}; > + > +static void pxa2xx_mmci_register_types(void) > { > - sd_set_cb(s->card, readonly, coverswitch); > + type_register_static(&pxa2xx_mmci_info); > } > + > +type_init(pxa2xx_mmci_register_types) Sorry for taking so long to review. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-09-07 16:40 ` Markus Armbruster @ 2015-09-07 16:42 ` Peter Maydell 2015-09-07 16:57 ` Markus Armbruster 2015-09-07 18:39 ` Peter Crosthwaite 0 siblings, 2 replies; 25+ messages in thread From: Peter Maydell @ 2015-09-07 16:42 UTC (permalink / raw) To: Markus Armbruster; +Cc: QEMU Developers, Patch Tracking On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> Convert the pxa2xx_mmci device to be a sysbus device. >> +static Property pxa2xx_mmci_properties[] = { >> + /* Note: pointer property 'drive' may remain NULL, thus no need >> + * for dc->cannot_instantiate_with_device_add_yet = true; >> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >> + * setting a 'drive' property results in a call to blk_attach_dev() >> + * attaching the BlockBackend to this device; that then means that >> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >> + * attach the BlockBackend to the SD card object aborts. >> + */ >> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >> + DEFINE_PROP_END_OF_LIST(), >> +}; > > As far as I can tell, this problem is an artifact of our interface to > the common sd-card code, namely sd_init(). sd_init() was made for the > pre-qdev world: it creates and completely initializes the common > SDState. > > In qdev, we do this in three separate steps: create, set properties, > realize. Split up sd_init(), and the problem should go away. Yes, but I don't really want to gate QOMification of mmc controller devices on the more complicated problem of QOMifying sd.c itself, especially since we already have several QOMified mmc controllers... thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-09-07 16:42 ` Peter Maydell @ 2015-09-07 16:57 ` Markus Armbruster 2015-12-03 21:20 ` Peter Maydell 2015-09-07 18:39 ` Peter Crosthwaite 1 sibling, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-09-07 16:57 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers, Patch Tracking Peter Maydell <peter.maydell@linaro.org> writes: > On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> Convert the pxa2xx_mmci device to be a sysbus device. > >>> +static Property pxa2xx_mmci_properties[] = { >>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>> + * setting a 'drive' property results in a call to blk_attach_dev() >>> + * attaching the BlockBackend to this device; that then means that >>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>> + * attach the BlockBackend to the SD card object aborts. >>> + */ >>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >> >> As far as I can tell, this problem is an artifact of our interface to >> the common sd-card code, namely sd_init(). sd_init() was made for the >> pre-qdev world: it creates and completely initializes the common >> SDState. >> >> In qdev, we do this in three separate steps: create, set properties, >> realize. Split up sd_init(), and the problem should go away. > > Yes, but I don't really want to gate QOMification of mmc > controller devices on the more complicated problem of > QOMifying sd.c itself, especially since we already have several > QOMified mmc controllers... Is serial.c QOMified? I don't think so, it's merely structured in a QOM-friendly way: typedef SerialState, realize helper serial_realize_core(), unrealize helper serial_exit_core(). If SerialState had more properties, we'd also need a macro to define them. I believe sd.c could be made like that, with not much effort. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-09-07 16:57 ` Markus Armbruster @ 2015-12-03 21:20 ` Peter Maydell 2015-12-04 7:30 ` Markus Armbruster 0 siblings, 1 reply; 25+ messages in thread From: Peter Maydell @ 2015-12-03 21:20 UTC (permalink / raw) To: Markus Armbruster; +Cc: QEMU Developers, Patch Tracking On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> Convert the pxa2xx_mmci device to be a sysbus device. >> >>>> +static Property pxa2xx_mmci_properties[] = { >>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>> + * attaching the BlockBackend to this device; that then means that >>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>> + * attach the BlockBackend to the SD card object aborts. >>>> + */ >>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>> + DEFINE_PROP_END_OF_LIST(), >>>> +}; >>> >>> As far as I can tell, this problem is an artifact of our interface to >>> the common sd-card code, namely sd_init(). sd_init() was made for the >>> pre-qdev world: it creates and completely initializes the common >>> SDState. >>> >>> In qdev, we do this in three separate steps: create, set properties, >>> realize. Split up sd_init(), and the problem should go away. >> >> Yes, but I don't really want to gate QOMification of mmc >> controller devices on the more complicated problem of >> QOMifying sd.c itself, especially since we already have several >> QOMified mmc controllers... > > Is serial.c QOMified? I don't think so, it's merely structured in a > QOM-friendly way: typedef SerialState, realize helper > serial_realize_core(), unrealize helper serial_exit_core(). If > SerialState had more properties, we'd also need a macro to define them. It looks like since we had this conversation the problem has been dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores its error return. So I should be able to do this with a DEFINE_PROP_DRIVE now I think... thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-03 21:20 ` Peter Maydell @ 2015-12-04 7:30 ` Markus Armbruster 2015-12-04 11:00 ` Peter Maydell 0 siblings, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-12-04 7:30 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers, Patch Tracking Peter Maydell <peter.maydell@linaro.org> writes: > On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> >>>>> Convert the pxa2xx_mmci device to be a sysbus device. >>> >>>>> +static Property pxa2xx_mmci_properties[] = { >>>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>>> + * attaching the BlockBackend to this device; that then means that >>>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>>> + * attach the BlockBackend to the SD card object aborts. >>>>> + */ >>>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>>> + DEFINE_PROP_END_OF_LIST(), >>>>> +}; >>>> >>>> As far as I can tell, this problem is an artifact of our interface to >>>> the common sd-card code, namely sd_init(). sd_init() was made for the >>>> pre-qdev world: it creates and completely initializes the common >>>> SDState. >>>> >>>> In qdev, we do this in three separate steps: create, set properties, >>>> realize. Split up sd_init(), and the problem should go away. >>> >>> Yes, but I don't really want to gate QOMification of mmc >>> controller devices on the more complicated problem of >>> QOMifying sd.c itself, especially since we already have several >>> QOMified mmc controllers... >> >> Is serial.c QOMified? I don't think so, it's merely structured in a >> QOM-friendly way: typedef SerialState, realize helper >> serial_realize_core(), unrealize helper serial_exit_core(). If >> SerialState had more properties, we'd also need a macro to define them. > > It looks like since we had this conversation the problem has been > dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call > to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores > its error return. So I should be able to do this with a DEFINE_PROP_DRIVE > now I think... Ignoring the error is intentional according to the comment, but why is it appropriate? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 7:30 ` Markus Armbruster @ 2015-12-04 11:00 ` Peter Maydell 2015-12-04 12:50 ` Markus Armbruster 2015-12-04 16:42 ` Paolo Bonzini 0 siblings, 2 replies; 25+ messages in thread From: Peter Maydell @ 2015-12-04 11:00 UTC (permalink / raw) To: Markus Armbruster Cc: Kevin OConnor, QEMU Developers, Stefan Hajnoczi, Patch Tracking On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>> >>>>>> Convert the pxa2xx_mmci device to be a sysbus device. >>>> >>>>>> +static Property pxa2xx_mmci_properties[] = { >>>>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>>>> + * attaching the BlockBackend to this device; that then means that >>>>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>>>> + * attach the BlockBackend to the SD card object aborts. >>>>>> + */ >>>>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>> +}; >>>>> >>>>> As far as I can tell, this problem is an artifact of our interface to >>>>> the common sd-card code, namely sd_init(). sd_init() was made for the >>>>> pre-qdev world: it creates and completely initializes the common >>>>> SDState. >>>>> >>>>> In qdev, we do this in three separate steps: create, set properties, >>>>> realize. Split up sd_init(), and the problem should go away. >>>> >>>> Yes, but I don't really want to gate QOMification of mmc >>>> controller devices on the more complicated problem of >>>> QOMifying sd.c itself, especially since we already have several >>>> QOMified mmc controllers... >>> >>> Is serial.c QOMified? I don't think so, it's merely structured in a >>> QOM-friendly way: typedef SerialState, realize helper >>> serial_realize_core(), unrealize helper serial_exit_core(). If >>> SerialState had more properties, we'd also need a macro to define them. >> >> It looks like since we had this conversation the problem has been >> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call >> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores >> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE >> now I think... > > Ignoring the error is intentional according to the comment, but why is > it appropriate? That seems like a question to ask the author and reviewer of that commit :-) [cc'd]. The intention seems to have been to allow sdhci to do the same thing I want -- take a drive property (which attaches the BlockBackend to the controller device) and then hand the BlockBackend to sd_init() without having it blow up. Incidentally, in an ideal world wouldn't the block/drive properties be on the SD card object rather than the controller object ? At least, we seem to have that split for IDE and SCSI disks. thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 11:00 ` Peter Maydell @ 2015-12-04 12:50 ` Markus Armbruster 2015-12-04 12:55 ` Peter Maydell 2015-12-04 16:42 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-12-04 12:50 UTC (permalink / raw) To: Peter Maydell Cc: Kevin OConnor, QEMU Developers, Stefan Hajnoczi, Patch Tracking Peter Maydell <peter.maydell@linaro.org> writes: > On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> >>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>>> >>>>>>> Convert the pxa2xx_mmci device to be a sysbus device. >>>>> >>>>>>> +static Property pxa2xx_mmci_properties[] = { >>>>>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>>>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>>>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>>>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>>>>> + * attaching the BlockBackend to this device; that then means that >>>>>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>>>>> + * attach the BlockBackend to the SD card object aborts. >>>>>>> + */ >>>>>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>>> +}; >>>>>> >>>>>> As far as I can tell, this problem is an artifact of our interface to >>>>>> the common sd-card code, namely sd_init(). sd_init() was made for the >>>>>> pre-qdev world: it creates and completely initializes the common >>>>>> SDState. >>>>>> >>>>>> In qdev, we do this in three separate steps: create, set properties, >>>>>> realize. Split up sd_init(), and the problem should go away. >>>>> >>>>> Yes, but I don't really want to gate QOMification of mmc >>>>> controller devices on the more complicated problem of >>>>> QOMifying sd.c itself, especially since we already have several >>>>> QOMified mmc controllers... >>>> >>>> Is serial.c QOMified? I don't think so, it's merely structured in a >>>> QOM-friendly way: typedef SerialState, realize helper >>>> serial_realize_core(), unrealize helper serial_exit_core(). If >>>> SerialState had more properties, we'd also need a macro to define them. >>> >>> It looks like since we had this conversation the problem has been >>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call >>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores >>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE >>> now I think... >> >> Ignoring the error is intentional according to the comment, but why is >> it appropriate? > > That seems like a question to ask the author and reviewer of that > commit :-) [cc'd]. > > The intention seems to have been to allow sdhci to do the same thing > I want -- take a drive property (which attaches the BlockBackend to > the controller device) and then hand the BlockBackend to sd_init() > without having it blow up. > > Incidentally, in an ideal world wouldn't the block/drive properties > be on the SD card object rather than the controller object ? At least, > we seem to have that split for IDE and SCSI disks. This is really an instance of the common split device pattern: we have a front end (a.k.a. device model) connected to a backend (or even multiple backends). A character device model is connected to a CharDriverState. A NIC device model is connected to NICPeers. And a block device is connected to a BlockBackend. For qdevified devices, the connection is made with a qdev property. A character device model has one defined with DEFINE_PROP_CHR(). A NIC device model has one defined with DEFINE_PROP_NETDEV(), usually via DEFINE_NIC_PROPERTIES(). A block device model has one defined with DEFINE_PROP_DRIVE(), often via DEFINE_BLOCK_PROPERTIES(). Backends commonly can only be connected to one frontend. The properties check the applicable constraints, and error out when the user tries to violate them. Example: if you try to connect a block backend to multiple frontends by specifying the same drive= value with each of them, you get an error. Good, because without that, you'd likely get data corruption. To finally answer your question: the proper owner of the property connecting the backend is the frontend half of the split device. So, if we have separate device models for controller and the block devices connected to it, the block backend property belongs to the latter, not the former. If we have separate device models for SD controller and card, the block backend property belongs to the card, not the controller. Non-qdevified devices need to setup the connection manually, making sure applicable constraints get checked. As I explained above, the problem with SD cards is "an artifact of our interface to the common sd-card code": it serves both non-qdevified and qdevified code, badly. We should either qdevify everything, or restructure the common code to make it serve both qdevified and non-qdevified code well. I acknowledge the former is a tall order. The latter, however, should be doable, and I pointed to similarly common code that already does it. Commit 5ec911c papers over the problem instead, in sd_init(). Works basically like this: 1. Try to connect the backend, ignoring errors. 2. Configure the backend to taste. Fine if the backend is already connected to this frontend, and we're trying to connect it again only because our code is too disorganized to know. However, if the backend is actually connected to something else, it's bound to end in tears. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 12:50 ` Markus Armbruster @ 2015-12-04 12:55 ` Peter Maydell 2015-12-04 13:04 ` Markus Armbruster 2015-12-04 18:49 ` Kevin O'Connor 0 siblings, 2 replies; 25+ messages in thread From: Peter Maydell @ 2015-12-04 12:55 UTC (permalink / raw) To: Markus Armbruster Cc: Kevin OConnor, QEMU Developers, Stefan Hajnoczi, Patch Tracking On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote: > To finally answer your question: the proper owner of the property > connecting the backend is the frontend half of the split device. So, if > we have separate device models for controller and the block devices > connected to it, the block backend property belongs to the latter, not > the former. If we have separate device models for SD controller and > card, the block backend property belongs to the card, not the > controller. I thought this might be your answer, but this is problematic because we now have a device in the tree (the sdhci pci card) which has the property on the controller, and so that's now user-facing command line ABI which we can't change... thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 12:55 ` Peter Maydell @ 2015-12-04 13:04 ` Markus Armbruster 2015-12-04 18:49 ` Kevin O'Connor 1 sibling, 0 replies; 25+ messages in thread From: Markus Armbruster @ 2015-12-04 13:04 UTC (permalink / raw) To: Peter Maydell Cc: Kevin OConnor, QEMU Developers, Stefan Hajnoczi, Patch Tracking Peter Maydell <peter.maydell@linaro.org> writes: > On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote: >> To finally answer your question: the proper owner of the property >> connecting the backend is the frontend half of the split device. So, if >> we have separate device models for controller and the block devices >> connected to it, the block backend property belongs to the latter, not >> the former. If we have separate device models for SD controller and >> card, the block backend property belongs to the card, not the >> controller. > > I thought this might be your answer, but this is problematic because > we now have a device in the tree (the sdhci pci card) which has the > property on the controller, and so that's now user-facing command > line ABI which we can't change... As far as I can tell, device sdhci-pci hasn't been in a release, and we can still keep it out of 2.5: * Created in commit 224d10f, v2.3.0. * Made unavailable in commit 1910913, v2.3.0 * Made available in commit 5ec911c, not yet released. I'll post a patch making it unavailable again right away, to give you options. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 12:55 ` Peter Maydell 2015-12-04 13:04 ` Markus Armbruster @ 2015-12-04 18:49 ` Kevin O'Connor 1 sibling, 0 replies; 25+ messages in thread From: Kevin O'Connor @ 2015-12-04 18:49 UTC (permalink / raw) To: Peter Maydell Cc: Patch Tracking, Markus Armbruster, Stefan Hajnoczi, QEMU Developers On Fri, Dec 04, 2015 at 12:55:07PM +0000, Peter Maydell wrote: > On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote: > > To finally answer your question: the proper owner of the property > > connecting the backend is the frontend half of the split device. So, if > > we have separate device models for controller and the block devices > > connected to it, the block backend property belongs to the latter, not > > the former. If we have separate device models for SD controller and > > card, the block backend property belongs to the card, not the > > controller. > > I thought this might be your answer, but this is problematic because > we now have a device in the tree (the sdhci pci card) which has the > property on the controller, and so that's now user-facing command > line ABI which we can't change... If by ABI, you mean the command line format: -device sdhci-pci,drive=drive0 -drive id=drive0,if=sd,file=myimage then I don't see why that should change. As Paolo points out, there is only one card per controller. That's how real hardware does it as well. Cheers, -Kevin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 11:00 ` Peter Maydell 2015-12-04 12:50 ` Markus Armbruster @ 2015-12-04 16:42 ` Paolo Bonzini 2015-12-04 18:50 ` Peter Crosthwaite 1 sibling, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2015-12-04 16:42 UTC (permalink / raw) To: Peter Maydell, Markus Armbruster Cc: Kevin OConnor, QEMU Developers, Stefan Hajnoczi, Patch Tracking On 04/12/2015 12:00, Peter Maydell wrote: > On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> >>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>>> >>>>>>> Convert the pxa2xx_mmci device to be a sysbus device. >>>>> >>>>>>> +static Property pxa2xx_mmci_properties[] = { >>>>>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>>>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>>>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>>>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>>>>> + * attaching the BlockBackend to this device; that then means that >>>>>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>>>>> + * attach the BlockBackend to the SD card object aborts. >>>>>>> + */ >>>>>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>>> +}; >>>>>> >>>>>> As far as I can tell, this problem is an artifact of our interface to >>>>>> the common sd-card code, namely sd_init(). sd_init() was made for the >>>>>> pre-qdev world: it creates and completely initializes the common >>>>>> SDState. >>>>>> >>>>>> In qdev, we do this in three separate steps: create, set properties, >>>>>> realize. Split up sd_init(), and the problem should go away. >>>>> >>>>> Yes, but I don't really want to gate QOMification of mmc >>>>> controller devices on the more complicated problem of >>>>> QOMifying sd.c itself, especially since we already have several >>>>> QOMified mmc controllers... >>>> >>>> Is serial.c QOMified? I don't think so, it's merely structured in a >>>> QOM-friendly way: typedef SerialState, realize helper >>>> serial_realize_core(), unrealize helper serial_exit_core(). If >>>> SerialState had more properties, we'd also need a macro to define them. >>> >>> It looks like since we had this conversation the problem has been >>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call >>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores >>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE >>> now I think... >> >> Ignoring the error is intentional according to the comment, but why is >> it appropriate? > > That seems like a question to ask the author and reviewer of that > commit :-) [cc'd]. > > The intention seems to have been to allow sdhci to do the same thing > I want -- take a drive property (which attaches the BlockBackend to > the controller device) and then hand the BlockBackend to sd_init() > without having it blow up. > > Incidentally, in an ideal world wouldn't the block/drive properties > be on the SD card object rather than the controller object ? At least, > we seem to have that split for IDE and SCSI disks. [copying from the other thread] FWIW, I don't think the SD card will be qdevified because it doesn't need a bus. It's similar indeed to SerialState, which was supposed to be the poster child of QOM embedding and never got QOMified. A host controller controls exactly one SD card, the SSI bridge is also for exactly one SD card, etc. So there's not much to gain from qdevification of the card itself. There would be a gain from qdevification of the OMAP and StrongARM controllers, which is exactly what this series does. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 16:42 ` Paolo Bonzini @ 2015-12-04 18:50 ` Peter Crosthwaite 2015-12-04 19:24 ` Kevin O'Connor 0 siblings, 1 reply; 25+ messages in thread From: Peter Crosthwaite @ 2015-12-04 18:50 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Patch Tracking, Markus Armbruster, QEMU Developers, Kevin OConnor, Stefan Hajnoczi On Fri, Dec 4, 2015 at 8:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 04/12/2015 12:00, Peter Maydell wrote: >> On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote: >>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>> >>>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >>>>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>>>> >>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device. >>>>>> >>>>>>>> +static Property pxa2xx_mmci_properties[] = { >>>>>>>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>>>>>>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>>>>>>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>>>>>>> + * setting a 'drive' property results in a call to blk_attach_dev() >>>>>>>> + * attaching the BlockBackend to this device; that then means that >>>>>>>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>>>>>>> + * attach the BlockBackend to the SD card object aborts. >>>>>>>> + */ >>>>>>>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>>>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>>>> +}; >>>>>>> >>>>>>> As far as I can tell, this problem is an artifact of our interface to >>>>>>> the common sd-card code, namely sd_init(). sd_init() was made for the >>>>>>> pre-qdev world: it creates and completely initializes the common >>>>>>> SDState. >>>>>>> >>>>>>> In qdev, we do this in three separate steps: create, set properties, >>>>>>> realize. Split up sd_init(), and the problem should go away. >>>>>> >>>>>> Yes, but I don't really want to gate QOMification of mmc >>>>>> controller devices on the more complicated problem of >>>>>> QOMifying sd.c itself, especially since we already have several >>>>>> QOMified mmc controllers... >>>>> >>>>> Is serial.c QOMified? I don't think so, it's merely structured in a >>>>> QOM-friendly way: typedef SerialState, realize helper >>>>> serial_realize_core(), unrealize helper serial_exit_core(). If >>>>> SerialState had more properties, we'd also need a macro to define them. >>>> >>>> It looks like since we had this conversation the problem has been >>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call >>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores >>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE >>>> now I think... >>> >>> Ignoring the error is intentional according to the comment, but why is >>> it appropriate? >> >> That seems like a question to ask the author and reviewer of that >> commit :-) [cc'd]. >> >> The intention seems to have been to allow sdhci to do the same thing >> I want -- take a drive property (which attaches the BlockBackend to >> the controller device) and then hand the BlockBackend to sd_init() >> without having it blow up. >> >> Incidentally, in an ideal world wouldn't the block/drive properties >> be on the SD card object rather than the controller object ? At least, >> we seem to have that split for IDE and SCSI disks. > > [copying from the other thread] > > FWIW, I don't think the SD card will be qdevified because it doesn't > need a bus. It's similar indeed to SerialState, which was supposed to > be the poster child of QOM embedding and never got QOMified. > SD is a bus in its own right and should be busified and QOMified IMO. SDHCI can talk to non-sd cards (SDIO). There is also a range of incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think anything that couples the controller to an SD card is a bug, the card and device should be arranged as separate devices. > A host controller controls exactly one SD card, the SSI bridge is also > for exactly one SD card, etc. I think you can RYO chip selects with a GPIO and control multiple SD cards with one SDHCI. Regards, Peter > So there's not much to gain from > qdevification of the card itself. There would be a gain from > qdevification of the OMAP and StrongARM controllers, which is exactly > what this series does. > > Paolo > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 18:50 ` Peter Crosthwaite @ 2015-12-04 19:24 ` Kevin O'Connor 2015-12-07 0:02 ` Peter Crosthwaite 0 siblings, 1 reply; 25+ messages in thread From: Kevin O'Connor @ 2015-12-04 19:24 UTC (permalink / raw) To: Peter Crosthwaite Cc: Peter Maydell, Patch Tracking, QEMU Developers, Markus Armbruster, Stefan Hajnoczi, Paolo Bonzini On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote: > > FWIW, I don't think the SD card will be qdevified because it doesn't > > need a bus. It's similar indeed to SerialState, which was supposed to > > be the poster child of QOM embedding and never got QOMified. > > SD is a bus in its own right and should be busified and QOMified IMO. > SDHCI can talk to non-sd cards (SDIO). There is also a range of > incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think > anything that couples the controller to an SD card is a bug, the card > and device should be arranged as separate devices. > > > A host controller controls exactly one SD card, the SSI bridge is also > > for exactly one SD card, etc. > > I think you can RYO chip selects with a GPIO and control multiple SD > cards with one SDHCI. In practice, the SDHCI controllers are one-to-one with cards. This is codified in the sdhci spec as it has a "card present" bit and "port location" information that is per controller. I suppose in theory, one could put an SDHCI contoller into SPI compatibility mode and "hot wire" it into a bus, but qemu doesn't support that anyway, and it is a lot of complexity for something that is not done in practice. -Kevin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-04 19:24 ` Kevin O'Connor @ 2015-12-07 0:02 ` Peter Crosthwaite 2015-12-07 6:11 ` Kevin O'Connor 0 siblings, 1 reply; 25+ messages in thread From: Peter Crosthwaite @ 2015-12-07 0:02 UTC (permalink / raw) To: Kevin O'Connor Cc: Peter Maydell, Patch Tracking, QEMU Developers, Markus Armbruster, Stefan Hajnoczi, Paolo Bonzini On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote: > On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote: >> > FWIW, I don't think the SD card will be qdevified because it doesn't >> > need a bus. It's similar indeed to SerialState, which was supposed to >> > be the poster child of QOM embedding and never got QOMified. >> >> SD is a bus in its own right and should be busified and QOMified IMO. >> SDHCI can talk to non-sd cards (SDIO). There is also a range of >> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think >> anything that couples the controller to an SD card is a bug, the card >> and device should be arranged as separate devices. >> >> > A host controller controls exactly one SD card, the SSI bridge is also >> > for exactly one SD card, etc. >> >> I think you can RYO chip selects with a GPIO and control multiple SD >> cards with one SDHCI. > > In practice, the SDHCI controllers are one-to-one with cards. This is > codified in the sdhci spec as it has a "card present" bit and "port > location" information that is per controller. > But SDHCI is only one SD controller implementation. Other SD controllers may easily implement the SDIO bus as non 1:1. I also don't see why a 1:1 bus should have a completely different implementation that propagates through to UI level issues. SD is a bus like any other IMO. > I suppose in theory, one could put an SDHCI contoller into SPI > compatibility mode and "hot wire" it into a bus, but qemu doesn't > support that anyway, and it is a lot of complexity for something that > is not done in practice. > Note that the chip selects for SD are on the copper interface as well, so you can easily implement multi-device SD bus on the level above SDHCI. SPI mode should not required. Even if SDHCI does have card-detection CS control logic this can be demultiplexed to multiple cards by extra logic. We also have precedent of implementers of SDHCI opting-out of features like power control, and assumptions about how SDHCI is wired have already led to bugs like this: https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html We don't need to be implementing the multi CS logic today, but making UI level decisions based on this 1:1 assumption seems wrong. Regards, Peter > -Kevin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-07 0:02 ` Peter Crosthwaite @ 2015-12-07 6:11 ` Kevin O'Connor 2015-12-07 8:50 ` Markus Armbruster 0 siblings, 1 reply; 25+ messages in thread From: Kevin O'Connor @ 2015-12-07 6:11 UTC (permalink / raw) To: Peter Crosthwaite Cc: Peter Maydell, Patch Tracking, QEMU Developers, Markus Armbruster, Stefan Hajnoczi, Paolo Bonzini On Sun, Dec 06, 2015 at 04:02:14PM -0800, Peter Crosthwaite wrote: > On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote: > > On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote: > >> > FWIW, I don't think the SD card will be qdevified because it doesn't > >> > need a bus. It's similar indeed to SerialState, which was supposed to > >> > be the poster child of QOM embedding and never got QOMified. > >> > >> SD is a bus in its own right and should be busified and QOMified IMO. > >> SDHCI can talk to non-sd cards (SDIO). There is also a range of > >> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think > >> anything that couples the controller to an SD card is a bug, the card > >> and device should be arranged as separate devices. > >> > >> > A host controller controls exactly one SD card, the SSI bridge is also > >> > for exactly one SD card, etc. > >> > >> I think you can RYO chip selects with a GPIO and control multiple SD > >> cards with one SDHCI. > > > > In practice, the SDHCI controllers are one-to-one with cards. This is > > codified in the sdhci spec as it has a "card present" bit and "port > > location" information that is per controller. > > > > But SDHCI is only one SD controller implementation. Other SD > controllers may easily implement the SDIO bus as non 1:1. I also don't > see why a 1:1 bus should have a completely different implementation > that propagates through to UI level issues. SD is a bus like any other > IMO. > > > I suppose in theory, one could put an SDHCI contoller into SPI > > compatibility mode and "hot wire" it into a bus, but qemu doesn't > > support that anyway, and it is a lot of complexity for something that > > is not done in practice. > > > > Note that the chip selects for SD are on the copper interface as well, > so you can easily implement multi-device SD bus on the level above > SDHCI. SPI mode should not required. The "card select" line becomes a data line (DAT3) when not in SPI mode. That said, the MMC specs do define a convoluted mechanism for assigning unique ids to each card so that a bus could be implemented. (Which doesn't apply to SD cards, by my read of the SD specs.) >Even if SDHCI does have > card-detection CS control logic this can be demultiplexed to multiple > cards by extra logic. We also have precedent of implementers of SDHCI > opting-out of features like power control, and assumptions about how > SDHCI is wired have already led to bugs like this: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html > > We don't need to be implementing the multi CS logic today, but making > UI level decisions based on this 1:1 assumption seems wrong. I have no objection to a different internal or external represention to the SD cards. However, the immiediate choice seems to be whether to disable pci-sdhci or not. It seems unfortunate to me that it would be disabled. The QEMU SD code (beyond just the sdhci controller) is not organized as a bus, and it's not clear to me that there are real world gains to be had by modeling it as a bus. My interest in the sd cards is for testing purposes. Several recent Intel chipsets include multiple sdhci controllers on them, and several currently shipping devices use them for both eMMC storage and for external SD card ports - in particular several of the Chromebooks have them. Being able to perform simple sanity checks in QEMU is helpful. Cheers, -Kevin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-07 6:11 ` Kevin O'Connor @ 2015-12-07 8:50 ` Markus Armbruster 2015-12-07 9:58 ` Paolo Bonzini 0 siblings, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-12-07 8:50 UTC (permalink / raw) To: Kevin O'Connor Cc: Peter Maydell, Patch Tracking, QEMU Developers, Peter Crosthwaite, Stefan Hajnoczi, Paolo Bonzini "Kevin O'Connor" <kevin@koconnor.net> writes: > On Sun, Dec 06, 2015 at 04:02:14PM -0800, Peter Crosthwaite wrote: >> On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote: >> > On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote: >> >> > FWIW, I don't think the SD card will be qdevified because it doesn't >> >> > need a bus. It's similar indeed to SerialState, which was supposed to >> >> > be the poster child of QOM embedding and never got QOMified. >> >> >> >> SD is a bus in its own right and should be busified and QOMified IMO. >> >> SDHCI can talk to non-sd cards (SDIO). There is also a range of >> >> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think >> >> anything that couples the controller to an SD card is a bug, the card >> >> and device should be arranged as separate devices. >> >> >> >> > A host controller controls exactly one SD card, the SSI bridge is also >> >> > for exactly one SD card, etc. >> >> >> >> I think you can RYO chip selects with a GPIO and control multiple SD >> >> cards with one SDHCI. >> > >> > In practice, the SDHCI controllers are one-to-one with cards. This is >> > codified in the sdhci spec as it has a "card present" bit and "port >> > location" information that is per controller. >> > >> >> But SDHCI is only one SD controller implementation. Other SD >> controllers may easily implement the SDIO bus as non 1:1. I also don't >> see why a 1:1 bus should have a completely different implementation >> that propagates through to UI level issues. SD is a bus like any other >> IMO. >> >> > I suppose in theory, one could put an SDHCI contoller into SPI >> > compatibility mode and "hot wire" it into a bus, but qemu doesn't >> > support that anyway, and it is a lot of complexity for something that >> > is not done in practice. >> > >> >> Note that the chip selects for SD are on the copper interface as well, >> so you can easily implement multi-device SD bus on the level above >> SDHCI. SPI mode should not required. > > The "card select" line becomes a data line (DAT3) when not in SPI > mode. That said, the MMC specs do define a convoluted mechanism for > assigning unique ids to each card so that a bus could be implemented. > (Which doesn't apply to SD cards, by my read of the SD specs.) > >>Even if SDHCI does have >> card-detection CS control logic this can be demultiplexed to multiple >> cards by extra logic. We also have precedent of implementers of SDHCI >> opting-out of features like power control, and assumptions about how >> SDHCI is wired have already led to bugs like this: >> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html >> >> We don't need to be implementing the multi CS logic today, but making >> UI level decisions based on this 1:1 assumption seems wrong. > > I have no objection to a different internal or external represention > to the SD cards. However, the immiediate choice seems to be whether > to disable pci-sdhci or not. It seems unfortunate to me that it would > be disabled. The QEMU SD code (beyond just the sdhci controller) is > not organized as a bus, and it's not clear to me that there are real > world gains to be had by modeling it as a bus. > > My interest in the sd cards is for testing purposes. Several recent > Intel chipsets include multiple sdhci controllers on them, and several > currently shipping devices use them for both eMMC storage and for > external SD card ports - in particular several of the Chromebooks have > them. Being able to perform simple sanity checks in QEMU is helpful. The device is obviously useful. However, there is dissent on how it ought to be modelled. The modelling is visible at the -device user interface. By making the device available there in 2.5, we commit to the current modelling's user interface before we reach consensus on how it ought to be modelled. Options: (1) Make device "sdhci-pci" unavailable with -device until we reach consensus. This is what we normally do. Trivial patch is on list. (2) Mark the properties that belong to the card rather than the controller as experimental until we reach consensus, by prefixing their name with "x-". Needs a patch. (3) Keep it available, commit to the user interface, deal with the consequences if and when they arise. I think (1) is the most prudent, but (2) should work, too. Having dealt with consequences of prior modelling mistakes, I dislike 3. Opinions? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-07 8:50 ` Markus Armbruster @ 2015-12-07 9:58 ` Paolo Bonzini 2015-12-07 10:31 ` Markus Armbruster 0 siblings, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2015-12-07 9:58 UTC (permalink / raw) To: Markus Armbruster, Kevin O'Connor Cc: Peter Maydell, Peter Crosthwaite, QEMU Developers, Stefan Hajnoczi, Patch Tracking On 07/12/2015 09:50, Markus Armbruster wrote: > The device is obviously useful. However, there is dissent on how it > ought to be modelled. The modelling is visible at the -device user > interface. By making the device available there in 2.5, we commit to > the current modelling's user interface before we reach consensus on how > it ought to be modelled. Options: > > (1) Make device "sdhci-pci" unavailable with -device until we reach > consensus. This is what we normally do. Trivial patch is on list. > > (2) Mark the properties that belong to the card rather than the > controller as experimental until we reach consensus, by prefixing > their name with "x-". Needs a patch. > > (3) Keep it available, commit to the user interface, deal with the > consequences if and when they arise. > > I think (1) is the most prudent, but (2) should work, too. Having dealt > with consequences of prior modelling mistakes, I dislike 3. There have been 10 commits in 2 years to sd.c, none of them getting a step closer to qdev-ification basically. So there's no interest, which is basically explained by the fact that quite frankly SDIO is dead. I don't see any real difference between sdhci-pci and pci-serial. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-07 9:58 ` Paolo Bonzini @ 2015-12-07 10:31 ` Markus Armbruster 2015-12-07 14:32 ` Peter Maydell 0 siblings, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2015-12-07 10:31 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Patch Tracking, QEMU Developers, Peter Crosthwaite, Kevin O'Connor, Stefan Hajnoczi Paolo Bonzini <pbonzini@redhat.com> writes: > On 07/12/2015 09:50, Markus Armbruster wrote: >> The device is obviously useful. However, there is dissent on how it >> ought to be modelled. The modelling is visible at the -device user >> interface. By making the device available there in 2.5, we commit to >> the current modelling's user interface before we reach consensus on how >> it ought to be modelled. Options: >> >> (1) Make device "sdhci-pci" unavailable with -device until we reach >> consensus. This is what we normally do. Trivial patch is on list. >> >> (2) Mark the properties that belong to the card rather than the >> controller as experimental until we reach consensus, by prefixing >> their name with "x-". Needs a patch. >> >> (3) Keep it available, commit to the user interface, deal with the >> consequences if and when they arise. >> >> I think (1) is the most prudent, but (2) should work, too. Having dealt >> with consequences of prior modelling mistakes, I dislike 3. > > There have been 10 commits in 2 years to sd.c, none of them getting a > step closer to qdev-ification basically. So there's no interest, which > is basically explained by the fact that quite frankly SDIO is dead. There hasn't been progress towards qdevification because we've offered neither sticks nor carrots to the donkeys who're supposed to do the donkey-work. > I don't see any real difference between sdhci-pci and pci-serial. The difference is dissent vs. consensus. I don't have a strong opinion myself, but Peter C. seems to have. I suppose we could find rough consensus, but finding it under duress of a release isn't how we normally do it. Option (2) would make the device available for testing in 2.5 without breaking off the debate before consensus is reached or it becomes clear it cannot be reached. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-12-07 10:31 ` Markus Armbruster @ 2015-12-07 14:32 ` Peter Maydell 0 siblings, 0 replies; 25+ messages in thread From: Peter Maydell @ 2015-12-07 14:32 UTC (permalink / raw) To: Markus Armbruster Cc: Patch Tracking, QEMU Developers, Peter Crosthwaite, Kevin O'Connor, Stefan Hajnoczi, Paolo Bonzini On 7 December 2015 at 10:31, Markus Armbruster <armbru@redhat.com> wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 07/12/2015 09:50, Markus Armbruster wrote: >>> The device is obviously useful. However, there is dissent on how it >>> ought to be modelled. The modelling is visible at the -device user >>> interface. By making the device available there in 2.5, we commit to >>> the current modelling's user interface before we reach consensus on how >>> it ought to be modelled. Options: >>> >>> (1) Make device "sdhci-pci" unavailable with -device until we reach >>> consensus. This is what we normally do. Trivial patch is on list. >>> >>> (2) Mark the properties that belong to the card rather than the >>> controller as experimental until we reach consensus, by prefixing >>> their name with "x-". Needs a patch. >>> >>> (3) Keep it available, commit to the user interface, deal with the >>> consequences if and when they arise. >>> >>> I think (1) is the most prudent, but (2) should work, too. Having dealt >>> with consequences of prior modelling mistakes, I dislike 3. >> >> There have been 10 commits in 2 years to sd.c, none of them getting a >> step closer to qdev-ification basically. So there's no interest, which >> is basically explained by the fact that quite frankly SDIO is dead. > > There hasn't been progress towards qdevification because we've offered > neither sticks nor carrots to the donkeys who're supposed to do the > donkey-work. I will QOMify sd.c for 2.6. (Most of the users are ARM boards anyway.) >> I don't see any real difference between sdhci-pci and pci-serial. > > The difference is dissent vs. consensus. > > I don't have a strong opinion myself, but Peter C. seems to have. > > I suppose we could find rough consensus, but finding it under duress of > a release isn't how we normally do it. > > Option (2) would make the device available for testing in 2.5 without > breaking off the debate before consensus is reached or it becomes clear > it cannot be reached. I definitely would prefer us not to have to be stuck with a wrongly modelled sd.c. So I'd prefer option (1) or option (2). Option 2 would allow people to use the device in 2.5 at least. The release notes will also want a warning that sdhci might end up with a migration compatibility break in 2.6. (Obviously we'd try to avoid that but QOMifying sd.c might require it.) thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object 2015-09-07 16:42 ` Peter Maydell 2015-09-07 16:57 ` Markus Armbruster @ 2015-09-07 18:39 ` Peter Crosthwaite 1 sibling, 0 replies; 25+ messages in thread From: Peter Crosthwaite @ 2015-09-07 18:39 UTC (permalink / raw) To: Peter Maydell; +Cc: Patch Tracking, Markus Armbruster, QEMU Developers On Mon, Sep 7, 2015 at 9:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> Convert the pxa2xx_mmci device to be a sysbus device. > >>> +static Property pxa2xx_mmci_properties[] = { >>> + /* Note: pointer property 'drive' may remain NULL, thus no need >>> + * for dc->cannot_instantiate_with_device_add_yet = true; >>> + * Unfortunately this can't be a DEFINE_PROP_DRIVE, because >>> + * setting a 'drive' property results in a call to blk_attach_dev() >>> + * attaching the BlockBackend to this device; that then means that >>> + * the call in sd_init() to blk_attach_dev_nofail() which tries to >>> + * attach the BlockBackend to the SD card object aborts. >>> + */ >>> + DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >> >> As far as I can tell, this problem is an artifact of our interface to >> the common sd-card code, namely sd_init(). sd_init() was made for the >> pre-qdev world: it creates and completely initializes the common >> SDState. >> >> In qdev, we do this in three separate steps: create, set properties, >> realize. Split up sd_init(), and the problem should go away. > > Yes, but I don't really want to gate QOMification of mmc > controller devices on the more complicated problem of > QOMifying sd.c itself, especially since we already have several > QOMified mmc controllers... > +1. We have QOMified SDHCI already and I would assume that the SD QOMification will be made easier if the attached controllers are QOMified in their own right. Regards, Peter > thanks > -- PMM > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription 2015-08-11 14:15 [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object Peter Maydell @ 2015-08-11 14:15 ` Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 3/3] hw/sd/pxa2xx_mmci: Add reset function Peter Maydell 2015-09-07 15:34 ` [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell 3 siblings, 0 replies; 25+ messages in thread From: Peter Maydell @ 2015-08-11 14:15 UTC (permalink / raw) To: qemu-devel; +Cc: Markus Armbruster, patches Convert the pxa2xx_mmci device from manual save/load functions to a VMStateDescription structure. This is a migration compatibility break. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/sd/pxa2xx_mmci.c | 149 ++++++++++++++++++++-------------------------------- 1 file changed, 57 insertions(+), 92 deletions(-) diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c index 5b676c7..ea42434 100644 --- a/hw/sd/pxa2xx_mmci.c +++ b/hw/sd/pxa2xx_mmci.c @@ -36,27 +36,72 @@ typedef struct PXA2xxMMCIState { uint32_t cmdat; uint32_t resp_tout; uint32_t read_tout; - int blklen; - int numblk; + int32_t blklen; + int32_t numblk; uint32_t intmask; uint32_t intreq; - int cmd; + int32_t cmd; uint32_t arg; - int active; - int bytesleft; + int32_t active; + int32_t bytesleft; uint8_t tx_fifo[64]; - int tx_start; - int tx_len; + uint32_t tx_start; + uint32_t tx_len; uint8_t rx_fifo[32]; - int rx_start; - int rx_len; + uint32_t rx_start; + uint32_t rx_len; uint16_t resp_fifo[9]; - int resp_len; + uint32_t resp_len; - int cmdreq; + int32_t cmdreq; } PXA2xxMMCIState; +static bool pxa2xx_mmci_vmstate_validate(void *opaque, int version_id) +{ + PXA2xxMMCIState *s = opaque; + + return s->tx_start < sizeof(s->tx_fifo) + && s->rx_start < sizeof(s->rx_fifo) + && s->tx_len <= sizeof(s->tx_fifo) + && s->rx_len <= sizeof(s->rx_fifo) + && s->resp_len <= sizeof(s->resp_fifo); +} + + +static const VMStateDescription vmstate_pxa2xx_mmci = { + .name = "pxa2xx-mmci", + .version_id = 2, + .minimum_version_id = 2, + .fields = (VMStateField[]) { + VMSTATE_UINT32(status, PXA2xxMMCIState), + VMSTATE_UINT32(clkrt, PXA2xxMMCIState), + VMSTATE_UINT32(spi, PXA2xxMMCIState), + VMSTATE_UINT32(cmdat, PXA2xxMMCIState), + VMSTATE_UINT32(resp_tout, PXA2xxMMCIState), + VMSTATE_UINT32(read_tout, PXA2xxMMCIState), + VMSTATE_INT32(blklen, PXA2xxMMCIState), + VMSTATE_INT32(numblk, PXA2xxMMCIState), + VMSTATE_UINT32(intmask, PXA2xxMMCIState), + VMSTATE_UINT32(intreq, PXA2xxMMCIState), + VMSTATE_INT32(cmd, PXA2xxMMCIState), + VMSTATE_UINT32(arg, PXA2xxMMCIState), + VMSTATE_INT32(cmdreq, PXA2xxMMCIState), + VMSTATE_INT32(active, PXA2xxMMCIState), + VMSTATE_INT32(bytesleft, PXA2xxMMCIState), + VMSTATE_UINT32(tx_start, PXA2xxMMCIState), + VMSTATE_UINT32(tx_len, PXA2xxMMCIState), + VMSTATE_UINT32(rx_start, PXA2xxMMCIState), + VMSTATE_UINT32(rx_len, PXA2xxMMCIState), + VMSTATE_UINT32(resp_len, PXA2xxMMCIState), + VMSTATE_VALIDATE("fifo size incorrect", pxa2xx_mmci_vmstate_validate), + VMSTATE_UINT8_ARRAY(tx_fifo, PXA2xxMMCIState, 64), + VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxMMCIState, 32), + VMSTATE_UINT16_ARRAY(resp_fifo, PXA2xxMMCIState, 9), + VMSTATE_END_OF_LIST() + } +}; + #define MMC_STRPCL 0x00 /* MMC Clock Start/Stop register */ #define MMC_STAT 0x04 /* MMC Status register */ #define MMC_CLKRT 0x08 /* MMC Clock Rate register */ @@ -398,84 +443,6 @@ static const MemoryRegionOps pxa2xx_mmci_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static void pxa2xx_mmci_save(QEMUFile *f, void *opaque) -{ - PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque; - int i; - - qemu_put_be32s(f, &s->status); - qemu_put_be32s(f, &s->clkrt); - qemu_put_be32s(f, &s->spi); - qemu_put_be32s(f, &s->cmdat); - qemu_put_be32s(f, &s->resp_tout); - qemu_put_be32s(f, &s->read_tout); - qemu_put_be32(f, s->blklen); - qemu_put_be32(f, s->numblk); - qemu_put_be32s(f, &s->intmask); - qemu_put_be32s(f, &s->intreq); - qemu_put_be32(f, s->cmd); - qemu_put_be32s(f, &s->arg); - qemu_put_be32(f, s->cmdreq); - qemu_put_be32(f, s->active); - qemu_put_be32(f, s->bytesleft); - - qemu_put_byte(f, s->tx_len); - for (i = 0; i < s->tx_len; i ++) - qemu_put_byte(f, s->tx_fifo[(s->tx_start + i) & 63]); - - qemu_put_byte(f, s->rx_len); - for (i = 0; i < s->rx_len; i ++) - qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 31]); - - qemu_put_byte(f, s->resp_len); - for (i = s->resp_len; i < 9; i ++) - qemu_put_be16s(f, &s->resp_fifo[i]); -} - -static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id) -{ - PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque; - int i; - - qemu_get_be32s(f, &s->status); - qemu_get_be32s(f, &s->clkrt); - qemu_get_be32s(f, &s->spi); - qemu_get_be32s(f, &s->cmdat); - qemu_get_be32s(f, &s->resp_tout); - qemu_get_be32s(f, &s->read_tout); - s->blklen = qemu_get_be32(f); - s->numblk = qemu_get_be32(f); - qemu_get_be32s(f, &s->intmask); - qemu_get_be32s(f, &s->intreq); - s->cmd = qemu_get_be32(f); - qemu_get_be32s(f, &s->arg); - s->cmdreq = qemu_get_be32(f); - s->active = qemu_get_be32(f); - s->bytesleft = qemu_get_be32(f); - - s->tx_len = qemu_get_byte(f); - s->tx_start = 0; - if (s->tx_len >= sizeof(s->tx_fifo) || s->tx_len < 0) - return -EINVAL; - for (i = 0; i < s->tx_len; i ++) - s->tx_fifo[i] = qemu_get_byte(f); - - s->rx_len = qemu_get_byte(f); - s->rx_start = 0; - if (s->rx_len >= sizeof(s->rx_fifo) || s->rx_len < 0) - return -EINVAL; - for (i = 0; i < s->rx_len; i ++) - s->rx_fifo[i] = qemu_get_byte(f); - - s->resp_len = qemu_get_byte(f); - if (s->resp_len > 9 || s->resp_len < 0) - return -EINVAL; - for (i = s->resp_len; i < 9; i ++) - qemu_get_be16s(f, &s->resp_fifo[i]); - - return 0; -} - PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem, hwaddr base, BlockBackend *blk, qemu_irq irq, @@ -512,9 +479,6 @@ static void pxa2xx_mmci_instance_init(Object *obj) sysbus_init_irq(sbd, &s->irq); sysbus_init_irq(sbd, &s->rx_dma); sysbus_init_irq(sbd, &s->tx_dma); - - register_savevm(NULL, "pxa2xx_mmci", 0, 0, - pxa2xx_mmci_save, pxa2xx_mmci_load, s); } static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp) @@ -548,6 +512,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data) dc->realize = pxa2xx_mmci_realize; dc->props = pxa2xx_mmci_properties; + dc->vmsd = &vmstate_pxa2xx_mmci; } static const TypeInfo pxa2xx_mmci_info = { -- 1.9.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 3/3] hw/sd/pxa2xx_mmci: Add reset function 2015-08-11 14:15 [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription Peter Maydell @ 2015-08-11 14:15 ` Peter Maydell 2015-09-07 15:34 ` [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell 3 siblings, 0 replies; 25+ messages in thread From: Peter Maydell @ 2015-08-11 14:15 UTC (permalink / raw) To: qemu-devel; +Cc: Markus Armbruster, patches Add a reset function to the pxa2xx_mmci device; previously it had no handling for system reset at all. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/sd/pxa2xx_mmci.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c index ea42434..0dec8c8 100644 --- a/hw/sd/pxa2xx_mmci.c +++ b/hw/sd/pxa2xx_mmci.c @@ -468,6 +468,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly, sd_set_cb(s->card, readonly, coverswitch); } +static void pxa2xx_mmci_reset(DeviceState *d) +{ + PXA2xxMMCIState *s = PXA2XX_MMCI(d); + + s->status = 0; + s->clkrt = 0; + s->spi = 0; + s->cmdat = 0; + s->resp_tout = 0; + s->read_tout = 0; + s->blklen = 0; + s->numblk = 0; + s->intmask = 0; + s->intreq = 0; + s->cmd = 0; + s->arg = 0; + s->active = 0; + s->bytesleft = 0; + s->tx_start = 0; + s->tx_len = 0; + s->rx_start = 0; + s->rx_len = 0; + s->resp_len = 0; + s->cmdreq = 0; + memset(s->tx_fifo, 0, sizeof(s->tx_fifo)); + memset(s->rx_fifo, 0, sizeof(s->rx_fifo)); + memset(s->resp_fifo, 0, sizeof(s->resp_fifo)); +} + static void pxa2xx_mmci_instance_init(Object *obj) { PXA2xxMMCIState *s = PXA2XX_MMCI(obj); @@ -513,6 +542,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data) dc->realize = pxa2xx_mmci_realize; dc->props = pxa2xx_mmci_properties; dc->vmsd = &vmstate_pxa2xx_mmci; + dc->reset = pxa2xx_mmci_reset; } static const TypeInfo pxa2xx_mmci_info = { -- 1.9.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate 2015-08-11 14:15 [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell ` (2 preceding siblings ...) 2015-08-11 14:15 ` [Qemu-devel] [PATCH 3/3] hw/sd/pxa2xx_mmci: Add reset function Peter Maydell @ 2015-09-07 15:34 ` Peter Maydell 3 siblings, 0 replies; 25+ messages in thread From: Peter Maydell @ 2015-09-07 15:34 UTC (permalink / raw) To: QEMU Developers; +Cc: Markus Armbruster, Patch Tracking On 11 August 2015 at 15:15, Peter Maydell <peter.maydell@linaro.org> wrote: > This patchset updates the ancient pxa2xx_mmci device to something > resembling modern standards for devices. In particular it makes > it a proper sysbus device and switches to VMStateDescription structs. > > The major issue I have with this is in patch 1: > I wanted the device to have a property so its users can set > the BlockBackend* it should use. Unfortunately, DEFINE_PROP_DRIVE() > is no good here, because setting a drive property results in a > call to blk_attach_dev() which attaches the BlockBackend to this > device. That then means that the call in sd_init() to attach the > BlockBackend to the SD card object aborts. I needed a way to > have a drive property which didn't mean "and this device claims > the drive", and the best I could come up with was to use a > pointer property. Suggestions for better approaches welcome. > (The other SD controller devices are either also ancient non-QOM > devices, or use drive_get_next() in the init function...) Ping! No opinions on this (or review of the patches)? thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2015-12-07 14:33 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-11 14:15 [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object Peter Maydell 2015-09-07 16:40 ` Markus Armbruster 2015-09-07 16:42 ` Peter Maydell 2015-09-07 16:57 ` Markus Armbruster 2015-12-03 21:20 ` Peter Maydell 2015-12-04 7:30 ` Markus Armbruster 2015-12-04 11:00 ` Peter Maydell 2015-12-04 12:50 ` Markus Armbruster 2015-12-04 12:55 ` Peter Maydell 2015-12-04 13:04 ` Markus Armbruster 2015-12-04 18:49 ` Kevin O'Connor 2015-12-04 16:42 ` Paolo Bonzini 2015-12-04 18:50 ` Peter Crosthwaite 2015-12-04 19:24 ` Kevin O'Connor 2015-12-07 0:02 ` Peter Crosthwaite 2015-12-07 6:11 ` Kevin O'Connor 2015-12-07 8:50 ` Markus Armbruster 2015-12-07 9:58 ` Paolo Bonzini 2015-12-07 10:31 ` Markus Armbruster 2015-12-07 14:32 ` Peter Maydell 2015-09-07 18:39 ` Peter Crosthwaite 2015-08-11 14:15 ` [Qemu-devel] [PATCH 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription Peter Maydell 2015-08-11 14:15 ` [Qemu-devel] [PATCH 3/3] hw/sd/pxa2xx_mmci: Add reset function Peter Maydell 2015-09-07 15:34 ` [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.