Hi Wolfram,
On Wed, Nov 16, 2022 at 9:21 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> Documentation for most CMTs say that we need to wait two input clocks
> before changes propagate to the timer. This is especially relevant when
> we stop the timer to change further settings. Implement the delays
> according to the spec. To avoid unnecessary delays in atomic mode, we
> also check if the to-be-written value actually differs. CMCNT is a bit
> special because testing showed that we need to wait 3 cycles instead.
> AFAIU, this is also true for all CMTs. Also, the WRFLAG needs to be
> checked before writing. This fixes "cannot clear CMCNT" messages which
> occur often on R-Car Gen4 SoCs, but only very rarely on older SoCs for
> some reason.
>
> Fixes: 81b3b2711072 ("clocksource: sh_cmt: Add support for multiple channels per device")
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
Thanks for your patch!
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -13,6 +13,7 @@
> #include <linux/init.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/ioport.h>
> #include <linux/irq.h>
> #include <linux/module.h>
> @@ -116,6 +117,7 @@ struct sh_cmt_device {
> void __iomem *mapbase;
> struct clk *clk;
> unsigned long rate;
> + unsigned long reg_delay;
unsigned int (everywhere)?
Yes, this is the result of a long-by-long division, but it should be
a small value anyway.
>
> raw_spinlock_t lock; /* Protect the shared start/stop register */
>
> @@ -270,12 +284,26 @@ static inline u32 sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
>
> static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch, u32 value)
> {
> + /* Tests showed that we need to wait 3 clocks here */
> + unsigned long cmcnt_delay = ch->cmt->reg_delay * 3 / 2;
DIV_ROUND_UP()
> + u32 reg;
> +
> + if (ch->cmt->info->model > SH_CMT_16BIT)
> + read_poll_timeout_atomic(sh_cmt_read_cmcsr, reg, !(reg & SH_CMT32_CMCSR_WRFLG),
> + 1, cmcnt_delay, false, ch);
> +
> ch->cmt->info->write_count(ch->ioctrl, CMCNT, value);
> + udelay(cmcnt_delay);
> }
> @@ -1032,10 +1043,16 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev)
> if (ret < 0)
> goto err_clk_unprepare;
>
> - if (cmt->info->width == 16)
> - cmt->rate = clk_get_rate(cmt->clk) / 512;
> - else
> - cmt->rate = clk_get_rate(cmt->clk) / 8;
> + rate = clk_get_rate(cmt->clk);
> + if (!rate) {
> + ret = -EINVAL;
> + goto err_clk_disable;
> + }
> +
> + /* We shall wait 2 input clks after register writes */
> + if (cmt->info->model >= SH_CMT_48BIT) // FIXME: Really not needed for older ones?
> + cmt->reg_delay = 2000000UL / rate + 1;
> + cmt->rate = cmt->info->width == 16 ? rate / 512 : rate / 8;
cmt->rate = rate / (cmt->info->width == 16 ? 512 : 8);
>
> /* Map the memory resource(s). */
> ret = sh_cmt_map_memory(cmt);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
