From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Keeping Subject: Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands Date: Tue, 31 Jan 2017 12:41:47 +0000 Message-ID: <20170131124147.170d8588.john@metanate.com> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170130201609.GM20076@art_vandelay> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Sean Paul Cc: Chris Zhong , linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gTW9uLCAzMCBKYW4gMjAxNyAxNToxNjowOSAtMDUwMCwgU2VhbiBQYXVsIHdyb3RlOgoKPiBP biBNb24sIEphbiAzMCwgMjAxNyBhdCAwNjoxNDoyN1BNICswMDAwLCBKb2huIEtlZXBpbmcgd3Jv dGU6Cj4gPiBPbiBNb24sIDMwIEphbiAyMDE3IDEwOjI2OjExIC0wNTAwLCBTZWFuIFBhdWwgd3Jv dGU6Cj4gPiAgIAo+ID4gPiBPbiBTdW4sIEphbiAyOSwgMjAxNyBhdCAwMToyNDo0NFBNICswMDAw LCBKb2huIEtlZXBpbmcgd3JvdGU6ICAKPiA+ID4gPiBJIGhhdmVuJ3QgZm91bmQgYW55IG1ldGhv ZCBmb3IgZ2V0dGluZyB0aGUgbGVuZ3RoIG9mIGEgcmVzcG9uc2UsIHNvIHRoaXMKPiA+ID4gPiBq dXN0IHVzZXMgdGhlIHJlcXVlc3RlZCByeF9sZW4KPiA+ID4gPiAKPiA+ID4gPiBTaWduZWQtb2Zm LWJ5OiBKb2huIEtlZXBpbmcgPGpvaG5AbWV0YW5hdGUuY29tPgo+ID4gPiA+IC0tLQo+ID4gPiA+ IHYzOgo+ID4gPiA+IC0gRml4IGNoZWNrcGF0Y2ggd2FybmluZ3MKPiA+ID4gPiBVbmNoYW5nZWQg aW4gdjIKPiA+ID4gPiAKPiA+ID4gPiAgZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3LW1pcGkt ZHNpLmMgfCA1NiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrCj4gPiA+ID4gIDEg ZmlsZSBjaGFuZ2VkLCA1NiBpbnNlcnRpb25zKCspCj4gPiA+ID4gCj4gPiA+ID4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaS5jIGIvZHJpdmVycy9ncHUv ZHJtL3JvY2tjaGlwL2R3LW1pcGktZHNpLmMKPiA+ID4gPiBpbmRleCBjZjNjYTZiMGNiZGIuLmNj NThhZGE3NTQyNSAxMDA2NDQKPiA+ID4gPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAv ZHctbWlwaS1kc2kuYwo+ID4gPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kdy1t aXBpLWRzaS5jCj4gPiA+ID4gQEAgLTY3OCw2ICs2NzgsNTYgQEAgc3RhdGljIGludCBkd19taXBp X2RzaV9kY3NfbG9uZ193cml0ZShzdHJ1Y3QgZHdfbWlwaV9kc2kgKmRzaSwKPiA+ID4gPiAgCXJl dHVybiBkd19taXBpX2RzaV9nZW5fcGt0X2hkcl93cml0ZShkc2ksIGhkcl92YWwpOwo+ID4gPiA+ ICB9Cj4gPiA+ID4gIAo+ID4gPiA+ICtzdGF0aWMgaW50IGR3X21pcGlfZHNpX2Rjc19yZWFkKHN0 cnVjdCBkd19taXBpX2RzaSAqZHNpLAo+ID4gPiA+ICsJCQkJY29uc3Qgc3RydWN0IG1pcGlfZHNp X21zZyAqbXNnKQo+ID4gPiA+ICt7Cj4gPiA+ID4gKwljb25zdCB1OCAqdHhfYnVmID0gbXNnLT50 eF9idWY7Cj4gPiA+ID4gKwl1OCAqcnhfYnVmID0gbXNnLT5yeF9idWY7Cj4gPiA+ID4gKwlzaXpl X3QgaTsKPiA+ID4gPiArCWludCByZXQsIHZhbDsKPiA+ID4gPiArCj4gPiA+ID4gKwlkc2lfd3Jp dGUoZHNpLCBEU0lfUENLSERMX0NGRywgRU5fQ1JDX1JYIHwgRU5fRUNDX1JYIHwgRU5fQlRBKTsK PiA+ID4gPiArCWRzaV93cml0ZShkc2ksIERTSV9HRU5fSERSLAo+ID4gPiA+ICsJCSAgR0VOX0hE QVRBKHR4X2J1ZlswXSkgfCBHRU5fSFRZUEUobXNnLT50eXBlKSk7Cj4gPiA+ID4gKwo+ID4gPiA+ ICsJcmV0ID0gcmVhZGxfcG9sbF90aW1lb3V0KGRzaS0+YmFzZSArIERTSV9DTURfUEtUX1NUQVRV UywKPiA+ID4gPiArCQkJCSB2YWwsICEodmFsICYgR0VOX1JEX0NNRF9CVVNZKSwgMTAwMCwKPiA+ ID4gPiArCQkJCSBDTURfUEtUX1NUQVRVU19USU1FT1VUX1VTKTsKPiA+ID4gPiArCWlmIChyZXQg PCAwKSB7Cj4gPiA+ID4gKwkJZGV2X2Vycihkc2ktPmRldiwgImZhaWxlZCB0byByZWFkIGNvbW1h bmQgcmVzcG9uc2VcbiIpOwo+ID4gPiA+ICsJCXJldHVybiByZXQ7Cj4gPiA+ID4gKwl9Cj4gPiA+ ID4gKwo+ID4gPiA+ICsJZm9yIChpID0gMDsgaSA8IG1zZy0+cnhfbGVuOykgewo+ID4gPiA+ICsJ CXUzMiBwbGQgPSBkc2lfcmVhZChkc2ksIERTSV9HRU5fUExEX0RBVEEpOwo+ID4gPiA+ICsKPiA+ ID4gPiArCQl3aGlsZSAoaSA8IG1zZy0+cnhfbGVuKSB7Cj4gPiA+ID4gKwkJCXJ4X2J1ZltpXSA9 IHBsZCAmIDB4ZmY7Cj4gPiA+ID4gKwkJCXBsZCA+Pj0gODsKPiA+ID4gPiArCQkJaSsrOwo+ID4g PiA+ICsJCX0KPiA+ID4gPiArCX0gICAgCj4gPiA+IAo+ID4gPiBBRkFJQ1QsIHRoZSBvdXRlciBm b3IgbG9vcCBqdXN0IGluaXRpYWxpemVzIGkgYW5kIGVuc3VyZXMgbXNnLT5yeF9sZW4gaXMKPiA+ ID4gbm9uLXplcm8/IAo+ID4gPiAKPiA+ID4gSSB0aGluayB0aGUgZm9sbG93aW5nIHdvdWxkIGJl IGVhc2llciB0byByZWFkIChhbmQgc2FmZSBhZ2FpbnN0IHRoZSBjYXNlIHdoZXJlCj4gPiA+IG1z Zy0+cnhfbGVuID4gc2l6ZW9mKHBsZCkgKGV2ZW4gdGhvdWdoIHRoaXMgc2hvdWxkbid0IGhhcHBl biBhY2NvcmRpbmcgdG8gRENTCj4gPiA+IHNwZWMpKS4KPiA+ID4gCj4gPiA+IGlmIChtc2ctPnJ4 X2xlbiA+IDApIHsKPiA+ID4gICAgICAgICB1MzIgcGxkID0gZHNpX3JlYWQoZHNpLCBEU0lfR0VO X1BMRF9EQVRBKTsKPiA+ID4gICAgICAgICBtZW1jcHkocnhfYnVmLCAmcGxkLCBNSU4obXNnLT5y eF9sZW4sIHNpemVvZihwbGQpKTsKPiA+ID4gfSAgCj4gPiAKPiA+IEkgdGhpbmsgdGhlIGludGVu dCB3YXMgdG8gaGFuZGxlIHJ4X2xlbiA+IDQsIGJ1dCB0aGUgcGF0Y2ggaXMgb2J2b3VzbHkKPiA+ IGNvbXBsZXRlbHkgYnJva2VuIHJlZ2FyZGluZyB0aGF0LiAgQXMgZmFyIGFzIEkgY2FuIHRlbGws IHJ4X2xlbiBpcwo+ID4gbGltaXRlZCBieSB0aGUgbWF4aW11bSByZXR1cm4gcGFja2V0IHNpemUg d2hpY2ggY2FuIGJlIGFueSB2YWx1ZSB1cCB0bwo+ID4gdGhlIG1heGltdW0gc2l6ZSBvZiBhIGxv bmcgcGFja2V0LCBzbyB3ZSBtYXkgbmVlZCB0byByZWFkIGZyb20gdGhlIEZJRk8KPiA+IG11bHRp cGxlIHRpbWVzLgo+ID4gCj4gPiBUaGUgbG9vcCBzaG91bGQgYmUgc29tZXRoaW5nIGxpa2UgdGhp czoKPiA+IAo+ID4gCWZvciAoaSA9IDA7IGkgPCBtc2ctPnJ4X2xlbjspIHsKPiA+IAkJdTMyIHBs ZCA9IGRzaV9yZWFkKGRzaSwgRFNJX0dFTl9QTERfREFUQSk7Cj4gPiAJCWludCBqOwo+ID4gCj4g PiAJCWZvciAoaiA9IDA7IGogPCA0ICYmIGkgPCBtc2ctPnJ4X2xlbjsgaSsrLCBqKyspIHsKPiA+ IAkJCXJ4X2J1ZltpXSA9IHBsZCAmIDB4ZmY7Cj4gPiAJCQlwbGQgPj49IDg7Cj4gPiAJCX0KPiA+ IAl9ICAKPiAKPiBTaG9ydCBwYWNrZXRzIHNob3VsZCBuZXZlciBleGNlZWQgMzIgYml0cywgc28g SSBkb24ndCB0aGluayB5b3UgbmVlZCB0byBhZGQgdGhlCj4gbmVzdGVkIGxvb3AuCgpUaGUgcmVh ZCByZXNwb25zZSBpcyBub3QgcmVzdHJpY3RlZCB0byBhIHNob3J0IHBhY2tldC4gIEkgaGF2ZSBh IHBhbmVsCnRoYXQgZG9jdW1lbnRzIGEgcmVhZCByZXF1ZXN0IHRoYXQgcmV0dXJucyB1cCB0byA2 NEtpQiwgYWRtaXR0ZWRseSB3aXRoCmEgY29udGludWF0aW9uIGNvbW1hbmQgYW5kIHRoZSBwYW5l bHMgSSBoYXZlIHNlZW0gdG8gb25seSBiZSBwcm9ncmFtbWVkCnRvIHJldHVybiA1IGJ5dGVzIG9m IG1lYW5pbmdmdWwgZGF0YSwgYnV0IHRoZXkgZG8gcmV0dXJuIGFsbCBvZiB0aG9zZQpieXRlcyBp biBhIHNpbmdsZSByZWFkIHJlc3BvbnNlLgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Tue, 31 Jan 2017 12:41:47 +0000 Subject: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands In-Reply-To: <20170130201609.GM20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> Message-ID: <20170131124147.170d8588.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 30 Jan 2017 15:16:09 -0500, Sean Paul wrote: > On Mon, Jan 30, 2017 at 06:14:27PM +0000, John Keeping wrote: > > On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote: > > > > > On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote: > > > > I haven't found any method for getting the length of a response, so this > > > > just uses the requested rx_len > > > > > > > > Signed-off-by: John Keeping > > > > --- > > > > v3: > > > > - Fix checkpatch warnings > > > > Unchanged in v2 > > > > > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 56 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > index cf3ca6b0cbdb..cc58ada75425 100644 > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > > > > return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); > > > > } > > > > > > > > +static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi, > > > > + const struct mipi_dsi_msg *msg) > > > > +{ > > > > + const u8 *tx_buf = msg->tx_buf; > > > > + u8 *rx_buf = msg->rx_buf; > > > > + size_t i; > > > > + int ret, val; > > > > + > > > > + dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA); > > > > + dsi_write(dsi, DSI_GEN_HDR, > > > > + GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type)); > > > > + > > > > + ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, > > > > + val, !(val & GEN_RD_CMD_BUSY), 1000, > > > > + CMD_PKT_STATUS_TIMEOUT_US); > > > > + if (ret < 0) { > > > > + dev_err(dsi->dev, "failed to read command response\n"); > > > > + return ret; > > > > + } > > > > + > > > > + for (i = 0; i < msg->rx_len;) { > > > > + u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > + > > > > + while (i < msg->rx_len) { > > > > + rx_buf[i] = pld & 0xff; > > > > + pld >>= 8; > > > > + i++; > > > > + } > > > > + } > > > > > > AFAICT, the outer for loop just initializes i and ensures msg->rx_len is > > > non-zero? > > > > > > I think the following would be easier to read (and safe against the case where > > > msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS > > > spec)). > > > > > > if (msg->rx_len > 0) { > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); > > > } > > > > I think the intent was to handle rx_len > 4, but the patch is obvously > > completely broken regarding that. As far as I can tell, rx_len is > > limited by the maximum return packet size which can be any value up to > > the maximum size of a long packet, so we may need to read from the FIFO > > multiple times. > > > > The loop should be something like this: > > > > for (i = 0; i < msg->rx_len;) { > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > int j; > > > > for (j = 0; j < 4 && i < msg->rx_len; i++, j++) { > > rx_buf[i] = pld & 0xff; > > pld >>= 8; > > } > > } > > Short packets should never exceed 32 bits, so I don't think you need to add the > nested loop. The read response is not restricted to a short packet. I have a panel that documents a read request that returns up to 64KiB, admittedly with a continuation command and the panels I have seem to only be programmed to return 5 bytes of meaningful data, but they do return all of those bytes in a single read response. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751996AbdAaMnI (ORCPT ); Tue, 31 Jan 2017 07:43:08 -0500 Received: from dougal.metanate.com ([90.155.101.14]:44601 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751882AbdAaMm6 (ORCPT ); Tue, 31 Jan 2017 07:42:58 -0500 Date: Tue, 31 Jan 2017 12:41:47 +0000 From: John Keeping To: Sean Paul Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, Chris Zhong , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands Message-ID: <20170131124147.170d8588.john@metanate.com> In-Reply-To: <20170130201609.GM20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> <20170130181427.1940024f.john@metanate.com> <20170130201609.GM20076@art_vandelay> Organization: Metanate Ltd X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 30 Jan 2017 15:16:09 -0500, Sean Paul wrote: > On Mon, Jan 30, 2017 at 06:14:27PM +0000, John Keeping wrote: > > On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote: > > > > > On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote: > > > > I haven't found any method for getting the length of a response, so this > > > > just uses the requested rx_len > > > > > > > > Signed-off-by: John Keeping > > > > --- > > > > v3: > > > > - Fix checkpatch warnings > > > > Unchanged in v2 > > > > > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 56 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > index cf3ca6b0cbdb..cc58ada75425 100644 > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > > > > return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); > > > > } > > > > > > > > +static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi, > > > > + const struct mipi_dsi_msg *msg) > > > > +{ > > > > + const u8 *tx_buf = msg->tx_buf; > > > > + u8 *rx_buf = msg->rx_buf; > > > > + size_t i; > > > > + int ret, val; > > > > + > > > > + dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA); > > > > + dsi_write(dsi, DSI_GEN_HDR, > > > > + GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type)); > > > > + > > > > + ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, > > > > + val, !(val & GEN_RD_CMD_BUSY), 1000, > > > > + CMD_PKT_STATUS_TIMEOUT_US); > > > > + if (ret < 0) { > > > > + dev_err(dsi->dev, "failed to read command response\n"); > > > > + return ret; > > > > + } > > > > + > > > > + for (i = 0; i < msg->rx_len;) { > > > > + u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > > + > > > > + while (i < msg->rx_len) { > > > > + rx_buf[i] = pld & 0xff; > > > > + pld >>= 8; > > > > + i++; > > > > + } > > > > + } > > > > > > AFAICT, the outer for loop just initializes i and ensures msg->rx_len is > > > non-zero? > > > > > > I think the following would be easier to read (and safe against the case where > > > msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS > > > spec)). > > > > > > if (msg->rx_len > 0) { > > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > > memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); > > > } > > > > I think the intent was to handle rx_len > 4, but the patch is obvously > > completely broken regarding that. As far as I can tell, rx_len is > > limited by the maximum return packet size which can be any value up to > > the maximum size of a long packet, so we may need to read from the FIFO > > multiple times. > > > > The loop should be something like this: > > > > for (i = 0; i < msg->rx_len;) { > > u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); > > int j; > > > > for (j = 0; j < 4 && i < msg->rx_len; i++, j++) { > > rx_buf[i] = pld & 0xff; > > pld >>= 8; > > } > > } > > Short packets should never exceed 32 bits, so I don't think you need to add the > nested loop. The read response is not restricted to a short packet. I have a panel that documents a read request that returns up to 64KiB, admittedly with a continuation command and the panels I have seem to only be programmed to return 5 bytes of meaningful data, but they do return all of those bytes in a single read response.