Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
On Thu, Jun 27, 2013 at 08:36:43PM +0300, Baruch Siach wrote: > Hi Maxime, > > On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote: > > On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote: > > > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote: > > > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote: > > > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote: > > > > > > +static u32 sun4i_timer_sched_read(void) > > > > > > > > > > You commit message mentions "64 bits free running counter", but this > > > > > one only > > > > > returns 32 bit. > > > > > > > > Yeah, the callback setup by setup_sched_clock is supposed to be > > > > returning a u32, and clocksource_mmio_init only accepts up to 32 bits as > > > > well, so I'm only using the lower 32bits of this 64 bits counter. > > > > > > > > I'll amend the commit log to state this. > > > > > > But using 64 bit counter for sched_clock is much easier that using 32 bit > > > one. > > > > Easier in what aspect? Both API looks similar. > > You can just implement your own simple sched_clock() that just returns the > current value of this 64 bit counter, and do away with all the tricky code in > kernel/time/sched_clock.c (in tip.git) that is needed to make the 32 -> 64 > extension safe. This is not compatible with multi-platform kernel, though. Which is a deal-breaker for us. I'll use the setup_sched_clock_64 introduced by Stephen then :) Thanks for the time you took to review these patches! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Maxime,
On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > > +static u32 sun4i_timer_sched_read(void)
> > > >
> > > > You commit message mentions "64 bits free running counter", but this
> > > > one only
> > > > returns 32 bit.
> > >
> > > Yeah, the callback setup by setup_sched_clock is supposed to be
> > > returning a u32, and clocksource_mmio_init only accepts up to 32 bits as
> > > well, so I'm only using the lower 32bits of this 64 bits counter.
> > >
> > > I'll amend the commit log to state this.
> >
> > But using 64 bit counter for sched_clock is much easier that using 32 bit
> > one.
>
> Easier in what aspect? Both API looks similar.
You can just implement your own simple sched_clock() that just returns the
current value of this 64 bit counter, and do away with all the tricky code in
kernel/time/sched_clock.c (in tip.git) that is needed to make the 32 -> 64
extension safe. This is not compatible with multi-platform kernel, though.
> > You can either wait for the rest of Stephen's patch set
> > (http://thread.gmane.org/gmane.linux.ports.arm.msm/4092) before adding 64
> > bit, or you can just make the trivial change to the now generic
> > sched_clock code.
>
> I'll wait for the Stephen's patches to be merged then, and add support
> for 64bits counters to clocksource_mmio_init.
Sound reasonable.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}ooO--U--Ooo{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Baruch, On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote: > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote: > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote: > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote: > > > > +static u32 sun4i_timer_sched_read(void) > > > > > > You commit message mentions "64 bits free running counter", but this one > > > only > > > returns 32 bit. > > > > Yeah, the callback setup by setup_sched_clock is supposed to be > > returning a u32, and clocksource_mmio_init only accepts up to 32 bits as > > well, so I'm only using the lower 32bits of this 64 bits counter. > > > > I'll amend the commit log to state this. > > But using 64 bit counter for sched_clock is much easier that using 32 bit > one. Easier in what aspect? Both API looks similar. > You can either wait for the rest of Stephen's patch set > (http://thread.gmane.org/gmane.linux.ports.arm.msm/4092) before adding 64 > bit, > or you can just make the trivial change to the now generic sched_clock code. I'll wait for the Stephen's patches to be merged then, and add support for 64bits counters to clocksource_mmio_init. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Maxime,
On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > +static u32 sun4i_timer_sched_read(void)
> >
> > You commit message mentions "64 bits free running counter", but this one
> > only
> > returns 32 bit.
>
> Yeah, the callback setup by setup_sched_clock is supposed to be
> returning a u32, and clocksource_mmio_init only accepts up to 32 bits as
> well, so I'm only using the lower 32bits of this 64 bits counter.
>
> I'll amend the commit log to state this.
But using 64 bit counter for sched_clock is much easier that using 32 bit one.
You can either wait for the rest of Stephen's patch set
(http://thread.gmane.org/gmane.linux.ports.arm.msm/4092) before adding 64 bit,
or you can just make the trivial change to the now generic sched_clock code.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}ooO--U--Ooo{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Baruch,
On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> Hi Maxime,
>
> On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > The A10 and the A13 has a 64 bits free running counter that we can use
> > as a clocksource and a sched clock, that were both not used yet on these
> > platforms.
> >
> > Signed-off-by: Maxime Ripard
> > ---
> > drivers/clocksource/sun4i_timer.c | 27 +++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/clocksource/sun4i_timer.c
> > b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -23,6 +23,8 @@
> > #include
> > #include
> >
> > +#include
>
> In the tip.git tree (and -next) this header is moved to
> in 38ff87f77a (sched_clock: Make ARM's sched_clock generic for all
> architectures).
Ah, good to know. Thanks!
> > +
> > #define TIMER_IRQ_EN_REG 0x00
> > #define TIMER_IRQ_EN(val) BIT(val)
> > #define TIMER_IRQ_ST_REG 0x04
> > @@ -34,6 +36,11 @@
> > #define TIMER_CNTVAL_REG(val) (0x10 * val + 0x18)
> >
> > #define TIMER_SCAL 16
> > +#define TIMER_CNT64_CTL_REG0xa0
> > +#define TIMER_CNT64_CTL_CLRBIT(0)
> > +#define TIMER_CNT64_CTL_RL BIT(1)
> > +#define TIMER_CNT64_LOW_REG0xa4
> > +#define TIMER_CNT64_HIGH_REG 0xa8
> >
> > static void __iomem *timer_base;
> >
> > @@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
> > .dev_id = &sun4i_clockevent,
> > };
> >
> > +static u32 sun4i_timer_sched_read(void)
>
> You commit message mentions "64 bits free running counter", but this one only
> returns 32 bit.
Yeah, the callback setup by setup_sched_clock is supposed to be
returning a u32, and clocksource_mmio_init only accepts up to 32 bits as
well, so I'm only using the lower 32bits of this 64 bits counter.
I'll amend the commit log to state this.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Daniel,
On Wed, Jun 26, 2013 at 11:27:35PM +0200, Daniel Lezcano wrote:
> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> > The A10 and the A13 has a 64 bits free running counter that we can use
> > as a clocksource and a sched clock, that were both not used yet on these
> > platforms.
> >
> > Signed-off-by: Maxime Ripard
> > ---
> > drivers/clocksource/sun4i_timer.c | 27 +++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/clocksource/sun4i_timer.c
> > b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -23,6 +23,8 @@
> > #include
> > #include
> >
> > +#include
> > +
> > #define TIMER_IRQ_EN_REG 0x00
> > #define TIMER_IRQ_EN(val) BIT(val)
> > #define TIMER_IRQ_ST_REG 0x04
> > @@ -34,6 +36,11 @@
> > #define TIMER_CNTVAL_REG(val) (0x10 * val + 0x18)
> >
> > #define TIMER_SCAL 16
> > +#define TIMER_CNT64_CTL_REG0xa0
> > +#define TIMER_CNT64_CTL_CLRBIT(0)
> > +#define TIMER_CNT64_CTL_RL BIT(1)
> > +#define TIMER_CNT64_LOW_REG0xa4
> > +#define TIMER_CNT64_HIGH_REG 0xa8
> >
> > static void __iomem *timer_base;
> >
> > @@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
> > .dev_id = &sun4i_clockevent,
> > };
> >
> > +static u32 sun4i_timer_sched_read(void)
> > +{
> > + u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);
> > + writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
> > + while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);
>
> Isn't a cpu_relax missing here ?
Right.
The AND is wrong as well, it should be TIMER_CNT64_CTL_RL, not the reg
offset.
> >
> > + writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
> > + setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
> > + clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
> > + clk_get_rate(clk), 300, 32,
> > + sun4i_timer_clksrc_read);
> > +
> > writel(rate / (TIMER_SCAL * HZ),
>
> DIV_ROUND_CLOSEST(rate, (TIMER_SCAL * HZ)), ?
It's actually fixed in a later patch, but yes.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
Hi Maxime,
On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> The A10 and the A13 has a 64 bits free running counter that we can use
> as a clocksource and a sched clock, that were both not used yet on these
> platforms.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/clocksource/sun4i_timer.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/clocksource/sun4i_timer.c
> b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -23,6 +23,8 @@
> #include
> #include
>
> +#include
In the tip.git tree (and -next) this header is moved to
in 38ff87f77a (sched_clock: Make ARM's sched_clock generic for all
architectures).
> +
> #define TIMER_IRQ_EN_REG 0x00
> #define TIMER_IRQ_EN(val)BIT(val)
> #define TIMER_IRQ_ST_REG 0x04
> @@ -34,6 +36,11 @@
> #define TIMER_CNTVAL_REG(val)(0x10 * val + 0x18)
>
> #define TIMER_SCAL 16
> +#define TIMER_CNT64_CTL_REG 0xa0
> +#define TIMER_CNT64_CTL_CLR BIT(0)
> +#define TIMER_CNT64_CTL_RL BIT(1)
> +#define TIMER_CNT64_LOW_REG 0xa4
> +#define TIMER_CNT64_HIGH_REG 0xa8
>
> static void __iomem *timer_base;
>
> @@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
> .dev_id = &sun4i_clockevent,
> };
>
> +static u32 sun4i_timer_sched_read(void)
You commit message mentions "64 bits free running counter", but this one only
returns 32 bit.
baruch
> +{
> + u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);
> + writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
> + while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);
> +
> + return readl(timer_base + TIMER_CNT64_LOW_REG);
> +}
> +
> +static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
> +{
> + return sun4i_timer_sched_read();
> +}
> +
> static void __init sun4i_timer_init(struct device_node *node)
> {
> unsigned long rate = 0;
> @@ -117,6 +138,12 @@ static void __init sun4i_timer_init(struct device_node
> *node)
>
> rate = clk_get_rate(clk);
>
> + writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
> + setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
> + clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
> + clk_get_rate(clk), 300, 32,
> + sun4i_timer_clksrc_read);
> +
> writel(rate / (TIMER_SCAL * HZ),
> timer_base + TIMER_INTVAL_REG(0));
>
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}ooO--U--Ooo{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers
On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> The A10 and the A13 has a 64 bits free running counter that we can use
> as a clocksource and a sched clock, that were both not used yet on these
> platforms.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/clocksource/sun4i_timer.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/clocksource/sun4i_timer.c
> b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -23,6 +23,8 @@
> #include
> #include
>
> +#include
> +
> #define TIMER_IRQ_EN_REG 0x00
> #define TIMER_IRQ_EN(val)BIT(val)
> #define TIMER_IRQ_ST_REG 0x04
> @@ -34,6 +36,11 @@
> #define TIMER_CNTVAL_REG(val)(0x10 * val + 0x18)
>
> #define TIMER_SCAL 16
> +#define TIMER_CNT64_CTL_REG 0xa0
> +#define TIMER_CNT64_CTL_CLR BIT(0)
> +#define TIMER_CNT64_CTL_RL BIT(1)
> +#define TIMER_CNT64_LOW_REG 0xa4
> +#define TIMER_CNT64_HIGH_REG 0xa8
>
> static void __iomem *timer_base;
>
> @@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
> .dev_id = &sun4i_clockevent,
> };
>
> +static u32 sun4i_timer_sched_read(void)
> +{
> + u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);
> + writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
> + while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);
Isn't a cpu_relax missing here ?
> +
> + return readl(timer_base + TIMER_CNT64_LOW_REG);
> +}
> +
> +static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
> +{
> + return sun4i_timer_sched_read();
> +}
> +
> static void __init sun4i_timer_init(struct device_node *node)
> {
> unsigned long rate = 0;
> @@ -117,6 +138,12 @@ static void __init sun4i_timer_init(struct device_node
> *node)
>
> rate = clk_get_rate(clk);
>
> + writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
> + setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
> + clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
> + clk_get_rate(clk), 300, 32,
> + sun4i_timer_clksrc_read);
> +
> writel(rate / (TIMER_SCAL * HZ),
DIV_ROUND_CLOSEST(rate, (TIMER_SCAL * HZ)), ?
> timer_base + TIMER_INTVAL_REG(0));
--
Facebook |
Twitter |
