Re: [PATCH v2] drm/mcde: Retry DSI read/write transactions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 14, 2020 at 09:44:51PM +0200, Linus Walleij wrote:
> The vendor driver makes a few retries on read DSI
> transactions, something that is needed especially in
> case of read (such as reading the panel MTP ID) while
> the panel is running in video mode. This happens on
> the Samsung s6e63m0 panel on the Golden device.
> 
> Retry reads and writes alike three times.
> 
> Cc: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Retry three times.
> - Only retry the actual command transmission like the vendor
>   driver does, no need to set up all registers and do checks
>   all over. Break out a part of the mcde_dsi_host_transfer()
>   function to achieve this.
> ---
>  drivers/gpu/drm/mcde/mcde_dsi.c | 158 +++++++++++++++++++-------------
>  1 file changed, 92 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index 4ce8cc5f0be2..b3c5d3cbda92 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -208,79 +208,16 @@ static int mcde_dsi_host_detach(struct mipi_dsi_host *host,
>  	 (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
>  	 (type == MIPI_DSI_DCS_READ))
>  
> -static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
> -				      const struct mipi_dsi_msg *msg)
> +static int mcde_dsi_execute_transfer(struct mcde_dsi *d,
> +				     const struct mipi_dsi_msg *msg)
>  {
> -	struct mcde_dsi *d = host_to_mcde_dsi(host);
>  	const u32 loop_delay_us = 10; /* us */
> -	const u8 *tx = msg->tx_buf;
>  	u32 loop_counter;
>  	size_t txlen = msg->tx_len;
>  	size_t rxlen = msg->rx_len;
> +	int i;
>  	u32 val;
>  	int ret;
> -	int i;
> -
> -	if (txlen > 16) {
> -		dev_err(d->dev,
> -			"dunno how to write more than 16 bytes yet\n");
> -		return -EIO;
> -	}
> -	if (rxlen > 4) {
> -		dev_err(d->dev,
> -			"dunno how to read more than 4 bytes yet\n");
> -		return -EIO;
> -	}
> -
> -	dev_dbg(d->dev,
> -		"message to channel %d, write %zd bytes read %zd bytes\n",
> -		msg->channel, txlen, rxlen);
> -
> -	/* Command "nature" */
> -	if (MCDE_DSI_HOST_IS_READ(msg->type))
> -		/* MCTL_MAIN_DATA_CTL already set up */
> -		val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_READ;
> -	else
> -		val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_WRITE;
> -	/*
> -	 * More than 2 bytes will not fit in a single packet, so it's
> -	 * time to set the "long not short" bit. One byte is used by
> -	 * the MIPI DCS command leaving just one byte for the payload
> -	 * in a short package.
> -	 */
> -	if (mipi_dsi_packet_format_is_long(msg->type))
> -		val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LONGNOTSHORT;
> -	val |= 0 << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_ID_SHIFT;
> -	val |= txlen << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_SIZE_SHIFT;
> -	val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LP_EN;
> -	val |= msg->type << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_HEAD_SHIFT;
> -	writel(val, d->regs + DSI_DIRECT_CMD_MAIN_SETTINGS);
> -
> -	/* MIPI DCS command is part of the data */
> -	if (txlen > 0) {
> -		val = 0;
> -		for (i = 0; i < 4 && i < txlen; i++)
> -			val |= tx[i] << (i * 8);
> -	}
> -	writel(val, d->regs + DSI_DIRECT_CMD_WRDAT0);
> -	if (txlen > 4) {
> -		val = 0;
> -		for (i = 0; i < 4 && (i + 4) < txlen; i++)
> -			val |= tx[i + 4] << (i * 8);
> -		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT1);
> -	}
> -	if (txlen > 8) {
> -		val = 0;
> -		for (i = 0; i < 4 && (i + 8) < txlen; i++)
> -			val |= tx[i + 8] << (i * 8);
> -		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT2);
> -	}
> -	if (txlen > 12) {
> -		val = 0;
> -		for (i = 0; i < 4 && (i + 12) < txlen; i++)
> -			val |= tx[i + 12] << (i * 8);
> -		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT3);
> -	}
>  
>  	writel(~0, d->regs + DSI_DIRECT_CMD_STS_CLR);
>  	writel(~0, d->regs + DSI_CMD_MODE_STS_CLR);
> @@ -297,6 +234,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
>  			usleep_range(loop_delay_us, (loop_delay_us * 3) / 2);
>  		if (!loop_counter) {
>  			dev_err(d->dev, "DSI read timeout!\n");
> +			/* Set exit code and retry */
>  			return -ETIME;
>  		}
>  	} else {
> @@ -307,6 +245,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
>  			usleep_range(loop_delay_us, (loop_delay_us * 3) / 2);
>  
>  		if (!loop_counter) {
> +			/* Set exit code and retry */
>  			dev_err(d->dev, "DSI write timeout!\n");
>  			return -ETIME;
>  		}
> @@ -348,6 +287,93 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
>  		ret = rdsz;
>  	}
>  
> +	/* Successful transmission */
> +	return ret;
> +}
> +
> +static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host,
> +				      const struct mipi_dsi_msg *msg)
> +{
> +	struct mcde_dsi *d = host_to_mcde_dsi(host);
> +	const u8 *tx = msg->tx_buf;
> +	size_t txlen = msg->tx_len;
> +	size_t rxlen = msg->rx_len;
> +	unsigned int retries = 0;
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	if (txlen > 16) {
> +		dev_err(d->dev,
> +			"dunno how to write more than 16 bytes yet\n");
> +		return -EIO;
> +	}
> +	if (rxlen > 4) {
> +		dev_err(d->dev,
> +			"dunno how to read more than 4 bytes yet\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(d->dev,
> +		"message to channel %d, write %zd bytes read %zd bytes\n",
> +		msg->channel, txlen, rxlen);
> +
> +	/* Command "nature" */
> +	if (MCDE_DSI_HOST_IS_READ(msg->type))
> +		/* MCTL_MAIN_DATA_CTL already set up */
> +		val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_READ;
> +	else
> +		val = DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_NAT_WRITE;
> +	/*
> +	 * More than 2 bytes will not fit in a single packet, so it's
> +	 * time to set the "long not short" bit. One byte is used by
> +	 * the MIPI DCS command leaving just one byte for the payload
> +	 * in a short package.
> +	 */
> +	if (mipi_dsi_packet_format_is_long(msg->type))
> +		val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LONGNOTSHORT;
> +	val |= 0 << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_ID_SHIFT;
> +	val |= txlen << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_SIZE_SHIFT;
> +	val |= DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_LP_EN;
> +	val |= msg->type << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_HEAD_SHIFT;
> +	writel(val, d->regs + DSI_DIRECT_CMD_MAIN_SETTINGS);
> +
> +	/* MIPI DCS command is part of the data */
> +	if (txlen > 0) {
> +		val = 0;
> +		for (i = 0; i < 4 && i < txlen; i++)
> +			val |= tx[i] << (i * 8);
> +	}
> +	writel(val, d->regs + DSI_DIRECT_CMD_WRDAT0);
> +	if (txlen > 4) {
> +		val = 0;
> +		for (i = 0; i < 4 && (i + 4) < txlen; i++)
> +			val |= tx[i + 4] << (i * 8);
> +		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT1);
> +	}
> +	if (txlen > 8) {
> +		val = 0;
> +		for (i = 0; i < 4 && (i + 8) < txlen; i++)
> +			val |= tx[i + 8] << (i * 8);
> +		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT2);
> +	}
> +	if (txlen > 12) {
> +		val = 0;
> +		for (i = 0; i < 4 && (i + 12) < txlen; i++)
> +			val |= tx[i + 12] << (i * 8);
> +		writel(val, d->regs + DSI_DIRECT_CMD_WRDAT3);
> +	}
> +
> +	while (retries < 3) {
> +		ret = mcde_dsi_execute_transfer(d, msg);
> +		if (ret >= 0)
> +			break;
> +		retries++;
> +	}

It might be a bit nicer to write this as a for loop, i.e.

	for (retries = 0; retries < 3; retries++) {
		ret = mcde_dsi_execute_transfer(d, msg);
		if (ret >= 0)
			break;
	}

But I guess it does not make much of a difference here.

Just a thought, looks good to me otherwise!
Reviewed-by: Stephan Gerhold <stephan@xxxxxxxxxxx>

Thanks!
Stephan

> +	if (ret < 0 && retries)
> +		dev_err(d->dev, "gave up after %d retries\n", retries);
> +
> +	/* Clear any errors */
>  	writel(~0, d->regs + DSI_DIRECT_CMD_STS_CLR);
>  	writel(~0, d->regs + DSI_CMD_MODE_STS_CLR);
>  
> -- 
> 2.26.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux