Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5236 bytes --]

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 <andrew@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 <andrew@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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]