[PATCH 0/5] Clocksource: Flextimer: Merged to LS1

[PATCH 0/5] Clocksource: Flextimer: Merged to LS1

[Xiubo Li]

Xiubo Li (5):
  Clocksource: Flextimer: Set cpumask to cpu_possible_mask
  Clocksource: Flextimer: Use internal clocksource read API.
  Clocksource: Flextimer: Remove the useless code.
  Clocksource: Flextimer: Fix counter clock prescaler calculation.
  Clocksource: Flextimer: Use Macro for clock source selection.

 drivers/clocksource/fsl_ftm_timer.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
1.8.5

[PATCH 1/5] Clocksource: Flextimer: Set cpumask to cpu_possible_mask

[Xiubo Li]

The Flextimer is not tied to CPU0, make it usable on any CPU.
For Vybrid there is only one CPU, while for LS1+ there are more
than one.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/clocksource/fsl_ftm_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
index 454227d..13222c1 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -214,7 +214,7 @@ static int __init ftm_clockevent_init(unsigned long freq, int irq)
 		return err;
 	}
 
-	ftm_clockevent.cpumask = cpumask_of(0);
+	ftm_clockevent.cpumask = cpu_all_mask;
 	ftm_clockevent.irq = irq;
 
 	clockevents_config_and_register(&ftm_clockevent,
-- 
1.8.5

[PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Xiubo Li]

Since the Flextimer device will be implemented in BE mode on
LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need
the endianness judgment before doing the mmio.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/clocksource/fsl_ftm_timer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
index 13222c1..c4d23d0f 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -226,6 +226,11 @@ static int __init ftm_clockevent_init(unsigned long freq, int irq)
 	return 0;
 }
 
+static cycle_t ftm_clocksource_read_up(struct clocksource *c)
+{
+	return ftm_readl(clksrc_base + FTM_CNT) & 0xffff;
+}
+
 static int __init ftm_clocksource_init(unsigned long freq)
 {
 	int err;
@@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long freq)
 	sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps));
 	err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm",
 				    freq / (1 << priv->ps), 300, 16,
-				    clocksource_mmio_readl_up);
+				    ftm_clocksource_read_up);
 	if (err) {
 		pr_err("ftm: init clock source mmio failed: %d\n", err);
 		return err;
-- 
1.8.5

Re: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Thomas Gleixner]

On Tue, 26 Aug 2014, Xiubo Li wrote:

> Since the Flextimer device will be implemented in BE mode on
> LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need
> the endianness judgment before doing the mmio.

Brilliant. So for every clocksource read you take a conditional.
 
> @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long freq)
>  	sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps));
>  	err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm",
>  				    freq / (1 << priv->ps), 300, 16,
> -				    clocksource_mmio_readl_up);
> +				    ftm_clocksource_read_up);

What's wrong with having endianess aware clocksource_mmio functions
and make the decision at init time?

Thanks

	tglx

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Li.Xiubo]

> Subject: Re: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read
> API.
> 
> On Tue, 26 Aug 2014, Xiubo Li wrote:
> 
> > Since the Flextimer device will be implemented in BE mode on
> > LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need
> > the endianness judgment before doing the mmio.
> 
> Brilliant. So for every clocksource read you take a conditional.
> 
> > @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long
> freq)
> >  	sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps));
> >  	err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm",
> >  				    freq / (1 << priv->ps), 300, 16,
> > -				    clocksource_mmio_readl_up);
> > +				    ftm_clocksource_read_up);
> 
> What's wrong with having endianess aware clocksource_mmio functions
> and make the decision at init time?
> 

Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
On LS2 platform.

And ftm_clocksource_read_up() will adapt to this different.

Thanks,

BRs
Xiubo

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Thomas Gleixner]

On Thu, 4 Sep 2014, Li.Xiubo@freescale.com wrote:

> > Subject: Re: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read
> > API.
> > 
> > On Tue, 26 Aug 2014, Xiubo Li wrote:
> > 
> > > Since the Flextimer device will be implemented in BE mode on
> > > LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need
> > > the endianness judgment before doing the mmio.
> > 
> > Brilliant. So for every clocksource read you take a conditional.
> > 
> > > @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long
> > freq)
> > >  	sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps));
> > >  	err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm",
> > >  				    freq / (1 << priv->ps), 300, 16,
> > > -				    clocksource_mmio_readl_up);
> > > +				    ftm_clocksource_read_up);
> > 
> > What's wrong with having endianess aware clocksource_mmio functions
> > and make the decision at init time?
> > 
> 
> Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> On LS2 platform.
> 
> And ftm_clocksource_read_up() will adapt to this different.

