[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <NRREDR.U6G6SM5BIXEC3@crapouillou.net>
Date: Mon, 13 Jun 2022 10:02:11 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
Cc: mturquette@...libre.com, sboyd@...nel.org,
linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: ingenic-tcu: Properly enable registers before
accessing timers
Hi Aidan,
Le ven., juin 3 2022 at 14:47:05 +0100, Aidan MacDonald
<aidanmacdonald.0x0@...il.com> a écrit :
> Access to registers is guarded by ingenic_tcu_{enable,disable}_regs()
> so the stop bit can be cleared before accessing a timer channel, but
> those functions did not clear the stop bit on SoCs with a global TCU
> clock gate.
>
> Testing on the X1000 has revealed that the stop bits must be cleared
> _and_ the global TCU clock must be ungated to access timer registers.
> Programming manuals for the X1000, JZ4740, and JZ4725B specify this
> behavior. If the stop bit isn't cleared, then writes to registers do
> not take effect, which can leave clocks with no defined parent when
> registered and leave clock tree state out of sync with the hardware,
> triggering bugs in downstream drivers relying on TCU clocks.
>
> Fixing this is easy: have ingenic_tcu_{enable,disable}_regs() always
> clear the stop bit, regardless of the presence of a global TCU gate.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
Tested on JZ4770, it still works fine.
Reviewed-by: Paul Cercueil <paul@...pouillou.net>
Still needs a Fixes: tag (+ Cc: linux-stable) so I'm expecting a V2.
Cheers,
-Paul
> ---
> drivers/clk/ingenic/tcu.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> index 201bf6e6b6e0..d5544cbc5c48 100644
> --- a/drivers/clk/ingenic/tcu.c
> +++ b/drivers/clk/ingenic/tcu.c
> @@ -101,15 +101,11 @@ static bool ingenic_tcu_enable_regs(struct
> clk_hw *hw)
> bool enabled = false;
>
> /*
> - * If the SoC has no global TCU clock, we must ungate the channel's
> - * clock to be able to access its registers.
> - * If we have a TCU clock, it will be enabled automatically as it
> has
> - * been attached to the regmap.
> + * According to the programming manual, a timer channel's registers
> can
> + * only be accessed when the channel's stop bit is clear.
> */
> - if (!tcu->clk) {
> - enabled = !!ingenic_tcu_is_enabled(hw);
> - regmap_write(tcu->map, TCU_REG_TSCR, BIT(info->gate_bit));
> - }
> + enabled = !!ingenic_tcu_is_enabled(hw);
> + regmap_write(tcu->map, TCU_REG_TSCR, BIT(info->gate_bit));
>
> return enabled;
> }
> @@ -120,8 +116,7 @@ static void ingenic_tcu_disable_regs(struct
> clk_hw *hw)
> const struct ingenic_tcu_clk_info *info = tcu_clk->info;
> struct ingenic_tcu *tcu = tcu_clk->tcu;
>
> - if (!tcu->clk)
> - regmap_write(tcu->map, TCU_REG_TSSR, BIT(info->gate_bit));
> + regmap_write(tcu->map, TCU_REG_TSSR, BIT(info->gate_bit));
> }
>
> static u8 ingenic_tcu_get_parent(struct clk_hw *hw)
> --
> 2.35.1
>
Powered by blists - more mailing lists