diff mbox series

[v4,76/80] drm/omap: dsi: cleanup initial vc setup

Message ID 20201124124538.660710-77-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show
Series Convert DSI code to use drm_mipi_dsi and drm_panel | expand

Commit Message

Tomi Valkeinen Nov. 24, 2020, 12:45 p.m. UTC 
As we now have a fixed setup for VCs (VC0 for video stream, VC1 for
commands), we can simplify the VC setup.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 85 +++++++++++--------------------
 1 file changed, 31 insertions(+), 54 deletions(-)

Comments

Laurent Pinchart Dec. 1, 2020, 12:36 a.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Tue, Nov 24, 2020 at 02:45:34PM +0200, Tomi Valkeinen wrote:
> As we now have a fixed setup for VCs (VC0 for video stream, VC1 for
> commands), we can simplify the VC setup.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dsi.c | 85 +++++++++++--------------------
>  1 file changed, 31 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index ff8ace957291..27d0d119668b 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -2017,40 +2017,6 @@ static void dsi_vc_initial_config(struct dsi_data *dsi, int vc)
>  	dsi->vc[vc].source = DSI_VC_SOURCE_L4;
>  }
>  
> -static int dsi_vc_config_source(struct dsi_data *dsi, int vc,
> -				enum dsi_vc_source source)
> -{
> -	if (dsi->vc[vc].source == source)
> -		return 0;
> -
> -	DSSDBG("Source config of VC %d", vc);
> -
> -	dsi_sync_vc(dsi, vc);
> -
> -	dsi_vc_enable(dsi, vc, 0);
> -
> -	/* VC_BUSY */
> -	if (!wait_for_bit_change(dsi, DSI_VC_CTRL(vc), 15, 0)) {
> -		DSSERR("vc(%d) busy when trying to config for VP\n", vc);
> -		return -EIO;
> -	}
> -
> -	/* SOURCE, 0 = L4, 1 = video port */
> -	REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), source, 1, 1);
> -
> -	/* DCS_CMD_ENABLE */
> -	if (dsi->data->quirks & DSI_QUIRK_DCS_CMD_CONFIG_VC) {
> -		bool enable = source == DSI_VC_SOURCE_VP;
> -		REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), enable, 30, 30);
> -	}
> -
> -	dsi_vc_enable(dsi, vc, 1);
> -
> -	dsi->vc[vc].source = source;
> -
> -	return 0;
> -}
> -
>  static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
>  		bool enable)
>  {
> @@ -2072,10 +2038,6 @@ static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
>  	dsi_if_enable(dsi, 1);
>  
>  	dsi_force_tx_stop_mode_io(dsi);
> -
> -	/* start the DDR clock by sending a NULL packet */
> -	if (dsi->vm_timings.ddr_clk_always_on && enable)
> -		dsi_vc_send_null(dsi, vc, dsi->dsidev->channel);
>  }
>  
>  static void dsi_vc_flush_long_data(struct dsi_data *dsi, int vc)
> @@ -2270,8 +2232,6 @@ static int dsi_vc_send_long(struct dsi_data *dsi, int vc,
>  		return -EINVAL;
>  	}
>  
> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
> -
>  	dsi_vc_write_long_header(dsi, vc, msg->channel, msg->type, msg->tx_len, 0);
>  
>  	p = msg->tx_buf;
> @@ -2331,8 +2291,6 @@ static int dsi_vc_send_short(struct dsi_data *dsi, int vc,
>  		DSSDBG("dsi_vc_send_short(ch%d, dt %#x, b1 %#x, b2 %#x)\n",
>  		       vc, msg->type, pkt.header[1], pkt.header[2]);
>  
> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
> -
>  	if (FLD_GET(dsi_read_reg(dsi, DSI_VC_CTRL(vc)), 16, 16)) {
>  		DSSERR("ERROR FIFO FULL, aborting transfer\n");
>  		return -EINVAL;
> @@ -3351,8 +3309,6 @@ static void dsi_update_screen_dispc(struct dsi_data *dsi)
>  
>  	DSSDBG("dsi_update_screen_dispc(%dx%d)\n", w, h);
>  
> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_VP);
> -
>  	bytespp	= mipi_dsi_pixel_format_to_bpp(dsi->pix_fmt) / 8;
>  	bytespl = w * bytespp;
>  	bytespf = bytespl * h;
> @@ -3522,9 +3478,7 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
>  
>  	dsi_set_ulps_auto(dsi, false);
>  
> -	dsi_vc_enable_hs(dssdev, vc, !(dsi->dsidev->mode_flags & MIPI_DSI_MODE_LPM));

Why is this not needed anymore ?

> -
> -	r = _dsi_send_nop(dsi, vc, dsi->dsidev->channel);
> +	r = _dsi_send_nop(dsi, VC_CMD, dsi->dsidev->channel);
>  	if (r < 0) {
>  		DSSWARN("failed to send nop between frames: %d\n", r);
>  		goto err;
> @@ -3649,6 +3603,35 @@ static int dsi_configure_dsi_clocks(struct dsi_data *dsi)
>  	return 0;
>  }
>  
> +static void dsi_setup_dsi_vcs(struct dsi_data *dsi)
> +{
> +	/* Setup VC_CMD for LP and cpu transfers */
> +	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_CMD), 0, 9, 9); /* LP */
> +
> +	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_CMD), 0, 1, 1); /* SOURCE_L4 */
> +	dsi->vc[VC_CMD].source = DSI_VC_SOURCE_L4;
> +
> +	/* Setup VC_VIDEO for HS and dispc transfers */
> +	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 9, 9); /* HS */
> +
> +	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 1, 1); /* SOURCE_VP */
> +	dsi->vc[VC_VIDEO].source = DSI_VC_SOURCE_VP;
> +
> +	if (dsi->data->quirks & DSI_QUIRK_DCS_CMD_CONFIG_VC)
> +		REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 30, 30); /* DCS_CMD_ENABLE */
> +
> +	dsi_vc_enable(dsi, VC_CMD, 1);
> +	dsi_vc_enable(dsi, VC_VIDEO, 1);
> +
> +	dsi_if_enable(dsi, 1);
> +
> +	dsi_force_tx_stop_mode_io(dsi);
> +
> +	/* start the DDR clock by sending a NULL packet */
> +	if (dsi->vm_timings.ddr_clk_always_on)
> +		dsi_vc_send_null(dsi, VC_CMD, dsi->dsidev->channel);
> +}
> +
>  static int dsi_init_dsi(struct dsi_data *dsi)
>  {
>  	int r;
> @@ -3691,13 +3674,7 @@ static int dsi_init_dsi(struct dsi_data *dsi)
>  	if (r)
>  		goto err3;
>  
> -	/* enable interface */
> -	dsi_vc_enable(dsi, 0, 1);
> -	dsi_vc_enable(dsi, 1, 1);
> -	dsi_vc_enable(dsi, 2, 1);
> -	dsi_vc_enable(dsi, 3, 1);
> -	dsi_if_enable(dsi, 1);
> -	dsi_force_tx_stop_mode_io(dsi);
> +	dsi_setup_dsi_vcs(dsi);
>  
>  	return 0;
>  err3:
Tomi Valkeinen Dec. 1, 2020, 11:51 a.m. UTC | #2 
On 01/12/2020 02:36, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Nov 24, 2020 at 02:45:34PM +0200, Tomi Valkeinen wrote:
>> As we now have a fixed setup for VCs (VC0 for video stream, VC1 for
>> commands), we can simplify the VC setup.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dsi.c | 85 +++++++++++--------------------
>>  1 file changed, 31 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> index ff8ace957291..27d0d119668b 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> @@ -2017,40 +2017,6 @@ static void dsi_vc_initial_config(struct dsi_data *dsi, int vc)
>>  	dsi->vc[vc].source = DSI_VC_SOURCE_L4;
>>  }
>>  
>> -static int dsi_vc_config_source(struct dsi_data *dsi, int vc,
>> -				enum dsi_vc_source source)
>> -{
>> -	if (dsi->vc[vc].source == source)
>> -		return 0;
>> -
>> -	DSSDBG("Source config of VC %d", vc);
>> -
>> -	dsi_sync_vc(dsi, vc);
>> -
>> -	dsi_vc_enable(dsi, vc, 0);
>> -
>> -	/* VC_BUSY */
>> -	if (!wait_for_bit_change(dsi, DSI_VC_CTRL(vc), 15, 0)) {
>> -		DSSERR("vc(%d) busy when trying to config for VP\n", vc);
>> -		return -EIO;
>> -	}
>> -
>> -	/* SOURCE, 0 = L4, 1 = video port */
>> -	REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), source, 1, 1);
>> -
>> -	/* DCS_CMD_ENABLE */
>> -	if (dsi->data->quirks & DSI_QUIRK_DCS_CMD_CONFIG_VC) {
>> -		bool enable = source == DSI_VC_SOURCE_VP;
>> -		REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), enable, 30, 30);
>> -	}
>> -
>> -	dsi_vc_enable(dsi, vc, 1);
>> -
>> -	dsi->vc[vc].source = source;
>> -
>> -	return 0;
>> -}
>> -
>>  static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
>>  		bool enable)
>>  {
>> @@ -2072,10 +2038,6 @@ static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
>>  	dsi_if_enable(dsi, 1);
>>  
>>  	dsi_force_tx_stop_mode_io(dsi);
>> -
>> -	/* start the DDR clock by sending a NULL packet */
>> -	if (dsi->vm_timings.ddr_clk_always_on && enable)
>> -		dsi_vc_send_null(dsi, vc, dsi->dsidev->channel);
>>  }
>>  
>>  static void dsi_vc_flush_long_data(struct dsi_data *dsi, int vc)
>> @@ -2270,8 +2232,6 @@ static int dsi_vc_send_long(struct dsi_data *dsi, int vc,
>>  		return -EINVAL;
>>  	}
>>  
>> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
>> -
>>  	dsi_vc_write_long_header(dsi, vc, msg->channel, msg->type, msg->tx_len, 0);
>>  
>>  	p = msg->tx_buf;
>> @@ -2331,8 +2291,6 @@ static int dsi_vc_send_short(struct dsi_data *dsi, int vc,
>>  		DSSDBG("dsi_vc_send_short(ch%d, dt %#x, b1 %#x, b2 %#x)\n",
>>  		       vc, msg->type, pkt.header[1], pkt.header[2]);
>>  
>> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
>> -
>>  	if (FLD_GET(dsi_read_reg(dsi, DSI_VC_CTRL(vc)), 16, 16)) {
>>  		DSSERR("ERROR FIFO FULL, aborting transfer\n");
>>  		return -EINVAL;
>> @@ -3351,8 +3309,6 @@ static void dsi_update_screen_dispc(struct dsi_data *dsi)
>>  
>>  	DSSDBG("dsi_update_screen_dispc(%dx%d)\n", w, h);
>>  
>> -	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_VP);
>> -
>>  	bytespp	= mipi_dsi_pixel_format_to_bpp(dsi->pix_fmt) / 8;
>>  	bytespl = w * bytespp;
>>  	bytespf = bytespl * h;
>> @@ -3522,9 +3478,7 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
>>  
>>  	dsi_set_ulps_auto(dsi, false);
>>  
>> -	dsi_vc_enable_hs(dssdev, vc, !(dsi->dsidev->mode_flags & MIPI_DSI_MODE_LPM));
> 
> Why is this not needed anymore ?

This is in dsi_update_channel(), so about sending the frame. We always want to send the frame in HS,
and we set the VC to HS in the dsi_setup_dsi_vcs() in this patch. So there's no longer need to
configure the speed for each dsi_update_channel invocation.

So there's a slight change in behavior, as before this patch you could send frames in LP, but can't
after this. But who would want to do that...

I think I can clean this up by changing "drm/omap: dsi: set LP/HS before update" to "always enable
HS before update". And then dropping the line here.

 Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index ff8ace957291..27d0d119668b 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -2017,40 +2017,6 @@  static void dsi_vc_initial_config(struct dsi_data *dsi, int vc)
 	dsi->vc[vc].source = DSI_VC_SOURCE_L4;
 }
 
-static int dsi_vc_config_source(struct dsi_data *dsi, int vc,
-				enum dsi_vc_source source)
-{
-	if (dsi->vc[vc].source == source)
-		return 0;
-
-	DSSDBG("Source config of VC %d", vc);
-
-	dsi_sync_vc(dsi, vc);
-
-	dsi_vc_enable(dsi, vc, 0);
-
-	/* VC_BUSY */
-	if (!wait_for_bit_change(dsi, DSI_VC_CTRL(vc), 15, 0)) {
-		DSSERR("vc(%d) busy when trying to config for VP\n", vc);
-		return -EIO;
-	}
-
-	/* SOURCE, 0 = L4, 1 = video port */
-	REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), source, 1, 1);
-
-	/* DCS_CMD_ENABLE */
-	if (dsi->data->quirks & DSI_QUIRK_DCS_CMD_CONFIG_VC) {
-		bool enable = source == DSI_VC_SOURCE_VP;
-		REG_FLD_MOD(dsi, DSI_VC_CTRL(vc), enable, 30, 30);
-	}
-
-	dsi_vc_enable(dsi, vc, 1);
-
-	dsi->vc[vc].source = source;
-
-	return 0;
-}
-
 static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
 		bool enable)
 {
@@ -2072,10 +2038,6 @@  static void dsi_vc_enable_hs(struct omap_dss_device *dssdev, int vc,
 	dsi_if_enable(dsi, 1);
 
 	dsi_force_tx_stop_mode_io(dsi);
-
-	/* start the DDR clock by sending a NULL packet */
-	if (dsi->vm_timings.ddr_clk_always_on && enable)
-		dsi_vc_send_null(dsi, vc, dsi->dsidev->channel);
 }
 
 static void dsi_vc_flush_long_data(struct dsi_data *dsi, int vc)
@@ -2270,8 +2232,6 @@  static int dsi_vc_send_long(struct dsi_data *dsi, int vc,
 		return -EINVAL;
 	}
 
-	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
-
 	dsi_vc_write_long_header(dsi, vc, msg->channel, msg->type, msg->tx_len, 0);
 
 	p = msg->tx_buf;
@@ -2331,8 +2291,6 @@  static int dsi_vc_send_short(struct dsi_data *dsi, int vc,
 		DSSDBG("dsi_vc_send_short(ch%d, dt %#x, b1 %#x, b2 %#x)\n",
 		       vc, msg->type, pkt.header[1], pkt.header[2]);
 
-	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_L4);
-
 	if (FLD_GET(dsi_read_reg(dsi, DSI_VC_CTRL(vc)), 16, 16)) {
 		DSSERR("ERROR FIFO FULL, aborting transfer\n");
 		return -EINVAL;
@@ -3351,8 +3309,6 @@  static void dsi_update_screen_dispc(struct dsi_data *dsi)
 
 	DSSDBG("dsi_update_screen_dispc(%dx%d)\n", w, h);
 
-	dsi_vc_config_source(dsi, vc, DSI_VC_SOURCE_VP);
-
 	bytespp	= mipi_dsi_pixel_format_to_bpp(dsi->pix_fmt) / 8;
 	bytespl = w * bytespp;
 	bytespf = bytespl * h;
@@ -3522,9 +3478,7 @@  static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
 
 	dsi_set_ulps_auto(dsi, false);
 
-	dsi_vc_enable_hs(dssdev, vc, !(dsi->dsidev->mode_flags & MIPI_DSI_MODE_LPM));
-
-	r = _dsi_send_nop(dsi, vc, dsi->dsidev->channel);
+	r = _dsi_send_nop(dsi, VC_CMD, dsi->dsidev->channel);
 	if (r < 0) {
 		DSSWARN("failed to send nop between frames: %d\n", r);
 		goto err;
@@ -3649,6 +3603,35 @@  static int dsi_configure_dsi_clocks(struct dsi_data *dsi)
 	return 0;
 }
 
+static void dsi_setup_dsi_vcs(struct dsi_data *dsi)
+{
+	/* Setup VC_CMD for LP and cpu transfers */
+	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_CMD), 0, 9, 9); /* LP */
+
+	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_CMD), 0, 1, 1); /* SOURCE_L4 */
+	dsi->vc[VC_CMD].source = DSI_VC_SOURCE_L4;
+
+	/* Setup VC_VIDEO for HS and dispc transfers */
+	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 9, 9); /* HS */
+
+	REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 1, 1); /* SOURCE_VP */
+	dsi->vc[VC_VIDEO].source = DSI_VC_SOURCE_VP;
+
+	if (dsi->data->quirks & DSI_QUIRK_DCS_CMD_CONFIG_VC)
+		REG_FLD_MOD(dsi, DSI_VC_CTRL(VC_VIDEO), 1, 30, 30); /* DCS_CMD_ENABLE */
+
+	dsi_vc_enable(dsi, VC_CMD, 1);
+	dsi_vc_enable(dsi, VC_VIDEO, 1);
+
+	dsi_if_enable(dsi, 1);
+
+	dsi_force_tx_stop_mode_io(dsi);
+
+	/* start the DDR clock by sending a NULL packet */
+	if (dsi->vm_timings.ddr_clk_always_on)
+		dsi_vc_send_null(dsi, VC_CMD, dsi->dsidev->channel);
+}
+
 static int dsi_init_dsi(struct dsi_data *dsi)
 {
 	int r;
@@ -3691,13 +3674,7 @@  static int dsi_init_dsi(struct dsi_data *dsi)
 	if (r)
 		goto err3;
 
-	/* enable interface */
-	dsi_vc_enable(dsi, 0, 1);
-	dsi_vc_enable(dsi, 1, 1);
-	dsi_vc_enable(dsi, 2, 1);
-	dsi_vc_enable(dsi, 3, 1);
-	dsi_if_enable(dsi, 1);
-	dsi_force_tx_stop_mode_io(dsi);
+	dsi_setup_dsi_vcs(dsi);
 
 	return 0;
 err3: