* [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration
@ 2025-04-27 13:44 Ahmad Fatoum
2025-05-06 7:51 ` Sascha Hauer
0 siblings, 1 reply; 4+ messages in thread
From: Ahmad Fatoum @ 2025-04-27 13:44 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
The precision improvement in Commit af3d99396a8b ("clk: imx: improve
precision of AV PLL to 1 Hz") broke the LVDS boot splash on the
imx6q-skov-reve-mi1010ait-1cp1 board, because recalculating the clock
rates broke the 7-fold relation between 7MHz pixel clock and the serial
clock.
Before:
pll5_video (rate 980041992
pll5_post_div (rate 490020996
pll5_video_div (rate 490020996
ldb_di0_sel (rate 490020996 <-- 49MHz / 7 = 7 MHz
ldb_di0_div_3_5 (rate 140005998 |
ldb_di0_podf (rate 70002999 ≈
ipu1_di0_sel (rate 70002999 |
ipu1_di0 (rate 70002999 v
2400000.ipu@2400000.of_di0_pixel (rate 70002992
After:
pll5_video (rate 980042001
pll5_post_div (rate 980042001
pll5_video_div (rate 980042001
ldb_di0_sel (rate 980042001 <-- 98MHz / 7 = 14 MHz
ldb_di0_div_3_5 (rate 280012000 |
ldb_di0_podf (rate 140006000 ≉
ipu1_di0_sel (rate 140006000 |
ipu1_di0 (rate 140006000 v
2400000.ipu@2400000.of_di0_pixel (rate 70002992
By adding an explicit clk_set_rate to the 7-fold frequency before
setting the pixel clock, we restore the ratio again:
pll5_video (rate 980042000
pll5_post_div (rate 980042000
pll5_video_div (rate 490021000
ldb_di0_sel (rate 490021000 <-- 49MHz / 7 = 7 MHz
ldb_di0_div_3_5 (rate 140006000 |
ldb_di0_podf (rate 70003000 ≈
ipu1_di0_sel (rate 70003000 |
ipu1_di0 (rate 70003000 v
2400000.ipu@2400000.of_di0_pixel (rate 70002992
Fixes: af3d99396a8b ("clk: imx: improve precision of AV PLL to 1 Hz")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Philipp, I wasn't completely sure about whether the dual frequency
calculation is correct, can you take a look?
---
drivers/video/imx-ipu-v3/imx-ldb.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/video/imx-ipu-v3/imx-ldb.c b/drivers/video/imx-ipu-v3/imx-ldb.c
index ae7d3548267a..8b9f6d00f6cb 100644
--- a/drivers/video/imx-ipu-v3/imx-ldb.c
+++ b/drivers/video/imx-ipu-v3/imx-ldb.c
@@ -138,9 +138,10 @@ static int imx_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, struct fb_videomo
return 0;
}
-static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno, unsigned long pixclk)
+static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno,
+ unsigned long serial_clk, unsigned long di_clk)
{
- struct clk *diclk, *ldbclk;
+ struct clk *diclk, *ldbclk, *ldbdiclk;
char *clkname;
int ret;
@@ -160,14 +161,24 @@ static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno, u
return PTR_ERR(ldbclk);
}
+ clkname = basprintf("ldb_di%d_sel", chno);
+ ldbdiclk = clk_lookup(clkname);
+ free(clkname);
+ if (IS_ERR(ldbdiclk)) {
+ dev_err(ldb->dev, "failed to get ldb di clk: %pe\n", ldbdiclk);
+ return PTR_ERR(ldbdiclk);
+ }
+
ret = clk_set_parent(diclk, ldbclk);
if (ret) {
dev_err(ldb->dev, "failed to set display clock parent: %pe\n", ERR_PTR(ret));
return ret;
}
- clk_set_rate(clk_get_parent(ldbclk), pixclk);
- clk_set_rate(ldbclk, pixclk);
+ clk_set_rate(ldbdiclk, serial_clk);
+
+ clk_set_rate(clk_get_parent(ldbclk), di_clk);
+ clk_set_rate(ldbclk, di_clk);
return 0;
}
@@ -189,10 +200,10 @@ static int imx6q_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, int di,
pixclk *= 2;
if (dual) {
- imx6q_set_clock(ldb, ipuno, dino, 0, pixclk);
- imx6q_set_clock(ldb, ipuno, dino, 1, pixclk);
+ imx6q_set_clock(ldb, ipuno, dino, 0, pixclk * 7, pixclk);
+ imx6q_set_clock(ldb, ipuno, dino, 1, pixclk * 7, pixclk);
} else {
- imx6q_set_clock(ldb, ipuno, dino, imx_ldb_ch->chno, pixclk);
+ imx6q_set_clock(ldb, ipuno, dino, imx_ldb_ch->chno, pixclk * 7, pixclk);
}
val = readl(gpr3);
--
2.39.5
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration
2025-04-27 13:44 [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration Ahmad Fatoum
@ 2025-05-06 7:51 ` Sascha Hauer
2025-05-07 7:25 ` Ahmad Fatoum
0 siblings, 1 reply; 4+ messages in thread
From: Sascha Hauer @ 2025-05-06 7:51 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Sun, Apr 27, 2025 at 03:44:53PM +0200, Ahmad Fatoum wrote:
> The precision improvement in Commit af3d99396a8b ("clk: imx: improve
> precision of AV PLL to 1 Hz") broke the LVDS boot splash on the
> imx6q-skov-reve-mi1010ait-1cp1 board, because recalculating the clock
> rates broke the 7-fold relation between 7MHz pixel clock and the serial
> clock.
>
> Before:
>
> pll5_video (rate 980041992
> pll5_post_div (rate 490020996
> pll5_video_div (rate 490020996
> ldb_di0_sel (rate 490020996 <-- 49MHz / 7 = 7 MHz
> ldb_di0_div_3_5 (rate 140005998 |
> ldb_di0_podf (rate 70002999 ≈
> ipu1_di0_sel (rate 70002999 |
> ipu1_di0 (rate 70002999 v
> 2400000.ipu@2400000.of_di0_pixel (rate 70002992
>
> After:
>
> pll5_video (rate 980042001
> pll5_post_div (rate 980042001
> pll5_video_div (rate 980042001
> ldb_di0_sel (rate 980042001 <-- 98MHz / 7 = 14 MHz
> ldb_di0_div_3_5 (rate 280012000 |
> ldb_di0_podf (rate 140006000 ≉
> ipu1_di0_sel (rate 140006000 |
> ipu1_di0 (rate 140006000 v
> 2400000.ipu@2400000.of_di0_pixel (rate 70002992
>
> By adding an explicit clk_set_rate to the 7-fold frequency before
> setting the pixel clock, we restore the ratio again:
>
> pll5_video (rate 980042000
> pll5_post_div (rate 980042000
> pll5_video_div (rate 490021000
> ldb_di0_sel (rate 490021000 <-- 49MHz / 7 = 7 MHz
> ldb_di0_div_3_5 (rate 140006000 |
> ldb_di0_podf (rate 70003000 ≈
> ipu1_di0_sel (rate 70003000 |
> ipu1_di0 (rate 70003000 v
> 2400000.ipu@2400000.of_di0_pixel (rate 70002992
>
> Fixes: af3d99396a8b ("clk: imx: improve precision of AV PLL to 1 Hz")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>
> Philipp, I wasn't completely sure about whether the dual frequency
> calculation is correct, can you take a look?
> ---
> drivers/video/imx-ipu-v3/imx-ldb.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/imx-ipu-v3/imx-ldb.c b/drivers/video/imx-ipu-v3/imx-ldb.c
> index ae7d3548267a..8b9f6d00f6cb 100644
> --- a/drivers/video/imx-ipu-v3/imx-ldb.c
> +++ b/drivers/video/imx-ipu-v3/imx-ldb.c
> @@ -138,9 +138,10 @@ static int imx_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, struct fb_videomo
> return 0;
> }
>
> -static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno, unsigned long pixclk)
> +static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno,
> + unsigned long serial_clk, unsigned long di_clk)
> {
> - struct clk *diclk, *ldbclk;
> + struct clk *diclk, *ldbclk, *ldbdiclk;
> char *clkname;
> int ret;
>
> @@ -160,14 +161,24 @@ static int imx6q_set_clock(struct imx_ldb *ldb, int ipuno, int dino, int chno, u
> return PTR_ERR(ldbclk);
> }
>
> + clkname = basprintf("ldb_di%d_sel", chno);
> + ldbdiclk = clk_lookup(clkname);
> + free(clkname);
> + if (IS_ERR(ldbdiclk)) {
> + dev_err(ldb->dev, "failed to get ldb di clk: %pe\n", ldbdiclk);
> + return PTR_ERR(ldbdiclk);
> + }
> +
> ret = clk_set_parent(diclk, ldbclk);
> if (ret) {
> dev_err(ldb->dev, "failed to set display clock parent: %pe\n", ERR_PTR(ret));
> return ret;
> }
>
> - clk_set_rate(clk_get_parent(ldbclk), pixclk);
> - clk_set_rate(ldbclk, pixclk);
> + clk_set_rate(ldbdiclk, serial_clk);
> +
> + clk_set_rate(clk_get_parent(ldbclk), di_clk);
> + clk_set_rate(ldbclk, di_clk);
>
> return 0;
> }
> @@ -189,10 +200,10 @@ static int imx6q_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, int di,
> pixclk *= 2;
>
> if (dual) {
> - imx6q_set_clock(ldb, ipuno, dino, 0, pixclk);
> - imx6q_set_clock(ldb, ipuno, dino, 1, pixclk);
> + imx6q_set_clock(ldb, ipuno, dino, 0, pixclk * 7, pixclk);
> + imx6q_set_clock(ldb, ipuno, dino, 1, pixclk * 7, pixclk);
I think in dual channel mode we have to maintain a 1:3.5 setting instead
of 1:7.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration
2025-05-06 7:51 ` Sascha Hauer
@ 2025-05-07 7:25 ` Ahmad Fatoum
2025-05-07 8:22 ` Philipp Zabel
0 siblings, 1 reply; 4+ messages in thread
From: Ahmad Fatoum @ 2025-05-07 7:25 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox, pza
Hello Sascha,
On 5/6/25 09:51, Sascha Hauer wrote:
> On Sun, Apr 27, 2025 at 03:44:53PM +0200, Ahmad Fatoum wrote:
>> @@ -189,10 +200,10 @@ static int imx6q_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, int di,
>> pixclk *= 2;
This multiplication by 2 is only done for the single channel mode.
>>
>> if (dual) {
>> - imx6q_set_clock(ldb, ipuno, dino, 0, pixclk);
>> - imx6q_set_clock(ldb, ipuno, dino, 1, pixclk);
>> + imx6q_set_clock(ldb, ipuno, dino, 0, pixclk * 7, pixclk);
>> + imx6q_set_clock(ldb, ipuno, dino, 1, pixclk * 7, pixclk);
>
> I think in dual channel mode we have to maintain a 1:3.5 setting instead
> of 1:7.
That means pixclk for dual channel mode here is already half the value
it would be for single channel, i,e. 1:3.5.
Thanks,
Ahmad
>
> Sascha
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration
2025-05-07 7:25 ` Ahmad Fatoum
@ 2025-05-07 8:22 ` Philipp Zabel
0 siblings, 0 replies; 4+ messages in thread
From: Philipp Zabel @ 2025-05-07 8:22 UTC (permalink / raw)
To: Ahmad Fatoum, Sascha Hauer; +Cc: barebox, pza
On Mi, 2025-05-07 at 09:25 +0200, Ahmad Fatoum wrote:
> Hello Sascha,
>
> On 5/6/25 09:51, Sascha Hauer wrote:
> > On Sun, Apr 27, 2025 at 03:44:53PM +0200, Ahmad Fatoum wrote:
>
> > > @@ -189,10 +200,10 @@ static int imx6q_ldb_prepare(struct imx_ldb_channel *imx_ldb_ch, int di,
> > > pixclk *= 2;
>
> This multiplication by 2 is only done for the single channel mode.
TBH, I don't quite understand this. In the kernel, di_clk is calculated
as drm_display_mode->clock * 1000 in either mode.
Does this mean that the pixclk parameter passed to imx6q_ldb_prepare
was 3.5 MHz here, resulting in the 7 MHz rate that ldb_di0_podf is set
to after this multiplication?
> > >
> > > if (dual) {
> > > - imx6q_set_clock(ldb, ipuno, dino, 0, pixclk);
> > > - imx6q_set_clock(ldb, ipuno, dino, 1, pixclk);
> > > + imx6q_set_clock(ldb, ipuno, dino, 0, pixclk * 7, pixclk);
> > > + imx6q_set_clock(ldb, ipuno, dino, 1, pixclk * 7, pixclk);
The di_clk:serial_clk ratio is 1:7 here, regardless of what happened to
pixclk before.
> > I think in dual channel mode we have to maintain a 1:3.5 setting instead
> > of 1:7.
>
> That means pixclk for dual channel mode here is already half the value
> it would be for single channel, i,e. 1:3.5.
In dual mode, the ipu_di clock has to run at double the serial_clk/7
rate because one IPU DI feeds two LDB channels. That's why the ratio
between ldb_di0/1_sel and ipu1_di0_sel must be set to 1:3.5, and so
that's what the ratio between imx6q_set_clock()'s serial_clk and di_clk
parameters should be as well.
regards
Philipp
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-07 9:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-27 13:44 [PATCH master] video: IPUv3-LDB: fix LVDS serial clock configuration Ahmad Fatoum
2025-05-06 7:51 ` Sascha Hauer
2025-05-07 7:25 ` Ahmad Fatoum
2025-05-07 8:22 ` Philipp Zabel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox