This series supports 10bit addressing for driver-model I2C

RFC PATCH New Masahiro Yamada 3 participants 13 messages Dec 19, 2014 – Jan 6, 2015 I2C

lore

Share & tools

lore

Masahiro Yamada

author

2014-12-19 18:34 UTC #0 lore raw

cover This series supports 10bit addressing for driver-model I2C

10bit addressing can be supported by purely software.

It seems reasonable enough to handle 10bit-addressing within i2c-uclass.c just like we have already supported offset address there.

Masahiro Yamada (2):
cmd_i2c: change variable type for 10bit addressing support
dm: i2c: support 10bit addressing in I2C uclass layer

 common/cmd_i2c.c         | 22 ++++++-------
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 3 files changed, 67 insertions(+), 39 deletions(-)

Signature · 1 line

1.9.1

Masahiro Yamada

author

2014-12-19 18:34 UTC #1 lore raw

RFC PATCH 1/2 cmd_i2c: change variable type for 10bit addressing support

To store 10bit chip address, the variable type should not be uchar, but uint.

Signed-off-by Masahiro Yamada <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Simon Glass <[email protected]>

---

 common/cmd_i2c.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

common/cmd_i2c.c +11 ~~-11~~

index 22db1bb..f65ab61 100644
@@ -83,12 +83,12 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Display values from last command.
  * Memory modify remembered values are different from display memory.
  */
-static uchar	i2c_dp_last_chip;
+static uint	i2c_dp_last_chip;
 static uint	i2c_dp_last_addr;
 static uint	i2c_dp_last_alen;
 static uint	i2c_dp_last_length = 0x10;
 
-static uchar	i2c_mm_last_chip;
+static uint	i2c_mm_last_chip;
 static uint	i2c_mm_last_addr;
 static uint	i2c_mm_last_alen;

@@ -282,7 +282,7 @@ static int i2c_report_err(int ret, enum i2c_err_op op)
  */
 static int do_i2c_read ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	u_char	chip;
+	uint	chip;
 	uint	devaddr, length;
 	int alen;
 	u_char  *memaddr;

@@ -335,7 +335,7 @@ static int do_i2c_read ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv
 
 static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	u_char	chip;
+	uint	chip;
 	uint	devaddr, length;
 	int alen;
 	u_char  *memaddr;

@@ -444,7 +444,7 @@ static int do_i2c_flags(cmd_tbl_t *cmdtp, int flag, int argc,
  */
 static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	u_char	chip;
+	uint	chip;
 	uint	addr, length;
 	int alen;
 	int	j, nbytes, linebytes;

@@ -563,7 +563,7 @@ static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
  */
 static int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	uchar	chip;
+	uint	chip;
 	ulong	addr;
 	int	alen;
 	uchar	byte;

@@ -649,7 +649,7 @@ static int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
  */
 static int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	uchar	chip;
+	uint	chip;
 	ulong	addr;
 	int	alen;
 	int	count;

@@ -734,7 +734,7 @@ static int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
 static int
 mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char * const argv[])
 {
-	uchar	chip;
+	uint	chip;
 	ulong	addr;
 	int	alen;
 	ulong	data;

@@ -957,7 +957,7 @@ static int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv
  */
 static int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	u_char	chip;
+	uint	chip;
 	int alen;
 	uint	addr;
 	uint	length;

@@ -1085,7 +1085,7 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 {
 	enum { unknown, EDO, SDRAM, DDR2 } type;
 
-	u_char	chip;
+	uint	chip;
 	u_char	data[128];
 	u_char	cksum;
 	int	j;

@@ -1563,7 +1563,7 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 #if defined(CONFIG_I2C_EDID)
 int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
 {
-	u_char chip;
+	uint chip;
 	struct edid1_info edid;
 	int ret;
 #ifdef CONFIG_DM_I2C

Signature · 1 line

1.9.1

Simon Glass

2014-12-19 21:23 UTC #2 lore raw

RFC PATCH 1/2 cmd_i2c: change variable type for 10bit addressing support

Quoted 10 lines · Masahiro Yamada

To store 10bit chip address, the variable type should not be uchar, but uint.
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/cmd_i2c.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Acked-by Simon Glass <[email protected]>

Simon Glass

2014-12-29 21:59 UTC #3 lore raw

RFC PATCH 1/2 cmd_i2c: change variable type for 10bit addressing support

Hi,

Quoted 9 lines · Simon Glass

Quoted 8 lines · Masahiro Yamada
To store 10bit chip address, the variable type should not be uchar, but uint.
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---

Quoted 4 lines

Quoted 2 lines
 common/cmd_i2c.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Acked-by Simon Glass <[email protected]>

I feel I should apply this to u-boot-dm as a bug fix, but please let me know if there are any objections.

Regards,
Simon

Heiko Schocher

2015-01-05 05:38 UTC #4 lore raw

RFC PATCH 1/2 cmd_i2c: change variable type for 10bit addressing support

Hello Simon,

Am 29.12.2014 22:59, schrieb Simon Glass:

Quoted 12 lines

Hi,
Quoted 9 lines · Simon Glass
Quoted 8 lines · Masahiro Yamada
To store 10bit chip address, the variable type should not be uchar, but uint.
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
  common/cmd_i2c.c | 22 +++++++++++-----------
  1 file changed, 11 insertions(+), 11 deletions(-)
Acked-by Simon Glass <[email protected]>
I feel I should apply this to u-boot-dm as a bug fix, but please let me know if there are any objections.

I am fine with that, so:

Acked-by Heiko Schocher<[email protected]>

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Masahiro Yamada

author

2014-12-19 18:34 UTC #5 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

In reply to Masahiro Yamada

Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.

Master Send to a 10bit-addressed slave chip is performed like this:

 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...

Master Receive from a 10bit-addressed slave chip is like this:

 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...

Signed-off-by Masahiro Yamada <[email protected]>

Cc Heiko Schocher <[email protected]>

Cc Simon Glass <[email protected]>

---

 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)

drivers/i2c/i2c-uclass.c +52 ~~-28~~

index 005bf86..de9d92a 100644
@@ -31,20 +31,28 @@ DECLARE_GLOBAL_DATA_PTR;
 static int i2c_setup_offset(struct dm_i2c_chip *chip, uint offset,
 			    uint8_t offset_buf[], struct i2c_msg *msg)
 {
-	int offset_len;
+	int offset_len = chip->offset_len;
 
-	msg->addr = chip->chip_addr;
-	msg->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
-	msg->len = chip->offset_len;
+	msg->flags = 0;
 	msg->buf = offset_buf;
-	if (!chip->offset_len)
-		return -EADDRNOTAVAIL;
-	assert(chip->offset_len <= I2C_MAX_OFFSET_LEN);
-	offset_len = chip->offset_len;
-	while (offset_len--)
+
+	if (chip->flags & DM_I2C_CHIP_10BIT) {
+		msg->addr = I2C_ADDR_TEN_HIGH(chip->chip_addr);
+		*offset_buf++ = I2C_ADDR_TEN_LOW(chip->chip_addr);
+		msg->len = 1;
+	} else {
+		msg->addr = chip->chip_addr;
+		msg->len = 0;
+	}
+
+	assert(offset_len <= I2C_MAX_OFFSET_LEN);
+
+	while (offset_len--) {
 		*offset_buf++ = offset >> (8 * offset_len);
+		msg->len++;
+	}
 
-	return 0;
+	return msg->len ? 0 : -EADDRNOTAVAIL;
 }
 
 static int i2c_read_bytewise(struct udevice *dev, uint offset,

@@ -54,7 +62,7 @@ static int i2c_read_bytewise(struct udevice *dev, uint offset,
 	struct udevice *bus = dev_get_parent(dev);
 	struct dm_i2c_ops *ops = i2c_get_ops(bus);
 	struct i2c_msg msg[2], *ptr;
-	uint8_t offset_buf[I2C_MAX_OFFSET_LEN];
+	uint8_t offset_buf[I2C_MAX_OFFSET_LEN + 1];
 	int ret;
 	int i;

@@ -62,7 +70,8 @@ static int i2c_read_bytewise(struct udevice *dev, uint offset,
 		if (i2c_setup_offset(chip, offset + i, offset_buf, msg))
 			return -EINVAL;
 		ptr = msg + 1;
-		ptr->addr = chip->chip_addr;
+		ptr->addr = chip->flags & DM_I2C_CHIP_10BIT ?
+			I2C_ADDR_TEN_HIGH(chip->chip_addr) : chip->chip_addr;
 		ptr->flags = msg->flags | I2C_M_RD;
 		ptr->len = 1;
 		ptr->buf = &buffer[i];

@@ -83,7 +92,7 @@ static int i2c_write_bytewise(struct udevice *dev, uint offset,
 	struct udevice *bus = dev_get_parent(dev);
 	struct dm_i2c_ops *ops = i2c_get_ops(bus);
 	struct i2c_msg msg[1];
-	uint8_t buf[I2C_MAX_OFFSET_LEN + 1];
+	uint8_t buf[I2C_MAX_OFFSET_LEN + 2];
 	int ret;
 	int i;

@@ -106,7 +115,7 @@ int i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
 	struct udevice *bus = dev_get_parent(dev);
 	struct dm_i2c_ops *ops = i2c_get_ops(bus);
 	struct i2c_msg msg[2], *ptr;
-	uint8_t offset_buf[I2C_MAX_OFFSET_LEN];
+	uint8_t offset_buf[I2C_MAX_OFFSET_LEN + 1];
 	int msg_count;
 
 	if (!ops->xfer)

@@ -118,9 +127,9 @@ int i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
 		ptr++;
 
 	if (len) {
-		ptr->addr = chip->chip_addr;
-		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
-		ptr->flags |= I2C_M_RD;
+		ptr->addr = chip->flags & DM_I2C_CHIP_10BIT ?
+			I2C_ADDR_TEN_HIGH(chip->chip_addr) : chip->chip_addr;
+		ptr->flags = I2C_M_RD;
 		ptr->len = len;
 		ptr->buf = buffer;
 		ptr++;

@@ -136,6 +145,7 @@ int i2c_write(struct udevice *dev, uint offset, const uint8_t *buffer, int len)
 	struct udevice *bus = dev_get_parent(dev);
 	struct dm_i2c_ops *ops = i2c_get_ops(bus);
 	struct i2c_msg msg[1];
+	int buf_len;
 
 	if (!ops->xfer)
 		return -ENOSYS;

@@ -157,27 +167,33 @@ int i2c_write(struct udevice *dev, uint offset, const uint8_t *buffer, int len)
 	 * copying the message.
 	 *
 	 * Use the stack for small messages, malloc() for larger ones. We
-	 * need to allow space for the offset (up to 4 bytes) and the message
+	 * need to allow space for the offset (up to 4 bytes), the second
+	 * byte of the slave address (if 10bit addressing) and the message
 	 * itself.
 	 */
-	if (len < 64) {
-		uint8_t buf[I2C_MAX_OFFSET_LEN + len];
+
+	buf_len = I2C_MAX_OFFSET_LEN + len;
+	if (chip->flags & DM_I2C_CHIP_10BIT)
+		buf_len++;
+
+	if (buf_len <= 64) {
+		uint8_t buf[64];
 
 		i2c_setup_offset(chip, offset, buf, msg);
+		memcpy(buf + msg->len, buffer, len);
 		msg->len += len;
-		memcpy(buf + chip->offset_len, buffer, len);
 
 		return ops->xfer(bus, msg, 1);
 	} else {
 		uint8_t *buf;
 		int ret;
 
-		buf = malloc(I2C_MAX_OFFSET_LEN + len);
+		buf = malloc(buf_len);
 		if (!buf)
 			return -ENOMEM;
 		i2c_setup_offset(chip, offset, buf, msg);
+		memcpy(buf + msg->len, buffer, len);
 		msg->len += len;
-		memcpy(buf + chip->offset_len, buffer, len);
 
 		ret = ops->xfer(bus, msg, 1);
 		free(buf);

@@ -199,6 +215,7 @@ static int i2c_probe_chip(struct udevice *bus, uint chip_addr,
 {
 	struct dm_i2c_ops *ops = i2c_get_ops(bus);
 	struct i2c_msg msg[1];
+	u8 low_address;
 	int ret;
 
 	if (ops->probe_chip) {

@@ -210,11 +227,18 @@ static int i2c_probe_chip(struct udevice *bus, uint chip_addr,
 	if (!ops->xfer)
 		return -ENOSYS;
 
-	/* Probe with a zero-length message */
-	msg->addr = chip_addr;
-	msg->flags = chip_flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
-	msg->len = 0;
-	msg->buf = NULL;
+	if (chip_flags & DM_I2C_CHIP_10BIT) {
+		msg->addr = I2C_ADDR_TEN_HIGH(chip_addr);
+		low_address = I2C_ADDR_TEN_LOW(chip_addr);
+		msg->buf = &low_address;
+		msg->len = 1;
+	} else {
+		/* Probe with a zero-length message */
+		msg->addr = chip_addr;
+		msg->buf = NULL;
+		msg->len = 0;
+	}
+	msg->flags = 0;
 
 	return ops->xfer(bus, msg, 1);
 }

include/i2c.h +4 -0

index 9c6a60c..e616909 100644
@@ -185,6 +185,10 @@ int i2c_set_chip_offset_len(struct udevice *dev, uint offset_len);
  */
 int i2c_deblock(struct udevice *bus);
 
+/* return upper, lower byte of 10 bit addressing, respectively */
+#define I2C_ADDR_TEN_HIGH(a)	(0x78 | (((a) >> 8) & 0x3))
+#define I2C_ADDR_TEN_LOW(a)	((a) & 0xff)
+
 /*
  * Not all of these flags are implemented in the U-Boot API
  */

Signature · 1 line

1.9.1

Simon Glass

2014-12-19 21:34 UTC #6 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

Hi Masahiro,

Quoted 24 lines · Masahiro Yamada

Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...

Quoted 8 lines

Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)

Seems like a good idea if we can make it work...

But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.

So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?

Regards,
Simon

Masahiro YAMADA

author

2014-12-20 07:25 UTC #7 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

Hi Simon,

2014-12-20 6:34 GMT+09:00 Simon Glass <[email protected]>:

Quoted 38 lines

Hi Masahiro,
Quoted 24 lines · Masahiro Yamada
Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...
Quoted 8 lines
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)
Seems like a good idea if we can make it work...

Quoted 8 lines

But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.
So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?

I was writing two I2C drivers on DM,
both of which have no dedicated hardware support for 10bit addressing.

Of course, the driver could implement it, but it means I put the completely the same code in each of driver.

For write transaction, for example, we create a new buffer and copy offset-address + data in Uclass layer.

Do I have to create a new buffer again, in my driver, and copy lower-slave-address + offset-address + data ?

Perhaps, is it a good idea to have it optional?

DM_I2C_FLAG_SW_TENBIT - if set, uclass takes care of 10bit addressing by software
if unset, each
driver is responsible to handle I2C_M_TEN correctly

altough I do think 10bit support on U-Boot is urgent necessity...

Signature · 2 lines

Best Regards
Masahiro Yamada

Simon Glass

2014-12-21 18:53 UTC #8 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

Hi Masahiro,

Quoted 55 lines · Masahiro YAMADA

Hi Simon,
2014-12-20 6:34 GMT+09:00 Simon Glass <[email protected]>:
Quoted 38 lines
Hi Masahiro,
Quoted 24 lines · Masahiro Yamada
Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...
Quoted 8 lines
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)
Seems like a good idea if we can make it work...
Quoted 8 lines
But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.
So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?
I was writing two I2C drivers on DM,
both of which have no dedicated hardware support for 10bit addressing.

Quoted 8 lines

Of course, the driver could implement it, but it means I put the completely the same code in each of driver.
For write transaction, for example, we create a new buffer and copy offset-address + data in Uclass layer.
Do I have to create a new buffer again, in my driver, and copy lower-slave-address + offset-address + data ?

I see your point!

Quoted 9 lines

Perhaps, is it a good idea to have it optional?
DM_I2C_FLAG_SW_TENBIT - if set, uclass takes care of 10bit addressing by software
if unset, each
driver is responsible to handle I2C_M_TEN correctly
altough I do think 10bit support on U-Boot is urgent necessity...

I've thought about this quite a bit. We have only just changed the API to be the same as Linux (the list of messages). It seems wrong to now hack it around, such that the address field only stores the first part of the address in 10-bit mode. Did we like the API or not?

I see two options that are somewhat sane and defensible:

- Add a helper function in the uclass that the driver can call to turn the address + message into a single stream of bytes (we can avoid a second malloc() by reserving space for the address bytes before the message or similar similar, so long is it is clearly documented)
- Allow the driver to request that the message bytes should always contain the address, which would remove the address-processing code from the driver.

I think this needs a little more discussion before we decide what to do.

Regards,
Simon

Simon Glass

2014-12-29 22:00 UTC #9 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

Hi Masahiro,

Quoted 94 lines · Simon Glass

Hi Masahiro,
Quoted 55 lines · Masahiro YAMADA
Hi Simon,
2014-12-20 6:34 GMT+09:00 Simon Glass <[email protected]>:
Quoted 38 lines
Hi Masahiro,
Quoted 24 lines · Masahiro Yamada
Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...
Quoted 8 lines
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)
Seems like a good idea if we can make it work...
Quoted 8 lines
But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.
So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?
I was writing two I2C drivers on DM,
both of which have no dedicated hardware support for 10bit addressing.
Quoted 8 lines
Of course, the driver could implement it, but it means I put the completely the same code in each of driver.
For write transaction, for example, we create a new buffer and copy offset-address + data in Uclass layer.
Do I have to create a new buffer again, in my driver, and copy lower-slave-address + offset-address + data ?
I see your point!
Quoted 9 lines
Perhaps, is it a good idea to have it optional?
DM_I2C_FLAG_SW_TENBIT - if set, uclass takes care of 10bit addressing by software
if unset, each
driver is responsible to handle I2C_M_TEN correctly
altough I do think 10bit support on U-Boot is urgent necessity...
I've thought about this quite a bit. We have only just changed the API to be the same as Linux (the list of messages). It seems wrong to now hack it around, such that the address field only stores the first part of the address in 10-bit mode. Did we like the API or not?
I see two options that are somewhat sane and defensible:
- Add a helper function in the uclass that the driver can call to turn the address + message into a single stream of bytes (we can avoid a second malloc() by reserving space for the address bytes before the message or similar similar, so long is it is clearly documented)
- Allow the driver to request that the message bytes should always contain the address, which would remove the address-processing code from the driver.

Quoted 1 line

I think this needs a little more discussion before we decide what to do.

Where do you want to take this one? Please see my suggestions above.

Regards,
Simon

Masahiro YAMADA

author

2015-01-05 14:56 UTC #10 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

In reply to Simon Glass

Hi Simon,

2014-12-22 3:53 GMT+09:00 Simon Glass <[email protected]>:

Quoted 77 lines

Hi Masahiro,
Quoted 55 lines · Masahiro YAMADA
Hi Simon,
2014-12-20 6:34 GMT+09:00 Simon Glass <[email protected]>:
Quoted 38 lines
Hi Masahiro,
Quoted 24 lines · Masahiro Yamada
Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...
Quoted 8 lines
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)
Seems like a good idea if we can make it work...
Quoted 8 lines
But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.
So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?
I was writing two I2C drivers on DM,
both of which have no dedicated hardware support for 10bit addressing.
Quoted 8 lines
Of course, the driver could implement it, but it means I put the completely the same code in each of driver.
For write transaction, for example, we create a new buffer and copy offset-address + data in Uclass layer.
Do I have to create a new buffer again, in my driver, and copy lower-slave-address + offset-address + data ?
I see your point!
Quoted 8 lines
Perhaps, is it a good idea to have it optional?
DM_I2C_FLAG_SW_TENBIT - if set, uclass takes care of 10bit addressing by software
if unset, each
driver is responsible to handle I2C_M_TEN correctly

Quoted 6 lines

Quoted 1 line
altough I do think 10bit support on U-Boot is urgent necessity...
I've thought about this quite a bit. We have only just changed the API to be the same as Linux (the list of messages). It seems wrong to now hack it around, such that the address field only stores the first part of the address in 10-bit mode. Did we like the API or not?

I am not sure...
That is why I sent this series as RFC.

Quoted 1 line

I see two options that are somewhat sane and defensible:

I see another option:
Do not support 10bit address (or leave it to each driver if necessary).

Rationale:
Until now, U-boot has not supported 10bit address and nobody has not complained about that.

Quoted 7 lines

- Add a helper function in the uclass that the driver can call to turn the address + message into a single stream of bytes (we can avoid a second malloc() by reserving space for the address bytes before the message or similar similar, so long is it is clearly documented)
- Allow the driver to request that the message bytes should always contain the address, which would remove the address-processing code from the driver.

How? set a flag??

Quoted 1 line

I think this needs a little more discussion before we decide what to do.

Agreed.
We do not rush to make a decision.

Signature · 2 lines

Best Regards
Masahiro Yamada

Simon Glass

2015-01-06 02:46 UTC #11 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

Hi Masahiro,

Quoted 93 lines · Masahiro YAMADA

Hi Simon,
2014-12-22 3:53 GMT+09:00 Simon Glass <[email protected]>:
Quoted 77 lines
Hi Masahiro,
Quoted 55 lines · Masahiro YAMADA
Hi Simon,
2014-12-20 6:34 GMT+09:00 Simon Glass <[email protected]>:
Quoted 38 lines
Hi Masahiro,
Quoted 24 lines · Masahiro Yamada
Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
 M->S   data0
 M->S   data1
      ...
Master Receive from a 10bit-addressed slave chip is like this:
 DIR    Format
 M->S   11110 + address[9:8] + R/W(0)
 M->S   address[7:0]
        (Restart)
 M->S   111110 + address[9:8] + R/W(1)
 S->M   data0
 S->M   data1
      ...
Quoted 8 lines
Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
 drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
 include/i2c.h            |  4 +++
 2 files changed, 56 insertions(+), 28 deletions(-)
Seems like a good idea if we can make it work...
Quoted 8 lines
But this is driver-specific. Some drivers have hardware to send the address and it isn't part of the message. For example see the tegra driver.
So what you have here feels a bit like a hack to me. Can't the driver implement it? If you are trying to avoid driver work to support 10-bit addresses, maybe it should be an option that we can enable for each driver, so we don't break the other drivers?
I was writing two I2C drivers on DM,
both of which have no dedicated hardware support for 10bit addressing.
Quoted 8 lines
Of course, the driver could implement it, but it means I put the completely the same code in each of driver.
For write transaction, for example, we create a new buffer and copy offset-address + data in Uclass layer.
Do I have to create a new buffer again, in my driver, and copy lower-slave-address + offset-address + data ?
I see your point!
Quoted 8 lines
Perhaps, is it a good idea to have it optional?
DM_I2C_FLAG_SW_TENBIT - if set, uclass takes care of 10bit addressing by software
if unset, each
driver is responsible to handle I2C_M_TEN correctly
Quoted 6 lines
Quoted 1 line
altough I do think 10bit support on U-Boot is urgent necessity...
I've thought about this quite a bit. We have only just changed the API to be the same as Linux (the list of messages). It seems wrong to now hack it around, such that the address field only stores the first part of the address in 10-bit mode. Did we like the API or not?
I am not sure...
That is why I sent this series as RFC.

Quoted 8 lines

Quoted 1 line
I see two options that are somewhat sane and defensible:
I see another option:
Do not support 10bit address (or leave it to each driver if necessary).
Rationale:
Until now, U-boot has not supported 10bit address and nobody has not complained about that.

OK, well it is certainly possible for the driver to support it, and it isn't very hard. As you say, there demand has not been high!

Quoted 11 lines

Quoted 10 lines
- Add a helper function in the uclass that the driver can call to turn the address + message into a single stream of bytes (we can avoid a second malloc() by reserving space for the address bytes before the message or similar similar, so long is it is clearly documented)
- Allow the driver to request that the message bytes should always contain the address, which would remove the address-processing code from the driver.

Quoted 1 line

How? set a flag??

I suppose the driver could set a flag in the uclass to tell the uclass how to behave. I'm not sure if this will make things simpler or more complicated.

Quoted 5 lines

Quoted 2 lines
I think this needs a little more discussion before we decide what to do.
Agreed.
We do not rush to make a decision.

OK, well patch 1 looks OK anyway, so I think we should take that.

Regards,
Simon

Heiko Schocher

2015-01-05 05:39 UTC #12 lore raw

RFC PATCH 2/2 dm: i2c: support 10bit addressing in I2C uclass layer

In reply to Masahiro Yamada

Hello Masahiro,

Am 19.12.2014 19:34, schrieb Masahiro Yamada:

Quoted 24 lines

Master send to / receive from 10-bit addressed slave devices can be supported by software layer without any hardware change because the LSB 8bit of the slave address is treated as data part.
Master Send to a 10bit-addressed slave chip is performed like this:
  DIR    Format
  M->S   11110 + address[9:8] + R/W(0)
  M->S   address[7:0]
  M->S   data0
  M->S   data1
       ...
Master Receive from a 10bit-addressed slave chip is like this:
  DIR    Format
  M->S   11110 + address[9:8] + R/W(0)
  M->S   address[7:0]
         (Restart)
  M->S   111110 + address[9:8] + R/W(1)
  S->M   data0
  S->M   data1
       ...

Quoted 8 lines

Signed-off-by Masahiro Yamada <[email protected]>
Cc Heiko Schocher <[email protected]>
Cc Simon Glass <[email protected]>
---
  drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
  include/i2c.h            |  4 +++
  2 files changed, 56 insertions(+), 28 deletions(-)

Acked-by Heiko Schocher <[email protected]>

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany