[v2,5/7] allwinner-a10-pit: implement prescaler and source selection

This implements the prescaler and source fields of the timer control
register as described in the A10 user manual.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <b.galvani@gmail.com>
> ---
>  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.

> +    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).

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

On Mon, Mar 03, 2014 at 08:57:33PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <b.galvani@gmail.com>
> > ---
> >  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
> >
> >