Hi

On Sat, Jul 20, 2019 at 11:32 AM Maxime Ripard
[off-list ref] wrote:

On Sat, Jul 20, 2019 at 12:46:27PM +0530, Jagan Teki wrote:

quoted

On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard
[off-list ref] wrote:

quoted

On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:

quoted

quoted

quoted

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);

        /* 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;

I had more something like this in mind:
http://code.bulix.org/nlp5a4-803511

Worth to look at it. was it working on your panel? meanwhile I will check it.

I haven't tested it.

quoted

We have updated with below change [1], seems working on but is
actually checking the each divider as before start with 4... till 127.

This new approach, is start looking the best divider from 4.. based on
the idea vs rounded it will ended up best divider like [2]

But why?

I mean, it's not like it's the first time I'm asking this...

If the issue is what Micheal described, then the divider has nothing
to do with it. We've had that discussion over and over again.

Sorry to quick copy and paste but my idea is somenthing like:

---
 drivers/gpu/drm/sun4i/sun4i_dotclock.c | 5 ++++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c     | 9 ++++++---
 drivers/gpu/drm/sun4i/sun4i_tcon.h     | 1 +
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c

b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
index 417ade3d2565..b8be67cbb037 100644

--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c

@@ -74,12 +74,15 @@ static long sun4i_dclk_round_rate(struct clk_hw

*hw, unsigned long rate,
        struct sun4i_tcon *tcon = dclk->tcon;
        unsigned long best_parent = 0;
        u8 best_div = 1;
+       u64 ideal = (u64)rate * tcon->dclk_mul;
        int i;

        for (i = tcon->dclk_min_div; i <= tcon->dclk_max_div; i++) {
-               u64 ideal = (u64)rate * i;
                unsigned long rounded;

+               if (!tcon->dclk_mul)
+                       ideal = (u64)rate * i;
+
                /*
                 * ideal has overflowed the max value that can be stored in an
                 * unsigned long, and every clk operation we might do on a

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c

b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 64c43ee6bd92..3301952bcf32 100644

--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c

@@ -337,8 +337,9 @@ static void sun4i_tcon0_mode_set_cpu(struct

sun4i_tcon *tcon,
        u32 block_space, start_delay;
        u32 tcon_div;

-       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;
+       tcon->dclk_mul = bpp / lanes;

        sun4i_tcon0_mode_set_common(tcon, mode);

@@ -366,7 +367,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->dclk_mul;
        block_space -= mode->hdisplay + 40;

        regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,

@@ -408,6 +409,7 @@ static void sun4i_tcon0_mode_set_lvds(struct

sun4i_tcon *tcon,

        tcon->dclk_min_div = 7;
        tcon->dclk_max_div = 7;
+       tcon->dclk_mul = 0;
        sun4i_tcon0_mode_set_common(tcon, mode);

        /* Set dithering if needed */

@@ -487,6 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct

sun4i_tcon *tcon,

        tcon->dclk_min_div = 6;
        tcon->dclk_max_div = 127;
+       tcon->dclk_mul = 0;
        sun4i_tcon0_mode_set_common(tcon, mode);

        /* Set dithering if needed */

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h

b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index f9f1fe80b206..44b69533778e 100644

--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h

@@ -248,6 +248,7 @@ struct sun4i_tcon {
        struct clk                      *dclk;
        u8                              dclk_max_div;
        u8                              dclk_min_div;
+       u8                              dclk_mul;

        /* Reset control */
        struct reset_control            *lcd_rst;

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;

-- 
2.17.1


Michael

> So you need to come with some argument and proof that the divider of
> that clock need to change. Otherwise, stop trying to make that happen:
> it won't.
>
> 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               |

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