[Qemu-devel] [PATCH 0/5] pxa2xx: minor bugfixes, updates to QOM, etc

[Qemu-devel] [PATCH 0/5] pxa2xx: minor bugfixes, updates to QOM, etc

[Peter Maydell]

These patches update some ancient PXA2xx devices to use
QOM, VMState and new MemoryRegionOps APIs. The main motivation
here was to get rid of a few of uses of legacy APIs
like register_savevm() and old_mmio, in the hope that eventually
we can manage to get rid of them completely...

Peter Maydell (5):
  hw/arm/pxa2xx: Mark coprocessor registers as ARM_CP_IO
  hw/arm/pxa2xx: Convert pxa2xx-fir to QOM and VMState
  hw/arm/pxa2xx: Add reset method for pxa2xx_ssp
  hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState
  hw/sd/pxa2xx_mmci: Stop using old_mmio in MemoryRegionOps

 hw/arm/pxa2xx.c     | 244 ++++++++++++++++++++++++++++------------------------
 hw/arm/pxa2xx_pic.c |   2 +-
 hw/sd/pxa2xx_mmci.c |  68 ++-------------
 3 files changed, 142 insertions(+), 172 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] hw/arm/pxa2xx: Mark coprocessor registers as ARM_CP_IO

[Peter Maydell]

The pxa2xx custom coprocessor registers in cp6 and cp14 do device
accesses, so mark the non-constant regs as ARM_CP_IO so that
icount works correctly and doesn't abort.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/pxa2xx.c     | 8 ++++----
 hw/arm/pxa2xx_pic.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f921a56..8123f05 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -334,10 +334,10 @@ static uint64_t pxa2xx_cpccnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static const ARMCPRegInfo pxa_cp_reginfo[] = {
     /* cp14 crm==1: perf registers */
     { .name = "CPPMNC", .cp = 14, .crn = 0, .crm = 1, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_IO,
       .readfn = pxa2xx_cppmnc_read, .writefn = pxa2xx_cppmnc_write },
     { .name = "CPCCNT", .cp = 14, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_IO,
       .readfn = pxa2xx_cpccnt_read, .writefn = arm_cp_write_ignore },
     { .name = "CPINTEN", .cp = 14, .crn = 4, .crm = 1, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
@@ -356,11 +356,11 @@ static const ARMCPRegInfo pxa_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
     /* cp14 crn==6: CLKCFG */
     { .name = "CLKCFG", .cp = 14, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_IO,
       .readfn = pxa2xx_clkcfg_read, .writefn = pxa2xx_clkcfg_write },
     /* cp14 crn==7: PWRMODE */
     { .name = "PWRMODE", .cp = 14, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_IO,
       .readfn = arm_cp_read_zero, .writefn = pxa2xx_pwrmode_write },
     REGINFO_SENTINEL
 };
diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
index 9cfc714..d41ac93 100644
--- a/hw/arm/pxa2xx_pic.c
+++ b/hw/arm/pxa2xx_pic.c
@@ -232,7 +232,7 @@ static void pxa2xx_pic_cp_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 #define REGINFO_FOR_PIC_CP(NAME, CRN) \
     { .name = NAME, .cp = 6, .crn = CRN, .crm = 0, .opc1 = 0, .opc2 = 0, \
-      .access = PL1_RW, \
+      .access = PL1_RW, .type = ARM_CP_IO, \
       .readfn = pxa2xx_pic_cp_read, .writefn = pxa2xx_pic_cp_write }
 
 static const ARMCPRegInfo pxa_pic_cp_reginfo[] = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/5] hw/arm/pxa2xx: Mark coprocessor registers as ARM_CP_IO

[Peter Crosthwaite]

On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The pxa2xx custom coprocessor registers in cp6 and cp14 do device
> accesses, so mark the non-constant regs as ARM_CP_IO so that
> icount works correctly and doesn't abort.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

I'm guessing some of the more trivial callback fn's don't actually
need this but I think setting the IO flag across the board is probably
the correct policy.

Regards,
Peter

