Message ID | 20220617122254.738900-1-aidanmacdonald.0x0@gmail.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | [v2] clk: ingenic-tcu: Properly enable registers before accessing timers | expand |
Quoting Aidan MacDonald (2022-06-17 05:22:54) > 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. > This appears to be the norm on Ingenic SoCs, and is specified in the > documentation for the X1000 and numerous JZ47xx SoCs. > > If the stop bit isn't cleared, register writes don't take effect and > the system can be left in a broken state, eg. the watchdog timer may > not run. > > The bug probably went unnoticed because stop bits are zeroed when > the SoC is reset, and the kernel does not set them unless a timer > gets disabled at runtime. However, it is possible that a bootloader > or a previous kernel (if using kexec) leaves the stop bits set and > we should not rely on them being cleared. > > Fixing this is easy: have ingenic_tcu_{enable,disable}_regs() always > clear the stop bit, regardless of the presence of a global TCU gate. > > Reviewed-by: Paul Cercueil <paul@crapouillou.net> > Tested-by: Paul Cercueil <paul@crapouillou.net> > Fixes: 4f89e4b8f121 ("clk: ingenic: Add driver for the TCU clocks") > Cc: stable@vger.kernel.org > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- Applied to clk-fixes
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)