Re: [PATCH 2/5] clocksource: Add Allwinner SoCs HS timers driver

[Maxime Ripard]

Hi Stephen,

On Wed, Sep 25, 2013 at 04:16:14PM -0700, Stephen Boyd wrote:
> On 09/25/13 07:03, Maxime Ripard wrote:
> > diff --git a/Documentation/devicetree/bindings/timer/allwinner,sun5i-a13-hstimer.txt b/Documentation/devicetree/bindings/timer/allwinner,sun5i-a13-hstimer.txt
> > new file mode 100644
> > index 0000000..b1f81e9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/allwinner,sun5i-a13-hstimer.txt
> > @@ -0,0 +1,22 @@
> > +Allwinner SoCs High Speed Timer Controller
> > +
> > +Required properties:
> > +
> > +- compatible :	should be "allwinner,sun5i-a13-hstimer" or
> > +		"allwinner,sun7i-a20-hstimer"
> > +- reg : Specifies base physical address and size of the registers.
> > +- interrupts :	The interrupts of these timers (2 for the sun5i IP, 4 for the sun7i
> > +		one)
> > +- clocks: phandle to the source clock (usually the AHB clock)
> > +
> > +Example:
> > +
> > +hstimer@01c60000 {
> 
> This should just be 'timer@1c60000'

Ok.

> > +	compatible = "allwinner,sun7i-a20-hstimer";
> > +        reg = <0x01c60000 0x1000>;
> > +        interrupts = <0 51 1>,
> > +                     <0 52 1>,
> > +                     <0 53 1>,
> > +                     <0 54 1>;
> > +        clocks = <&ahb1_gates 19>;
> > +};
> 
> Weird mix of tabs and spaces here.

Right.

> > +
> > +static void __iomem *timer_base;
> > +static u32 ticks_per_jiffy;
> > +
> > +/*
> > + * When we disable a timer, we need to wait at least for 2 cycles of
> > + * the timer source clock. We will use for that the clocksource timer
> > + * that is already setup and runs at the same frequency than the other
> > + * timers, and we never will be disabled.
> > + */
> > +static void sun5i_clkevt_sync(void)
> > +{
> > +	u32 old = readl(timer_base + TIMER_CNTVAL_LO_REG(1));
> > +
> > +	while ((old - readl(timer_base + TIMER_CNTVAL_LO_REG(1))) < 3)
> > +		cpu_relax();
> > +}
> > +
> [...]
> > +
> > +static int sun5i_clkevt_next_event(unsigned long evt,
> > +				   struct clock_event_device *unused)
> > +{
> > +	sun5i_clkevt_time_stop(0);
> > +	sun5i_clkevt_time_setup(0, evt);
> > +	sun5i_clkevt_time_start(0, false);
> 
> I suppose the min delta wants to be 3 instead of 1 because if we program
> an evt one tick in the future first we'll wait for two ticks (or is that
> three?) while we stop the timer and then program the timer to fire one
> tick after that. Perhaps we should subtract two ticks from the evt as
> well when we program it here?

Hmmm, indeed.

> > +
> > +	return 0;
> > +}
> > +
> > +static struct clock_event_device sun5i_clockevent = {
> > +	.name = "sun5i_tick",
> > +	.rating = 350,
> > +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> > +	.set_mode = sun5i_clkevt_mode,
> > +	.set_next_event = sun5i_clkevt_next_event,
> > +};
> > +
> > +
> > +static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
> > +{
> > +	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
> > +
> > +	writel(0x1, timer_base + TIMER_IRQ_ST_REG);
> > +	evt->event_handler(evt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static struct irqaction sun5i_timer_irq = {
> > +	.name = "sun5i_timer0",
> > +	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> 
> IRQF_DISABLED is a no-op and can be dropped.

Right.

> > +
> > +static u32 sun5i_timer_sched_read(void)
> > +{
> > +	return ~readl(timer_base + TIMER_CNTVAL_LO_REG(1));
> > +}
> > +
> > +static void __init sun5i_timer_init(struct device_node *node)
> > +{
> [...]
> > +
> > +	sun5i_clockevent.cpumask = cpumask_of(0);
> 
> Can this timer interrupt any CPU or is it hardwired to CPU0? If the
> interrupt can go to any CPU this should be cpu_possible_mask instead.

This is an interrupt that can interrupt any CPU available on the system,
so yes, cpu_possible_mask would make sense.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachment: signature.asc
Description: Digital signature