> ---
>  hw/arm/pxa2xx.c     | 8 ++++----
>  hw/arm/pxa2xx_pic.c | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f921a56..8123f05 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -334,10 +334,10 @@ static uint64_t pxa2xx_cpccnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  static const ARMCPRegInfo pxa_cp_reginfo[] = {
>      /* cp14 crm==1: perf registers */
>      { .name = "CPPMNC", .cp = 14, .crn = 0, .crm = 1, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .type = ARM_CP_IO,
>        .readfn = pxa2xx_cppmnc_read, .writefn = pxa2xx_cppmnc_write },
>      { .name = "CPCCNT", .cp = 14, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .type = ARM_CP_IO,
>        .readfn = pxa2xx_cpccnt_read, .writefn = arm_cp_write_ignore },
>      { .name = "CPINTEN", .cp = 14, .crn = 4, .crm = 1, .opc1 = 0, .opc2 = 0,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -356,11 +356,11 @@ static const ARMCPRegInfo pxa_cp_reginfo[] = {
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      /* cp14 crn==6: CLKCFG */
>      { .name = "CLKCFG", .cp = 14, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .type = ARM_CP_IO,
>        .readfn = pxa2xx_clkcfg_read, .writefn = pxa2xx_clkcfg_write },
>      /* cp14 crn==7: PWRMODE */
>      { .name = "PWRMODE", .cp = 14, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .type = ARM_CP_IO,
>        .readfn = arm_cp_read_zero, .writefn = pxa2xx_pwrmode_write },
>      REGINFO_SENTINEL
>  };
> diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
> index 9cfc714..d41ac93 100644
> --- a/hw/arm/pxa2xx_pic.c
> +++ b/hw/arm/pxa2xx_pic.c
> @@ -232,7 +232,7 @@ static void pxa2xx_pic_cp_write(CPUARMState *env, const ARMCPRegInfo *ri,
>
>  #define REGINFO_FOR_PIC_CP(NAME, CRN) \
>      { .name = NAME, .cp = 6, .crn = CRN, .crm = 0, .opc1 = 0, .opc2 = 0, \
> -      .access = PL1_RW, \
> +      .access = PL1_RW, .type = ARM_CP_IO, \
>        .readfn = pxa2xx_pic_cp_read, .writefn = pxa2xx_pic_cp_write }
>
>  static const ARMCPRegInfo pxa_pic_cp_reginfo[] = {
> --
> 1.9.1
>
>

[Qemu-devel] [PATCH 2/5] hw/arm/pxa2xx: Convert pxa2xx-fir to QOM and VMState

[Peter Maydell]

Convert the pxa2xx-fir device to QOM, including using a
VMState for its migration info.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/pxa2xx.c | 137 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 82 insertions(+), 55 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 8123f05..fc77b44 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1759,24 +1759,33 @@ static PXA2xxI2SState *pxa2xx_i2s_init(MemoryRegion *sysmem,
 }
 
 /* PXA Fast Infra-red Communications Port */
+#define TYPE_PXA2XX_FIR "pxa2xx-fir"
+#define PXA2XX_FIR(obj) OBJECT_CHECK(PXA2xxFIrState, (obj), TYPE_PXA2XX_FIR)
+
 struct PXA2xxFIrState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     MemoryRegion iomem;
     qemu_irq irq;
     qemu_irq rx_dma;
     qemu_irq tx_dma;
-    int enable;
+    uint32_t enable;
     CharDriverState *chr;
 
     uint8_t control[3];
     uint8_t status[2];
 
-    int rx_len;
-    int rx_start;
+    uint32_t rx_len;
+    uint32_t rx_start;
     uint8_t rx_fifo[64];
 };
 
-static void pxa2xx_fir_reset(PXA2xxFIrState *s)
+static void pxa2xx_fir_reset(DeviceState *d)
 {
+    PXA2xxFIrState *s = PXA2XX_FIR(d);
+
     s->control[0] = 0x00;
     s->control[1] = 0x00;
     s->control[2] = 0x00;
@@ -1953,73 +1962,91 @@ static void pxa2xx_fir_event(void *opaque, int event)
 {
 }
 
-static void pxa2xx_fir_save(QEMUFile *f, void *opaque)
+static void pxa2xx_fir_instance_init(Object *obj)
 {
-    PXA2xxFIrState *s = (PXA2xxFIrState *) opaque;
-    int i;
-
-    qemu_put_be32(f, s->enable);
-
-    qemu_put_8s(f, &s->control[0]);
-    qemu_put_8s(f, &s->control[1]);
-    qemu_put_8s(f, &s->control[2]);
-    qemu_put_8s(f, &s->status[0]);
-    qemu_put_8s(f, &s->status[1]);
+    PXA2xxFIrState *s = PXA2XX_FIR(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    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) & 63]);
+    memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s,
+                          "pxa2xx-fir", 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
 }
 
-static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
+static void pxa2xx_fir_realize(DeviceState *dev, Error **errp)
 {
-    PXA2xxFIrState *s = (PXA2xxFIrState *) opaque;
-    int i;
+    PXA2xxFIrState *s = PXA2XX_FIR(dev);
 
-    s->enable = qemu_get_be32(f);
-
-    qemu_get_8s(f, &s->control[0]);
-    qemu_get_8s(f, &s->control[1]);
-    qemu_get_8s(f, &s->control[2]);
-    qemu_get_8s(f, &s->status[0]);
-    qemu_get_8s(f, &s->status[1]);
+    if (s->chr) {
+        qemu_chr_fe_claim_no_fail(s->chr);
+        qemu_chr_add_handlers(s->chr, pxa2xx_fir_is_empty,
+                        pxa2xx_fir_rx, pxa2xx_fir_event, s);
+    }
+}
 
-    s->rx_len = qemu_get_byte(f);
-    s->rx_start = 0;
-    for (i = 0; i < s->rx_len; i ++)
-        s->rx_fifo[i] = qemu_get_byte(f);
+static bool pxa2xx_fir_vmstate_validate(void *opaque, int version_id)
+{
+    PXA2xxFIrState *s = opaque;
 
-    return 0;
+    return s->rx_start < sizeof(s->rx_fifo);
 }
 
-static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion *sysmem,
-                hwaddr base,
-                qemu_irq irq, qemu_irq rx_dma, qemu_irq tx_dma,
-                CharDriverState *chr)
-{
-    PXA2xxFIrState *s = (PXA2xxFIrState *)
-            g_malloc0(sizeof(PXA2xxFIrState));
+static const VMStateDescription pxa2xx_fir_vmsd = {
+    .name = "pxa2xx-fir",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(enable, PXA2xxFIrState),
+        VMSTATE_UINT8_ARRAY(control, PXA2xxFIrState, 3),
+        VMSTATE_UINT8_ARRAY(status, PXA2xxFIrState, 2),
+        VMSTATE_UINT32(rx_len, PXA2xxFIrState),
+        VMSTATE_UINT32(rx_start, PXA2xxFIrState),
+        VMSTATE_VALIDATE("fifo is 64 bytes", pxa2xx_fir_vmstate_validate),
+        VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxFIrState, 64),
+        VMSTATE_END_OF_LIST()
+    }
+};
 
-    s->irq = irq;
-    s->rx_dma = rx_dma;
-    s->tx_dma = tx_dma;
-    s->chr = chr;
+static Property pxa2xx_fir_properties[] = {
+    DEFINE_PROP_CHR("chardev", PXA2xxFIrState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
-    pxa2xx_fir_reset(s);
+static void pxa2xx_fir_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
-    memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s, "pxa2xx-fir", 0x1000);
-    memory_region_add_subregion(sysmem, base, &s->iomem);
+    dc->realize = pxa2xx_fir_realize;
+    dc->vmsd = &pxa2xx_fir_vmsd;
+    dc->props = pxa2xx_fir_properties;
+    dc->reset = pxa2xx_fir_reset;
+}
 
