[PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Aidan MacDonald]

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>
---
 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

Re: [PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Stephen Boyd]

Quoting Aidan MacDonald (2022-06-03 06:47:05)
> 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>
> ---

Any Fixes: tag?

Re: [PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Aidan MacDonald]

Stephen Boyd <sboyd@kernel.org> writes:

> Quoting Aidan MacDonald (2022-06-03 06:47:05)
>> 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>
>> ---
>
> Any Fixes: tag?

Probably 4f89e4b8f121 ("clk: ingenic: Add driver for the TCU clocks")
but I don't have docs or hardware to confirm the bug affects the jz4770,
which is the only other SoC affected by the change.

I think what caused my problem was my bootloader stopping all the timer
channels. The stop bits are supposed to be zeroed at reset, so I'd guess
the jz4770 relied on that and only worked by accident.

I'll send a v2 along shortly. Is it worth CC'ing stable as well?

Re: [PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Paul Cercueil]

Le ven., juin 10 2022 at 16:43:27 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> 
> Stephen Boyd <sboyd@kernel.org> writes:
> 
>>  Quoting Aidan MacDonald (2022-06-03 06:47:05)
>>>  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>
>>>  ---
>> 
>>  Any Fixes: tag?
> 
> Probably 4f89e4b8f121 ("clk: ingenic: Add driver for the TCU clocks")
> but I don't have docs or hardware to confirm the bug affects the 
> jz4770,
> which is the only other SoC affected by the change.
> 
> I think what caused my problem was my bootloader stopping all the 
> timer
> channels. The stop bits are supposed to be zeroed at reset, so I'd 
> guess
> the jz4770 relied on that and only worked by accident.

I'll test it on JZ4770 this weekend.

> I'll send a v2 along shortly. Is it worth CC'ing stable as well?

If the bug is in jz-5.18 or earlier, yes.

Cheers,
-Paul

Re: [PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Aidan MacDonald]

Paul Cercueil <paul@crapouillou.net> writes:

> Le ven., juin 10 2022 at 16:43:27 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> Stephen Boyd <sboyd@kernel.org> writes:
>> 
>>>  Quoting Aidan MacDonald (2022-06-03 06:47:05)
>>>>  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>
>>>>  ---
>>>  Any Fixes: tag?
>> Probably 4f89e4b8f121 ("clk: ingenic: Add driver for the TCU clocks")
>> but I don't have docs or hardware to confirm the bug affects the jz4770,
>> which is the only other SoC affected by the change.
>> I think what caused my problem was my bootloader stopping all the timer
>> channels. The stop bits are supposed to be zeroed at reset, so I'd guess
>> the jz4770 relied on that and only worked by accident.
>
> I'll test it on JZ4770 this weekend.
>
>> I'll send a v2 along shortly. Is it worth CC'ing stable as well?
>
> If the bug is in jz-5.18 or earlier, yes.
>
> Cheers,
> -Paul

Thanks. Guess I'll wait for your test results, though I don't expect any
problems.

Re: [PATCH] clk: ingenic-tcu: Properly enable registers before accessing timers

[Paul Cercueil]

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
>