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

[Peter Crosthwaite]

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.

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