-    if (chr) {
-        qemu_chr_fe_claim_no_fail(chr);
-        qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
-                        pxa2xx_fir_rx, pxa2xx_fir_event, s);
-    }
+static const TypeInfo pxa2xx_fir_info = {
+    .name = TYPE_PXA2XX_FIR,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PXA2xxFIrState),
+    .class_init = pxa2xx_fir_class_init,
+    .instance_init = pxa2xx_fir_instance_init,
+};
 
-    register_savevm(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
-                    pxa2xx_fir_load, s);
+static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion *sysmem,
+                                       hwaddr base,
+                                       qemu_irq irq, qemu_irq rx_dma,
+                                       qemu_irq tx_dma,
+                                       CharDriverState *chr)
+{
+    DeviceState *dev;
+    SysBusDevice *sbd;
 
-    return s;
+    dev = qdev_create(NULL, TYPE_PXA2XX_FIR);
+    qdev_prop_set_chr(dev, "chardev", chr);
+    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_FIR(dev);
 }
 
 static void pxa2xx_reset(void *opaque, int line, int level)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/5] hw/arm/pxa2xx: Convert pxa2xx-fir to QOM and VMState

[Peter Crosthwaite]

On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Convert the pxa2xx-fir device to QOM, including using a
> VMState for its migration info.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/pxa2xx.c | 137 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 82 insertions(+), 55 deletions(-)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 8123f05..fc77b44 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1759,24 +1759,33 @@ static PXA2xxI2SState *pxa2xx_i2s_init(MemoryRegion *sysmem,
>  }
>
>  /* PXA Fast Infra-red Communications Port */
> +#define TYPE_PXA2XX_FIR "pxa2xx-fir"
> +#define PXA2XX_FIR(obj) OBJECT_CHECK(PXA2xxFIrState, (obj), TYPE_PXA2XX_FIR)
> +
>  struct PXA2xxFIrState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
>      MemoryRegion iomem;
>      qemu_irq irq;
>      qemu_irq rx_dma;
>      qemu_irq tx_dma;
> -    int enable;
> +    uint32_t enable;
>      CharDriverState *chr;
>
>      uint8_t control[3];
>      uint8_t status[2];
>
> -    int rx_len;
> -    int rx_start;
> +    uint32_t rx_len;
> +    uint32_t rx_start;
>      uint8_t rx_fifo[64];
>  };
>
> -static void pxa2xx_fir_reset(PXA2xxFIrState *s)
> +static void pxa2xx_fir_reset(DeviceState *d)
>  {
> +    PXA2xxFIrState *s = PXA2XX_FIR(d);
> +
>      s->control[0] = 0x00;
>      s->control[1] = 0x00;
>      s->control[2] = 0x00;
> @@ -1953,73 +1962,91 @@ static void pxa2xx_fir_event(void *opaque, int event)
>  {
>  }
>
> -static void pxa2xx_fir_save(QEMUFile *f, void *opaque)
> +static void pxa2xx_fir_instance_init(Object *obj)
>  {
> -    PXA2xxFIrState *s = (PXA2xxFIrState *) opaque;
> -    int i;
> -
> -    qemu_put_be32(f, s->enable);
> -
> -    qemu_put_8s(f, &s->control[0]);
> -    qemu_put_8s(f, &s->control[1]);
> -    qemu_put_8s(f, &s->control[2]);
> -    qemu_put_8s(f, &s->status[0]);
> -    qemu_put_8s(f, &s->status[1]);
> +    PXA2xxFIrState *s = PXA2XX_FIR(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>
> -    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) & 63]);
> +    memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s,
> +                          "pxa2xx-fir", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);

Should there be some sysbus_init_irq's?

Regards,
Peter

>  }
>
> -static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
> +static void pxa2xx_fir_realize(DeviceState *dev, Error **errp)
>  {
> -    PXA2xxFIrState *s = (PXA2xxFIrState *) opaque;
> -    int i;
> +    PXA2xxFIrState *s = PXA2XX_FIR(dev);

Re: [Qemu-devel] [PATCH 2/5] hw/arm/pxa2xx: Convert pxa2xx-fir to QOM and VMState

[Peter Maydell]

On 6 June 2015 at 02:07, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Should there be some sysbus_init_irq's?

Oops, yes...

(I don't have a test for this device specifically, so I haven't
been able to test beyond "does the zaurus image I have still boot?".)

-- PMM

[Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Maydell]

The pxa2xx_ssp device was missing a reset method; add one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/pxa2xx.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index fc77b44..770902f 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void pxa2xx_ssp_reset(DeviceState *d)
+{
+    PXA2xxSSPState *s = PXA2XX_SSP(d);
+
+    s->enable = 0;
+    s->sscr[0] = s->sscr[1] = 0;
+    s->sspsp = 0;
+    s->ssto = 0;
+    s->ssitr = 0;
+    s->sssr = 0;
+    s->sstsa = 0;
+    s->ssrsa = 0;
+    s->ssacd = 0;
+    s->rx_start = s->rx_level = 0;
+}
+
 static int pxa2xx_ssp_init(SysBusDevice *sbd)
 {
     DeviceState *dev = DEVICE(sbd);
@@ -2333,8 +2349,10 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
 static void pxa2xx_ssp_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
     sdc->init = pxa2xx_ssp_init;
+    dc->reset = pxa2xx_ssp_reset;
 }
 
 static const TypeInfo pxa2xx_ssp_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Crosthwaite]

