[v6,11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI

Submitted by Jagan Teki on Jan. 24, 2019, 7:58 p.m.

Details

Message ID 20190124195900.22620-12-jagan@amarulasolutions.com
State Superseded
Headers show
Series "drm/sun4i: Allwinner A64 MIPI-DSI support" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jagan Teki Jan. 24, 2019, 7:58 p.m.
Minimum PLL used for MIPI is 500MHz, as per manual, but
lowering the min rate by 300MHz can result proper working
nkms divider with the help of desired dclock rate from
panel driver.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 932836d26e2b..296d489aad6e 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -183,6 +183,7 @@  static struct ccu_nkm pll_mipi_clk = {
 	.n		= _SUNXI_CCU_MULT(8, 4),
 	.k		= _SUNXI_CCU_MULT_MIN(4, 2, 2),
 	.m		= _SUNXI_CCU_DIV(0, 4),
+	.min_rate	= 300000000,
 	.common		= {
 		.reg		= 0x040,
 		.hw.init	= CLK_HW_INIT("pll-mipi", "pll-video0",

Comments

On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> Minimum PLL used for MIPI is 500MHz, as per manual, but
> lowering the min rate by 300MHz can result proper working
> nkms divider with the help of desired dclock rate from
> panel driver.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Acked-by: Stephen Boyd <sboyd@kernel.org>

Going 200MHz below the minimum doesn't seem really reasonable. What
is the issue that you are trying to fix here?

It looks like it's picking bad dividers, but if that's the case, this
isn't the proper fix.

Maxime
On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > lowering the min rate by 300MHz can result proper working
> > nkms divider with the help of desired dclock rate from
> > panel driver.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Acked-by: Stephen Boyd <sboyd@kernel.org>
>
> Going 200MHz below the minimum doesn't seem really reasonable. What
> is the issue that you are trying to fix here?
>
> It looks like it's picking bad dividers, but if that's the case, this
> isn't the proper fix.

As I stated in earlier patches, the whole idea is pick the desired
dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
is unable to get the proper dclk divider at the end, so it eventually
picking up wrong divider value and fired vblank timeout.

So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
get the desired clock something like below.
[    2.415773] [drm] No driver support for vblank timestamp query.
[    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
[    2.424172] ideal = 220000000, rounded = 0
[    2.424176] ideal = 275000000, rounded = 0
[    2.424194] ccu_nkm_round_rate: rate = 330000000
[    2.424197] ideal = 330000000, rounded = 330000000
[    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
[    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
[    2.424209] ideal = 220000000, rounded = 0
[    2.424213] ideal = 275000000, rounded = 0
[    2.424230] ccu_nkm_round_rate: rate = 330000000
[    2.424233] ideal = 330000000, rounded = 330000000
[    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
[    2.424253] ccu_nkm_round_rate: rate = 330000000
[    2.424270] ccu_nkm_round_rate: rate = 330000000
[    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
[    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
[    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
[    2.424309] ccu_nkm_set_rate: _nkm.n = 5
[    2.424311] ccu_nkm_set_rate: _nkm.k = 2
[    2.424313] ccu_nkm_set_rate: _nkm.m = 9
[    2.424661] sun4i_dclk_set_rate div 6
[    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000

But look like this wouldn't valid for all other dclock rates, say BPI
panel has 30MHz clock that would failed with this logic.

On the other side Allwinner BSP calculating dclk divider based on the
SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
calculated based on the bpp/lanes.

Since the above min_rate is not desired for possible panels clock, I
think we can rely on BSP to make a move here. I'm planning to do these
changes in next version.

Let me know if you have any comments here?

[1] https://patchwork.kernel.org/patch/10777519/
On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > lowering the min rate by 300MHz can result proper working
> > > nkms divider with the help of desired dclock rate from
> > > panel driver.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> >
> > Going 200MHz below the minimum doesn't seem really reasonable. What
> > is the issue that you are trying to fix here?
> >
> > It looks like it's picking bad dividers, but if that's the case, this
> > isn't the proper fix.
> 
> As I stated in earlier patches, the whole idea is pick the desired
> dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> is unable to get the proper dclk divider at the end, so it eventually
> picking up wrong divider value and fired vblank timeout.
> 
> So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> get the desired clock something like below.
> [    2.415773] [drm] No driver support for vblank timestamp query.
> [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> [    2.424172] ideal = 220000000, rounded = 0
> [    2.424176] ideal = 275000000, rounded = 0
> [    2.424194] ccu_nkm_round_rate: rate = 330000000
> [    2.424197] ideal = 330000000, rounded = 330000000
> [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> [    2.424209] ideal = 220000000, rounded = 0
> [    2.424213] ideal = 275000000, rounded = 0
> [    2.424230] ccu_nkm_round_rate: rate = 330000000
> [    2.424233] ideal = 330000000, rounded = 330000000
> [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> [    2.424253] ccu_nkm_round_rate: rate = 330000000
> [    2.424270] ccu_nkm_round_rate: rate = 330000000
> [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> [    2.424661] sun4i_dclk_set_rate div 6
> [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> 
> But look like this wouldn't valid for all other dclock rates, say BPI
> panel has 30MHz clock that would failed with this logic.
> 
> On the other side Allwinner BSP calculating dclk divider based on the
> SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> calculated based on the bpp/lanes.

It looks like the A64 has the same divider of 4:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12

I think you're confusing it with the ratio between the pixel clock and
the dotclock, called dsi_div:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198

Maxime
On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > lowering the min rate by 300MHz can result proper working
> > > > nkms divider with the help of desired dclock rate from
> > > > panel driver.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > >
> > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > is the issue that you are trying to fix here?
> > >
> > > It looks like it's picking bad dividers, but if that's the case, this
> > > isn't the proper fix.
> >
> > As I stated in earlier patches, the whole idea is pick the desired
> > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > is unable to get the proper dclk divider at the end, so it eventually
> > picking up wrong divider value and fired vblank timeout.
> >
> > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > get the desired clock something like below.
> > [    2.415773] [drm] No driver support for vblank timestamp query.
> > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > [    2.424172] ideal = 220000000, rounded = 0
> > [    2.424176] ideal = 275000000, rounded = 0
> > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > [    2.424197] ideal = 330000000, rounded = 330000000
> > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > [    2.424209] ideal = 220000000, rounded = 0
> > [    2.424213] ideal = 275000000, rounded = 0
> > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > [    2.424233] ideal = 330000000, rounded = 330000000
> > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > [    2.424661] sun4i_dclk_set_rate div 6
> > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> >
> > But look like this wouldn't valid for all other dclock rates, say BPI
> > panel has 30MHz clock that would failed with this logic.
> >
> > On the other side Allwinner BSP calculating dclk divider based on the
> > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > calculated based on the bpp/lanes.
>
> It looks like the A64 has the same divider of 4:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
>
> I think you're confusing it with the ratio between the pixel clock and
> the dotclock, called dsi_div:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198

Ahh.. I thought this initially but as far as DSI clock computation is
concern, the L12 tcon_div is local variable which is used for edge0
computation in burst mode and not for the dsi clock computation. Since
the BSP is unable to get the tcon_div during edge0 computation, they
defined it locally I think.

You can see the lcd_clk_config() code [2], where we can see DSI clock
computation using dsi_div value.

Here is dump after the in Line 792 which is after computation[3]
[   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
[   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
[   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000

The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
into dsi_div 6. So this can be our actual divider values dclk_min_div,
dclk_max_div in sun4i_dclk_round_rate (from
drivers/gpu/drm/sun4i/sun4i_dotclock.c)

We can even confirm this from Mainline code:
[    1.866128] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 55000000
[    1.873112] round_rate, parent = 330000000
[    1.877351] round_rate, rate = 330000000
[    1.881338] ideal = 330000000, rounded = 330000000, div = 6
[    1.887232] sun4i_dclk_round_rate: div = 6 rate = 55000000
[    1.887239] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 55000000
[    1.887243] round_rate, parent = 330000000
[    1.887259] round_rate, rate = 330000000
[    1.887264] ideal = 330000000, rounded = 330000000, div = 6
[    1.887267] sun4i_dclk_round_rate: div = 6 rate = 55000000
[    1.887270] round_rate, parent = 330000000
[    1.887286] round_rate, rate = 330000000
[    1.887292] round_rate, parent = 330000000
[    1.887307] round_rate, rate = 330000000
[    1.887320] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
[    1.887324] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
[    1.887350] rate = 330000000
[    1.887353] parent_rate = 297000000
[    1.887355] reg = 0x80c00000
[    1.887359] _nkm.n = 5, nkm->n.offset = 0x1, nkm->n.shift = 8
[    1.887362] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
[    1.887365] _nkm.m = 9, nkm->m.offset = 0x1, nkm->m.shift = 0
[    1.887712] sun4i_dclk_set_rate div 6
[    1.887720] sun4i_dclk_recalc_rate: val = 6, rate = 55000000

So, the dsi_div from AW BSP is our dclk_mini_div(and dclk_max_div) and
that can be computed as format/lanes in A64.

Hope this explaining clears the diff, let me know if I miss anything.

[2] https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L781
[3] https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L792
On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > lowering the min rate by 300MHz can result proper working
> > > > > nkms divider with the help of desired dclock rate from
> > > > > panel driver.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > >
> > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > is the issue that you are trying to fix here?
> > > >
> > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > isn't the proper fix.
> > >
> > > As I stated in earlier patches, the whole idea is pick the desired
> > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > is unable to get the proper dclk divider at the end, so it eventually
> > > picking up wrong divider value and fired vblank timeout.
> > >
> > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > get the desired clock something like below.
> > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > [    2.424172] ideal = 220000000, rounded = 0
> > > [    2.424176] ideal = 275000000, rounded = 0
> > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > [    2.424209] ideal = 220000000, rounded = 0
> > > [    2.424213] ideal = 275000000, rounded = 0
> > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > [    2.424661] sun4i_dclk_set_rate div 6
> > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > >
> > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > panel has 30MHz clock that would failed with this logic.
> > >
> > > On the other side Allwinner BSP calculating dclk divider based on the
> > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > calculated based on the bpp/lanes.
> >
> > It looks like the A64 has the same divider of 4:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> >
> > I think you're confusing it with the ratio between the pixel clock and
> > the dotclock, called dsi_div:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> 
> Ahh.. I thought this initially but as far as DSI clock computation is
> concern, the L12 tcon_div is local variable which is used for edge0
> computation in burst mode and not for the dsi clock computation. Since
> the BSP is unable to get the tcon_div during edge0 computation, they
> defined it locally I think.
> 
> You can see the lcd_clk_config() code [2], where we can see DSI clock
> computation using dsi_div value.
> 
> Here is dump after the in Line 792 which is after computation[3]
> [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> 
> The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> into dsi_div 6. So this can be our actual divider values dclk_min_div,
> dclk_max_div in sun4i_dclk_round_rate (from
> drivers/gpu/drm/sun4i/sun4i_dotclock.c)

I wish it was in your commit log in the first place, instead of having
to exchange multiple mails over this.

However, I don't think that's quite true, and it might be a bug in
Allwinner's implementation (or rather something quite confusing).

You're right that the lcd_rate and pll_rate seem to be generated from
the pixel clock, and it indeed looks like the ratio between the pixel
clock and the TCON dotclock is defined through the number of bits per
lanes.

However, in this case, dsi_rate is actually the same than lcd_rate,
since pll_rate is going to be divided by dsi_div:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791

Since lcd_div is 1, it also means that in this case, dsi_rate ==
dclk_rate.

The DSI module clock however, is always set to 148.5 MHz. Indeed, if
we look at:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804

We can see that the rate in clk_info is used if it's different than
0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
DSI panel, will hardcode it to 148.5 MHz:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164

So, the DSI clock is set to this here:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805

The TCON *module* clock (the one in the clock controller) has been set
to lcd_rate (so the pixel clock times the number of bits per lane) here:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800

And the PLL has been set to the same rate here:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794

Let's take a step back now: that function we were looking at,
lcd_clk_config, is called by lcd_clk_enable, which is in turn called
by disp_lcd_enable here:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328

The next function being called is disp_al_lcd_cfg, and that function
will hardcode the TCON dotclock divider to 4, here:
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240

So, in the end, the dotclock divider is always 4, the DSI module clock
is set to 148.5 MHz, and the TCON module clock is set to 330MHz. Since
the TCON module clock doesn't have a divider, the PLL is set at that
same value but this is redundant.

I'll experiment with this and try to see how it works on the A33.

Maxime
On Fri, Feb 1, 2019 at 8:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> > On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > > lowering the min rate by 300MHz can result proper working
> > > > > > nkms divider with the help of desired dclock rate from
> > > > > > panel driver.
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > > >
> > > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > > is the issue that you are trying to fix here?
> > > > >
> > > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > > isn't the proper fix.
> > > >
> > > > As I stated in earlier patches, the whole idea is pick the desired
> > > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > > is unable to get the proper dclk divider at the end, so it eventually
> > > > picking up wrong divider value and fired vblank timeout.
> > > >
> > > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > > get the desired clock something like below.
> > > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424172] ideal = 220000000, rounded = 0
> > > > [    2.424176] ideal = 275000000, rounded = 0
> > > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424209] ideal = 220000000, rounded = 0
> > > > [    2.424213] ideal = 275000000, rounded = 0
> > > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > > [    2.424661] sun4i_dclk_set_rate div 6
> > > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > > >
> > > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > > panel has 30MHz clock that would failed with this logic.
> > > >
> > > > On the other side Allwinner BSP calculating dclk divider based on the
> > > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > > calculated based on the bpp/lanes.
> > >
> > > It looks like the A64 has the same divider of 4:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > >
> > > I think you're confusing it with the ratio between the pixel clock and
> > > the dotclock, called dsi_div:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> >
> > Ahh.. I thought this initially but as far as DSI clock computation is
> > concern, the L12 tcon_div is local variable which is used for edge0
> > computation in burst mode and not for the dsi clock computation. Since
> > the BSP is unable to get the tcon_div during edge0 computation, they
> > defined it locally I think.
> >
> > You can see the lcd_clk_config() code [2], where we can see DSI clock
> > computation using dsi_div value.
> >
> > Here is dump after the in Line 792 which is after computation[3]
> > [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> > [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> > [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> >
> > The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> > into dsi_div 6. So this can be our actual divider values dclk_min_div,
> > dclk_max_div in sun4i_dclk_round_rate (from
> > drivers/gpu/drm/sun4i/sun4i_dotclock.c)
>
> I wish it was in your commit log in the first place, instead of having
> to exchange multiple mails over this.
>
> However, I don't think that's quite true, and it might be a bug in
> Allwinner's implementation (or rather something quite confusing).
>
> You're right that the lcd_rate and pll_rate seem to be generated from
> the pixel clock, and it indeed looks like the ratio between the pixel
> clock and the TCON dotclock is defined through the number of bits per
> lanes.
>
> However, in this case, dsi_rate is actually the same than lcd_rate,
> since pll_rate is going to be divided by dsi_div:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
>
> Since lcd_div is 1, it also means that in this case, dsi_rate ==
> dclk_rate.

Yes, but the lcd_rate and dsi_rate are not same, since dsi_rate is
again computed as dsi_div with pll_rate.

lcd_rate = dclk_rate * 6;
pll_rate = lcd_rate * 1;
dsi_rate = pll_rate / 6;

[   14.719981] tcon_div = 4, lcd_div = 1, dsi_div = 6, dsi_rate = 148500000
[   14.722666] dsi_div = 6, lcd_div = 1
[   14.722695] [DISP]disp_module_init finish
[   14.727699] lcd_rate = 180000000, pll_rate = 180000000
[   14.730269] dsi_rate = 30000000

> The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> we look at:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
>
> We can see that the rate in clk_info is used if it's different than
> 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> DSI panel, will hardcode it to 148.5 MHz:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
>
> So, the DSI clock is set to this here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
>
> The TCON *module* clock (the one in the clock controller) has been set
> to lcd_rate (so the pixel clock times the number of bits per lane) here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
>
> And the PLL has been set to the same rate here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
>
> Let's take a step back now: that function we were looking at,
> lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> by disp_lcd_enable here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
>
> The next function being called is disp_al_lcd_cfg, and that function
> will hardcode the TCON dotclock divider to 4, here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240

Haa.. this is reason I have TCON_DCLK register value is 4 in BSP
# devmem 0x01c0c044
0xF0000004

> So, in the end, the dotclock divider is always 4, the DSI module clock
> is set to 148.5 MHz, and the TCON module clock is set to 330MHz. Since
> the TCON module clock doesn't have a divider, the PLL is set at that
> same value but this is redundant.

330 or 180MHz. if we have pixel clock 30MHz with 6 div value it's 180MHz.

>
> I'll experiment with this and try to see how it works on the A33.

OK, thanks. will try digging further on this from my side as well.
Hi Maxime,

On Fri, Feb 1, 2019 at 8:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> > On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > > lowering the min rate by 300MHz can result proper working
> > > > > > nkms divider with the help of desired dclock rate from
> > > > > > panel driver.
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > > >
> > > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > > is the issue that you are trying to fix here?
> > > > >
> > > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > > isn't the proper fix.
> > > >
> > > > As I stated in earlier patches, the whole idea is pick the desired
> > > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > > is unable to get the proper dclk divider at the end, so it eventually
> > > > picking up wrong divider value and fired vblank timeout.
> > > >
> > > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > > get the desired clock something like below.
> > > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424172] ideal = 220000000, rounded = 0
> > > > [    2.424176] ideal = 275000000, rounded = 0
> > > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424209] ideal = 220000000, rounded = 0
> > > > [    2.424213] ideal = 275000000, rounded = 0
> > > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > > [    2.424661] sun4i_dclk_set_rate div 6
> > > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > > >
> > > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > > panel has 30MHz clock that would failed with this logic.
> > > >
> > > > On the other side Allwinner BSP calculating dclk divider based on the
> > > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > > calculated based on the bpp/lanes.
> > >
> > > It looks like the A64 has the same divider of 4:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > >
> > > I think you're confusing it with the ratio between the pixel clock and
> > > the dotclock, called dsi_div:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> >
> > Ahh.. I thought this initially but as far as DSI clock computation is
> > concern, the L12 tcon_div is local variable which is used for edge0
> > computation in burst mode and not for the dsi clock computation. Since
> > the BSP is unable to get the tcon_div during edge0 computation, they
> > defined it locally I think.
> >
> > You can see the lcd_clk_config() code [2], where we can see DSI clock
> > computation using dsi_div value.
> >
> > Here is dump after the in Line 792 which is after computation[3]
> > [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> > [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> > [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> >
> > The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> > into dsi_div 6. So this can be our actual divider values dclk_min_div,
> > dclk_max_div in sun4i_dclk_round_rate (from
> > drivers/gpu/drm/sun4i/sun4i_dotclock.c)
>
> I wish it was in your commit log in the first place, instead of having
> to exchange multiple mails over this.
>
> However, I don't think that's quite true, and it might be a bug in
> Allwinner's implementation (or rather something quite confusing).
>
> You're right that the lcd_rate and pll_rate seem to be generated from
> the pixel clock, and it indeed looks like the ratio between the pixel
> clock and the TCON dotclock is defined through the number of bits per
> lanes.
>
> However, in this case, dsi_rate is actually the same than lcd_rate,
> since pll_rate is going to be divided by dsi_div:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
>
> Since lcd_div is 1, it also means that in this case, dsi_rate ==
> dclk_rate.
>
> The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> we look at:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
>
> We can see that the rate in clk_info is used if it's different than
> 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> DSI panel, will hardcode it to 148.5 MHz:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
>
> So, the DSI clock is set to this here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
>
> The TCON *module* clock (the one in the clock controller) has been set
> to lcd_rate (so the pixel clock times the number of bits per lane) here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
>
> And the PLL has been set to the same rate here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
>
> Let's take a step back now: that function we were looking at,
> lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> by disp_lcd_enable here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
>
> The next function being called is disp_al_lcd_cfg, and that function
> will hardcode the TCON dotclock divider to 4, here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
>
> So, in the end, the dotclock divider is always 4, the DSI module clock
> is set to 148.5 MHz, and the TCON module clock is set to 330MHz. Since
> the TCON module clock doesn't have a divider, the PLL is set at that
> same value but this is redundant.
>
> I'll experiment with this and try to see how it works on the A33.

How is it with A33?
On Mon, Feb 11, 2019 at 07:37:57PM +0530, Jagan Teki wrote:
> Hi Maxime,
> 
> On Fri, Feb 1, 2019 at 8:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> > > On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > >
> > > > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > > > lowering the min rate by 300MHz can result proper working
> > > > > > > nkms divider with the help of desired dclock rate from
> > > > > > > panel driver.
> > > > > > >
> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > > > >
> > > > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > > > is the issue that you are trying to fix here?
> > > > > >
> > > > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > > > isn't the proper fix.
> > > > >
> > > > > As I stated in earlier patches, the whole idea is pick the desired
> > > > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > > > is unable to get the proper dclk divider at the end, so it eventually
> > > > > picking up wrong divider value and fired vblank timeout.
> > > > >
> > > > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > > > get the desired clock something like below.
> > > > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > > [    2.424172] ideal = 220000000, rounded = 0
> > > > > [    2.424176] ideal = 275000000, rounded = 0
> > > > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > > [    2.424209] ideal = 220000000, rounded = 0
> > > > > [    2.424213] ideal = 275000000, rounded = 0
> > > > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > > > [    2.424661] sun4i_dclk_set_rate div 6
> > > > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > > > >
> > > > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > > > panel has 30MHz clock that would failed with this logic.
> > > > >
> > > > > On the other side Allwinner BSP calculating dclk divider based on the
> > > > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > > > calculated based on the bpp/lanes.
> > > >
> > > > It looks like the A64 has the same divider of 4:
> > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > >
> > > > I think you're confusing it with the ratio between the pixel clock and
> > > > the dotclock, called dsi_div:
> > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> > >
> > > Ahh.. I thought this initially but as far as DSI clock computation is
> > > concern, the L12 tcon_div is local variable which is used for edge0
> > > computation in burst mode and not for the dsi clock computation. Since
> > > the BSP is unable to get the tcon_div during edge0 computation, they
> > > defined it locally I think.
> > >
> > > You can see the lcd_clk_config() code [2], where we can see DSI clock
> > > computation using dsi_div value.
> > >
> > > Here is dump after the in Line 792 which is after computation[3]
> > > [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> > > [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> > > [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> > >
> > > The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> > > into dsi_div 6. So this can be our actual divider values dclk_min_div,
> > > dclk_max_div in sun4i_dclk_round_rate (from
> > > drivers/gpu/drm/sun4i/sun4i_dotclock.c)
> >
> > I wish it was in your commit log in the first place, instead of having
> > to exchange multiple mails over this.
> >
> > However, I don't think that's quite true, and it might be a bug in
> > Allwinner's implementation (or rather something quite confusing).
> >
> > You're right that the lcd_rate and pll_rate seem to be generated from
> > the pixel clock, and it indeed looks like the ratio between the pixel
> > clock and the TCON dotclock is defined through the number of bits per
> > lanes.
> >
> > However, in this case, dsi_rate is actually the same than lcd_rate,
> > since pll_rate is going to be divided by dsi_div:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> >
> > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > dclk_rate.
> >
> > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > we look at:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> >
> > We can see that the rate in clk_info is used if it's different than
> > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > DSI panel, will hardcode it to 148.5 MHz:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> >
> > So, the DSI clock is set to this here:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> >
> > The TCON *module* clock (the one in the clock controller) has been set
> > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> >
> > And the PLL has been set to the same rate here:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> >
> > Let's take a step back now: that function we were looking at,
> > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > by disp_lcd_enable here:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> >
> > The next function being called is disp_al_lcd_cfg, and that function
> > will hardcode the TCON dotclock divider to 4, here:
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> >
> > So, in the end, the dotclock divider is always 4, the DSI module clock
> > is set to 148.5 MHz, and the TCON module clock is set to 330MHz. Since
> > the TCON module clock doesn't have a divider, the PLL is set at that
> > same value but this is redundant.
> >
> > I'll experiment with this and try to see how it works on the A33.
> 
> How is it with A33?

The conclusions are the one I sent in my last series

Maxime
On Tue, Feb 12, 2019 at 3:00 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Feb 11, 2019 at 07:37:57PM +0530, Jagan Teki wrote:
> > Hi Maxime,
> >
> > On Fri, Feb 1, 2019 at 8:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> > > > On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > > > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > > > > lowering the min rate by 300MHz can result proper working
> > > > > > > > nkms divider with the help of desired dclock rate from
> > > > > > > > panel driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > > > > >
> > > > > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > > > > is the issue that you are trying to fix here?
> > > > > > >
> > > > > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > > > > isn't the proper fix.
> > > > > >
> > > > > > As I stated in earlier patches, the whole idea is pick the desired
> > > > > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > > > > is unable to get the proper dclk divider at the end, so it eventually
> > > > > > picking up wrong divider value and fired vblank timeout.
> > > > > >
> > > > > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > > > > get the desired clock something like below.
> > > > > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > > > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > > > [    2.424172] ideal = 220000000, rounded = 0
> > > > > > [    2.424176] ideal = 275000000, rounded = 0
> > > > > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > > > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > > > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > > > [    2.424209] ideal = 220000000, rounded = 0
> > > > > > [    2.424213] ideal = 275000000, rounded = 0
> > > > > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > > > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > > > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > > > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > > > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > > > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > > > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > > > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > > > > [    2.424661] sun4i_dclk_set_rate div 6
> > > > > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > > > > >
> > > > > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > > > > panel has 30MHz clock that would failed with this logic.
> > > > > >
> > > > > > On the other side Allwinner BSP calculating dclk divider based on the
> > > > > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > > > > calculated based on the bpp/lanes.
> > > > >
> > > > > It looks like the A64 has the same divider of 4:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > > >
> > > > > I think you're confusing it with the ratio between the pixel clock and
> > > > > the dotclock, called dsi_div:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> > > >
> > > > Ahh.. I thought this initially but as far as DSI clock computation is
> > > > concern, the L12 tcon_div is local variable which is used for edge0
> > > > computation in burst mode and not for the dsi clock computation. Since
> > > > the BSP is unable to get the tcon_div during edge0 computation, they
> > > > defined it locally I think.
> > > >
> > > > You can see the lcd_clk_config() code [2], where we can see DSI clock
> > > > computation using dsi_div value.
> > > >
> > > > Here is dump after the in Line 792 which is after computation[3]
> > > > [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> > > > [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> > > > [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> > > >
> > > > The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> > > > into dsi_div 6. So this can be our actual divider values dclk_min_div,
> > > > dclk_max_div in sun4i_dclk_round_rate (from
> > > > drivers/gpu/drm/sun4i/sun4i_dotclock.c)
> > >
> > > I wish it was in your commit log in the first place, instead of having
> > > to exchange multiple mails over this.
> > >
> > > However, I don't think that's quite true, and it might be a bug in
> > > Allwinner's implementation (or rather something quite confusing).
> > >
> > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > clock and the TCON dotclock is defined through the number of bits per
> > > lanes.
> > >
> > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > since pll_rate is going to be divided by dsi_div:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > >
> > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > dclk_rate.
> > >
> > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > we look at:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > >
> > > We can see that the rate in clk_info is used if it's different than
> > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > DSI panel, will hardcode it to 148.5 MHz:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> > >
> > > So, the DSI clock is set to this here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> > >
> > > The TCON *module* clock (the one in the clock controller) has been set
> > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > >
> > > And the PLL has been set to the same rate here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > >
> > > Let's take a step back now: that function we were looking at,
> > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > by disp_lcd_enable here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > >
> > > The next function being called is disp_al_lcd_cfg, and that function
> > > will hardcode the TCON dotclock divider to 4, here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> > >
> > > So, in the end, the dotclock divider is always 4, the DSI module clock
> > > is set to 148.5 MHz, and the TCON module clock is set to 330MHz. Since
> > > the TCON module clock doesn't have a divider, the PLL is set at that
> > > same value but this is redundant.
> > >
> > > I'll experiment with this and try to see how it works on the A33.
> >
> > How is it with A33?
>
> The conclusions are the one I sent in my last series

In this series?
[PATCH v3 0/8] drm/sun4i: dsi: Add burst mode support
On Fri, Feb 1, 2019 at 8:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Jan 29, 2019 at 11:01:31PM +0530, Jagan Teki wrote:
> > On Tue, Jan 29, 2019 at 8:43 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Jan 28, 2019 at 03:06:10PM +0530, Jagan Teki wrote:
> > > > On Sat, Jan 26, 2019 at 2:54 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Fri, Jan 25, 2019 at 01:28:49AM +0530, Jagan Teki wrote:
> > > > > > Minimum PLL used for MIPI is 500MHz, as per manual, but
> > > > > > lowering the min rate by 300MHz can result proper working
> > > > > > nkms divider with the help of desired dclock rate from
> > > > > > panel driver.
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > > >
> > > > > Going 200MHz below the minimum doesn't seem really reasonable. What
> > > > > is the issue that you are trying to fix here?
> > > > >
> > > > > It looks like it's picking bad dividers, but if that's the case, this
> > > > > isn't the proper fix.
> > > >
> > > > As I stated in earlier patches, the whole idea is pick the desired
> > > > dclk divider based dclk rate. So the dotclock, sun4i_dclk_round_rate
> > > > is unable to get the proper dclk divider at the end, so it eventually
> > > > picking up wrong divider value and fired vblank timeout.
> > > >
> > > > So, we come-up with optimal and working min_rate 300MHz in pll-mipi to
> > > > get the desired clock something like below.
> > > > [    2.415773] [drm] No driver support for vblank timestamp query.
> > > > [    2.424116] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424172] ideal = 220000000, rounded = 0
> > > > [    2.424176] ideal = 275000000, rounded = 0
> > > > [    2.424194] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424197] ideal = 330000000, rounded = 330000000
> > > > [    2.424201] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424205] sun4i_dclk_round_rate: min_div = 4 max_div = 127, rate = 55000000
> > > > [    2.424209] ideal = 220000000, rounded = 0
> > > > [    2.424213] ideal = 275000000, rounded = 0
> > > > [    2.424230] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424233] ideal = 330000000, rounded = 330000000
> > > > [    2.424236] sun4i_dclk_round_rate: div = 6 rate = 55000000
> > > > [    2.424253] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424270] ccu_nkm_round_rate: rate = 330000000
> > > > [    2.424278] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424281] sun4i_dclk_recalc_rate: val = 1, rate = 330000000
> > > > [    2.424306] ccu_nkm_set_rate: rate = 330000000, parent_rate = 297000000
> > > > [    2.424309] ccu_nkm_set_rate: _nkm.n = 5
> > > > [    2.424311] ccu_nkm_set_rate: _nkm.k = 2
> > > > [    2.424313] ccu_nkm_set_rate: _nkm.m = 9
> > > > [    2.424661] sun4i_dclk_set_rate div 6
> > > > [    2.424668] sun4i_dclk_recalc_rate: val = 6, rate = 55000000
> > > >
> > > > But look like this wouldn't valid for all other dclock rates, say BPI
> > > > panel has 30MHz clock that would failed with this logic.
> > > >
> > > > On the other side Allwinner BSP calculating dclk divider based on the
> > > > SoC's. for A33 [1] it is fixed dclk divider of 4 and for A64 is is
> > > > calculated based on the bpp/lanes.
> > >
> > > It looks like the A64 has the same divider of 4:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > >
> > > I think you're confusing it with the ratio between the pixel clock and
> > > the dotclock, called dsi_div:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L198
> >
> > Ahh.. I thought this initially but as far as DSI clock computation is
> > concern, the L12 tcon_div is local variable which is used for edge0
> > computation in burst mode and not for the dsi clock computation. Since
> > the BSP is unable to get the tcon_div during edge0 computation, they
> > defined it locally I think.
> >
> > You can see the lcd_clk_config() code [2], where we can see DSI clock
> > computation using dsi_div value.
> >
> > Here is dump after the in Line 792 which is after computation[3]
> > [   10.800737] lcd_clk_config: dsi_div = 6, tcon_div = 4, lcd_div = 1
> > [   10.800743] lcd_clk_config: lcd_dclk_freq = 55, dclk_rate = 55000000
> > [   10.800749] lcd_clk_config: lcd_rate = 330000000, pll_rate = 330000000
> >
> > The above dump the lcd_rate 330MHz is computed with panel clock, 55MHz
> > into dsi_div 6. So this can be our actual divider values dclk_min_div,
> > dclk_max_div in sun4i_dclk_round_rate (from
> > drivers/gpu/drm/sun4i/sun4i_dotclock.c)
>
> I wish it was in your commit log in the first place, instead of having
> to exchange multiple mails over this.
>
> However, I don't think that's quite true, and it might be a bug in
> Allwinner's implementation (or rather something quite confusing).
>
> You're right that the lcd_rate and pll_rate seem to be generated from
> the pixel clock, and it indeed looks like the ratio between the pixel
> clock and the TCON dotclock is defined through the number of bits per
> lanes.
>
> However, in this case, dsi_rate is actually the same than lcd_rate,
> since pll_rate is going to be divided by dsi_div:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
>
> Since lcd_div is 1, it also means that in this case, dsi_rate ==
> dclk_rate.
>
> The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> we look at:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
>
> We can see that the rate in clk_info is used if it's different than
> 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> DSI panel, will hardcode it to 148.5 MHz:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
>
> So, the DSI clock is set to this here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
>
> The TCON *module* clock (the one in the clock controller) has been set
> to lcd_rate (so the pixel clock times the number of bits per lane) here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
>
> And the PLL has been set to the same rate here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794

Let me explain, something more.

According to bsp there are clk_info.tcon_div which I will explain below.
clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
is 6 for 24bpp and 4 lanes devices.

PLL rate here depends on dsi_div (not tcon_div)

Code here
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784

is computing the actual set rate, which depends on dsi_rate.

lcd_rate = dclk_rate * clk_info.dsi_div;
dsi_rate = pll_rate / clk_info.dsi_div;

Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
for above link you mentioned.

Here are the evidence with some prints.

https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a

>
> Let's take a step back now: that function we were looking at,
> lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> by disp_lcd_enable here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
>
> The next function being called is disp_al_lcd_cfg, and that function
> will hardcode the TCON dotclock divider to 4, here:
> https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240

tcon_div from BSP point-of-view of there are two variants
00) clk_info.tcon_div which is 4 and same is set the divider position
in SUN4I_TCON0_DCLK_REG (like above link refer)
01) tcon_div which is 4 and used for edge timings computation
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12

The real reason for 01) is again 4 is they set the divider to 4 in 00)
which is technically wrong because the dividers which used during
dotclock in above (dsi_div) should be used here as well. Since there
is no dynamic way of doing this BSP hard-coding these values.

Patches 5,6,7 on this series doing this
https://patchwork.freedesktop.org/series/60847/

Hope this explanation helps?

On Wed, Jun 5, 2019 at 12:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi,
>
> I've reordered the mail a bit to work on chunks
>
> On Fri, May 24, 2019 at 03:37:42PM +0530, Jagan Teki wrote:
> > > I wish it was in your commit log in the first place, instead of having
> > > to exchange multiple mails over this.
> > >
> > > However, I don't think that's quite true, and it might be a bug in
> > > Allwinner's implementation (or rather something quite confusing).
> > >
> > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > clock and the TCON dotclock is defined through the number of bits per
> > > lanes.
> > >
> > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > since pll_rate is going to be divided by dsi_div:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > >
> > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > dclk_rate.
> > >
> > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > we look at:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > >
> > > We can see that the rate in clk_info is used if it's different than
> > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > DSI panel, will hardcode it to 148.5 MHz:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> >
> > Let me explain, something more.
> >
> > According to bsp there are clk_info.tcon_div which I will explain below.
> > clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
> > is 6 for 24bpp and 4 lanes devices.
> >
> > PLL rate here depends on dsi_div (not tcon_div)
> >
> > Code here
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784
> >
> > is computing the actual set rate, which depends on dsi_rate.
> >
> > lcd_rate = dclk_rate * clk_info.dsi_div;
> > dsi_rate = pll_rate / clk_info.dsi_div;
> >
> > Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
> > for above link you mentioned.
> >
> > Here are the evidence with some prints.
> >
> > https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
> > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
>
> Ok, so we agree up to this point, and the prints confirm that the
> analysis above is the right one.
>
> > > So, the DSI clock is set to this here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
>
> Your patch doesn't address that, so let's leave that one alone.

Basically this is final pll set rate when sun4i_dotclock.c called the
desired rate with ccu_nkm.c so it ended the final rate with parent as
Line 8 of
https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a

>
> > > The TCON *module* clock (the one in the clock controller) has been set
> > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > >
> > > And the PLL has been set to the same rate here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > >
> > > Let's take a step back now: that function we were looking at,
> > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > by disp_lcd_enable here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > >
> > > The next function being called is disp_al_lcd_cfg, and that function
> > > will hardcode the TCON dotclock divider to 4, here:
> > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> >
> > tcon_div from BSP point-of-view of there are two variants
> > 00) clk_info.tcon_div which is 4 and same is set the divider position
> > in SUN4I_TCON0_DCLK_REG (like above link refer)
> > 01) tcon_div which is 4 and used for edge timings computation
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> >
> > The real reason for 01) is again 4 is they set the divider to 4 in 00)
> > which is technically wrong because the dividers which used during
> > dotclock in above (dsi_div) should be used here as well. Since there
> > is no dynamic way of doing this BSP hard-coding these values.
> >
> > Patches 5,6,7 on this series doing this
> > https://patchwork.freedesktop.org/series/60847/
> >
> > Hope this explanation helps?
>
> It doesn't.
>
> The clock tree is this one:
>
> PLL(s) -> TCON module clock -> TCON dotclock.
>
> The links I mentioned above show that the clock set to lcd_rate is the
> TCON module clocks (and it should be the one taking the bpp and lanes
> into account), while the TCON dotclock uses a fixed divider of 4.

Sorry, I can argue much other-than giving some code snips, according to [1]

00) Line 785, 786 with dclk_rate 148000000

lcd_rate = dclk_rate * clk_info.dsi_div;
pll_rate = lcd_rate * clk_info.lcd_div;

Since dsi_div is 6 (bpp/lanes), lcd_div 1

lcd_rate = 888000000, pll_rate = 888000000

01)  Line 801, 804 are final rates computed as per clock driver (say
ccu_nkm in mainline)

lcd_rate_set=891000000

As per your comments if it would be 4 then the desired numbers are
would be 592000000 not 888000000.

This is what I'm trying to say in all mails, and same as verified with
2-lanes devices as well where the dsi_div is 12 so the final rate is
290MHz * 12

>
> In your patches, you're using the bpp / lanes divider on the TCON
> dotclock, ie, the wrong clock.
>
> Again, I'm not saying that my analysis of the source code is correct
> here. But you haven't said anything to prove it's wrong either.

Don't understand what proves are remaining, I have explained each line
from BSP and saying pll rate is depends on dsi_div which is bpp/lanes
not wrt tcon_div on BSP (which is set to default 4) and which indeed
verified in A33, R40. all the code using bpp/lanes.

Please let me know if you need any more information to look?

[1] https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805

On Fri, Jun 14, 2019 at 7:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Wed, Jun 05, 2019 at 01:03:16PM +0530, Jagan Teki wrote:
> > On Wed, Jun 5, 2019 at 12:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > Hi,
> > >
> > > I've reordered the mail a bit to work on chunks
> > >
> > > On Fri, May 24, 2019 at 03:37:42PM +0530, Jagan Teki wrote:
> > > > > I wish it was in your commit log in the first place, instead of having
> > > > > to exchange multiple mails over this.
> > > > >
> > > > > However, I don't think that's quite true, and it might be a bug in
> > > > > Allwinner's implementation (or rather something quite confusing).
> > > > >
> > > > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > > > clock and the TCON dotclock is defined through the number of bits per
> > > > > lanes.
> > > > >
> > > > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > > > since pll_rate is going to be divided by dsi_div:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > > > >
> > > > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > > > dclk_rate.
> > > > >
> > > > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > > > we look at:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > > > >
> > > > > We can see that the rate in clk_info is used if it's different than
> > > > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > > > DSI panel, will hardcode it to 148.5 MHz:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> > > >
> > > > Let me explain, something more.
> > > >
> > > > According to bsp there are clk_info.tcon_div which I will explain below.
> > > > clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
> > > > is 6 for 24bpp and 4 lanes devices.
> > > >
> > > > PLL rate here depends on dsi_div (not tcon_div)
> > > >
> > > > Code here
> > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784
> > > >
> > > > is computing the actual set rate, which depends on dsi_rate.
> > > >
> > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > dsi_rate = pll_rate / clk_info.dsi_div;
> > > >
> > > > Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
> > > > for above link you mentioned.
> > > >
> > > > Here are the evidence with some prints.
> > > >
> > > > https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
> > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > >
> > > Ok, so we agree up to this point, and the prints confirm that the
> > > analysis above is the right one.
> > >
> > > > > So, the DSI clock is set to this here:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> > >
> > > Your patch doesn't address that, so let's leave that one alone.
> >
> > Basically this is final pll set rate when sun4i_dotclock.c called the
> > desired rate with ccu_nkm.c so it ended the final rate with parent as
> > Line 8 of
> > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
>
> If that's important to the driver, it should be set explicitly then,
> and not work by accident.
>
> > > > > The TCON *module* clock (the one in the clock controller) has been set
> > > > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > > > >
> > > > > And the PLL has been set to the same rate here:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > > > >
> > > > > Let's take a step back now: that function we were looking at,
> > > > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > > > by disp_lcd_enable here:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > > > >
> > > > > The next function being called is disp_al_lcd_cfg, and that function
> > > > > will hardcode the TCON dotclock divider to 4, here:
> > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> > > >
> > > > tcon_div from BSP point-of-view of there are two variants
> > > > 00) clk_info.tcon_div which is 4 and same is set the divider position
> > > > in SUN4I_TCON0_DCLK_REG (like above link refer)
> > > > 01) tcon_div which is 4 and used for edge timings computation
> > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > >
> > > > The real reason for 01) is again 4 is they set the divider to 4 in 00)
> > > > which is technically wrong because the dividers which used during
> > > > dotclock in above (dsi_div) should be used here as well. Since there
> > > > is no dynamic way of doing this BSP hard-coding these values.
> > > >
> > > > Patches 5,6,7 on this series doing this
> > > > https://patchwork.freedesktop.org/series/60847/
> > > >
> > > > Hope this explanation helps?
> > >
> > > It doesn't.
> > >
> > > The clock tree is this one:
> > >
> > > PLL(s) -> TCON module clock -> TCON dotclock.
> > >
> > > The links I mentioned above show that the clock set to lcd_rate is the
> > > TCON module clocks (and it should be the one taking the bpp and lanes
> > > into account), while the TCON dotclock uses a fixed divider of 4.
> >
> > Sorry, I can argue much other-than giving some code snips, according to [1]
> >
> > 00) Line 785, 786 with dclk_rate 148000000
> >
> > lcd_rate = dclk_rate * clk_info.dsi_div;
> > pll_rate = lcd_rate * clk_info.lcd_div;
> >
> > Since dsi_div is 6 (bpp/lanes), lcd_div 1
> >
> > lcd_rate = 888000000, pll_rate = 888000000
> >
> > 01)  Line 801, 804 are final rates computed as per clock driver (say
> > ccu_nkm in mainline)
> >
> > lcd_rate_set=891000000
> >
> > As per your comments if it would be 4 then the desired numbers are
> > would be 592000000 not 888000000.
> >
> > This is what I'm trying to say in all mails, and same as verified with
> > 2-lanes devices as well where the dsi_div is 12 so the final rate is
> > 290MHz * 12
>
> In the code you sent, you're forcing a divider on the internal TCON
> clock, while that one is fixed in the BSP.
>
> There's indeed the bpp / lanes divider, but it's used in the *parent*
> clock of the one you're changing.
>
> And the dsi0_clk clock you pointed out in the code snippet is yet
> another clock, the MIPI DSI module clock.

Correct, look like I refereed wrong reference in the above mail. sorry
for the noise.

Actually I'm trying to explain about pll_rate here which indeed
depends on dsi.div
https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L786

lcd_rate = dclk_rate * clk_info.dsi_div;
pll_rate = lcd_rate * clk_info.lcd_div;

Say

1) For 148MHz dclk_rate with dsi_div is 6 (24/4) lcd_div is 1 which
resulting pll_rate is 888MHz.

2) For 30MHz dclk_rate with 4 lane and 24 RGB the resulting pll_rate is 180MHz

3) For 27.5MHz dclk_rate with 2 lane and 24 RGB the resulting pll_rate is 330MHz

Here is the few more logs in code, for case 2)

[    1.920441] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
[    1.920505] ideal = 180000000, rounded = 178200000
[    1.920509] sun4i_dclk_round_rate: div = 6 rate = 29700000
[    1.920514] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
[    1.920532] ideal = 180000000, rounded = 178200000
[    1.920535] sun4i_dclk_round_rate: div = 6 rate = 29700000
[    1.920572] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
[    1.920576] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
[    1.920597] rate = 178200000
[    1.920599] parent_rate = 297000000
[    1.920602] reg = 0x90c00000
[    1.920605] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
[    1.920609] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
[    1.920612] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
[    1.920958] sun4i_dclk_set_rate div 6
[    1.920966] sun4i_dclk_recalc_rate: val = 6, rate = 29700000

and clk_summary:

    pll-video0                        1        1        1   297000000
        0     0  50000
       hdmi                           0        0        0   297000000
        0     0  50000
       tcon1                          0        0        0   297000000
        0     0  50000
       pll-mipi                       1        1        1   178200000
        0     0  50000
          tcon0                       2        2        1   178200000
        0     0  50000
             tcon-pixel-clock         1        1        1    29700000
        0     0  50000
       pll-video0-2x                  0        0        0   594000000
        0     0  50000

On Tue, Jun 25, 2019 at 8:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Jun 20, 2019 at 11:57:44PM +0530, Jagan Teki wrote:
> > On Fri, Jun 14, 2019 at 7:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Wed, Jun 05, 2019 at 01:03:16PM +0530, Jagan Teki wrote:
> > > > On Wed, Jun 5, 2019 at 12:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > I've reordered the mail a bit to work on chunks
> > > > >
> > > > > On Fri, May 24, 2019 at 03:37:42PM +0530, Jagan Teki wrote:
> > > > > > > I wish it was in your commit log in the first place, instead of having
> > > > > > > to exchange multiple mails over this.
> > > > > > >
> > > > > > > However, I don't think that's quite true, and it might be a bug in
> > > > > > > Allwinner's implementation (or rather something quite confusing).
> > > > > > >
> > > > > > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > > > > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > > > > > clock and the TCON dotclock is defined through the number of bits per
> > > > > > > lanes.
> > > > > > >
> > > > > > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > > > > > since pll_rate is going to be divided by dsi_div:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > > > > > >
> > > > > > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > > > > > dclk_rate.
> > > > > > >
> > > > > > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > > > > > we look at:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > > > > > >
> > > > > > > We can see that the rate in clk_info is used if it's different than
> > > > > > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > > > > > DSI panel, will hardcode it to 148.5 MHz:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> > > > > >
> > > > > > Let me explain, something more.
> > > > > >
> > > > > > According to bsp there are clk_info.tcon_div which I will explain below.
> > > > > > clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
> > > > > > is 6 for 24bpp and 4 lanes devices.
> > > > > >
> > > > > > PLL rate here depends on dsi_div (not tcon_div)
> > > > > >
> > > > > > Code here
> > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784
> > > > > >
> > > > > > is computing the actual set rate, which depends on dsi_rate.
> > > > > >
> > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > dsi_rate = pll_rate / clk_info.dsi_div;
> > > > > >
> > > > > > Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
> > > > > > for above link you mentioned.
> > > > > >
> > > > > > Here are the evidence with some prints.
> > > > > >
> > > > > > https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
> > > > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > > > >
> > > > > Ok, so we agree up to this point, and the prints confirm that the
> > > > > analysis above is the right one.
> > > > >
> > > > > > > So, the DSI clock is set to this here:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> > > > >
> > > > > Your patch doesn't address that, so let's leave that one alone.
> > > >
> > > > Basically this is final pll set rate when sun4i_dotclock.c called the
> > > > desired rate with ccu_nkm.c so it ended the final rate with parent as
> > > > Line 8 of
> > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > >
> > > If that's important to the driver, it should be set explicitly then,
> > > and not work by accident.
> > >
> > > > > > > The TCON *module* clock (the one in the clock controller) has been set
> > > > > > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > > > > > >
> > > > > > > And the PLL has been set to the same rate here:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > > > > > >
> > > > > > > Let's take a step back now: that function we were looking at,
> > > > > > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > > > > > by disp_lcd_enable here:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > > > > > >
> > > > > > > The next function being called is disp_al_lcd_cfg, and that function
> > > > > > > will hardcode the TCON dotclock divider to 4, here:
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> > > > > >
> > > > > > tcon_div from BSP point-of-view of there are two variants
> > > > > > 00) clk_info.tcon_div which is 4 and same is set the divider position
> > > > > > in SUN4I_TCON0_DCLK_REG (like above link refer)
> > > > > > 01) tcon_div which is 4 and used for edge timings computation
> > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > > > >
> > > > > > The real reason for 01) is again 4 is they set the divider to 4 in 00)
> > > > > > which is technically wrong because the dividers which used during
> > > > > > dotclock in above (dsi_div) should be used here as well. Since there
> > > > > > is no dynamic way of doing this BSP hard-coding these values.
> > > > > >
> > > > > > Patches 5,6,7 on this series doing this
> > > > > > https://patchwork.freedesktop.org/series/60847/
> > > > > >
> > > > > > Hope this explanation helps?
> > > > >
> > > > > It doesn't.
> > > > >
> > > > > The clock tree is this one:
> > > > >
> > > > > PLL(s) -> TCON module clock -> TCON dotclock.
> > > > >
> > > > > The links I mentioned above show that the clock set to lcd_rate is the
> > > > > TCON module clocks (and it should be the one taking the bpp and lanes
> > > > > into account), while the TCON dotclock uses a fixed divider of 4.
> > > >
> > > > Sorry, I can argue much other-than giving some code snips, according to [1]
> > > >
> > > > 00) Line 785, 786 with dclk_rate 148000000
> > > >
> > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > pll_rate = lcd_rate * clk_info.lcd_div;
> > > >
> > > > Since dsi_div is 6 (bpp/lanes), lcd_div 1
> > > >
> > > > lcd_rate = 888000000, pll_rate = 888000000
> > > >
> > > > 01)  Line 801, 804 are final rates computed as per clock driver (say
> > > > ccu_nkm in mainline)
> > > >
> > > > lcd_rate_set=891000000
> > > >
> > > > As per your comments if it would be 4 then the desired numbers are
> > > > would be 592000000 not 888000000.
> > > >
> > > > This is what I'm trying to say in all mails, and same as verified with
> > > > 2-lanes devices as well where the dsi_div is 12 so the final rate is
> > > > 290MHz * 12
> > >
> > > In the code you sent, you're forcing a divider on the internal TCON
> > > clock, while that one is fixed in the BSP.
> > >
> > > There's indeed the bpp / lanes divider, but it's used in the *parent*
> > > clock of the one you're changing.
> > >
> > > And the dsi0_clk clock you pointed out in the code snippet is yet
> > > another clock, the MIPI DSI module clock.
> >
> > Correct, look like I refereed wrong reference in the above mail. sorry
> > for the noise.
> >
> > Actually I'm trying to explain about pll_rate here which indeed
> > depends on dsi.div
> > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L786
> >
> > lcd_rate = dclk_rate * clk_info.dsi_div;
> > pll_rate = lcd_rate * clk_info.lcd_div;
> >
> > Say
> >
> > 1) For 148MHz dclk_rate with dsi_div is 6 (24/4) lcd_div is 1 which
> > resulting pll_rate is 888MHz.
> >
> > 2) For 30MHz dclk_rate with 4 lane and 24 RGB the resulting pll_rate is 180MHz
> >
> > 3) For 27.5MHz dclk_rate with 2 lane and 24 RGB the resulting pll_rate is 330MHz
> >
> > Here is the few more logs in code, for case 2)
> >
> > [    1.920441] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > [    1.920505] ideal = 180000000, rounded = 178200000
> > [    1.920509] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > [    1.920514] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > [    1.920532] ideal = 180000000, rounded = 178200000
> > [    1.920535] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > [    1.920572] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > [    1.920576] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > [    1.920597] rate = 178200000
> > [    1.920599] parent_rate = 297000000
> > [    1.920602] reg = 0x90c00000
> > [    1.920605] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
> > [    1.920609] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
> > [    1.920612] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
> > [    1.920958] sun4i_dclk_set_rate div 6
> > [    1.920966] sun4i_dclk_recalc_rate: val = 6, rate = 29700000
> >
> > and clk_summary:
> >
> >     pll-video0                        1        1        1   297000000
> >         0     0  50000
> >        hdmi                           0        0        0   297000000
> >         0     0  50000
> >        tcon1                          0        0        0   297000000
> >         0     0  50000
> >        pll-mipi                       1        1        1   178200000
> >         0     0  50000
> >           tcon0                       2        2        1   178200000
> >         0     0  50000
> >              tcon-pixel-clock         1        1        1    29700000
> >         0     0  50000
> >        pll-video0-2x                  0        0        0   594000000
> >         0     0  50000
>
> This discussion is going nowhere. I'm telling you that your patch
> doesn't apply the divider you want on the proper clock, and you're
> replying that indeed, you're applying it on the wrong clock.
>
> It might work by accident in your case, but the board I have here
> clearly indicates otherwise, so there's two possible way out here:
>
>   - Either you apply that divider to the TCON *module* clock, and not
>     the dclk
>
>   - Or you point to somewhere in the allwinner code where the bpp /
>     lanes divider is used for the dclk divider.

I don't know how to proceed further on this, as you say it might work
in accident but I have tested this in A33, A64 and R40 with 4
different DSI panels and one DSI-RGB bridge. All of them do use
PLL_MIPI (pll_rate) and it indeed depends on bpp/lanes

4-lane, 24-bit: Novatek NT35596 panel
4-lane, 24-bit: Feiyang, FY07024di26a30d panel
4-lane, 24-bit: Bananapi-s070wv20 panel
2-lane, 24-bit: Techstar,ts8550b panel

and

4-lane, 24-bit, ICN6211 DSI-to-RGB bridge panel

All above listed panels and bridges are working as per BSP and do
follow bpp/lanes and for DIVIDER 4 no panel is working.

The panels/bridges I have has tested in BSP and as you mentioned in
another mail, your panel is not tested in BSP - this is the only
difference. I did much reverse-engineering on PLL_MIPI clocking in BSP
so I'm afraid what can I do next on this, If you want to look further
on BSP I would suggest to verify on pll_rate side. If you feel
anything I'm missing please let me know.

Jagan.

Hi Maxime

On Wed, Jul 3, 2019 at 1:49 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Jun 25, 2019 at 09:00:36PM +0530, Jagan Teki wrote:
> > On Tue, Jun 25, 2019 at 8:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Jun 20, 2019 at 11:57:44PM +0530, Jagan Teki wrote:
> > > > On Fri, Jun 14, 2019 at 7:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Wed, Jun 05, 2019 at 01:03:16PM +0530, Jagan Teki wrote:
> > > > > > On Wed, Jun 5, 2019 at 12:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > I've reordered the mail a bit to work on chunks
> > > > > > >
> > > > > > > On Fri, May 24, 2019 at 03:37:42PM +0530, Jagan Teki wrote:
> > > > > > > > > I wish it was in your commit log in the first place, instead of having
> > > > > > > > > to exchange multiple mails over this.
> > > > > > > > >
> > > > > > > > > However, I don't think that's quite true, and it might be a bug in
> > > > > > > > > Allwinner's implementation (or rather something quite confusing).
> > > > > > > > >
> > > > > > > > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > > > > > > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > > > > > > > clock and the TCON dotclock is defined through the number of bits per
> > > > > > > > > lanes.
> > > > > > > > >
> > > > > > > > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > > > > > > > since pll_rate is going to be divided by dsi_div:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > > > > > > > >
> > > > > > > > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > > > > > > > dclk_rate.
> > > > > > > > >
> > > > > > > > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > > > > > > > we look at:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > > > > > > > >
> > > > > > > > > We can see that the rate in clk_info is used if it's different than
> > > > > > > > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > > > > > > > DSI panel, will hardcode it to 148.5 MHz:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> > > > > > > >
> > > > > > > > Let me explain, something more.
> > > > > > > >
> > > > > > > > According to bsp there are clk_info.tcon_div which I will explain below.
> > > > > > > > clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
> > > > > > > > is 6 for 24bpp and 4 lanes devices.
> > > > > > > >
> > > > > > > > PLL rate here depends on dsi_div (not tcon_div)
> > > > > > > >
> > > > > > > > Code here
> > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784
> > > > > > > >
> > > > > > > > is computing the actual set rate, which depends on dsi_rate.
> > > > > > > >
> > > > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > > > dsi_rate = pll_rate / clk_info.dsi_div;
> > > > > > > >
> > > > > > > > Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
> > > > > > > > for above link you mentioned.
> > > > > > > >
> > > > > > > > Here are the evidence with some prints.
> > > > > > > >
> > > > > > > > https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
> > > > > > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > > > > > >
> > > > > > > Ok, so we agree up to this point, and the prints confirm that the
> > > > > > > analysis above is the right one.
> > > > > > >
> > > > > > > > > So, the DSI clock is set to this here:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> > > > > > >
> > > > > > > Your patch doesn't address that, so let's leave that one alone.
> > > > > >
> > > > > > Basically this is final pll set rate when sun4i_dotclock.c called the
> > > > > > desired rate with ccu_nkm.c so it ended the final rate with parent as
> > > > > > Line 8 of
> > > > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > > > >
> > > > > If that's important to the driver, it should be set explicitly then,
> > > > > and not work by accident.
> > > > >
> > > > > > > > > The TCON *module* clock (the one in the clock controller) has been set
> > > > > > > > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > > > > > > > >
> > > > > > > > > And the PLL has been set to the same rate here:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > > > > > > > >
> > > > > > > > > Let's take a step back now: that function we were looking at,
> > > > > > > > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > > > > > > > by disp_lcd_enable here:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > > > > > > > >
> > > > > > > > > The next function being called is disp_al_lcd_cfg, and that function
> > > > > > > > > will hardcode the TCON dotclock divider to 4, here:
> > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> > > > > > > >
> > > > > > > > tcon_div from BSP point-of-view of there are two variants
> > > > > > > > 00) clk_info.tcon_div which is 4 and same is set the divider position
> > > > > > > > in SUN4I_TCON0_DCLK_REG (like above link refer)
> > > > > > > > 01) tcon_div which is 4 and used for edge timings computation
> > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > > > > > >
> > > > > > > > The real reason for 01) is again 4 is they set the divider to 4 in 00)
> > > > > > > > which is technically wrong because the dividers which used during
> > > > > > > > dotclock in above (dsi_div) should be used here as well. Since there
> > > > > > > > is no dynamic way of doing this BSP hard-coding these values.
> > > > > > > >
> > > > > > > > Patches 5,6,7 on this series doing this
> > > > > > > > https://patchwork.freedesktop.org/series/60847/
> > > > > > > >
> > > > > > > > Hope this explanation helps?
> > > > > > >
> > > > > > > It doesn't.
> > > > > > >
> > > > > > > The clock tree is this one:
> > > > > > >
> > > > > > > PLL(s) -> TCON module clock -> TCON dotclock.
> > > > > > >
> > > > > > > The links I mentioned above show that the clock set to lcd_rate is the
> > > > > > > TCON module clocks (and it should be the one taking the bpp and lanes
> > > > > > > into account), while the TCON dotclock uses a fixed divider of 4.
> > > > > >
> > > > > > Sorry, I can argue much other-than giving some code snips, according to [1]
> > > > > >
> > > > > > 00) Line 785, 786 with dclk_rate 148000000
> > > > > >
> > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > pll_rate = lcd_rate * clk_info.lcd_div;
> > > > > >
> > > > > > Since dsi_div is 6 (bpp/lanes), lcd_div 1
> > > > > >
> > > > > > lcd_rate = 888000000, pll_rate = 888000000
> > > > > >
> > > > > > 01)  Line 801, 804 are final rates computed as per clock driver (say
> > > > > > ccu_nkm in mainline)
> > > > > >
> > > > > > lcd_rate_set=891000000
> > > > > >
> > > > > > As per your comments if it would be 4 then the desired numbers are
> > > > > > would be 592000000 not 888000000.
> > > > > >
> > > > > > This is what I'm trying to say in all mails, and same as verified with
> > > > > > 2-lanes devices as well where the dsi_div is 12 so the final rate is
> > > > > > 290MHz * 12
> > > > >
> > > > > In the code you sent, you're forcing a divider on the internal TCON
> > > > > clock, while that one is fixed in the BSP.
> > > > >
> > > > > There's indeed the bpp / lanes divider, but it's used in the *parent*
> > > > > clock of the one you're changing.
> > > > >
> > > > > And the dsi0_clk clock you pointed out in the code snippet is yet
> > > > > another clock, the MIPI DSI module clock.
> > > >
> > > > Correct, look like I refereed wrong reference in the above mail. sorry
> > > > for the noise.
> > > >
> > > > Actually I'm trying to explain about pll_rate here which indeed
> > > > depends on dsi.div
> > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L786
> > > >
> > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > pll_rate = lcd_rate * clk_info.lcd_div;
> > > >
> > > > Say
> > > >
> > > > 1) For 148MHz dclk_rate with dsi_div is 6 (24/4) lcd_div is 1 which
> > > > resulting pll_rate is 888MHz.
> > > >
> > > > 2) For 30MHz dclk_rate with 4 lane and 24 RGB the resulting pll_rate is 180MHz
> > > >
> > > > 3) For 27.5MHz dclk_rate with 2 lane and 24 RGB the resulting pll_rate is 330MHz
> > > >
> > > > Here is the few more logs in code, for case 2)
> > > >
> > > > [    1.920441] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > > > [    1.920505] ideal = 180000000, rounded = 178200000
> > > > [    1.920509] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > > > [    1.920514] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > > > [    1.920532] ideal = 1800ls and one DSI-RGB bridge. All of them do use
> > PLL_MIPI (pll_rate) and it indeed depends on bpp/lanes
> >00000, rounded = 178200000
> > > > [    1.920535] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > > > [    1.920572] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > > > [    1.920576] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > > > [    1.920597] rate = 178200000
> > > > [    1.920599] parent_rate = 297000000
> > > > [    1.920602] reg = 0x90c00000
> > > > [    1.920605] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
> > > > [    1.920609] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
> > > > [    1.920612] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
> > > > [    1.920958] sun4i_dclk_set_rate div 6
> > > > [    1.920966] sun4i_dclk_recalc_rate: val = 6, rate = 29700000
> > > >
> > > > and clk_summary:
> > > >
> > > >     pll-video0                        1        1        1   297000000
> > > >         0     0  50000
> > > >        hdmi                           0        0        0   297000000
> > > >         0     0  50000
> > > >        tcon1                          0        0        0   297000000
> > > >         0     0  50000
> > > >        pll-mipi                       1        1        1   178200000
> > > >         0     0  50000
> > > >           tcon0                       2        2        1   178200000
> > > >         0     0  50000
> > > >              tcon-pixel-clock         1        1        1    29700000
> > > >         0     0  50000
> > > >        pll-video0-2x                  0        0        0   594000000
> > > >         0     0  50000
> > >
> > > This discussion is going nowhere. I'm telling you that your patch
> > > doesn't apply the divider you want on the proper clock, and you're
> > > replying that indeed, you're applying it on the wrong clock.
> > >
> > > It might work by accident in your case, but the board I have here
> > > clearly indicates otherwise, so there's two possible way out here:
> > >
> > >   - Either you apply that divider to the TCON *module* clock, and not
> > >     the dclk
> > >
> > >   - Or you point to somewhere in the allwinner code where the bpp /
> > >     lanes divider is used for the dclk divider.
> >
> > I don't know how to proceed further on this, as you say it might work
> > in accident but I have tested this in A33, A64 and R40 with 4
> > different DSI panels and one DSI-RGB bridge. All of them do use
> > PLL_MIPI (pll_rate) and it indeed depends on bpp/lanes
> >
> > 4-lane, 24-bit: Novatek NT35596 panel
> > 4-lane, 24-bit: Feiyang, FY07024di26a30d panel
> > 4-lane, 24-bit: Bananapi-s070wv20 panel
> > 2-lane, 24-bit: Techstar,ts8550b panel
> >
> > and
> >
> > 4-lane, 24-bit, ICN6211 DSI-to-RGB bridge panel
> >
> > All above listed panels and bridges are working as per BSP and do
> > follow bpp/lanes and for DIVIDER 4 no panel is working.
>
> Look. I'm not saying that there's no issue, I'm saying that your
> patch, applied to the clock you're applying it to, doesn't make sense
> and isn't what the BSP does.

