[PATCH 0/4] drm/sun4i: dsi: Add burst mode support

[PATCH 0/4] drm/sun4i: dsi: Add burst mode support

[Maxime Ripard]

Hi,

Here is a series implementing the burst mode support for DSI.

It's been tested on an A33 board with the panel supported on the 4th patch,
which should remove all quirks due to a different SoC from the equation.

Let me know what you think,
Maxime

Konstantin Sudakov (2):
  drm/sun4i: dsi: Add burst support
  drm/panel: Add Rondo RB070D30 panel

Maxime Ripard (2):
  drm/sun4i: dsi: Restrict DSI tcon clock divider
  drm/sun4i: dsi: Change the start delay calculation

 drivers/gpu/drm/panel/Kconfig                |   9 +-
 drivers/gpu/drm/panel/Makefile               |   1 +-
 drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c           |   4 +-
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c       | 185 ++++++++++----
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h       |   2 +-
 6 files changed, 414 insertions(+), 45 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c

base-commit: f6028e7e48b0f3865b0837d400ca783d3d200b62
-- 
git-series 0.9.1

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

[PATCH 1/4] drm/sun4i: dsi: Restrict DSI tcon clock divider

[Maxime Ripard]

The current code allows the TCON clock divider to have a range between 4
and 127 when feeding the DSI controller.

The only display supported so far had a display clock rate that ended up
using a divider of 4, but testing with other displays show that only 4
seems to be functional.

This also aligns with what Allwinner is doing in their BSP, so let's just
hardcode that we want a divider of 4 when using the DSI output.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c     | 4 ++--
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 0420f5c978b9..bee73ead732a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -341,8 +341,8 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
 	u32 block_space, start_delay;
 	u32 tcon_div;
 
-	tcon->dclk_min_div = 4;
-	tcon->dclk_max_div = 127;
+	tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
+	tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
 
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index dbbc5b3ecbda..6d4a3c0fd9b5 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -13,6 +13,8 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_mipi_dsi.h>
 
+#define SUN6I_DSI_TCON_DIV	4
+
 struct sun6i_dphy {
 	struct clk		*bus_clk;
 	struct clk		*mod_clk;
-- 
git-series 0.9.1

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

Re: [PATCH 1/4] drm/sun4i: dsi: Restrict DSI tcon clock divider

[Jagan Teki]

On Wed, Jan 23, 2019 at 9:24 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> The current code allows the TCON clock divider to have a range between 4
> and 127 when feeding the DSI controller.
>
> The only display supported so far had a display clock rate that ended up
> using a divider of 4, but testing with other displays show that only 4
> seems to be functional.
>
> This also aligns with what Allwinner is doing in their BSP, so let's just
> hardcode that we want a divider of 4 when using the DSI output.

So, bad that existing dotclock is unable to produce the desired
divider logic. I have tried certain possibilities wrt changing the
clock rate in pll-mipi via min_rate, but it eventually not working
with all panels.

Yes, I have seen Allwinner BSP has logic to this dividers wrt SoC's.
for A33 it's default to 4 but for A64 it computed in other-way wrt
format and lanes.

Let me send the change wrt format and lanes, I'm hoping the same will
work with existing display or If you have any inputs please let me
know.

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

Re: [PATCH 1/4] drm/sun4i: dsi: Restrict DSI tcon clock divider

[Paul Kocialkowski]

Hi,

On Wed, 2019-01-23 at 16:54 +0100, Maxime Ripard wrote:
> The current code allows the TCON clock divider to have a range between 4
> and 127 when feeding the DSI controller.
> 
> The only display supported so far had a display clock rate that ended up
> using a divider of 4, but testing with other displays show that only 4
> seems to be functional.
> 
> This also aligns with what Allwinner is doing in their BSP, so let's just
> hardcode that we want a divider of 4 when using the DSI output.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c     | 4 ++--
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 0420f5c978b9..bee73ead732a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -341,8 +341,8 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
>  	u32 block_space, start_delay;
>  	u32 tcon_div;
>  
> -	tcon->dclk_min_div = 4;
> -	tcon->dclk_max_div = 127;
> +	tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> +	tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
>  
>  	sun4i_tcon0_mode_set_common(tcon, mode);
>  
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index dbbc5b3ecbda..6d4a3c0fd9b5 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -13,6 +13,8 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_mipi_dsi.h>
>  
> +#define SUN6I_DSI_TCON_DIV	4
> +
>  struct sun6i_dphy {
>  	struct clk		*bus_clk;
>  	struct clk		*mod_clk;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


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

[PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Maxime Ripard]

The current calculation for the video start delay in the current DSI driver
is that it is the total vertical size, minus the backporch and sync length,
plus 1.

However, the Allwinner code has it as the active vertical size, plus the
back porch and the sync length. This doesn't make any difference on the
only panel it has been tested with so far, since in that particular case
the front porch is equal to the sum of the back porch and sync length.

This is not the case for all panels, obviously, so we need to fix it. Since
the Allwinner code has a bunch of extra code to deal with out of bounds
values, so let's add them as well.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 380fc527a707..e3e4ba90c059 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
 static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
 					   struct drm_display_mode *mode)
 {
-	return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;
+	u16 delay = (mode->vsync_end + 1) % mode->vtotal;
+
+	if (!delay)
+		delay = 1;
+
+	return delay;
 }
 
 static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
-- 
git-series 0.9.1

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

Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Paul Kocialkowski]

Hi,

On Wed, 2019-01-23 at 16:54 +0100, Maxime Ripard wrote:
> The current calculation for the video start delay in the current DSI driver
> is that it is the total vertical size, minus the backporch and sync length,
> plus 1.
> 
> However, the Allwinner code has it as the active vertical size, plus the
> back porch and the sync length. This doesn't make any difference on the
> only panel it has been tested with so far, since in that particular case
> the front porch is equal to the sum of the back porch and sync length.
> 
> This is not the case for all panels, obviously, so we need to fix it. Since
> the Allwinner code has a bunch of extra code to deal with out of bounds
> values, so let's add them as well.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 380fc527a707..e3e4ba90c059 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
>  static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
>  					   struct drm_display_mode *mode)
>  {
> -	return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;
> +	u16 delay = (mode->vsync_end + 1) % mode->vtotal;
> +
> +	if (!delay)
> +		delay = 1;

For increased clarity, you might want to change this last block to:

delay = max(delay, 1);

Cheers,

Paul

> +	return delay;
>  }
>  
>  static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


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

Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Chen-Yu Tsai]

On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> The current calculation for the video start delay in the current DSI driver
> is that it is the total vertical size, minus the backporch and sync length,
> plus 1.
>
> However, the Allwinner code has it as the active vertical size, plus the
> back porch and the sync length. This doesn't make any difference on the
> only panel it has been tested with so far, since in that particular case
> the front porch is equal to the sum of the back porch and sync length.
>
> This is not the case for all panels, obviously, so we need to fix it. Since
> the Allwinner code has a bunch of extra code to deal with out of bounds
> values, so let's add them as well.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 380fc527a707..e3e4ba90c059 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
>  static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
>                                            struct drm_display_mode *mode)
>  {
> -       return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;

According to the diagram in include/drm/drm_modes.h ,
This is active region plus back porch plus 1, as

    sync_end - display = length of front porch and sync

> +       u16 delay = (mode->vsync_end + 1) % mode->vtotal;

And this actually means

    (length of active region and front porch and sync pulse plus 1) %
total length

So I don't really understand what's happening here. And the modulus
is unexplained.

> +
> +       if (!delay)
> +               delay = 1;

Same comment as Paul.

ChenYu

> +
> +       return delay;
>  }
>
>  static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
> --
> git-series 0.9.1
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Chen-Yu Tsai]