On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The pxa2xx_ssp device was missing a reset method; add one.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/pxa2xx.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index fc77b44..770902f 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>
> +static void pxa2xx_ssp_reset(DeviceState *d)
> +{
> +    PXA2xxSSPState *s = PXA2XX_SSP(d);
> +
> +    s->enable = 0;
> +    s->sscr[0] = s->sscr[1] = 0;
> +    s->sspsp = 0;
> +    s->ssto = 0;
> +    s->ssitr = 0;
> +    s->sssr = 0;
> +    s->sstsa = 0;
> +    s->ssrsa = 0;
> +    s->ssacd = 0;
> +    s->rx_start = s->rx_level = 0;

Does this need a ssp_int_update to deassert any set interrupts?

Regards,
Peter

> +}
> +
>  static int pxa2xx_ssp_init(SysBusDevice *sbd)
>  {
>      DeviceState *dev = DEVICE(sbd);
> @@ -2333,8 +2349,10 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>  static void pxa2xx_ssp_class_init(ObjectClass *klass, void *data)
>  {
>      SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>
>      sdc->init = pxa2xx_ssp_init;
> +    dc->reset = pxa2xx_ssp_reset;
>  }
>
>  static const TypeInfo pxa2xx_ssp_info = {
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Maydell]

On 5 June 2015 at 23:57, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The pxa2xx_ssp device was missing a reset method; add one.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/pxa2xx.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index fc77b44..770902f 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
>>      return 0;
>>  }
>>
>> +static void pxa2xx_ssp_reset(DeviceState *d)
>> +{
>> +    PXA2xxSSPState *s = PXA2XX_SSP(d);
>> +
>> +    s->enable = 0;
>> +    s->sscr[0] = s->sscr[1] = 0;
>> +    s->sspsp = 0;
>> +    s->ssto = 0;
>> +    s->ssitr = 0;
>> +    s->sssr = 0;
>> +    s->sstsa = 0;
>> +    s->ssrsa = 0;
>> +    s->ssacd = 0;
>> +    s->rx_start = s->rx_level = 0;
>
> Does this need a ssp_int_update to deassert any set interrupts?

No, because the thing on the other end of the irq line should also
reset itself to an "interrupt deasserted" state. Calling qemu_set_irq
or qemu_reset_irq in a reset function is dubious, because you don't
know if the device on the other end has (a) already reset itself
or (b) not yet reset itself but will do so after you; so the effect
on the other device is indeterminate...

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Crosthwaite]

On Fri, Jun 5, 2015 at 4:00 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 June 2015 at 23:57, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> The pxa2xx_ssp device was missing a reset method; add one.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  hw/arm/pxa2xx.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>>> index fc77b44..770902f 100644
>>> --- a/hw/arm/pxa2xx.c
>>> +++ b/hw/arm/pxa2xx.c
>>> @@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
>>>      return 0;
>>>  }
>>>
>>> +static void pxa2xx_ssp_reset(DeviceState *d)
>>> +{
>>> +    PXA2xxSSPState *s = PXA2XX_SSP(d);
>>> +
>>> +    s->enable = 0;
>>> +    s->sscr[0] = s->sscr[1] = 0;
>>> +    s->sspsp = 0;
>>> +    s->ssto = 0;
>>> +    s->ssitr = 0;
>>> +    s->sssr = 0;
>>> +    s->sstsa = 0;
>>> +    s->ssrsa = 0;
>>> +    s->ssacd = 0;
>>> +    s->rx_start = s->rx_level = 0;
>>
>> Does this need a ssp_int_update to deassert any set interrupts?
>
> No, because the thing on the other end of the irq line should also
> reset itself to an "interrupt deasserted" state. Calling qemu_set_irq
> or qemu_reset_irq in a reset function is dubious, because you don't
> know if the device on the other end has (a) already reset itself
> or (b) not yet reset itself but will do so after you; so the effect
> on the other device is indeterminate...
>

Ok so out of scope of this patches so:

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

But I have some follow up design questions, mainly in that I have
started using individual device resets that back onto the ->reset hook
to implement reset controllers. This means an IRQ source should reset
its IRQ pin to notify its sinks of the state change as those sinks
will not get a reset callback.

I know its not supported by the current reset semantic but can we
start doing this going forward?

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Maydell]

On 6 June 2015 at 00:06, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> But I have some follow up design questions, mainly in that I have
> started using individual device resets that back onto the ->reset hook
> to implement reset controllers. This means an IRQ source should reset
> its IRQ pin to notify its sinks of the state change as those sinks
> will not get a reset callback.
>
> I know its not supported by the current reset semantic but can we
> start doing this going forward?

Device reset is a sink and a quagmire. Note incidentally that what
we call "reset" in QEMU is actually "we hard powercycled the simulation",
not an emulated reset. If you can propose some coherent and workable
semantics I think it would be nice if we could clean things up.
(You may find you need two-phase reset by analogy with two-phase init...)

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Crosthwaite]

On Fri, Jun 5, 2015 at 4:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 June 2015 at 00:06, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> But I have some follow up design questions, mainly in that I have
>> started using individual device resets that back onto the ->reset hook
>> to implement reset controllers. This means an IRQ source should reset
>> its IRQ pin to notify its sinks of the state change as those sinks
>> will not get a reset callback.
>>
>> I know its not supported by the current reset semantic but can we
>> start doing this going forward?
>
> Device reset is a sink and a quagmire. Note incidentally that what
> we call "reset" in QEMU is actually "we hard powercycled the simulation",
> not an emulated reset. If you can propose some coherent and workable
> semantics I think it would be nice if we could clean things up.
> (You may find you need two-phase reset by analogy with two-phase init...)
>

So I don't really like the predetermined finite number of init phases
as the number of phases seems to just grow over time. We already have
about 4 phases of init if you count all of init() post_init(),
realize() and machine_init_done-notifiers. Resets will probably go the
same way. Can we instead have a dependency management system where
reset handlers can register deps? If you rely on another device for
your reset (e.g. a boot-loader must wait out a CPU reset) then it
makes sense you have awareness of that dep and can use a QOM link
navigation to register a reset dep. Devs can register multiple resets
if deps demand but I would expect that to be super rare.

The resetter functionality then ensures a topological ordering of the
resets for the reset.

IRQs and GPIOs can form loops though, so I think it is a case of
machines that support partial reset need to update their GPIOs on
reset to remove a large number of reset ordering deps.

So my semantics are:

* The reset function must reset the device to POR state.
* Resets may depend on any number of other resets to occur first. No
loops in the deps.
* If a device is reset concurrently with one of its deps, the dep is
guaranteed to go first
* a dev with externally provided state that doesn't register reset dep
may need to preserve that state through reset (e.g. state of a GPIO
input).
* Systems can register reset domains, with reset operations respecting
deps within
* It is an error to define a reset domain that doesn't include all deps.
* By extension a trivial one-device domain can implement an individual
core reset (if the platform/dev supports that)

FWIW, I already have reset GPIOs for ARM CPUs that back onto
device::reset. These in turn connect to the reset controller to
support run time CPU reset via hardware peripheral.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/pxa2xx: Add reset method for pxa2xx_ssp

[Peter Maydell]

On 6 June 2015 at 02:37, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, Jun 5, 2015 at 4:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Device reset is a sink and a quagmire. Note incidentally that what
>> we call "reset" in QEMU is actually "we hard powercycled the simulation",
>> not an emulated reset. If you can propose some coherent and workable
>> semantics I think it would be nice if we could clean things up.
>> (You may find you need two-phase reset by analogy with two-phase init...)
>>
>
> So I don't really like the predetermined finite number of init phases
> as the number of phases seems to just grow over time. We already have
> about 4 phases of init if you count all of init() post_init(),
> realize() and machine_init_done-notifiers. Resets will probably go the
> same way. Can we instead have a dependency management system where
> reset handlers can register deps? If you rely on another device for
> your reset (e.g. a boot-loader must wait out a CPU reset) then it
> makes sense you have awareness of that dep and can use a QOM link
> navigation to register a reset dep. Devs can register multiple resets
> if deps demand but I would expect that to be super rare.

I don't think this will work, because as a device you don't know
what you're connected to. The idea I had was something like:
 * phase 1, all devices reset their internal state and reset
   outbound signal lines accordingly
   They also set a flag to say "being held in reset"; calls
   inbound for "your gpio line changed" are only handled to
   note the new state of the line, not to trigger device state change
 * phase 2, devices go out of reset: they do whatever the device
   does when it leaves reset and samples the state of its inbound
   signals (which might result in changing state, outbound signal
   levels, etc)

These basically correspond to "leading edge of reset" and
"trailing edge of reset". If you're modelling them with QEMU
IRQs then they'd be "things done when level goes 0->1" and
"things done when level goes 1->0".

There is an obvious problem here, though, which is that this
is (a) an enormous upheaval in every device and (b) breaking
migration, because of all the new state.

