* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
@ 2015-01-30 17:56 Lubomir Popov
2015-02-03 0:59 ` Simon Glass
0 siblings, 1 reply; 8+ messages in thread
From: Lubomir Popov @ 2015-01-30 17:56 UTC (permalink / raw)
To: u-boot
I2C chips do exist that require a write of some multi-byte data to occur in
a single bus transaction (aka atomic transfer), otherwise either the write
does not come into effect at all, or normal operation of internal circuitry
cannot be guaranteed. The current implementation of the 'i2c write' command
(transfer of multiple bytes from a memory buffer) in fact performs a separate
transaction for each byte to be written and thus cannot support such types of
I2C slave devices.
This patch provides an alternative by allowing 'i2c write' to execute the
write transfer of the given number of bytes in a single bus transaction if
the '-s' option is specified as a final command argument. Else the current
re-addressing method is used.
Signed-off-by: Lubomir Popov <l-popov@ti.com>
---
Changes in V3:
Rebased on current master.
Changes in V2:
The option to use bulk transfer vs re-addressing is implemented as a run-time
command argument. V1 used conditional compilation through a board header
definition.
common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
index 22db1bb..8d4f5f6 100644
--- a/common/cmd_i2c.c
+++ b/common/cmd_i2c.c
@@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
struct udevice *dev;
#endif
- if (argc != 5)
+ if ((argc < 5) || (argc > 6))
return cmd_usage(cmdtp);
/*
@@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
return cmd_usage(cmdtp);
/*
- * Length is the number of objects, not number of bytes.
+ * Length is the number of bytes.
*/
length = simple_strtoul(argv[4], NULL, 16);
@@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
return i2c_report_err(ret, I2C_ERR_WRITE);
#endif
- while (length-- > 0) {
+ if (argc == 6 && !strcmp(argv[5], "-s")) {
+ /*
+ * Write all bytes in a single I2C transaction. If the target
+ * device is an EEPROM, it is your responsibility to not cross
+ * a page boundary. No write delay upon completion, take this
+ * into account if linking commands.
+ */
#ifdef CONFIG_DM_I2C
- ret = i2c_write(dev, devaddr++, memaddr++, 1);
+ ret = i2c_write(dev, devaddr, memaddr, length);
#else
- ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
+ ret = i2c_write(chip, devaddr, alen, memaddr, length);
#endif
if (ret)
return i2c_report_err(ret, I2C_ERR_WRITE);
+ } else {
+ /*
+ * Repeated addressing - perform <length> separate
+ * write transactions of one byte each
+ */
+ while (length-- > 0) {
+#ifdef CONFIG_DM_I2C
+ ret = i2c_write(dev, devaddr++, memaddr++, 1);
+#else
+ ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
+#endif
+ if (ret)
+ return i2c_report_err(ret, I2C_ERR_WRITE);
/*
* No write delay with FRAM devices.
*/
#if !defined(CONFIG_SYS_I2C_FRAM)
- udelay(11000);
+ udelay(11000);
#endif
+ }
}
return 0;
}
@@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = {
U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""),
U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""),
U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""),
- U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""),
+ U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""),
#ifdef CONFIG_DM_I2C
U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""),
#endif
@@ -1890,7 +1910,8 @@ static char i2c_help_text[] =
"i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n"
"i2c probe [address] - test for and show device(s) on the I2C bus\n"
"i2c read chip address[.0, .1, .2] length memaddress - read to memory\n"
- "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n"
+ "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n"
+ " to I2C; the -s option selects bulk write in a single transaction\n"
#ifdef CONFIG_DM_I2C
"i2c flags chip [flags] - set or get chip flags\n"
#endif
@@ -1902,7 +1923,7 @@ static char i2c_help_text[] =
#endif
U_BOOT_CMD(
- i2c, 6, 1, do_i2c,
+ i2c, 7, 1, do_i2c,
"I2C sub-system",
i2c_help_text
);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-01-30 17:56 [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction Lubomir Popov
@ 2015-02-03 0:59 ` Simon Glass
2015-02-03 8:10 ` Lubomir Popov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Simon Glass @ 2015-02-03 0:59 UTC (permalink / raw)
To: u-boot
Hi,
On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
> I2C chips do exist that require a write of some multi-byte data to occur in
> a single bus transaction (aka atomic transfer), otherwise either the write
> does not come into effect at all, or normal operation of internal circuitry
> cannot be guaranteed. The current implementation of the 'i2c write' command
> (transfer of multiple bytes from a memory buffer) in fact performs a separate
> transaction for each byte to be written and thus cannot support such types of
> I2C slave devices.
>
> This patch provides an alternative by allowing 'i2c write' to execute the
> write transfer of the given number of bytes in a single bus transaction if
> the '-s' option is specified as a final command argument. Else the current
> re-addressing method is used.
>
> Signed-off-by: Lubomir Popov <l-popov@ti.com>
> ---
> Changes in V3:
> Rebased on current master.
> Changes in V2:
> The option to use bulk transfer vs re-addressing is implemented as a run-time
> command argument. V1 used conditional compilation through a board header
> definition.
>
> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
What platform are you testing on?
It seems like you could implement this using driver model - just set
or clear the DM_I2C_CHIP_WR_ADDRESS flag.
That would solve the problem of existing platforms, since they could
be tested when converted to driver model.
So what do you think about adjusting this patch to move the '#ifdef
CONFIG_DM_I2C' outside the while loop, and set the flag instead?
Although then your feature would only be available for driver model.
>
> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> index 22db1bb..8d4f5f6 100644
> --- a/common/cmd_i2c.c
> +++ b/common/cmd_i2c.c
> @@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> struct udevice *dev;
> #endif
>
> - if (argc != 5)
> + if ((argc < 5) || (argc > 6))
> return cmd_usage(cmdtp);
>
> /*
> @@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> return cmd_usage(cmdtp);
>
> /*
> - * Length is the number of objects, not number of bytes.
> + * Length is the number of bytes.
> */
> length = simple_strtoul(argv[4], NULL, 16);
>
> @@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> return i2c_report_err(ret, I2C_ERR_WRITE);
> #endif
>
> - while (length-- > 0) {
> + if (argc == 6 && !strcmp(argv[5], "-s")) {
> + /*
> + * Write all bytes in a single I2C transaction. If the target
> + * device is an EEPROM, it is your responsibility to not cross
> + * a page boundary. No write delay upon completion, take this
> + * into account if linking commands.
> + */
> #ifdef CONFIG_DM_I2C
> - ret = i2c_write(dev, devaddr++, memaddr++, 1);
> + ret = i2c_write(dev, devaddr, memaddr, length);
> #else
> - ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
> + ret = i2c_write(chip, devaddr, alen, memaddr, length);
> #endif
> if (ret)
> return i2c_report_err(ret, I2C_ERR_WRITE);
> + } else {
> + /*
> + * Repeated addressing - perform <length> separate
> + * write transactions of one byte each
> + */
> + while (length-- > 0) {
> +#ifdef CONFIG_DM_I2C
> + ret = i2c_write(dev, devaddr++, memaddr++, 1);
> +#else
> + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
> +#endif
> + if (ret)
> + return i2c_report_err(ret, I2C_ERR_WRITE);
> /*
> * No write delay with FRAM devices.
> */
> #if !defined(CONFIG_SYS_I2C_FRAM)
> - udelay(11000);
> + udelay(11000);
> #endif
> + }
> }
> return 0;
> }
> @@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = {
> U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""),
> U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""),
> U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""),
> - U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""),
> + U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""),
> #ifdef CONFIG_DM_I2C
> U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""),
> #endif
> @@ -1890,7 +1910,8 @@ static char i2c_help_text[] =
> "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n"
> "i2c probe [address] - test for and show device(s) on the I2C bus\n"
> "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n"
> - "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n"
> + "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n"
> + " to I2C; the -s option selects bulk write in a single transaction\n"
> #ifdef CONFIG_DM_I2C
> "i2c flags chip [flags] - set or get chip flags\n"
> #endif
> @@ -1902,7 +1923,7 @@ static char i2c_help_text[] =
> #endif
>
> U_BOOT_CMD(
> - i2c, 6, 1, do_i2c,
> + i2c, 7, 1, do_i2c,
> "I2C sub-system",
> i2c_help_text
> );
> --
> 1.7.9.5
>
Regards,
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-03 0:59 ` Simon Glass
@ 2015-02-03 8:10 ` Lubomir Popov
2015-02-03 8:35 ` Masahiro Yamada
2015-02-06 6:20 ` Heiko Schocher
2015-02-06 6:54 ` Heiko Schocher
2 siblings, 1 reply; 8+ messages in thread
From: Lubomir Popov @ 2015-02-03 8:10 UTC (permalink / raw)
To: u-boot
Hi Simon,
> Hi,
>
> On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
>> I2C chips do exist that require a write of some multi-byte data to occur in
>> a single bus transaction (aka atomic transfer), otherwise either the write
>> does not come into effect at all, or normal operation of internal circuitry
>> cannot be guaranteed. The current implementation of the 'i2c write' command
>> (transfer of multiple bytes from a memory buffer) in fact performs a separate
>> transaction for each byte to be written and thus cannot support such types of
>> I2C slave devices.
>>
>> This patch provides an alternative by allowing 'i2c write' to execute the
>> write transfer of the given number of bytes in a single bus transaction if
>> the '-s' option is specified as a final command argument. Else the current
>> re-addressing method is used.
>>
>> Signed-off-by: Lubomir Popov <l-popov@ti.com>
>> ---
>> Changes in V3:
>> Rebased on current master.
>> Changes in V2:
>> The option to use bulk transfer vs re-addressing is implemented as a run-time
>> command argument. V1 used conditional compilation through a board header
>> definition.
>>
>> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> What platform are you testing on?
TI OMAP/ARM, in particular the J6Eco EVM (DRA726) where we have a
I2C chip that requires this patch for proper operation of some of
its functions.
>
> It seems like you could implement this using driver model - just set
> or clear the DM_I2C_CHIP_WR_ADDRESS flag.
You are right, I could if I were sure that the DM is/shall be supported
by all TI platforms and that I would not break anything now, about which
I'm not... :( I'm looping in Tom here.
Heiko had asked me just to rebase on current mainline, so that the patch
applies cleanly, and that was what I did :)
>
> That would solve the problem of existing platforms, since they could
> be tested when converted to driver model.
>
> So what do you think about adjusting this patch to move the '#ifdef
> CONFIG_DM_I2C' outside the while loop, and set the flag instead?
> Although then your feature would only be available for driver model.
When/if the DM shall be supported by TI platforms.
Tom, do we have the DM enabled for any board, so that this could be
tested?
>
> Regards,
> Simon
>
Best regards,
Lubo
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-03 8:10 ` Lubomir Popov
@ 2015-02-03 8:35 ` Masahiro Yamada
2015-02-04 0:41 ` Simon Glass
0 siblings, 1 reply; 8+ messages in thread
From: Masahiro Yamada @ 2015-02-03 8:35 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, 3 Feb 2015 10:10:02 +0200
"Lubomir Popov" <lpopov@mm-sol.com> wrote:
> Hi Simon,
>
> > Hi,
> >
> > On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
> >> I2C chips do exist that require a write of some multi-byte data to occur in
> >> a single bus transaction (aka atomic transfer), otherwise either the write
> >> does not come into effect at all, or normal operation of internal circuitry
> >> cannot be guaranteed. The current implementation of the 'i2c write' command
> >> (transfer of multiple bytes from a memory buffer) in fact performs a separate
> >> transaction for each byte to be written and thus cannot support such types of
> >> I2C slave devices.
> >>
> >> This patch provides an alternative by allowing 'i2c write' to execute the
> >> write transfer of the given number of bytes in a single bus transaction if
> >> the '-s' option is specified as a final command argument. Else the current
> >> re-addressing method is used.
> >>
> >> Signed-off-by: Lubomir Popov <l-popov@ti.com>
Can we support "pagesize" property in Driver Model I2C?
The binding information is written in
Documentation/devicetree/bindings/eeprom.txt of Linux Kernel.
If it is possible, it would help make the write access faster.
(Sorry, I have not taken a close look.)
Perhaps, it may also satisfy Lubomir's demand ??
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-03 8:35 ` Masahiro Yamada
@ 2015-02-04 0:41 ` Simon Glass
0 siblings, 0 replies; 8+ messages in thread
From: Simon Glass @ 2015-02-04 0:41 UTC (permalink / raw)
To: u-boot
Hi Masahiro,
On 3 February 2015 at 01:35, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
> On Tue, 3 Feb 2015 10:10:02 +0200
> "Lubomir Popov" <lpopov@mm-sol.com> wrote:
>
>> Hi Simon,
>>
>> > Hi,
>> >
>> > On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
>> >> I2C chips do exist that require a write of some multi-byte data to occur in
>> >> a single bus transaction (aka atomic transfer), otherwise either the write
>> >> does not come into effect at all, or normal operation of internal circuitry
>> >> cannot be guaranteed. The current implementation of the 'i2c write' command
>> >> (transfer of multiple bytes from a memory buffer) in fact performs a separate
>> >> transaction for each byte to be written and thus cannot support such types of
>> >> I2C slave devices.
>> >>
>> >> This patch provides an alternative by allowing 'i2c write' to execute the
>> >> write transfer of the given number of bytes in a single bus transaction if
>> >> the '-s' option is specified as a final command argument. Else the current
>> >> re-addressing method is used.
>> >>
>> >> Signed-off-by: Lubomir Popov <l-popov@ti.com>
>
>
>
>
> Can we support "pagesize" property in Driver Model I2C?
>
> The binding information is written in
> Documentation/devicetree/bindings/eeprom.txt of Linux Kernel.
>
> If it is possible, it would help make the write access faster.
> (Sorry, I have not taken a close look.)
>
> Perhaps, it may also satisfy Lubomir's demand ??
>
I think this is slightly different - some devices cannot support
writes with more than one data value at a time - they require a
separate address cycle before each data value.
Regards,
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-03 0:59 ` Simon Glass
2015-02-03 8:10 ` Lubomir Popov
@ 2015-02-06 6:20 ` Heiko Schocher
2015-02-06 6:54 ` Heiko Schocher
2 siblings, 0 replies; 8+ messages in thread
From: Heiko Schocher @ 2015-02-06 6:20 UTC (permalink / raw)
To: u-boot
Hello Simon, Lubomir,
Am 03.02.2015 01:59, schrieb Simon Glass:
> Hi,
>
> On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
>> I2C chips do exist that require a write of some multi-byte data to occur in
>> a single bus transaction (aka atomic transfer), otherwise either the write
>> does not come into effect at all, or normal operation of internal circuitry
>> cannot be guaranteed. The current implementation of the 'i2c write' command
>> (transfer of multiple bytes from a memory buffer) in fact performs a separate
>> transaction for each byte to be written and thus cannot support such types of
>> I2C slave devices.
>>
>> This patch provides an alternative by allowing 'i2c write' to execute the
>> write transfer of the given number of bytes in a single bus transaction if
>> the '-s' option is specified as a final command argument. Else the current
>> re-addressing method is used.
>>
>> Signed-off-by: Lubomir Popov <l-popov@ti.com>
>> ---
>> Changes in V3:
>> Rebased on current master.
>> Changes in V2:
>> The option to use bulk transfer vs re-addressing is implemented as a run-time
>> command argument. V1 used conditional compilation through a board header
>> definition.
>>
>> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> What platform are you testing on?
>
> It seems like you could implement this using driver model - just set
> or clear the DM_I2C_CHIP_WR_ADDRESS flag.
>
> That would solve the problem of existing platforms, since they could
> be tested when converted to driver model.
>
> So what do you think about adjusting this patch to move the '#ifdef
> CONFIG_DM_I2C' outside the while loop, and set the flag instead?
> Although then your feature would only be available for driver model.
I tend to accept this patch at it is ... because old boards without
DM benefit also from it ... any objections?
bye,
Heiko
>
>>
>> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
>> index 22db1bb..8d4f5f6 100644
>> --- a/common/cmd_i2c.c
>> +++ b/common/cmd_i2c.c
>> @@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> struct udevice *dev;
>> #endif
>>
>> - if (argc != 5)
>> + if ((argc < 5) || (argc > 6))
>> return cmd_usage(cmdtp);
>>
>> /*
>> @@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> return cmd_usage(cmdtp);
>>
>> /*
>> - * Length is the number of objects, not number of bytes.
>> + * Length is the number of bytes.
>> */
>> length = simple_strtoul(argv[4], NULL, 16);
>>
>> @@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> return i2c_report_err(ret, I2C_ERR_WRITE);
>> #endif
>>
>> - while (length-- > 0) {
>> + if (argc == 6 && !strcmp(argv[5], "-s")) {
>> + /*
>> + * Write all bytes in a single I2C transaction. If the target
>> + * device is an EEPROM, it is your responsibility to not cross
>> + * a page boundary. No write delay upon completion, take this
>> + * into account if linking commands.
>> + */
>> #ifdef CONFIG_DM_I2C
>> - ret = i2c_write(dev, devaddr++, memaddr++, 1);
>> + ret = i2c_write(dev, devaddr, memaddr, length);
>> #else
>> - ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>> + ret = i2c_write(chip, devaddr, alen, memaddr, length);
>> #endif
>> if (ret)
>> return i2c_report_err(ret, I2C_ERR_WRITE);
>> + } else {
>> + /*
>> + * Repeated addressing - perform <length> separate
>> + * write transactions of one byte each
>> + */
>> + while (length-- > 0) {
>> +#ifdef CONFIG_DM_I2C
>> + ret = i2c_write(dev, devaddr++, memaddr++, 1);
>> +#else
>> + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>> +#endif
>> + if (ret)
>> + return i2c_report_err(ret, I2C_ERR_WRITE);
>> /*
>> * No write delay with FRAM devices.
>> */
>> #if !defined(CONFIG_SYS_I2C_FRAM)
>> - udelay(11000);
>> + udelay(11000);
>> #endif
>> + }
>> }
>> return 0;
>> }
>> @@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = {
>> U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""),
>> U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""),
>> U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""),
>> - U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""),
>> + U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""),
>> #ifdef CONFIG_DM_I2C
>> U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""),
>> #endif
>> @@ -1890,7 +1910,8 @@ static char i2c_help_text[] =
>> "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n"
>> "i2c probe [address] - test for and show device(s) on the I2C bus\n"
>> "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n"
>> - "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n"
>> + "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n"
>> + " to I2C; the -s option selects bulk write in a single transaction\n"
>> #ifdef CONFIG_DM_I2C
>> "i2c flags chip [flags] - set or get chip flags\n"
>> #endif
>> @@ -1902,7 +1923,7 @@ static char i2c_help_text[] =
>> #endif
>>
>> U_BOOT_CMD(
>> - i2c, 6, 1, do_i2c,
>> + i2c, 7, 1, do_i2c,
>> "I2C sub-system",
>> i2c_help_text
>> );
>> --
>> 1.7.9.5
>>
>
> Regards,
> Simon
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-03 0:59 ` Simon Glass
2015-02-03 8:10 ` Lubomir Popov
2015-02-06 6:20 ` Heiko Schocher
@ 2015-02-06 6:54 ` Heiko Schocher
2015-02-06 10:54 ` Lubomir Popov
2 siblings, 1 reply; 8+ messages in thread
From: Heiko Schocher @ 2015-02-06 6:54 UTC (permalink / raw)
To: u-boot
Hello Simon, Lubomir,
Am 03.02.2015 01:59, schrieb Simon Glass:
> Hi,
>
> On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
>> I2C chips do exist that require a write of some multi-byte data to occur in
>> a single bus transaction (aka atomic transfer), otherwise either the write
>> does not come into effect at all, or normal operation of internal circuitry
>> cannot be guaranteed. The current implementation of the 'i2c write' command
>> (transfer of multiple bytes from a memory buffer) in fact performs a separate
>> transaction for each byte to be written and thus cannot support such types of
>> I2C slave devices.
>>
>> This patch provides an alternative by allowing 'i2c write' to execute the
>> write transfer of the given number of bytes in a single bus transaction if
>> the '-s' option is specified as a final command argument. Else the current
>> re-addressing method is used.
>>
>> Signed-off-by: Lubomir Popov <l-popov@ti.com>
>> ---
>> Changes in V3:
>> Rebased on current master.
>> Changes in V2:
>> The option to use bulk transfer vs re-addressing is implemented as a run-time
>> command argument. V1 used conditional compilation through a board header
>> definition.
>>
>> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 30 insertions(+), 9 deletions(-)
I should try to apply a patch before saying I tend to accept a patch ;-)
This patch fails again (Sorry Lubomir) ... because in the meantime
this patch from Simon is in mainline:
commit f9a4c2da72d04e13b05deecb800f232d2948eb85
Author: Simon Glass <sjg@chromium.org>
Date: Mon Jan 12 18:02:07 2015 -0700
dm: i2c: Rename driver model I2C functions to permit compatibility
Which introduced dm_i2c_write() ...
> What platform are you testing on?
>
> It seems like you could implement this using driver model - just set
> or clear the DM_I2C_CHIP_WR_ADDRESS flag.
>
> That would solve the problem of existing platforms, since they could
> be tested when converted to driver model.
>
> So what do you think about adjusting this patch to move the '#ifdef
> CONFIG_DM_I2C' outside the while loop, and set the flag instead?
> Although then your feature would only be available for driver model.
Thinking about this, wouldn;t it be better to add this patch to
this patch?
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
index a1a269f..df18b3f 100644
--- a/common/cmd_i2c.c
+++ b/common/cmd_i2c.c
@@ -342,6 +342,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
int ret;
#ifdef CONFIG_DM_I2C
struct udevice *dev;
+ struct dm_i2c_chip *i2c_chip;
#endif
if ((argc < 5) || (argc > 6))
@@ -377,6 +378,9 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
ret = i2c_set_chip_offset_len(dev, alen);
if (ret)
return i2c_report_err(ret, I2C_ERR_WRITE);
+ i2c_chip = dev_get_parent_platdata(dev);
+ if (!i2c_chip)
+ return i2c_report_err(ret, I2C_ERR_WRITE);
#endif
if (argc == 6 && !strcmp(argv[5], "-s")) {
@@ -387,7 +391,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
* into account if linking commands.
*/
#ifdef CONFIG_DM_I2C
- ret = i2c_write(dev, devaddr, memaddr, length);
+ i2c_chip &= ~DM_I2C_CHIP_WR_ADDRESS;
+ ret = dm_i2c_write(dev, devaddr, memaddr, length);
#else
ret = i2c_write(chip, devaddr, alen, memaddr, length);
#endif
@@ -400,7 +405,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
*/
while (length-- > 0) {
#ifdef CONFIG_DM_I2C
- ret = i2c_write(dev, devaddr++, memaddr++, 1);
+ i2c_chip |= DM_I2C_CHIP_WR_ADDRESS;
+ ret = dm_i2c_write(dev, devaddr++, memaddr++, 1);
#else
ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
#endif
@Simon: Do I have to check if dev_get_parent_platdata() returns
a pointer?
bye,
Heiko
>> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
>> index 22db1bb..8d4f5f6 100644
>> --- a/common/cmd_i2c.c
>> +++ b/common/cmd_i2c.c
>> @@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> struct udevice *dev;
>> #endif
>>
>> - if (argc != 5)
>> + if ((argc < 5) || (argc > 6))
>> return cmd_usage(cmdtp);
>>
>> /*
>> @@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> return cmd_usage(cmdtp);
>>
>> /*
>> - * Length is the number of objects, not number of bytes.
>> + * Length is the number of bytes.
>> */
>> length = simple_strtoul(argv[4], NULL, 16);
>>
>> @@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>> return i2c_report_err(ret, I2C_ERR_WRITE);
>> #endif
>>
>> - while (length-- > 0) {
>> + if (argc == 6 && !strcmp(argv[5], "-s")) {
>> + /*
>> + * Write all bytes in a single I2C transaction. If the target
>> + * device is an EEPROM, it is your responsibility to not cross
>> + * a page boundary. No write delay upon completion, take this
>> + * into account if linking commands.
>> + */
>> #ifdef CONFIG_DM_I2C
>> - ret = i2c_write(dev, devaddr++, memaddr++, 1);
>> + ret = i2c_write(dev, devaddr, memaddr, length);
>> #else
>> - ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>> + ret = i2c_write(chip, devaddr, alen, memaddr, length);
>> #endif
>> if (ret)
>> return i2c_report_err(ret, I2C_ERR_WRITE);
>> + } else {
>> + /*
>> + * Repeated addressing - perform <length> separate
>> + * write transactions of one byte each
>> + */
>> + while (length-- > 0) {
>> +#ifdef CONFIG_DM_I2C
>> + ret = i2c_write(dev, devaddr++, memaddr++, 1);
>> +#else
>> + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>> +#endif
>> + if (ret)
>> + return i2c_report_err(ret, I2C_ERR_WRITE);
>> /*
>> * No write delay with FRAM devices.
>> */
>> #if !defined(CONFIG_SYS_I2C_FRAM)
>> - udelay(11000);
>> + udelay(11000);
>> #endif
>> + }
>> }
>> return 0;
>> }
>> @@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = {
>> U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""),
>> U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""),
>> U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""),
>> - U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""),
>> + U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""),
>> #ifdef CONFIG_DM_I2C
>> U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""),
>> #endif
>> @@ -1890,7 +1910,8 @@ static char i2c_help_text[] =
>> "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n"
>> "i2c probe [address] - test for and show device(s) on the I2C bus\n"
>> "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n"
>> - "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n"
>> + "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n"
>> + " to I2C; the -s option selects bulk write in a single transaction\n"
>> #ifdef CONFIG_DM_I2C
>> "i2c flags chip [flags] - set or get chip flags\n"
>> #endif
>> @@ -1902,7 +1923,7 @@ static char i2c_help_text[] =
>> #endif
>>
>> U_BOOT_CMD(
>> - i2c, 6, 1, do_i2c,
>> + i2c, 7, 1, do_i2c,
>> "I2C sub-system",
>> i2c_help_text
>> );
>> --
>> 1.7.9.5
>>
>
> Regards,
> Simon
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
2015-02-06 6:54 ` Heiko Schocher
@ 2015-02-06 10:54 ` Lubomir Popov
0 siblings, 0 replies; 8+ messages in thread
From: Lubomir Popov @ 2015-02-06 10:54 UTC (permalink / raw)
To: u-boot
Hi Heiko,
> Hello Simon, Lubomir,
>
> Am 03.02.2015 01:59, schrieb Simon Glass:
>> Hi,
>>
>> On 30 January 2015 at 10:56, Lubomir Popov <lpopov@mm-sol.com> wrote:
>>> I2C chips do exist that require a write of some multi-byte data to occur in
>>> a single bus transaction (aka atomic transfer), otherwise either the write
>>> does not come into effect at all, or normal operation of internal circuitry
>>> cannot be guaranteed. The current implementation of the 'i2c write' command
>>> (transfer of multiple bytes from a memory buffer) in fact performs a separate
>>> transaction for each byte to be written and thus cannot support such types of
>>> I2C slave devices.
>>>
>>> This patch provides an alternative by allowing 'i2c write' to execute the
>>> write transfer of the given number of bytes in a single bus transaction if
>>> the '-s' option is specified as a final command argument. Else the current
>>> re-addressing method is used.
>>>
>>> Signed-off-by: Lubomir Popov <l-popov@ti.com>
>>> ---
>>> Changes in V3:
>>> Rebased on current master.
>>> Changes in V2:
>>> The option to use bulk transfer vs re-addressing is implemented as a run-time
>>> command argument. V1 used conditional compilation through a board header
>>> definition.
>>>
>>> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++---------
>>> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> I should try to apply a patch before saying I tend to accept a patch ;-)
>
> This patch fails again (Sorry Lubomir) ... because in the meantime
> this patch from Simon is in mainline:
>
> commit f9a4c2da72d04e13b05deecb800f232d2948eb85
> Author: Simon Glass <sjg@chromium.org>
> Date: Mon Jan 12 18:02:07 2015 -0700
>
> dm: i2c: Rename driver model I2C functions to permit compatibility
>
> Which introduced dm_i2c_write() ...
>
>> What platform are you testing on?
>>
>> It seems like you could implement this using driver model - just set
>> or clear the DM_I2C_CHIP_WR_ADDRESS flag.
>>
>> That would solve the problem of existing platforms, since they could
>> be tested when converted to driver model.
>>
>> So what do you think about adjusting this patch to move the '#ifdef
>> CONFIG_DM_I2C' outside the while loop, and set the flag instead?
>> Although then your feature would only be available for driver model.
>
> Thinking about this, wouldn;t it be better to add this patch to
> this patch?
>
> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> index a1a269f..df18b3f 100644
> --- a/common/cmd_i2c.c
> +++ b/common/cmd_i2c.c
> @@ -342,6 +342,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> int ret;
> #ifdef CONFIG_DM_I2C
> struct udevice *dev;
> + struct dm_i2c_chip *i2c_chip;
> #endif
>
> if ((argc < 5) || (argc > 6))
> @@ -377,6 +378,9 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> ret = i2c_set_chip_offset_len(dev, alen);
> if (ret)
> return i2c_report_err(ret, I2C_ERR_WRITE);
> + i2c_chip = dev_get_parent_platdata(dev);
> + if (!i2c_chip)
> + return i2c_report_err(ret, I2C_ERR_WRITE);
> #endif
>
> if (argc == 6 && !strcmp(argv[5], "-s")) {
> @@ -387,7 +391,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> * into account if linking commands.
> */
> #ifdef CONFIG_DM_I2C
> - ret = i2c_write(dev, devaddr, memaddr, length);
> + i2c_chip &= ~DM_I2C_CHIP_WR_ADDRESS;
> + ret = dm_i2c_write(dev, devaddr, memaddr, length);
> #else
> ret = i2c_write(chip, devaddr, alen, memaddr, length);
> #endif
> @@ -400,7 +405,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
> */
> while (length-- > 0) {
> #ifdef CONFIG_DM_I2C
> - ret = i2c_write(dev, devaddr++, memaddr++, 1);
> + i2c_chip |= DM_I2C_CHIP_WR_ADDRESS;
> + ret = dm_i2c_write(dev, devaddr++, memaddr++, 1);
> #else
> ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
> #endif
>
This looks OK to me; however, since I don't have any DM-enabled
board to test upon, nor currently have any time to test whatever
in general (really sorry), I'm leaving this in your hands, guys.
BR,
Lubo
> @Simon: Do I have to check if dev_get_parent_platdata() returns
> a pointer?
>
> bye,
> Heiko
>>> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
>>> index 22db1bb..8d4f5f6 100644
>>> --- a/common/cmd_i2c.c
>>> +++ b/common/cmd_i2c.c
>>> @@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>>> struct udevice *dev;
>>> #endif
>>>
>>> - if (argc != 5)
>>> + if ((argc < 5) || (argc > 6))
>>> return cmd_usage(cmdtp);
>>>
>>> /*
>>> @@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>>> return cmd_usage(cmdtp);
>>>
>>> /*
>>> - * Length is the number of objects, not number of bytes.
>>> + * Length is the number of bytes.
>>> */
>>> length = simple_strtoul(argv[4], NULL, 16);
>>>
>>> @@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>>> return i2c_report_err(ret, I2C_ERR_WRITE);
>>> #endif
>>>
>>> - while (length-- > 0) {
>>> + if (argc == 6 && !strcmp(argv[5], "-s")) {
>>> + /*
>>> + * Write all bytes in a single I2C transaction. If the target
>>> + * device is an EEPROM, it is your responsibility to not cross
>>> + * a page boundary. No write delay upon completion, take this
>>> + * into account if linking commands.
>>> + */
>>> #ifdef CONFIG_DM_I2C
>>> - ret = i2c_write(dev, devaddr++, memaddr++, 1);
>>> + ret = i2c_write(dev, devaddr, memaddr, length);
>>> #else
>>> - ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>>> + ret = i2c_write(chip, devaddr, alen, memaddr, length);
>>> #endif
>>> if (ret)
>>> return i2c_report_err(ret, I2C_ERR_WRITE);
>>> + } else {
>>> + /*
>>> + * Repeated addressing - perform <length> separate
>>> + * write transactions of one byte each
>>> + */
>>> + while (length-- > 0) {
>>> +#ifdef CONFIG_DM_I2C
>>> + ret = i2c_write(dev, devaddr++, memaddr++, 1);
>>> +#else
>>> + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1);
>>> +#endif
>>> + if (ret)
>>> + return i2c_report_err(ret, I2C_ERR_WRITE);
>>> /*
>>> * No write delay with FRAM devices.
>>> */
>>> #if !defined(CONFIG_SYS_I2C_FRAM)
>>> - udelay(11000);
>>> + udelay(11000);
>>> #endif
>>> + }
>>> }
>>> return 0;
>>> }
>>> @@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = {
>>> U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""),
>>> U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""),
>>> U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""),
>>> - U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""),
>>> + U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""),
>>> #ifdef CONFIG_DM_I2C
>>> U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""),
>>> #endif
>>> @@ -1890,7 +1910,8 @@ static char i2c_help_text[] =
>>> "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n"
>>> "i2c probe [address] - test for and show device(s) on the I2C bus\n"
>>> "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n"
>>> - "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n"
>>> + "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n"
>>> + " to I2C; the -s option selects bulk write in a single transaction\n"
>>> #ifdef CONFIG_DM_I2C
>>> "i2c flags chip [flags] - set or get chip flags\n"
>>> #endif
>>> @@ -1902,7 +1923,7 @@ static char i2c_help_text[] =
>>> #endif
>>>
>>> U_BOOT_CMD(
>>> - i2c, 6, 1, do_i2c,
>>> + i2c, 7, 1, do_i2c,
>>> "I2C sub-system",
>>> i2c_help_text
>>> );
>>> --
>>> 1.7.9.5
>>>
>>
>> Regards,
>> Simon
>>
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-02-06 10:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 17:56 [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction Lubomir Popov
2015-02-03 0:59 ` Simon Glass
2015-02-03 8:10 ` Lubomir Popov
2015-02-03 8:35 ` Masahiro Yamada
2015-02-04 0:41 ` Simon Glass
2015-02-06 6:20 ` Heiko Schocher
2015-02-06 6:54 ` Heiko Schocher
2015-02-06 10:54 ` Lubomir Popov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.