diff mbox series

[v2,1/4] drm/bridge: ti-sn65dsi86: Combine register accesses in ti_sn_aux_transfer()

Message ID 20201030011738.2028313-2-swboyd@chromium.org
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support EDID reading | expand

Commit Message

Stephen Boyd Oct. 30, 2020, 1:17 a.m. UTC
These register reads and writes are sometimes directly next to each
other in the register address space. Let's use regmap bulk read/write
APIs to get the data with one transfer instead of multiple i2c
transfers. This helps cut down on the number of transfers in the case of
something like reading an EDID where we read in blocks of 16 bytes at a
time and the last for loop here is sending an i2c transfer for each of
those 16 bytes, one at a time. Ouch!

Changes in v2:
 - Combined AUX_CMD register write

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 52 ++++++++++++---------------
 1 file changed, 23 insertions(+), 29 deletions(-)

Comments

Doug Anderson Nov. 2, 2020, 4:18 p.m. UTC | #1
Hi,

On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> These register reads and writes are sometimes directly next to each
> other in the register address space. Let's use regmap bulk read/write
> APIs to get the data with one transfer instead of multiple i2c
> transfers. This helps cut down on the number of transfers in the case of
> something like reading an EDID where we read in blocks of 16 bytes at a
> time and the last for loop here is sending an i2c transfer for each of
> those 16 bytes, one at a time. Ouch!
>
> Changes in v2:
>  - Combined AUX_CMD register write

The change from v1 to v2 makes me slightly nervous, though I guess
it's fine.  Specifically, all the examples in the datasheet show
programming the CMD before the ADDR and LEN.  This change will make it
programmed after.  Since there's a separate START bit I guess it's OK,
though.  Nothing in the datasheet explicitly says that the order in
the examples is the only order that will work...

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Stephen Boyd Nov. 2, 2020, 5:06 p.m. UTC | #2
Quoting Doug Anderson (2020-11-02 08:18:47)
> Hi,
> 
> On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > These register reads and writes are sometimes directly next to each
> > other in the register address space. Let's use regmap bulk read/write
> > APIs to get the data with one transfer instead of multiple i2c
> > transfers. This helps cut down on the number of transfers in the case of
> > something like reading an EDID where we read in blocks of 16 bytes at a
> > time and the last for loop here is sending an i2c transfer for each of
> > those 16 bytes, one at a time. Ouch!
> >
> > Changes in v2:
> >  - Combined AUX_CMD register write
> 
> The change from v1 to v2 makes me slightly nervous, though I guess
> it's fine.  Specifically, all the examples in the datasheet show
> programming the CMD before the ADDR and LEN.  This change will make it
> programmed after.  Since there's a separate START bit I guess it's OK,
> though.  Nothing in the datasheet explicitly says that the order in
> the examples is the only order that will work...

Hmmm now that you mention it the SEND bit is explicitly being cleared in
the programming sequence by being there at the start. If I want to
combine that with the adjacent register writes then I should make sure
that the SEND bit is cleared at the beginning. Otherwise the hardware
may be in the middle of a transaction if the previous transaction is
still running, i.e. a timeout where the SEND bit never cleared.

I think we should go back to the previous patch I had here. Combining
this register write is wrong. If anything, we should clear the SEND bit
on a timeout and make sure during probe that this bit is clear and then
drop the programming of this register from this function entirely. That
would reduce the sequence by one register, but is more complicated vs.
just making sure it has the clear bit here to begin with.

> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks, but I'll send another round picking up acks and such and your
previous review tag on the v1 of this patch.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ecdf9b01340f..a1ebfa95088c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -17,6 +17,8 @@ 
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <asm/unaligned.h>
+
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -72,6 +74,7 @@ 
 #define SN_AUX_ADDR_19_16_REG			0x74
 #define SN_AUX_ADDR_15_8_REG			0x75
 #define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_ADDR_MASK			GENMASK(19, 0)
 #define SN_AUX_LENGTH_REG			0x77
 #define SN_AUX_CMD_REG				0x78
 #define  AUX_CMD_SEND				BIT(0)
@@ -841,11 +844,13 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);
 	u32 request = msg->request & ~DP_AUX_I2C_MOT;
 	u32 request_val = AUX_CMD_REQ(msg->request);
-	u8 *buf = (u8 *)msg->buffer;
+	u8 *buf = msg->buffer;
+	unsigned int len = msg->size;
 	unsigned int val;
-	int ret, i;
+	int ret;
+	u8 reg_buf[SN_AUX_CMD_REG + 1 - SN_AUX_ADDR_19_16_REG];
 
-	if (msg->size > SN_AUX_MAX_PAYLOAD_BYTES)
+	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
 		return -EINVAL;
 
 	switch (request) {
@@ -853,25 +858,20 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	case DP_AUX_I2C_WRITE:
 	case DP_AUX_NATIVE_READ:
 	case DP_AUX_I2C_READ:
-		regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG,
-		     (msg->address >> 16) & 0xF);
-	regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG,
-		     (msg->address >> 8) & 0xFF);
-	regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, msg->address & 0xFF);
-
-	regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, msg->size);
+	BUILD_BUG_ON(sizeof(reg_buf) < sizeof(__be32));
+	put_unaligned_be32((msg->address & SN_AUX_ADDR_MASK) << 8 | len,
+			   reg_buf);
+	reg_buf[SN_AUX_CMD_REG - SN_AUX_ADDR_19_16_REG] = request_val;
+	regmap_bulk_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, reg_buf,
+			  ARRAY_SIZE(reg_buf));
 
-	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
-		for (i = 0; i < msg->size; i++)
-			regmap_write(pdata->regmap, SN_AUX_WDATA_REG(i),
-				     buf[i]);
-	}
+	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
+		regmap_bulk_write(pdata->regmap, SN_AUX_WDATA_REG(0), buf, len);
 
 	/* Clear old status bits before start so we don't get confused */
 	regmap_write(pdata->regmap, SN_AUX_CMD_STATUS_REG,
@@ -895,21 +895,15 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		 || (val & AUX_IRQ_STATUS_AUX_SHORT))
 		return -ENXIO;
 
-	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
-		return msg->size;
+	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
+	    len == 0)
+		return len;
 
-	for (i = 0; i < msg->size; i++) {
-		unsigned int val;
-		ret = regmap_read(pdata->regmap, SN_AUX_RDATA_REG(i),
-				  &val);
-		if (ret)
-			return ret;
-
-		WARN_ON(val & ~0xFF);
-		buf[i] = (u8)(val & 0xFF);
-	}
+	ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
+	if (ret)
+		return ret;
 
-	return msg->size;
+	return len;
 }
 
 static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)