Anthony Liguori had a vaguely similar idea at one point IIRC,
but done by making the QEMU IRQ lines maintain internal state
(at the moment they don't at all) rather than having the device
do it. It's probably worth trawling the list archives for earlier
discussions and ideas...

> So my semantics are:
>
> * The reset function must reset the device to POR state.
> * Resets may depend on any number of other resets to occur first. No
> loops in the deps.
> * If a device is reset concurrently with one of its deps, the dep is
> guaranteed to go first
> * a dev with externally provided state that doesn't register reset dep
> may need to preserve that state through reset (e.g. state of a GPIO
> input).
> * Systems can register reset domains, with reset operations respecting
> deps within
> * It is an error to define a reset domain that doesn't include all deps.
> * By extension a trivial one-device domain can implement an individual
> core reset (if the platform/dev supports that)

How many cases really need this dependency-driven reset rather
than just being "I have an external signal that tells me when
to reset" ? There are oddities like the boot loader, but is there
anything else?

> FWIW, I already have reset GPIOs for ARM CPUs that back onto
> device::reset. These in turn connect to the reset controller to
> support run time CPU reset via hardware peripheral.

Right, this is what I would think of as the "normal case".

-- PMM

[Qemu-devel] [PATCH 4/5] hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState

[Peter Maydell]

The pxa2xx-ssp device is already a QOM device but is still
using the old-style register_savevm(); convert to VMState.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/pxa2xx.c | 89 +++++++++++++++++++++------------------------------------
 1 file changed, 33 insertions(+), 56 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 770902f..09401f9 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -457,7 +457,7 @@ typedef struct {
 
     MemoryRegion iomem;
     qemu_irq irq;
-    int enable;
+    uint32_t enable;
     SSIBus *bus;
 
     uint32_t sscr[2];
@@ -470,10 +470,39 @@ typedef struct {
     uint8_t ssacd;
 
     uint32_t rx_fifo[16];
-    int rx_level;
-    int rx_start;
+    uint32_t rx_level;
+    uint32_t rx_start;
 } PXA2xxSSPState;
 
+static bool pxa2xx_ssp_vmstate_validate(void *opaque, int version_id)
+{
+    PXA2xxSSPState *s = opaque;
+
+    return s->rx_start < sizeof(s->rx_fifo);
+}
+
+static const VMStateDescription vmstate_pxa2xx_ssp = {
+    .name = "pxa2xx-ssp",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(enable, PXA2xxSSPState),
+        VMSTATE_UINT32_ARRAY(sscr, PXA2xxSSPState, 2),
+        VMSTATE_UINT32(sspsp, PXA2xxSSPState),
+        VMSTATE_UINT32(ssto, PXA2xxSSPState),
+        VMSTATE_UINT32(ssitr, PXA2xxSSPState),
+        VMSTATE_UINT32(sssr, PXA2xxSSPState),
+        VMSTATE_UINT8(sstsa, PXA2xxSSPState),
+        VMSTATE_UINT8(ssrsa, PXA2xxSSPState),
+        VMSTATE_UINT8(ssacd, PXA2xxSSPState),
+        VMSTATE_UINT32(rx_level, PXA2xxSSPState),
+        VMSTATE_UINT32(rx_start, PXA2xxSSPState),
+        VMSTATE_VALIDATE("fifo is 16 bytes", pxa2xx_ssp_vmstate_validate),
+        VMSTATE_UINT32_ARRAY(rx_fifo, PXA2xxSSPState, 16),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define SSCR0	0x00	/* SSP Control register 0 */
 #define SSCR1	0x04	/* SSP Control register 1 */
 #define SSSR	0x08	/* SSP Status register */
@@ -705,57 +734,6 @@ static const MemoryRegionOps pxa2xx_ssp_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pxa2xx_ssp_save(QEMUFile *f, void *opaque)
-{
-    PXA2xxSSPState *s = (PXA2xxSSPState *) opaque;
-    int i;
-
-    qemu_put_be32(f, s->enable);
-
-    qemu_put_be32s(f, &s->sscr[0]);
-    qemu_put_be32s(f, &s->sscr[1]);
-    qemu_put_be32s(f, &s->sspsp);
-    qemu_put_be32s(f, &s->ssto);
-    qemu_put_be32s(f, &s->ssitr);
-    qemu_put_be32s(f, &s->sssr);
-    qemu_put_8s(f, &s->sstsa);
-    qemu_put_8s(f, &s->ssrsa);
-    qemu_put_8s(f, &s->ssacd);
-
-    qemu_put_byte(f, s->rx_level);
-    for (i = 0; i < s->rx_level; i ++)
-        qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 0xf]);
-}
-
-static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
-{
-    PXA2xxSSPState *s = (PXA2xxSSPState *) opaque;
-    int i, v;
-
-    s->enable = qemu_get_be32(f);
-
-    qemu_get_be32s(f, &s->sscr[0]);
-    qemu_get_be32s(f, &s->sscr[1]);
-    qemu_get_be32s(f, &s->sspsp);
-    qemu_get_be32s(f, &s->ssto);
-    qemu_get_be32s(f, &s->ssitr);
-    qemu_get_be32s(f, &s->sssr);
-    qemu_get_8s(f, &s->sstsa);
-    qemu_get_8s(f, &s->ssrsa);
-    qemu_get_8s(f, &s->ssacd);
-
-    v = qemu_get_byte(f);
-    if (v < 0 || v > ARRAY_SIZE(s->rx_fifo)) {
-        return -EINVAL;
-    }
-    s->rx_level = v;
-    s->rx_start = 0;
-    for (i = 0; i < s->rx_level; i ++)
-        s->rx_fifo[i] = qemu_get_byte(f);
-
-    return 0;
-}
-
 static void pxa2xx_ssp_reset(DeviceState *d)
 {
     PXA2xxSSPState *s = PXA2XX_SSP(d);
@@ -782,8 +760,6 @@ static int pxa2xx_ssp_init(SysBusDevice *sbd)
     memory_region_init_io(&s->iomem, OBJECT(s), &pxa2xx_ssp_ops, s,
                           "pxa2xx-ssp", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
-    register_savevm(dev, "pxa2xx_ssp", -1, 0,
-                    pxa2xx_ssp_save, pxa2xx_ssp_load, s);
 
     s->bus = ssi_create_bus(dev, "ssi");
     return 0;
@@ -2353,6 +2329,7 @@ static void pxa2xx_ssp_class_init(ObjectClass *klass, void *data)
 
     sdc->init = pxa2xx_ssp_init;
     dc->reset = pxa2xx_ssp_reset;
+    dc->vmsd = &vmstate_pxa2xx_ssp;
 }
 
 static const TypeInfo pxa2xx_ssp_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/5] hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState

[Peter Crosthwaite]

On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The pxa2xx-ssp device is already a QOM device but is still
> using the old-style register_savevm(); convert to VMState.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/pxa2xx.c | 89 +++++++++++++++++++++------------------------------------
>  1 file changed, 33 insertions(+), 56 deletions(-)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 770902f..09401f9 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -457,7 +457,7 @@ typedef struct {
>
>      MemoryRegion iomem;
>      qemu_irq irq;
> -    int enable;
> +    uint32_t enable;
>      SSIBus *bus;
>
>      uint32_t sscr[2];
> @@ -470,10 +470,39 @@ typedef struct {
>      uint8_t ssacd;
>
>      uint32_t rx_fifo[16];
> -    int rx_level;
> -    int rx_start;
> +    uint32_t rx_level;
> +    uint32_t rx_start;
>  } PXA2xxSSPState;
>
> +static bool pxa2xx_ssp_vmstate_validate(void *opaque, int version_id)
> +{
> +    PXA2xxSSPState *s = opaque;
> +
> +    return s->rx_start < sizeof(s->rx_fifo);

Does this need to be ARRAY_SIZE to account for unit32_t indexing?

Regards,
Peter

> +}
> +
> +static const VMStateDescription vmstate_pxa2xx_ssp = {
> +    .name = "pxa2xx-ssp",
> +    .version_id = 1,
> +    .minimum_version_id = 1,

Re: [Qemu-devel] [PATCH 4/5] hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState

[Peter Maydell]

On 6 June 2015 at 00:18, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The pxa2xx-ssp device is already a QOM device but is still
>> using the old-style register_savevm(); convert to VMState.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/pxa2xx.c | 89 +++++++++++++++++++++------------------------------------
>>  1 file changed, 33 insertions(+), 56 deletions(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index 770902f..09401f9 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -457,7 +457,7 @@ typedef struct {
>>
>>      MemoryRegion iomem;
>>      qemu_irq irq;
>> -    int enable;
>> +    uint32_t enable;
>>      SSIBus *bus;
>>
>>      uint32_t sscr[2];
>> @@ -470,10 +470,39 @@ typedef struct {
>>      uint8_t ssacd;
>>
>>      uint32_t rx_fifo[16];
>> -    int rx_level;
>> -    int rx_start;
>> +    uint32_t rx_level;
>> +    uint32_t rx_start;
>>  } PXA2xxSSPState;
>>
>> +static bool pxa2xx_ssp_vmstate_validate(void *opaque, int version_id)
>> +{
>> +    PXA2xxSSPState *s = opaque;
>> +
>> +    return s->rx_start < sizeof(s->rx_fifo);
>
> Does this need to be ARRAY_SIZE to account for unit32_t indexing?

Yes, good catch.

-- PMM

Re: [Qemu-devel] [PATCH 4/5] hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState

[Peter Crosthwaite]

On Fri, Jun 5, 2015 at 4:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 June 2015 at 00:18, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> The pxa2xx-ssp device is already a QOM device but is still
>>> using the old-style register_savevm(); convert to VMState.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  hw/arm/pxa2xx.c | 89 +++++++++++++++++++++------------------------------------
>>>  1 file changed, 33 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>>> index 770902f..09401f9 100644
>>> --- a/hw/arm/pxa2xx.c
>>> +++ b/hw/arm/pxa2xx.c
>>> @@ -457,7 +457,7 @@ typedef struct {
>>>
>>>      MemoryRegion iomem;
>>>      qemu_irq irq;
>>> -    int enable;
>>> +    uint32_t enable;
>>>      SSIBus *bus;
>>>
>>>      uint32_t sscr[2];
>>> @@ -470,10 +470,39 @@ typedef struct {
>>>      uint8_t ssacd;
>>>
>>>      uint32_t rx_fifo[16];
>>> -    int rx_level;
>>> -    int rx_start;
>>> +    uint32_t rx_level;
>>> +    uint32_t rx_start;
>>>  } PXA2xxSSPState;
>>>
>>> +static bool pxa2xx_ssp_vmstate_validate(void *opaque, int version_id)
>>> +{
>>> +    PXA2xxSSPState *s = opaque;
>>> +
>>> +    return s->rx_start < sizeof(s->rx_fifo);
>>
>> Does this need to be ARRAY_SIZE to account for unit32_t indexing?
>
> Yes, good catch.
>

Ok that's all I found. So otherwise,

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

I assume this is intended to break backwards compat on the VMSD with
the conversion from put_byte loop to VMSTATE_UINT32_ARRAY? I'm not
sure what our policy is on these old ARM machines and preserving the
backwards compat.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH 4/5] hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState

[Peter Maydell]

On 6 June 2015 at 01:49, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Ok that's all I found. So otherwise,
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks!

> I assume this is intended to break backwards compat on the VMSD with
> the conversion from put_byte loop to VMSTATE_UINT32_ARRAY? I'm not
> sure what our policy is on these old ARM machines and preserving the
> backwards compat.

We don't currently care about back-compat on migration state
for any ARM boards (and definitely not for these ancient
devboard models).

-- PMM

[Qemu-devel] [PATCH 5/5] hw/sd/pxa2xx_mmci: Stop using old_mmio in MemoryRegionOps

[Peter Maydell]

Update the pxa2xx_mmci device to stop using the old_mmio read
and write callbacks in its MemoryRegionOps. This actually
simplifies the code because the separate byte/halfword/word
access functions were all calling into a single function to
do the work anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 68 +++++++----------------------------------------------
 1 file changed, 8 insertions(+), 60 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index ac3ab39..d1fe6d5 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -48,7 +48,6 @@ struct PXA2xxMMCIState {
     int resp_len;
 
     int cmdreq;
-    int ac_width;
 };
 
 #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
@@ -215,7 +214,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
     pxa2xx_mmci_fifo_update(s);
 }
 
-static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
+static uint64_t pxa2xx_mmci_read(void *opaque, hwaddr offset, unsigned size)
 {
     PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
     uint32_t ret;
@@ -257,8 +256,8 @@ static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
         return 0;
     case MMC_RXFIFO:
         ret = 0;
-        while (s->ac_width -- && s->rx_len) {
-            ret |= s->rx_fifo[s->rx_start ++] << (s->ac_width << 3);
+        while (size-- && s->rx_len) {
+            ret |= s->rx_fifo[s->rx_start++] << (size << 3);
             s->rx_start &= 0x1f;
             s->rx_len --;
         }
@@ -277,7 +276,7 @@ static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
 }
 
 static void pxa2xx_mmci_write(void *opaque,
-                hwaddr offset, uint32_t value)
+                              hwaddr offset, uint64_t value, unsigned size)
 {
     PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
 
@@ -370,9 +369,9 @@ static void pxa2xx_mmci_write(void *opaque,
         break;
 
     case MMC_TXFIFO:
-        while (s->ac_width -- && s->tx_len < 0x20)
+        while (size-- && s->tx_len < 0x20)
             s->tx_fifo[(s->tx_start + (s->tx_len ++)) & 0x1f] =
-                    (value >> (s->ac_width << 3)) & 0xff;
+                    (value >> (size << 3)) & 0xff;
         s->intreq &= ~INT_TXFIFO_REQ;
         pxa2xx_mmci_fifo_update(s);
         break;
@@ -386,60 +385,9 @@ static void pxa2xx_mmci_write(void *opaque,
     }
 }
 
-static uint32_t pxa2xx_mmci_readb(void *opaque, hwaddr offset)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 1;
-    return pxa2xx_mmci_read(opaque, offset);
-}
-
-static uint32_t pxa2xx_mmci_readh(void *opaque, hwaddr offset)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 2;
-    return pxa2xx_mmci_read(opaque, offset);
-}
-
-static uint32_t pxa2xx_mmci_readw(void *opaque, hwaddr offset)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 4;
-    return pxa2xx_mmci_read(opaque, offset);
-}
-
-static void pxa2xx_mmci_writeb(void *opaque,
-                hwaddr offset, uint32_t value)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 1;
-    pxa2xx_mmci_write(opaque, offset, value);
-}
-
-static void pxa2xx_mmci_writeh(void *opaque,
-                hwaddr offset, uint32_t value)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 2;
-    pxa2xx_mmci_write(opaque, offset, value);
-}
-
-static void pxa2xx_mmci_writew(void *opaque,
-                hwaddr offset, uint32_t value)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    s->ac_width = 4;
-    pxa2xx_mmci_write(opaque, offset, value);
-}
-
 static const MemoryRegionOps pxa2xx_mmci_ops = {
-    .old_mmio = {
-        .read = { pxa2xx_mmci_readb,
-                  pxa2xx_mmci_readh,
-                  pxa2xx_mmci_readw, },
-        .write = { pxa2xx_mmci_writeb,
-                   pxa2xx_mmci_writeh,
-                   pxa2xx_mmci_writew, },
-    },
+    .read = pxa2xx_mmci_read,
+    .write = pxa2xx_mmci_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/5] hw/sd/pxa2xx_mmci: Stop using old_mmio in MemoryRegionOps

[Peter Crosthwaite]

On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Update the pxa2xx_mmci device to stop using the old_mmio read
> and write callbacks in its MemoryRegionOps. This actually
> simplifies the code because the separate byte/halfword/word
> access functions were all calling into a single function to
> do the work anyway.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/sd/pxa2xx_mmci.c | 68 +++++++----------------------------------------------
>  1 file changed, 8 insertions(+), 60 deletions(-)
>
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index ac3ab39..d1fe6d5 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -48,7 +48,6 @@ struct PXA2xxMMCIState {
>      int resp_len;
>
>      int cmdreq;
> -    int ac_width;
>  };
>
>  #define MMC_STRPCL     0x00    /* MMC Clock Start/Stop register */
> @@ -215,7 +214,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
>      pxa2xx_mmci_fifo_update(s);
>  }
>
> -static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
> +static uint64_t pxa2xx_mmci_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
>      uint32_t ret;
> @@ -257,8 +256,8 @@ static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
>          return 0;
>      case MMC_RXFIFO:
>          ret = 0;
> -        while (s->ac_width -- && s->rx_len) {
> -            ret |= s->rx_fifo[s->rx_start ++] << (s->ac_width << 3);
> +        while (size-- && s->rx_len) {
> +            ret |= s->rx_fifo[s->rx_start++] << (size << 3);
>              s->rx_start &= 0x1f;
>              s->rx_len --;
>          }
> @@ -277,7 +276,7 @@ static uint32_t pxa2xx_mmci_read(void *opaque, hwaddr offset)
>  }
>
>  static void pxa2xx_mmci_write(void *opaque,
> -                hwaddr offset, uint32_t value)
> +                              hwaddr offset, uint64_t value, unsigned size)
>  {
>      PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
>
> @@ -370,9 +369,9 @@ static void pxa2xx_mmci_write(void *opaque,
>          break;
>
>      case MMC_TXFIFO:
> -        while (s->ac_width -- && s->tx_len < 0x20)
> +        while (size-- && s->tx_len < 0x20)
>              s->tx_fifo[(s->tx_start + (s->tx_len ++)) & 0x1f] =
> -                    (value >> (s->ac_width << 3)) & 0xff;
> +                    (value >> (size << 3)) & 0xff;
>          s->intreq &= ~INT_TXFIFO_REQ;
>          pxa2xx_mmci_fifo_update(s);
>          break;
> @@ -386,60 +385,9 @@ static void pxa2xx_mmci_write(void *opaque,
>      }
>  }
>
> -static uint32_t pxa2xx_mmci_readb(void *opaque, hwaddr offset)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 1;
> -    return pxa2xx_mmci_read(opaque, offset);
> -}
> -
> -static uint32_t pxa2xx_mmci_readh(void *opaque, hwaddr offset)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 2;
> -    return pxa2xx_mmci_read(opaque, offset);
> -}
> -
> -static uint32_t pxa2xx_mmci_readw(void *opaque, hwaddr offset)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 4;
> -    return pxa2xx_mmci_read(opaque, offset);
> -}
> -
> -static void pxa2xx_mmci_writeb(void *opaque,
> -                hwaddr offset, uint32_t value)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 1;
> -    pxa2xx_mmci_write(opaque, offset, value);
> -}
> -
> -static void pxa2xx_mmci_writeh(void *opaque,
> -                hwaddr offset, uint32_t value)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 2;
> -    pxa2xx_mmci_write(opaque, offset, value);
> -}
> -
> -static void pxa2xx_mmci_writew(void *opaque,
> -                hwaddr offset, uint32_t value)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    s->ac_width = 4;
> -    pxa2xx_mmci_write(opaque, offset, value);
> -}
> -
>  static const MemoryRegionOps pxa2xx_mmci_ops = {
> -    .old_mmio = {
> -        .read = { pxa2xx_mmci_readb,
> -                  pxa2xx_mmci_readh,
> -                  pxa2xx_mmci_readw, },
> -        .write = { pxa2xx_mmci_writeb,
> -                   pxa2xx_mmci_writeh,
> -                   pxa2xx_mmci_writew, },
> -    },
> +    .read = pxa2xx_mmci_read,
> +    .write = pxa2xx_mmci_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 0/5] pxa2xx: minor bugfixes, updates to QOM, etc