tcon-pixel clock is the rate that you want to achive on display side and
if you have 4 lanes 32bit or lanes and different bit number that you need
to have a clock that is able to put outside bits and speed equal to
pixel-clock * bits / lanes. so If you want a pixel-clock of 40 mhz
and you have 32bits and 4 lanes you need to have a clock of
40 * 32 / 4 in no-burst mode. I think that this is done but most of the display.
Now in burst mode I don't know how should work the calculation of the
clock for the
require bandwidth and even I understand your comment I would like to have your
clock tree after you boot on the display side and if it is possible I
want to assemble a kit
like you have.

>
> You can keep on arguing that your patch is perfect as is, but the fact
> that there's regressions proves otherwise.
>

Well when you push your code you said that you have tested on more
then one display.
Can I know where are the others?

> > The panels/bridges I have has tested in BSP and as you mentioned in

> > another mail, your panel is not tested in BSP - this is the only
> > difference. I did much reverse-engineering on PLL_MIPI clocking in BSP
> > so I'm afraid what can I do next on this, If you want to look further
> > on BSP I would suggest to verify on pll_rate side. If you feel
> > anything I'm missing please let me know.
>
> I already told you how we can make some progress in the mail you
> quoted, but you chose to ignore that.
>

Yes, the idea is to make progress. Thank you about your helping

