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

[Li Guang <lig.fnst@cn.fujitsu.com>]

Peter Crosthwaite wrote:
> On Wed, Dec 4, 2013 at 6:09 PM, liguang<lig.fnst@cn.fujitsu.com>  wrote:
>    
>> Signed-off-by: liguang<lig.fnst@cn.fujitsu.com>
>> ---
>>   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