On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > The current calculation for the video start delay in the current DSI driver
> > is that it is the total vertical size, minus the backporch and sync length,
> > plus 1.
> >
> > However, the Allwinner code has it as the active vertical size, plus the
> > back porch and the sync length. This doesn't make any difference on the
> > only panel it has been tested with so far, since in that particular case
> > the front porch is equal to the sum of the back porch and sync length.
> >
> > This is not the case for all panels, obviously, so we need to fix it. Since
> > the Allwinner code has a bunch of extra code to deal with out of bounds
> > values, so let's add them as well.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 380fc527a707..e3e4ba90c059 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
> >  static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
> >                                            struct drm_display_mode *mode)
> >  {
> > -       return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;
>
> According to the diagram in include/drm/drm_modes.h ,
> This is active region plus back porch plus 1, as
>
>     sync_end - display = length of front porch and sync
>
> > +       u16 delay = (mode->vsync_end + 1) % mode->vtotal;
>
> And this actually means
>
>     (length of active region and front porch and sync pulse plus 1) %
> total length
>
> So I don't really understand what's happening here. And the modulus
> is unexplained.

Attempting to make sense of this. Allwinner's A64 BSP the following
which uses their definitions for y, vbp, vt:

    s32 start_delay = panel->lcd_vt - panel->lcd_y -10;
    u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp;
    u32 dsi_start_delay;

    /* put start_delay to tcon. set ready sync early to dramfreq, so
set start_delay 1 */
    start_delay = 1;

    dsi_start_delay = panel->lcd_vt - vfp + start_delay;
    if (dsi_start_delay > panel->lcd_vt)
           dsi_start_delay -= panel->lcd_vt;
    if (dsi_start_delay==0)
           dsi_start_delay = 1;

This can be converted to

    dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal -
mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) %
mode->vtotal)

and simplified to

    dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay -
mode->vsync_start) + 1) % mode->vtotal)

and reordered to the following

    dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start -
mode->vdisplay) + 1) % mode->vtotal)

which means total length minus front porch, or length of active
portion plus back porch plus sync.
Based on your commit message, the last derivation is what you want.

ChenYu

> > +
> > +       if (!delay)
> > +               delay = 1;
>
> Same comment as Paul.
>
> ChenYu
>
> > +
> > +       return delay;
> >  }
> >
> >  static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
> > --
> > git-series 0.9.1
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4472 bytes --]

Hi Chen-Yu,

On Wed, Feb 06, 2019 at 12:48:21AM +0800, Chen-Yu Tsai wrote:
> On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote:
> >
> > On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > >
> > > The current calculation for the video start delay in the current DSI driver
> > > is that it is the total vertical size, minus the backporch and sync length,
> > > plus 1.
> > >
> > > However, the Allwinner code has it as the active vertical size, plus the
> > > back porch and the sync length. This doesn't make any difference on the
> > > only panel it has been tested with so far, since in that particular case
> > > the front porch is equal to the sum of the back porch and sync length.
> > >
> > > This is not the case for all panels, obviously, so we need to fix it. Since
> > > the Allwinner code has a bunch of extra code to deal with out of bounds
> > > values, so let's add them as well.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 380fc527a707..e3e4ba90c059 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
> > >  static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
> > >                                            struct drm_display_mode *mode)
> > >  {
> > > -       return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;
> >
> > According to the diagram in include/drm/drm_modes.h ,
> > This is active region plus back porch plus 1, as
> >
> >     sync_end - display = length of front porch and sync
> >
> > > +       u16 delay = (mode->vsync_end + 1) % mode->vtotal;
> >
> > And this actually means
> >
> >     (length of active region and front porch and sync pulse plus 1) %
> > total length
> >
> > So I don't really understand what's happening here. And the modulus
> > is unexplained.
> 
> Attempting to make sense of this.

Sorry for dragging you into this

> Allwinner's A64 BSP the following which uses their definitions for
> y, vbp, vt:
> 
>     s32 start_delay = panel->lcd_vt - panel->lcd_y -10;
>     u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp;
>     u32 dsi_start_delay;
> 
>     /* put start_delay to tcon. set ready sync early to dramfreq, so
> set start_delay 1 */
>     start_delay = 1;
> 
>     dsi_start_delay = panel->lcd_vt - vfp + start_delay;
>     if (dsi_start_delay > panel->lcd_vt)
>            dsi_start_delay -= panel->lcd_vt;
>     if (dsi_start_delay==0)
>            dsi_start_delay = 1;
> 
> This can be converted to
> 
>     dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal -
> mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) %
> mode->vtotal)
> 
> and simplified to
> 
>     dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay -
> mode->vsync_start) + 1) % mode->vtotal)
> 
> and reordered to the following
> 
>     dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start -
> mode->vdisplay) + 1) % mode->vtotal)

Where did you find that Allwinner's back porch is including the sync
length?

If we treat the bp as vtotal - vsync_end as we should (and yet I often
fail to), then we end up with (leaving out the modulo for now):

dsi_start_delay = max(1, mode->vtotal - (mode->vtotal - mode->vdisplay -
                                         (mode->vtotal - mode->vsync_end)) + 1)

which equals (if we remove the first mode->vtotal - mode->vtotal)

dsi_start_delay = max(1, mode->vdisplay + (mode->vtotal - mode->vsync_end) + 1)

Otherwise,

Then yeah, I end up with the same results than you, and on the Rondo
display I have, the start delay difference is of one between the
formula in this patch and the one you came up with, so I think we're
good.

The BPI-M2M display timings would need to be adjusted as well, since
the timings were created from the FEX file and the BP was assumed to
be only the BP. We don't have the datasheet for this one though, so
it's difficult to check.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation

[Chen-Yu Tsai]

On Wed, Feb 6, 2019 at 10:12 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi Chen-Yu,
>
> On Wed, Feb 06, 2019 at 12:48:21AM +0800, Chen-Yu Tsai wrote:
> > On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
> > > <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > The current calculation for the video start delay in the current DSI driver
> > > > is that it is the total vertical size, minus the backporch and sync length,
> > > > plus 1.
> > > >
> > > > However, the Allwinner code has it as the active vertical size, plus the
> > > > back porch and the sync length. This doesn't make any difference on the
> > > > only panel it has been tested with so far, since in that particular case
> > > > the front porch is equal to the sum of the back porch and sync length.
> > > >
> > > > This is not the case for all panels, obviously, so we need to fix it. Since
> > > > the Allwinner code has a bunch of extra code to deal with out of bounds
> > > > values, so let's add them as well.
> > > >
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > index 380fc527a707..e3e4ba90c059 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > @@ -357,7 +357,12 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
> > > >  static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
> > > >                                            struct drm_display_mode *mode)
> > > >  {
> > > > -       return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;
> > >
> > > According to the diagram in include/drm/drm_modes.h ,
> > > This is active region plus back porch plus 1, as
> > >
> > >     sync_end - display = length of front porch and sync
> > >
> > > > +       u16 delay = (mode->vsync_end + 1) % mode->vtotal;
> > >
> > > And this actually means
> > >
> > >     (length of active region and front porch and sync pulse plus 1) %
> > > total length
> > >
> > > So I don't really understand what's happening here. And the modulus
> > > is unexplained.
> >
> > Attempting to make sense of this.
>
> Sorry for dragging you into this
>
> > Allwinner's A64 BSP the following which uses their definitions for
> > y, vbp, vt:
> >
> >     s32 start_delay = panel->lcd_vt - panel->lcd_y -10;
> >     u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp;
> >     u32 dsi_start_delay;
> >
> >     /* put start_delay to tcon. set ready sync early to dramfreq, so
> > set start_delay 1 */
> >     start_delay = 1;
> >
> >     dsi_start_delay = panel->lcd_vt - vfp + start_delay;
> >     if (dsi_start_delay > panel->lcd_vt)
> >            dsi_start_delay -= panel->lcd_vt;
> >     if (dsi_start_delay==0)
> >            dsi_start_delay = 1;
> >
> > This can be converted to
> >
> >     dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal -
> > mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) %
> > mode->vtotal)
> >
> > and simplified to
> >
> >     dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay -
> > mode->vsync_start) + 1) % mode->vtotal)
> >
> > and reordered to the following
> >
> >     dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start -
> > mode->vdisplay) + 1) % mode->vtotal)
>
> Where did you find that Allwinner's back porch is including the sync
> length?

