| Message ID | 20230517-fix-clk-index-v2-1-1b686cefcb7e@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix and clean MT8365 clock indexes | expand |
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 >
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 >
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...
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).
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 --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
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(-)