[Peter Maydell]

Ping? Peter C, maybe you'd like to review these, given they're
mostly QOM conversions?

thanks
-- PMM

On 28 May 2015 at 13:09, Peter Maydell <peter.maydell@linaro.org> wrote:
> These patches update some ancient PXA2xx devices to use
> QOM, VMState and new MemoryRegionOps APIs. The main motivation
> here was to get rid of a few of uses of legacy APIs
> like register_savevm() and old_mmio, in the hope that eventually
> we can manage to get rid of them completely...
>
> Peter Maydell (5):
>   hw/arm/pxa2xx: Mark coprocessor registers as ARM_CP_IO
>   hw/arm/pxa2xx: Convert pxa2xx-fir to QOM and VMState
>   hw/arm/pxa2xx: Add reset method for pxa2xx_ssp
>   hw/arm/pxa2xx: Convert pxa2xx-ssp to VMState
>   hw/sd/pxa2xx_mmci: Stop using old_mmio in MemoryRegionOps
>
>  hw/arm/pxa2xx.c     | 244 ++++++++++++++++++++++++++++------------------------
>  hw/arm/pxa2xx_pic.c |   2 +-
>  hw/sd/pxa2xx_mmci.c |  68 ++-------------
>  3 files changed, 142 insertions(+), 172 deletions(-)