[PATCH v2 0/2] Fix and clean MT8365 clock indexes

[PATCH v2 0/2] Fix and clean MT8365 clock indexes

[Alexandre Mergnat]

The first commit replace un unusable clock by another one.

The second one fix a regression which prevent the initialization of the
latest indexed clocks. The regression is introduced with [1].

[1]: Commit ffe91cb28f6a ("clk: mediatek: mt8365: Convert to
     mtk_clk_simple_{probe,remove}()")

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
Changes in v2:
- Add the missing clocks in the mt8365 clock driver.
- Revert all change in binding file except for one clock.
- Link to v1: https://lore.kernel.org/r/20230517-fix-clk-index-v1-0-142077a1732b@baylibre.com

---
Alexandre Mergnat (2):
      dt-bindings: clock: mediatek: replace unusable clock
      clk: mediatek: mt8365: Fix index issue

 drivers/clk/mediatek/clk-mt8365.c               | 11 +++++++++++
 include/dt-bindings/clock/mediatek,mt8365-clk.h |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)
---
base-commit: ac9a78681b921877518763ba0e89202254349d1b
change-id: 20230517-fix-clk-index-96043baf65be

Best regards,
-- 
Alexandre Mergnat <amergnat@baylibre.com>

[PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[Alexandre Mergnat]

The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
added to the kernel driver, otherwise the CPU just halt and the board is
rebooted by the wathdog.

Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
re-shuffling index and then preserve the ABI.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 include/dt-bindings/clock/mediatek,mt8365-clk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/dt-bindings/clock/mediatek,mt8365-clk.h b/include/dt-bindings/clock/mediatek,mt8365-clk.h
index f9aff1775810..0a841e7cc1c3 100644
--- a/include/dt-bindings/clock/mediatek,mt8365-clk.h
+++ b/include/dt-bindings/clock/mediatek,mt8365-clk.h
@@ -199,7 +199,7 @@
 #define CLK_IFR_PWRAP_TMR		46
 #define CLK_IFR_PWRAP_SPI		47
 #define CLK_IFR_PWRAP_SYS		48
-#define CLK_IFR_MCU_PM_BK		49
+#define CLK_IFR_AES_TOP0_BK		49
 #define CLK_IFR_IRRX_26M		50
 #define CLK_IFR_IRRX_32K		51
 #define CLK_IFR_I2C0_AXI		52

-- 
2.25.1

[PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue

[Alexandre Mergnat]

Before the patch [1], the clock probe was done directly in the
clk-mt8365 driver. In this probe function, the array which stores the
data clocks is sized using the higher defined numbers (*_NR_CLOCK) in
the clock lists [2]. Currently, with the patch [1], the specific
clk-mt8365 probe function is replaced by the mtk generic one [3], which
size the clock data array by adding all the clock descriptor array size
provided by the clk-mt8365 driver.

Actually, all clock indexes come from the header file [2], that mean, if
there are more clock (then more index) in the header file [2] than the
number of clock declared in the clock descriptor arrays (which is the
case currently), the clock data array will be undersized and then the
generic probe function will overflow when it will try to write in
"clk_data[CLK_INDEX]". Actually, instead of crashing at boot, the probe
function returns an error in the log which looks like:
"of_clk_hw_onecell_get: invalid index 135", then this clock isn't
enabled.

Solve this issue by adding in the driver the missing clocks declared in
the header clock file [2].

[1]: Commit ffe91cb28f6a ("clk: mediatek: mt8365: Convert to
     mtk_clk_simple_{probe,remove}()")
[2]: include/dt-bindings/clock/mediatek,mt8365-clk.h
[3]: drivers/clk/mediatek/clk-mtk.c

Fixes: ffe91cb28f6a ("clk: mediatek: mt8365: Convert to mtk_clk_simple_{probe,remove}()")

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/clk/mediatek/clk-mt8365.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8365.c b/drivers/clk/mediatek/clk-mt8365.c
index 6b4e193f648d..35aafeefbf31 100644
--- a/drivers/clk/mediatek/clk-mt8365.c
+++ b/drivers/clk/mediatek/clk-mt8365.c
@@ -23,6 +23,7 @@
 static DEFINE_SPINLOCK(mt8365_clk_lock);
 
 static const struct mtk_fixed_clk top_fixed_clks[] = {
+	FIXED_CLK(CLK_TOP_CLK_NULL, "clk_null", NULL, 0),
 	FIXED_CLK(CLK_TOP_I2S0_BCK, "i2s0_bck", NULL, 26000000),
 	FIXED_CLK(CLK_TOP_DSI0_LNTC_DSICK, "dsi0_lntc_dsick", "clk26m",
 		  75000000),
@@ -559,6 +560,14 @@ static const struct mtk_clk_divider top_adj_divs[] = {
 		  0x324, 16, 8, CLK_DIVIDER_ROUND_CLOSEST),
 	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV3, "apll12_ck_div3", "apll_i2s3_sel",
 		  0x324, 24, 8, CLK_DIVIDER_ROUND_CLOSEST),
+	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV4, "apll12_ck_div4", "apll_tdmout_sel",
+		  0x328, 0, 8, CLK_DIVIDER_ROUND_CLOSEST),
+	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV4B, "apll12_ck_div4b", "apll_tdmout_sel",
+		  0x328, 8, 8, CLK_DIVIDER_ROUND_CLOSEST),
+	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV5, "apll12_ck_div5", "apll_tdmin_sel",
+		  0x328, 16, 8, CLK_DIVIDER_ROUND_CLOSEST),
+	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV5B, "apll12_ck_div5b", "apll_tdmin_sel",
+		  0x328, 24, 8, CLK_DIVIDER_ROUND_CLOSEST),
 	DIV_ADJ_F(CLK_TOP_APLL12_CK_DIV6, "apll12_ck_div6", "apll_spdif_sel",
 		  0x32c, 0, 8, CLK_DIVIDER_ROUND_CLOSEST),
 };
@@ -696,6 +705,7 @@ static const struct mtk_gate ifr_clks[] = {
 	GATE_IFR3(CLK_IFR_GCPU, "ifr_gcpu", "axi_sel", 8),
 	GATE_IFR3(CLK_IFR_TRNG, "ifr_trng", "axi_sel", 9),
 	GATE_IFR3(CLK_IFR_AUXADC, "ifr_auxadc", "clk26m", 10),
+	GATE_IFR3(CLK_IFR_CPUM, "ifr_cpum", "clk26m", 11),
 	GATE_IFR3(CLK_IFR_AUXADC_MD, "ifr_auxadc_md", "clk26m", 14),
 	GATE_IFR3(CLK_IFR_AP_DMA, "ifr_ap_dma", "axi_sel", 18),
 	GATE_IFR3(CLK_IFR_DEBUGSYS, "ifr_debugsys", "axi_sel", 24),
@@ -717,6 +727,7 @@ static const struct mtk_gate ifr_clks[] = {
 	GATE_IFR5(CLK_IFR_PWRAP_TMR, "ifr_pwrap_tmr", "clk26m", 12),
 	GATE_IFR5(CLK_IFR_PWRAP_SPI, "ifr_pwrap_spi", "clk26m", 13),
 	GATE_IFR5(CLK_IFR_PWRAP_SYS, "ifr_pwrap_sys", "clk26m", 14),
+	GATE_IFR5(CLK_IFR_AES_TOP0_BK, "ifr_aes_top0_bk", "clk26m", 16),
 	GATE_IFR5(CLK_IFR_IRRX_26M, "ifr_irrx_26m", "clk26m", 22),
 	GATE_IFR5(CLK_IFR_IRRX_32K, "ifr_irrx_32k", "clk32k", 23),
 	GATE_IFR5(CLK_IFR_I2C0_AXI, "ifr_i2c0_axi", "i2c_sel", 24),

-- 
2.25.1

Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Thu, May 25, 2023 at 04:50:27PM +0200, Alexandre Mergnat wrote:
> The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
> added to the kernel driver, otherwise the CPU just halt and the board is
> rebooted by the wathdog.
> 
> Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
> re-shuffling index and then preserve the ABI.

How does this preserve the ABI exactly? Please describe exactly what you
mean by that.
Also, what about any other users of these definitions, outside of Linux?

Cheers,
Conor

> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  include/dt-bindings/clock/mediatek,mt8365-clk.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/dt-bindings/clock/mediatek,mt8365-clk.h b/include/dt-bindings/clock/mediatek,mt8365-clk.h
> index f9aff1775810..0a841e7cc1c3 100644
> --- a/include/dt-bindings/clock/mediatek,mt8365-clk.h
> +++ b/include/dt-bindings/clock/mediatek,mt8365-clk.h
> @@ -199,7 +199,7 @@
>  #define CLK_IFR_PWRAP_TMR		46
>  #define CLK_IFR_PWRAP_SPI		47
>  #define CLK_IFR_PWRAP_SYS		48
> -#define CLK_IFR_MCU_PM_BK		49
> +#define CLK_IFR_AES_TOP0_BK		49
>  #define CLK_IFR_IRRX_26M		50
>  #define CLK_IFR_IRRX_32K		51
>  #define CLK_IFR_I2C0_AXI		52
> 
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[AngeloGioacchino Del Regno]

Il 25/05/23 16:50, Alexandre Mergnat ha scritto:
> The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
> added to the kernel driver, otherwise the CPU just halt and the board is
> rebooted by the wathdog.
> 
> Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
> re-shuffling index and then preserve the ABI.

It's still a breakage. Besides, have you tried to add it as CLK_IS_CRITICAL? :-)

Cheers,
Angelo

> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   include/dt-bindings/clock/mediatek,mt8365-clk.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/dt-bindings/clock/mediatek,mt8365-clk.h b/include/dt-bindings/clock/mediatek,mt8365-clk.h
> index f9aff1775810..0a841e7cc1c3 100644
> --- a/include/dt-bindings/clock/mediatek,mt8365-clk.h
> +++ b/include/dt-bindings/clock/mediatek,mt8365-clk.h
> @@ -199,7 +199,7 @@
>   #define CLK_IFR_PWRAP_TMR		46
>   #define CLK_IFR_PWRAP_SPI		47
>   #define CLK_IFR_PWRAP_SYS		48
> -#define CLK_IFR_MCU_PM_BK		49
> +#define CLK_IFR_AES_TOP0_BK		49
>   #define CLK_IFR_IRRX_26M		50
>   #define CLK_IFR_IRRX_32K		51
>   #define CLK_IFR_I2C0_AXI		52
> 

Re: [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue

[AngeloGioacchino Del Regno]

Il 25/05/23 16:50, Alexandre Mergnat ha scritto:
> Before the patch [1], the clock probe was done directly in the
> clk-mt8365 driver. In this probe function, the array which stores the
> data clocks is sized using the higher defined numbers (*_NR_CLOCK) in
> the clock lists [2]. Currently, with the patch [1], the specific
> clk-mt8365 probe function is replaced by the mtk generic one [3], which
> size the clock data array by adding all the clock descriptor array size
> provided by the clk-mt8365 driver.
> 
> Actually, all clock indexes come from the header file [2], that mean, if
> there are more clock (then more index) in the header file [2] than the
> number of clock declared in the clock descriptor arrays (which is the
> case currently), the clock data array will be undersized and then the
> generic probe function will overflow when it will try to write in
> "clk_data[CLK_INDEX]". Actually, instead of crashing at boot, the probe
> function returns an error in the log which looks like:
> "of_clk_hw_onecell_get: invalid index 135", then this clock isn't
> enabled.
> 
> Solve this issue by adding in the driver the missing clocks declared in
> the header clock file [2].
> 
> [1]: Commit ffe91cb28f6a ("clk: mediatek: mt8365: Convert to
>       mtk_clk_simple_{probe,remove}()")
> [2]: include/dt-bindings/clock/mediatek,mt8365-clk.h
> [3]: drivers/clk/mediatek/clk-mtk.c
> 
> Fixes: ffe91cb28f6a ("clk: mediatek: mt8365: Convert to mtk_clk_simple_{probe,remove}()")

This is not fixing the conversion, but the clock driver, as it originally missed
clock entries and hence was not compliant with its binding (header).
It worked before, probably, but this doesn't mean that this driver didn't contain
a logic mistake from the beginning :-)

So, add (or replace the current one with) the relevant Fixes tag...

Cheers,
Angelo

Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[Alexandre Mergnat]

On 25/05/2023 19:51, Conor Dooley wrote:
> On Thu, May 25, 2023 at 04:50:27PM +0200, Alexandre Mergnat wrote:
>> The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
>> added to the kernel driver, otherwise the CPU just halt and the board is
>> rebooted by the wathdog.
>>
>> Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
>> re-shuffling index and then preserve the ABI.
> 
> How does this preserve the ABI exactly? Please describe exactly what you
> mean by that.

I mean that reduce the impact of the change compared to the v1 where 
I've changed the index of the following defines to be clean.

> Also, what about any other users of these definitions, outside of Linux?

The clock driver and bindings are only a couple of kernel versions old, 
I'm pretty sure no one is using it. Also, if someone use 
CLK_IFR_MCU_PM_BK define, I'm wondering how his CPU is working since 
Mediatek told me that shouldn't be used, and after some try, I confirm.

I've a question: If something is wrong in the binding, you don't fix it 
to avoid ABI change ?

TBH, I just try to clean the binding. I can fix the driver index issue 
(patch 2/2) without fixing the binding if you prefer. But IMHO, keep an 
unusable define isn't great...

-- 
Regards,
Alexandre

Re: [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue

[Alexandre Mergnat]

On 26/05/2023 10:33, AngeloGioacchino Del Regno wrote:
> Il 25/05/23 16:50, Alexandre Mergnat ha scritto:
>> Before the patch [1], the clock probe was done directly in the
>> clk-mt8365 driver. In this probe function, the array which stores the
>> data clocks is sized using the higher defined numbers (*_NR_CLOCK) in
>> the clock lists [2]. Currently, with the patch [1], the specific
>> clk-mt8365 probe function is replaced by the mtk generic one [3], which
>> size the clock data array by adding all the clock descriptor array size
>> provided by the clk-mt8365 driver.
>>
>> Actually, all clock indexes come from the header file [2], that mean, if
>> there are more clock (then more index) in the header file [2] than the
>> number of clock declared in the clock descriptor arrays (which is the
>> case currently), the clock data array will be undersized and then the
>> generic probe function will overflow when it will try to write in
>> "clk_data[CLK_INDEX]". Actually, instead of crashing at boot, the probe
>> function returns an error in the log which looks like:
>> "of_clk_hw_onecell_get: invalid index 135", then this clock isn't
>> enabled.
>>
>> Solve this issue by adding in the driver the missing clocks declared in
>> the header clock file [2].
>>
>> [1]: Commit ffe91cb28f6a ("clk: mediatek: mt8365: Convert to
>>       mtk_clk_simple_{probe,remove}()")
>> [2]: include/dt-bindings/clock/mediatek,mt8365-clk.h
>> [3]: drivers/clk/mediatek/clk-mtk.c
>>
>> Fixes: ffe91cb28f6a ("clk: mediatek: mt8365: Convert to 
>> mtk_clk_simple_{probe,remove}()")
>
> This is not fixing the conversion, but the clock driver, as it 
> originally missed
> clock entries and hence was not compliant with its binding (header).
> It worked before, probably, but this doesn't mean that this driver 
> didn't contain
> a logic mistake from the beginning :-)
>
> So, add (or replace the current one with) the relevant Fixes tag...
>

Briefly and factually, the mt8365 clk probe mechanism was different

compared to the mtk clk driver. Even if it was an issue or not, it was

working (for sure). When [1] improved the mt8365 clk driver by using

the mtk clk generic probe, some clocks (USB here) no longer worked.

So, IMHO, it still a functional regression introduced by [1], because it

come from the switch of the probe function.


I'm not blaming & shaming the author of [1], as you said, it originally

missed clock entries and hence was not compliant with its binding

(whereas other MTK SoC was I guess). This commit is pointed thanks

to the bisect + test.


-- 
Regards,
Alexandre

Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[Alexandre Mergnat]

On 26/05/2023 10:30, AngeloGioacchino Del Regno wrote:
> Il 25/05/23 16:50, Alexandre Mergnat ha scritto:
>> The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
>> added to the kernel driver, otherwise the CPU just halt and the board is
>> rebooted by the wathdog.
>>
>> Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
>> re-shuffling index and then preserve the ABI.
> 
> It's still a breakage. Besides, have you tried to add it as 
> CLK_IS_CRITICAL? :-)

As I said to Conor, I can fix the driver index issue (patch 2/2) without 
fixing the binding (using CLK_IGNORE_UNUSED but CLK_IS_CRITICAL works too).

-- 
Regards,
Alexandre

Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

On Fri, May 26, 2023 at 10:54:04AM +0200, Alexandre Mergnat wrote:
> On 25/05/2023 19:51, Conor Dooley wrote:
> > On Thu, May 25, 2023 at 04:50:27PM +0200, Alexandre Mergnat wrote:
> > > The “mcu_pm_bclk_ck_cg” clock is used by co-processors and should not be
> > > added to the kernel driver, otherwise the CPU just halt and the board is
> > > rebooted by the wathdog.
> > > 
> > > Instead, add the "aes_top0_bclk_ck_cg" missing clock to prevent
> > > re-shuffling index and then preserve the ABI.
> > 
> > How does this preserve the ABI exactly? Please describe exactly what you
> > mean by that.
> 
> I mean that reduce the impact of the change compared to the v1 where I've
> changed the index of the following defines to be clean.

Oh, you can't do that at all as you probably discovered!

> > Also, what about any other users of these definitions, outside of Linux?
> 
> The clock driver and bindings are only a couple of kernel versions old, I'm
> pretty sure no one is using it.

Pretty sure, or sure?

> Also, if someone use CLK_IFR_MCU_PM_BK
> define, I'm wondering how his CPU is working since Mediatek told me that
> shouldn't be used, and after some try, I confirm.

Maybe that person is actually using the index to make sure that the
clock at that index is left untouched.

> I've a question: If something is wrong in the binding, you don't fix it to
> avoid ABI change ?

I don't quite get what you mean by "wrong". These header files just
define a set of arbitrary meanings, since the clock numbers are really
just something that developers came up with rather than being lifted
from a TRM. They don't prescribe behaviour for each of these clocks, or
that these clocks should actually be used - just a simple "this number
means this clock".
It sounds more like a driver or devicetree is _using_ the number
incorrectly, but that does not make the binding wrong :)

> TBH, I just try to clean the binding. I can fix the driver index issue
> (patch 2/2) without fixing the binding if you prefer. But IMHO, keep an
> unusable define isn't great...

I, at least, would prefer that.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]