| Message ID | 20190903081129.19617-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] drm/mcde: Fix DSI transfers | expand |
On Tue, Sep 03, 2019 at 10:11:29AM +0200, Linus Walleij wrote: > There were bugs in the DSI transfer (read and write) function > as it was only tested with displays ever needing a single byte > to be written. Fixed it up and tested so we can now write > messages of up to 16 bytes and read up to 4 bytes from the > display. > > Tested with a Sony ACX424AKP display: this display now self- > identifies and can control backlight in command mode. > > Reported-by: kbuild test robot <lkp@intel.com> > Fixes: 5fc537bfd000 ("drm/mcde: Add new driver for ST-Ericsson MCDE") > Reviewed-by: Stephan Gerhold <stephan@gerhold.net> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v2->v3: > - Fix an error message to indicate reading error rather than > writing error. > - Use the local variable for underflow print. > - Collected Stephan's reviewed-by. > ChangeLog v1->v2: > - Fix a print modifier for dev_err() found by the build robot. > --- > drivers/gpu/drm/mcde/mcde_dsi.c | 70 ++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c > index 07f7090d08b3..cd261c266f35 100644 > --- a/drivers/gpu/drm/mcde/mcde_dsi.c > +++ b/drivers/gpu/drm/mcde/mcde_dsi.c > @@ -178,22 +178,26 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, > const u32 loop_delay_us = 10; /* us */ > const u8 *tx = msg->tx_buf; > u32 loop_counter; > - size_t txlen; > + size_t txlen = msg->tx_len; > + size_t rxlen = msg->rx_len; > u32 val; > int ret; > int i; > > - txlen = msg->tx_len; > - if (txlen > 12) { > + if (txlen > 16) { > dev_err(d->dev, > - "dunno how to write more than 12 bytes yet\n"); > + "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, %zd bytes", > - msg->channel, > - txlen); > + "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)) > @@ -210,9 +214,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, > 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; > - /* Add one to the length for the MIPI DCS command */ > - val |= txlen > - << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_SIZE_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); > @@ -249,17 +251,36 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, > writel(1, d->regs + DSI_DIRECT_CMD_SEND); > > loop_counter = 1000 * 1000 / loop_delay_us; > - while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & > - DSI_DIRECT_CMD_STS_WRITE_COMPLETED) > - && --loop_counter) > - usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); > - > - if (!loop_counter) { > - dev_err(d->dev, "DSI write timeout!\n"); > - return -ETIME; > + if (MCDE_DSI_HOST_IS_READ(msg->type)) { > + /* Read command */ > + while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & > + (DSI_DIRECT_CMD_STS_READ_COMPLETED | > + DSI_DIRECT_CMD_STS_READ_COMPLETED_WITH_ERR)) > + && --loop_counter) > + usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); > + if (!loop_counter) { > + dev_err(d->dev, "DSI write timeout!\n"); It looks like you changed the wrong message. This one is the "read timeout", > + return -ETIME; > + } > + } else { > + /* Writing only */ > + while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & > + DSI_DIRECT_CMD_STS_WRITE_COMPLETED) > + && --loop_counter) > + usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); > + > + if (!loop_counter) { > + dev_err(d->dev, "DSI read timeout!\n"); and this one the "write timeout".
diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 07f7090d08b3..cd261c266f35 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -178,22 +178,26 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, const u32 loop_delay_us = 10; /* us */ const u8 *tx = msg->tx_buf; u32 loop_counter; - size_t txlen; + size_t txlen = msg->tx_len; + size_t rxlen = msg->rx_len; u32 val; int ret; int i; - txlen = msg->tx_len; - if (txlen > 12) { + if (txlen > 16) { dev_err(d->dev, - "dunno how to write more than 12 bytes yet\n"); + "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, %zd bytes", - msg->channel, - txlen); + "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)) @@ -210,9 +214,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, 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; - /* Add one to the length for the MIPI DCS command */ - val |= txlen - << DSI_DIRECT_CMD_MAIN_SETTINGS_CMD_SIZE_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); @@ -249,17 +251,36 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, writel(1, d->regs + DSI_DIRECT_CMD_SEND); loop_counter = 1000 * 1000 / loop_delay_us; - while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & - DSI_DIRECT_CMD_STS_WRITE_COMPLETED) - && --loop_counter) - usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); - - if (!loop_counter) { - dev_err(d->dev, "DSI write timeout!\n"); - return -ETIME; + if (MCDE_DSI_HOST_IS_READ(msg->type)) { + /* Read command */ + while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & + (DSI_DIRECT_CMD_STS_READ_COMPLETED | + DSI_DIRECT_CMD_STS_READ_COMPLETED_WITH_ERR)) + && --loop_counter) + usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); + if (!loop_counter) { + dev_err(d->dev, "DSI write timeout!\n"); + return -ETIME; + } + } else { + /* Writing only */ + while (!(readl(d->regs + DSI_DIRECT_CMD_STS) & + DSI_DIRECT_CMD_STS_WRITE_COMPLETED) + && --loop_counter) + usleep_range(loop_delay_us, (loop_delay_us * 3) / 2); + + if (!loop_counter) { + dev_err(d->dev, "DSI read timeout!\n"); + return -ETIME; + } } val = readl(d->regs + DSI_DIRECT_CMD_STS); + if (val & DSI_DIRECT_CMD_STS_READ_COMPLETED_WITH_ERR) { + dev_err(d->dev, "read completed with error\n"); + writel(1, d->regs + DSI_DIRECT_CMD_RD_INIT); + return -EIO; + } if (val & DSI_DIRECT_CMD_STS_ACKNOWLEDGE_WITH_ERR_RECEIVED) { val >>= DSI_DIRECT_CMD_STS_ACK_VAL_SHIFT; dev_err(d->dev, "error during transmission: %04x\n", @@ -269,10 +290,7 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, if (!MCDE_DSI_HOST_IS_READ(msg->type)) { /* Return number of bytes written */ - if (mipi_dsi_packet_format_is_long(msg->type)) - ret = 4 + txlen; - else - ret = 4; + ret = txlen; } else { /* OK this is a read command, get the response */ u32 rdsz; @@ -282,7 +300,13 @@ static ssize_t mcde_dsi_host_transfer(struct mipi_dsi_host *host, rdsz = readl(d->regs + DSI_DIRECT_CMD_RD_PROPERTY); rdsz &= DSI_DIRECT_CMD_RD_PROPERTY_RD_SIZE_MASK; rddat = readl(d->regs + DSI_DIRECT_CMD_RDDAT); - for (i = 0; i < 4 && i < rdsz; i++) + if (rdsz < rxlen) { + dev_err(d->dev, "read error, requested %zd got %zd\n", + msg->rx_len, rxlen); + return -EIO; + } + /* FIXME: read more than 4 bytes */ + for (i = 0; i < 4 && i < rxlen; i++) rx[i] = (rddat >> (i * 8)) & 0xff; ret = rdsz; }
There were bugs in the DSI transfer (read and write) function as it was only tested with displays ever needing a single byte to be written. Fixed it up and tested so we can now write messages of up to 16 bytes and read up to 4 bytes from the display. Tested with a Sony ACX424AKP display: this display now self- identifies and can control backlight in command mode. Reported-by: kbuild test robot <lkp@intel.com> Fixes: 5fc537bfd000 ("drm/mcde: Add new driver for ST-Ericsson MCDE") Reviewed-by: Stephan Gerhold <stephan@gerhold.net> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - Fix an error message to indicate reading error rather than writing error. - Use the local variable for underflow print. - Collected Stephan's reviewed-by. ChangeLog v1->v2: - Fix a print modifier for dev_err() found by the build robot. --- drivers/gpu/drm/mcde/mcde_dsi.c | 70 ++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 23 deletions(-)