Re: [PATCH v2 4/9] clocksource: mediatek: Use GPT as sched clock source

[Matthias Brugger]

2015-05-20 16:05 GMT+02:00 Yingjoe Chen <yingjoe.chen@xxxxxxxxxxxx>:
> On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote:
>> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@xxxxxxxxxxxx>:
>> > When cpu is in deep idle, arch timer will stop counting. Setup GPT as
>> > sched clock source so it can keep counting in idle.
>> >
>> > Signed-off-by: Yingjoe Chen <yingjoe.chen@xxxxxxxxxxxx>
>> > ---
>> >  drivers/clocksource/mtk_timer.c | 10 ++++++++++
>> >  1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
>> > index 91206f9..fe7cf72 100644
>> > --- a/drivers/clocksource/mtk_timer.c
>> > +++ b/drivers/clocksource/mtk_timer.c
>> > @@ -24,6 +24,7 @@
>> >  #include <linux/of.h>
>> >  #include <linux/of_address.h>
>> >  #include <linux/of_irq.h>
>> > +#include <linux/sched_clock.h>
>> >  #include <linux/slab.h>
>> >
>> >  #define GPT_IRQ_EN_REG         0x00
>> > @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
>> >         struct clock_event_device dev;
>> >  };
>> >
>> > +static void __iomem *gpt_base __read_mostly;
>> > +
>> > +static u64 notrace mtk_read_sched_clock(void)
>> > +{
>> > +       return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
>> > +}
>> > +
>> >  static inline struct mtk_clock_event_device *to_mtk_clk(
>> >                                 struct clock_event_device *c)
>> >  {
>> > @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
>> >         mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
>> >         clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
>> >                         node->name, rate, 300, 32, clocksource_mmio_readl_up);
>> > +       gpt_base = evt->gpt_base;
>>
>> This is really hacky. We should clean up the code and provide
>> mtk_clock_event_device globally.
>> Please add the patch below, which does exactly this.
>> ---- 8< ---------------- >8 ------
>> From 631e7bf4e5d9456d0bb4a29b2dee4b84e8c052bd Mon Sep 17 00:00:00 2001
>> From: Matthias Brugger <matthias.bgg@xxxxxxxxx>
>> Date: Wed, 20 May 2015 12:43:16 +0200
>> Subject: [PATCH] clocksource: mediatek: Define mtk_clock_event_device globally
>>
>> Sched clock code, especially sched_clock_register does not allow to pass a
>> pointer to actual_read_sched_clock. So if in the driver the register base
>> address is not globally defined, we are not able to read the scheduler
>> clock register. This patch sets the mtk_clock_event_device struct globally
>> for the driver, to be able to read the register.
>
> Hi,
>
> I'm not sure using a global device pointer is any better.

Why not?

>
> Actually, almost every user of sched_clock_register need to keep a
> global register base address. Does it make sense to fix this in
> sched_clock_register?
>

Not sure about that. Normally you have just one timer in your system,
so you don't get any problems to declare driver private structs
globally on a per driver basis.
Looking on other drivers I have seen a bit of everything, no struct at
all, a global pointer to the driver private struct, only iomem
pointers global and the rest of the driver private struct as is.
It seems that there is no uniform way so up to you. Just please don't
use two times the exactly same iomem pointer, one in the driver
private struct and one for the scheduler clock. :)

Thanks,
Matthias
-- 
motzblog.wordpress.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/