I believe that was the case with the TCON? And since Allwinner's code
take the values from the FEX files directly for both TCON and DSI,
it should be the same.

There's also some dead code in the BSP that alludes to it, such as the
function tcon1_cfg_ex() in drivers/video/de/lowlevel_sun50iw1/de_lcd.c
from the A64 BSP.

> If we treat the bp as vtotal - vsync_end as we should (and yet I often
> fail to), then we end up with (leaving out the modulo for now):
>
> dsi_start_delay = max(1, mode->vtotal - (mode->vtotal - mode->vdisplay -
>                                          (mode->vtotal - mode->vsync_end)) + 1)
>
> which equals (if we remove the first mode->vtotal - mode->vtotal)
>
> dsi_start_delay = max(1, mode->vdisplay + (mode->vtotal - mode->vsync_end) + 1)
>
> Otherwise,
>
> Then yeah, I end up with the same results than you, and on the Rondo
> display I have, the start delay difference is of one between the
> formula in this patch and the one you came up with, so I think we're
> good.
>
> The BPI-M2M display timings would need to be adjusted as well, since
> the timings were created from the FEX file and the BP was assumed to
> be only the BP. We don't have the datasheet for this one though, so
> it's difficult to check.

Let's just see if it still behaves correctly.

ChenYu

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

[PATCH 3/4] drm/sun4i: dsi: Add burst support

[Maxime Ripard]

From: Konstantin Sudakov <k.sudakov@integrasources.com>

The current driver doesn't support the DSI burst operation mode.

Let's add the needed quirks to make it work.

Signed-off-by: Konstantin Sudakov <k.sudakov@integrasources.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 178 +++++++++++++++++++-------
 1 file changed, 136 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index e3e4ba90c059..6840b3512a45 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -23,7 +23,9 @@
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_panel.h>
 
+#include "sun4i_crtc.h"
 #include "sun4i_drv.h"
+#include "sun4i_tcon.h"
 #include "sun6i_mipi_dsi.h"
 
 #include <video/mipi_display.h>
@@ -32,6 +34,8 @@
 #define SUN6I_DSI_CTL_EN			BIT(0)
 
 #define SUN6I_DSI_BASIC_CTL_REG		0x00c
+#define SUN6I_DSI_BASIC_CTL_TRAIL_INV(n)		(((n) & 0xf) << 4)
+#define SUN6I_DSI_BASIC_CTL_TRAIL_FILL		BIT(3)
 #define SUN6I_DSI_BASIC_CTL_HBP_DIS		BIT(2)
 #define SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS		BIT(1)
 #define SUN6I_DSI_BASIC_CTL_VIDEO_BURST		BIT(0)
@@ -152,6 +156,8 @@
 
 #define SUN6I_DSI_CMD_TX_REG(n)		(0x300 + (n) * 0x04)
 
+#define SUN6I_DSI_SYNC_POINT		40
+
 enum sun6i_dsi_start_inst {
 	DSI_START_LPRX,
 	DSI_START_LPTX,
@@ -365,31 +371,101 @@ static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
 	return delay;
 }
 
+static u16 sun6i_dsi_get_line_num(struct sun6i_dsi *dsi,
+				  struct drm_display_mode *mode)
+{
+	struct mipi_dsi_device *device = dsi->device;
+	unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
+
+	return mode->htotal * Bpp / device->lanes;
+}
+
+static u16 sun6i_dsi_get_drq_edge0(struct sun6i_dsi *dsi,
+				   struct drm_display_mode *mode,
+				   u16 line_num, u16 edge1)
+{
+	u16 edge0 = edge1;
+
+	edge0 += (mode->hdisplay + 40) * SUN6I_DSI_TCON_DIV / 8;
+
+	if (edge0 > line_num)
+		return edge0 - line_num;
+
+	return 1;
+}
+
+static u16 sun6i_dsi_get_drq_edge1(struct sun6i_dsi *dsi,
+				   struct drm_display_mode *mode,
+				   u16 line_num)
+{
+	struct mipi_dsi_device *device = dsi->device;
+	unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
+	unsigned hbp = mode->htotal - mode->hsync_end;
+	u16 edge1;
+
+
+	edge1 = SUN6I_DSI_SYNC_POINT;
+	edge1 += mode->hdisplay + hbp + 20;
+	edge1 = edge1 * Bpp / device->lanes;
+
+	if (edge1 > line_num)
+		return line_num;
+
+	return edge1;
+}
+
 static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
 				  struct drm_display_mode *mode)
 {
 	struct mipi_dsi_device *device = dsi->device;
-	u32 val = 0;
+	u32 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
+	u32 tcon0_drq = 0;
+
+	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+		u16 line_num = sun6i_dsi_get_line_num(dsi, mode);
+		u16 edge0, edge1;
+
+		edge1 = sun6i_dsi_get_drq_edge1(dsi, mode, line_num);
+		edge0 = sun6i_dsi_get_drq_edge0(dsi, mode, line_num, edge1);
 
-	if ((mode->hsync_end - mode->hdisplay) > 20) {
+		regmap_write(dsi->regs, SUN6I_DSI_BURST_DRQ_REG,
+			     SUN6I_DSI_BURST_DRQ_EDGE0(edge0) |
+			     SUN6I_DSI_BURST_DRQ_EDGE1(edge1));
+
+		regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
+			     SUN6I_DSI_BURST_LINE_NUM(line_num) |
+			     SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT));
+
+		tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE;
+	} else if ((mode->hsync_end - mode->hdisplay) > 20) {
 		/* Maaaaaagic */
 		u16 drq = (mode->hsync_end - mode->hdisplay) - 20;
-
-		drq *= mipi_dsi_pixel_format_to_bpp(device->format);
+		drq *= bpp;
 		drq /= 32;
 
-		val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
-		       SUN6I_DSI_TCON_DRQ_SET(drq));
+		tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
+			SUN6I_DSI_TCON_DRQ_SET(drq);
 	}
 
-	regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
+	regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, tcon0_drq);
 }
 
 static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
 				      struct drm_display_mode *mode)
 {
+	struct mipi_dsi_device *device = dsi->device;
 	u16 delay = 50 - 1;
 
+	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+		delay = (mode->htotal - mode->hdisplay) * 150;
+		delay /= (mode->clock / 1000) * 8;
+		delay -= 50;
+	}
+
+	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
+			 2 << (4 * DSI_INST_ID_LP11) |
+			 3 << (4 * DSI_INST_ID_DLY));
+
 	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
 		     SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
 		     SUN6I_DSI_INST_LOOP_NUM_N1(delay));
@@ -456,48 +532,66 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 	struct mipi_dsi_device *device = dsi->device;
 	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
 	u16 hbp, hfp, hsa, hblk, vblk;
