RE: [PATCH 2/2] clocksource/drivers/imx-tpm: add different counter width support

[Anson Huang]

Anson Huang
Best Regards!


> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@xxxxxxxxxx]
> Sent: Monday, March 26, 2018 10:11 PM
> To: Anson Huang <anson.huang@xxxxxxx>; tglx@xxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; A.s. Dong
> <aisheng.dong@xxxxxxx>
> Cc: dl-linux-imx <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] clocksource/drivers/imx-tpm: add different counter
> width support
> 
> On 26/03/2018 09:47, Anson Huang wrote:
> > Different TPM modules have different width counters which is 16-bit or
> > 32-bit, the counter width can be read from TPM_PARAM register
> > bit[23:16], this patch adds dynamic check for counter width to support
> > both 16-bit and 32-bit TPM modules.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> 
> Are you sure you shouldn't use a prescalar with 16b and reduce its rating? What
> will be the duration of the clocksource before wrapping up?
 
The duration currently is 0xffff / 8MHz, ~8.2mS, yes, it is too short, I will make the prescalar
higher to increase the duration. 

> 
> Also, can you do the change to prevent computing the mask at each
> 'read_count' call ?

I think we can just return the value read from the CNT register, even the width
is 16, the upper 16 bits always 0, so it does NOT matter, I will remove the mask computation
in V2 patch.

Thanks.

Anson.

> 
> > ---
> >  drivers/clocksource/timer-imx-tpm.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-imx-tpm.c
> > b/drivers/clocksource/timer-imx-tpm.c
> > index 7403e49..5063b77 100644
> > --- a/drivers/clocksource/timer-imx-tpm.c
> > +++ b/drivers/clocksource/timer-imx-tpm.c
> > @@ -17,6 +17,9 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/sched_clock.h>
> >
> > +#define TPM_PARAM			0x4
> > +#define TPM_PARAM_WIDTH_SHIFT		16
> > +#define TPM_PARAM_WIDTH_MASK		(0xff << 16)
> >  #define TPM_SC				0x10
> >  #define TPM_SC_CMOD_INC_PER_CNT		(0x1 << 3)
> >  #define TPM_SC_CMOD_DIV_DEFAULT		0x3
> > @@ -33,6 +36,7 @@
> >  #define TPM_C0SC_CHF_MASK		(0x1 << 7)
> >  #define TPM_C0V				0x24
> >
> > +static int counter_width;
> >  static void __iomem *timer_base;
> >  static struct clock_event_device clockevent_tpm;
> >
> > @@ -66,7 +70,7 @@ static struct delay_timer tpm_delay_timer;
> >
> >  static inline unsigned long tpm_read_counter(void)  {
> > -	return readl(timer_base + TPM_CNT);
> > +	return readl(timer_base + TPM_CNT) & GENMASK(counter_width - 1, 0);
> >  }
> >
> >  static unsigned long tpm_read_current_timer(void) @@ -85,10 +89,11 @@
> > static int __init tpm_clocksource_init(unsigned long rate)
> >  	tpm_delay_timer.freq = rate;
> >  	register_current_timer_delay(&tpm_delay_timer);
> >
> > -	sched_clock_register(tpm_read_sched_clock, 32, rate);
> > +	sched_clock_register(tpm_read_sched_clock, counter_width, rate);
> >
> >  	return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm",
> > -				     rate, 200, 32, clocksource_mmio_readl_up);
> > +				     rate, 200, counter_width,
> > +				     clocksource_mmio_readl_up);
> >  }
> >
> >  static int tpm_set_next_event(unsigned long delta, @@ -153,8 +158,8
> > @@ static int __init tpm_clockevent_init(unsigned long rate, int irq)
> >
> >  	clockevent_tpm.cpumask = cpumask_of(0);
> >  	clockevent_tpm.irq = irq;
> > -	clockevents_config_and_register(&clockevent_tpm,
> > -					rate, 300, 0xfffffffe);
> > +	clockevents_config_and_register(&clockevent_tpm, rate, 300,
> > +					GENMASK(counter_width - 1, 1));
> >
> >  	return ret;
> >  }
> > @@ -199,6 +204,8 @@ static int __init tpm_timer_init(struct device_node
> *np)
> >  		goto err_per_clk_enable;
> >  	}
> >
> > +	counter_width = (readl(timer_base + TPM_PARAM) &
> TPM_PARAM_WIDTH_MASK)
> > +		>> TPM_PARAM_WIDTH_SHIFT;
> >  	/*
> >  	 * Initialize tpm module to a known state
> >  	 * 1) Counter disabled
> > @@ -220,7 +227,7 @@ static int __init tpm_timer_init(struct device_node
> *np)
> >  		     timer_base + TPM_SC);
> >
> >  	/* set MOD register to maximum for free running mode */
> > -	writel(0xffffffff, timer_base + TPM_MOD);
> > +	writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD);
> >
> >  	rate = clk_get_rate(per) >> 3;
> >  	ret = tpm_clocksource_init(rate);
> >
> 
> 
> --
> 
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.li
> naro.org%2F&data=02%7C01%7CAnson.Huang%40nxp.com%7C34d41253162e
> 4956105a08d593236499%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636576702585890585&sdata=td%2FN%2BpDuc%2BeFn5SZjAS6u6v6NgCQ
> AcZkNr9KS%2B8XiK0%3D&reserved=0> Linaro.org â Open source software for
> ARM SoCs
> 
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.f
> acebook.com%2Fpages%2FLinaro&data=02%7C01%7CAnson.Huang%40nxp.co
> m%7C34d41253162e4956105a08d593236499%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C636576702585890585&sdata=CePZL2guHTui9SbxWpY
> S2V%2FRqTZsIUEgPVLz6m8794E%3D&reserved=0> Facebook |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitter
> .com%2F%23!%2Flinaroorg&data=02%7C01%7CAnson.Huang%40nxp.com%7C
> 34d41253162e4956105a08d593236499%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636576702585890585&sdata=f37TVduDxk3ihZ8NQA2O1Mq
> LBB5wdLLhbehnQLkQKFA%3D&reserved=0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.li
> naro.org%2Flinaro-blog%2F&data=02%7C01%7CAnson.Huang%40nxp.com%7C
> 34d41253162e4956105a08d593236499%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636576702585890585&sdata=%2BAOgLPpZHN%2FjX66pyo8
> TW5RhybKrPYaijWs4z8OYP48%3D&reserved=0> Blog