Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
Hello ksi,
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
Also it is compatible with existing board ports. I tried this on an 8xx based board (mgsuvd) with success.
From 4f47e858059d02ca9a9a38b35bef55b69c98c3d3 Mon Sep 17 00:00:00 2001
From: Heiko Schocher hs@denx.de Date: Fri, 13 Feb 2009 11:10:54 +0100 Subject: [PATCH] soft_i2c: add multibus support
Signed-off-by: Heiko Schocher hs@denx.de --- drivers/i2c/i2c_core.c | 3 + drivers/i2c/soft_i2c.c | 129 ++++++++++++++++++++++++++++------------------ include/configs/mgsuvd.h | 3 + include/i2c.h | 8 +++- 4 files changed, 91 insertions(+), 52 deletions(-)
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index 4466908..e0afd10 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -39,6 +39,7 @@ extern i2c_adap_t tsi108_i2c_adap; #endif
i2c_adap_t *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] = CONFIG_SYS_I2C_ADAPTERS; +i2c_adap_t *cur_adap_nr = NULL; #ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_bus_t i2c_bus[CONFIG_SYS_NUM_I2C_BUSSES] = CONFIG_SYS_I2C_BUSSES; #endif @@ -208,6 +209,7 @@ int i2c_set_bus_num(unsigned int bus) #endif
i2c_cur_bus = bus; + cur_adap_nr = (i2c_adap_t *)&ADAP(bus); return(0); }
@@ -243,6 +245,7 @@ void i2c_init_all(void) { int i;
+ cur_adap_nr = (i2c_adap_t *)&ADAP(0); for (i = 0; i < CONFIG_SYS_NUM_I2C_ADAPTERS; i++) { if ((i2c_adap[i]->speed != 0) && (i2c_adap[i]->init != NULL)) { i2c_adap[i]->init(i2c_adap[i]->speed, i2c_adap[i]->slaveaddr); diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c index da6cec1..b5302a4 100644 --- a/drivers/i2c/soft_i2c.c +++ b/drivers/i2c/soft_i2c.c @@ -55,7 +55,6 @@ DECLARE_GLOBAL_DATA_PTR; /*----------------------------------------------------------------------- * Definitions */ - #define RETRIES 0
@@ -216,52 +215,6 @@ static int write_byte(uchar data) return(nack); /* not a nack is an ack */ }
-#if defined(CONFIG_I2C_MULTI_BUS) -/* - * Functions for multiple I2C bus handling - */ -unsigned int i2c_get_bus_num(void) -{ - return i2c_bus_num; -} - -int i2c_set_bus_num(unsigned int bus) -{ -#if defined(CONFIG_I2C_MUX) - if (bus < CONFIG_SYS_MAX_I2C_BUS) { - i2c_bus_num = bus; - } else { - int ret; - - ret = i2x_mux_select_mux(bus); - if (ret == 0) - i2c_bus_num = bus; - else - return ret; - } -#else - if (bus >= CONFIG_SYS_MAX_I2C_BUS) - return -1; - i2c_bus_num = bus; -#endif - return 0; -} - -/* TODO: add 100/400k switching */ -unsigned int i2c_get_bus_speed(void) -{ - return CONFIG_SYS_I2C_SPEED; -} - -int i2c_set_bus_speed(unsigned int speed) -{ - if (speed != CONFIG_SYS_I2C_SPEED) - return -1; - - return 0; -} -#endif - /*----------------------------------------------------------------------- * if ack == I2C_ACK, ACK the byte so can continue reading, else * send I2C_NOACK to end the read. @@ -299,7 +252,7 @@ static uchar read_byte(int ack) /*----------------------------------------------------------------------- * Initialization */ -void i2c_init (int speed, int slaveaddr) +void soft_i2c_init (int speed, int slaveaddr) { #if defined(CONFIG_SYS_I2C_INIT_BOARD) /* call board specific i2c bus reset routine before accessing the */ @@ -322,7 +275,7 @@ void i2c_init (int speed, int slaveaddr) * completion of EEPROM writes since the chip stops responding until * the write completes (typically 10mSec). */ -int i2c_probe(uchar addr) +int soft_i2c_probe(u_int8_t addr) { int rc;
@@ -340,7 +293,7 @@ int i2c_probe(uchar addr) /*----------------------------------------------------------------------- * Read bytes */ -int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) +int soft_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { int shift; PRINTD("i2c_read: chip %02X addr %02X alen %d buffer %p len %d\n", @@ -414,7 +367,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) /*----------------------------------------------------------------------- * Write bytes */ -int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) +int soft_i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) { int shift, failures = 0;
@@ -444,3 +397,77 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) send_stop(); return(failures); } + +static unsigned int soft_i2c_get_bus_speed(void) +{ + return cur_adap_nr->speed; +} + +static unsigned int soft_i2c_set_bus_speed(unsigned int speed) +{ + if (speed != cur_adap_nr->speed) + return -1; + return(speed); +} + +i2c_adap_t soft_i2c_adap[] = { + { + .init = soft_i2c_init, + .probe = soft_i2c_probe, + .read = soft_i2c_read, + .write = soft_i2c_write, + .set_bus_speed = soft_i2c_set_bus_speed, + .get_bus_speed = soft_i2c_get_bus_speed, + .speed = CONFIG_SYS_SOFT_I2C_SPEED, + .slaveaddr = CONFIG_SYS_SOFT_I2C_SLAVE, + .init_done = 0, + .hwadapnr = 0, + .name = "soft-i2c" + }, +#if defined(I2C_SOFT_DECLARATIONS2) + { + .init = soft_i2c_init, + .probe = soft_i2c_probe, + .read = soft_i2c_read, + .write = soft_i2c_write, + .set_bus_speed = soft_i2c_set_bus_speed, + .get_bus_speed = soft_i2c_get_bus_speed, + .speed = CONFIG_SYS_SOFT_I2C2_SPEED, + .slaveaddr = CONFIG_SYS_SOFT_I2C2_SLAVE, + .init_done = 0, + .hwadapnr = 1, + .name = "soft-i2c#2" + }, +#endif +#if defined(I2C_SOFT_DECLARATIONS3) + { + .init = soft_i2c_init, + .probe = soft_i2c_probe, + .read = soft_i2c_read, + .write = soft_i2c_write, + .set_bus_speed = soft_i2c_set_bus_speed, + .get_bus_speed = soft_i2c_get_bus_speed, + .speed = CONFIG_SYS_SOFT_I2C3_SPEED, + .slaveaddr = CONFIG_SYS_SOFT_I2C3_SLAVE, + .init_done = 0, + .hwadapnr = 2, + .name = "soft-i2c#3" + }, +#endif +#if defined(I2C_SOFT_DECLARATIONS4) + { + .init = soft_i2c_init, + .probe = soft_i2c_probe, + .read = soft_i2c_read, + .write = soft_i2c_write, + .set_bus_speed = soft_i2c_set_bus_speed, + .get_bus_speed = soft_i2c_get_bus_speed, + .speed = CONFIG_SYS_SOFT_I2C4_SPEED, + .slaveaddr = CONFIG_SYS_SOFT_I2C4_SLAVE, + .init_done = 0, + .hwadapnr = 3, + .name = "soft-i2c#4" + }, +#endif +}; + diff --git a/include/configs/mgsuvd.h b/include/configs/mgsuvd.h index 3ea0725..b1debbc 100644 --- a/include/configs/mgsuvd.h +++ b/include/configs/mgsuvd.h @@ -270,6 +270,7 @@ #define CONFIG_SYS_NUM_I2C_BUSSES 3 #define CONFIG_SYS_I2C_MAX_HOPS 1 #define CONFIG_SOFT_I2C /* I2C bit-banged */ +#define I2C_SOFT_DEFS #define I2C_SOFT_DECLARATIONS I2C_SOFT_DEFS #define CONFIG_SYS_SOFT_I2C_SPEED 50000 #define CONFIG_SYS_SOFT_I2C_SLAVE 0x7F @@ -277,6 +278,8 @@ #define CONFIG_SYS_I2C_BUSSES { {0, {I2C_NULL_HOP}}, \ {0, {{I2C_MUX_PCA9542, 0x70, 0}}}, \ {0, {{I2C_MUX_PCA9542, 0x70, 1}}}} + +#define CONFIG_I2C_CMD_TREE 1 /* * Software (bit-bang) I2C driver configuration */ diff --git a/include/i2c.h b/include/i2c.h index ea2c3b2..a3cc3ca 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -95,7 +95,8 @@ typedef struct i2c_adapter { int speed; int slaveaddr; int init_done; - char *name; + int hwadapnr; + char *name; } i2c_adap_t;
#ifndef CONFIG_SYS_I2C_DIRECT_BUS @@ -130,6 +131,7 @@ extern i2c_bus_t i2c_bus[]; #endif
extern i2c_adap_t *i2c_adap[]; +extern i2c_adap_t *cur_adap_nr;
/* * i2c_get_bus_num: @@ -226,7 +228,11 @@ void i2c_reloc_fixup(void); # else # define I2C_SOFT_DEFS # endif +#endif
+#ifndef CONFIG_SYS_I2C_SLAVE +#define CONFIG_SYS_I2C_SLAVE 0x7f +#endif /* * Initialization, must be called once on start up, may be called * repeatedly to change the speed and slave addresses.
On Fri, 13 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different pin pairs from different chips.
OK, please explain how are you going to make different functions for different adapters? Let's say you want to use 2 on-SoC GPIO pins for adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from some chip sitting behind that bridge for adapter #2 if all those pin sets are accessed totally different. I won't even start about using pins from different chips for SDA and SCL (let's say you only have one GPIO available on your SoC and another one on PCI Bridge.)
What your patch creates is just aliases to the SAME physical adapter.
Please explain.
Also it is compatible with existing board ports. I tried this on an 8xx based board (mgsuvd) with success.
From 4f47e858059d02ca9a9a38b35bef55b69c98c3d3 Mon Sep 17 00:00:00 2001
From: Heiko Schocher hs@denx.de Date: Fri, 13 Feb 2009 11:10:54 +0100 Subject: [PATCH] soft_i2c: add multibus support
Signed-off-by: Heiko Schocher hs@denx.de
drivers/i2c/i2c_core.c | 3 + drivers/i2c/soft_i2c.c | 129 ++++++++++++++++++++++++++++------------------ include/configs/mgsuvd.h | 3 + include/i2c.h | 8 +++- 4 files changed, 91 insertions(+), 52 deletions(-)
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index 4466908..e0afd10 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -39,6 +39,7 @@ extern i2c_adap_t tsi108_i2c_adap; #endif
i2c_adap_t *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] = CONFIG_SYS_I2C_ADAPTERS; +i2c_adap_t *cur_adap_nr = NULL; #ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_bus_t i2c_bus[CONFIG_SYS_NUM_I2C_BUSSES] = CONFIG_SYS_I2C_BUSSES; #endif @@ -208,6 +209,7 @@ int i2c_set_bus_num(unsigned int bus) #endif
i2c_cur_bus = bus;
- cur_adap_nr = (i2c_adap_t *)&ADAP(bus); return(0);
}
@@ -243,6 +245,7 @@ void i2c_init_all(void) { int i;
- cur_adap_nr = (i2c_adap_t *)&ADAP(0); for (i = 0; i < CONFIG_SYS_NUM_I2C_ADAPTERS; i++) { if ((i2c_adap[i]->speed != 0) && (i2c_adap[i]->init != NULL)) { i2c_adap[i]->init(i2c_adap[i]->speed, i2c_adap[i]->slaveaddr);
diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c index da6cec1..b5302a4 100644 --- a/drivers/i2c/soft_i2c.c +++ b/drivers/i2c/soft_i2c.c @@ -55,7 +55,6 @@ DECLARE_GLOBAL_DATA_PTR; /*-----------------------------------------------------------------------
- Definitions
*/
#define RETRIES 0
@@ -216,52 +215,6 @@ static int write_byte(uchar data) return(nack); /* not a nack is an ack */ }
-#if defined(CONFIG_I2C_MULTI_BUS) -/*
- Functions for multiple I2C bus handling
- */
-unsigned int i2c_get_bus_num(void) -{
- return i2c_bus_num;
-}
-int i2c_set_bus_num(unsigned int bus) -{ -#if defined(CONFIG_I2C_MUX)
- if (bus < CONFIG_SYS_MAX_I2C_BUS) {
i2c_bus_num = bus;- } else {
int ret;ret = i2x_mux_select_mux(bus);if (ret == 0)i2c_bus_num = bus;elsereturn ret;- }
-#else
- if (bus >= CONFIG_SYS_MAX_I2C_BUS)
return -1;- i2c_bus_num = bus;
-#endif
- return 0;
-}
-/* TODO: add 100/400k switching */ -unsigned int i2c_get_bus_speed(void) -{
- return CONFIG_SYS_I2C_SPEED;
-}
-int i2c_set_bus_speed(unsigned int speed) -{
- if (speed != CONFIG_SYS_I2C_SPEED)
return -1;- return 0;
-} -#endif
/*-----------------------------------------------------------------------
- if ack == I2C_ACK, ACK the byte so can continue reading, else
- send I2C_NOACK to end the read.
@@ -299,7 +252,7 @@ static uchar read_byte(int ack) /*-----------------------------------------------------------------------
- Initialization
*/ -void i2c_init (int speed, int slaveaddr) +void soft_i2c_init (int speed, int slaveaddr) { #if defined(CONFIG_SYS_I2C_INIT_BOARD) /* call board specific i2c bus reset routine before accessing the */ @@ -322,7 +275,7 @@ void i2c_init (int speed, int slaveaddr)
- completion of EEPROM writes since the chip stops responding until
- the write completes (typically 10mSec).
*/ -int i2c_probe(uchar addr) +int soft_i2c_probe(u_int8_t addr) { int rc;
@@ -340,7 +293,7 @@ int i2c_probe(uchar addr) /*-----------------------------------------------------------------------
- Read bytes
*/ -int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) +int soft_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { int shift; PRINTD("i2c_read: chip %02X addr %02X alen %d buffer %p len %d\n", @@ -414,7 +367,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) /*-----------------------------------------------------------------------
- Write bytes
*/ -int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) +int soft_i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) { int shift, failures = 0;
@@ -444,3 +397,77 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) send_stop(); return(failures); }
+static unsigned int soft_i2c_get_bus_speed(void) +{
- return cur_adap_nr->speed;
+}
+static unsigned int soft_i2c_set_bus_speed(unsigned int speed) +{
- if (speed != cur_adap_nr->speed)
return -1;- return(speed);
+}
+i2c_adap_t soft_i2c_adap[] = {
- {
.init = soft_i2c_init,.probe = soft_i2c_probe,.read = soft_i2c_read,.write = soft_i2c_write,.set_bus_speed = soft_i2c_set_bus_speed,.get_bus_speed = soft_i2c_get_bus_speed,.speed = CONFIG_SYS_SOFT_I2C_SPEED,.slaveaddr = CONFIG_SYS_SOFT_I2C_SLAVE,.init_done = 0,.hwadapnr = 0,.name = "soft-i2c"- },
+#if defined(I2C_SOFT_DECLARATIONS2)
- {
.init = soft_i2c_init,.probe = soft_i2c_probe,.read = soft_i2c_read,.write = soft_i2c_write,.set_bus_speed = soft_i2c_set_bus_speed,.get_bus_speed = soft_i2c_get_bus_speed,.speed = CONFIG_SYS_SOFT_I2C2_SPEED,.slaveaddr = CONFIG_SYS_SOFT_I2C2_SLAVE,.init_done = 0,.hwadapnr = 1,.name = "soft-i2c#2"- },
+#endif +#if defined(I2C_SOFT_DECLARATIONS3)
- {
.init = soft_i2c_init,.probe = soft_i2c_probe,.read = soft_i2c_read,.write = soft_i2c_write,.set_bus_speed = soft_i2c_set_bus_speed,.get_bus_speed = soft_i2c_get_bus_speed,.speed = CONFIG_SYS_SOFT_I2C3_SPEED,.slaveaddr = CONFIG_SYS_SOFT_I2C3_SLAVE,.init_done = 0,.hwadapnr = 2,.name = "soft-i2c#3"- },
+#endif +#if defined(I2C_SOFT_DECLARATIONS4)
- {
.init = soft_i2c_init,.probe = soft_i2c_probe,.read = soft_i2c_read,.write = soft_i2c_write,.set_bus_speed = soft_i2c_set_bus_speed,.get_bus_speed = soft_i2c_get_bus_speed,.speed = CONFIG_SYS_SOFT_I2C4_SPEED,.slaveaddr = CONFIG_SYS_SOFT_I2C4_SLAVE,.init_done = 0,.hwadapnr = 3,.name = "soft-i2c#4"- },
+#endif +};
diff --git a/include/configs/mgsuvd.h b/include/configs/mgsuvd.h index 3ea0725..b1debbc 100644 --- a/include/configs/mgsuvd.h +++ b/include/configs/mgsuvd.h @@ -270,6 +270,7 @@ #define CONFIG_SYS_NUM_I2C_BUSSES 3 #define CONFIG_SYS_I2C_MAX_HOPS 1 #define CONFIG_SOFT_I2C /* I2C bit-banged */ +#define I2C_SOFT_DEFS #define I2C_SOFT_DECLARATIONS I2C_SOFT_DEFS #define CONFIG_SYS_SOFT_I2C_SPEED 50000 #define CONFIG_SYS_SOFT_I2C_SLAVE 0x7F @@ -277,6 +278,8 @@ #define CONFIG_SYS_I2C_BUSSES { {0, {I2C_NULL_HOP}}, \ {0, {{I2C_MUX_PCA9542, 0x70, 0}}}, \ {0, {{I2C_MUX_PCA9542, 0x70, 1}}}}
+#define CONFIG_I2C_CMD_TREE 1 /*
- Software (bit-bang) I2C driver configuration
*/ diff --git a/include/i2c.h b/include/i2c.h index ea2c3b2..a3cc3ca 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -95,7 +95,8 @@ typedef struct i2c_adapter { int speed; int slaveaddr; int init_done;
- char *name;
- int hwadapnr;
- char *name;
} i2c_adap_t;
#ifndef CONFIG_SYS_I2C_DIRECT_BUS @@ -130,6 +131,7 @@ extern i2c_bus_t i2c_bus[]; #endif
extern i2c_adap_t *i2c_adap[]; +extern i2c_adap_t *cur_adap_nr;
/*
- i2c_get_bus_num:
@@ -226,7 +228,11 @@ void i2c_reloc_fixup(void); # else # define I2C_SOFT_DEFS # endif +#endif
+#ifndef CONFIG_SYS_I2C_SLAVE +#define CONFIG_SYS_I2C_SLAVE 0x7f +#endif /*
- Initialization, must be called once on start up, may be called
- repeatedly to change the speed and slave addresses.
-- 1.6.0.6
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different pin pairs from different chips.
This you can also do with "my" suggestion ...
OK, please explain how are you going to make different functions for different adapters? Let's say you want to use 2 on-SoC GPIO pins for
You can do now the following for example in your include/configs/MPC8548CDS.h example:
you only have to define
#define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
if this is a real driver you can make a function in your board code say (just a fast thought):
void i2c_soft_sda (int bit) { switch(cur_adap_nr->hwadapnr) { case 0: /* adapter specfic code 0 */ break; case 1: /* adapter specfic code 1 */ break; [...] } }
and define in config file
#define I2C_SDA(bit) i2c_soft_sda (bit)
adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from some chip sitting behind that bridge for adapter #2 if all those pin sets are accessed totally different. I won't even start about using pins from different chips for SDA and SCL (let's say you only have one GPIO available on your SoC and another one on PCI Bridge.)
What your patch creates is just aliases to the SAME physical adapter.
No, it is not! I only use the same functions, but in the board specific code it is possible to made a switch and access the Pins where ever they are.
bye Heiko
On Sat, 14 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different pin pairs from different chips.
This you can also do with "my" suggestion ...
OK, please explain how are you going to make different functions for different adapters? Let's say you want to use 2 on-SoC GPIO pins for
You can do now the following for example in your include/configs/MPC8548CDS.h example:
you only have to define
#define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
if this is a real driver you can make a function in your board code say (just a fast thought):
void i2c_soft_sda (int bit) { switch(cur_adap_nr->hwadapnr) { case 0: /* adapter specfic code 0 */ break; case 1: /* adapter specfic code 1 */ break; [...] } }
and define in config file
#define I2C_SDA(bit) i2c_soft_sda (bit)
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no "this" pointer... And that is just a tip of an iceberg...
And the million dollar question -- what is the potential gain?
adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from some chip sitting behind that bridge for adapter #2 if all those pin sets are accessed totally different. I won't even start about using pins from different chips for SDA and SCL (let's say you only have one GPIO available on your SoC and another one on PCI Bridge.)
What your patch creates is just aliases to the SAME physical adapter.
No, it is not! I only use the same functions, but in the board specific code it is possible to made a switch and access the Pins where ever they are.
You are adding unnecessary complexity to the code. And you break uniformity. Those defines in config/soft_i2c.c make real inline functions at _COMPILE_ time. Your approach shifts it to _LINK_ time. It also makes those drivers come from 3 places ($(BOARD).c, include/configs/$(BOARD).h, and soft_i2c.c) instead of just two. And it introduces a whole bunch of subtle problems with no simple and elegant solutions...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Sat, 14 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different pin pairs from different chips.
This you can also do with "my" suggestion ...
OK, please explain how are you going to make different functions for different adapters? Let's say you want to use 2 on-SoC GPIO pins for
You can do now the following for example in your include/configs/MPC8548CDS.h example:
you only have to define
#define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
if this is a real driver you can make a function in your board code say (just a fast thought):
void i2c_soft_sda (int bit) { switch(cur_adap_nr->hwadapnr) { case 0: /* adapter specfic code 0 */ break; case 1: /* adapter specfic code 1 */ break; [...] } }
and define in config file
#define I2C_SDA(bit) i2c_soft_sda (bit)
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can
Yes. But that was just a thought, it should be possible to do this also in macros.
NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do
Maybe more logical, but not needed.
everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
The only problem with that is it breaks uniformity and makes another mess.
Just, if we do this, but we don;t need to do it so.
The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
"this" pointer... And that is just a tip of an iceberg...
And the million dollar question -- what is the potential gain?
I want to avoid such a big change in soft_i2c.c. Also if you have 4 bitbang instances with your version you have 3 times more code.
But if others are on your side, I have no problem with your approach.
adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from some chip sitting behind that bridge for adapter #2 if all those pin sets are accessed totally different. I won't even start about using pins from different chips for SDA and SCL (let's say you only have one GPIO available on your SoC and another one on PCI Bridge.)
What your patch creates is just aliases to the SAME physical adapter.
No, it is not! I only use the same functions, but in the board specific code it is possible to made a switch and access the Pins where ever they are.
You are adding unnecessary complexity to the code. And you break uniformity.
not really.
Those defines in config/soft_i2c.c make real inline functions at _COMPILE_ time. Your approach shifts it to _LINK_ time. It also makes those drivers come from 3 places ($(BOARD).c, include/configs/$(BOARD).h, and soft_i2c.c)
Thats not really a problem.
bye Heiko
On Sun, 15 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Sat, 14 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@koi8.net
diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 @@ -1,4 +1,8 @@ /*
- Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
[...]
The following patch is based on your patches without 7/12 and adds multibus support for the soft_i2c driver without doing such a big change as you did. Maybe it is not yet perfect, because it is just a fast try, but I think we should go this way. What do you/others think?
The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different pin pairs from different chips.
This you can also do with "my" suggestion ...
OK, please explain how are you going to make different functions for different adapters? Let's say you want to use 2 on-SoC GPIO pins for
You can do now the following for example in your include/configs/MPC8548CDS.h example:
you only have to define
#define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
if this is a real driver you can make a function in your board code say (just a fast thought):
void i2c_soft_sda (int bit) { switch(cur_adap_nr->hwadapnr) { case 0: /* adapter specfic code 0 */ break; case 1: /* adapter specfic code 1 */ break; [...] } }
and define in config file
#define I2C_SDA(bit) i2c_soft_sda (bit)
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can
Yes. But that was just a thought, it should be possible to do this also in macros.
Not if you want to make functions in $(BOARD).c.
NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do
Maybe more logical, but not needed.
everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
The only problem with that is it breaks uniformity and makes another mess.
Just, if we do this, but we don;t need to do it so.
The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
"this" pointer... And that is just a tip of an iceberg...
And the million dollar question -- what is the potential gain?
I want to avoid such a big change in soft_i2c.c. Also if you have 4 bitbang instances with your version you have 3 times more code.
There is almost NO change in soft_i2c.c. It is simply templetized and 4 #ifdef'ed instances are added. If you only have one bitbanged adapter in the system it generates _EXACTLY_ the same source code as original one did after CPP pass.
But if others are on your side, I have no problem with your approach.
I don't think there is a viable reason to object unless somebody came up with something better.
adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from some chip sitting behind that bridge for adapter #2 if all those pin sets are accessed totally different. I won't even start about using pins from different chips for SDA and SCL (let's say you only have one GPIO available on your SoC and another one on PCI Bridge.)
What your patch creates is just aliases to the SAME physical adapter.
No, it is not! I only use the same functions, but in the board specific code it is possible to made a switch and access the Pins where ever they are.
You are adding unnecessary complexity to the code. And you break uniformity.
not really.
Those defines in config/soft_i2c.c make real inline functions at _COMPILE_ time. Your approach shifts it to _LINK_ time. It also makes those drivers come from 3 places ($(BOARD).c, include/configs/$(BOARD).h, and soft_i2c.c)
Thats not really a problem.
It is. The more files you spread the same code between the more places to fix if something's changed.
There is another reason why those functions were made into macros instead of functions. I'll come to that in my reply to your next email.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Sun, 15 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Sat, 14 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
> Signed-off-by: Sergey Kubushyn ksi@koi8.net > --- > diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c > --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800 > +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800 > @@ -1,4 +1,8 @@ > /* > + * Copyright (c) 2009 Sergey Kubushyn ksi@koi8.net > + * > + * Changes for multibus/multiadapter I2C support. > + * > * (C) Copyright 2001, 2002 > * Wolfgang Denk, DENX Software Engineering, wd@denx.de. >
[...]
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
OK, if we really need this.
bye Heiko
Dear Heiko Schocher,
In message 49992BCE.5020906@denx.de you wrote:
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
OK, if we really need this.
But - do we? Really?
We never promised that U-Boot will support each and every broken hardware design ;-)
Best regards,
Wolfgang Denk
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message 49992BCE.5020906@denx.de you wrote:
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
OK, if we really need this.
But - do we? Really?
We never promised that U-Boot will support each and every broken hardware design ;-)
Yep. But it would've been nice if it did :)
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902152224290.17769@home-gw.koi8.net you wrote:
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
This is a very unlikely hypothetical case. Based on such assumptions you can design any arbitrary complexity in any code.
For now, please lets assume that there is no such usage mode, and that we do not add complexity to support it.
I don't think there is a viable reason to object unless somebody came up with something better.
Well, complex code that is difficult to read and difficult to understand is a very viable reason to object, me thinks.
Best regards,
Wolfgang Denk
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902152224290.17769@home-gw.koi8.net you wrote:
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
This is a very unlikely hypothetical case. Based on such assumptions you can design any arbitrary complexity in any code.
For now, please lets assume that there is no such usage mode, and that we do not add complexity to support it.
I don't think there is a viable reason to object unless somebody came up with something better.
Well, complex code that is difficult to read and difficult to understand is a very viable reason to object, me thinks.
The code is not all that complex. Most of the code is the same, there is just a thin HAL over it. And it is _NOT_ for reading two SPDs on different busses, it is for consistency and uniformity. Reading two SPDs is a free bonus, there is no a single line of additional code for this.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902152224290.17769@home-gw.koi8.net you wrote:
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember that infamous "640K ought to be enough for anybody..."
This is a very unlikely hypothetical case. Based on such assumptions you can design any arbitrary complexity in any code.
For now, please lets assume that there is no such usage mode, and that we do not add complexity to support it.
I don't think there is a viable reason to object unless somebody came up with something better.
Well, complex code that is difficult to read and difficult to understand is a very viable reason to object, me thinks.
The code is not all that complex. Most of the code is the same, there is just a thin HAL over it. And it is _NOT_ for reading two SPDs on different busses, it is for consistency and uniformity. Reading two SPDs is a free bonus, there is no a single line of additional code for this.
If the current pointer is writeable, this is also possible ...
bye Heiko
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
And the million dollar question -- what is the potential gain?
Indeed. The same question goes to you - where is this added complexity really needed? So far nowhere. Are we just talking about hypothetical cases, or about a real design? How many such designs? Just a single one?
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Best regards,
Wolfgang Denk
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \ I2C_SOFT_DECLARATIONS##n \ I2C_DELAY##n; \ I2C_SDA##n(1); \ I2C_ACTIVE##n; \ I2C_DELAY##n; \ I2C_SCL##n(1); \ I2C_DELAY##n; \ I2C_SDA##n(0); \ I2C_DELAY##n; \ }
...
I2C_SOFT_SEND_START()
I2C_SOFT_SEND_START(2) === Cut ===
That will make two functions replacing "##n" with literal argument:
=== Cut === static void send_start(void) { I2C_SOFT_DECLARATIONS I2C_DELAY; I2C_SDA(1); I2C_ACTIVE; I2C_DELAY; I2C_SCL(1); I2C_DELAY; I2C_SDA(0); I2C_DELAY; }
static void send_start2(void) { I2C_SOFT_DECLARATIONS2 I2C_DELAY2; I2C_SDA2(1); I2C_ACTIVE2; I2C_DELAY2; I2C_SCL2(1); I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2; } === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
Not rocket science and not much of black magic, just simple and straightforward token pasting...
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
And the million dollar question -- what is the potential gain?
Indeed. The same question goes to you - where is this added complexity really needed? So far nowhere. Are we just talking about hypothetical cases, or about a real design? How many such designs? Just a single one?
Please see above.
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
What is unreadable in that code?
It gets out of control. You don;t see any more how much code gets generated from that.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
Well, I tell you again that size does matter, and may even be a killing point when deciding abouut a patch.
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
[On the other hand I still wonder why I have never seen any such board appear to me in real life yet. None of the 500+ boards supported in U-Boot uses any such configuration.]
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
And the million dollar question -- what is the potential gain?
Indeed. The same question goes to you - where is this added complexity really needed? So far nowhere. Are we just talking about hypothetical cases, or about a real design? How many such designs? Just a single one?
Please see above.
You did not answer my question.
Best regards,
Wolfgang Denk
On Tue, 17 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
What is unreadable in that code?
It gets out of control. You don;t see any more how much code gets generated from that.
Why not?
But anyways, I can make those instances manually, without CPP participations. Would it be better?
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
Well, I tell you again that size does matter, and may even be a killing point when deciding abouut a patch.
There is no much size increase here. Also it is still possible to simply include just those adapters that are required for SPD and thus reduce size if several I2C drivers make it too big.
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
[On the other hand I still wonder why I have never seen any such board appear to me in real life yet. None of the 500+ boards supported in U-Boot uses any such configuration.]
Eh, for those 500+ (actually 472 judging on the number of files in include/configs) boards there are at least twice that many that were never submitted to the U-Boot tree... I have ported U-Boot/Linux to at least 10 different boards that were never submitted anywhere myself. You know, company only pays for making their boards work, not for pushing them in the official tree.
This is the second company where I have a luxury of crafting patches for submission (the first one was that now defunct that allowed me to push that Davinci port.)
And one can not even imagine what runaway hardware designers can design... I still remember some Brasilian MPC8560/Fujitsu ARM boards that were pure nightmare to work on :(
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
No, it is not easy. You simply did not dig deep enough :)
And the million dollar question -- what is the potential gain?
Indeed. The same question goes to you - where is this added complexity really needed? So far nowhere. Are we just talking about hypothetical cases, or about a real design? How many such designs? Just a single one?
Please see above.
You did not answer my question.
OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.)
This is about consistent logical API for multiple I2C busses. As for multiple busses I personally need that right now for my new board. That board has 2 ST Micro M41ST87 chips on it (braindead chip, I wouldn't've used such a weirdo but it is "certified" by our licensing authorities so I have to) and DS1307 RTT. M41ST87 is a Tamper Detector with RTT but I can not use those RTTs for timekeeping because they are frozen to give a tamper event timestamp hence the separate RTT.
Operation procedure requires to check for tamper events on bootup and if one's detected the system must log the current bootup time, give some indication to the user and halt for proper authorities to investigate.
The problem is M41ST87 uses a fixed I2C address, 0x68 that is also used by RTT. That means I need 3 I2C busses to talk to those 3 chips...
And this is just a first such board out of a whole bunch that we have in various stages. The next one, OMAP3-based, is almost ready, we should have prototypes in a month or two. And we are not the only players at that niche market (however very old ones,) there are others.
And this is just one of heavily regulated industries (you know those big shiny casinos on Las Vegas Strip, don't ya? :) There are others with different braindead requirements out there...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171456520.30777@home-gw.koi8.net you wrote:
[On the other hand I still wonder why I have never seen any such board appear to me in real life yet. None of the 500+ boards supported in U-Boot uses any such configuration.]
Eh, for those 500+ (actually 472 judging on the number of files in include/configs) boards there are at least twice that many that were never
There are some boards that share one config file...
OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.)
...
We agree that there are cases where multiple busses are needed. But do we need such complexity before relocation?
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171456520.30777@home-gw.koi8.net you wrote:
[On the other hand I still wonder why I have never seen any such board appear to me in real life yet. None of the 500+ boards supported in U-Boot uses any such configuration.]
Eh, for those 500+ (actually 472 judging on the number of files in include/configs) boards there are at least twice that many that were never
There are some boards that share one config file...
Yes, I know.
OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.)
...
We agree that there are cases where multiple busses are needed. But do we need such complexity before relocation?
It is not before relocation. I don't see a reason to do this before relocation either. But if we want to have such a possibility after relocation we also need a mechanism to do this.
And it is not all that complex, I can't understand why you think it is.
As for now we have a set of regular i2c functions (i2c_init/i2c_read/etc.) in each and every i2c driver. Those functions are exported so when we pick up a driver (with e.g. CONFIG_HARD_I2C that metastasize through the entire U-Boot source choosing different drivers for different platforms) those functions get called where i2c_read() etc. are used.
I make all those functions static so they are not exported and add a simple exported structure with pointers to those functions. Then there is one wrapper, i2c_core.c that holds global bus number and exports that usual set of i2c_read() and friends. Those functions in turn just play a dispatcher role calling appropriate functions from an array of those driver structures using current bus number as an index into that array. There is nothing more complex than that.
The i2c_set_bus_number() is a bit more complex than just changing that global variable to accomodate multiplexed busses but not rocket science either -- if the current bus is multiplexed, it switches off all the muxes in the path starting with the farthest one (if it is multihop, but I doubt we'll see such a beast so it will be just one chip) and turn on the muxes for the new bus (if it is multiplexed) starting with the closest one. The companion i2c_get_bus_num() just returns that global variable.
And that's all to it.
What is that complex with such a design?
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171456520.30777@home-gw.koi8.net you wrote:
[...]
OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.)
...
We agree that there are cases where multiple busses are needed. But do we need such complexity before relocation?
It is not before relocation. I don't see a reason to do this before relocation either. But if we want to have such a possibility after relocation we also need a mechanism to do this.
But, if this current pointer is writeable, it is also usable before relocation!
And it is not all that complex, I can't understand why you think it is.
We actually mix things:
- complex: There we (Wolfgang and I) talk about your implentation of the bitbang driver
not about your i2c-core ...
As for now we have a set of regular i2c functions (i2c_init/i2c_read/etc.) in each and every i2c driver. Those functions are exported so when we pick up a driver (with e.g. CONFIG_HARD_I2C that metastasize through the entire U-Boot source choosing different drivers for different platforms) those functions get called where i2c_read() etc. are used.
I make all those functions static so they are not exported and add a simple exported structure with pointers to those functions. Then there is one wrapper, i2c_core.c that holds global bus number and exports that usual set of i2c_read() and friends. Those functions in turn just play a dispatcher role calling appropriate functions from an array of those driver structures using current bus number as an index into that array. There is nothing more complex than that.
Perfectly.
The i2c_set_bus_number() is a bit more complex than just changing that global variable to accomodate multiplexed busses but not rocket science either -- if the current bus is multiplexed, it switches off all the muxes in the path starting with the farthest one (if it is multihop, but I doubt we'll see such a beast so it will be just one chip) and turn on the muxes for the new bus (if it is multiplexed) starting with the closest one. The companion i2c_get_bus_num() just returns that global variable.
Ok.
What is that complex with such a design?
Nothing. And there is not even more complexity, when we add a current pointer, or? We just can use this current pointer, to make driver code more simpler by using this current pointer and hwadapnr ...
See: http://lists.denx.de/pipermail/u-boot/2009-February/047487.html
bye Heiko
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171456520.30777@home-gw.koi8.net you wrote:
[...]
OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.)
...
We agree that there are cases where multiple busses are needed. But do we need such complexity before relocation?
It is not before relocation. I don't see a reason to do this before relocation either. But if we want to have such a possibility after relocation we also need a mechanism to do this.
But, if this current pointer is writeable, it is also usable before relocation!
Once again -- it is not the pointer itself that should be writeable, it is what it is pointing to.
And it is not all that complex, I can't understand why you think it is.
We actually mix things:
- complex: There we (Wolfgang and I) talk about your implentation of the bitbang driver
not about your i2c-core ...
OK, for bitbang driver it is just a source file size reduction. We can simply duplicate (triplicate etc.) that file for more than one adapter. What I did makes CPP make that duplication instead of us. But I can simply do it manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the resulting object code will remain exactly the same, byte-to-byte.
The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we should have 2 such files. If we want 4 adapters there should be 4 such files. It is that simple.
As for now we have a set of regular i2c functions (i2c_init/i2c_read/etc.) in each and every i2c driver. Those functions are exported so when we pick up a driver (with e.g. CONFIG_HARD_I2C that metastasize through the entire U-Boot source choosing different drivers for different platforms) those functions get called where i2c_read() etc. are used.
I make all those functions static so they are not exported and add a simple exported structure with pointers to those functions. Then there is one wrapper, i2c_core.c that holds global bus number and exports that usual set of i2c_read() and friends. Those functions in turn just play a dispatcher role calling appropriate functions from an array of those driver structures using current bus number as an index into that array. There is nothing more complex than that.
Perfectly.
The i2c_set_bus_number() is a bit more complex than just changing that global variable to accomodate multiplexed busses but not rocket science either -- if the current bus is multiplexed, it switches off all the muxes in the path starting with the farthest one (if it is multihop, but I doubt we'll see such a beast so it will be just one chip) and turn on the muxes for the new bus (if it is multiplexed) starting with the closest one. The companion i2c_get_bus_num() just returns that global variable.
Ok.
What is that complex with such a design?
Nothing. And there is not even more complexity, when we add a current pointer, or? We just can use this current pointer, to make driver code more simpler by using this current pointer and hwadapnr ...
It will NOT make code any simpler. And it will NOT make it any smaller.
For the functions that can be parameterized I'm adding a simple wrapper function that simply passes additional argument to that parameterizable function and calls it. That is in no way worse, or bigger, or more complex than accessing some pointer, then getting adapter number, then branching to the appropriate function. The wrapper is simpler and smaller. Codewise it is even smaller if all arguments fit into registers because when you are preparing that actual function call from a wrapper function ALL arguments are already loaded into appropriate registers; you only have to load one additional register with an immediate constant (channel number) and perform a branch.
Your current pointer adds additional member to the adapter structure and lot of complexity for such an easy task.
Look what I do:
=== Cut === static int _i2c_probe(chip, adap_no) { ... }
static int xx_i2c_probe1(chip) { return _i2c_probe(chip, 0); }
static int xx_i2c_probe2(chip) { return _i2c_probe(chip, 1); } === Cut ===
Is it any bigger or more complex than using additional structure members, accessing those structure and doing other work? And it does NOT require anything to be writeable to call a function from appropriate adapter.
What complexity are you talking about, guys?
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902180945100.5002@home-gw.koi8.net you wrote:
OK, for bitbang driver it is just a source file size reduction. We can simply duplicate (triplicate etc.) that file for more than one adapter. What I did makes CPP make that duplication instead of us. But I can simply do it manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the resulting object code will remain exactly the same, byte-to-byte.
The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we should have 2 such files. If we want 4 adapters there should be 4 such files. It is that simple.
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
For the functions that can be parameterized I'm adding a simple wrapper function that simply passes additional argument to that parameterizable function and calls it. That is in no way worse, or bigger, or more complex than accessing some pointer, then getting adapter number, then branching to the appropriate function. The wrapper is simpler and smaller. Codewise it is even smaller if all arguments fit into registers because when you are preparing that actual function call from a wrapper function ALL arguments are already loaded into appropriate registers; you only have to load one additional register with an immediate constant (channel number) and perform a branch.
Is this theory or did you perform code size measurements?
Look what I do:
=== Cut === static int _i2c_probe(chip, adap_no) { ... }
static int xx_i2c_probe1(chip) { return _i2c_probe(chip, 0); }
static int xx_i2c_probe2(chip) { return _i2c_probe(chip, 1); } === Cut ===
Looks like overhead to me.
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902180945100.5002@home-gw.koi8.net you wrote:
OK, for bitbang driver it is just a source file size reduction. We can simply duplicate (triplicate etc.) that file for more than one adapter. What I did makes CPP make that duplication instead of us. But I can simply do it manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the resulting object code will remain exactly the same, byte-to-byte.
The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we should have 2 such files. If we want 4 adapters there should be 4 such files. It is that simple.
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only the _NAME_ is the same.
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a source from set of macros that come from an external file (in our case it is board config file.) And that is a beauty of this driver -- one doesn't have to write his own driver for each and every board; he just defines a set of basic operations and CPP takes it from there.
And it can _NOT_ scale because it is impossible to make a generic driver _SOURCE_ for each and every hardware configuration imaginable. That existing soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem with it is it only makes _ONE_ such interface.
And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for different adapters. We just creating several _INSTANCES_ using that template with _DIFFERENT_ parameters. And those instances are all different. The template itself does _NOT_ go into the final code.
For the functions that can be parameterized I'm adding a simple wrapper function that simply passes additional argument to that parameterizable function and calls it. That is in no way worse, or bigger, or more complex than accessing some pointer, then getting adapter number, then branching to the appropriate function. The wrapper is simpler and smaller. Codewise it is even smaller if all arguments fit into registers because when you are preparing that actual function call from a wrapper function ALL arguments are already loaded into appropriate registers; you only have to load one additional register with an immediate constant (channel number) and perform a branch.
Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference here because it is miniscule -- how many I2C adapters do we have on a board?
Look what I do:
=== Cut === static int _i2c_probe(chip, adap_no) { ... }
static int xx_i2c_probe1(chip) { return _i2c_probe(chip, 0); }
static int xx_i2c_probe2(chip) { return _i2c_probe(chip, 1); } === Cut ===
Looks like overhead to me.
Sure it is. There is no way how you can avoid overhead without writing a totally custom U-Boot for each and every board. But it is no worse than this:
=== Cut === static int _i2c_probe(chip) { adap_no = ADAP(i2c_cur_bus)->hwadap_no; .... }
xx_i2c_probe1 = xx_i2c_probe2 = _i2c_probe; === Cut ===
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
and there is no Catch 22 with changing current bus number to initialize adapter that requires that adapter must be initialized to change the current bus number.
As a rule of thumb, member functions should _NEVER_ use any global variables. It is up to higher layer to work on those if they are required.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181112400.5002@home-gw.koi8.net you wrote:
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only the _NAME_ is the same.
This is one way to implement it, but not the only one.
Just to give an example of a different implementation (without claiming that that would be a better one): we could provide an array of functions (instead of macros) for each such adapter, so we could use just a single instance of the driver which takes the address of the respective array as argument.
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
And it can _NOT_ scale because it is impossible to make a generic driver _SOURCE_ for each and every hardware configuration imaginable. That existing
Impossible. Famous last words.
soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem with it is it only makes _ONE_ such interface.
And it duplicates the source code for each additional instance that is needed.
And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for different adapters. We just creating several _INSTANCES_ using that template with _DIFFERENT_ parameters. And those instances are all different. The template itself does _NOT_ go into the final code.
Call it what you want, I call it duplication of code.
Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference here because it is miniscule -- how many I2C adapters do we have on a board?
It is obvious. Famous last words again.
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
Most probably we never *want* to initialize all adapters...
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181112400.5002@home-gw.koi8.net you wrote:
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only the _NAME_ is the same.
This is one way to implement it, but not the only one.
Just to give an example of a different implementation (without claiming that that would be a better one): we could provide an array of functions (instead of macros) for each such adapter, so we could use just a single instance of the driver which takes the address of the respective array as argument.
Yes, it is possible, but it is not the best approach. Most of those macros translate in 2 to 3 words. If you use functions you should add at very least 2 more words to it, one for a function call another for a function return. That is without array, just directly linked functions. If you are to use an array it adds another 2 words -- one for the array element (the pointer) itself and another one for the pointer to that array in i2c function code.
Linux uses functions for that but they are _INLINE_ ones in headers included into the driver file i.e. they are essentially macros.
That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose?
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it.
And, BTW, you can see something very similar in Linux kernel (i2c-algo-bit.c.)
I'm open to any alternative possibilities but I can not see anything better. That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle that was there since the start of times so I can't see any reason to throw it away and reinvent the wheel.
And it can _NOT_ scale because it is impossible to make a generic driver _SOURCE_ for each and every hardware configuration imaginable. That existing
Impossible. Famous last words.
Well, it looks like it is you are Russian, not me :) It is well known Russian passion to scrap everything and design a Universal Server Of Everything from scratch :) I'm trying to avoid that...
soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem with it is it only makes _ONE_ such interface.
And it duplicates the source code for each additional instance that is needed.
And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for different adapters. We just creating several _INSTANCES_ using that template with _DIFFERENT_ parameters. And those instances are all different. The template itself does _NOT_ go into the final code.
Call it what you want, I call it duplication of code.
It might be a duplication of SOURCE TEXT, but not CODE.
Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference here because it is miniscule -- how many I2C adapters do we have on a board?
It is obvious. Famous last words again.
Eh, do you think anyone really has time to make such comparisons to find out that size difference is zero?
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
Most probably we never *want* to initialize all adapters...
It is easier that way and wouldn't do any harm in most cases...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181434310.5729@home-gw.koi8.net you wrote:
Yes, it is possible, but it is not the best approach. Most of those macros
...
That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose?
Define "decent". It seems we have a different opintion abot such a definition.
And define "much worse", please. How much wors is this (in bytes) for a single adapter, and for 2, 3, or 5?
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it.
Yes, and this is what I'm complaining about. The whole idea behind software engineering is not to copy code.
Well, it looks like it is you are Russian, not me :) It is well known
Maybe I am; kind of, at least :-)
Russian passion to scrap everything and design a Universal Server Of Everything from scratch :) I'm trying to avoid that...
Me too.
Call it what you want, I call it duplication of code.
It might be a duplication of SOURCE TEXT, but not CODE.
Source text compiles to object code, doesn't it?
Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference here because it is miniscule -- how many I2C adapters do we have on a board?
It is obvious. Famous last words again.
Eh, do you think anyone really has time to make such comparisons to find out that size difference is zero?
You have the code base ready available and can compile and measure it, can't you?
If you base your argumentation on such claims, you better have prove for it.
Best regards,
Wolfgang Denk
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181434310.5729@home-gw.koi8.net you wrote:
Yes, it is possible, but it is not the best approach. Most of those macros
...
That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose?
Define "decent". It seems we have a different opintion abot such a definition.
And define "much worse", please. How much wors is this (in bytes) for a single adapter, and for 2, 3, or 5?
I already gave an estimate. It is at very least not better. It doesn't give any benefits and introduces some problems. Yes, there is already a tradeof and all our weaknesses are just an extension of our advantages but in this case there is no advantages...
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it.
Yes, and this is what I'm complaining about. The whole idea behind software engineering is not to copy code.
Well, it looks like it is you are Russian, not me :) It is well known
Maybe I am; kind of, at least :-)
:)
Russian passion to scrap everything and design a Universal Server Of Everything from scratch :) I'm trying to avoid that...
Me too.
Call it what you want, I call it duplication of code.
It might be a duplication of SOURCE TEXT, but not CODE.
Source text compiles to object code, doesn't it?
Not always. E.g. defines never compile to object code. Function declarations don't compile to object code. And so on...
Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference here because it is miniscule -- how many I2C adapters do we have on a board?
It is obvious. Famous last words again.
Eh, do you think anyone really has time to make such comparisons to find out that size difference is zero?
You have the code base ready available and can compile and measure it, can't you?
If you base your argumentation on such claims, you better have prove for it.
There is a threshhold. Yes, I should've proved it if it had been very different. But in this case the difference is minimal and not worth an effort.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181112400.5002@home-gw.koi8.net you wrote:
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only the _NAME_ is the same.
This is one way to implement it, but not the only one.
Just to give an example of a different implementation (without claiming that that would be a better one): we could provide an array of functions (instead of macros) for each such adapter, so we could use just a single instance of the driver which takes the address of the respective array as argument.
Yes, it is possible, but it is not the best approach. Most of those macros translate in 2 to 3 words. If you use functions you should add at very least 2 more words to it, one for a function call another for a function return. That is without array, just directly linked functions. If you are to use an array it adds another 2 words -- one for the array element (the pointer) itself and another one for the pointer to that array in i2c function code.
Linux uses functions for that but they are _INLINE_ ones in headers included into the driver file i.e. they are essentially macros.
From which Linux and which driver you speak?
If you use from actual 2.6 Linux the drivers/i2c/algos/i2c-algo-bit.c bitbang driver with gpio support (drivers/i2c/busses/i2c-gpio.c) you get called your gpio_get/set_value (pin, state) function in your board code (or gpio code)!
And in this function you have to switch dependent on the pin, and then again to switch dependent on the state, before you can do any action ... so tell me, where we are worser, when we making
#define I2C_SDA(bit) \ switch(cur_adap_nr->hw_adapnr) { \ case 0: \ if (bit) { \ /* set pin for adapter 0 = 1*/ \ } else { \ /* set pin for adapter 0 = 0*/ \ } \ [...] \ } \ } \
in board config file ... OK, we are worser against your approach, because we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think this is such a problem.
That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose?
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it.
And, BTW, you can see something very similar in Linux kernel (i2c-algo-bit.c.)
Please have a deeper look in it, see above comment.
I'm open to any alternative possibilities but I can not see anything better. That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle that was there since the start of times so I can't see any reason to throw it away and reinvent the wheel.
Nobody wants to throw it away!
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
Most probably we never *want* to initialize all adapters...
It is easier that way and wouldn't do any harm in most cases...
But it is not needed when doing this in i2c_set_bus_num() !! It is sufficent to init a hardwareadapter, when switching to it.
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181112400.5002@home-gw.koi8.net you wrote:
Duplicating the source code (and thus the object code, too) to create additional instances of basicly the same driver seems to be the wrong approach to me.
It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only the _NAME_ is the same.
This is one way to implement it, but not the only one.
Just to give an example of a different implementation (without claiming that that would be a better one): we could provide an array of functions (instead of macros) for each such adapter, so we could use just a single instance of the driver which takes the address of the respective array as argument.
Yes, it is possible, but it is not the best approach. Most of those macros translate in 2 to 3 words. If you use functions you should add at very least 2 more words to it, one for a function call another for a function return. That is without array, just directly linked functions. If you are to use an array it adds another 2 words -- one for the array element (the pointer) itself and another one for the pointer to that array in i2c function code.
Linux uses functions for that but they are _INLINE_ ones in headers included into the driver file i.e. they are essentially macros.
From which Linux and which driver you speak?
If you use from actual 2.6 Linux the drivers/i2c/algos/i2c-algo-bit.c bitbang driver with gpio support (drivers/i2c/busses/i2c-gpio.c) you get called your gpio_get/set_value (pin, state) function in your board code (or gpio code)!
And in this function you have to switch dependent on the pin, and then again to switch dependent on the state, before you can do any action ... so tell me, where we are worser, when we making
#define I2C_SDA(bit) \ switch(cur_adap_nr->hw_adapnr) { \ case 0: \ if (bit) { \ /* set pin for adapter 0 = 1*/ \ } else { \ /* set pin for adapter 0 = 0*/ \ } \ [...] \ } \ } \
in board config file ... OK, we are worser against your approach, because we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think this is such a problem.
First of all, you are using an external global variable for object methods. That is a VERY BAD practice and I can't even imagine a use case that would justify this.
Second, what do you gain by commiting such a blasphemy?
That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose?
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
This is your implementation. It is not the only possible implemen- tation. Please try and open your mind to discuss alternative possibilities as well.
No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it.
And, BTW, you can see something very similar in Linux kernel (i2c-algo-bit.c.)
Please have a deeper look in it, see above comment.
I did.
I'm open to any alternative possibilities but I can not see anything better. That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle that was there since the start of times so I can't see any reason to throw it away and reinvent the wheel.
Nobody wants to throw it away!
You want...
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
Most probably we never *want* to initialize all adapters...
It is easier that way and wouldn't do any harm in most cases...
But it is not needed when doing this in i2c_set_bus_num() !! It is sufficent to init a hardwareadapter, when switching to it.
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
Sorry guys, I do not have THAT much free time that my employer would let me to spend on this.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191141090.18501@home-gw.koi8.net you wrote:
in board config file ... OK, we are worser against your approach, because we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think this is such a problem.
First of all, you are using an external global variable for object methods. That is a VERY BAD practice and I can't even imagine a use case that would justify this.
We are pretty pragmatic here. If it solves a problem efficiently, we use even global variables.
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
I cannot follow your argument.
Yes, the status quo is as you describe, it relies on i2c_init() and is simple-minded and does not support an arbitry number of arbitrarily complex I2C bus trees and multiplexors and expanders and what else. But it was sufficient for the first 10 years and 500 boards of U-Boot development.
Now we are discussion a major redesign, so what is the big problem of changing this part? "rewrite the entire U-Boot"? Please stay serious. Compared to the other changes you suggest, this is not that big a part.
Sorry guys, I do not have THAT much free time that my employer would let me to spend on this.
Well, you at least have some commercial motivation to spend time for this code and discussion, while for me it's all my "free" time (and I better don't tell you what my wife says of my interpretation "free" here).
Best regards,
Wolfgang Denk
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191141090.18501@home-gw.koi8.net you wrote:
in board config file ... OK, we are worser against your approach, because we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think this is such a problem.
First of all, you are using an external global variable for object methods. That is a VERY BAD practice and I can't even imagine a use case that would justify this.
We are pretty pragmatic here. If it solves a problem efficiently, we use even global variables.
But not in object methods... And it must serve some purpose. I'm a sinner myself and I have to confess to even using goto's sometimes but it must have some reason...
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
I cannot follow your argument.
Yes, the status quo is as you describe, it relies on i2c_init() and is simple-minded and does not support an arbitry number of arbitrarily complex I2C bus trees and multiplexors and expanders and what else. But it was sufficient for the first 10 years and 500 boards of U-Boot development.
Now we are discussion a major redesign, so what is the big problem of changing this part? "rewrite the entire U-Boot"? Please stay serious. Compared to the other changes you suggest, this is not that big a part.
No, my changes are limited. Look, somebody must initialize an adapter. As for now it is done with a single i2c_init() usually in libxxx/board.c. Then the entire code assumes adapter is already initialized and just issues i2c_read/write() as it see fits. 99% of this code is written on assumption that there is only one I2C bus so it doesn't use i2c_set_bus_num() or whatever, it just fires up i2c_read() and that's it.
This would perfectly work with my changes without modifying that code -- the only bus is bus number 0 so there is nothing wrong with not setting the bus for each I2C access; it is already at that only bus.
Now, if we have adapter initialization moved to i2c_set_bus() all that code will cease to work because i2c_set_bus() is never called thus adapter will never be initialized and all those i2c_read() and friends will fail.
That means that we should read through each and every board's code to find where i2c functions are used and add i2c_set_bus() calls as needed. That is not INSTEAD of that big rewrite, that is _IN ADDITION_ to it. That is a very sizeable chunk of additional changes.
I DID think of adding adapter initialization to i2c_set_bus() initially but then it turned out it generated more problems than it solved (and it solved none) so I dropped that idea.
Sorry guys, I do not have THAT much free time that my employer would let me to spend on this.
Well, you at least have some commercial motivation to spend time for this code and discussion, while for me it's all my "free" time (and I better don't tell you what my wife says of my interpretation "free" here).
Eh, you won't believe what constitutes my "free" time and for how long ahead that "free" time is planned out. I don't think mere mortals live that long... :)
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191141090.18501@home-gw.koi8.net you wrote:
[...]
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
I cannot follow your argument.
Yes, the status quo is as you describe, it relies on i2c_init() and is simple-minded and does not support an arbitry number of arbitrarily complex I2C bus trees and multiplexors and expanders and what else. But it was sufficient for the first 10 years and 500 boards of U-Boot development.
Now we are discussion a major redesign, so what is the big problem of changing this part? "rewrite the entire U-Boot"? Please stay serious. Compared to the other changes you suggest, this is not that big a part.
No, my changes are limited. Look, somebody must initialize an adapter. As for now it is done with a single i2c_init() usually in libxxx/board.c. Then the entire code assumes adapter is already initialized and just issues i2c_read/write() as it see fits. 99% of this code is written on assumption that there is only one I2C bus so it doesn't use i2c_set_bus_num() or whatever, it just fires up i2c_read() and that's it.
This would perfectly work with my changes without modifying that code -- the only bus is bus number 0 so there is nothing wrong with not setting the bus for each I2C access; it is already at that only bus.
Now, if we have adapter initialization moved to i2c_set_bus() all that code will cease to work because i2c_set_bus() is never called thus adapter will never be initialized and all those i2c_read() and friends will fail.
But you can call i2c_set_bus_num instead of i2c_init in lib_xxx/board.c and all old boards will work as they did. Ok, we didn;t can get rid of initializing bus 0, but with moving init() in i2c_set_bus_num, we only init additional hardwareadapters only, if we need them.
That means that we should read through each and every board's code to find where i2c functions are used and add i2c_set_bus() calls as needed. That is not INSTEAD of that big rewrite, that is _IN ADDITION_ to it. That is a very sizeable chunk of additional changes.
No, we have not to do this. See above. And if some board use multibus, yes they have to do a i2c_set_bus_num before they access the i2c bus, because they must know on which bus they have to read, and the actual bus could be changed.
I DID think of adding adapter initialization to i2c_set_bus() initially but then it turned out it generated more problems than it solved (and it solved none) so I dropped that idea.
Please tell us the problems, so we can think of it, maybe I overlook something. Thats the reason why we discuss this here.
bye Heiko
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191141090.18501@home-gw.koi8.net you wrote:
[...]
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
I cannot follow your argument.
Yes, the status quo is as you describe, it relies on i2c_init() and is simple-minded and does not support an arbitry number of arbitrarily complex I2C bus trees and multiplexors and expanders and what else. But it was sufficient for the first 10 years and 500 boards of U-Boot development.
Now we are discussion a major redesign, so what is the big problem of changing this part? "rewrite the entire U-Boot"? Please stay serious. Compared to the other changes you suggest, this is not that big a part.
I think so too.
bye Heiko
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
In message Pine.LNX.4.64ksi.0902181112400.5002@home-gw.koi8.net you wrote:
[...]
I'm open to any alternative possibilities but I can not see anything better. That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle that was there since the start of times so I can't see any reason to throw it away and reinvent the wheel.
Nobody wants to throw it away!
You want...
No, we don;t want to throw away soft-i2c.c and make it new. Nobody said this. We just discus your multibus support for it.
The former does not require additional adapter struct member, hwadap_no. And, unlike the latter, it is self-contained, it doesn't require any external global variable to decide what to do. One can initialize all adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++) i2c_adap[i]->init(...);
Most probably we never *want* to initialize all adapters...
It is easier that way and wouldn't do any harm in most cases...
But it is not needed when doing this in i2c_set_bus_num() !! It is sufficent to init a hardwareadapter, when switching to it.
That means you'll have to rewrite the entire U-Boot. 99% of the boards have only one bus so they did not switch busses. That means they never called that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.
But we can call i2c_set_bus_num instead of i2c_init() right?
Sorry guys, I do not have THAT much free time that my employer would let me to spend on this.
Hey, this is "my" free time. I don;t get money for this discussion.
bye Heiko
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
They use it before relocation, because the DTTs are read before relocation. But this is another approach. Actually, the "way" to the DTTs is in an environment variable. When running from flash, it get directly parsed, if running from RAM, the var gets analyzed and this "new bus" is added with the "i2c bus" command resp. by calling the underlying C function.
bye Heiko
Dear Heiko Schocher,
In message 499BB9C6.6010602@denx.de you wrote:
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
Yes, it is possible, but then - ther eis no need for it.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
They use it before relocation, because the DTTs are read before relocation.
I am not aware that any piece of code in the init sequence makes use of the information read from the DTT's, so why is this performaned before relocation?
More - why is this performed at all for each reset cycle? Normally we should not even initialize interfaces that are nt used for U-Boot's own operation.
I think the automatic DTT checking should be dropped.
Best regards,
Wolfgang Denk
Hello Wolfgang,
Wolfgang Denk wrote:
Dear Heiko Schocher,
In message 499BB9C6.6010602@denx.de you wrote:
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
Yes, it is possible, but then - ther eis no need for it.
For what is no need?
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
They use it before relocation, because the DTTs are read before relocation.
I am not aware that any piece of code in the init sequence makes use of the information read from the DTT's, so why is this performaned before relocation?
Thats I am asking me too.
More - why is this performed at all for each reset cycle? Normally we should not even initialize interfaces that are nt used for U-Boot's own operation.
I think the automatic DTT checking should be dropped.
Yes, I think so too. If nobody screams, I can make such a patch ... (This will solve another problem from me ... :-)
bye Heiko
Dear Heiko,
In message 499BC39C.9050509@denx.de you wrote:
But is is possible to use that code when running from flash, if this current pointer is writeable ...
Yes, it is possible, but then - ther eis no need for it.
For what is no need?
For dynamic reconfiguration of I2C busses before relocation.
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear Heiko,
In message 499BC39C.9050509@denx.de you wrote:
But is is possible to use that code when running from flash, if this current pointer is writeable ...
Yes, it is possible, but then - ther eis no need for it.
For what is no need?
For dynamic reconfiguration of I2C busses before relocation.
Yep, totally agree.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message 499BB9C6.6010602@denx.de you wrote:
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
Yes, it is possible, but then - ther eis no need for it.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
They use it before relocation, because the DTTs are read before relocation.
I am not aware that any piece of code in the init sequence makes use of the information read from the DTT's, so why is this performaned before relocation?
More - why is this performed at all for each reset cycle? Normally we should not even initialize interfaces that are nt used for U-Boot's own operation.
I think the automatic DTT checking should be dropped.
Exactly.
At least it must be postponed until relocation is done. There is nothing required for relocation in DTT.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
It is not the pointer that must be writeable, it's what it is pointing to...
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, it is, because none of them needs any such switching before relocation. And switching is really simple so far.
They use it before relocation, because the DTTs are read before relocation. But this is another approach. Actually, the "way" to the DTTs is in an environment variable. When running from flash, it get directly parsed, if running from RAM, the var gets analyzed and this "new bus" is added with the "i2c bus" command resp. by calling the underlying C function.
I don't think there is any need to access DTT before relocation. The only device that absolutely must be read is SPD EEPROM[s].
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
It is not the pointer that must be writeable, it's what it is pointing to...
But in your approach this is also not writeable! So we lost nothing, when using such a pointer.
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
It is not the pointer that must be writeable, it's what it is pointing to...
But in your approach this is also not writeable! So we lost nothing, when using such a pointer.
No, we did NOT. I can still cal adap[N]->init() and it will init the proper adapter. It does NOT require any global variables for it, it is self-sufficient.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
> What makes you insist that we cannot change a variable if we need to > be able to change one? It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches, then do that i2c_set_current_bus() and connect the switches to the new bus after that.
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
It is not the pointer that must be writeable, it's what it is pointing to...
But in your approach this is also not writeable! So we lost nothing, when using such a pointer.
No, we did NOT. I can still cal adap[N]->init() and it will init the proper
For what you will do this, when you can;t use the adapter, when running from flash? See Later, why you cannot use it.
adapter. It does NOT require any global variables for it, it is self-sufficient.
But you could not switch busses, nor work with it, first because i2c_set_bus_num() don;t work for you and i2c_cur_bus is not writeable, so _all_ accesses with the API in i2c-core.c like for example
int i2c_read(u_int8_t chip, unsigned int addr, int alen, u_int8_t *buffer, int len) { return(ADAP(i2c_cur_bus)->read(chip, addr, alen, buffer, len)); }
_will_ fail, when running from flash, because i2c_cur_bus is not writeable!
So why will you initialize all adapters when running from flash?
But this is no problem, when we make i2c_cur_bus or this pointer I would like to see, writeable. And think about my proposal for i2c_set_bus_num() and we have there also no problem with calling init() for an adapter.
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902171233390.30435@home-gw.koi8.net you wrote:
[...]
>> What makes you insist that we cannot change a variable if we need to >> be able to change one? > It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And > number of busses can be bigger than number of adapters (e.g. when some > busses a reached via muxes or switches.) When doing i2c_set_current_bus() > you are switching _NOT_ adapters, but busses. That involves not only > changing that global variable but also reprogramming muxes/switches for > i2c_set_current_bus() to be consistent and hardware independent. Otherwise > your code should know if that particular bus it is switching to is directly > connected or switched and check the bus it is switching from for muxes. If > they are switched, your code should disconnect the current bus switches, > then do that i2c_set_current_bus() and connect the switches to the new bus > after that. > > That means that code MUST somehow know the topology to take appropriate > actions and properly configure those switches. That means you should somehow > describe that topology for each and every board in CONFIG_* terms and make > each and every place at U-Boot that invokes _ANY_ i2c function to take care > of that switching. You convinced me. This code must not be used before relocation to RAM, then.
But is is possible to use that code when running from flash, if this current pointer is writeable ...
It is not the pointer that must be writeable, it's what it is pointing to...
But in your approach this is also not writeable! So we lost nothing, when using such a pointer.
No, we did NOT. I can still cal adap[N]->init() and it will init the proper
For what you will do this, when you can;t use the adapter, when running from flash? See Later, why you cannot use it.
Using multiple adapters while running from flash is an exception. I can let myself call adapter-specific functions directly if needed, without changing busses.
adapter. It does NOT require any global variables for it, it is self-sufficient.
But you could not switch busses, nor work with it, first because i2c_set_bus_num() don;t work for you and i2c_cur_bus is not writeable, so _all_ accesses with the API in i2c-core.c like for example
int i2c_read(u_int8_t chip, unsigned int addr, int alen, u_int8_t *buffer, int len) { return(ADAP(i2c_cur_bus)->read(chip, addr, alen, buffer, len)); }
_will_ fail, when running from flash, because i2c_cur_bus is not writeable!
So why will you initialize all adapters when running from flash?
But this is no problem, when we make i2c_cur_bus or this pointer I would like to see, writeable. And think about my proposal for i2c_set_bus_num() and we have there also no problem with calling init() for an adapter.
I'm against using global variables at all. And I will never ever use them in object's member functions, that's a blasphemy.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191149360.18501@home-gw.koi8.net you wrote:
For what you will do this, when you can;t use the adapter, when running from flash? See Later, why you cannot use it.
Using multiple adapters while running from flash is an exception. I can let myself call adapter-specific functions directly if needed, without changing busses.
Maybe we all can save some time here?
For the following discussion, please let's assume that we have reached an agreement that we do NOT support multiple adapters while running from flash.
OK? Thanks.
Best regards,
Wolfgang Denk
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191149360.18501@home-gw.koi8.net you wrote:
For what you will do this, when you can;t use the adapter, when running from flash? See Later, why you cannot use it.
Using multiple adapters while running from flash is an exception. I can let myself call adapter-specific functions directly if needed, without changing busses.
Maybe we all can save some time here?
For the following discussion, please let's assume that we have reached an agreement that we do NOT support multiple adapters while running from flash.
OK? Thanks.
As a matter of fact I wasn't going to support it either :) That bus_num variable is not writable before relocation and I deliberately made i2c_set_bus() fail when running from flash.
But if somebody wants to use several adapters it can do it by calling adapter methods directly. This provides necessary means for a simple hack if a board or two would require such access thus saving us from extensive and complex hack. But those are clear exceptions and they are NOT covered by regular API.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello Wolfgang,
Wolfgang Denk wrote:
In message Pine.LNX.4.64ksi.0902191149360.18501@home-gw.koi8.net you wrote:
For what you will do this, when you can;t use the adapter, when running from flash? See Later, why you cannot use it.
Using multiple adapters while running from flash is an exception. I can let myself call adapter-specific functions directly if needed, without changing busses.
Maybe we all can save some time here?
For the following discussion, please let's assume that we have reached an agreement that we do NOT support multiple adapters while running from flash.
OK? Thanks.
Ok.
bye Heiko
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Not rocket science and not much of black magic, just simple and straightforward token pasting...
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, and they add i2c busses frem env variables, which you dropped ...
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
bye Heiko
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_ parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for different adapters. There is _NOTHING_ common between them.
In my case you simply make N i2c_write() functions for N adapters and assign pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you still have that same code size _PLUS_ switch. Than, you assign a pointer to that monstrous function to i2c_write() member of _ALL_ adapter structures so a call to i2c_write() ends up calling that function. Now you have to somehow find out which switch case to execute. For that you need an additional global variable and additional member in each adapter struct. And those must be writeable because you don't have any means of executing something like "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M) because in your case that "N" in "adap[N]" has absolutely no effect...
Please explain how it is better than simple and straightforward function per adapter? Which one is more complex?
It looks like I simply don't understand something. You must've meant something else that I didn't get because the above comparison is SO obvious that it is almost impossible to somehow misunderstand it...
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Source file preprocessing.
Not rocket science and not much of black magic, just simple and straightforward token pasting...
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
Make several of those for several drivers if it looks easier. Multiply it.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
Please read above.
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
No, it is _NOT_.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
The problem is you can _NOT_ change the bus if adapter is not initialized and you can _NOT_ initialize adapter because you _MUST_ change the bus to do this. No matter running from RAM or from ROM, you have exactly the same Catch 22.
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds actual source code from set of external defines. One more time -- it is _ALREADY_ a template.
I did nothing to that file, just added an option of generating several drivers instead of one. There is absolutely nothing I changed in that driver per se, that is _EXACTLY_ the same code.
You can make N such files for N adapters, or you can multiply those functions in that file N times to make N adapters. I'm doing the latter and nothing else.
What are you trying to prevent? I simply don't understand. Do you want me to just make several sets of functions in soft_i2c.c file because those defines look scary? No problems, just say it and I will do that. But there is no need for reinventing a wheel or adding sophisticated crutches to the obvious...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
We'll return to those dynamic busses later. I personally can't see any viable reason for that.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
It solves Catch 22.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
You can _NOT_. Please read above.
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, and they add i2c busses frem env variables, which you dropped ...
Again, I can't see any reason for such a feature. If you want to attach something to the already running board and do some accesses you can easily do it with existing commands. If you want to use it for U-Boot itself, then it does NOT belong to the environment; it belongs to board config file.
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
We do already have current bus. There is absolutely no need for such a pointer. Occam's razor.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_ parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for different adapters. There is _NOTHING_ common between them.
How SDA, SCL are get/set is common, just how SDA and SCL are accessed is different! So there is no need for different i2c_write(), ... only SDA,SCL accessors are different.
In my case you simply make N i2c_write() functions for N adapters and assign
Thats not needed.
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
still have that same code size _PLUS_ switch. Than, you assign a pointer to that monstrous function to i2c_write() member of _ALL_ adapter structures so a call to i2c_write() ends up calling that function. Now you have to somehow find out which switch case to execute. For that you need an additional global variable and additional member in each adapter struct. And those must
No problem with this.
be writeable because you don't have any means of executing something like "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M) because in your case that "N" in "adap[N]" has absolutely no effect...
? you again mixing thinks. With my approach no need for changing the API in i2c-core.c. I think, thats you don;t got.
Please explain how it is better than simple and straightforward function per adapter? Which one is more complex?
It looks like I simply don't understand something. You must've meant something else that I didn't get because the above comparison is SO obvious that it is almost impossible to somehow misunderstand it...
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Source file preprocessing.
I think you mean gcc, right?
Not rocket science and not much of black magic, just simple and straightforward token pasting...
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
Make several of those for several drivers if it looks easier. Multiply it.
No need for it.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
Please read above.
So you to ;-)
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
No, it is _NOT_.
It is.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
It is.
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
The problem is you can _NOT_ change the bus if adapter is not initialized and you can _NOT_ initialize adapter because you _MUST_ change the bus to do this. No matter running from RAM or from ROM, you have exactly the same Catch 22.
You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number() will not work from flash, so how works this for you?
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds actual source code from set of external defines. One more time -- it is _ALREADY_ a template.
Yes, I know! But different adapters just differnet in how they access SDA, SCL not in i2c_write,..
I did nothing to that file, just added an option of generating several drivers instead of one. There is absolutely nothing I changed in that driver per se, that is _EXACTLY_ the same code.
You can make N such files for N adapters, or you can multiply those functions in that file N times to make N adapters. I'm doing the latter and nothing else.
What are you trying to prevent? I simply don't understand. Do you want me to just make several sets of functions in soft_i2c.c file because those defines look scary? No problems, just say it and I will do that. But there is no need for reinventing a wheel or adding sophisticated crutches to the obvious...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
We'll return to those dynamic busses later. I personally can't see any viable reason for that.
Maybe you not, but I told you, that there is a hardware manufacturer, that has his DTTs, EEProms,... on different hardware on different I2C busses. With this dynamic busses, he can have one image for all his boardvariants, and this is a need.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
It solves Catch 22.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
You can _NOT_. Please read above.
Why?
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, and they add i2c busses frem env variables, which you dropped ...
Again, I can't see any reason for such a feature. If you want to attach something to the already running board and do some accesses you can easily do it with existing commands. If you want to use it for U-Boot itself, then it does NOT belong to the environment; it belongs to board config file.
I think, you don;t read my EMails ...
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
We do already have current bus. There is absolutely no need for such a pointer. Occam's razor.
OK, do we not make a pointer, just an integer, but it is the same problem, the integer must also be writeable! How would you change busses when running in flash?
(I prefer to have such a pointer)
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
That means you have to make changes in two places instead of one -- config file AND $(BOARD).c. Also you use functions instead of macros and you can NOT make them inline because they come from a separate object file. This essentially defeats the very purpose of that common soft_i2c.c driver. If you want to make functions for bitbanged I2C into the $(BOARD).c there is no reason to have them as a base for that driver. It is much more logical to do everything in reverse, i.e. instead of having soft_i2c.c as a bona fide drivers and those I2C_SDA and friends as its building blocks make those i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and build the actual driver in the $(BOARD).c itself. Just convert that soft_i2c.c into a header file with macros for real functions (soft_i2c_read etc.) and instantiate them in the $(BOARD).c.
Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_ parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for different adapters. There is _NOTHING_ common between them.
How SDA, SCL are get/set is common, just how SDA and SCL are accessed is different! So there is no need for different i2c_write(), ... only SDA,SCL accessors are different.
Argh... Do you understand that those send_start etc. are _NOT_ the functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Template is _NOT_ a code, it is a _TOOL_ that generates code.
In my case you simply make N i2c_write() functions for N adapters and assign
Thats not needed.
Hm... Please explain how are you going to use 2 different sets of pins with different access methods with one function?
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
And what are the advantages?
still have that same code size _PLUS_ switch. Than, you assign a pointer to that monstrous function to i2c_write() member of _ALL_ adapter structures so a call to i2c_write() ends up calling that function. Now you have to somehow find out which switch case to execute. For that you need an additional global variable and additional member in each adapter struct. And those must
No problem with this.
There IS a problem First, that means you have _ABSOLUTELY_ no way to access any other adapter than default one until that variable made writable.
Second, all class functions must be self-sufficient, they should NOT rely on some external global variable to work. That might sound C++ese but no matter how I don't like C++ it does many things right.
be writeable because you don't have any means of executing something like "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M) because in your case that "N" in "adap[N]" has absolutely no effect...
? you again mixing thinks. With my approach no need for changing the API in i2c-core.c. I think, thats you don;t got.
No, it is you who didn't get it. As we russians use to say, the bride is very good, just a little bit pregnant. You want to use a global variable in I2C object member functions. Global variables are _EVIL_ by theirself and should be only used as last resort, but in member functions they are not just plain evil, they are absolute NO-NO.
Please explain how it is better than simple and straightforward function per adapter? Which one is more complex?
It looks like I simply don't understand something. You must've meant something else that I didn't get because the above comparison is SO obvious that it is almost impossible to somehow misunderstand it...
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Source file preprocessing.
I think you mean gcc, right?
No, I mean CPP. GCC is a frontend for cpp, as, ld etc. It does NOT preprocess files, cpp does.
Not rocket science and not much of black magic, just simple and straightforward token pasting...
The only problem with that is it breaks uniformity and makes another mess. The whole idea was to bring _ALL_ I2C drivers to a single place and make them totally transparent and uniform. Something like e.g. Linux VFS.
This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
Make several of those for several drivers if it looks easier. Multiply it.
No need for it.
Please show me the code.
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
Please read above.
So you to ;-)
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
No, it is _NOT_.
It is.
That means you have another design for that. Please explain it.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
It is.
Please show your code.
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
The problem is you can _NOT_ change the bus if adapter is not initialized and you can _NOT_ initialize adapter because you _MUST_ change the bus to do this. No matter running from RAM or from ROM, you have exactly the same Catch 22.
You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number() will not work from flash, so how works this for you?
I can call a member function for any adapter directly if needed with adap[N]->function(). You can NOT because in your case that "N" does not have any effect and your functions rely on an external global variable that you can not change.
BTW, in my design i2c_cur_bus variable is NOT global. It has file scope.
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds actual source code from set of external defines. One more time -- it is _ALREADY_ a template.
Yes, I know! But different adapters just differnet in how they access SDA, SCL not in i2c_write,..
That i2c_write uses a bunch of helper functions that are GENERATED from I2C_SDA and other macros. All those helper functions are different for different pin sets. And there is no compilable source code for them in soft_i2c.c (_EXISTING_ one, not just new,) only TEMPLATEs. Function is something you can get an address for. Templates do NOT have such address. You can NOT make a pointer to a template.
I did nothing to that file, just added an option of generating several drivers instead of one. There is absolutely nothing I changed in that driver per se, that is _EXACTLY_ the same code.
You can make N such files for N adapters, or you can multiply those functions in that file N times to make N adapters. I'm doing the latter and nothing else.
What are you trying to prevent? I simply don't understand. Do you want me to just make several sets of functions in soft_i2c.c file because those defines look scary? No problems, just say it and I will do that. But there is no need for reinventing a wheel or adding sophisticated crutches to the obvious...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
We'll return to those dynamic busses later. I personally can't see any viable reason for that.
Maybe you not, but I told you, that there is a hardware manufacturer, that has his DTTs, EEProms,... on different hardware on different I2C busses. With this dynamic busses, he can have one image for all his boardvariants, and this is a need.
That's a separate issue. I can offer N other ways to achieve this other than using that horror. And if they use the same binary image for different boards those boards are not different. If they put a chip on different bus that means the hardware is different. Different hardware means quite a lengthy process involving changes to the schematics, then re-layout, new set of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort definitely deserves a separate config file and simple repompile of U-Boot that takes less than a minute time.
But again, that is a separate issue. We do NOT have anything yet, just a cloned repository with no new source.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
It solves Catch 22.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
You can _NOT_. Please read above.
Why?
Because you use an external global variable to choose which adapter's function to call and that variable is not writable. You can NOT use adap[N]->function() because in your case that N does not have any effect.
And yes, we DO have some boards with switched I2C busses in U-Boot main tree so this is NOT a hypothetical situation.
Yes, and they add i2c busses frem env variables, which you dropped ...
Again, I can't see any reason for such a feature. If you want to attach something to the already running board and do some accesses you can easily do it with existing commands. If you want to use it for U-Boot itself, then it does NOT belong to the environment; it belongs to board config file.
I think, you don;t read my EMails ...
I don't know what is the exact reason for such weird approach. Also those EPROMs must be preprogrammed to be used for bus topology and that is more expensive option than using a different U-Boot image.
You are adding unnecessary complexity to the code. And you break uniformity.
He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
We do already have current bus. There is absolutely no need for such a pointer. Occam's razor.
OK, do we not make a pointer, just an integer, but it is the same problem, the integer must also be writeable! How would you change busses when running in flash?
I'm not going to change busses when I'm running from flash. But I can call functions on any adapter I want if needed.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191024560.18501@home-gw.koi8.net you wrote:
Argh... Do you understand that those send_start etc. are _NOT_ the functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Please, please: calm down.
I think there is no reason to claim that Heiko did not understand this.
On the other side, I, too, feel that you should (just for the theoretical possibility of another, probably completey stupid and braindead and whatever, but still another approach to solve the same set of problems) try to follow Heiko's thoughts and see where that would take us?
If you continue to perseverate on the position that your code is the only possible solution, we might as well stop the discussion here.
Hm... Please explain how are you going to use 2 different sets of pins with different access methods with one function?
Been there before. See for example http://lists.denx.de/pipermail/u-boot/2009-February/047937.html
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
No, of course not.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
That seams a _terrible_ overhead to me: one register load per call. How fast is that I2C bus? Well below 50 kByte per second, right?
Please stay serious.
No problem with this.
There IS a problem First, that means you have _ABSOLUTELY_ no way to access any other adapter than default one until that variable made writable.
Please let's stop considering this as a problem.
Let's accept that this is a self-imposed and accepted restriction.
Second, all class functions must be self-sufficient, they should NOT rely on some external global variable to work. That might sound C++ese but no matter how I don't like C++ it does many things right.
We don't use C++ here, and we are not that strict if it allows for smaller code or solves other problems.
I can call a member function for any adapter directly if needed with adap[N]->function(). You can NOT because in your case that "N" does not have any effect and your functions rely on an external global variable that you can not change.
Regards from the rat race. I am really tired of that.
It makes no sense to continue this discussion with you when you are not willing to even discuss the possibility of an alternative implementation that allows for a (writable!) global variable, and then you continue claiming we could not use writable variables (especially when we will not even require it because we don;t need bus switching before relocation).
There are better things I can do in my spare time.
That's a separate issue. I can offer N other ways to achieve this other than using that horror. And if they use the same binary image for different boards those boards are not different. If they put a chip on different bus that means the hardware is different. Different hardware means quite a lengthy process involving changes to the schematics, then re-layout, new set of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort definitely deserves a separate config file and simple repompile of U-Boot that takes less than a minute time.
Do you know which effort is involved in maintaining different software versions in a big project? It is a BIG win if you can use the same binary image on many differen board configurations.
And who tells you that that allthe I2C busses and devices are on one board? There is things like backplanes that connect boards, where different boards in differnt configfurations may be inserted or removed and ... and ...
Please get a clue before making such statements!
Best regards,
Wolfgang Denk
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902191024560.18501@home-gw.koi8.net you wrote:
Argh... Do you understand that those send_start etc. are _NOT_ the functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Please, please: calm down.
I think there is no reason to claim that Heiko did not understand this.
Yeah, that was a bit too far from my side, my apologies for that.
On the other side, I, too, feel that you should (just for the theoretical possibility of another, probably completey stupid and braindead and whatever, but still another approach to solve the same set of problems) try to follow Heiko's thoughts and see where that would take us?
If you continue to perseverate on the position that your code is the only possible solution, we might as well stop the discussion here.
Eh, I did try and I analyzed that approach in several postings. Please trust me, I'm not sticking to my own approach and I do not hesitate to borrow anything I could. Nobody's perfect and one have to run very fast just to stay in place in our profession less for moving forward... We have to learn constantly and I love to learn new things. That replaces me drugs, alcohol, whatever.
I wouldn't've object for a second if that've had any benefits. But that did not have any and handicaps however small they are were aplenty.
Hm... Please explain how are you going to use 2 different sets of pins with different access methods with one function?
Been there before. See for example http://lists.denx.de/pipermail/u-boot/2009-February/047937.html
Yep. It is possible but it is not any better. It doesn't provide any benefits. I did already explain this in several postings. If it did give some benefits I would definitely use that way.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
No, of course not.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
That seams a _terrible_ overhead to me: one register load per call. How fast is that I2C bus? Well below 50 kByte per second, right?
Please stay serious.
It is not big but it IS an overhead. One register load here, one function call there and soon we are bloated up to our ears. But the main factor is NOT that overhead. It simply doesn't give any benefits so it is not any better even if didn't have any overhead. I wouldn't hesitate to go that way if there were any benefits.
No problem with this.
There IS a problem First, that means you have _ABSOLUTELY_ no way to access any other adapter than default one until that variable made writable.
Please let's stop considering this as a problem.
Let's accept that this is a self-imposed and accepted restriction.
Eh, this is not about switching busses while running from flash, I was not going to support this either. But it gives at least some means for a simple hack if one or two boards really need this. Using global variable for this a.) does not give any benefits and b.) removes any possibility for accessing any adapter but the default one at all, hackish or not.
Second, all class functions must be self-sufficient, they should NOT rely on some external global variable to work. That might sound C++ese but no matter how I don't like C++ it does many things right.
We don't use C++ here, and we are not that strict if it allows for smaller code or solves other problems.
But it does not make code smaller or solve any problems. I would definitely consider it if it did.
I can call a member function for any adapter directly if needed with adap[N]->function(). You can NOT because in your case that "N" does not have any effect and your functions rely on an external global variable that you can not change.
Regards from the rat race. I am really tired of that.
It makes no sense to continue this discussion with you when you are not willing to even discuss the possibility of an alternative implementation that allows for a (writable!) global variable, and then you continue claiming we could not use writable variables (especially when we will not even require it because we don;t need bus switching before relocation).
There are better things I can do in my spare time.
Eh... I AM discussing it here. And that is NOT about switching busses while running from flash...
Guys, why are you not listening?... That variable, put aside all the ugliness and blasphemy, what problem it solves and what benefits it gives? Please show me some, pull my eyelids and open my eyes and may be I'll see the light... Where are the benefits?
That's a separate issue. I can offer N other ways to achieve this other than using that horror. And if they use the same binary image for different boards those boards are not different. If they put a chip on different bus that means the hardware is different. Different hardware means quite a lengthy process involving changes to the schematics, then re-layout, new set of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort definitely deserves a separate config file and simple repompile of U-Boot that takes less than a minute time.
Do you know which effort is involved in maintaining different software versions in a big project? It is a BIG win if you can use the same binary image on many differen board configurations.
Sure I do know. I'm in charge of some such projects right now and had been doing this before. Just for info -- I'm not an impulsive young kid, I'm 50 years old fart so I saw plenty of anything...
And who tells you that that allthe I2C busses and devices are on one board? There is things like backplanes that connect boards, where different boards in differnt configfurations may be inserted or removed and ... and ...
I can suggest many other ways to accomodate such systems without those dynamic busses. And keeping the entire maintenance and deployment concept intact. With all that topology in EEPROM etc. Those dynamic busses are not the only way to achieve this and even not the best one.
Please get a clue before making such statements!
If you guys had looked at the code you would've noticed that I _DID_ reworked those 2 particular boards and offered not just an approach but a working code to implement it. It might be not all that perfect (it never happens on the first attempt) but it works and does not require that oddity.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote: > That means you have to make changes in two places instead of one -- config > file AND $(BOARD).c. Also you use functions instead of macros and you can > NOT make them inline because they come from a separate object file. This > essentially defeats the very purpose of that common soft_i2c.c driver. If > you want to make functions for bitbanged I2C into the $(BOARD).c there is no > reason to have them as a base for that driver. It is much more logical to do > everything in reverse, i.e. instead of having soft_i2c.c as a bona fide > drivers and those I2C_SDA and friends as its building blocks make those > i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and > build the actual driver in the $(BOARD).c itself. Just convert that > soft_i2c.c into a header file with macros for real functions (soft_i2c_read > etc.) and instantiate them in the $(BOARD).c. Ecept that the code you posted is unreadable and you will need lots of very good arguments to make me accept it.
What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_ parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for different adapters. There is _NOTHING_ common between them.
How SDA, SCL are get/set is common, just how SDA and SCL are accessed is different! So there is no need for different i2c_write(), ... only SDA,SCL accessors are different.
Argh... Do you understand that those send_start etc. are _NOT_ the functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Template is _NOT_ a code, it is a _TOOL_ that generates code.
I know this.
In my case you simply make N i2c_write() functions for N adapters and assign
Thats not needed.
Hm... Please explain how are you going to use 2 different sets of pins with different access methods with one function?
I explained you this in more than one EMail. You have one more "if" in your SDA, SCL functions/macro. And don;t say to me, your processor is not fast enough to do one more if for every SDA, SCL access. When running u-boot, you have full 100% from your CPU for doing this. If your CPU is to slow to do this, I bet you have no fun when starting Linux. And look deeper in the i2c-bitbang driver from linux. When using this with the gpio API you get for every SDA, SCL access a function call, in this fn you must decide which pin, and what state for that pin -> min 2 "if" ... and so we are not worser than Linux.
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent you a soft-i2c driver proposal. If you look in this you see, no fn in it is changed!
We want this, because we don;t need to change everything in Sourcecode, as Wolfgang mentioned to you.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
Look above. If speed is here your problem, you have a lot of other problems with that, when running Linux.
And what are the advantages?
No Sourcecode change is needed.
still have that same code size _PLUS_ switch. Than, you assign a pointer to that monstrous function to i2c_write() member of _ALL_ adapter structures so a call to i2c_write() ends up calling that function. Now you have to somehow find out which switch case to execute. For that you need an additional global variable and additional member in each adapter struct. And those must
No problem with this.
There IS a problem First, that means you have _ABSOLUTELY_ no way to access any other adapter than default one until that variable made writable.
And we make it writeable, so no problem. But you have the _same_ problem, and as I told you. + you cannot switch actually busses with your approach!
Second, all class functions must be self-sufficient, they should NOT rely on some external global variable to work. That might sound C++ese but no matter how I don't like C++ it does many things right.
Hmm.. think this is not so a big problem here.
be writeable because you don't have any means of executing something like "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M) because in your case that "N" in "adap[N]" has absolutely no effect...
? you again mixing thinks. With my approach no need for changing the API in i2c-core.c. I think, thats you don;t got.
No, it is you who didn't get it. As we russians use to say, the bride is very good, just a little bit pregnant. You want to use a global variable in I2C object member functions. Global variables are _EVIL_ by theirself and should be only used as last resort, but in member functions they are not just plain evil, they are absolute NO-NO.
see above.
Please explain how it is better than simple and straightforward function per adapter? Which one is more complex? It looks like I simply don't understand something. You must've meant something else that I didn't get because the above comparison is SO obvious that it is almost impossible to somehow misunderstand it...
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Source file preprocessing.
I think you mean gcc, right?
No, I mean CPP. GCC is a frontend for cpp, as, ld etc. It does NOT preprocess files, cpp does.
Ah, okay, thanks.
Not rocket science and not much of black magic, just simple and straightforward token pasting...
> The only problem with that is it breaks uniformity and makes another mess. > The whole idea was to bring _ALL_ I2C drivers to a single place and make > them totally transparent and uniform. Something like e.g. Linux VFS. This is a boot loader with limited resources, not a general purpose OS.
It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
Make several of those for several drivers if it looks easier. Multiply it.
No need for it.
Please show me the code.
which code? The soft-i2c driver I posted here as a proposal, also how a I2C_SDA macro can look, if you have more than one bitbang driver. Look in the archive.
> And remember, the devil is in details. How are you going to assign > (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you > going to work on an adapter other that "current" in a situation when you can > NOT change "current" adapter (e.g. perform all I2C layer initialization > while still running from flash?) Remember, this is plain C and there is no What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
Please read above.
So you to ;-)
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
No, it is _NOT_.
It is.
That means you have another design for that. Please explain it.
I explained it. I posted the soft-i2c driver, also how i2c_set_bus_num can look, to support switching busses and init hardware adapters from it.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
It is.
Please show your code.
posted here.
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
The problem is you can _NOT_ change the bus if adapter is not initialized and you can _NOT_ initialize adapter because you _MUST_ change the bus to do this. No matter running from RAM or from ROM, you have exactly the same Catch 22.
You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number() will not work from flash, so how works this for you?
I can call a member function for any adapter directly if needed with adap[N]->function(). You can NOT because in your case that "N" does not have any effect and your functions rely on an external global variable that you can not change.
BTW, in my design i2c_cur_bus variable is NOT global. It has file scope.
Okay.
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds actual source code from set of external defines. One more time -- it is _ALREADY_ a template.
Yes, I know! But different adapters just differnet in how they access SDA, SCL not in i2c_write,..
That i2c_write uses a bunch of helper functions that are GENERATED from I2C_SDA and other macros. All those helper functions are different for different pin sets. And there is no compilable source code for them in soft_i2c.c (_EXISTING_ one, not just new,) only TEMPLATEs. Function is something you can get an address for. Templates do NOT have such address. You can NOT make a pointer to a template.
I give up.
I did nothing to that file, just added an option of generating several drivers instead of one. There is absolutely nothing I changed in that driver per se, that is _EXACTLY_ the same code.
You can make N such files for N adapters, or you can multiply those functions in that file N times to make N adapters. I'm doing the latter and nothing else.
What are you trying to prevent? I simply don't understand. Do you want me to just make several sets of functions in soft_i2c.c file because those defines look scary? No problems, just say it and I will do that. But there is no need for reinventing a wheel or adding sophisticated crutches to the obvious...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
We'll return to those dynamic busses later. I personally can't see any viable reason for that.
Maybe you not, but I told you, that there is a hardware manufacturer, that has his DTTs, EEProms,... on different hardware on different I2C busses. With this dynamic busses, he can have one image for all his boardvariants, and this is a need.
That's a separate issue. I can offer N other ways to achieve this other than using that horror. And if they use the same binary image for different boards those boards are not different. If they put a chip on different bus that means the hardware is different. Different hardware means quite a lengthy process involving changes to the schematics, then re-layout, new set of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort definitely deserves a separate config file and simple repompile of U-Boot that takes less than a minute time.
Thats your opinion, but there are others you cannot ignore.
But again, that is a separate issue. We do NOT have anything yet, just a cloned repository with no new source.
I agree here with you. First let us make some base, and then we can go on with that issue.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
It solves Catch 22.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
You can _NOT_. Please read above.
Why?
Because you use an external global variable to choose which adapter's function to call and that variable is not writable. You can NOT use adap[N]->function() because in your case that N does not have any effect.
But we can make this variable "writeable", so there is no problem with that.
> You are adding unnecessary complexity to the code. And you break uniformity. He. I must have thought the same before about someone else's code ;-)
Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
We do already have current bus. There is absolutely no need for such a pointer. Occam's razor.
OK, do we not make a pointer, just an integer, but it is the same problem, the integer must also be writeable! How would you change busses when running in flash?
I'm not going to change busses when I'm running from flash. But I can call functions on any adapter I want if needed.
Is it needed?
bye Heiko
On Fri, 20 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear ksi@koi8.net, > > In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote: >> That means you have to make changes in two places instead of one -- config >> file AND $(BOARD).c. Also you use functions instead of macros and you can >> NOT make them inline because they come from a separate object file. This >> essentially defeats the very purpose of that common soft_i2c.c driver. If >> you want to make functions for bitbanged I2C into the $(BOARD).c there is no >> reason to have them as a base for that driver. It is much more logical to do >> everything in reverse, i.e. instead of having soft_i2c.c as a bona fide >> drivers and those I2C_SDA and friends as its building blocks make those >> i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and >> build the actual driver in the $(BOARD).c itself. Just convert that >> soft_i2c.c into a header file with macros for real functions (soft_i2c_read >> etc.) and instantiate them in the $(BOARD).c. > Ecept that the code you posted is unreadable and you will need lots of > very good arguments to make me accept it. What is unreadable in that code?
I wouldn;t say unreadable but unnecessary swollen.
Take e.g. this:
=== Cut === #define I2C_SOFT_SEND_START(n) \ static void send_start##n(void) \ { \
[...]
I2C_DELAY2; I2C_SDA2(0); I2C_DELAY2;} === Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it will not make that source file any more readable by simply repeating those functions several times with just that "##n" different. And it will make that source file 4 times bigger with 4 instances or twice as big if there are only two of them.
Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t have to change anything in this driver (I posted such a patch as a proposal)
And again, you don;t need to do, as i did in this proposal, make this I2C_SDA, ... in function. You can of course make this in macros. OK, you have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_ parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for different adapters. There is _NOTHING_ common between them.
How SDA, SCL are get/set is common, just how SDA and SCL are accessed is different! So there is no need for different i2c_write(), ... only SDA,SCL accessors are different.
Argh... Do you understand that those send_start etc. are _NOT_ the functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Template is _NOT_ a code, it is a _TOOL_ that generates code.
I know this.
In my case you simply make N i2c_write() functions for N adapters and assign
Thats not needed.
Hm... Please explain how are you going to use 2 different sets of pins with different access methods with one function?
I explained you this in more than one EMail. You have one more "if" in your SDA, SCL functions/macro. And don;t say to me, your processor is not fast enough to do one more if for every SDA, SCL access. When running u-boot, you have full 100% from your CPU for doing this. If your CPU is to slow to do this, I bet you have no fun when starting Linux. And look deeper in the i2c-bitbang driver from linux. When using this with the gpio API you get for every SDA, SCL access a function call, in this fn you must decide which pin, and what state for that pin -> min 2 "if" ... and so we are not worser than Linux.
Why would we need _ADDITIONAL_ ifs in extremely simple macros that just access some memory/io location? They are so simple and small that you can not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_ make them any smaller. You can _NOT_ exclude actual code from them, you can only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have _ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in those macros.
But that is not all. You make those macros in function-like ones so you have to pass an argument to them. That requires for that argument to come from somewhere. For that you add an additional external global variable and additional member to the i2c_adap structure.
Less for extreme ugliness of using an external global variable in object methods that work on that particular object, you add a whole bunch of totally unnecessary entities. You are _NOT_ removing anything, you _ADD_ things.
Look, every adapter is represented by its very own structure with pointers to its own methods. There is absolutely no need for anything else, each such object (I2C adapter) has its own methods (accessor functions or whatever) and its own set of local variables. You pick the object then call its method that already knows how to deal with that very object.
Now you propose to scrap all that and replace object methods with some generic ones that do _NOT_ know what they are supposed to work on. There is no "this" pointer in C so you have to somehow let 'em know what they are supposed to work on. For this you add a HORRENDEOUS kludge, an external global variable. But the story does not end here, that variable does _NOT_ hold that argument, it is just some index in the _GLOBAL_ array of objects. To get that argument you add yet another entity, hwadapnr or whatever to the object and make yet another level of indirection to get that argument.
Here is the path to the real action in my case:
bus_no -> adap_no -> function -> action
Here is yours:
bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action
Not only that makes the path longer, not only it requires a blasphemy of using a global variable in object methods, but it also defeats a purpose of having a separate object for each adapter. Why do we need those separate objects if they contain exactly the same poiners to the very same functions?
There is already an index into the objects array and it is already used to choose a proper object. Why should we use that index again in the already chosen object? Why go twice over the same rake? And even if we were going that way what do we gain by using such horrible kludges? Is there any benefit?
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent you a soft-i2c driver proposal. If you look in this you see, no fn in it is changed!
We want this, because we don;t need to change everything in Sourcecode, as Wolfgang mentioned to you.
But you _DO_ want to change something! It is impossible to change something without changing it!
Furthermore, you also must change everything because you'll have _ALL_ the functions and templates in that same driver to take an additional argument and pass it all the way down to the bare metal, those accessor macros! That means you will have to add an access to that global variable to each and every I2C method (i2c_read etc.,) then change all utility functions (send_start etc.) to take that argument and pass it further to those macros.
And that will _NOT_ make resulting code any smaller because you'll still have all the actual guts of all those macros _PLUS_ register loads for arguments you are passing _PLUS_ conditional branches in each and every macro (and they will be multi-step if you have more than 2 adapters...)
I fail to see how this can be simpler, or better, or smaller, or any more logical than just multiplying that code ADAPTER_NUMBER times and use separate methods for each adapter.
Then comes an issue of where those macros should come from. In existing code you simply define methods to set/reset/tristate particular pins for separate adapters in separate macros.
In your case instead of N sets of such macros you must put a single set but each macro will have _ALL_ those macros in a switch statement with a separate case for each adapter. It is wrong to put a complex C code in a config file and it is much more prone to errors (think about all those macro expansion pitfalls that are aplenty) and more difficult to comprehense.
But there is not the end of a story yet... There is another thing that is probably overlooked. When adding an adapter to the config file one must define a separate set of macros for it in my case. If he forgot to define some macro U-Boot build will fail with "XYZ undefined" so it will be immediately known and very easy to fix.
In your case there will be just one set of macros with actual guts going into "case" statements of that omnipresent switch. When one adds an adapter he must add another "case" with the guts to that switch. The problem is that it will be still perfectly legal C code that will build without any errors of warnings if it is forgotten. That makes it much more difficult to debug.
That all is just basic principles of good engineering, not something special to U-Boot, C, or programming.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
Look above. If speed is here your problem, you have a lot of other problems with that, when running Linux.
And what are the advantages?
No Sourcecode change is needed.
How can you change sourcecode without changing sourcecode?
still have that same code size _PLUS_ switch. Than, you assign a pointer to that monstrous function to i2c_write() member of _ALL_ adapter structures so a call to i2c_write() ends up calling that function. Now you have to somehow find out which switch case to execute. For that you need an additional global variable and additional member in each adapter struct. And those must
No problem with this.
Less for regular principles of good engineering there are other problems some of which are described above.
There IS a problem First, that means you have _ABSOLUTELY_ no way to access any other adapter than default one until that variable made writable.
And we make it writeable, so no problem. But you have the _same_ problem, and as I told you. + you cannot switch actually busses with your approach!
Second, all class functions must be self-sufficient, they should NOT rely on some external global variable to work. That might sound C++ese but no matter how I don't like C++ it does many things right.
Hmm.. think this is not so a big problem here.
be writeable because you don't have any means of executing something like "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M) because in your case that "N" in "adap[N]" has absolutely no effect...
? you again mixing thinks. With my approach no need for changing the API in i2c-core.c. I think, thats you don;t got.
No, it is you who didn't get it. As we russians use to say, the bride is very good, just a little bit pregnant. You want to use a global variable in I2C object member functions. Global variables are _EVIL_ by theirself and should be only used as last resort, but in member functions they are not just plain evil, they are absolute NO-NO.
see above.
Please explain how it is better than simple and straightforward function per adapter? Which one is more complex? It looks like I simply don't understand something. You must've meant something else that I didn't get because the above comparison is SO obvious that it is almost impossible to somehow misunderstand it...
Why should we avoid using CPP feature that is SPECIALLY made for cases like this?
What CPP feature?
Source file preprocessing.
I think you mean gcc, right?
No, I mean CPP. GCC is a frontend for cpp, as, ld etc. It does NOT preprocess files, cpp does.
Ah, okay, thanks.
Not rocket science and not much of black magic, just simple and straightforward token pasting...
>> The only problem with that is it breaks uniformity and makes another mess. >> The whole idea was to bring _ALL_ I2C drivers to a single place and make >> them totally transparent and uniform. Something like e.g. Linux VFS. > This is a boot loader with limited resources, not a general purpose > OS. It doesn't matter. It is much better to have a uniform API for all the future developers to use than to multiply horrible hacks and reinventing the wheel again and again.
? We didn;t want to change the API, you mix things. We only want to prevent such a define monster in the bitbang driver.
Make several of those for several drivers if it looks easier. Multiply it.
No need for it.
Please show me the code.
which code? The soft-i2c driver I posted here as a proposal, also how a I2C_SDA macro can look, if you have more than one bitbang driver. Look in the archive.
Please see above. It is not just plain ugly and violates good engineering basics, it has a bunch of problems. And there is absolutely no benefits.
Those guys who wrote that driver as a template, generating actual code from a set of configuration macros did a wonderful job and I can't see any valid reason to reinvent the wheel.
>> And remember, the devil is in details. How are you going to assign >> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you >> going to work on an adapter other that "current" in a situation when you can >> NOT change "current" adapter (e.g. perform all I2C layer initialization >> while still running from flash?) Remember, this is plain C and there is no > What makes you insist that we cannot change a variable if we need to > be able to change one? It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only
What has this to do with soft_i2c.c?
Please read above.
So you to ;-)
changing that global variable but also reprogramming muxes/switches for
Yes, and this is independent of changing also this current pointer.
No, it is _NOT_.
It is.
That means you have another design for that. Please explain it.
I explained it. I posted the soft-i2c driver, also how i2c_set_bus_num can look, to support switching busses and init hardware adapters from it.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
It is.
Please show your code.
posted here.
your code should know if that particular bus it is switching to is directly connected or switched and check the bus it is switching from for muxes. If they are switched, your code should disconnect the current bus switches,
Yes, and this you did perfectly in i2c-core.c, where is your problem?
The problem is you can _NOT_ change the bus if adapter is not initialized and you can _NOT_ initialize adapter because you _MUST_ change the bus to do this. No matter running from RAM or from ROM, you have exactly the same Catch 22.
You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number() will not work from flash, so how works this for you?
I can call a member function for any adapter directly if needed with adap[N]->function(). You can NOT because in your case that "N" does not have any effect and your functions rely on an external global variable that you can not change.
BTW, in my design i2c_cur_bus variable is NOT global. It has file scope.
Okay.
then do that i2c_set_current_bus() and connect the switches to the new bus after that.
I don;t understand you know, really. Nobody in this discussion criticize the API, we just discuss the soft_i2c.c driver, and how we can prevent this defines ... or I lost something ...
You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds actual source code from set of external defines. One more time -- it is _ALREADY_ a template.
Yes, I know! But different adapters just differnet in how they access SDA, SCL not in i2c_write,..
That i2c_write uses a bunch of helper functions that are GENERATED from I2C_SDA and other macros. All those helper functions are different for different pin sets. And there is no compilable source code for them in soft_i2c.c (_EXISTING_ one, not just new,) only TEMPLATEs. Function is something you can get an address for. Templates do NOT have such address. You can NOT make a pointer to a template.
I give up.
I did nothing to that file, just added an option of generating several drivers instead of one. There is absolutely nothing I changed in that driver per se, that is _EXACTLY_ the same code.
You can make N such files for N adapters, or you can multiply those functions in that file N times to make N adapters. I'm doing the latter and nothing else.
What are you trying to prevent? I simply don't understand. Do you want me to just make several sets of functions in soft_i2c.c file because those defines look scary? No problems, just say it and I will do that. But there is no need for reinventing a wheel or adding sophisticated crutches to the obvious...
That means that code MUST somehow know the topology to take appropriate actions and properly configure those switches. That means you should somehow describe that topology for each and every board in CONFIG_* terms and make each and every place at U-Boot that invokes _ANY_ i2c function to take care of that switching.
Yep, this we(you did it ;-) have this in i2c-core.c ...
(And, I want to start this discuss again, you just dropped the support for adding new such busses per command shell ... you could not do this! But I have a solution for this on top of your patches, but I want start this discussion, if we have your patches in a testing branch in u-boot-i2c.git)
We'll return to those dynamic busses later. I personally can't see any viable reason for that.
Maybe you not, but I told you, that there is a hardware manufacturer, that has his DTTs, EEProms,... on different hardware on different I2C busses. With this dynamic busses, he can have one image for all his boardvariants, and this is a need.
That's a separate issue. I can offer N other ways to achieve this other than using that horror. And if they use the same binary image for different boards those boards are not different. If they put a chip on different bus that means the hardware is different. Different hardware means quite a lengthy process involving changes to the schematics, then re-layout, new set of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort definitely deserves a separate config file and simple repompile of U-Boot that takes less than a minute time.
Thats your opinion, but there are others you cannot ignore.
But again, that is a separate issue. We do NOT have anything yet, just a cloned repository with no new source.
I agree here with you. First let us make some base, and then we can go on with that issue.
My code does it transparently in the single place, i2c_set_current_bus() so higher level code doesn't have to bother with details.
Again, what has this to do with introducing a current pointer?
It solves Catch 22.
Then, all those I2C multiplexers/switches are I2C devices theirself that means you can NOT talk to them if the adapter they connected to is not initialized.
Ok, come, read my previous EMail, you can init this adapter before switching the muxes.
You can _NOT_. Please read above.
Why?
Because you use an external global variable to choose which adapter's function to call and that variable is not writable. You can NOT use adap[N]->function() because in your case that N does not have any effect.
But we can make this variable "writeable", so there is no problem with that.
There is a problem of multiplying entities beyond necessity. Entia non sunt multiplicanda praeter necessitatem. Occam's razor. It does _NOT_ solve any problems, does not give any benefits, and creates additional problems.
>> You are adding unnecessary complexity to the code. And you break uniformity. > He. I must have thought the same before about someone else's code ;-) Eh, I'm trying to make things simpler... For that particular board I'm expecting from assembly house by the end of this week I can make its particular hardware work with a bunch of one-time hacks in its $(BOARD).c...
But I think I'm not the first one to face such a problem and not the last one. So why wouldn't we make the proper API to get rid of all those hacks? I can do it now while paid by my current employer but there is no guarantee my next one would allow for such a waste from business standpoint...
I don;t understand why you have such problems with introducing a current pointer. And again, that has nothing to do with the API.
We do already have current bus. There is absolutely no need for such a pointer. Occam's razor.
OK, do we not make a pointer, just an integer, but it is the same problem, the integer must also be writeable! How would you change busses when running in flash?
I'm not going to change busses when I'm running from flash. But I can call functions on any adapter I want if needed.
Is it needed?
It might be. In my case there is nothing special required for this, this comes as a free extra.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Fri, 20 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote: > On Mon, 16 Feb 2009, Wolfgang Denk wrote:
[...]
Why would we need _ADDITIONAL_ ifs in extremely simple macros that just access some memory/io location? They are so simple and small that you can not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_ make them any smaller. You can _NOT_ exclude actual code from them, you can only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have _ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in those macros.
I know this, but as Wolfgang mentioned, we wouldn;t have such a lot of defines in soft-i2c.c.
[...]
Here is the path to the real action in my case:
bus_no -> adap_no -> function -> action
precisly: i2c_adap[i2c_bus[(bus)].adapter]-> function -> action ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is yours:
bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action
precisly: cur_ptr -> function -> if (cur_ptr -> hwadap_no) -> action
Hmm.. you got an idea, why i want a cur_ptr?
Just saying we don;t want in soft-i2c.c this hwadap_no
cur_ptr -> function -> action
Which _is_ faster than your actual implementation, or?
And you need not to do:
include/i2c.h: #ifndef CONFIG_SYS_I2C_DIRECT_BUS #define ADAP(bus) i2c_adap[i2c_bus[(bus)].adapter] #else #define ADAP(bus) i2c_adap[(bus)] #endif
it is always just cur_ptr->
Not only that makes the path longer, not only it requires a blasphemy of using a global variable in object methods, but it also defeats a purpose of having a separate object for each adapter. Why do we need those separate objects if they contain exactly the same poiners to the very same functions?
There is already an index into the objects array and it is already used to choose a proper object. Why should we use that index again in the already chosen object? Why go twice over the same rake? And even if we were going that way what do we gain by using such horrible kludges? Is there any benefit?
Only to save SourceCode as Wolfgang mentioned. Don;t ignore this.
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent you a soft-i2c driver proposal. If you look in this you see, no fn in it is changed!
We want this, because we don;t need to change everything in Sourcecode, as Wolfgang mentioned to you.
But you _DO_ want to change something! It is impossible to change something without changing it!
? Look in the soft-i2c.c I made, there is _NO_ change in the code for adding multibus (just i2c_adap_t struct added at the end). Why you claim there is a change? Again, only the I2C_SDA,....macros have to do be changed when using more than one driver (and this is absolute compatible with existing boards, when using only one there is no need for using hwadap_no in it, also for one adapter it is the same sourcecode as actual code)
I don;t say it is better than yours, but we can save SourceCode space (soft-i2c.c).
Furthermore, you also must change everything because you'll have _ALL_ the functions and templates in that same driver to take an additional argument and pass it all the way down to the bare metal, those accessor macros! That means you will have to add an access to that global variable to each and every I2C method (i2c_read etc.,) then change all utility functions (send_start etc.) to take that argument and pass it further to those macros.
Hey come on, look in my implementation of soft-i2c.c. Where did you see this changes? I tried my soft-i2c.c on a 8xx based board with 3 soft-i2c adapters (the last two are made only printfs, but i saw them), and no changes in i2c-core.c ... of course i added the cur_pointer (but I can use use your "i2c_adap[i2c_bus[(bus)].adapter]->" if you prefer this, and in the struct i2c_adapter the hwadap_nr"
And that will _NOT_ make resulting code any smaller because you'll still have all the actual guts of all those macros _PLUS_ register loads for arguments you are passing _PLUS_ conditional branches in each and every macro (and they will be multi-step if you have more than 2 adapters...)
Yep, but is it a real problem?
I fail to see how this can be simpler, or better, or smaller, or any more logical than just multiplying that code ADAPTER_NUMBER times and use separate methods for each adapter.
Its just simpler in sourcecode.
Then comes an issue of where those macros should come from. In existing code you simply define methods to set/reset/tristate particular pins for separate adapters in separate macros.
In your case instead of N sets of such macros you must put a single set but each macro will have _ALL_ those macros in a switch statement with a separate case for each adapter. It is wrong to put a complex C code in a config file and it is much more prone to errors (think about all those macro expansion pitfalls that are aplenty) and more difficult to comprehense.
OK, somebody who uses this, must know what he does ...
But there is not the end of a story yet... There is another thing that is probably overlooked. When adding an adapter to the config file one must define a separate set of macros for it in my case. If he forgot to define some macro U-Boot build will fail with "XYZ undefined" so it will be immediately known and very easy to fix.
Hmm.. and if he try his new adapter in may case and it didn;t work, he must think with his brain, okay ... thats bad.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
Look above. If speed is here your problem, you have a lot of other problems with that, when running Linux.
And what are the advantages?
No Sourcecode change is needed.
How can you change sourcecode without changing sourcecode?
Look at my code ... i only need to add the i2c_adapter struct in soft-i2c.c ...
[...]
which code? The soft-i2c driver I posted here as a proposal, also how a I2C_SDA macro can look, if you have more than one bitbang driver. Look in the archive.
Please see above. It is not just plain ugly and violates good engineering basics, it has a bunch of problems. And there is absolutely no benefits.
You ignore again the benefit in not changing soft-i2c code.
Those guys who wrote that driver as a template, generating actual code from a set of configuration macros did a wonderful job and I can't see any valid reason to reinvent the wheel.
This guy was Wolfgang, so why you ignore his arguments?
And why we are reinventing the wheel? I change nothing in the soft-i2c.c driver, so _why_ I are reinventing the wheel?????
[...]
I'm not going to change busses when I'm running from flash. But I can call functions on any adapter I want if needed.
Is it needed?
It might be. In my case there is nothing special required for this, this comes as a free extra.
No, we defined it is as _not_ required. But just to say it again, it also works with our suggestions.
bye Heiko
On Sat, 21 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Fri, 20 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Thu, 19 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote: > ksi@koi8.net wrote: >> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
[...]
Why would we need _ADDITIONAL_ ifs in extremely simple macros that just access some memory/io location? They are so simple and small that you can not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_ make them any smaller. You can _NOT_ exclude actual code from them, you can only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have _ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in those macros.
I know this, but as Wolfgang mentioned, we wouldn;t have such a lot of defines in soft-i2c.c.
So that is those defines that look scary, right? As I already said we could have them replaced with just repeated text.
It is quite a strange position - make everything ugly, make it bloated, violate every taboo just because something look scary.
And why is it just that soft_i2c.c, there are other multiadapter drivers (fsl_i2c.c, mxc_i2c.c, omap*.) Should we also apply those kludges to them?
[...]
Here is the path to the real action in my case:
bus_no -> adap_no -> function -> action
precisly: i2c_adap[i2c_bus[(bus)].adapter]-> function -> action ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is yours:
bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action
precisly: cur_ptr -> function -> if (cur_ptr -> hwadap_no) -> action
Hmm.. you got an idea, why i want a cur_ptr?
No, I don't. That scary looking underlined construction IS your cur_ptr. And the variable itself is an integer, bus number, that doesn't have to be adjusted when the code is relocated and i2c_get_bus_num() just returns this integer. And it is file scope.
Just saying we don;t want in soft-i2c.c this hwadap_no
cur_ptr -> function -> action
Which _is_ faster than your actual implementation, or?
My actual implementation.
And you need not to do:
include/i2c.h: #ifndef CONFIG_SYS_I2C_DIRECT_BUS #define ADAP(bus) i2c_adap[i2c_bus[(bus)].adapter] #else #define ADAP(bus) i2c_adap[(bus)] #endif
it is always just cur_ptr->
OK, I will not do a lengthy analysis again, I do not have more time to waste. With your cur_ptr you introduce a whole bunch of problems, violate rules of good engineering and do not provide _ANY_ benefits.
Not only that makes the path longer, not only it requires a blasphemy of using a global variable in object methods, but it also defeats a purpose of having a separate object for each adapter. Why do we need those separate objects if they contain exactly the same poiners to the very same functions?
There is already an index into the objects array and it is already used to choose a proper object. Why should we use that index again in the already chosen object? Why go twice over the same rake? And even if we were going that way what do we gain by using such horrible kludges? Is there any benefit?
Only to save SourceCode as Wolfgang mentioned. Don;t ignore this.
You do NOT save any SourceCode. And there is no reason to save SourceCode even if you did.
pointers to those functions to appropriate adapter struct members at _COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where you still have those N functions as "case" bodies of some switch so you
No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start() and friends and pass them a value to decide which set to use?
It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent you a soft-i2c driver proposal. If you look in this you see, no fn in it is changed!
We want this, because we don;t need to change everything in Sourcecode, as Wolfgang mentioned to you.
But you _DO_ want to change something! It is impossible to change something without changing it!
? Look in the soft-i2c.c I made, there is _NO_ change in the code for adding multibus (just i2c_adap_t struct added at the end). Why you claim there is a change? Again, only the I2C_SDA,....macros have to do be changed when using more than one driver (and this is absolute compatible with existing boards, when using only one there is no need for using hwadap_no in it, also for one adapter it is the same sourcecode as actual code)
I don;t say it is better than yours, but we can save SourceCode space (soft-i2c.c).
Eh, how are you going to pass that adap_no to those macros? Or have I underestimated the extent of that horror and you want to access that external global variable in each that macro? An you want THAT in config files?
Furthermore, you also must change everything because you'll have _ALL_ the functions and templates in that same driver to take an additional argument and pass it all the way down to the bare metal, those accessor macros! That means you will have to add an access to that global variable to each and every I2C method (i2c_read etc.,) then change all utility functions (send_start etc.) to take that argument and pass it further to those macros.
Hey come on, look in my implementation of soft-i2c.c. Where did you see this changes? I tried my soft-i2c.c on a 8xx based board with 3 soft-i2c adapters (the last two are made only printfs, but i saw them), and no changes in i2c-core.c ... of course i added the cur_pointer (but I can use use your "i2c_adap[i2c_bus[(bus)].adapter]->" if you prefer this, and in the struct i2c_adapter the hwadap_nr"
There in no such thing as "slightly pregnant" :) You should also change other multiadapter drivers for consistency. And please explain why we should have separate structures for each adapter that hold exactly the same pointers to the same function?
And that will _NOT_ make resulting code any smaller because you'll still have all the actual guts of all those macros _PLUS_ register loads for arguments you are passing _PLUS_ conditional branches in each and every macro (and they will be multi-step if you have more than 2 adapters...)
Yep, but is it a real problem?
Where are the benefits? It does NOT make _ANY_ good, just little harm. And "it is not that much harm" does _NOT_ justify something if there is no some benefit that is worth that additional harm.
I fail to see how this can be simpler, or better, or smaller, or any more logical than just multiplying that code ADAPTER_NUMBER times and use separate methods for each adapter.
Its just simpler in sourcecode.
It is NOT.
Then comes an issue of where those macros should come from. In existing code you simply define methods to set/reset/tristate particular pins for separate adapters in separate macros.
In your case instead of N sets of such macros you must put a single set but each macro will have _ALL_ those macros in a switch statement with a separate case for each adapter. It is wrong to put a complex C code in a config file and it is much more prone to errors (think about all those macro expansion pitfalls that are aplenty) and more difficult to comprehense.
OK, somebody who uses this, must know what he does ...
Yep. You make the code bloated, more difficult to comprehense for board designer and this is OK because some part of code that that designer would probably never ever look at looks scary to you? Perfect logic...
But there is not the end of a story yet... There is another thing that is probably overlooked. When adding an adapter to the config file one must define a separate set of macros for it in my case. If he forgot to define some macro U-Boot build will fail with "XYZ undefined" so it will be immediately known and very easy to fix.
Hmm.. and if he try his new adapter in may case and it didn;t work, he must think with his brain, okay ... thats bad.
That is really bad. And there is no way for a complier to detect it because that code is absolutely legal as far as compiler concerned.
That makes everything more messy and has a performance penalty -- instead of simple branch to those simple blocks you are adding at least one register load per call to load that argument.
Look above. If speed is here your problem, you have a lot of other problems with that, when running Linux.
And what are the advantages?
No Sourcecode change is needed.
How can you change sourcecode without changing sourcecode?
Look at my code ... i only need to add the i2c_adapter struct in soft-i2c.c ...
No you don't. You should also add that global variable, you should rewrite couple of functions, you should take care of adjusting that cur_pointer after relocation so it is still pointing to a proper place, you should change those I2C_xxx macros in _ALL_ config files because you either allow for multiadapter and then your configuration always uses that pointer or you do NOT allow it at all if you are not using that pointer to have a consistent code and so on...
And where is the benefit for all that mess?
[...]
which code? The soft-i2c driver I posted here as a proposal, also how a I2C_SDA macro can look, if you have more than one bitbang driver. Look in the archive.
Please see above. It is not just plain ugly and violates good engineering basics, it has a bunch of problems. And there is absolutely no benefits.
You ignore again the benefit in not changing soft-i2c code.
You can _NOT_ change soft_i2c.c code without changing the code! My patch did _NOT_ change that code, it is yours that does !!!
Those guys who wrote that driver as a template, generating actual code from a set of configuration macros did a wonderful job and I can't see any valid reason to reinvent the wheel.
This guy was Wolfgang, so why you ignore his arguments?
And why we are reinventing the wheel? I change nothing in the soft-i2c.c driver, so _why_ I are reinventing the wheel?????
You _DO_ change. That is me who don't. I'm just multiplying that file, not changing anything in it.
[...]
I'm not going to change busses when I'm running from flash. But I can call functions on any adapter I want if needed.
Is it needed?
It might be. In my case there is nothing special required for this, this comes as a free extra.
No, we defined it is as _not_ required. But just to say it again, it also works with our suggestions.
OK, I rest my case... Sorry guys I simply don't have any more time to waste, almost two weeks is more than enough. And I have another, more urgent job waiting.
Please count me off.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1); [...]
This function wouldn;t work from flash ...
bye Heiko
Hello ksi,
Heiko Schocher schrieb:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
And one more reason, why your function will not work from flash:
later in your i2c_set_bus_num function: [...] if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
You only switch muxes if init_done != 0 ... but init_done is not writeable when running from flash, so it is "= 0" when code is not relocated ... how work this for you?
But this can be solved, if we always init the hardwareadapter when switching to it and running from flash ... and if we are relocated, we can analyse this flag, if init_done = 0, we init this hardware adapter ...
My proposal for the i2c_set_bus_num(unsigned int bus) function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if (bus >= CONFIG_SYS_NUM_I2C_BUSSES) return(-1);
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Disconnect current bus (turn off muxes if any) */ if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
i = CONFIG_SYS_I2C_MAX_HOPS;
do { u_int8_t chip; if ((chip = i2c_bus[i2c_cur_bus].next_hop[--i].chip) == 0) continue;
ADAP(i2c_cur_bus)->write(chip, 0, 0, &buf, 1);
} while (i > 0); } #endif
cur_adap_nr = (i2c_adap_t *)&ADAP(bus); if (ADAP(bus)->init_done == 0) { i2c_init_bus(bus, ADAP(bus)->speed, ADAP(bus)->slaveaddr); }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Connect requested bus if behind muxes */ if (i2c_bus[bus].next_hop[0].chip != 0) {
/* Set all muxes along the path to that bus */ for (i = 0; i < CONFIG_SYS_I2C_MAX_HOPS; i++) {
if (i2c_bus[bus].next_hop[i].chip == 0) break;
i2c_mux_set(i2c_bus[bus].adapter, i2c_bus[bus].next_hop[i].mux.id, i2c_bus[bus].next_hop[i].chip, i2c_bus[bus].next_hop[i].channel); } } #endif
i2c_cur_bus = bus; return(0); }
This function should also work, when running from flash. + a hardware adapter is only initialized when we switch to it, so no need to initialize all hardwareadapters somewhere ... (requirement: cur_adap_nr, i2c_cur_bus is writeable when running in flash)
bye Heiko
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
Heiko Schocher schrieb:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
And one more reason, why your function will not work from flash:
later in your i2c_set_bus_num function: [...] if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
You only switch muxes if init_done != 0 ... but init_done is not writeable when running from flash, so it is "= 0" when code is not relocated ... how work this for you?
But this can be solved, if we always init the hardwareadapter when switching to it and running from flash ... and if we are relocated, we can analyse this flag, if init_done = 0, we init this hardware adapter ...
My proposal for the i2c_set_bus_num(unsigned int bus) function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if (bus >= CONFIG_SYS_NUM_I2C_BUSSES) return(-1);
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Disconnect current bus (turn off muxes if any) */ if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
i = CONFIG_SYS_I2C_MAX_HOPS; do { u_int8_t chip; if ((chip = i2c_bus[i2c_cur_bus].next_hop[--i].chip) == 0) continue; ADAP(i2c_cur_bus)->write(chip, 0, 0, &buf, 1); } while (i > 0);} #endif
cur_adap_nr = (i2c_adap_t *)&ADAP(bus); if (ADAP(bus)->init_done == 0) { i2c_init_bus(bus, ADAP(bus)->speed, ADAP(bus)->slaveaddr); }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Connect requested bus if behind muxes */ if (i2c_bus[bus].next_hop[0].chip != 0) {
/* Set all muxes along the path to that bus */ for (i = 0; i < CONFIG_SYS_I2C_MAX_HOPS; i++) { if (i2c_bus[bus].next_hop[i].chip == 0) break; i2c_mux_set(i2c_bus[bus].adapter, i2c_bus[bus].next_hop[i].mux.id, i2c_bus[bus].next_hop[i].chip, i2c_bus[bus].next_hop[i].channel); }} #endif
i2c_cur_bus = bus; return(0); }
This function should also work, when running from flash. + a hardware adapter is only initialized when we switch to it, so no need to initialize all hardwareadapters somewhere ... (requirement: cur_adap_nr, i2c_cur_bus is writeable when running in flash)
You are multiplying entities. i2c_init() is invoked as a part of system bootup process in libXXX/board.c anyways. There is no need for any global variables, even non-writable for proposed code to initialize adapters.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181054310.5002@home-gw.koi8.net you wrote:
You are multiplying entities. i2c_init() is invoked as a part of system bootup process in libXXX/board.c anyways. There is no need for any global variables, even non-writable for proposed code to initialize adapters.
Please keep in mind that (even if it should be different at the moment), I2C should only be initialized when needed, i. e. when U-Boot is running any code that needs to access the I2C bus, but not always after each reset on all systems that have I2C enabled.
This is a mandatory requirement for a rewrite.
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181054310.5002@home-gw.koi8.net you wrote:
You are multiplying entities. i2c_init() is invoked as a part of system bootup process in libXXX/board.c anyways. There is no need for any global variables, even non-writable for proposed code to initialize adapters.
Please keep in mind that (even if it should be different at the moment), I2C should only be initialized when needed, i. e. when U-Boot is running any code that needs to access the I2C bus, but not always after each reset on all systems that have I2C enabled.
This is a mandatory requirement for a rewrite.
How would you know what to initialize and what not to? We were initializing _ALL_ I2C adapters up to today with a single i2c_init() function. It just happened that in most cases the total number of adapters was 1 but not always (fsl_i2c.c is a notable example.) There was no mechanism for selective initialization in current U-Boot.
It is easy to initialize just a selected set of adapters in the new code but how do we decide what to initialize and what not to?
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But that is simply ridiculous and adds _ABSOLUTELY_ unnecessary code.
Or should we remove i2c_init() from _ALL_ common places and let board developers to call i2c_adap[X]->init() as they see fit? But that is a big rewrite... And there is another place, cmd_i2c.c that must be taken care of...
We can easily add such an initialization to i2c_set_current_bus() (or however it is called) so it will be initializing adapters as bus is changed but that does _NOT_ solve all the problems and requires messing with the entire U-Boot source -- that set_bus call should be added everywhere where I2C is used and it is not a simple grep-and-replace because there is absolutely no reason to add set_bus to each of e.g. 3 i2c_write() calls executed in a row...
Please remember, that even with the new code we don't have to modify anything but config file (trivial one-time changes) for 99% of supported boards that only use one I2C bus -- no set_bus needed at all.
There is also no way of DE-initilizing those interfaces so that init is like a gun trigger -- once it is pulled, there is no way to bring the bullet back.
And the final question -- what is wrong with initializing all I2C adapters for a handful of boards that have more than one? What is the problem? I can make that init_all function a weak alias so if there's some problem with performing total init it can be replaced with board-specific function. But frankly I can not see any problems with initializing all 2-3 adapters for a few multiadapter boards...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181400470.5729@home-gw.koi8.net you wrote:
How would you know what to initialize and what not to? We were initializing
I don't know. You probably need some way to encode some kind of routing information that tells you which adapter(s) need to be initialized to reach some specific device.
_ALL_ I2C adapters up to today with a single i2c_init() function. It just
Yes, that's the status quo. And it is not good as is, and shall be changed.
It is easy to initialize just a selected set of adapters in the new code but how do we decide what to initialize and what not to?
Good question. Probably each I2C device will have some list of bus/adapter ID's that need to be up to access it, and that will get shut down afterwards.
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
No, because we probably do not need to activate all tehse adapters at the same time.
Or should we remove i2c_init() from _ALL_ common places and let board developers to call i2c_adap[X]->init() as they see fit? But that is a big rewrite... And there is another place, cmd_i2c.c that must be taken care of...
You will always call i2c_init() for a specific I2C device.
The code should automatically know which adapters need to be initia- lized to "talk" to that device. Yes, you must somehow describe the I2C bus topology, but a single one-way description for the path from the specific device to the CPU should be sufficient.
There is also no way of DE-initilizing those interfaces so that init is like a gun trigger -- once it is pulled, there is no way to bring the bullet back.
That needs to be changed, too.
And the final question -- what is wrong with initializing all I2C adapters for a handful of boards that have more than one? What is the problem? I can
Initializing things that are not actually needed is bad for many reasons. It takes time, and boot time is critical on many systems. It increases the power consumption, and we have a growing number of mobile devices where power consumption is critical. It carries big potential for "unexpected" behaviour (read: nasty failure modes), and so on.
There are projets around that paid a bite price for not deinitia- lizing devices after use. I2C may be relatively harmless, but we thought the same about USB until it bit. And it bit hard. That's a lesson learned.
make that init_all function a weak alias so if there's some problem with performing total init it can be replaced with board-specific function. But frankly I can not see any problems with initializing all 2-3 adapters for a few multiadapter boards...
A rule is a rule is a rule.
Best regards,
Wolfgang Denk
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181400470.5729@home-gw.koi8.net you wrote:
How would you know what to initialize and what not to? We were initializing
I don't know. You probably need some way to encode some kind of routing information that tells you which adapter(s) need to be initialized to reach some specific device.
It's already there. ADAP(bus_no) macro takes care of this returning the right adapter .
_ALL_ I2C adapters up to today with a single i2c_init() function. It just
Yes, that's the status quo. And it is not good as is, and shall be changed.
It is easy to initialize just a selected set of adapters in the new code but how do we decide what to initialize and what not to?
Good question. Probably each I2C device will have some list of bus/adapter ID's that need to be up to access it, and that will get shut down afterwards.
Eh, that is what I call overcomplicated... And it definitely does not belong to I2C subsystem per se, it's up to the board developer to know which bus each device is connected to. I2C system can not now where that "afterwards" happen. If we are reading something from a device, checking some bit, writing something else back depending on that bit and then repeat the entire cycle (let's say we are polling for something and until that something happened we are sending a "please continue" command back to that device, then we tell it "thanks, stop now",) where in this sequence that "afterwards" happens?
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
No, because we probably do not need to activate all tehse adapters at the same time.
Or should we remove i2c_init() from _ALL_ common places and let board developers to call i2c_adap[X]->init() as they see fit? But that is a big rewrite... And there is another place, cmd_i2c.c that must be taken care of...
You will always call i2c_init() for a specific I2C device.
The code should automatically know which adapters need to be initia- lized to "talk" to that device. Yes, you must somehow describe the I2C bus topology, but a single one-way description for the path from the specific device to the CPU should be sufficient.
Topology is already there, this is not a problem. That is that initinialization that is. The a.m. approach means the I2C layer should not initialize anything at all, it should only provide API to do it (that it does.) It is up to each board developer to call appropriate init function when needed. That means huge, no, even _HUGE_ rewrite with entire U-Boot affected. And this is not grep-and-replace kind of job, it requires intimate knowledge of the boards affected...
Sorry, I do not have _THAT_ much time, my board is already on a pick-and-place machine and I will have to start the bringup process really soon...
There is also no way of DE-initilizing those interfaces so that init is like a gun trigger -- once it is pulled, there is no way to bring the bullet back.
That needs to be changed, too.
Again, most of the drivers disable the adapter when transfer is complete but that does not involve turning of its power or clock or reconfiguring GPIOs or whatever else it was before initial adapter init. Then, it is all different for different chips. E.g. OMAP can only turn power on/off for all I2C adapters so initializing any of them should turn the power on and it must not be turned back off until the last adapter is shutdown. Pinmuxes are usually configured as a part of board setup etc.
This means a whole lot of, IMHO, absolutely unneeded work.
And the final question -- what is wrong with initializing all I2C adapters for a handful of boards that have more than one? What is the problem? I can
Initializing things that are not actually needed is bad for many reasons. It takes time, and boot time is critical on many systems. It increases the power consumption, and we have a growing number of mobile devices where power consumption is critical. It carries big potential for "unexpected" behaviour (read: nasty failure modes), and so on.
There are projets around that paid a bite price for not deinitia- lizing devices after use. I2C may be relatively harmless, but we thought the same about USB until it bit. And it bit hard. That's a lesson learned.
I'd say it should not be treated as "never do it again" but rather as "take caution"...
make that init_all function a weak alias so if there's some problem with performing total init it can be replaced with board-specific function. But frankly I can not see any problems with initializing all 2-3 adapters for a few multiadapter boards...
A rule is a rule is a rule.
Eh, never say never...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181400470.5729@home-gw.koi8.net you wrote:
[...]
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
No, because we probably do not need to activate all tehse adapters at the same time.
Or should we remove i2c_init() from _ALL_ common places and let board developers to call i2c_adap[X]->init() as they see fit? But that is a big rewrite... And there is another place, cmd_i2c.c that must be taken care of...
You will always call i2c_init() for a specific I2C device.
The code should automatically know which adapters need to be initia- lized to "talk" to that device. Yes, you must somehow describe the I2C bus topology, but a single one-way description for the path from the specific device to the CPU should be sufficient.
Topology is already there, this is not a problem. That is that initinialization that is. The a.m. approach means the I2C layer should not
No this is no problem, if you have a look at my proposal for i2c_set_bus_num()
bye Heiko
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181602470.6072@home-gw.koi8.net you wrote:
Good question. Probably each I2C device will have some list of bus/adapter ID's that need to be up to access it, and that will get shut down afterwards.
Eh, that is what I call overcomplicated... And it definitely does not belong to I2C subsystem per se, it's up to the board developer to know which bus
You want to support complex systems, so let's assume this: The board developer may not know this. He designs a board which runs some I2C signals to some connectors where other boards, unknown to him may or may not be connected. Only the system integrator plugs together a system and knows the bus topology and where which device is.
each device is connected to. I2C system can not now where that "afterwards" happen. If we are reading something from a device, checking some bit, writing something else back depending on that bit and then repeat the entire cycle (let's say we are polling for something and until that something happened we are sending a "please continue" command back to that device, then we tell it "thanks, stop now",) where in this sequence that "afterwards" happens?
After your last question mark.
Best regards,
Wolfgang Denk
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181400470.5729@home-gw.koi8.net you wrote:
How would you know what to initialize and what not to? We were initializing
I don't know. You probably need some way to encode some kind of
Look at my proposal for i2c_set_bus_num().
http://lists.denx.de/pipermail/u-boot/2009-February/047860.html
This function is the central point for switching between busses. It _must_ be called, when switching busses, so this function can decide, if the hardware adapter is initialized or not. If not, it will call the init function ... and there is no problem for calling the init function, when running in flash. We _first_ call the adapter specific init() function and _then_ switching the muxes.
Also we can do in this function the adapterspecific deinit() function ... one central point for doing init(), deinit() sounds good to me.
We just have to look, that boards who uses i2c_* functions, now call before accessing the I2C bus, i2c_set_bus_num(). But this is with every implemntation for multibussuport a ToDo, because there probably now more then one bus, so we must take care of this.
It is easy to initialize just a selected set of adapters in the new code but how do we decide what to initialize and what not to?
Good question. Probably each I2C device will have some list of bus/adapter ID's that need to be up to access it, and that will get shut down afterwards.
No need for this i2c_set_bus_num() will do that automagically.
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
No, because we probably do not need to activate all tehse adapters at the same time.
We don;t need this, because there is i2c_set_bus_number, which can do this for us.
There is also no way of DE-initilizing those interfaces so that init is like a gun trigger -- once it is pulled, there is no way to bring the bullet back.
That needs to be changed, too.
Ack.
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello Wolfgang,
Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181400470.5729@home-gw.koi8.net you wrote:
How would you know what to initialize and what not to? We were initializing
I don't know. You probably need some way to encode some kind of
Look at my proposal for i2c_set_bus_num().
http://lists.denx.de/pipermail/u-boot/2009-February/047860.html
This function is the central point for switching between busses. It _must_ be called, when switching busses, so this function can decide, if the hardware adapter is initialized or not. If not, it will call the init function ... and there is no problem for calling the init function, when running in flash. We _first_ call the adapter specific init() function and _then_ switching the muxes.
Also we can do in this function the adapterspecific deinit() function ... one central point for doing init(), deinit() sounds good to me.
We just have to look, that boards who uses i2c_* functions, now call before accessing the I2C bus, i2c_set_bus_num(). But this is with every implemntation for multibussuport a ToDo, because there probably now more then one bus, so we must take care of this.
It is easy to initialize just a selected set of adapters in the new code but how do we decide what to initialize and what not to?
Good question. Probably each I2C device will have some list of bus/adapter ID's that need to be up to access it, and that will get shut down afterwards.
No need for this i2c_set_bus_num() will do that automagically.
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
No, because we probably do not need to activate all tehse adapters at the same time.
We don;t need this, because there is i2c_set_bus_number, which can do this for us.
There is also no way of DE-initilizing those interfaces so that init is like a gun trigger -- once it is pulled, there is no way to bring the bullet back.
That needs to be changed, too.
Ack.
Sorry guys, I simply do not have THAT much time... I have my board coming from assembly house so I'll probably end up with some board-specific hack; that will be much faster.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Heiko Schocher schrieb:
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
PLease answer my question!!!
How can you switch busses when running from flash???
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
And one more reason, why your function will not work from flash:
later in your i2c_set_bus_num function: [...] if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
You only switch muxes if init_done != 0 ... but init_done is not writeable when running from flash, so it is "= 0" when code is not relocated ... how work this for you?
Again, how work this!
But this can be solved, if we always init the hardwareadapter when switching to it and running from flash ... and if we are relocated, we can analyse this flag, if init_done = 0, we init this hardware adapter ...
My proposal for the i2c_set_bus_num(unsigned int bus) function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if (bus >= CONFIG_SYS_NUM_I2C_BUSSES) return(-1);
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Disconnect current bus (turn off muxes if any) */ if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) && (ADAP(i2c_cur_bus)->init_done != 0)) {
i = CONFIG_SYS_I2C_MAX_HOPS; do { u_int8_t chip; if ((chip = i2c_bus[i2c_cur_bus].next_hop[--i].chip) == 0) continue; ADAP(i2c_cur_bus)->write(chip, 0, 0, &buf, 1); } while (i > 0);} #endif
cur_adap_nr = (i2c_adap_t *)&ADAP(bus); if (ADAP(bus)->init_done == 0) { i2c_init_bus(bus, ADAP(bus)->speed, ADAP(bus)->slaveaddr); }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS /* Connect requested bus if behind muxes */ if (i2c_bus[bus].next_hop[0].chip != 0) {
/* Set all muxes along the path to that bus */ for (i = 0; i < CONFIG_SYS_I2C_MAX_HOPS; i++) { if (i2c_bus[bus].next_hop[i].chip == 0) break; i2c_mux_set(i2c_bus[bus].adapter, i2c_bus[bus].next_hop[i].mux.id, i2c_bus[bus].next_hop[i].chip, i2c_bus[bus].next_hop[i].channel); }} #endif
i2c_cur_bus = bus; return(0); }
This function should also work, when running from flash. + a hardware adapter is only initialized when we switch to it, so no need to initialize all hardwareadapters somewhere ... (requirement: cur_adap_nr, i2c_cur_bus is writeable when running in flash)
You are multiplying entities. i2c_init() is invoked as a part of system
But Wolfgang suggested to init an I2C adapter just if we use it, and above function will do this. Again read my EMails. I wrote:
This function should also work, when running from flash. + a hardware adapter is only initialized when we switch to it, so no need to initialize all hardwareadapters somewhere ...
So no need for calling i2c_init in libXXX/board.c or somewhere. We must just look that for example in dtt_init is also a i2c_set_bus_num () called.
bootup process in libXXX/board.c anyways. There is no need for any global variables, even non-writable for proposed code to initialize adapters.
Not for initialize adapters, because you want to init all adapters. But if we want to do what Wolfgang suggested, we don;t need to call i2c_init for all adapters.
bye Heiko
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
So what? I don't need that function to initialize adapters.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
So what? I don't need that function to initialize adapters.
Read my EMail!
But to switch busses, right? And how you switch busses, when running from flash, and you do a:
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);
in it??
bye Heiko
On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@home-gw.koi8.net you wrote:
[...]
And remember, the devil is in details. How are you going to assign (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you going to work on an adapter other that "current" in a situation when you can NOT change "current" adapter (e.g. perform all I2C layer initialization while still running from flash?) Remember, this is plain C and there is no
What makes you insist that we cannot change a variable if we need to be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And number of busses can be bigger than number of adapters (e.g. when some busses a reached via muxes or switches.) When doing i2c_set_current_bus() you are switching _NOT_ adapters, but busses. That involves not only changing that global variable but also reprogramming muxes/switches for i2c_set_current_bus() to be consistent and hardware independent. Otherwise
You have no i2c_set_current_bus() in your code! I think you mean i2c_set_current_bus(), right?
And this function fails when running from flash! So, how can you switch busses with your patches when running from flash?
Here your function:
int i2c_set_bus_num(unsigned int bus) { #ifndef CONFIG_SYS_I2C_DIRECT_BUS int i; u_int8_t buf; #endif
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);[...]
This function wouldn;t work from flash ...
So what? I don't need that function to initialize adapters.
Read my EMail!
But to switch busses, right? And how you switch busses, when running from flash, and you do a:
if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC)) return(-1);in it??
I'm not going to switch busses while running from flash with that function.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
participants (3)
-
Heiko Schocher -
ksi＠koi8.net -
Wolfgang Denk