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: Mon, 30 Jan 2017 18:14:27 +0000 Message-ID: <20170130181427.1940024f.john@metanate.com> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170130152611.GA20076@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: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, Chris Zhong , linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gTW9uLCAzMCBKYW4gMjAxNyAxMDoyNjoxMSAtMDUwMCwgU2VhbiBQYXVsIHdyb3RlOgoKPiBP biBTdW4sIEphbiAyOSwgMjAxNyBhdCAwMToyNDo0NFBNICswMDAwLCBKb2huIEtlZXBpbmcgd3Jv dGU6Cj4gPiBJIGhhdmVuJ3QgZm91bmQgYW55IG1ldGhvZCBmb3IgZ2V0dGluZyB0aGUgbGVuZ3Ro IG9mIGEgcmVzcG9uc2UsIHNvIHRoaXMKPiA+IGp1c3QgdXNlcyB0aGUgcmVxdWVzdGVkIHJ4X2xl bgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBKb2huIEtlZXBpbmcgPGpvaG5AbWV0YW5hdGUuY29t Pgo+ID4gLS0tCj4gPiB2MzoKPiA+IC0gRml4IGNoZWNrcGF0Y2ggd2FybmluZ3MKPiA+IFVuY2hh bmdlZCBpbiB2Mgo+ID4gCj4gPiAgZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3LW1pcGktZHNp LmMgfCA1NiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrCj4gPiAgMSBmaWxlIGNo YW5nZWQsIDU2IGluc2VydGlvbnMoKykKPiA+IAo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1 L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaS5jIGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3 LW1pcGktZHNpLmMKPiA+IGluZGV4IGNmM2NhNmIwY2JkYi4uY2M1OGFkYTc1NDI1IDEwMDY0NAo+ ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3LW1pcGktZHNpLmMKPiA+ICsrKyBi L2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kdy1taXBpLWRzaS5jCj4gPiBAQCAtNjc4LDYgKzY3 OCw1NiBAQCBzdGF0aWMgaW50IGR3X21pcGlfZHNpX2Rjc19sb25nX3dyaXRlKHN0cnVjdCBkd19t aXBpX2RzaSAqZHNpLAo+ID4gIAlyZXR1cm4gZHdfbWlwaV9kc2lfZ2VuX3BrdF9oZHJfd3JpdGUo ZHNpLCBoZHJfdmFsKTsKPiA+ICB9Cj4gPiAgCj4gPiArc3RhdGljIGludCBkd19taXBpX2RzaV9k Y3NfcmVhZChzdHJ1Y3QgZHdfbWlwaV9kc2kgKmRzaSwKPiA+ICsJCQkJY29uc3Qgc3RydWN0IG1p cGlfZHNpX21zZyAqbXNnKQo+ID4gK3sKPiA+ICsJY29uc3QgdTggKnR4X2J1ZiA9IG1zZy0+dHhf YnVmOwo+ID4gKwl1OCAqcnhfYnVmID0gbXNnLT5yeF9idWY7Cj4gPiArCXNpemVfdCBpOwo+ID4g KwlpbnQgcmV0LCB2YWw7Cj4gPiArCj4gPiArCWRzaV93cml0ZShkc2ksIERTSV9QQ0tIRExfQ0ZH LCBFTl9DUkNfUlggfCBFTl9FQ0NfUlggfCBFTl9CVEEpOwo+ID4gKwlkc2lfd3JpdGUoZHNpLCBE U0lfR0VOX0hEUiwKPiA+ICsJCSAgR0VOX0hEQVRBKHR4X2J1ZlswXSkgfCBHRU5fSFRZUEUobXNn LT50eXBlKSk7Cj4gPiArCj4gPiArCXJldCA9IHJlYWRsX3BvbGxfdGltZW91dChkc2ktPmJhc2Ug KyBEU0lfQ01EX1BLVF9TVEFUVVMsCj4gPiArCQkJCSB2YWwsICEodmFsICYgR0VOX1JEX0NNRF9C VVNZKSwgMTAwMCwKPiA+ICsJCQkJIENNRF9QS1RfU1RBVFVTX1RJTUVPVVRfVVMpOwo+ID4gKwlp ZiAocmV0IDwgMCkgewo+ID4gKwkJZGV2X2Vycihkc2ktPmRldiwgImZhaWxlZCB0byByZWFkIGNv bW1hbmQgcmVzcG9uc2VcbiIpOwo+ID4gKwkJcmV0dXJuIHJldDsKPiA+ICsJfQo+ID4gKwo+ID4g Kwlmb3IgKGkgPSAwOyBpIDwgbXNnLT5yeF9sZW47KSB7Cj4gPiArCQl1MzIgcGxkID0gZHNpX3Jl YWQoZHNpLCBEU0lfR0VOX1BMRF9EQVRBKTsKPiA+ICsKPiA+ICsJCXdoaWxlIChpIDwgbXNnLT5y eF9sZW4pIHsKPiA+ICsJCQlyeF9idWZbaV0gPSBwbGQgJiAweGZmOwo+ID4gKwkJCXBsZCA+Pj0g ODsKPiA+ICsJCQlpKys7Cj4gPiArCQl9Cj4gPiArCX0gIAo+IAo+IEFGQUlDVCwgdGhlIG91dGVy IGZvciBsb29wIGp1c3QgaW5pdGlhbGl6ZXMgaSBhbmQgZW5zdXJlcyBtc2ctPnJ4X2xlbiBpcwo+ IG5vbi16ZXJvPyAKPiAKPiBJIHRoaW5rIHRoZSBmb2xsb3dpbmcgd291bGQgYmUgZWFzaWVyIHRv IHJlYWQgKGFuZCBzYWZlIGFnYWluc3QgdGhlIGNhc2Ugd2hlcmUKPiBtc2ctPnJ4X2xlbiA+IHNp emVvZihwbGQpIChldmVuIHRob3VnaCB0aGlzIHNob3VsZG4ndCBoYXBwZW4gYWNjb3JkaW5nIHRv IERDUwo+IHNwZWMpKS4KPiAKPiBpZiAobXNnLT5yeF9sZW4gPiAwKSB7Cj4gICAgICAgICB1MzIg cGxkID0gZHNpX3JlYWQoZHNpLCBEU0lfR0VOX1BMRF9EQVRBKTsKPiAgICAgICAgIG1lbWNweShy eF9idWYsICZwbGQsIE1JTihtc2ctPnJ4X2xlbiwgc2l6ZW9mKHBsZCkpOwo+IH0KCkkgdGhpbmsg dGhlIGludGVudCB3YXMgdG8gaGFuZGxlIHJ4X2xlbiA+IDQsIGJ1dCB0aGUgcGF0Y2ggaXMgb2J2 b3VzbHkKY29tcGxldGVseSBicm9rZW4gcmVnYXJkaW5nIHRoYXQuICBBcyBmYXIgYXMgSSBjYW4g dGVsbCwgcnhfbGVuIGlzCmxpbWl0ZWQgYnkgdGhlIG1heGltdW0gcmV0dXJuIHBhY2tldCBzaXpl IHdoaWNoIGNhbiBiZSBhbnkgdmFsdWUgdXAgdG8KdGhlIG1heGltdW0gc2l6ZSBvZiBhIGxvbmcg cGFja2V0LCBzbyB3ZSBtYXkgbmVlZCB0byByZWFkIGZyb20gdGhlIEZJRk8KbXVsdGlwbGUgdGlt ZXMuCgpUaGUgbG9vcCBzaG91bGQgYmUgc29tZXRoaW5nIGxpa2UgdGhpczoKCglmb3IgKGkgPSAw OyBpIDwgbXNnLT5yeF9sZW47KSB7CgkJdTMyIHBsZCA9IGRzaV9yZWFkKGRzaSwgRFNJX0dFTl9Q TERfREFUQSk7CgkJaW50IGo7CgoJCWZvciAoaiA9IDA7IGogPCA0ICYmIGkgPCBtc2ctPnJ4X2xl bjsgaSsrLCBqKyspIHsKCQkJcnhfYnVmW2ldID0gcGxkICYgMHhmZjsKCQkJcGxkID4+PSA4OwoJ CX0KCX0KCkkgaGF2ZSBzdWNjZXNzZnVsbHkgcmVhZCA1IGJ5dGVzIGZyb20gYSBEU0kgZGlzcGxh eSB1c2luZyB0aGlzIGNvZGUsIGJ1dApJJ20gdGVtcHRlZCB0byBqdXN0IGRyb3AgdGhpcyBwYXRj aCBzaW5jZSBJIG9ubHkgdXNlZCBpdCBmb3IgZGVidWdnaW5nCndoaWxlIGJyaW5naW5nIHVwIGEg bmV3IHBhbmVsLgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Mon, 30 Jan 2017 18:14:27 +0000 Subject: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands In-Reply-To: <20170130152611.GA20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@art_vandelay> Message-ID: <20170130181427.1940024f.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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; } } I have successfully read 5 bytes from a DSI display using this code, but I'm tempted to just drop this patch since I only used it for debugging while bringing up a new panel. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754041AbdA3Sgx (ORCPT ); Mon, 30 Jan 2017 13:36:53 -0500 Received: from dougal.metanate.com ([90.155.101.14]:24362 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753587AbdA3Sgw (ORCPT ); Mon, 30 Jan 2017 13:36:52 -0500 Date: Mon, 30 Jan 2017 18:14:27 +0000 From: John Keeping To: Sean Paul Cc: Mark Yao , 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: <20170130181427.1940024f.john@metanate.com> In-Reply-To: <20170130152611.GA20076@art_vandelay> References: <20170129132444.25251-1-john@metanate.com> <20170129132444.25251-25-john@metanate.com> <20170130152611.GA20076@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 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; } } I have successfully read 5 bytes from a DSI display using this code, but I'm tempted to just drop this patch since I only used it for debugging while bringing up a new panel.