Re: [Qemu-devel] [PATCH v8 4/5] hw/arm: add allwinner a10 SoC support

[Li Guang]

Peter Crosthwaite wrote:
> On Wed, Dec 4, 2013 at 6:09 PM, liguang<address@hidden>  wrote:
>    
> > Signed-off-by: liguang<address@hidden>
> > ---
> >   hw/arm/Makefile.objs           |    2 +-
> >   hw/arm/allwinner-a10.c         |   77 ++++++++++++++++++++++++++++++++++++++++
> >   include/hw/arm/allwinner-a10.h |   36 ++++++++++++++++++
> >   3 files changed, 114 insertions(+), 1 deletions(-)
> >   create mode 100644 hw/arm/allwinner-a10.c
> >   create mode 100644 include/hw/arm/allwinner-a10.h
> >
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index 3671b42..b9e5983 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -4,4 +4,4 @@ obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
> >   obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
> >
> >   obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
> > -obj-y += omap1.o omap2.o strongarm.o
> > +obj-y += omap1.o omap2.o strongarm.o allwinner-a10.o
> > diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> > new file mode 100644
> > index 0000000..c4699b7
> > --- /dev/null
> > +++ b/hw/arm/allwinner-a10.c
> > @@ -0,0 +1,77 @@
> > +#include "hw/sysbus.h"
> > +#include "hw/devices.h"
> > +#include "hw/arm/allwinner-a10.h"
> > +
> > +
> > +static void aw_a10_init(Object *obj)
> > +{
> > +    AwA10State *s = AW_A10(obj);
> > +    DeviceState *dev;
> > +
> > +    object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
> > +    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> > +
> > +    object_initialize(&s->intc, sizeof(s->timer), TYPE_AW_A10_PIC);
> > +    dev = DEVICE(&s->intc);
> >      
> You could just use DEVICE(foo) inline here and below and drop the dev
> variable to reduce verbosity.
>
>    
> > +    qdev_set_parent_bus(dev, sysbus_get_default());
> > +
> > +    object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
> > +    dev = DEVICE(&s->timer);
> > +    qdev_set_parent_bus(dev, sysbus_get_default());
> > +}
> > +
> > +static void aw_a10_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AwA10State *s = AW_A10(dev);
> > +    SysBusDevice *sysbusdev;
> > +    uint8_t i;
> > +    Error *err = NULL;
> > +
> > +    object_property_set_bool(OBJECT(&s->cpu), true, "realized",&err);
> > +    s->cpu_irq[0] = qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_IRQ);
> > +    s->cpu_irq[1] = qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_FIQ);
> > +
> > +    object_property_set_bool(OBJECT(&s->intc), true, "realized",&err);
> >      
> You neither assert nor propagate the error. So the repeated usage
> below will cause a delayed (and somewhat obscure) assertion below if
> both happen to error for some reason.
>
>    
> > +    sysbusdev = SYS_BUS_DEVICE(&s->intc);
> > +    sysbus_mmio_map(sysbusdev, 0, AW_A10_PIC_REG_BASE);
> > +    sysbus_connect_irq(sysbusdev, 0, s->cpu_irq[0]);
> > +    sysbus_connect_irq(sysbusdev, 1, s->cpu_irq[1]);
> > +    for (i = 0; i<  AW_A10_PIC_INT_NR; i++) {
> > +        s->irq[i] = qdev_get_gpio_in(DEVICE(&s->intc), i);
> > +    }
> > +
> > +    object_property_set_bool(OBJECT(&s->timer), true, "realized",&err);
> > +    sysbusdev = SYS_BUS_DEVICE(&s->timer);
> > +    sysbus_mmio_map(sysbusdev, 0, AW_A10_PIT_REG_BASE);
> > +    sysbus_connect_irq(sysbusdev, 0, s->irq[22]);
> > +    sysbus_connect_irq(sysbusdev, 1, s->irq[23]);
> > +    sysbus_connect_irq(sysbusdev, 2, s->irq[24]);
> > +    sysbus_connect_irq(sysbusdev, 3, s->irq[25]);
> > +    sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
> > +    sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
> > +
> > +    serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
> > +                   115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
> > +}
> > +
> > +static void aw_a10_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = aw_a10_realize;
> > +}
> > +
> > +static const TypeInfo aw_a10_type_info = {
> > +    .name = TYPE_AW_A10,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(AwA10State),
> > +    .instance_init = aw_a10_init,
> > +    .class_init = aw_a10_class_init,
> > +};
> > +
> > +static void aw_a10_register_types(void)
> > +{
> > +    type_register_static(&aw_a10_type_info);
> > +}
> > +
> > +type_init(aw_a10_register_types)
> > diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> > new file mode 100644
> > index 0000000..a3e7b77
> > --- /dev/null
> > +++ b/include/hw/arm/allwinner-a10.h
> > @@ -0,0 +1,36 @@
> > +#ifndef ALLWINNER_H_
> > +
> > +#include "qemu-common.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/char/serial.h"
> > +#include "hw/arm/arm.h"
> > +#include "hw/timer/allwinner-a10-pit.h"
> > +#include "hw/intc/allwinner-a10-pic.h"
> > +
> > +#include "sysemu/sysemu.h"
> > +#include "exec/address-spaces.h"
> > +
> > +
> > +#define AW_A10_PIC_REG_BASE     0x01c20400
> > +#define AW_A10_PIT_REG_BASE     0x01c20c00
> > +#define AW_A10_UART0_REG_BASE   0x01c28000
> > +
> > +#define AW_A10_SDRAM_BASE       0x40000000
> > +
> > +#define TYPE_AW_A10 "allwiner-a10"
> >      
> "allwinner"
>
>    
> > +#define AW_A10(obj) OBJECT_CHECK(AwA10State, (obj), TYPE_AW_A10)
> > +
> > +typedef struct AwA10State {
> > +    /*<  private>*/
> > +    DeviceState parent_obj;
> > +    /*<  public>*/
> > +
> > +    ARMCPU cpu;
> > +    qemu_irq irq[AW_A10_PIC_INT_NR];
> > +    qemu_irq cpu_irq[2];
> >      
> I dont see the need to keep these as device state. They appear to be
> just local variables to realize().
>
>    
> > +    AwA10PITState timer;
> > +    AwA10PICState intc;
> > +} AwA10State;
> > +
> > +#define ALLWINNER_H_
> > +#endif
> > --
> > 1.7.2.5
> >
> >
> >      
>    
OK, will fix.

Thanks!
Li Guang