Hi, Daniel Best Regards! Anson Huang
> -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] > Sent: 2018年11月22日 21:21 > To: Anson Huang <anson.hu...@nxp.com>; t...@linutronix.de; > linux-kernel@vger.kernel.org > Cc: dl-linux-imx <linux-...@nxp.com> > Subject: Re: [PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to > timer-of > > > Hi Anson, > > > On 06/11/2018 06:15, Anson Huang wrote: > > Convert the driver to use the timer_of helpers. > > This allows to handle timer base, clock and irq using common timer_of > > driver and it simplifies the code. > > Can you do an extra move by passing the timer_of structure around for: > - tpm_timer_disable > - tpm_timer_enable > - tpm_irq_acknowledge > > and use to_timer_of(clkevt) to retrieve the timer_base value for other > clockevent callbacks ? > > The clockevent code will be self-encapsulated. > I remove below inline function, and put the register operation in the clockevent callback using to_timer_of(clkevt) and timer_of_base(), it make the code more simplified. tpm_timer_disable tpm_timer_enable tpm_irq_acknowledge For other functions like tpm_read_counter(), if using timer_of_base() to retrieve the timer base, it needs to_tpm to be defined before this function, but to_tpm need to refer to some clockevent callbacks which calls tpm_read_counter(), so the function definition sequence is conflict unless we add an function statement, so I prefer to leave it as what it is now. Please help review V2 patch, thanks. Anson. > > > > Signed-off-by: Anson Huang <anson.hu...@nxp.com> > > --- > > drivers/clocksource/timer-imx-tpm.c | 136 > > +++++++++++++++--------------------- > > 1 file changed, 55 insertions(+), 81 deletions(-) > > > > diff --git a/drivers/clocksource/timer-imx-tpm.c > > b/drivers/clocksource/timer-imx-tpm.c > > index b7aa2b8..c3dd4d2 100644 > > --- a/drivers/clocksource/timer-imx-tpm.c > > +++ b/drivers/clocksource/timer-imx-tpm.c > > @@ -12,6 +12,8 @@ > > #include <linux/of_irq.h> > > #include <linux/sched_clock.h> > > > > +#include "timer-of.h" > > + > > #define TPM_PARAM 0x4 > > #define TPM_PARAM_WIDTH_SHIFT 16 > > #define TPM_PARAM_WIDTH_MASK (0xff << 16) > > @@ -33,9 +35,7 @@ > > #define TPM_C0V 0x24 > > > > static int counter_width; > > -static int rating; > > static void __iomem *timer_base; > > -static struct clock_event_device clockevent_tpm; > > > > static inline void tpm_timer_disable(void) { @@ -80,19 +80,6 @@ > > static u64 notrace tpm_read_sched_clock(void) > > return tpm_read_counter(); > > } > > > > -static int __init tpm_clocksource_init(unsigned long rate) -{ > > - tpm_delay_timer.read_current_timer = &tpm_read_current_timer; > > - tpm_delay_timer.freq = rate; > > - register_current_timer_delay(&tpm_delay_timer); > > - > > - sched_clock_register(tpm_read_sched_clock, counter_width, rate); > > - > > - return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm", > > - rate, rating, counter_width, > > - clocksource_mmio_readl_up); > > -} > > - > > static int tpm_set_next_event(unsigned long delta, > > struct clock_event_device *evt) > > { > > @@ -137,74 +124,77 @@ static irqreturn_t tpm_timer_interrupt(int irq, > void *dev_id) > > return IRQ_HANDLED; > > } > > > > -static struct clock_event_device clockevent_tpm = { > > - .name = "i.MX7ULP TPM Timer", > > - .features = CLOCK_EVT_FEAT_ONESHOT, > > - .set_state_oneshot = tpm_set_state_oneshot, > > - .set_next_event = tpm_set_next_event, > > - .set_state_shutdown = tpm_set_state_shutdown, > > +static struct timer_of to_tpm = { > > + .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, > > + .clkevt = { > > + .name = "i.MX7ULP TPM Timer", > > + .rating = 200, > > + .features = CLOCK_EVT_FEAT_ONESHOT, > > + .set_state_shutdown = tpm_set_state_shutdown, > > + .set_state_oneshot = tpm_set_state_oneshot, > > + .set_next_event = tpm_set_next_event, > > + .cpumask = cpu_possible_mask, > > + }, > > + .of_irq = { > > + .handler = tpm_timer_interrupt, > > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > > + }, > > }; > > > > -static int __init tpm_clockevent_init(unsigned long rate, int irq) > > +static int __init tpm_clocksource_init(void) > > { > > - int ret; > > + tpm_delay_timer.read_current_timer = &tpm_read_current_timer; > > + tpm_delay_timer.freq = timer_of_rate(&to_tpm) >> 3; > > + register_current_timer_delay(&tpm_delay_timer); > > > > - ret = request_irq(irq, tpm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL, > > - "i.MX7ULP TPM Timer", &clockevent_tpm); > > + sched_clock_register(tpm_read_sched_clock, counter_width, > > + timer_of_rate(&to_tpm) >> 3); > > > > - clockevent_tpm.rating = rating; > > - clockevent_tpm.cpumask = cpumask_of(0); > > - clockevent_tpm.irq = irq; > > - clockevents_config_and_register(&clockevent_tpm, rate, 300, > > - GENMASK(counter_width - 1, 1)); > > + return clocksource_mmio_init(timer_base + TPM_CNT, > > + "imx-tpm", > > + timer_of_rate(&to_tpm) >> 3, > > + to_tpm.clkevt.rating, > > + counter_width, > > + clocksource_mmio_readl_up); > > +} > > > > - return ret; > > +static void __init tpm_clockevent_init(void) { > > + clockevents_config_and_register(&to_tpm.clkevt, > > + timer_of_rate(&to_tpm) >> 3, > > + 300, > > + GENMASK(counter_width - 1, > > + 1)); > > } > > > > static int __init tpm_timer_init(struct device_node *np) { > > - struct clk *ipg, *per; > > - int irq, ret; > > - u32 rate; > > - > > - timer_base = of_iomap(np, 0); > > - if (!timer_base) { > > - pr_err("tpm: failed to get base address\n"); > > - return -ENXIO; > > - } > > - > > - irq = irq_of_parse_and_map(np, 0); > > - if (!irq) { > > - pr_err("tpm: failed to get irq\n"); > > - ret = -ENOENT; > > - goto err_iomap; > > - } > > + struct clk *ipg; > > + int ret; > > > > ipg = of_clk_get_by_name(np, "ipg"); > > - per = of_clk_get_by_name(np, "per"); > > - if (IS_ERR(ipg) || IS_ERR(per)) { > > - pr_err("tpm: failed to get ipg or per clk\n"); > > - ret = -ENODEV; > > - goto err_clk_get; > > + if (IS_ERR(ipg)) { > > + pr_err("tpm: failed to get ipg clk\n"); > > + return -ENODEV; > > } > > - > > /* enable clk before accessing registers */ > > ret = clk_prepare_enable(ipg); > > if (ret) { > > pr_err("tpm: ipg clock enable failed (%d)\n", ret); > > - goto err_clk_get; > > + clk_put(ipg); > > + return ret; > > } > > > > - ret = clk_prepare_enable(per); > > - if (ret) { > > - pr_err("tpm: per clock enable failed (%d)\n", ret); > > - goto err_per_clk_enable; > > - } > > + ret = timer_of_init(np, &to_tpm); > > + if (ret) > > + return ret; > > + > > + timer_base = timer_of_base(&to_tpm); > > > > - counter_width = (readl(timer_base + TPM_PARAM) & > TPM_PARAM_WIDTH_MASK) > > - >> TPM_PARAM_WIDTH_SHIFT; > > + counter_width = (readl(timer_base + TPM_PARAM) > > + & TPM_PARAM_WIDTH_MASK) >> TPM_PARAM_WIDTH_SHIFT; > > /* use rating 200 for 32-bit counter and 150 for 16-bit counter */ > > - rating = counter_width == 0x20 ? 200 : 150; > > + to_tpm.clkevt.rating = counter_width == 0x20 ? 200 : 150; > > > > /* > > * Initialize tpm module to a known state @@ -229,29 +219,13 @@ > > static int __init tpm_timer_init(struct device_node *np) > > writel(TPM_SC_CMOD_INC_PER_CNT | > > (counter_width == 0x20 ? > > TPM_SC_CMOD_DIV_DEFAULT : TPM_SC_CMOD_DIV_MAX), > > - timer_base + TPM_SC); > > + timer_base + TPM_SC); > > > > /* set MOD register to maximum for free running mode */ > > writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD); > > > > - rate = clk_get_rate(per) >> 3; > > - ret = tpm_clocksource_init(rate); > > - if (ret) > > - goto err_per_clk_enable; > > - > > - ret = tpm_clockevent_init(rate, irq); > > - if (ret) > > - goto err_per_clk_enable; > > - > > - return 0; > > + tpm_clockevent_init(); > > > > -err_per_clk_enable: > > - clk_disable_unprepare(ipg); > > -err_clk_get: > > - clk_put(per); > > - clk_put(ipg); > > -err_iomap: > > - iounmap(timer_base); > > - return ret; > > + return tpm_clocksource_init(); > > } > > TIMER_OF_DECLARE(imx7ulp, "fsl,imx7ulp-tpm", tpm_timer_init); > > > > > -- > > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww > .linaro.org%2F&data=02%7C01%7Canson.huang%40nxp.com%7C5ed9d8 > 83d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd99c5c301635% > 7C0%7C0%7C636784896822292291&sdata=XOymB9k6maxvJhG4%2F2yj > jBP3YmbdY18mUnFoP9YwKqg%3D&reserved=0> Linaro.org │ Open > source software for ARM SoCs > > Follow Linaro: > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww > .facebook.com%2Fpages%2FLinaro&data=02%7C01%7Canson.huang%40 > nxp.com%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa > 92cd99c5c301635%7C0%7C0%7C636784896822292291&sdata=xUKFR2 > 6E90IoKaRZP5nSk82kn3UT8F5zsBV2cJVfmcs%3D&reserved=0> Facebook > | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt > er.com%2F%23!%2Flinaroorg&data=02%7C01%7Canson.huang%40nxp.c > om%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd > 99c5c301635%7C0%7C0%7C636784896822292291&sdata=3iesM9aEJZ0 > QNqcfIdX%2Fe8dgah1SWJaHxIdRQlAqelg%3D&reserved=0> Twitter | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww > .linaro.org%2Flinaro-blog%2F&data=02%7C01%7Canson.huang%40nxp.c > om%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd > 99c5c301635%7C0%7C0%7C636784896822292291&sdata=6e%2FyD7wj > dyLvmRnOe6G0YlnUr8yQx2z2oniVOsRMmOo%3D&reserved=0> Blog
