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

[Peter Crosthwaite]

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.

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

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

Regards,
Peter

>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>
>  typedef struct AwA10PITState AwA10PITState;
> --
> 1.7.10.4
>
>