Re: [PATCH v3 4/5] clk: stm32: Fix stm32f429 ltdc driver loading hang in clk set rate. keep ltdc clk running after kernel startup

Hi Stephen,

thanks for reviewing.

On Fri, May 15, 2020 at 5:02 AM Stephen Boyd [off-list ref] wrote:

Quoting dillon.minfei@gmail.com (2020-05-12 00:03:36)

quoted

From: dillon min <redacted>

as store stm32f4_rcc_register_pll return to the wrong offset of clks,

Use () on functions, i.e. stm32f4_rcc_register_pll().

ok

quoted

so ltdc gate clk is null. need change clks[PLL_VCO_SAI] to clks[PLL_SAI]

And quote variables like 'clks[PLL_VCO_SAI]'

ok

quoted

add CLK_IGNORE_UNUSED for ltdc to make sure clk not be freed by
clk_disable_unused

clk_disable_unused() doesn't free anything. Why does ltdc not need to be
turned off if it isn't used? Is it critical to system operation? Should
it be marked with the critical clk flag then? The CLK_IGNORE_UNUSED flag
is almost always wrong to use.

Yes, you are right. thanks. CLK_IGNORE_UNUSED just hide the root
cause. after deeper debugging.
i need to drop the last changes , they are not the root cause.

post diff and analyse here first, i will resubmit clk's changes in
next patchset with gyro and ili9341.

--- a/drivers/clk/clk-stm32f4.c
+++ b/drivers/clk/clk-stm32f4.c

@@ -129,8 +129,6 @@ static const struct stm32f4_gate_data

stm32f429_gates[] __initconst = {
        { STM32F4_RCC_APB2ENR, 20,      "spi5",         "apb2_div" },
        { STM32F4_RCC_APB2ENR, 21,      "spi6",         "apb2_div" },
        { STM32F4_RCC_APB2ENR, 22,      "sai1",         "apb2_div" },
-       { STM32F4_RCC_APB2ENR, 26,      "ltdc",         "apb2_div",
-               CLK_IGNORE_UNUSED },
 };

 static const struct stm32f4_gate_data stm32f469_gates[] __initconst = {

@@ -558,13 +556,13 @@ static const struct clk_div_table post_divr_table[] = {

 #define MAX_POST_DIV 3
 static const struct stm32f4_pll_post_div_data  post_div_data[MAX_POST_DIV] = {
-       { CLK_I2SQ_PDIV, PLL_I2S, "plli2s-q-div", "plli2s-q",
+       { CLK_I2SQ_PDIV, PLL_VCO_I2S, "plli2s-q-div", "plli2s-q",
                CLK_SET_RATE_PARENT, STM32F4_RCC_DCKCFGR, 0, 5, 0, NULL},

-       { CLK_SAIQ_PDIV, PLL_SAI, "pllsai-q-div", "pllsai-q",
+       { CLK_SAIQ_PDIV, PLL_VCO_SAI, "pllsai-q-div", "pllsai-q",
                CLK_SET_RATE_PARENT, STM32F4_RCC_DCKCFGR, 8, 5, 0, NULL },

-       { NO_IDX, PLL_SAI, "pllsai-r-div", "pllsai-r", CLK_SET_RATE_PARENT,
+       { NO_IDX, PLL_VCO_SAI, "pllsai-r-div", "pllsai-r", CLK_SET_RATE_PARENT,
                STM32F4_RCC_DCKCFGR, 16, 2, 0, post_divr_table },
 };

@@ -1758,7 +1756,7 @@ static void __init stm32f4_rcc_init(struct

device_node *np)
        clks[PLL_VCO_I2S] = stm32f4_rcc_register_pll("vco_in",
                        &data->pll_data[1], &stm32f4_clk_lock);

-       clks[PLL_SAI] = stm32f4_rcc_register_pll("vco_in",
+       clks[PLL_VCO_SAI] = stm32f4_rcc_register_pll("vco_in",
                        &data->pll_data[2], &stm32f4_clk_lock);

        for (n = 0; n < MAX_POST_DIV; n++) {

issue 1: ili9341 hang in clk set rate, the root cause should be
PLL_VCO_SAI, PLL_SAI mismatch
for 'clks[]'

1, first at stm32f4_rcc_init() ,
    clks[PLL_VCO_SAI] = stm32f4_rcc_register_pll("vco_in",
                        &data->pll_data[2], &stm32f4_clk_lock);
   the clk_hw from stm32f4_rcc_register_pll() is store to 'clks[7]', defined in
   'include/dt-bindings/clock/stm32fx-clock.h'

2, next
hw = clk_register_pll_div(post_div->name,
                                post_div->parent,
                                post_div->flag,
                                base + post_div->offset,
                                post_div->shift,
                                post_div->width,
                                post_div->flag_div,
                                post_div->div_table,
                                clks[post_div->pll_num],
                                &stm32f4_clk_lock);
the 'clks[post_div->pll_num]', the pll_num is PLL_SAI, the value is 2,
defined at
enum {
        PLL,
        PLL_I2S,
        PLL_SAI,
};
'post_div_data[]'

so 7 != 2 offset of 'clks[]', input the wrong 'clks[]' to
clk_register_pll_div. cause to_clk_gate result is null,
crashed in ltdc driver's loading.

issue 2: clk_disable_unused() turn off ltdc clock.
1, ltdc clk is defined in 'stm32f429_gates[]', register to clk core,
but there is no user to use it.
    ltdc driver use dts binding name "lcd", connect to CLK_LCD, then
aux clk 'lcd-tft '
2, as no one use 'stm32f429_gates[]' s ltdc clock , so the
enable_count is zero, after kernel startup
    it's been turn off by clk_disable_unused() is fine.

my chages for this is remove the ltdc from 'stm32f429_gates[]'
but this modification still need 'clk-stm32f4.c''s maintainer to check
if there is side effect.

quoted

Signed-off-by: dillon min <redacted>
---
 drivers/clk/clk-stm32f4.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
index 18117ce..0ba73de 100644
--- a/drivers/clk/clk-stm32f4.c
+++ b/drivers/clk/clk-stm32f4.c

@@ -129,7 +129,8 @@ static const struct stm32f4_gate_data stm32f429_gates[] __initconst = {
        { STM32F4_RCC_APB2ENR, 20,      "spi5",         "apb2_div" },
        { STM32F4_RCC_APB2ENR, 21,      "spi6",         "apb2_div" },
        { STM32F4_RCC_APB2ENR, 22,      "sai1",         "apb2_div" },
-       { STM32F4_RCC_APB2ENR, 26,      "ltdc",         "apb2_div" },
+       { STM32F4_RCC_APB2ENR, 26,      "ltdc",         "apb2_div",
+               CLK_IGNORE_UNUSED },
 };

 static const struct stm32f4_gate_data stm32f469_gates[] __initconst = {

@@ -1757,7 +1758,7 @@ static void __init stm32f4_rcc_init(struct device_node *np)
        clks[PLL_VCO_I2S] = stm32f4_rcc_register_pll("vco_in",
                        &data->pll_data[1], &stm32f4_clk_lock);

-       clks[PLL_VCO_SAI] = stm32f4_rcc_register_pll("vco_in",
+       clks[PLL_SAI] = stm32f4_rcc_register_pll("vco_in",
                        &data->pll_data[2], &stm32f4_clk_lock);

        for (n = 0; n < MAX_POST_DIV; n++) {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel