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
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;
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.
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;
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,
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,
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
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
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, soset 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
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, soset 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
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,
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
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
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");
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
Hello, Sam! Thank you for the comments.
+#include <drm/drmP.h>
Please do not use drmP.h in new drivers. We are trying to get rid of it.
It can be replaced by these headers:
#include <drm/drm_connector.h> #include <drm/drm_modes.h> #include <uapi/linux/media-bus-format.h>
Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers. This is general for the whole file.
Good idea, I think. So, this header is needed:
#include <drm/drm_print.h>
- /* Reset */
- msleep(20);
- gpiod_set_value(ctx->gpios.power, 1);
- msleep(20);
- gpiod_set_value(ctx->gpios.reset, 1);
- msleep(20);
To verify the above pointer to a datasheet would be nice.
I looked the datasheet again, looks like the reset should be first. But I didn't find information about timings. It's needed to be checked on hardware. The power pin is named "STBYB" originally.
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?
I sure, this algorithm was copied from the ilitek driver. The default_mode (drm_display_mode) variable and panel->connector->display_info (drm_display_info) variable has a different types, I'm not sure about this point. The bpc is not not applicable for the drm_display_mode.
- 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));
Not sure because the mode is NULL. May the default_mode be used for print?
- 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.
Use devm_of_find_backlight()
I agree with you.
The lastly, I think the rondo should be replaced by ronbo, because the company name is Ronbo Electronics Ltd. http://www.ronboe.com/about.html
Суббота, 26 января 2019, 22:39 +07:00 от Sam Ravnborg sam@ravnborg.org:
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Jagan Teki -
Konstantin Sudakov -
Maxime Ripard -
Paul Kocialkowski -
Sam Ravnborg