You are missing the point. Why do you want a conditional in a hot
path? You know at init time whether the thing is BE or LE, so you can
have separate functions for BE/LE or whatever and register that with
clocksource_mmio_init(). i.e.

     if (be)
     	clocksource_mmio_init(....., cs_read_be);
     else if (le)
     	clocksource_mmio_init(....., cs_read_le);
     else if (magic)
     	clocksource_mmio_init(....., cs_read_magic);

Hmm?

	tglx

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Li.Xiubo]

> >
> > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> > On LS2 platform.
> >
> > And ftm_clocksource_read_up() will adapt to this different.
> 
> You are missing the point. Why do you want a conditional in a hot
> path? You know at init time whether the thing is BE or LE, so you can
> have separate functions for BE/LE or whatever and register that with
> clocksource_mmio_init(). i.e.
> 
>      if (be)
>      	clocksource_mmio_init(....., cs_read_be);
>      else if (le)
>      	clocksource_mmio_init(....., cs_read_le);
>      else if (magic)
>      	clocksource_mmio_init(....., cs_read_magic);
> 

There already has the following access interfaces:

static inline u32 ftm_readl(void __iomem *addr)
{
        if (priv->big_endian)
                return ioread32be(addr);
        else
                return ioread32(addr);
}

static inline void ftm_writel(u32 val, void __iomem *addr)
{
        if (priv->big_endian)
                iowrite32be(val, addr);
        else
                iowrite32(val, addr);
}

So I added the following code:

static cycle_t ftm_clocksource_read_up(struct clocksource *c)
{
        return ftm_readl(priv->clksrc_base + FTM_CNT) & 0xffff;
}

clocksource_mmio_init(.....,ftm_clocksource_read_up);

Is this okay ?

Thanks,

BRs
Xiubo

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Thomas Gleixner]

On Thu, 4 Sep 2014, Li.Xiubo@freescale.com wrote:
> > >
> > > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> > > On LS2 platform.
> > >
> > > And ftm_clocksource_read_up() will adapt to this different.
> > 
> > You are missing the point. Why do you want a conditional in a hot
> > path? You know at init time whether the thing is BE or LE, so you can
> > have separate functions for BE/LE or whatever and register that with
> > clocksource_mmio_init(). i.e.
> > 
> >      if (be)
> >      	clocksource_mmio_init(....., cs_read_be);
> >      else if (le)
> >      	clocksource_mmio_init(....., cs_read_le);
> >      else if (magic)
> >      	clocksource_mmio_init(....., cs_read_magic);
> > 
> 
> There already has the following access interfaces:
> 
> static inline u32 ftm_readl(void __iomem *addr)
> {
>         if (priv->big_endian)
>                 return ioread32be(addr);
>         else
>                 return ioread32(addr);
> }
> 
> static inline void ftm_writel(u32 val, void __iomem *addr)
> {
>         if (priv->big_endian)
>                 iowrite32be(val, addr);
>         else
>                 iowrite32(val, addr);
> }
> 
> So I added the following code:
> 
> static cycle_t ftm_clocksource_read_up(struct clocksource *c)
> {
>         return ftm_readl(priv->clksrc_base + FTM_CNT) & 0xffff;
> }
> 
> clocksource_mmio_init(.....,ftm_clocksource_read_up);
> 
> Is this okay ?

No. Sit down, read and try to understand what I wrote, look at your
existing code and figure out WHY it is fundamentally different to what
I told you.

Thanks,

	tglx

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Li.Xiubo]

Hi Thomas,

> > > > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> > > > On LS2 platform.
> > > >
> > > > And ftm_clocksource_read_up() will adapt to this different.
> > >
> > > You are missing the point. Why do you want a conditional in a hot
> > > path? You know at init time whether the thing is BE or LE, so you can
> > > have separate functions for BE/LE or whatever and register that with
> > > clocksource_mmio_init(). i.e.

For our LS1 and LS2+ platforms, there will be only one Kernel Image and can work
for both of them with different dtbs.

Thanks,

BRs
Xiubo

RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.

[Li.Xiubo]

Hi Thomas,

