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

[Beniamino Galvani]

On Mon, Mar 03, 2014 at 08:57:33PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, 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 |   19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index 4723b25..f2f2567 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -79,6 +79,23 @@ 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_freqs[] = {32768, 24000000};
> > +
> 
> These smell like external oscillator frequencies which is board level
> information. Ideally, you should propertyify the input frequencies and
> pass them in on the board, so others can make different boards with
> with different crystals.

According to the A10 datasheet, the possible values for the field are:

  00: Low speed OSC
  01: OSC24M
  10: PLL6/6
  11: /

OSC24M is a fixed 24MHz oscillator; I suppose that "Low speed OSC" is
the 32768Hz oscillator described in section 6. So both frequencies
seem to be fixed in this case.
 
> > +    prescaler = 1 << extract32(s->control[index], 4, 3);
> > +    source = extract32(s->control[index], 2, 2);
> > +
> > +    if (source > 1) {
> > +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %d",
> 
> LOG_UNIMP is usually used for something that does exist but QEMU
> chooses knowingly not to implement. Are there extra clock wires from
> other sources not handled or are encodings 1 & 2 just unpopulated on
> this board? (In which case it should be a GUEST_ERROR).

Encoding 2 is associated to PLL6/6 and should give a UNIMPL; encoding
3 is not defined and should give a GUEST_ERROR, right?

Thanks for the reviews,
Beniamino

> 
> Regards,
> Peter
> 
> > +                      __func__, source);
> > +        source = 0;
> > +    }
> > +
> > +    ptimer_set_freq(s->timer[index], source_freqs[source] / prescaler);
> > +}
> > +
> >  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> >                              unsigned size)
> >  {
> > @@ -101,6 +118,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]);
> >              }
> > @@ -244,7 +262,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);
> >      }
> >  }
> >
> > --
> > 1.7.10.4
> >
> >