Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hi Simon, 2014-12-22 3:53 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 20 December 2014 at 00:25, Masahiro YAMADA yamad...@jp.panasonic.com wrote: Hi Simon, 2014-12-20 6:34 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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? 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 ? I see your point! 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 am not sure... That is why I sent this series as RFC. 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. - 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?? I think this needs a little more discussion before we decide what to do. Agreed. We do not rush to make a decision. -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hi Masahiro, On 5 January 2015 at 07:56, Masahiro YAMADA yamad...@jp.panasonic.com wrote: Hi Simon, 2014-12-22 3:53 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 20 December 2014 at 00:25, Masahiro YAMADA yamad...@jp.panasonic.com wrote: Hi Simon, 2014-12-20 6:34 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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? 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 ? I see your point! 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 am not sure... That is why I sent this series as RFC. 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! - 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?? 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. 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hello Masahiro, Am 19.12.2014 19:34, schrieb 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- drivers/i2c/i2c-uclass.c | 80 +++- include/i2c.h| 4 +++ 2 files changed, 56 insertions(+), 28 deletions(-) Acked-by: Heiko Schocher h...@denx.de bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hi Masahiro, On 21 December 2014 at 11:53, Simon Glass s...@chromium.org wrote: Hi Masahiro, On 20 December 2014 at 00:25, Masahiro YAMADA yamad...@jp.panasonic.com wrote: Hi Simon, 2014-12-20 6:34 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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? 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 ? I see your point! 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. Where do you want to take this one? Please see my suggestions above. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hi Masahiro, On 20 December 2014 at 00:25, Masahiro YAMADA yamad...@jp.panasonic.com wrote: Hi Simon, 2014-12-20 6:34 GMT+09:00 Simon Glass s...@chromium.org: Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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? 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 ? I see your point! 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
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:
DIRFormat
M-S 0 + 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:
DIRFormat
M-S 0 + address[9:8] + R/W(0)
M-S address[7:0]
(Restart)
M-S 10 + address[9:8] + R/W(1)
S-M data0
S-M data1
...
Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com
Cc: Heiko Schocher h...@denx.de
Cc: Simon Glass s...@chromium.org
---
drivers/i2c/i2c-uclass.c | 80 +++-
include/i2c.h| 4 +++
2 files changed, 56 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index 005bf86..de9d92a 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -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
-*
Re: [U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer
Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [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 s...@chromium.org: Hi Masahiro, On 19 December 2014 at 11:34, Masahiro Yamada yamad...@jp.panasonic.com wrote: 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: DIRFormat M-S 0 + 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: DIRFormat M-S 0 + address[9:8] + R/W(0) M-S address[7:0] (Restart) M-S 10 + address[9:8] + R/W(1) S-M data0 S-M data1 ... Signed-off-by: Masahiro Yamada yamad...@jp.panasonic.com Cc: Heiko Schocher h...@denx.de Cc: Simon Glass s...@chromium.org --- 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? 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... -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