+	u32 basic_ctl = 0;
 	size_t bytes;
 	u8 *buffer;
 
 	/* Do all timing calculations up front to allocate buffer space */
 
-	/*
-	 * A sync period is composed of a blanking packet (4 bytes +
-	 * payload + 2 bytes) and a sync event packet (4 bytes). Its
-	 * minimal size is therefore 10 bytes
-	 */
+	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+		hsa = 0;
+		hbp = 0;
+		hfp = 0;
+		hblk = mode->hdisplay * Bpp;
+		vblk = 0;
+		basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
+			    SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
+			    SUN6I_DSI_BASIC_CTL_HBP_DIS;
+
+		if (device->lanes == 4)
+			basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
+				     SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
+	} else {
+		/*
+		 * A sync period is composed of a blanking packet (4 bytes +
+		 * payload + 2 bytes) and a sync event packet (4 bytes). Its
+		 * minimal size is therefore 10 bytes
+		 */
 #define HSA_PACKET_OVERHEAD	10
-	hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
-		  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
-
-	/*
-	 * The backporch is set using a blanking packet (4 bytes +
-	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
-	 */
+		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
+			  (mode->hsync_end - mode->hsync_start) * Bpp -
+			  HSA_PACKET_OVERHEAD);
+
+		/*
+		 * The backporch is set using a blanking packet (4 bytes +
+		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+		 */
 #define HBP_PACKET_OVERHEAD	6
-	hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
-		  (mode->hsync_start - mode->hdisplay) * Bpp - HBP_PACKET_OVERHEAD);
-
-	/*
-	 * The frontporch is set using a blanking packet (4 bytes +
-	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
-	 */
+		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
+			  (mode->hsync_start - mode->hdisplay) * Bpp -
+			  HBP_PACKET_OVERHEAD);
+
+		/*
+		 * The frontporch is set using a blanking packet (4 bytes +
+		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+		 */
 #define HFP_PACKET_OVERHEAD	6
