On Tue, Mar 25, 2014 at 09:49:21AM +1000, Peter Crosthwaite wrote:
> On Fri, Mar 21, 2014 at 7:25 AM, Beniamino Galvani <b.galv...@gmail.com>
> wrote:
> > This implements the prescaler and source fields of the timer control
> > register. The source for each timer can be selected among 4 clock
> > inputs whose frequencies are set through model properties.
> >
> > Signed-off-by: Beniamino Galvani <b.galv...@gmail.com>
> > ---
> > hw/arm/cubieboard.c | 13 ++++++++++
> > hw/timer/allwinner-a10-pit.c | 43
> > +++++++++++++++++++++++++++++++++-
> > include/hw/timer/allwinner-a10-pit.h | 4 ++++
> > 3 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> > index d95a7f3..9d158c7 100644
> > --- a/hw/arm/cubieboard.c
> > +++ b/hw/arm/cubieboard.c
> > @@ -43,6 +43,19 @@ static void cubieboard_init(QEMUMachineInitArgs *args)
> > exit(1);
> > }
> >
> > + object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq",
> > &err);
> > + if (err != NULL) {
> > + error_report("Couldn't set clk0 frequency: %s",
> > error_get_pretty(err));
> > + exit(1);
> > + }
> > +
> > + object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
> > + &err);
> > + if (err != NULL) {
> > + error_report("Couldn't set clk1 frequency: %s",
> > error_get_pretty(err));
> > + exit(1);
> > + }
> > +
> > object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
> > if (err != NULL) {
> > error_report("Couldn't realize Allwinner A10: %s",
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index 5aa78a9..5c1ffba 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -74,6 +74,37 @@ static uint64_t a10_pit_read(void *opaque, hwaddr
> > offset, unsigned size)
> > return 0;
> > }
> >
> > +static void a10_pit_set_freq(AwA10PITState *s, int index)
> > +{
> > + uint32_t prescaler, source;
> > + uint32_t source_freq;
> > +
> > + prescaler = 1 << extract32(s->control[index], 4, 3);
> > + source = extract32(s->control[index], 2, 2);
> > +
> > + switch (source) {
> > + case 0:
> > + source_freq = s->clk0_freq;
> > + break;
> > + case 1:
> > + source_freq = s->clk1_freq;
> > + break;
> > + case 2:
> > + source_freq = s->clk2_freq;
> > + break;
> > + case 3:
> > + source_freq = s->clk3_freq;
> > + break;
> > + }
>
> You can get rid of this [1] ...
>
> > +
> > + if (source_freq) {
> > + ptimer_set_freq(s->timer[index], source_freq / prescaler);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
> > + __func__, source);
> > + }
> > +}
> > +
> > static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> > unsigned size)
> > {
> > @@ -96,6 +127,7 @@ static void a10_pit_write(void *opaque, hwaddr offset,
> > uint64_t value,
> > switch (offset & 0x0f) {
> > case AW_A10_PIT_TIMER_CONTROL:
> > s->control[index] = value;
> > + a10_pit_set_freq(s, index);
> > if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
> > ptimer_set_count(s->timer[index], s->interval[index]);
> > }
> > @@ -161,6 +193,14 @@ static const MemoryRegionOps a10_pit_ops = {
> > .endianness = DEVICE_NATIVE_ENDIAN,
> > };
> >
> > +static Property a10_pit_properties[] = {
> > + DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk0_freq, 0),
>
> ... [1] by changing this to:
>
> DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk_freq[0], 0),
>
> > + DEFINE_PROP_UINT32("clk1-freq", AwA10PITState, clk1_freq, 0),
> > + DEFINE_PROP_UINT32("clk2-freq", AwA10PITState, clk2_freq, 0),
> > + DEFINE_PROP_UINT32("clk3-freq", AwA10PITState, clk3_freq, 0),
>
>
> Note that there is an array property system (DEFINE_PROP_ARRAY) that
> should kinda work here. Although it requires you to set the length of
> the array on the setter level, which should be fixed to 4 in this
> case. Check the "db-voltage" property of arm_sysctrl and its usage in
> vexpress. But I can't see a clean way of fixing this imminently
> without patching that property system. Curious it works nicely for
> dynamic arrays but not static (unless I am missing something).
>
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > static const VMStateDescription vmstate_a10_pit = {
> > .name = "a10.pit",
> > .version_id = 1,
> > @@ -196,6 +236,7 @@ static void a10_pit_reset(DeviceState *dev)
> > s->interval[i] = 0;
> > s->count[i] = 0;
> > ptimer_stop(s->timer[i]);
> > + a10_pit_set_freq(s, i);
> > }
> > s->watch_dog_mode = 0;
> > s->watch_dog_control = 0;
> > @@ -241,7 +282,6 @@ static void a10_pit_init(Object *obj)
> > tc->index = i;
> > bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
> > s->timer[i] = ptimer_init(bh[i]);
> > - ptimer_set_freq(s->timer[i], 240000);
> > }
> > }
> >
> > @@ -250,6 +290,7 @@ static void a10_pit_class_init(ObjectClass *klass, void
> > *data)
> > DeviceClass *dc = DEVICE_CLASS(klass);
> >
> > dc->reset = a10_pit_reset;
> > + dc->props = a10_pit_properties;
> > dc->desc = "allwinner a10 timer";
> > dc->vmsd = &vmstate_a10_pit;
> > }
> > diff --git a/include/hw/timer/allwinner-a10-pit.h
> > b/include/hw/timer/allwinner-a10-pit.h
> > index a48d3c7..5388b2b 100644
> > --- a/include/hw/timer/allwinner-a10-pit.h
> > +++ b/include/hw/timer/allwinner-a10-pit.h
> > @@ -50,6 +50,10 @@ typedef struct AwA10PITState {
> > ptimer_state * timer[AW_A10_PIT_TIMER_NR];
> > AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
> > MemoryRegion iomem;
> > + uint32_t clk0_freq;
> > + uint32_t clk1_freq;
> > + uint32_t clk2_freq;
> > + uint32_t clk3_freq;
> >
>
> And this becomes an array.
I will change this in the next version, thank you for the review.
Beniamino
