diff mbox series

[RESEND,v4] clk: stm32f4: fix post divisor setup for I2S/SAI PLLs

Message ID 20210725160725.10788-1-dariobin@libero.it (mailing list archive)
State Accepted, archived
Headers show
Series [RESEND,v4] clk: stm32f4: fix post divisor setup for I2S/SAI PLLs | expand

Commit Message

Dario Binacchi July 25, 2021, 4:07 p.m. UTC 
Enabling the framebuffer leads to a system hang. Running, as a debug
hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
without taking the console_lock, allows to see the crash backtrace on
the serial line.

~ # echo 0 0 > /sys/class/graphics/fb0/pan

[    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
[    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
[    9.733008] Hardware name: STM32 (Device Tree Support)
[    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
[    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
[    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
[    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
[    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
[    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
[    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
[    9.779911] xPSR: 0100000b
[    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
[    9.788791] Hardware name: STM32 (Device Tree Support)
[    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
[    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)

The `pll_num' field in the post_div_data configuration contained a wrong
value which also referenced an uninitialized hardware clock when
clk_register_pll_div() was called.

Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
Signed-off-by: Dario Binacchi <dariobin@libero.it>
Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>

---
I added Gabriel Fernandez's 'Reviewed-by' tag as requested by himself
15 days ago at https://lore.kernel.org/patchwork/patch/1450964/.

Changes in v4:
- Really add Gabriel Fernandez 'Reviewed-by' tag. In version 3 I forgot
  to add the tag.

Changes in v3:
- Add Gabriel Fernandez 'Reviewed-by' tag.

Changes in v2:
- Change  'u8 pll_num' from 'stm32f4_pll_post_div_data' structure into
  'int pll_idx'.

 drivers/clk/clk-stm32f4.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Stephen Boyd July 27, 2021, 12:22 a.m. UTC | #1 
Quoting Dario Binacchi (2021-07-25 09:07:25)
> Enabling the framebuffer leads to a system hang. Running, as a debug
> hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
> without taking the console_lock, allows to see the crash backtrace on
> the serial line.
> 
> ~ # echo 0 0 > /sys/class/graphics/fb0/pan
> 
> [    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
> [    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> [    9.733008] Hardware name: STM32 (Device Tree Support)
> [    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
> [    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
> [    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
> [    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
> [    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
> [    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
> [    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
> [    9.779911] xPSR: 0100000b
> [    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> [    9.788791] Hardware name: STM32 (Device Tree Support)
> [    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
> [    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)
> 
> The `pll_num' field in the post_div_data configuration contained a wrong
> value which also referenced an uninitialized hardware clock when
> clk_register_pll_div() was called.
> 
> Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> 
> ---

Applied to clk-fixes
Dillon Hua July 28, 2021, 10:03 a.m. UTC | #2 
Hi Dario,

I have a similar patch [1] submitted last year.
Unfortunately, it did not get accepted by the maintainer.

Just a reminder here for you, should remove

{ STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },

from stm32{f429, f469, f746, f769}_gates[]; or else
run into white screen after the kernel enters the console.

This patch was verified by Patrice Chotard, you can
find  the history from [2].

Hope you can help to submit a patch to include [2], thanks.

[1]
https://lore.kernel.org/linux-arm-kernel/1590564453-24499-7-git-send-email-dillon.minfei@gmail.com/
https://lore.kernel.org/linux-arm-kernel/1590564453-24499-6-git-send-email-dillon.minfei@gmail.com/

resend this year:
https://lore.kernel.org/lkml/1590378348-8115-6-git-send-email-dillon.minfei@gmail.com/

[2]
https://lore.kernel.org/lkml/6915fa2a-e211-476f-8317-6825e280c322@foss.st.com/

Thanks
Best Regards

Dillon

On Mon, Jul 26, 2021 at 12:08 AM Dario Binacchi <dariobin@libero.it> wrote:
>
> Enabling the framebuffer leads to a system hang. Running, as a debug
> hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
> without taking the console_lock, allows to see the crash backtrace on
> the serial line.
>
> ~ # echo 0 0 > /sys/class/graphics/fb0/pan
>
> [    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
> [    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> [    9.733008] Hardware name: STM32 (Device Tree Support)
> [    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
> [    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
> [    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
> [    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
> [    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
> [    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
> [    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
> [    9.779911] xPSR: 0100000b
> [    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> [    9.788791] Hardware name: STM32 (Device Tree Support)
> [    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
> [    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)
>
> The `pll_num' field in the post_div_data configuration contained a wrong
> value which also referenced an uninitialized hardware clock when
> clk_register_pll_div() was called.
>
> Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>
> ---
> I added Gabriel Fernandez's 'Reviewed-by' tag as requested by himself
> 15 days ago at https://lore.kernel.org/patchwork/patch/1450964/.
>
> Changes in v4:
> - Really add Gabriel Fernandez 'Reviewed-by' tag. In version 3 I forgot
>   to add the tag.
>
> Changes in v3:
> - Add Gabriel Fernandez 'Reviewed-by' tag.
>
> Changes in v2:
> - Change  'u8 pll_num' from 'stm32f4_pll_post_div_data' structure into
>   'int pll_idx'.
>
>  drivers/clk/clk-stm32f4.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> index 18117ce5ff85..5c75e3d906c2 100644
> --- a/drivers/clk/clk-stm32f4.c
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -526,7 +526,7 @@ struct stm32f4_pll {
>
>  struct stm32f4_pll_post_div_data {
>         int idx;
> -       u8 pll_num;
> +       int pll_idx;
>         const char *name;
>         const char *parent;
>         u8 flag;
> @@ -557,13 +557,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 },
>  };
>
> @@ -1774,7 +1774,7 @@ static void __init stm32f4_rcc_init(struct device_node *np)
>                                 post_div->width,
>                                 post_div->flag_div,
>                                 post_div->div_table,
> -                               clks[post_div->pll_num],
> +                               clks[post_div->pll_idx],
>                                 &stm32f4_clk_lock);
>
>                 if (post_div->idx != NO_IDX)
> --
> 2.17.1
>
Dario Binacchi July 29, 2021, 10:03 a.m. UTC | #3 
Hi Dillon,

> Il 28/07/2021 12:03 Dillon Hua <dillonhua@gmail.com> ha scritto:
> 
>  
> Hi Dario,
> 
> I have a similar patch [1] submitted last year.
> Unfortunately, it did not get accepted by the maintainer.
> 
> Just a reminder here for you, should remove
> 
> { STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },

Thank you for your suggestion.
While testing the patch on the stm32f469-disco board I didn't
notice the white screen issue. I'll try to run the tests again. 
I will let you know.

Thanks and regards,
Dario

> 
> from stm32{f429, f469, f746, f769}_gates[]; or else
> run into white screen after the kernel enters the console.
> 
> This patch was verified by Patrice Chotard, you can
> find  the history from [2].
> 
> Hope you can help to submit a patch to include [2], thanks.
> 
> [1]
> https://lore.kernel.org/linux-arm-kernel/1590564453-24499-7-git-send-email-dillon.minfei@gmail.com/
> https://lore.kernel.org/linux-arm-kernel/1590564453-24499-6-git-send-email-dillon.minfei@gmail.com/
> 
> resend this year:
> https://lore.kernel.org/lkml/1590378348-8115-6-git-send-email-dillon.minfei@gmail.com/
> 
> [2]
> https://lore.kernel.org/lkml/6915fa2a-e211-476f-8317-6825e280c322@foss.st.com/
> 
> Thanks
> Best Regards
> 
> Dillon
> 
> On Mon, Jul 26, 2021 at 12:08 AM Dario Binacchi <dariobin@libero.it> wrote:
> >
> > Enabling the framebuffer leads to a system hang. Running, as a debug
> > hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
> > without taking the console_lock, allows to see the crash backtrace on
> > the serial line.
> >
> > ~ # echo 0 0 > /sys/class/graphics/fb0/pan
> >
> > [    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
> > [    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > [    9.733008] Hardware name: STM32 (Device Tree Support)
> > [    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
> > [    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
> > [    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
> > [    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
> > [    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
> > [    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
> > [    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
> > [    9.779911] xPSR: 0100000b
> > [    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > [    9.788791] Hardware name: STM32 (Device Tree Support)
> > [    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
> > [    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)
> >
> > The `pll_num' field in the post_div_data configuration contained a wrong
> > value which also referenced an uninitialized hardware clock when
> > clk_register_pll_div() was called.
> >
> > Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> >
> > ---
> > I added Gabriel Fernandez's 'Reviewed-by' tag as requested by himself
> > 15 days ago at https://lore.kernel.org/patchwork/patch/1450964/.
> >
> > Changes in v4:
> > - Really add Gabriel Fernandez 'Reviewed-by' tag. In version 3 I forgot
> >   to add the tag.
> >
> > Changes in v3:
> > - Add Gabriel Fernandez 'Reviewed-by' tag.
> >
> > Changes in v2:
> > - Change  'u8 pll_num' from 'stm32f4_pll_post_div_data' structure into
> >   'int pll_idx'.
> >
> >  drivers/clk/clk-stm32f4.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> > index 18117ce5ff85..5c75e3d906c2 100644
> > --- a/drivers/clk/clk-stm32f4.c
> > +++ b/drivers/clk/clk-stm32f4.c
> > @@ -526,7 +526,7 @@ struct stm32f4_pll {
> >
> >  struct stm32f4_pll_post_div_data {
> >         int idx;
> > -       u8 pll_num;
> > +       int pll_idx;
> >         const char *name;
> >         const char *parent;
> >         u8 flag;
> > @@ -557,13 +557,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 },
> >  };
> >
> > @@ -1774,7 +1774,7 @@ static void __init stm32f4_rcc_init(struct device_node *np)
> >                                 post_div->width,
> >                                 post_div->flag_div,
> >                                 post_div->div_table,
> > -                               clks[post_div->pll_num],
> > +                               clks[post_div->pll_idx],
> >                                 &stm32f4_clk_lock);
> >
> >                 if (post_div->idx != NO_IDX)
> > --
> > 2.17.1
> >
Stephen Boyd July 30, 2021, 7:47 p.m. UTC | #4 
Quoting Dario Binacchi (2021-07-29 03:03:22)
> Hi Dillon,
> 
> > Il 28/07/2021 12:03 Dillon Hua <dillonhua@gmail.com> ha scritto:
> > 
> >  
> > Hi Dario,
> > 
> > I have a similar patch [1] submitted last year.
> > Unfortunately, it did not get accepted by the maintainer.
> > 
> > Just a reminder here for you, should remove
> > 
> > { STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },
> 
> Thank you for your suggestion.
> While testing the patch on the stm32f469-disco board I didn't
> notice the white screen issue. I'll try to run the tests again. 
> I will let you know.
> 

Ok, I'll drop this from the tree.
Dario Binacchi July 30, 2021, 9:22 p.m. UTC | #5 
Hi Stephen,

> Il 30/07/2021 21:47 Stephen Boyd <sboyd@kernel.org> ha scritto:
> 
>  
> Quoting Dario Binacchi (2021-07-29 03:03:22)
> > Hi Dillon,
> > 
> > > Il 28/07/2021 12:03 Dillon Hua <dillonhua@gmail.com> ha scritto:
> > > 
> > >  
> > > Hi Dario,
> > > 
> > > I have a similar patch [1] submitted last year.
> > > Unfortunately, it did not get accepted by the maintainer.
> > > 
> > > Just a reminder here for you, should remove
> > > 
> > > { STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },
> > 
> > Thank you for your suggestion.
> > While testing the patch on the stm32f469-disco board I didn't
> > notice the white screen issue. I'll try to run the tests again. 
> > I will let you know.
> > 
> 
> Ok, I'll drop this from the tree.

The patch fixes commit 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs"),
so IMHO it should not be dropped from the tree.
What Dillon suggests, if confirmed, should be fixed by another patch.

Thanks and regards,
Dario
Dillon Hua Aug. 1, 2021, 2:56 p.m. UTC | #6 
Hi Dario

On Thu, Jul 29, 2021 at 6:03 PM Dario Binacchi <dariobin@libero.it> wrote:
>
> Hi Dillon,
>
> > Il 28/07/2021 12:03 Dillon Hua <dillonhua@gmail.com> ha scritto:
> >
> >
> > Hi Dario,
> >
> > I have a similar patch [1] submitted last year.
> > Unfortunately, it did not get accepted by the maintainer.
> >
> > Just a reminder here for you, should remove
> >
> > { STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },
>
> Thank you for your suggestion.
> While testing the patch on the stm32f469-disco board I didn't
> notice the white screen issue. I'll try to run the tests again.
> I will let you know.

Thanks, be sure clk_ignore_unused is not set from the kernel cmdline
at your test.

Best Regards
Dillon

>
> Thanks and regards,
> Dario
>
> >
> > from stm32{f429, f469, f746, f769}_gates[]; or else
> > run into white screen after the kernel enters the console.
> >
> > This patch was verified by Patrice Chotard, you can
> > find  the history from [2].
> >
> > Hope you can help to submit a patch to include [2], thanks.
> >
> > [1]
> > https://lore.kernel.org/linux-arm-kernel/1590564453-24499-7-git-send-email-dillon.minfei@gmail.com/
> > https://lore.kernel.org/linux-arm-kernel/1590564453-24499-6-git-send-email-dillon.minfei@gmail.com/
> >
> > resend this year:
> > https://lore.kernel.org/lkml/1590378348-8115-6-git-send-email-dillon.minfei@gmail.com/
> >
> > [2]
> > https://lore.kernel.org/lkml/6915fa2a-e211-476f-8317-6825e280c322@foss.st.com/
> >
> > Thanks
> > Best Regards
> >
> > Dillon
> >
> > On Mon, Jul 26, 2021 at 12:08 AM Dario Binacchi <dariobin@libero.it> wrote:
> > >
> > > Enabling the framebuffer leads to a system hang. Running, as a debug
> > > hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
> > > without taking the console_lock, allows to see the crash backtrace on
> > > the serial line.
> > >
> > > ~ # echo 0 0 > /sys/class/graphics/fb0/pan
> > >
> > > [    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
> > > [    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > > [    9.733008] Hardware name: STM32 (Device Tree Support)
> > > [    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
> > > [    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
> > > [    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
> > > [    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
> > > [    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
> > > [    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
> > > [    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
> > > [    9.779911] xPSR: 0100000b
> > > [    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > > [    9.788791] Hardware name: STM32 (Device Tree Support)
> > > [    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
> > > [    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)
> > >
> > > The `pll_num' field in the post_div_data configuration contained a wrong
> > > value which also referenced an uninitialized hardware clock when
> > > clk_register_pll_div() was called.
> > >
> > > Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
> > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> > >
> > > ---
> > > I added Gabriel Fernandez's 'Reviewed-by' tag as requested by himself
> > > 15 days ago at https://lore.kernel.org/patchwork/patch/1450964/.
> > >
> > > Changes in v4:
> > > - Really add Gabriel Fernandez 'Reviewed-by' tag. In version 3 I forgot
> > >   to add the tag.
> > >
> > > Changes in v3:
> > > - Add Gabriel Fernandez 'Reviewed-by' tag.
> > >
> > > Changes in v2:
> > > - Change  'u8 pll_num' from 'stm32f4_pll_post_div_data' structure into
> > >   'int pll_idx'.
> > >
> > >  drivers/clk/clk-stm32f4.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> > > index 18117ce5ff85..5c75e3d906c2 100644
> > > --- a/drivers/clk/clk-stm32f4.c
> > > +++ b/drivers/clk/clk-stm32f4.c
> > > @@ -526,7 +526,7 @@ struct stm32f4_pll {
> > >
> > >  struct stm32f4_pll_post_div_data {
> > >         int idx;
> > > -       u8 pll_num;
> > > +       int pll_idx;
> > >         const char *name;
> > >         const char *parent;
> > >         u8 flag;
> > > @@ -557,13 +557,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 },
> > >  };
> > >
> > > @@ -1774,7 +1774,7 @@ static void __init stm32f4_rcc_init(struct device_node *np)
> > >                                 post_div->width,
> > >                                 post_div->flag_div,
> > >                                 post_div->div_table,
> > > -                               clks[post_div->pll_num],
> > > +                               clks[post_div->pll_idx],
> > >                                 &stm32f4_clk_lock);
> > >
> > >                 if (post_div->idx != NO_IDX)
> > > --
> > > 2.17.1
> > >
Dario Binacchi Aug. 1, 2021, 7:37 p.m. UTC | #7 
Hi Dillon and Stephen,

> Il 01/08/2021 16:56 Dillon Hua <dillonhua@gmail.com> ha scritto:
> 
>  
> Hi Dario
> 
> On Thu, Jul 29, 2021 at 6:03 PM Dario Binacchi <dariobin@libero.it> wrote:
> >
> > Hi Dillon,
> >
> > > Il 28/07/2021 12:03 Dillon Hua <dillonhua@gmail.com> ha scritto:
> > >
> > >
> > > Hi Dario,
> > >
> > > I have a similar patch [1] submitted last year.
> > > Unfortunately, it did not get accepted by the maintainer.
> > >
> > > Just a reminder here for you, should remove
> > >
> > > { STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div" },
> >
> > Thank you for your suggestion.
> > While testing the patch on the stm32f469-disco board I didn't
> > notice the white screen issue. I'll try to run the tests again.
> > I will let you know.
> 
> Thanks, be sure clk_ignore_unused is not set from the kernel cmdline
> at your test.

This is the kernel command line of my tests:
console=ttySTM0,115200 root=/dev/mmcblk0p2 rw rootfstype=ext2 rootwait earlyprintk consoleblank=0 ignore_loglevel

In previous emails I forgot to mention that the patch was in turn applied to a buildroot patch [1] and
acked by Christophe Priouzeau of ST microelectronics. However, I rerun the tests and did not notice
anything strange.

I then enabled CONFIG_VT, CONFIG_FRAMEBUFFER_CONSOLE and CONFIG_LOGO, and indedd the command
'echo Dario >/dev/tty0' prints my name on the display only by removing
{STM32F4_RCC_APB2ENR, 26, "ltdc", "apb2_div"}.

As said in a previous email to Stephen, the patch I submitted fixes commit 517633ef630e
("clk: stm32f4: Add post divisor for I2S & SAI PLLs"), so IMHO it should not be dropped from the tree.
What Dillon suggested should instead be fixed by another patch.

Waiting for your opinion,
Thanks and regards,
Dario

[1] https://patchwork.ozlabs.org/project/buildroot/patch/20210716164243.17988-1-dariobin@libero.it/


> 
> Best Regards
> Dillon
> 
> >
> > Thanks and regards,
> > Dario
> >
> > >
> > > from stm32{f429, f469, f746, f769}_gates[]; or else
> > > run into white screen after the kernel enters the console.
> > >
> > > This patch was verified by Patrice Chotard, you can
> > > find  the history from [2].
> > >
> > > Hope you can help to submit a patch to include [2], thanks.
> > >
> > > [1]
> > > https://lore.kernel.org/linux-arm-kernel/1590564453-24499-7-git-send-email-dillon.minfei@gmail.com/
> > > https://lore.kernel.org/linux-arm-kernel/1590564453-24499-6-git-send-email-dillon.minfei@gmail.com/
> > >
> > > resend this year:
> > > https://lore.kernel.org/lkml/1590378348-8115-6-git-send-email-dillon.minfei@gmail.com/
> > >
> > > [2]
> > > https://lore.kernel.org/lkml/6915fa2a-e211-476f-8317-6825e280c322@foss.st.com/
> > >
> > > Thanks
> > > Best Regards
> > >
> > > Dillon
> > >
> > > On Mon, Jul 26, 2021 at 12:08 AM Dario Binacchi <dariobin@libero.it> wrote:
> > > >
> > > > Enabling the framebuffer leads to a system hang. Running, as a debug
> > > > hack, the store_pan() function in drivers/video/fbdev/core/fbsysfs.c
> > > > without taking the console_lock, allows to see the crash backtrace on
> > > > the serial line.
> > > >
> > > > ~ # echo 0 0 > /sys/class/graphics/fb0/pan
> > > >
> > > > [    9.719414] Unhandled exception: IPSR = 00000005 LR = fffffff1
> > > > [    9.726937] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > > > [    9.733008] Hardware name: STM32 (Device Tree Support)
> > > > [    9.738296] PC is at clk_gate_is_enabled+0x0/0x28
> > > > [    9.743426] LR is at stm32f4_pll_div_set_rate+0xf/0x38
> > > > [    9.748857] pc : [<0011e4be>]    lr : [<0011f9e3>]    psr: 0100000b
> > > > [    9.755373] sp : 00bc7be0  ip : 00000000  fp : 001f3ac4
> > > > [    9.760812] r10: 002610d0  r9 : 01efe920  r8 : 00540560
> > > > [    9.766269] r7 : 02e7ddb0  r6 : 0173eed8  r5 : 00000000  r4 : 004027c0
> > > > [    9.773081] r3 : 0011e4bf  r2 : 02e7ddb0  r1 : 0173eed8  r0 : 1d3267b8
> > > > [    9.779911] xPSR: 0100000b
> > > > [    9.782719] CPU: 0 PID: 49 Comm: sh Not tainted 5.13.0-rc5 #9
> > > > [    9.788791] Hardware name: STM32 (Device Tree Support)
> > > > [    9.794120] [<0000afa1>] (unwind_backtrace) from [<0000a33f>] (show_stack+0xb/0xc)
> > > > [    9.802421] [<0000a33f>] (show_stack) from [<0000a8df>] (__invalid_entry+0x4b/0x4c)
> > > >
> > > > The `pll_num' field in the post_div_data configuration contained a wrong
> > > > value which also referenced an uninitialized hardware clock when
> > > > clk_register_pll_div() was called.
> > > >
> > > > Fixes: 517633ef630e ("clk: stm32f4: Add post divisor for I2S & SAI PLLs")
> > > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > > Reviewed-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> > > >
> > > > ---
> > > > I added Gabriel Fernandez's 'Reviewed-by' tag as requested by himself
> > > > 15 days ago at https://lore.kernel.org/patchwork/patch/1450964/.
> > > >
> > > > Changes in v4:
> > > > - Really add Gabriel Fernandez 'Reviewed-by' tag. In version 3 I forgot
> > > >   to add the tag.
> > > >
> > > > Changes in v3:
> > > > - Add Gabriel Fernandez 'Reviewed-by' tag.
> > > >
> > > > Changes in v2:
> > > > - Change  'u8 pll_num' from 'stm32f4_pll_post_div_data' structure into
> > > >   'int pll_idx'.
> > > >
> > > >  drivers/clk/clk-stm32f4.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> > > > index 18117ce5ff85..5c75e3d906c2 100644
> > > > --- a/drivers/clk/clk-stm32f4.c
> > > > +++ b/drivers/clk/clk-stm32f4.c
> > > > @@ -526,7 +526,7 @@ struct stm32f4_pll {
> > > >
> > > >  struct stm32f4_pll_post_div_data {
> > > >         int idx;
> > > > -       u8 pll_num;
> > > > +       int pll_idx;
> > > >         const char *name;
> > > >         const char *parent;
> > > >         u8 flag;
> > > > @@ -557,13 +557,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 },
> > > >  };
> > > >
> > > > @@ -1774,7 +1774,7 @@ static void __init stm32f4_rcc_init(struct device_node *np)
> > > >                                 post_div->width,
> > > >                                 post_div->flag_div,
> > > >                                 post_div->div_table,
> > > > -                               clks[post_div->pll_num],
> > > > +                               clks[post_div->pll_idx],
> > > >                                 &stm32f4_clk_lock);
> > > >
> > > >                 if (post_div->idx != NO_IDX)
> > > > --
> > > > 2.17.1
> > > >
Stephen Boyd Aug. 2, 2021, 10:09 p.m. UTC | #8 
Quoting Dario Binacchi (2021-08-01 12:37:10)
> 
> As said in a previous email to Stephen, the patch I submitted fixes commit 517633ef630e
> ("clk: stm32f4: Add post divisor for I2S & SAI PLLs"), so IMHO it should not be dropped from the tree.
> What Dillon suggested should instead be fixed by another patch.

Ok. I'll send off the fix to Linus today.
diff mbox series

Patch

diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
index 18117ce5ff85..5c75e3d906c2 100644
--- a/drivers/clk/clk-stm32f4.c
+++ b/drivers/clk/clk-stm32f4.c
@@ -526,7 +526,7 @@  struct stm32f4_pll {
 
 struct stm32f4_pll_post_div_data {
 	int idx;
-	u8 pll_num;
+	int pll_idx;
 	const char *name;
 	const char *parent;
 	u8 flag;
@@ -557,13 +557,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 },
 };
 
@@ -1774,7 +1774,7 @@  static void __init stm32f4_rcc_init(struct device_node *np)
 				post_div->width,
 				post_div->flag_div,
 				post_div->div_table,
-				clks[post_div->pll_num],
+				clks[post_div->pll_idx],
 				&stm32f4_clk_lock);
 
 		if (post_div->idx != NO_IDX)