-	hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
-		  (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
-
-	/*
-	 * hblk seems to be the line + porches length.
-	 */
-	hblk = mode->htotal * Bpp - hsa;
-
-	/*
-	 * And I'm not entirely sure what vblk is about. The driver in
-	 * Allwinner BSP is using a rather convoluted calculation
-	 * there only for 4 lanes. However, using 0 (the !4 lanes
-	 * case) even with a 4 lanes screen seems to work...
-	 */
-	vblk = 0;
+		hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
+			  (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
+
+		/*
+		 * hblk seems to be the line + porches length.
+		 */
+		hblk = mode->htotal * Bpp - hsa;
+
+		/*
+		 * And I'm not entirely sure what vblk is about. The driver in
+		 * Allwinner BSP is using a rather convoluted calculation
+		 * there only for 4 lanes. However, using 0 (the !4 lanes
+		 * case) even with a 4 lanes screen seems to work...
+		 */
+		vblk = 0;
+	}
 
 	/* How many bytes do we need to send all payloads? */
 	bytes = max_t(size_t, max(max(hfp, hblk), max(hsa, hbp)), vblk);
@@ -505,7 +599,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 	if (WARN_ON(!buffer))
 		return;
 
-	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, 0);
+	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, basic_ctl);
 
 	regmap_write(dsi->regs, SUN6I_DSI_SYNC_HSS_REG,
 		     sun6i_dsi_build_sync_pkt(MIPI_DSI_H_SYNC_START,
-- 
git-series 0.9.1

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

Re: [PATCH 3/4] drm/sun4i: dsi: Add burst support

[Chen-Yu Tsai]

On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> From: Konstantin Sudakov <k.sudakov@integrasources.com>
>
> The current driver doesn't support the DSI burst operation mode.
>
> Let's add the needed quirks to make it work.
>
> Signed-off-by: Konstantin Sudakov <k.sudakov@integrasources.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 178 +++++++++++++++++++-------
>  1 file changed, 136 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index e3e4ba90c059..6840b3512a45 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -23,7 +23,9 @@
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_panel.h>
>
> +#include "sun4i_crtc.h"
>  #include "sun4i_drv.h"
> +#include "sun4i_tcon.h"
>  #include "sun6i_mipi_dsi.h"
>
>  #include <video/mipi_display.h>
> @@ -32,6 +34,8 @@
>  #define SUN6I_DSI_CTL_EN                       BIT(0)
>
>  #define SUN6I_DSI_BASIC_CTL_REG                0x00c
> +#define SUN6I_DSI_BASIC_CTL_TRAIL_INV(n)               (((n) & 0xf) << 4)
> +#define SUN6I_DSI_BASIC_CTL_TRAIL_FILL         BIT(3)
>  #define SUN6I_DSI_BASIC_CTL_HBP_DIS            BIT(2)
>  #define SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS                BIT(1)
>  #define SUN6I_DSI_BASIC_CTL_VIDEO_BURST                BIT(0)
> @@ -152,6 +156,8 @@
>
>  #define SUN6I_DSI_CMD_TX_REG(n)                (0x300 + (n) * 0x04)
>
> +#define SUN6I_DSI_SYNC_POINT           40
> +
>  enum sun6i_dsi_start_inst {
>         DSI_START_LPRX,
>         DSI_START_LPTX,
> @@ -365,31 +371,101 @@ static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
>         return delay;
>  }
>
> +static u16 sun6i_dsi_get_line_num(struct sun6i_dsi *dsi,
> +                                 struct drm_display_mode *mode)
> +{
> +       struct mipi_dsi_device *device = dsi->device;
> +       unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> +
> +       return mode->htotal * Bpp / device->lanes;
> +}
> +
> +static u16 sun6i_dsi_get_drq_edge0(struct sun6i_dsi *dsi,
> +                                  struct drm_display_mode *mode,
> +                                  u16 line_num, u16 edge1)
> +{
> +       u16 edge0 = edge1;
> +
> +       edge0 += (mode->hdisplay + 40) * SUN6I_DSI_TCON_DIV / 8;
> +
> +       if (edge0 > line_num)
> +               return edge0 - line_num;
> +
> +       return 1;
> +}
> +
> +static u16 sun6i_dsi_get_drq_edge1(struct sun6i_dsi *dsi,
> +                                  struct drm_display_mode *mode,
> +                                  u16 line_num)
> +{
> +       struct mipi_dsi_device *device = dsi->device;
> +       unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> +       unsigned hbp = mode->htotal - mode->hsync_end;

Allwinner's hbp is actually horizontal back porch plus sync pulse.
So this should be

    hbp = mode->htotal - mode->hsync_start;

> +       u16 edge1;
> +
> +
> +       edge1 = SUN6I_DSI_SYNC_POINT;
> +       edge1 += mode->hdisplay + hbp + 20;
> +       edge1 = edge1 * Bpp / device->lanes;

Compared to the A64 BSP, this seems to be incorrect. The original code was

    edge1 = sync_point +
                (panel->lcd_x + panel->lcd_hbp + 20) *
                dsi_pixel_bits[panel->lcd_dsi_format] /
                (8 * panel->lcd_dsi_lane);

Note that sync_point is outside of the parentheses and should not be
multiplied and divided.

This would make sense if sync_point is a fixed delay that is needed
regardless of the timings.

> +
> +       if (edge1 > line_num)
> +               return line_num;
> +
> +       return edge1;
> +}
> +
>  static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
>                                   struct drm_display_mode *mode)
>  {
>         struct mipi_dsi_device *device = dsi->device;
> -       u32 val = 0;
> +       u32 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> +       u32 tcon0_drq = 0;
> +
> +       if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +               u16 line_num = sun6i_dsi_get_line_num(dsi, mode);
> +               u16 edge0, edge1;
> +
> +               edge1 = sun6i_dsi_get_drq_edge1(dsi, mode, line_num);
> +               edge0 = sun6i_dsi_get_drq_edge0(dsi, mode, line_num, edge1);
>
> -       if ((mode->hsync_end - mode->hdisplay) > 20) {
> +               regmap_write(dsi->regs, SUN6I_DSI_BURST_DRQ_REG,
> +                            SUN6I_DSI_BURST_DRQ_EDGE0(edge0) |
> +                            SUN6I_DSI_BURST_DRQ_EDGE1(edge1));
> +
> +               regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
> +                            SUN6I_DSI_BURST_LINE_NUM(line_num) |
> +                            SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT));
> +
> +               tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE;
> +       } else if ((mode->hsync_end - mode->hdisplay) > 20) {
>                 /* Maaaaaagic */
>                 u16 drq = (mode->hsync_end - mode->hdisplay) - 20;

This and the above if clause should use hsync_start instead of hsync_end.

The A64 BSP specifies drq = vfp - 20.

> -
> -               drq *= mipi_dsi_pixel_format_to_bpp(device->format);
> +               drq *= bpp;
>                 drq /= 32;
>
> -               val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> -                      SUN6I_DSI_TCON_DRQ_SET(drq));
> +               tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> +                       SUN6I_DSI_TCON_DRQ_SET(drq);
>         }
>
> -       regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
> +       regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, tcon0_drq);
>  }
>
>  static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>                                       struct drm_display_mode *mode)
>  {
> +       struct mipi_dsi_device *device = dsi->device;
>         u16 delay = 50 - 1;
>
> +       if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +               delay = (mode->htotal - mode->hdisplay) * 150;
> +               delay /= (mode->clock / 1000) * 8;
> +               delay -= 50;
> +       }
> +
> +       regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
> +                        2 << (4 * DSI_INST_ID_LP11) |
> +                        3 << (4 * DSI_INST_ID_DLY));
> +
>         regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
>                      SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
>                      SUN6I_DSI_INST_LOOP_NUM_N1(delay));
> @@ -456,48 +532,66 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>         struct mipi_dsi_device *device = dsi->device;
>         unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
>         u16 hbp, hfp, hsa, hblk, vblk;
> +       u32 basic_ctl = 0;
>         size_t bytes;
>         u8 *buffer;
>
>         /* Do all timing calculations up front to allocate buffer space */
>
> -       /*
> -        * A sync period is composed of a blanking packet (4 bytes +
> -        * payload + 2 bytes) and a sync event packet (4 bytes). Its
> -        * minimal size is therefore 10 bytes
> -        */
> +       if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +               hsa = 0;
> +               hbp = 0;
> +               hfp = 0;
> +               hblk = mode->hdisplay * Bpp;
> +               vblk = 0;
> +               basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
> +                           SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
> +                           SUN6I_DSI_BASIC_CTL_HBP_DIS;
> +
> +               if (device->lanes == 4)
> +                       basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
> +                                    SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
> +       } else {
> +               /*
> +                * A sync period is composed of a blanking packet (4 bytes +
> +                * payload + 2 bytes) and a sync event packet (4 bytes). Its
> +                * minimal size is therefore 10 bytes
> +                */
>  #define HSA_PACKET_OVERHEAD    10
> -       hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> -                 (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
> -
> -       /*
> -        * The backporch is set using a blanking packet (4 bytes +
> -        * payload + 2 bytes). Its minimal size is therefore 6 bytes
> -        */
> +               hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> +                         (mode->hsync_end - mode->hsync_start) * Bpp -
> +                         HSA_PACKET_OVERHEAD);

I believe for all these packets, that minimum value is not needed. The
WC field of the long command packet refers to the length of the payload
only. The payload is used to pad the packet to the desired length, but
is otherwise useless. Likewise, the CRC value is only calculated against
the payload.

Specifying a minimum means you are likely skewing the timings.

> +
> +               /*
> +                * The backporch is set using a blanking packet (4 bytes +
> +                * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +                */
>  #define HBP_PACKET_OVERHEAD    6
> -       hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> -                 (mode->hsync_start - mode->hdisplay) * Bpp - HBP_PACKET_OVERHEAD);
> -
> -       /*
> -        * The frontporch is set using a blanking packet (4 bytes +
> -        * payload + 2 bytes). Its minimal size is therefore 6 bytes
> -        */
> +               hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> +                         (mode->hsync_start - mode->hdisplay) * Bpp -
> +                         HBP_PACKET_OVERHEAD);
> +
> +               /*
> +                * The frontporch is set using a blanking packet (4 bytes +
> +                * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +                */
>  #define HFP_PACKET_OVERHEAD    6
> -       hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> -                 (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> -
> -       /*
> -        * hblk seems to be the line + porches length.
> -        */
> -       hblk = mode->htotal * Bpp - hsa;
> -
> -       /*
> -        * And I'm not entirely sure what vblk is about. The driver in
> -        * Allwinner BSP is using a rather convoluted calculation
> -        * there only for 4 lanes. However, using 0 (the !4 lanes
> -        * case) even with a 4 lanes screen seems to work...
> -        */
> -       vblk = 0;
> +               hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> +                         (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> +
> +               /*
> +                * hblk seems to be the line + porches length.
> +                */
> +               hblk = mode->htotal * Bpp - hsa;

Based on the timing diagram in section 8.11 of the MIPI DSI spec, hblk is
supposed to take up the remainder of the scan line after the sync event or
sync pulse during the vertical inactive period. It is the BLLP portion.
This seems like the correct value, though it is somewhat simplified.
The packet overhead is gone (cancelled out with the one from hsa).

> +
> +               /*
> +                * And I'm not entirely sure what vblk is about. The driver in
> +                * Allwinner BSP is using a rather convoluted calculation
> +                * there only for 4 lanes. However, using 0 (the !4 lanes
> +                * case) even with a 4 lanes screen seems to work...
> +                */

No idea either.
Hope this helps.

ChenYu


> +               vblk = 0;
> +       }
>
>         /* How many bytes do we need to send all payloads? */
>         bytes = max_t(size_t, max(max(hfp, hblk), max(hsa, hbp)), vblk);
> @@ -505,7 +599,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>         if (WARN_ON(!buffer))
>                 return;
>
> -       regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, 0);
> +       regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, basic_ctl);
>
>         regmap_write(dsi->regs, SUN6I_DSI_SYNC_HSS_REG,
>                      sun6i_dsi_build_sync_pkt(MIPI_DSI_H_SYNC_START,
> --
> git-series 0.9.1

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

Re: [PATCH 3/4] drm/sun4i: dsi: Add burst support

[Maxime Ripard]

Hi,

On Wed, Feb 06, 2019 at 02:28:39AM +0800, Chen-Yu Tsai wrote:
> > +static u16 sun6i_dsi_get_drq_edge1(struct sun6i_dsi *dsi,
> > +                                  struct drm_display_mode *mode,
> > +                                  u16 line_num)
> > +{
> > +       struct mipi_dsi_device *device = dsi->device;
> > +       unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> > +       unsigned hbp = mode->htotal - mode->hsync_end;
> 
> Allwinner's hbp is actually horizontal back porch plus sync pulse.
> So this should be
> 
>     hbp = mode->htotal - mode->hsync_start;

So I've tested this and it actually adds a margin on the left side of
the panel, which seems to indicate that one of the porches isn't
right. I've applied Jagan's patches to fix the backporch and front
porch inversions, and it doesn't fix anything, so even though I don't
really know why, this seems to be the actual backporch.

It's really hard to tell without a DSI analyzer though, but that's not
really affordable...

> > +       u16 edge1;
> > +
> > +
> > +       edge1 = SUN6I_DSI_SYNC_POINT;
> > +       edge1 += mode->hdisplay + hbp + 20;
> > +       edge1 = edge1 * Bpp / device->lanes;
> 
> Compared to the A64 BSP, this seems to be incorrect. The original code was
> 
>     edge1 = sync_point +
>                 (panel->lcd_x + panel->lcd_hbp + 20) *
>                 dsi_pixel_bits[panel->lcd_dsi_format] /
>                 (8 * panel->lcd_dsi_lane);
> 
> Note that sync_point is outside of the parentheses and should not be
> multiplied and divided.
> 
> This would make sense if sync_point is a fixed delay that is needed
> regardless of the timings.

Good catch, thanks!

> > -       if ((mode->hsync_end - mode->hdisplay) > 20) {
> > +               regmap_write(dsi->regs, SUN6I_DSI_BURST_DRQ_REG,
> > +                            SUN6I_DSI_BURST_DRQ_EDGE0(edge0) |
> > +                            SUN6I_DSI_BURST_DRQ_EDGE1(edge1));
> > +
> > +               regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
> > +                            SUN6I_DSI_BURST_LINE_NUM(line_num) |
> > +                            SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT));
> > +
> > +               tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE;
> > +       } else if ((mode->hsync_end - mode->hdisplay) > 20) {
> >                 /* Maaaaaagic */
> >                 u16 drq = (mode->hsync_end - mode->hdisplay) - 20;
> 
> This and the above if clause should use hsync_start instead of hsync_end.
> 
> The A64 BSP specifies drq = vfp - 20.

Indeed, I'll fix it.

> >  #define HSA_PACKET_OVERHEAD    10
> > -       hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> > -                 (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
> > -
> > -       /*
> > -        * The backporch is set using a blanking packet (4 bytes +
> > -        * payload + 2 bytes). Its minimal size is therefore 6 bytes
> > -        */
> > +               hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> > +                         (mode->hsync_end - mode->hsync_start) * Bpp -
> > +                         HSA_PACKET_OVERHEAD);
> 
> I believe for all these packets, that minimum value is not needed. The
> WC field of the long command packet refers to the length of the payload
> only. The payload is used to pad the packet to the desired length, but
> is otherwise useless. Likewise, the CRC value is only calculated against
> the payload.
> 
> Specifying a minimum means you are likely skewing the timings.

This code was here already, but it's a valid point. I'm not sure what
the DSI spec tells in that case. I'll have a look.

> >  #define HFP_PACKET_OVERHEAD    6
> > -       hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> > -                 (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> > -
> > -       /*
> > -        * hblk seems to be the line + porches length.
> > -        */
> > -       hblk = mode->htotal * Bpp - hsa;
> > -
> > -       /*
> > -        * And I'm not entirely sure what vblk is about. The driver in
> > -        * Allwinner BSP is using a rather convoluted calculation
> > -        * there only for 4 lanes. However, using 0 (the !4 lanes
> > -        * case) even with a 4 lanes screen seems to work...
> > -        */
> > -       vblk = 0;
> > +               hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> > +                         (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> > +
> > +               /*
> > +                * hblk seems to be the line + porches length.
> > +                */
> > +               hblk = mode->htotal * Bpp - hsa;
> 
> Based on the timing diagram in section 8.11 of the MIPI DSI spec, hblk is
> supposed to take up the remainder of the scan line after the sync event or
> sync pulse during the vertical inactive period. It is the BLLP portion.
> This seems like the correct value, though it is somewhat simplified.
> The packet overhead is gone (cancelled out with the one from hsa).

I actually found out that Allwinner tells that it's the total - the
sync pulse - the blank overhead (so 10 bytes).

I'll update it.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Maxime Ripard]

From: Konstantin Sudakov <k.sudakov@integrasources.com>

The Rondo RB070D30 panel is a MIPI-DSI panel based on a Fitipower EK79007
controller and a 1024x600 panel.

Signed-off-by: Konstantin Sudakov <k.sudakov@integrasources.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/panel/Kconfig                |   9 +-
 drivers/gpu/drm/panel/Makefile               |   1 +-
 drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++-
 3 files changed, 268 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3f3537719beb..3164fa824a63 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -138,6 +138,15 @@ config DRM_PANEL_RAYDIUM_RM68200
 	  Say Y here if you want to enable support for Raydium RM68200
 	  720x1280 DSI video mode panel.
 
+config DRM_PANEL_RONDO_RB070D30
+	tristate "Rondo Electronics RB070D30 panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for Rondo Electronics
+	  RB070D30 1024x600 DSI panel.
+
 config DRM_PANEL_SAMSUNG_S6D16D0
 	tristate "Samsung S6D16D0 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4396658a7996..4fe4cf1bfdb5 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
 obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
+obj-$(CONFIG_DRM_PANEL_RONDO_RB070D30) += panel-rondo-rb070d30.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
new file mode 100644
index 000000000000..4f8e63f367b1
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018, Bridge Systems BV
+ * Copyright (C) 2018, Bootlin
+ * Copyright (C) 2017, Free Electrons
+ *
+ * This file based on panel-ilitek-ili9881c.c
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drmP.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+#include <video/of_display_timing.h>
+#include <video/videomode.h>
+
+struct rb070d30_panel {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct backlight_device *backlight;
+	struct regulator *supply;
+
+	struct {
+		struct gpio_desc *power;
+		struct gpio_desc *reset;
+		struct gpio_desc *updn;
+		struct gpio_desc *shlr;
+	} gpios;
+};
+
+static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel)
+{
+	return container_of(panel, struct rb070d30_panel, panel);
+}
+
+static int rb070d30_panel_prepare(struct drm_panel *panel)
+{
+	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
+	int ret;
+
+	ret = regulator_enable(ctx->supply);
+	if (ret < 0) {
+		dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret);
+		return ret;
+	}
+
+	/* Reset */
+	msleep(20);
+	gpiod_set_value(ctx->gpios.power, 1);
+	msleep(20);
+	gpiod_set_value(ctx->gpios.reset, 1);
+	msleep(20);
+	return 0;
+}
+
+static int rb070d30_panel_unprepare(struct drm_panel *panel)
+{
+	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
+
+	gpiod_set_value(ctx->gpios.power, 0);
+	gpiod_set_value(ctx->gpios.reset, 0);
+	regulator_disable(ctx->supply);
+
+	return 0;
+}
+
+static int rb070d30_panel_enable(struct drm_panel *panel)
+{
+	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
+	int ret;
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi);
+	if (ret)
+		return ret;
+
+	ret = backlight_enable(ctx->backlight);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
+	return ret;
+}
+
+static int rb070d30_panel_disable(struct drm_panel *panel)
+{
+	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
+
+	backlight_disable(ctx->backlight);
+	return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
+}
+
+/* Default timings */
+static const struct drm_display_mode default_mode = {
+	.clock		= 51206,
+	.hdisplay	= 1024,
+	.hsync_start	= 1024 + 160,
+	.hsync_end	= 1024 + 160 + 80,
+	.htotal		= 1024 + 160 + 80 + 80,
+	.vdisplay	= 600,
+	.vsync_start	= 600 + 12,
+	.vsync_end	= 600 + 12 + 10,
+	.vtotal		= 600 + 12 + 10 + 13,
+	.vrefresh	= 60,
+};
+
+static int rb070d30_panel_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
+	struct drm_display_mode *mode;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	mode = drm_mode_duplicate(panel->drm, &default_mode);
+	if (!mode) {
+		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
+			default_mode.hdisplay, default_mode.vdisplay,
+			default_mode.vrefresh);
+		return -EINVAL;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	panel->connector->display_info.bpc = 8;
+	panel->connector->display_info.width_mm = 154;
+	panel->connector->display_info.height_mm = 85;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs rb070d30_panel_funcs = {
+	.get_modes	= rb070d30_panel_get_modes,
+	.prepare	= rb070d30_panel_prepare,
+	.enable		= rb070d30_panel_enable,
+	.disable	= rb070d30_panel_disable,
+	.unprepare	= rb070d30_panel_unprepare,
+};
+
+static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	struct device_node *np;
+	struct rb070d30_panel *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd");
+	if (IS_ERR(ctx->supply))
+		return PTR_ERR(ctx->supply);
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+	ctx->dsi = dsi;
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = &dsi->dev;
+	ctx->panel.funcs = &rb070d30_panel_funcs;
+
+	ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->gpios.reset)) {
+		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
+		return PTR_ERR(ctx->gpios.reset);
+	}
+
+	ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->gpios.power)) {
+		dev_err(&dsi->dev, "Couldn't get our power GPIO\n");
+		return PTR_ERR(ctx->gpios.power);
+	}
+
+	ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->gpios.updn)) {
+		dev_err(&dsi->dev, "Couldn't get our updn GPIO\n");
+		return PTR_ERR(ctx->gpios.updn);
+	}
+
+	ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->gpios.shlr)) {
+		dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n");
+		return PTR_ERR(ctx->gpios.shlr);
+	}
+
+	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
+	if (np) {
+		ctx->backlight = of_find_backlight_by_node(np);
+		of_node_put(np);
+
+		if (!ctx->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	ret = drm_panel_add(&ctx->panel);
+	if (ret < 0)
+		goto free_backlight;
+
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->lanes = 4;
+
+	return mipi_dsi_attach(dsi);
+
+free_backlight:
+	backlight_put(ctx->backlight);
+	return ret;
+}
+
+static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi)
+{
+	struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&ctx->panel);
+	backlight_put(ctx->backlight);
+
+	return 0;
+}
+
+static const struct of_device_id rb070d30_panel_of_match[] = {
+	{ .compatible = "rondo,rb070d30" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match);
+
+static struct mipi_dsi_driver rb070d30_panel_driver = {
+	.probe = rb070d30_panel_dsi_probe,
+	.remove = rb070d30_panel_dsi_remove,
+	.driver = {
+		.name = "panel-rondo-rb070d30",
+		.of_match_table	= rb070d30_panel_of_match,
+	},
+};
+module_mipi_dsi_driver(rb070d30_panel_driver);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
+MODULE_AUTHOR("Konstantin Sudakov <k.sudakov@integrasources.com>");
+MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver");
+MODULE_LICENSE("GPL");
-- 
git-series 0.9.1

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

Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Sam Ravnborg]

