On Tue, 2016-03-15 at 14:14 +0100, Cédric Le Goater wrote: > On 03/14/2016 05:13 AM, Andrew Jeffery wrote: > > Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to > > 8 timers can independently be configured, enabled, reset and disabled. > > Some hardware features are not implemented, namely clock value matching > > and pulse generation, but the implementation is enough to boot the Linux > > kernel configured with aspeed_defconfig. > > > > [1] http://www.aspeedtech.com/products.php?fPath=20&rId=376 > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au> > > Looks good. One stylistic comment and a possible compile break in > timer_to_ctrl(). > > > > > --- > > Since v3: > > * Drop unnecessary mention of VMStateDescription in timer_to_ctrl > > description > > * Mention hw/timer/a9gtimer.c with respect to clock value matching > > * Add missing VMSTATE_END_OF_LIST() to vmstate_aspeed_timer_state > > > > Since v2: > > * Improve handling of timer configuration with respect to enabled state > > * Remove redundant enabled member from AspeedTimer > > * Implement VMStateDescriptions > > * Fix interrupt behaviour (edge triggered, both edges) > > * Fix various issues with trace-event declarations > > * Include qemu/osdep.h > > > > Since v1: > > * Refactor initialisation of and respect requested clock rates > > (APB/External) > > * Simplify some index calculations > > * Use tracing infrastructure instead of internal DPRINTF > > * Enforce access size constraints and alignment in MemoryRegionOps > > > > default-configs/arm-softmmu.mak | 1 + > > hw/timer/Makefile.objs | 1 + > > hw/timer/aspeed_timer.c | 451 > > ++++++++++++++++++++++++++++++++++++++++ > > include/hw/timer/aspeed_timer.h | 59 ++++++ > > trace-events | 9 + > > 5 files changed, 521 insertions(+) > > create mode 100644 hw/timer/aspeed_timer.c > > create mode 100644 include/hw/timer/aspeed_timer.h > > > > diff --git a/default-configs/arm-softmmu.mak > > b/default-configs/arm-softmmu.mak > > index a9f82a1..2bcd236 100644 > > --- a/default-configs/arm-softmmu.mak > > +++ b/default-configs/arm-softmmu.mak > > @@ -110,3 +110,4 @@ CONFIG_IOH3420=y > > CONFIG_I82801B11=y > > CONFIG_ACPI=y > > CONFIG_SMBIOS=y > > +CONFIG_ASPEED_SOC=y > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > > index 5cfea6e..003c14f 100644 > > --- a/hw/timer/Makefile.objs > > +++ b/hw/timer/Makefile.objs > > @@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o > > obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o > > > > common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o > > +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > > new file mode 100644 > > index 0000000..0e82178 > > --- /dev/null > > +++ b/hw/timer/aspeed_timer.c > > @@ -0,0 +1,452 @@ > > +/* > > + * ASPEED AST2400 Timer > > + * > > + * Andrew Jeffery <and...@aj.id.au> > > + * > > + * Copyright (C) 2016 IBM Corp. > > + * > > + * This code is licensed under the GPL version 2 or later. See > > + * the COPYING file in the top-level directory. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "hw/ptimer.h" > > +#include "hw/sysbus.h" > > +#include "hw/timer/aspeed_timer.h" > > +#include "qemu-common.h" > > +#include "qemu/bitops.h" > > +#include "qemu/main-loop.h" > > +#include "qemu/timer.h" > > +#include "trace.h" > > + > > +#define TIMER_NR_REGS 4 > > + > > +#define TIMER_CTRL_BITS 4 > > +#define TIMER_CTRL_MASK ((1 << TIMER_CTRL_BITS) - 1) > > + > > +#define TIMER_CLOCK_USE_EXT true > > +#define TIMER_CLOCK_EXT_HZ 1000000 > > +#define TIMER_CLOCK_USE_APB false > > +#define TIMER_CLOCK_APB_HZ 24000000 > > + > > +#define TIMER_REG_STATUS 0 > > +#define TIMER_REG_RELOAD 1 > > +#define TIMER_REG_MATCH_FIRST 2 > > +#define TIMER_REG_MATCH_SECOND 3 > > + > > +#define TIMER_FIRST_CAP_PULSE 4 > > + > > +enum timer_ctrl_op { > > + op_enable = 0, > > + op_external_clock, > > + op_overflow_interrupt, > > + op_pulse_enable > > +}; > > + > > +/** > > + * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer > > + * structs, as it's a waste of memory. The ptimer BH callback needs to know > > + * whether a specific AspeedTimer is enabled, but this information is held > > in > > + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an > > + * arbitrary AspeedTimer to AspeedTimerCtrlState. > > + */ > > +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t) > > > you can remove the 'struct' above.
Good catch, will do.
>
> > +{
> > + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
>
> This will not compile on gcc < 5.0. You need to add a 'const' :
>
> const AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
>
> That should work on all versions.
Thanks, I've confirmed the failure and fix with gcc-4.7. I'll make sure
to test with the earliest gcc easily available to me (looks like it's
4.7, on Ubuntu Wily) going forward.
Cheers,
Andrew
signature.asc
Description: This is a digitally signed message part
