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