Hi Maxime / Konstantin.

Nice welstructured and small driver.
Please see a few comments below

Some of the comments in the following apply to a lot of
the existing panel drivers as well.
But lets see if we can get new drivers to be even better.

	Sam 

On Wed, Jan 23, 2019 at 04:54:24PM +0100, Maxime Ripard wrote:
> From: Konstantin Sudakov <k.sudakov@integrasources.com>
> 
> The Rondo RB070D30 panel is a MIPI-DSI panel based on a Fitipower EK79007
> controller and a 1024x600 panel.
> 
> Signed-off-by: Konstantin Sudakov <k.sudakov@integrasources.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/panel/Kconfig                |   9 +-
>  drivers/gpu/drm/panel/Makefile               |   1 +-
>  drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++-
>  3 files changed, 268 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c
> 
> diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
> new file mode 100644
> index 000000000000..4f8e63f367b1
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, Bridge Systems BV
> + * Copyright (C) 2018, Bootlin
> + * Copyright (C) 2017, Free Electrons
> + *
> + * This file based on panel-ilitek-ili9881c.c
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_panel.h>
Please do not use drmP.h in new drivers. We are trying to get rid of it.

> +
> +#include <video/mipi_display.h>
> +#include <video/of_display_timing.h>
> +#include <video/videomode.h>
> +
> +struct rb070d30_panel {
> +	struct drm_panel panel;
> +	struct mipi_dsi_device *dsi;
> +	struct backlight_device *backlight;
> +	struct regulator *supply;
> +
> +	struct {
> +		struct gpio_desc *power;
> +		struct gpio_desc *reset;
> +		struct gpio_desc *updn;
> +		struct gpio_desc *shlr;
> +	} gpios;
> +};
> +
> +static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct rb070d30_panel, panel);
> +}
> +
> +static int rb070d30_panel_prepare(struct drm_panel *panel)
> +{
> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> +	int ret;
> +
> +	ret = regulator_enable(ctx->supply);
> +	if (ret < 0) {
> +		dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret);
Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers.
This is general for the whole file.

> +		return ret;
> +	}
> +
> +	/* Reset */
> +	msleep(20);
> +	gpiod_set_value(ctx->gpios.power, 1);
> +	msleep(20);
> +	gpiod_set_value(ctx->gpios.reset, 1);
> +	msleep(20);
> +	return 0;
> +}
To verify the above pointer to a datasheet would be nice.


