Le 16-04-2013 11:50, Peter Maydell a écrit :
> On 15 April 2013 16:41, François Legal <francois.le...@thom.fr.eu.org> wrote: > >> I made up this patch to implement the Cortex A9 global timer in Qemu. My patch is based on the Qemu branch maintained by Xilinx for the Zynq. > > Hi François; thanks for this patch. Some comments on the code below. > > Firstly, if you could send future versions of this patch in the > standard QEMU format that would be helpful: > * text/plain mail, not multipart with an HTML part > * commit message at the top describing the patch, with comments > below a '---' line > * Signed-off-by: line in the commit message itself > * please submit patches based on git.qemu.org's master branch, > not on other trees > > http://wiki.qemu.org/Contribute/SubmitAPatch [1]has some other helpful > suggestions. This is correct. I made my best to follow the guidelines, but failed for some of them. My point here was that globaltimer was required for my application based on the Xilinx tree, and the comments on that tree tend to let me believe they were quite close to the official tree. Sorry about the format I did not pay attention to my mail reader. This is my very first post to this list. > Onto the code: > >> diff -urN qemu-master/hw/cpu/a9mpcore.c qemu-master.new/hw/cpu/a9mpcore.c --- qemu-master/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200 +++ qemu-master.new/hw/cpu/a9mpcore.c 2013-04-15 12:54:06.000000000 +0200 @@ -15,6 +15,7 @@ uint32_t num_cpu; MemoryRegion container; DeviceState *mptimer; + DeviceState *mpgtimer; DeviceState *wdt; DeviceState *gic; DeviceState *scu; @@ -31,6 +32,7 @@ { A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev); SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev; + SysBusDevice *gtimerbusdev; int i; s->gic = qdev_create(NULL, "arm_gic"); @@ -50,6 +52,11 @@ qdev_init_nofail(s->scu); scubusdev = SYS_BUS_DEVICE(s->scu); + s->mpgtimer = qdev_create(NULL, "arm_mp_globaltimer"); > > I think a better name for the device would be "a9-globaltimer". > This fits our convention of preferring '-' rather than '_' > in new device names, and makes it clear that the global > timer is only used for the A9. (The private timers are used > also by the 11MPCore.) Alright. I'll fix this is the next version of patch + qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu); + qdev_init_nofail(s->mpgtimer); + gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer > um_cpu); qdev_init_nofail(s->mptimer); @@ -68,8 +75,6 @@ * 0x0600-0x06ff -- private timers and watchdogs * 0x0700-0x0fff -- nothing * 0x1000-0x1fff -- GIC Distributor - * - * We should implement the global timer but don't currently do so. */ memory_region_init(&s->container, "a9mp-priv-container", 0x2000); memory_region_add_subregion(&s->container, 0, @@ -80,6 +85,8 @@ /* Note that the A9 exposes only the "timer/watchdog for this core" * memory region, not the "timer/watchdog for core X" ones 11MPcore has. */ + memory_region_add_subregion(&s->container, 0x200, + sysbus_mmio_get_region(gtimerbusdev, 0)); memory_region_add_subregion(&s->container, 0x600, sysbus_mmio_get_region(timerbusdev, 0)); memory_region_add_subregion(&s->container, 0x620, @@ -90,10 +97,13 @@ sysbus_init_mmio(dev, &s->container); /* Wire up the interrupt from each watchdog and timer. - * For each core the timer is PPI 29 and the watchdog PPI 30. + * For each core the global timer is PPI 27, the private + * timer is PPI 29 and the watchdog PPI 30. */ for (i = 0; i < s->num_cpu; i++) { int ppibase = (s->num_irq - 32) + i * 32; + sysbus_connect_irq(gtimerbusdev, i, + qdev_get_gpio_in(s->gic, ppibase + 27)); sysbus_connect_irq(timerbusdev, i, qdev_get_gpio_in(s->gic, ppibase + 29)); sysbus_connect_irq(wdtbusdev, i, diff -urN qemu-master/hw/timer/arm_mpgtimer.c qemu-master.new/hw/timer/arm_mpgtimer.c --- qemu-master/hw/timer/arm_mpgtimer.c 1970-01-01 01:00:00.000000000 +0100 +++ qemu-master.new/hw/timer/arm_mpgtimer.c 2013-04-15 13:56:23.000000000 +0200 > > The file should also be renamed: 'hw/timer/a9gtimer.c' > > Alright. I'll fix this is the next version of patch > > @@ -0,0 +1,359 @@ +/* + * Global peripheral timer block Core and A9MP This isn't used in 11MPCore. Alright. I'll fix this is the next version of the patch + int i; + for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) { + gtimer_reset(&s->gtimer[i]); + } +} + +static int arm_mpgtimer_init(SysBusDevice *dev) +{ + ARMMPGTimerState *s = FROM_SYSBUS(ARMMPGTimerState, dev); + int i; + if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) { + hw_error("%s: num-cpu must be between 1 and %dn", __func__, MAX_CPUS); If you make this a DeviceClass realize method (see below) > br />Alright. I'll fix this is the next version of patch > >>> + } + + /* We implement one timer block per CPU, and expose multiple MMIO regions: + * * region 0 is "timer for this core" + * * region 1 is "timer for core 0" + * * region 2 is "timer for core 1" + * and so on. >> >> Why implement multiple memory regions? There is only one >> for the global timer. > > Sorry. Copy/Paste... > > + * The outgoing interrupt lines are + * * timer for core 0 + * * timer for core 1 + * and so on. + */ + memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s, + "arm_mptimer_gtimer", 0x20); + sysbus_init_mmio(dev, &s->iomem); + for (i = 0; i < s->num_cpu; i++) { + gTimerBlock *gtb = &s->gtimer[i]; + gtb->gtimer_counter = &s->gtimer_counter; imer_control = &s->gtimer_control; + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb); + sysbus_init_irq(dev, >b->irq); + memory_region_init_io(>b->iomem, >imerblock_ops, gtb, + "arm_mptimer_gtimerblock", 0x20); + sysbus_init_mmio(dev, >b->iomem); + } + + return 0; +} + +static const VMStateDescription vmstate_gtimerblock = { + .name = "arm_mptimer_gtimerblock", + .version_id = 2, + .minimum_version_id = 2, Why vers> dding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%"> + .fields = (VMStateField[]) { + VMSTATE_UINT32(control, gTimerBlock), + VMSTATE_UINT32(status, gTimerBlock), + VMSTATE_UINT64(c rBlock), + VMSTATE_INT64(tick, gTimerBlock), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_arm_mpgtimer = { + .name = "arm_mp_globaltimer", + .version_id = 2, + .minimum_version_id = 2, + .fields = (VMStateField[]) { + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, ARMMPGTimerState, num_cpu, + 1, vmstate_gtimerblock, gTimerBlock), + VMSTATE_END_OF_LIST() + } +}; + +static Property arm_mpgtimer_properties[] = { + DEFINE_PROP_UINT32("num-cpu", ARMMPGTimerState, num_cpu, 0), + DEFINE_PROP_END_OF_LIST() +}; + +static void arm_mpgtimer_class_init(ObjectClass *klass, void *d > sbc->init = arm_mpgtimer_init; > > Can you use the DeviceClass realize method rather than > SysBusDeviceClass init, please? (see eg a9scu.c for an > example.) > > Alright. I'll fix this is the next version of patch > > + dc->vmsd = &vmstate_arm_mpgtimer; + dc->reset = arm_mpgtimer_reset; + dc->no_user = 1; + dc->props = arm_mpgtimer_properties; +} + t TypeInfo arm_mpgtimer_info = { + .name = "arm_mp_globaltimer", + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(ARMMPGTimerState), + .class_init = arm_mpgtimer_class_init, +}; + +static void arm_mpgtimer_register_types(void) +{ + type_register_static(&arm_mpgtimer_info); > w/timer/arm_mptimer.c --- qemu-master/hw/timer/arm_mptimer.c 2013-04-08 20:12:33.000000000 +0200 +++ qemu-master.new/hw/timer/arm_mptimer.c 2013-04-15 13:44:33.000000000 +0200 @@ -49,13 +49,19 @@ static inline int get_current_cpu(ARMMPTimerState *s) { - CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env); + CPUState *cpu_single_cpu; - if (cpu_single_cpu->cpu_index >= s->num_cpu) { - hw_error("arm_mptimer: num-cpu pu is %d!n", - s->num_cpu, cpu_single_cpu->cpu_index); + if (cpu_single_env != NULL) { + cpu_single_cpu = ENV_GET_CPU(cpu_single_env); + + if (cpu_single_cpu->cpu_index >= s->num_cpu) { + hw_error("arm_mptimer: num-cpu %d but this cpu is %d!n", + s->num_cpu, cpu_single_cpu->cpu_index); + } + return cpu_single_cpu->cpu_index; + } else { + return 0; } - return cpu_single_cpu > ockquote> > > This is for the same reason I modified the get_current_cpu for global timer. > > static inline void timerblock_update_irq(TimerBlock *tb) diff -urN qemu-master/hw/timer/Makefile.objs qemu-master.new/hw/timer/Makefile.objs --- qemu-master/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000 +0200 +++ qemu-master.new/hw/timer/Makefile.objs 2013-04-15 12:54:06.000000000 +0200 @@ -24,5 +24,5 @@ obj-$(CONFIG_SH4) += sh_timer.o obj-$(CONFIG_TUSB6010) += tusb6010.o -obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o +obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o arm_mpgtimer.o obj-$(CONFIG_MC146818RTC) += mc146818rtc.o Signed-off-by: François LEGAL <de...@thom.fr.eu.org> > > thanks > -- PMM > > > Links: ------ [1] http://wiki.qemu.org/Contribute/SubmitAPatch
