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

From: Anson Huang
Date: Tue Mar 27 2018 - 22:25:00 EST




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