Michael

> Until there's been some progress on either points mentionned above,
> I'm just going to stop answering on this topic.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Hi

On Thu, Jul 11, 2019 at 7:43 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Maxime
>
> On Thu, Jul 11, 2019 at 2:23 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, Jul 05, 2019 at 07:52:27PM +0200, Michael Nazzareno Trimarchi wrote:
> > > On Wed, Jul 3, 2019 at 1:49 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Tue, Jun 25, 2019 at 09:00:36PM +0530, Jagan Teki wrote:
> > > > > On Tue, Jun 25, 2019 at 8:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 20, 2019 at 11:57:44PM +0530, Jagan Teki wrote:
> > > > > > > On Fri, Jun 14, 2019 at 7:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jun 05, 2019 at 01:03:16PM +0530, Jagan Teki wrote:
> > > > > > > > > On Wed, Jun 5, 2019 at 12:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I've reordered the mail a bit to work on chunks
> > > > > > > > > >
> > > > > > > > > > On Fri, May 24, 2019 at 03:37:42PM +0530, Jagan Teki wrote:
> > > > > > > > > > > > I wish it was in your commit log in the first place, instead of having
> > > > > > > > > > > > to exchange multiple mails over this.
> > > > > > > > > > > >
> > > > > > > > > > > > However, I don't think that's quite true, and it might be a bug in
> > > > > > > > > > > > Allwinner's implementation (or rather something quite confusing).
> > > > > > > > > > > >
> > > > > > > > > > > > You're right that the lcd_rate and pll_rate seem to be generated from
> > > > > > > > > > > > the pixel clock, and it indeed looks like the ratio between the pixel
> > > > > > > > > > > > clock and the TCON dotclock is defined through the number of bits per
> > > > > > > > > > > > lanes.
> > > > > > > > > > > >
> > > > > > > > > > > > However, in this case, dsi_rate is actually the same than lcd_rate,
> > > > > > > > > > > > since pll_rate is going to be divided by dsi_div:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L791
> > > > > > > > > > > >
> > > > > > > > > > > > Since lcd_div is 1, it also means that in this case, dsi_rate ==
> > > > > > > > > > > > dclk_rate.
> > > > > > > > > > > >
> > > > > > > > > > > > The DSI module clock however, is always set to 148.5 MHz. Indeed, if
> > > > > > > > > > > > we look at:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L804
> > > > > > > > > > > >
> > > > > > > > > > > > We can see that the rate in clk_info is used if it's different than
> > > > > > > > > > > > 0. This is filled by disp_al_lcd_get_clk_info, which, in the case of a
> > > > > > > > > > > > DSI panel, will hardcode it to 148.5 MHz:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L164
> > > > > > > > > > >
> > > > > > > > > > > Let me explain, something more.
> > > > > > > > > > >
> > > > > > > > > > > According to bsp there are clk_info.tcon_div which I will explain below.
> > > > > > > > > > > clk_info.dsi_div which is dynamic and it depends on bpp/lanes, so it
> > > > > > > > > > > is 6 for 24bpp and 4 lanes devices.
> > > > > > > > > > >
> > > > > > > > > > > PLL rate here depends on dsi_div (not tcon_div)
> > > > > > > > > > >
> > > > > > > > > > > Code here
> > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L784
> > > > > > > > > > >
> > > > > > > > > > > is computing the actual set rate, which depends on dsi_rate.
> > > > > > > > > > >
> > > > > > > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > > > > > > dsi_rate = pll_rate / clk_info.dsi_div;
> > > > > > > > > > >
> > > > > > > > > > > Say if the dclk_rate 148MHz then the dsi_rate is 888MHz which set rate
> > > > > > > > > > > for above link you mentioned.
> > > > > > > > > > >
> > > > > > > > > > > Here are the evidence with some prints.
> > > > > > > > > > >
> > > > > > > > > > > https://gist.github.com/openedev/9bae2d87d2fcc06b999fe48c998b7043
> > > > > > > > > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > > > > > > > > >
> > > > > > > > > > Ok, so we agree up to this point, and the prints confirm that the
> > > > > > > > > > analysis above is the right one.
> > > > > > > > > >
> > > > > > > > > > > > So, the DSI clock is set to this here:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L805
> > > > > > > > > >
> > > > > > > > > > Your patch doesn't address that, so let's leave that one alone.
> > > > > > > > >
> > > > > > > > > Basically this is final pll set rate when sun4i_dotclock.c called the
> > > > > > > > > desired rate with ccu_nkm.c so it ended the final rate with parent as
> > > > > > > > > Line 8 of
> > > > > > > > > https://gist.github.com/openedev/700de2e3701b2bf3ad1aa0f0fa862c9a
> > > > > > > >
> > > > > > > > If that's important to the driver, it should be set explicitly then,
> > > > > > > > and not work by accident.
> > > > > > > >
> > > > > > > > > > > > The TCON *module* clock (the one in the clock controller) has been set
> > > > > > > > > > > > to lcd_rate (so the pixel clock times the number of bits per lane) here:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L800
> > > > > > > > > > > >
> > > > > > > > > > > > And the PLL has been set to the same rate here:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L794
> > > > > > > > > > > >
> > > > > > > > > > > > Let's take a step back now: that function we were looking at,
> > > > > > > > > > > > lcd_clk_config, is called by lcd_clk_enable, which is in turn called
> > > > > > > > > > > > by disp_lcd_enable here:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1328
> > > > > > > > > > > >
> > > > > > > > > > > > The next function being called is disp_al_lcd_cfg, and that function
> > > > > > > > > > > > will hardcode the TCON dotclock divider to 4, here:
> > > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L240
> > > > > > > > > > >
> > > > > > > > > > > tcon_div from BSP point-of-view of there are two variants
> > > > > > > > > > > 00) clk_info.tcon_div which is 4 and same is set the divider position
> > > > > > > > > > > in SUN4I_TCON0_DCLK_REG (like above link refer)
> > > > > > > > > > > 01) tcon_div which is 4 and used for edge timings computation
> > > > > > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L12
> > > > > > > > > > >
> > > > > > > > > > > The real reason for 01) is again 4 is they set the divider to 4 in 00)
> > > > > > > > > > > which is technically wrong because the dividers which used during
> > > > > > > > > > > dotclock in above (dsi_div) should be used here as well. Since there
> > > > > > > > > > > is no dynamic way of doing this BSP hard-coding these values.
> > > > > > > > > > >
> > > > > > > > > > > Patches 5,6,7 on this series doing this
> > > > > > > > > > > https://patchwork.freedesktop.org/series/60847/
> > > > > > > > > > >
> > > > > > > > > > > Hope this explanation helps?
> > > > > > > > > >
> > > > > > > > > > It doesn't.
> > > > > > > > > >
> > > > > > > > > > The clock tree is this one:
> > > > > > > > > >
> > > > > > > > > > PLL(s) -> TCON module clock -> TCON dotclock.
> > > > > > > > > >
> > > > > > > > > > The links I mentioned above show that the clock set to lcd_rate is the
> > > > > > > > > > TCON module clocks (and it should be the one taking the bpp and lanes
> > > > > > > > > > into account), while the TCON dotclock uses a fixed divider of 4.
> > > > > > > > >
> > > > > > > > > Sorry, I can argue much other-than giving some code snips, according to [1]
> > > > > > > > >
> > > > > > > > > 00) Line 785, 786 with dclk_rate 148000000
> > > > > > > > >
> > > > > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > > > > pll_rate = lcd_rate * clk_info.lcd_div;
> > > > > > > > >
> > > > > > > > > Since dsi_div is 6 (bpp/lanes), lcd_div 1
> > > > > > > > >
> > > > > > > > > lcd_rate = 888000000, pll_rate = 888000000
> > > > > > > > >
> > > > > > > > > 01)  Line 801, 804 are final rates computed as per clock driver (say
> > > > > > > > > ccu_nkm in mainline)
> > > > > > > > >
> > > > > > > > > lcd_rate_set=891000000
> > > > > > > > >
> > > > > > > > > As per your comments if it would be 4 then the desired numbers are
> > > > > > > > > would be 592000000 not 888000000.
> > > > > > > > >
> > > > > > > > > This is what I'm trying to say in all mails, and same as verified with
> > > > > > > > > 2-lanes devices as well where the dsi_div is 12 so the final rate is
> > > > > > > > > 290MHz * 12
> > > > > > > >
> > > > > > > > In the code you sent, you're forcing a divider on the internal TCON
> > > > > > > > clock, while that one is fixed in the BSP.
> > > > > > > >
> > > > > > > > There's indeed the bpp / lanes divider, but it's used in the *parent*
> > > > > > > > clock of the one you're changing.
> > > > > > > >
> > > > > > > > And the dsi0_clk clock you pointed out in the code snippet is yet
> > > > > > > > another clock, the MIPI DSI module clock.
> > > > > > >
> > > > > > > Correct, look like I refereed wrong reference in the above mail. sorry
> > > > > > > for the noise.
> > > > > > >
> > > > > > > Actually I'm trying to explain about pll_rate here which indeed
> > > > > > > depends on dsi.div
> > > > > > > https://github.com/BPI-SINOVOIP/BPI-M64-bsp/blob/master/linux-sunxi/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L786
> > > > > > >
> > > > > > > lcd_rate = dclk_rate * clk_info.dsi_div;
> > > > > > > pll_rate = lcd_rate * clk_info.lcd_div;
> > > > > > >
> > > > > > > Say
> > > > > > >
> > > > > > > 1) For 148MHz dclk_rate with dsi_div is 6 (24/4) lcd_div is 1 which
> > > > > > > resulting pll_rate is 888MHz.
> > > > > > >
> > > > > > > 2) For 30MHz dclk_rate with 4 lane and 24 RGB the resulting pll_rate is 180MHz
> > > > > > >
> > > > > > > 3) For 27.5MHz dclk_rate with 2 lane and 24 RGB the resulting pll_rate is 330MHz
> > > > > > >
> > > > > > > Here is the few more logs in code, for case 2)
> > > > > > >
> > > > > > > [    1.920441] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > > > > > > [    1.920505] ideal = 180000000, rounded = 178200000
> > > > > > > [    1.920509] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > > > > > > [    1.920514] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> > > > > > > [    1.920532] ideal = 1800ls and one DSI-RGB bridge. All of them do use
> > > > > PLL_MIPI (pll_rate) and it indeed depends on bpp/lanes
> > > > >00000, rounded = 178200000
> > > > > > > [    1.920535] sun4i_dclk_round_rate: div = 6 rate = 29700000
> > > > > > > [    1.920572] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > > > > > > [    1.920576] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> > > > > > > [    1.920597] rate = 178200000
> > > > > > > [    1.920599] parent_rate = 297000000
> > > > > > > [    1.920602] reg = 0x90c00000
> > > > > > > [    1.920605] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
> > > > > > > [    1.920609] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
> > > > > > > [    1.920612] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
> > > > > > > [    1.920958] sun4i_dclk_set_rate div 6
> > > > > > > [    1.920966] sun4i_dclk_recalc_rate: val = 6, rate = 29700000
> > > > > > >
> > > > > > > and clk_summary:
> > > > > > >
> > > > > > >     pll-video0                        1        1        1   297000000
> > > > > > >         0     0  50000
> > > > > > >        hdmi                           0        0        0   297000000
> > > > > > >         0     0  50000
> > > > > > >        tcon1                          0        0        0   297000000
> > > > > > >         0     0  50000
> > > > > > >        pll-mipi                       1        1        1   178200000
> > > > > > >         0     0  50000
> > > > > > >           tcon0                       2        2        1   178200000
> > > > > > >         0     0  50000
> > > > > > >              tcon-pixel-clock         1        1        1    29700000
> > > > > > >         0     0  50000
> > > > > > >        pll-video0-2x                  0        0        0   594000000
> > > > > > >         0     0  50000
> > > > > >
> > > > > > This discussion is going nowhere. I'm telling you that your patch
> > > > > > doesn't apply the divider you want on the proper clock, and you're
> > > > > > replying that indeed, you're applying it on the wrong clock.
> > > > > >
> > > > > > It might work by accident in your case, but the board I have here
> > > > > > clearly indicates otherwise, so there's two possible way out here:
> > > > > >
> > > > > >   - Either you apply that divider to the TCON *module* clock, and not
> > > > > >     the dclk
> > > > > >
> > > > > >   - Or you point to somewhere in the allwinner code where the bpp /
> > > > > >     lanes divider is used for the dclk divider.
> > > > >
> > > > > I don't know how to proceed further on this, as you say it might work
> > > > > in accident but I have tested this in A33, A64 and R40 with 4
> > > > > different DSI panels and one DSI-RGB bridge. All of them do use
> > > > > PLL_MIPI (pll_rate) and it indeed depends on bpp/lanes
> > > > >
> > > > > 4-lane, 24-bit: Novatek NT35596 panel
> > > > > 4-lane, 24-bit: Feiyang, FY07024di26a30d panel
> > > > > 4-lane, 24-bit: Bananapi-s070wv20 panel
> > > > > 2-lane, 24-bit: Techstar,ts8550b panel
> > > > >
> > > > > and
> > > > >
> > > > > 4-lane, 24-bit, ICN6211 DSI-to-RGB bridge panel
> > > > >
> > > > > All above listed panels and bridges are working as per BSP and do
> > > > > follow bpp/lanes and for DIVIDER 4 no panel is working.
> > > >
> > > > Look. I'm not saying that there's no issue, I'm saying that your
> > > > patch, applied to the clock you're applying it to, doesn't make sense
> > > > and isn't what the BSP does.
> > >
> > > tcon-pixel clock is the rate that you want to achive on display side
> > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > you need to have a clock that is able to put outside bits and speed
> > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > the display.
> >
> > So this is what the issue is then?
> >
> > This one does make sense, and you should just change the rate in the
> > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> >
> > I'm still wondering why that hasn't been brought up in either the
> > discussion or the commit log before though.
> >
> Something like this?
>
> drivers/gpu/drm/sun4i/sun4i_tcon.c     | 20 +++++++++++---------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  2 --
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 64c43ee6bd92..42560d5c327c 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> drm_display_mode *mode,
>  }
>
>  static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> -                                       const struct drm_display_mode *mode)
> +                                       const struct drm_display_mode *mode,
> +                                       u32 tcon_mul)
>  {
>         /* Configure the dot clock */
> -       clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> +       clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
>
>         /* Set the resolution */
>         regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> sun4i_tcon *tcon,
>         u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>         u8 lanes = device->lanes;
>         u32 block_space, start_delay;
> -       u32 tcon_div;
> +       u32 tcon_div, tcon_mul;
>
> -       tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> -       tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> +       tcon->dclk_min_div = 4;
> +       tcon->dclk_max_div = 127;
>
> -       sun4i_tcon0_mode_set_common(tcon, mode);
> +       tcon_mul = bpp / lanes;
> +       sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
>

Maybe wrong clock ;) but is this your idea?

Michael
>         /* Set dithering if needed */
>         sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> sun4i_tcon *tcon,
>          */
>         regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
>         tcon_div &= GENMASK(6, 0);
> -       block_space = mode->htotal * bpp / (tcon_div * lanes);
> +       block_space = mode->htotal * tcon_div * tcon_mul;
>         block_space -= mode->hdisplay + 40;
>
>         regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> sun4i_tcon *tcon,
>
>         tcon->dclk_min_div = 7;
>         tcon->dclk_max_div = 7;
> -       sun4i_tcon0_mode_set_common(tcon, mode);
> +       sun4i_tcon0_mode_set_common(tcon, mode, 1);
>
>         /* Set dithering if needed */
>         sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> sun4i_tcon *tcon,
>
>         tcon->dclk_min_div = 6;
>         tcon->dclk_max_div = 127;
> -       sun4i_tcon0_mode_set_common(tcon, mode);
> +       sun4i_tcon0_mode_set_common(tcon, mode, 1);
>
>         /* Set dithering if needed */
>         sun4i_tcon0_mode_set_dithering(tcon, connector);
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index 5c3ad5be0690..a07090579f84 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -13,8 +13,6 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_mipi_dsi.h>
>
> -#define SUN6I_DSI_TCON_DIV     4
> -
>  struct sun6i_dsi {
>         struct drm_connector    connector;
>         struct drm_encoder      encoder;
>
>
> > > Now in burst mode I don't know how should work the calculation of
> > > the clock for the require bandwidth and even I understand your
> > > comment I would like to have your clock tree after you boot on the
> > > display side and if it is possible I want to assemble a kit like you
> > > have.
> >
> > The setup is probably going to be a bit difficult to reproduce, it's a
> > prototype that I have that can't really be found anywhere. Jagan asked
> > me on IRC for the reference, and he found the part, but it's unclear
> > to me if it can be easily adapted to a common board.
> >
> > However, I'm not even sure we need this. I'll test the next version
> > and let you know if it works.
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |