| Message ID | CABuKBeJhdRBBw1v1kj8J0YFVWZ27vzx5W=XU8jiwePVgL_7g_g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote: > 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > > 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@mediatek.com> > > --- > > 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@gmail.com> > 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. 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? > Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/clocksource/mtk_timer.c | 50 +++++++++++++++-------------------------- > 1 file changed, 18 insertions(+), 32 deletions(-) > > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c > index 68ab423..c5f5b40 100644 > --- a/drivers/clocksource/mtk_timer.c > +++ b/drivers/clocksource/mtk_timer.c > @@ -59,13 +59,9 @@ struct mtk_clock_event_device { > struct clock_event_device dev; > }; > > -static inline struct mtk_clock_event_device *to_mtk_clk( > - struct clock_event_device *c) > -{ > - return container_of(c, struct mtk_clock_event_device, dev); > -} > +struct mtk_clock_event_device *evt; The name is too short even if we make it static: static struct mtk_clock_event_device *evt __read_mostly; Joe.C
2015-05-20 16:05 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote: >> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: >> > 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@mediatek.com> >> > --- >> > 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@gmail.com> >> 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
On 05/20, Matthias Brugger wrote: > 2015-05-20 16:05 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > > On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote: > >> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > >> > 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@mediatek.com> > >> > --- > >> > 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@gmail.com> > >> 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. :) > Typically the point of doing the container_of design on the clockevent is to keep cache locality of all the data. This way, when the clockevent is programmed it's cheap pointer math on a pointer that's hot in the cache instead of a global pointer dereference and likely cache miss to find the data necessary.
On Wed, May 20, 2015 at 7:02 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote: > 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: >> 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@mediatek.com> >> --- >> 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. I don't think this is so hacky. In light of Stephen's comment about the benefit of using container_of() to extract gpt_base from the passed in struct clock_event_device in the other routines, what is the benefit of making more of mtk_clock_event_device global? I think what Yingjoe has implemented is short and sweet. Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
On Friday, July 03, 2015 09:48:42 PM Daniel Kurtz wrote: > On Wed, May 20, 2015 at 7:02 PM, Matthias Brugger > > <matthias.bgg@gmail.com> wrote: > > 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > >> 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@mediatek.com> > >> --- > >> > >> 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. > > I don't think this is so hacky. > In light of Stephen's comment about the benefit of using > container_of() to extract gpt_base from the passed in struct > clock_event_device in the other routines, what is the benefit of > making more of mtk_clock_event_device global? > I think what Yingjoe has implemented is short and sweet. > Huh, this patch got somehow forgotten. Ok, just one comment. I would prefer to rename the global gpt_base to gpt_sched_base or something similar and set the pointer + offset directly mtk_timer_init. Thanks, Matthias
On Fri, 2015-07-03 at 17:51 +0200, Matthias Brugger wrote: > On Friday, July 03, 2015 09:48:42 PM Daniel Kurtz wrote: <...> > > >> @@ -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. > > > > I don't think this is so hacky. > > In light of Stephen's comment about the benefit of using > > container_of() to extract gpt_base from the passed in struct > > clock_event_device in the other routines, what is the benefit of > > making more of mtk_clock_event_device global? > > I think what Yingjoe has implemented is short and sweet. > > > > Huh, this patch got somehow forgotten. > Ok, just one comment. I would prefer to rename the global gpt_base to > gpt_sched_base or something similar and set the pointer + offset directly > mtk_timer_init. In next version, I will it change to: static void __iomem *gpt_sched_reg __read_mostly; and in mtk_timer_init: + gpt_sched_reg = evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC); Joe.C
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c index 68ab423..c5f5b40 100644 --- a/drivers/clocksource/mtk_timer.c +++ b/drivers/clocksource/mtk_timer.c @@ -59,13 +59,9 @@ struct mtk_clock_event_device { struct clock_event_device dev; }; -static inline struct mtk_clock_event_device *to_mtk_clk( - struct clock_event_device *c) -{ - return container_of(c, struct mtk_clock_event_device, dev); -} +struct mtk_clock_event_device *evt; -static void mtk_clkevt_time_stop(struct mtk_clock_event_device *evt, u8 timer) +static void mtk_clkevt_time_stop(u8 timer) { u32 val; @@ -74,14 +70,12 @@ static void mtk_clkevt_time_stop(struct mtk_clock_event_device *evt, u8 timer) TIMER_CTRL_REG(timer)); } -static void mtk_clkevt_time_setup(struct mtk_clock_event_device *evt, - unsigned long delay, u8 timer) +static void mtk_clkevt_time_setup(unsigned long delay, u8 timer) { writel(delay, evt->gpt_base + TIMER_CMP_REG(timer)); } -static void mtk_clkevt_time_start(struct mtk_clock_event_device *evt, - bool periodic, u8 timer) +static void mtk_clkevt_time_start(bool periodic, u8 timer) { u32 val;