> Subject: RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read
> API.
> 
> Hi Thomas,
> 
> > > > > Since the FTM will be in BE mode on LS1 platform, but will be in LE
> mode
> > > > > On LS2 platform.
> > > > >
> > > > > And ftm_clocksource_read_up() will adapt to this different.
> > > >
> > > > You are missing the point. Why do you want a conditional in a hot
> > > > path? You know at init time whether the thing is BE or LE, so you can
> > > > have separate functions for BE/LE or whatever and register that with
> > > > clocksource_mmio_init(). i.e.
> 
> For our LS1 and LS2+ platforms, there will be only one Kernel Image and can
> work
> for both of them with different dtbs.
> 

Additional:

The LS1 CPUs and some DEVs are in LE endian mode, while most DEVs are in BE mode.
For LS2, CPUs and all DEVs are in LE endian mode.

So here we used one dt node property to distinguish this in run time.

Thanks,

BRs
Xiubo

[PATCH 3/5] Clocksource: Flextimer: Remove the useless code.

[Xiubo Li]

The clock envnt counter will be enabled in proper time and proper
place when needed.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/clocksource/fsl_ftm_timer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
index c4d23d0f..f70fcf2 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -221,8 +221,6 @@ static int __init ftm_clockevent_init(unsigned long freq, int irq)
 					freq / (1 << priv->ps),
 					1, 0xffff);
 
-	ftm_counter_enable(priv->clkevt_base);
-
 	return 0;
 }
 
-- 
1.8.5

[PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler calculation.

[Xiubo Li]

We should minus one after calculating the counter input clock's
prescaler.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/clocksource/fsl_ftm_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
index f70fcf2..974890e 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -311,7 +311,7 @@ static int __init ftm_calc_closest_round_cyc(unsigned long freq)
 						HZ * (1 << priv->ps++));
 	} while (priv->periodic_cyc > 0xFFFF);
 
-	if (priv->ps > FTM_PS_MAX) {
+	if (--priv->ps > FTM_PS_MAX) {
 		pr_err("ftm: the prescaler is %lu > %d\n",
 				priv->ps, FTM_PS_MAX);
 		return -EINVAL;
-- 
1.8.5

Re: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler calculation.

[Thomas Gleixner]

On Tue, 26 Aug 2014, Xiubo Li wrote:

> We should minus one after calculating the counter input clock's
> prescaler.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
> ---
>  drivers/clocksource/fsl_ftm_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
> index f70fcf2..974890e 100644
> --- a/drivers/clocksource/fsl_ftm_timer.c
> +++ b/drivers/clocksource/fsl_ftm_timer.c
> @@ -311,7 +311,7 @@ static int __init ftm_calc_closest_round_cyc(unsigned long freq)
>  						HZ * (1 << priv->ps++));
>  	} while (priv->periodic_cyc > 0xFFFF);
>  
> -	if (priv->ps > FTM_PS_MAX) {
> +	if (--priv->ps > FTM_PS_MAX) {

Looking at this makes me run away screaming. Just because you
increment priv->ps unconditionally in the loop above, you decrement it
again here. Why not fix the calculation proper in the first place?

      for (cyc = ~0UL, ps = 0, div = HZ; cyc > 0xffff; ps++, div *= 2)
      	       cyc = DIV_ROUND_CLOSEST(freq, div);

Hmm?

	tglx

RE: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler calculation.

[Li.Xiubo]

> Subject: Re: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler
> calculation.
> 
> On Tue, 26 Aug 2014, Xiubo Li wrote:
> 
> > We should minus one after calculating the counter input clock's
> > prescaler.
> >
> > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> > Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
> > ---
> >  drivers/clocksource/fsl_ftm_timer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/fsl_ftm_timer.c
> b/drivers/clocksource/fsl_ftm_timer.c
> > index f70fcf2..974890e 100644
> > --- a/drivers/clocksource/fsl_ftm_timer.c
> > +++ b/drivers/clocksource/fsl_ftm_timer.c
> > @@ -311,7 +311,7 @@ static int __init ftm_calc_closest_round_cyc(unsigned
> long freq)
> >  						HZ * (1 << priv->ps++));
> >  	} while (priv->periodic_cyc > 0xFFFF);
> >
> > -	if (priv->ps > FTM_PS_MAX) {
> > +	if (--priv->ps > FTM_PS_MAX) {
> 
> Looking at this makes me run away screaming. Just because you
> increment priv->ps unconditionally in the loop above, you decrement it
> again here. Why not fix the calculation proper in the first place?
> 
>       for (cyc = ~0UL, ps = 0, div = HZ; cyc > 0xffff; ps++, div *= 2)
>       	       cyc = DIV_ROUND_CLOSEST(freq, div);
> 

Yes, this looks nice.

Thanks,

BRs
Xiubo

Re: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler calculation.

[Thomas Gleixner]

On Tue, 26 Aug 2014, Xiubo Li wrote:

> We should minus one after calculating the counter input clock's
> prescaler.

And that's a complete useless changelog.

A) It lacks a proper description of the problem

B) It lacks a proper argument WHY we must decrement the prescaler
   value

   Hint: I used the verb "must" instead of "should". Can you figure
   out why?

Thanks,

	tglx

RE: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler calculation.

[Li.Xiubo]

Hi Thomas,

Thanks very much for your comments.

Firstly I must tell the true that English is not my mother language.


> Subject: Re: [PATCH 4/5] Clocksource: Flextimer: Fix counter clock prescaler
> calculation.
> 
> On Tue, 26 Aug 2014, Xiubo Li wrote:
> 
> > We should minus one after calculating the counter input clock's
> > prescaler.
> 
> And that's a complete useless changelog.
> 
> A) It lacks a proper description of the problem
> 
> B) It lacks a proper argument WHY we must decrement the prescaler
>    value
> 
>    Hint: I used the verb "must" instead of "should". Can you figure
>    out why?
> 

Yes, we should use 'must' here, because we are using 'ps++' in the loop code,
the correct value should be 'ps = ps - 1', after the loop.

BRs
Xiubo

[PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.

[Xiubo Li]

FTM source clock is selectable:
Source clock can be the system clock, the fixed frequency clock,
or an external clock.
Fixed frequency clock is an additional clock input to allow the
selection of an on chip clock source other than the system clock.
Selecting external clock connects FTM clock to a chip level input
pin therefore allowing to synchronize the FTM counter with an off
chip clock source

Clock Source Selection:
Selects one of the three FTM counter clock sources.

00 No clock selected. This in effect disables the FTM counter.
01 System clock
---
10 Fixed frequency clock
11 External clock

These two will be useful for the alarm on LS1 platform in late future,
the system clock's frequency is too high to get a long delay timer when
go to sleep, so the fixed or external clock could be used instead.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
Cc: Wang Dongsheng <dongsheng.wang@freescale.com>
---
 drivers/clocksource/fsl_ftm_timer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c
index 974890e..3935ba2 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 
 #define FTM_SC		0x00
+#define FTM_SC_CLKS_NON		0
+#define FTM_SC_CLKS_SYS		1
 #define FTM_SC_CLK_SHIFT	3
 #define FTM_SC_CLK_MASK	(0x3 << FTM_SC_CLK_SHIFT)
 #define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_SHIFT)
@@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base)
 	/* select and enable counter clock source */
 	val = ftm_readl(base + FTM_SC);
 	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
-	val |= priv->ps | FTM_SC_CLK(1);
+	val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS);
 	ftm_writel(val, base + FTM_SC);
 }
 
@@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base)
 
 	/* disable counter clock source */
 	val = ftm_readl(base + FTM_SC);
-	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
+	val &= ~(FTM_SC_CLK_MASK);
+	val |= FTM_SC_CLK(FTM_SC_CLKS_NON);
 	ftm_writel(val, base + FTM_SC);
 }
 
-- 
1.8.5

Re: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.

[Thomas Gleixner]

On Tue, 26 Aug 2014, Xiubo Li wrote:
> FTM source clock is selectable:
> Source clock can be the system clock, the fixed frequency clock,
> or an external clock.
> Fixed frequency clock is an additional clock input to allow the
> selection of an on chip clock source other than the system clock.
> Selecting external clock connects FTM clock to a chip level input
> pin therefore allowing to synchronize the FTM counter with an off
> chip clock source
> 
> Clock Source Selection:
> Selects one of the three FTM counter clock sources.
> 
> 00 No clock selected. This in effect disables the FTM counter.
> 01 System clock
> ---
> 10 Fixed frequency clock
> 11 External clock
> 
> These two will be useful for the alarm on LS1 platform in late future,
> the system clock's frequency is too high to get a long delay timer when
> go to sleep, so the fixed or external clock could be used instead.

And how is that relevant to changing the mask from a hardcoded value
to a constant? It's still hardcoded at compile time and I don't see
how that helps chosing the clock at runtime for a particular use case.
 
>  #define FTM_SC		0x00
> +#define FTM_SC_CLKS_NON		0
> +#define FTM_SC_CLKS_SYS		1
>  #define FTM_SC_CLK_SHIFT	3
>  #define FTM_SC_CLK_MASK	(0x3 << FTM_SC_CLK_SHIFT)
>  #define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_SHIFT)
> @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base)
>  	/* select and enable counter clock source */
>  	val = ftm_readl(base + FTM_SC);
>  	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
> -	val |= priv->ps | FTM_SC_CLK(1);
> +	val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS);
>  	ftm_writel(val, base + FTM_SC);
>  }
>  
> @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base)
>  
>  	/* disable counter clock source */
>  	val = ftm_readl(base + FTM_SC);
> -	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
> +	val &= ~(FTM_SC_CLK_MASK);

So you remove the FTM_SC_PS_MASK without mentioning it in the
changelog. Either this is by mistake or it wants to be documented WHY
it is not needed in the first place.

Thanks,

	tglx

RE: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.

[Li.Xiubo]

> Subject: Re: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source
> selection.
> 
> On Tue, 26 Aug 2014, Xiubo Li wrote:
> > FTM source clock is selectable:
> > Source clock can be the system clock, the fixed frequency clock,
> > or an external clock.
> > Fixed frequency clock is an additional clock input to allow the
> > selection of an on chip clock source other than the system clock.
> > Selecting external clock connects FTM clock to a chip level input
> > pin therefore allowing to synchronize the FTM counter with an off
> > chip clock source
> >
> > Clock Source Selection:
> > Selects one of the three FTM counter clock sources.
> >
> > 00 No clock selected. This in effect disables the FTM counter.
> > 01 System clock
> > ---
> > 10 Fixed frequency clock
> > 11 External clock
> >
> > These two will be useful for the alarm on LS1 platform in late future,
> > the system clock's frequency is too high to get a long delay timer when
> > go to sleep, so the fixed or external clock could be used instead.
> 
> And how is that relevant to changing the mask from a hardcoded value
> to a constant? It's still hardcoded at compile time and I don't see
> how that helps chosing the clock at runtime for a particular use case.
> 
> >  #define FTM_SC		0x00
> > +#define FTM_SC_CLKS_NON		0
> > +#define FTM_SC_CLKS_SYS		1
> >  #define FTM_SC_CLK_SHIFT	3
> >  #define FTM_SC_CLK_MASK	(0x3 << FTM_SC_CLK_SHIFT)
> >  #define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_SHIFT)
> > @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base)
> >  	/* select and enable counter clock source */
> >  	val = ftm_readl(base + FTM_SC);
> >  	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
> > -	val |= priv->ps | FTM_SC_CLK(1);
> > +	val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS);
> >  	ftm_writel(val, base + FTM_SC);
> >  }
> >
> > @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base)
> >
> >  	/* disable counter clock source */
> >  	val = ftm_readl(base + FTM_SC);
> > -	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
> > +	val &= ~(FTM_SC_CLK_MASK);
> 
> So you remove the FTM_SC_PS_MASK without mentioning it in the
> changelog. Either this is by mistake or it wants to be documented WHY
> it is not needed in the first place.
> 

This is just prepare for the other new features in the future.

And delay this changes maybe better.

Thanks,

BRs
Xiubo

RE: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.

[Thomas Gleixner]

On Thu, 4 Sep 2014, Li.Xiubo@freescale.com wrote:
> > So you remove the FTM_SC_PS_MASK without mentioning it in the
> > changelog. Either this is by mistake or it wants to be documented WHY
> > it is not needed in the first place.
> > 
> 
> This is just prepare for the other new features in the future.

So split it into a separate patch and provide a proper changelog WHY
you remove it.

Thanks,

	tglx

RE: [PATCH 0/5] Clocksource: Flextimer: Merged to LS1

[Li.Xiubo]

Ping :)

Thanks,

BRs
Xiubo




> -----Original Message-----
> From: Xiubo Li [mailto:Li.Xiubo@freescale.com]
> Sent: Tuesday, August 26, 2014 1:46 PM
> To: daniel.lezcano@linaro.org; tglx@linutronix.de
> Cc: Wang Dongsheng-B40534; linux-kernel@vger.kernel.org; Xiubo Li-B47053
> Subject: [PATCH 0/5] Clocksource: Flextimer: Merged to LS1
> 
> 
> Xiubo Li (5):
>   Clocksource: Flextimer: Set cpumask to cpu_possible_mask
>   Clocksource: Flextimer: Use internal clocksource read API.
>   Clocksource: Flextimer: Remove the useless code.
>   Clocksource: Flextimer: Fix counter clock prescaler calculation.
>   Clocksource: Flextimer: Use Macro for clock source selection.
> 
>  drivers/clocksource/fsl_ftm_timer.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> --
> 1.8.5