Re: [Qemu-devel] [PATCH v3 5/7] allwinner-a10-pit: implement prescaler and source selection

[Beniamino Galvani]

On Mon, Mar 17, 2014 at 11:27:01AM +1000, Peter Crosthwaite wrote:
> On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani <address@hidden> wrote:
> > This implements the prescaler and source fields of the timer control
> > register as described in the A10 user manual.
> >
> > Signed-off-by: Beniamino Galvani <address@hidden>
> > ---
> >  hw/timer/allwinner-a10-pit.c         |   30 +++++++++++++++++++++++++++++-
> >  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
> >  2 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index f8c9236..a448689 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -74,6 +74,34 @@ 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 = AW_A10_PIT_OSC24M_FREQ;
> > +
> > +    prescaler = 1 << extract32(s->control[index], 4, 3);
> > +    source = extract32(s->control[index], 2, 2);
> > +
> > +    switch (source) {
> > +    case AW_A10_PIT_SOURCE_LS_OSC:
> > +        source_freq = AW_A10_PIT_LS_OSC_FREQ;
> > +        break;
> > +    case AW_A10_PIT_SOURCE_OSC24M:
> > +        source_freq = AW_A10_PIT_OSC24M_FREQ;
> > +        break;
> > +    case AW_A10_PIT_SOURCE_PLL6:
> > +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %u", 
> > __func__,
> > +                      source);
> > +        break;
> > +    case AW_A10_PIT_SOURCE_UNDEF:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid clock source %u", 
> > __func__,
> > +                      source);
> > +        break;
> > +    }
> > +
> > +    ptimer_set_freq(s->timer[index], source_freq / prescaler);
> > +}
> > +
> >  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> >                              unsigned size)
> >  {
> > @@ -96,6 +124,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);
> 
> Similar to comment in previous patch, I think you need to call this
> from the reset handler. Otherwise you are reliant on a control write
> to set the timer frequency in the first place. Looking at reset
> handler, control is reset to a default value, so reset may cause the
> side effect of a timer frequency change as well.

Hi,
the code calls set_frequency() only when the control register gets
written and the motivation behind this is that the frequency of the
ptimer doesn't matter while the timer is stopped.

After a reset all timers are stopped and to enable them you have to
write to the control register, thus setting the frequency to the right
value.

> 
> >              if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
> >                  ptimer_set_count(s->timer[index], s->interval[index]);
> >              }
> > @@ -239,7 +268,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);
> >      }
> >  }
> >
> > diff --git a/include/hw/timer/allwinner-a10-pit.h 
> > b/include/hw/timer/allwinner-a10-pit.h
> > index a48d3c7..37a2662 100644
> > --- a/include/hw/timer/allwinner-a10-pit.h
> > +++ b/include/hw/timer/allwinner-a10-pit.h
> > @@ -33,6 +33,14 @@
> >  #define AW_A10_PIT_TIMER_BASE_END  \
> >      (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
> >
> > +#define AW_A10_PIT_SOURCE_LS_OSC   0
> > +#define AW_A10_PIT_SOURCE_OSC24M   1
> > +#define AW_A10_PIT_SOURCE_PLL6     2
> > +#define AW_A10_PIT_SOURCE_UNDEF    3
> > +
> > +#define AW_A10_PIT_LS_OSC_FREQ     32768
> > +#define AW_A10_PIT_OSC24M_FREQ     24000000
> > +
> 
> So a quick look at a cubieboard schematic:
> 
> http://dl.cubieboard.org/hardware/cubieboard_schematic_2012-08-08.pdf
> 
> Both of these clocks are pinned out on the A10 SoC top level. (sheet 2
> pins CLK32K_IN / OUT etc). Seems for cubieboard in particular, the 32K
> clock is absent. The pins names are somewhat ugly, one would assume
> you could have a range of crystal frequencies when populating a
> crystal on a board, but naming your pin "CLK24M" suggests otherwise. I
> do think the best org for this is to just define 4 arbitrary
> prameterisable clock inputs and push the policy of whats what up to
> the board level. Its highly likely that that is the design intent of
> the Timer IP.

Ok, seems a good idea.
Aren't 2 properties (one for clk32k and one clk24m) enough?

> 
> Are you using the clk 32k in your test case at all?

No, Linux uses clk24m as source for timers.

Beniamino