From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPeQ5-0006Zz-St for qemu-devel@nongnu.org; Mon, 17 Mar 2014 16:46:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPePz-0006pk-NC for qemu-devel@nongnu.org; Mon, 17 Mar 2014 16:46:01 -0400 Received: from mail-ee0-x234.google.com ([2a00:1450:4013:c00::234]:52610) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPePz-0006pd-Bw for qemu-devel@nongnu.org; Mon, 17 Mar 2014 16:45:55 -0400 Received: by mail-ee0-f52.google.com with SMTP id e49so4560727eek.39 for ; Mon, 17 Mar 2014 13:45:54 -0700 (PDT) Date: Mon, 17 Mar 2014 21:45:20 +0100 From: Beniamino Galvani Message-ID: <20140317204518.GA5159@gmail.com> References: <1394888493-20487-1-git-send-email-b.galvani@gmail.com> <1394888493-20487-6-git-send-email-b.galvani@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 5/7] allwinner-a10-pit: implement prescaler and source selection List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" , Li Guang On Mon, Mar 17, 2014 at 11:27:01AM +1000, Peter Crosthwaite wrote: > On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani 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 > > --- > > 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