diff mbox

[v2,04/11] clocksource/moxart: Generalise timer for use on other socs

Message ID 571A5FBA.9010204@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano April 22, 2016, 5:30 p.m. UTC 
On 04/21/2016 10:04 AM, Joel Stanley wrote:
> The moxart timer IP is shared with another soc made by Aspeed.
> Generalise the registers that differ so the same driver can be used for
> both.
>
> As we now depend on CLKSRC_MMIO, create a Kconfig symbol for the driver
> so we can express this dependency.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---

In the future, please Cc the maintainers.

>   .../bindings/timer/moxa,moxart-timer.txt           |  4 +-
>   drivers/clocksource/Kconfig                        |  6 ++
>   drivers/clocksource/Makefile                       |  2 +-
>   drivers/clocksource/moxart_timer.c                 | 90 +++++++++++++++++-----
>   4 files changed, 79 insertions(+), 23 deletions(-)

[ ... ]

> +config MOXART_TIMER
> +	def_bool ARCH_MOXART || ARCH_ASPEED
> +	depends on ARM && OF
> +	select CLKSRC_OF
> +	select CLKSRC_MMIO

Refer to the other drivers to see how they are selected (eg. digicolor 
or mtk).

[ ... ]

> -#define TIMEREG_CR_1_ENABLE	BIT(0)
> -#define TIMEREG_CR_1_CLOCK	BIT(1)
> -#define TIMEREG_CR_1_INT	BIT(2)
> -#define TIMEREG_CR_2_ENABLE	BIT(3)
> -#define TIMEREG_CR_2_CLOCK	BIT(4)
> -#define TIMEREG_CR_2_INT	BIT(5)
> -#define TIMEREG_CR_3_ENABLE	BIT(6)
> -#define TIMEREG_CR_3_CLOCK	BIT(7)
> -#define TIMEREG_CR_3_INT	BIT(8)
> -#define TIMEREG_CR_COUNT_UP	BIT(9)
> -
> -#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
> -#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
> +#define MOXART_CR_1_ENABLE	BIT(0)
> +#define MOXART_CR_1_CLOCK	BIT(1)
> +#define MOXART_CR_1_INT		BIT(2)
> +#define MOXART_CR_2_ENABLE	BIT(3)
> +#define MOXART_CR_2_CLOCK	BIT(4)
> +#define MOXART_CR_2_INT		BIT(5)
> +#define MOXART_CR_3_ENABLE	BIT(6)
> +#define MOXART_CR_3_CLOCK	BIT(7)
> +#define MOXART_CR_3_INT		BIT(8)
> +#define MOXART_CR_COUNT_UP	BIT(9)
> +
> +#define MOXART_TIMER1_ENABLE	(MOXART_CR_2_ENABLE | MOXART_CR_1_ENABLE)
> +#define MOXART_TIMER1_DISABLE	(MOXART_CR_2_ENABLE)
> +
> +/*
> + * The ASpeed variant of the IP block has a different layout
> + * for the control register
> + */
> +#define ASPEED_CR_1_ENABLE	BIT(0)
> +#define ASPEED_CR_1_CLOCK	BIT(1)
> +#define ASPEED_CR_1_INT		BIT(2)
> +#define ASPEED_CR_2_ENABLE	BIT(4)
> +#define ASPEED_CR_2_CLOCK	BIT(5)
> +#define ASPEED_CR_2_INT		BIT(6)
> +#define ASPEED_CR_3_ENABLE	BIT(8)
> +#define ASPEED_CR_3_CLOCK	BIT(9)
> +#define ASPEED_CR_3_INT		BIT(10)
> +
> +#define ASPEED_TIMER1_ENABLE	(ASPEED_CR_2_ENABLE | ASPEED_CR_1_ENABLE)
> +#define ASPEED_TIMER1_DISABLE	(ASPEED_CR_2_ENABLE)

You probably can remove all the unused macro definition here for both 
MOXART and ASPEED to have something just a couple of definition.

>   static void __iomem *base;
>   static unsigned int clock_count_per_tick;
> +static unsigned int t1_disable_val, t1_enable_val;

It will be cleaner to:

1. Factor out:
	writel(TIMER1_DISABLE, base + TIMER_CR);
	writel(TIMER1_ENABLE, base + TIMER_CR);


@@ -83,12 +93,12 @@ static int moxart_clkevt_next_event(unsigned long 
cycles,
  {
         u32 u;

-       writel(TIMER1_DISABLE, base + TIMER_CR);
+       moxart_disable(evt);

         u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
         writel(u, base + TIMER1_BASE + REG_MATCH1);

-       writel(TIMER1_ENABLE, base + TIMER_CR);
+       moxart_enable(evt);

         return 0;
  }

2. Encapsulate clkevt and use container_of

-static void __iomem *base;
-static unsigned int clock_count_per_tick;
+struct moxart_timer {
+       void __iomem *base;
+       unsigned int clock_count_per_tick;
+       struct clock_event_device clkevt;
+};
+
+static struct moxart_timer moxart_timer = {
+       .clkevt = {
+               .name                   = "moxart_timer",
+               .rating                 = 200,
+               .features               = CLOCK_EVT_FEAT_PERIODIC |
+                                               CLOCK_EVT_FEAT_ONESHOT,
+               .set_state_shutdown     = moxart_shutdown,
+               .set_state_periodic     = moxart_set_periodic,
+               .set_state_oneshot      = moxart_set_oneshot,
+               .tick_resume            = moxart_set_oneshot,
+               .set_next_event         = moxart_clkevt_next_event,
+       },
+};
+
+static inline struct moxart_timer *clkevt_to_moxart(struct 
clock_event_device *evt)
+{
+       return container_of(evt, struct moxart_timer, clkevt);
+}

  static inline void moxart_disable(struct clock_event_device *evt)
  {
-       writel(TIMER1_DISABLE, base + TIMER_CR);
+       writel(TIMER1_DISABLE, clkevt_to_moxart(evt)->base + TIMER_CR);
  }

3. And finally, add 't1_disable' / 't2_disable' to the structure.


   -- Daniel

Comments

Benjamin Herrenschmidt April 22, 2016, 11:55 p.m. UTC | #1 
On Fri, 2016-04-22 at 19:30 +0200, Daniel Lezcano wrote:
> 
> You probably can remove all the unused macro definition here for
> both 
> MOXART and ASPEED to have something just a couple of definition.

I disagree. Quite strongly in fact. These provide documentation (which
isn't otherwise publicly available) and this will be handy if somebody
wants to do something like add another timer etc.. in the future.

I much prefer when register definitions rather more exhaustive than
trimmed to be only what's used by the driver.

Cheers,
Ben
Joel Stanley May 3, 2016, 5:56 a.m. UTC | #2 
Hey Daniel,

Thanks for the review.

On Sat, Apr 23, 2016 at 3:00 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 04/21/2016 10:04 AM, Joel Stanley wrote:
>>
>> The moxart timer IP is shared with another soc made by Aspeed.
>> Generalise the registers that differ so the same driver can be used for
>> both.
>>
>> As we now depend on CLKSRC_MMIO, create a Kconfig symbol for the driver
>> so we can express this dependency.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>
>
> In the future, please Cc the maintainers.

Sure.

>
> You probably can remove all the unused macro definition here for both MOXART
> and ASPEED to have something just a couple of definition.

I agree with Ben; we're helping out by documenting the hardware in
lieu of a public datasheet. I'd prefer to keep this here.

>>   static void __iomem *base;
>>   static unsigned int clock_count_per_tick;
>> +static unsigned int t1_disable_val, t1_enable_val;
>
>
> It will be cleaner to:
>
> 1. Factor out:
>         writel(TIMER1_DISABLE, base + TIMER_CR);
>         writel(TIMER1_ENABLE, base + TIMER_CR);

I considered this myself but went with the minimal change. I'm not
fussed, so I will rework it as you suggest.

>From the register layout I suspect this IP block is a Faraday Tech
FTTMR010[1], but I don't have any other evidence. Would you take a
patch to change the name or would you prefer leaving it as moxart?

Cheers,

Joel

[1] https://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg04333.html
Daniel Lezcano May 3, 2016, 1:36 p.m. UTC | #3 
On Tue, May 03, 2016 at 03:26:33PM +0930, Joel Stanley wrote:
> Hey Daniel,
> 
> Thanks for the review.
> 
> On Sat, Apr 23, 2016 at 3:00 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On 04/21/2016 10:04 AM, Joel Stanley wrote:
> >>
> >> The moxart timer IP is shared with another soc made by Aspeed.
> >> Generalise the registers that differ so the same driver can be used for
> >> both.
> >>
> >> As we now depend on CLKSRC_MMIO, create a Kconfig symbol for the driver
> >> so we can express this dependency.
> >>
> >> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >> ---
> >
> >
> > In the future, please Cc the maintainers.
> 
> Sure.
> 
> >
> > You probably can remove all the unused macro definition here for both MOXART
> > and ASPEED to have something just a couple of definition.
> 
> I agree with Ben; we're helping out by documenting the hardware in
> lieu of a public datasheet. I'd prefer to keep this here.

Ok, let's keep it.
 
> >>   static void __iomem *base;
> >>   static unsigned int clock_count_per_tick;
> >> +static unsigned int t1_disable_val, t1_enable_val;
> >
> >
> > It will be cleaner to:
> >
> > 1. Factor out:
> >         writel(TIMER1_DISABLE, base + TIMER_CR);
> >         writel(TIMER1_ENABLE, base + TIMER_CR);
> 
> I considered this myself but went with the minimal change. I'm not
> fussed, so I will rework it as you suggest.
> 
> From the register layout I suspect this IP block is a Faraday Tech
> FTTMR010[1], but I don't have any other evidence.

Apparently, it could be the fttmr010 [2].

May be Jonas Jensen can confirm that.

> Would you take a
> patch to change the name or would you prefer leaving it as moxart?

If Jonas can confirm the moxart SoC is using the faraday timer, then it 
would make much more sense to rename it to timer-fttmr010.c and have the 
different instance of this timer to set it up with the platform specific 
bits.

> [1] https://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg04333.html

[2] 
http://git.denx.de/?p=u-boot.git;a=blob;f=include/faraday/fttmr010.h;h=2ab68d10218ed8241e5d2c916437c5918c17173d;hb=HEAD
Jonas Jensen May 6, 2016, 2:50 p.m. UTC | #4 
On 3 May 2016 at 15:36, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Apparently, it could be the fttmr010 [2].
>
> May be Jonas Jensen can confirm that.

The best I can do is infer (if that helps).

Also I can test on UC-7112-LX Plus hardware (the patch I got from Ben
back in May 2015 was verified OK).


   Jonas
diff mbox

Patch

diff --git a/drivers/clocksource/moxart_timer.c 
b/drivers/clocksource/moxart_timer.c
index 19857af..aede6f1 100644
--- a/drivers/clocksource/moxart_timer.c
+++ b/drivers/clocksource/moxart_timer.c
@@ -58,15 +58,25 @@ 
  static void __iomem *base;
  static unsigned int clock_count_per_tick;

-static int moxart_shutdown(struct clock_event_device *evt)
+static inline void moxart_disable(struct clock_event_device *evt)
  {
         writel(TIMER1_DISABLE, base + TIMER_CR);
+}
+
+static inline void moxart_enable(struct clock_event_device *evt)
+{
+       writel(TIMER1_ENABLE, base + TIMER_CR);
+}
+
+static int moxart_shutdown(struct clock_event_device *evt)
+{
+       moxart_disable(evt);
         return 0;
  }

  static int moxart_set_oneshot(struct clock_event_device *evt)
  {
-       writel(TIMER1_DISABLE, base + TIMER_CR);
+       moxart_disable(evt);
         writel(~0, base + TIMER1_BASE + REG_LOAD);
         return 0;
  }
@@ -74,7 +84,7 @@  static int moxart_set_oneshot(struct 
clock_event_device *evt)
  static int moxart_set_periodic(struct clock_event_device *evt)
  {
         writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
-       writel(TIMER1_ENABLE, base + TIMER_CR);
+       moxart_enable(evt);
         return 0;
  }