> +
> +static int rb070d30_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> +
> +	gpiod_set_value(ctx->gpios.power, 0);
> +	gpiod_set_value(ctx->gpios.reset, 0);
> +	regulator_disable(ctx->supply);
> +
> +	return 0;
> +}
There is sometimes timing constrains after deassert reset..
The order is not strictly reverse of prepare - is that on purpose?


> +
> +static int rb070d30_panel_enable(struct drm_panel *panel)
> +{
> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = backlight_enable(ctx->backlight);
> +	if (ret)
> +		goto out;
> +
> +	return 0;
> +
> +out:
> +	mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +	return ret;
> +}
> +
> +static int rb070d30_panel_disable(struct drm_panel *panel)
> +{
> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> +
> +	backlight_disable(ctx->backlight);
> +	return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +}
> +
> +/* Default timings */
> +static const struct drm_display_mode default_mode = {
> +	.clock		= 51206,
> +	.hdisplay	= 1024,
> +	.hsync_start	= 1024 + 160,
> +	.hsync_end	= 1024 + 160 + 80,
> +	.htotal		= 1024 + 160 + 80 + 80,
> +	.vdisplay	= 600,
> +	.vsync_start	= 600 + 12,
> +	.vsync_end	= 600 + 12 + 10,
> +	.vtotal		= 600 + 12 + 10 + 13,
> +	.vrefresh	= 60,
> +};
height and width missing here. Seems better to add them here than hiding in code below.
Is it on purpose that no flags are specified?

> +
> +static int rb070d30_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> +	struct drm_display_mode *mode;
> +	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +
> +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> +	if (!mode) {
> +		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			default_mode.hdisplay, default_mode.vdisplay,
> +			default_mode.vrefresh);
Use"
		DRM_DEV_ERROR(&ctx->dsi->dev,
			      "failed to add mode" DRM_MODE_FMT,
			      DRM_MODE_ARG(mode));
> +		return -EINVAL;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	panel->connector->display_info.bpc = 8;
> +	panel->connector->display_info.width_mm = 154;
> +	panel->connector->display_info.height_mm = 85;
See comment on height above.
Same goes for bpc

> +	drm_display_info_set_bus_formats(&connector->display_info,
> +					 &bus_format, 1);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs rb070d30_panel_funcs = {
> +	.get_modes	= rb070d30_panel_get_modes,
> +	.prepare	= rb070d30_panel_prepare,
> +	.enable		= rb070d30_panel_enable,
> +	.disable	= rb070d30_panel_disable,
> +	.unprepare	= rb070d30_panel_unprepare,
> +};
> +
> +static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device_node *np;
> +	struct rb070d30_panel *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd");
> +	if (IS_ERR(ctx->supply))
> +		return PTR_ERR(ctx->supply);
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dsi = dsi;
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = &dsi->dev;
> +	ctx->panel.funcs = &rb070d30_panel_funcs;
> +
> +	ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpios.reset)) {
> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(ctx->gpios.reset);
> +	}
> +
> +	ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpios.power)) {
> +		dev_err(&dsi->dev, "Couldn't get our power GPIO\n");
> +		return PTR_ERR(ctx->gpios.power);
> +	}
> +
> +	ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpios.updn)) {
> +		dev_err(&dsi->dev, "Couldn't get our updn GPIO\n");
> +		return PTR_ERR(ctx->gpios.updn);
> +	}
This gpio is never used, it is only read from DT


> +
> +	ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpios.shlr)) {
> +		dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n");
> +		return PTR_ERR(ctx->gpios.shlr);
> +	}
This gpio is never used, it is only read from DT

> +
> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> +	if (np) {
> +		ctx->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!ctx->backlight)
> +			return -EPROBE_DEFER;
> +	}
Use devm_of_find_backlight()

> +
> +	ret = drm_panel_add(&ctx->panel);
> +	if (ret < 0)
> +		goto free_backlight;
> +
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 4;
> +
> +	return mipi_dsi_attach(dsi);
> +
> +free_backlight:
> +	backlight_put(ctx->backlight);
If devm_of_find_backlight() is used this can go.

> +	return ret;
> +}
> +
> +static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&ctx->panel);
> +	backlight_put(ctx->backlight);
If devm_of_find_backlight() is used this can go.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rb070d30_panel_of_match[] = {
> +	{ .compatible = "rondo,rb070d30" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match);
> +
> +static struct mipi_dsi_driver rb070d30_panel_driver = {
> +	.probe = rb070d30_panel_dsi_probe,
> +	.remove = rb070d30_panel_dsi_remove,
> +	.driver = {
> +		.name = "panel-rondo-rb070d30",
> +		.of_match_table	= rb070d30_panel_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(rb070d30_panel_driver);
> +
> +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
> +MODULE_AUTHOR("Konstantin Sudakov <k.sudakov@integrasources.com>");
> +MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver");
> +MODULE_LICENSE("GPL");
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Sam Ravnborg]

Hi Konstantin

> >> + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW);
> >> + if (IS_ERR(ctx->gpios.updn)) {
> >> + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n");
> >> + return PTR_ERR(ctx->gpios.updn);
> >> + }
> > This gpio is never used, it is only read from DT
> The gpio is initialized with low state. The state may be inverted by DT. The same for the "shlr".
> It is a vertical / horizontal inversion.

Ohh, then it makes sense again.
Consider adding a comment so this becomes more obvious

	Sam

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

Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2848 bytes --]

Hi Sam,

Thanks for the review, I'll address the points left out.

On Sat, Jan 26, 2019 at 04:39:46PM +0100, Sam Ravnborg wrote:
> > +		return ret;
> > +	}
> > +
> > +	/* Reset */
> > +	msleep(20);
> > +	gpiod_set_value(ctx->gpios.power, 1);
> > +	msleep(20);
> > +	gpiod_set_value(ctx->gpios.reset, 1);
> > +	msleep(20);
> > +	return 0;
> > +}
> To verify the above pointer to a datasheet would be nice.

Unfortunately, it's not publicly available :/

> > +
> > +static int rb070d30_panel_unprepare(struct drm_panel *panel)
> > +{
> > +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> > +
> > +	gpiod_set_value(ctx->gpios.power, 0);
> > +	gpiod_set_value(ctx->gpios.reset, 0);
> > +	regulator_disable(ctx->supply);
> > +
> > +	return 0;
> > +}
>
> There is sometimes timing constrains after deassert reset..
> The order is not strictly reverse of prepare - is that on purpose?

You're right about the order. I couldn't find any delay after a reset
though in the datasheet.

> > +/* Default timings */
> > +static const struct drm_display_mode default_mode = {
> > +	.clock		= 51206,
> > +	.hdisplay	= 1024,
> > +	.hsync_start	= 1024 + 160,
> > +	.hsync_end	= 1024 + 160 + 80,
> > +	.htotal		= 1024 + 160 + 80 + 80,
> > +	.vdisplay	= 600,
> > +	.vsync_start	= 600 + 12,
> > +	.vsync_end	= 600 + 12 + 10,
> > +	.vtotal		= 600 + 12 + 10 + 13,
> > +	.vrefresh	= 60,
> > +};
> height and width missing here. Seems better to add them here than hiding in code below.
> Is it on purpose that no flags are specified?
> 
> > +
> > +static int rb070d30_panel_get_modes(struct drm_panel *panel)
> > +{
> > +	struct drm_connector *connector = panel->connector;
> > +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> > +	struct drm_display_mode *mode;
> > +	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +
> > +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> > +	if (!mode) {
> > +		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> > +			default_mode.hdisplay, default_mode.vdisplay,
> > +			default_mode.vrefresh);
> Use"
> 		DRM_DEV_ERROR(&ctx->dsi->dev,
> 			      "failed to add mode" DRM_MODE_FMT,
> 			      DRM_MODE_ARG(mode));
> > +		return -EINVAL;
> > +	}
> > +
> > +	drm_mode_set_name(mode);
> > +
> > +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > +	drm_mode_probed_add(connector, mode);
> > +
> > +	panel->connector->display_info.bpc = 8;
> > +	panel->connector->display_info.width_mm = 154;
> > +	panel->connector->display_info.height_mm = 85;
> See comment on height above.
> Same goes for bpc

Sorry, I'm not sure to follow you here. bpc and height are both set?

Thanks!
Maxime


-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Sam Ravnborg]

Hi Maxime.

> > > +	}
> > > +
> > > +	drm_mode_set_name(mode);
> > > +
> > > +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > > +	drm_mode_probed_add(connector, mode);
> > > +
> > > +	panel->connector->display_info.bpc = 8;
> > > +	panel->connector->display_info.width_mm = 154;
> > > +	panel->connector->display_info.height_mm = 85;
> > See comment on height above.
> > Same goes for bpc
> 
> Sorry, I'm not sure to follow you here. bpc and height are both set?

I assumed that if we had specified height and width in display_mode
then we did not have to do it here. But I may be wrong.
And for bpc I was plain wrong.

	Sam

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

Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 951 bytes --]

Hi Sam,

On Tue, Jan 29, 2019 at 04:48:33PM +0100, Sam Ravnborg wrote:
> > > > +	}
> > > > +
> > > > +	drm_mode_set_name(mode);
> > > > +
> > > > +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > > > +	drm_mode_probed_add(connector, mode);
> > > > +
> > > > +	panel->connector->display_info.bpc = 8;
> > > > +	panel->connector->display_info.width_mm = 154;
> > > > +	panel->connector->display_info.height_mm = 85;
> > > See comment on height above.
> > > Same goes for bpc
> > 
> > Sorry, I'm not sure to follow you here. bpc and height are both set?
> 
> I assumed that if we had specified height and width in display_mode
> then we did not have to do it here. But I may be wrong.

It looks like it's not done by the core, but panel-simple simply
copies it, so I'll do it as well.

Thanks for the suggestion!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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