Messages in this thread | | | Date | Mon, 13 Jun 2022 10:02:11 +0100 | From | Paul Cercueil <> | 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@gmail.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@gmail.com>
Tested on JZ4770, it still works fine.
Reviewed-by: Paul Cercueil <paul@crapouillou.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 >
| |