diff mbox series

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

Message ID 20230517-fix-clk-index-v2-1-1b686cefcb7e@baylibre.com
State New
Headers
Series Fix and clean MT8365 clock indexes |

Commit Message

Alexandre Mergnat May 25, 2023, 2:50 p.m. UTC 
  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(-)

Comments

Conor Dooley May 25, 2023, 5:51 p.m. UTC | #1 
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
>
AngeloGioacchino Del Regno May 26, 2023, 8:30 a.m. UTC | #2 
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
>
Alexandre Mergnat May 26, 2023, 8:54 a.m. UTC | #3 
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...
Alexandre Mergnat May 26, 2023, 9:46 a.m. UTC | #4 
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).
Conor Dooley May 26, 2023, 11:39 a.m. UTC | #5 
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.
diff mbox series

Patch

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