Re: [Qemu-devel] [PATCH 07/10] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Fri, Dec 11, 2015 at 04:37:08PM +0000, Peter Maydell wrote:
> Convert the pxa2xx_mmci device to be a sysbus device.
> 
> In this commit we only change the device itself, and leave
> the interface to the SD card using the old non-SDBus APIs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 73 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index b217080..b6bb390 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -11,16 +11,24 @@
>   */
>  
>  #include "hw/hw.h"
> +#include "hw/sysbus.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sd/sd.h"
>  #include "hw/qdev.h"
> +#include "hw/qdev-properties.h"
> +
> +#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
> +#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
> +
> +typedef struct PXA2xxMMCIState {
> +    SysBusDevice parent_obj;
>  
> -struct PXA2xxMMCIState {
>      MemoryRegion iomem;
>      qemu_irq irq;
>      qemu_irq rx_dma;
>      qemu_irq tx_dma;
>  
> +    BlockBackend *blk;
>      SDState *card;
>  
>      uint32_t status;
> @@ -48,7 +56,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 +482,60 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  BlockBackend *blk, qemu_irq irq,
>                  qemu_irq rx_dma, qemu_irq tx_dma)
>  {
> +    DeviceState *dev;
> +    SysBusDevice *sbd;
>      PXA2xxMMCIState *s;
>  
> -    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
> -    s->irq = irq;
> -    s->rx_dma = rx_dma;
> -    s->tx_dma = tx_dma;
> -
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
> -                          "pxa2xx-mmci", 0x00100000);
> -    memory_region_add_subregion(sysmem, base, &s->iomem);
> -
> -    /* Instantiate the actual storage */
> -    s->card = sd_init(blk, false);
> +    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
> +    s = PXA2XX_MMCI(dev);
> +    /* Reach into the device and initialize the SD card. This is
> +     * unclean but will vanish when we update to SDBus APIs.
> +     */
> +    s->card = sd_init(s->blk, false);
>      if (s->card == NULL) {
>          exit(1);
>      }
> -
> -    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> -                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> -
> +    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);

This looks like heterogenous GPIOs so should be separate named GPIO
sets. Looking at the machine model, the irq goes to the "pic" but the
DMA signals go to a different IP. I would leave the IRQ as the one and
only sysbus IRQ, but implement the two DMAs as named GPIOs.

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>      return s;
>  }
>  
>  void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> -                qemu_irq coverswitch)
> +                          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, obj, &pxa2xx_mmci_ops, s,
> +                          "pxa2xx-mmci", 0x00100000);
> +    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 const TypeInfo pxa2xx_mmci_info = {
> +    .name = TYPE_PXA2XX_MMCI,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PXA2xxMMCIState),
> +    .instance_init = pxa2xx_mmci_instance_init,
> +};
> +
> +static void pxa2xx_mmci_register_types(void)
> +{
> +    type_register_static(&pxa2xx_mmci_info);
> +}
> +
> +type_init(pxa2xx_mmci_register_types)
> -- 
> 1.9.1
>