| Message ID | 1351618133-14909-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
On 10/30/2012 11:28 AM, Simon Glass wrote: > (just for illustration, please don't merge) > > This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra > i2c driver to support this. > diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > +#ifdef CONFIG_TEGRA_I2C > +extern struct i2c_adapter tegra_i2c_adap[]; > +#endif I'm not sure why that's needed if the config files have to put the adpater list into a #define: > diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h > +#define CONFIG_SYS_I2C > +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} > +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves? Aside from that, this looks OK to me at a quick glance.
Hello Simon, On 30.10.2012 18:28, Simon Glass wrote: > (just for illustration, please don't merge) > > This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra > i2c driver to support this. > > Signed-off-by: Simon Glass<sjg@chromium.org> > --- > drivers/i2c/i2c_core.c | 3 ++ > drivers/i2c/tegra_i2c.c | 64 +++++++++++++++++-------------------------- > include/configs/seaboard.h | 4 ++- > include/configs/trimslice.h | 4 ++- > include/configs/whistler.h | 4 ++- > 5 files changed, 37 insertions(+), 42 deletions(-) > > diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > index 5a738d5..44f1297 100644 > --- a/drivers/i2c/i2c_core.c > +++ b/drivers/i2c/i2c_core.c > @@ -9,6 +9,9 @@ > #ifdef CONFIG_SYS_I2C_SOFT > extern struct i2c_adapter soft_i2c_adap[]; > #endif > +#ifdef CONFIG_TEGRA_I2C > +extern struct i2c_adapter tegra_i2c_adap[]; > +#endif > > struct i2c_adapter *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] = > CONFIG_SYS_I2C_ADAPTERS; > diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c > index 3e157f4..b92bcf1 100644 > --- a/drivers/i2c/tegra_i2c.c > +++ b/drivers/i2c/tegra_i2c.c > @@ -35,8 +35,6 @@ > > DECLARE_GLOBAL_DATA_PTR; > > -static unsigned int i2c_bus_num; > - > /* Information about i2c controller */ > struct i2c_bus { > int id; > @@ -327,21 +325,11 @@ static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num) > return bus; > } > > -unsigned int i2c_get_bus_speed(void) > +static unsigned int tegra_i2c_set_bus_speed(unsigned int speed) > { > struct i2c_bus *bus; > > - bus = tegra_i2c_get_bus(i2c_bus_num); > - if (!bus) > - return 0; > - return bus->speed; > -} > - > -int i2c_set_bus_speed(unsigned int speed) > -{ > - struct i2c_bus *bus; > - > - bus = tegra_i2c_get_bus(i2c_bus_num); > + bus = tegra_i2c_get_bus(i2c_get_bus_num()); > if (!bus) > return 0; > bus->speed = speed; > @@ -450,7 +438,7 @@ void i2c_init_board(void) > return; > } > > -void i2c_init(int speed, int slaveaddr) > +static void tegra_i2c_init(int speed, int slaveaddr) > { > /* This will override the speed selected in the fdt for that port */ > debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr); > @@ -500,7 +488,7 @@ int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) > } > > /* Probe to see if a chip is present. */ > -int i2c_probe(uchar chip) > +static int tegra_i2c_probe(uchar chip) > { > struct i2c_bus *bus; > int rc; > @@ -526,7 +514,8 @@ static int i2c_addr_ok(const uint addr, const int alen) > } > > /* Read bytes */ > -int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) > +static int tegra_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, > + int len) > { > struct i2c_bus *bus; > uint offset; > @@ -534,7 +523,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) > > debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n", > chip, addr, len); > - bus = tegra_i2c_get_bus(i2c_bus_num); > + bus = tegra_i2c_get_bus(i2c_get_bus_num()); > if (!bus) > return 1; > if (!i2c_addr_ok(addr, alen)) { > @@ -564,7 +553,8 @@ 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) > +static int tegra_i2c_write(uchar chip, uint addr, int alen, uchar *buffer, > + int len) > { > struct i2c_bus *bus; > uint offset; > @@ -572,7 +562,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) > > debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n", > chip, addr, len); > - bus = tegra_i2c_get_bus(i2c_bus_num); > + bus = tegra_i2c_get_bus(i2c_get_bus_num()); > if (!bus) > return 1; > if (!i2c_addr_ok(addr, alen)) { > @@ -593,25 +583,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) > return 0; > } > > -#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 (bus>= TEGRA_I2C_NUM_CONTROLLERS || !i2c_controllers[bus].inited) > - return -1; > - i2c_bus_num = bus; > - > - return 0; > -} > -#endif > - > int tegra_i2c_get_dvc_bus_num(void) > { > int i; > @@ -625,3 +596,18 @@ int tegra_i2c_get_dvc_bus_num(void) > > return -1; > } > + > +struct i2c_adapter tegra_i2c_adap[] = { > + { > + .init = tegra_i2c_init, > + .probe = tegra_i2c_probe, > + .read = tegra_i2c_read, > + .write = tegra_i2c_write, > + .set_bus_speed = tegra_i2c_set_bus_speed, > + .speed = 100000, > + .slaveaddr = 0, speed and slaveaddr always fix? > + .name = "tegra-i2c", > + .init_done = 0, > + .hwadapnr = 0, > + } > +}; > diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h > index 0727a4c..61aca66 100644 > --- a/include/configs/seaboard.h > +++ b/include/configs/seaboard.h > @@ -58,10 +58,12 @@ > /* I2C */ > #define CONFIG_TEGRA_I2C > #define CONFIG_SYS_I2C_INIT_BOARD > -#define CONFIG_I2C_MULTI_BUS > #define CONFIG_SYS_MAX_I2C_BUS 4 > #define CONFIG_SYS_I2C_SPEED 100000 > #define CONFIG_CMD_I2C > +#define CONFIG_SYS_I2C > +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} > +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS TEGRA_I2C_NUM_CONTROLLERS is defined with a value of 4, so we need 4 entries in tegra_i2c_adap[] ... > +struct i2c_adapter tegra_i2c_adap[] = { > + { > + .init = tegra_i2c_init, > + .probe = tegra_i2c_probe, > + .read = tegra_i2c_read, > + .write = tegra_i2c_write, > + .set_bus_speed = tegra_i2c_set_bus_speed, > + .speed = 100000, > + .slaveaddr = 0, > + .name = "tegra-i2c", > + .init_done = 0, > + .hwadapnr = 0, > + }, > + { > + .init = tegra_i2c_init, > + .probe = tegra_i2c_probe, > + .read = tegra_i2c_read, > + .write = tegra_i2c_write, > + .set_bus_speed = tegra_i2c_set_bus_speed, > + .speed = 100000, > + .slaveaddr = 0, > + .name = "tegra-i2c-2", > + .init_done = 0, > + .hwadapnr = 1, > + }, [...] > +}; [...] Just pushed my v2 patches to: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20121030 there I added the fsl driver to the new framework, see: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blobdiff;f=drivers/i2c/fsl_i2c.c;h=aa12d87a94937cf2f850ce74af244543336f7699;hp=3cb232fdd110c44ab391e38252b707a89c565fa2;hb=ec77e97e9750c4e9cde7f2e5c646d7cf5bda6c29;hpb=48cf290f61518a4c26a163c823c570fea3660b66 maybe it helps to look in it for you? But if you agree, I rework your two patches and add it to my v2 serie (post it to you in provate EMail for testing) and if tests are OK, post the v2 serie with it? bye, Heiko
Hello Stephen, On 30.10.2012 23:32, Stephen Warren wrote: > On 10/30/2012 11:28 AM, Simon Glass wrote: >> (just for illustration, please don't merge) >> >> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra >> i2c driver to support this. > >> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > >> +#ifdef CONFIG_TEGRA_I2C >> +extern struct i2c_adapter tegra_i2c_adap[]; >> +#endif > > I'm not sure why that's needed if the config files have to put the > adpater list into a #define: > >> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h > >> +#define CONFIG_SYS_I2C >> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS > > But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init > functions (which presumably would be called from board code or as a > result of DT parsing) dynamically register themselves? This would not work, if we are running for example from flash on powerpc before relocation. There some boards need to read RAM infos from an SPD Eeprom through i2c ... so we must have very early i2c bus ready and usable ... It maybe a good plus to add i2c busses/adapters dynamically after relocation is done and we have enough RAM ... I tried such an approach here: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4 but this is an old state (only rebased a long time), but it is maybe a good base for doing such an feature if it is needed ... > Aside from that, this looks OK to me at a quick glance. bye, Heiko
On 10/31/2012 12:00 AM, Heiko Schocher wrote: > Hello Stephen, > > On 30.10.2012 23:32, Stephen Warren wrote: >> On 10/30/2012 11:28 AM, Simon Glass wrote: >>> (just for illustration, please don't merge) >>> >>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and >>> the Tegra >>> i2c driver to support this. >> >>> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c >> >>> +#ifdef CONFIG_TEGRA_I2C >>> +extern struct i2c_adapter tegra_i2c_adap[]; >>> +#endif >> >> I'm not sure why that's needed if the config files have to put the >> adpater list into a #define: >> >>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >> >>> +#define CONFIG_SYS_I2C >>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >> >> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >> functions (which presumably would be called from board code or as a >> result of DT parsing) dynamically register themselves? > > This would not work, if we are running for example from flash on > powerpc before relocation. There some boards need to read RAM > infos from an SPD Eeprom through i2c ... so we must have very early > i2c bus ready and usable ... > > It maybe a good plus to add i2c busses/adapters dynamically after > relocation is done and we have enough RAM ... I tried such an > approach here: > > http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4 > > but this is an old state (only rebased a long time), > but it is maybe a good base for doing such an feature > if it is needed ... I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.
Hi Stephen, On Wed, Oct 31, 2012 at 8:41 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/31/2012 12:00 AM, Heiko Schocher wrote: >> Hello Stephen, >> >> On 30.10.2012 23:32, Stephen Warren wrote: >>> On 10/30/2012 11:28 AM, Simon Glass wrote: >>>> (just for illustration, please don't merge) >>>> >>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and >>>> the Tegra >>>> i2c driver to support this. >>> >>>> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c >>> >>>> +#ifdef CONFIG_TEGRA_I2C >>>> +extern struct i2c_adapter tegra_i2c_adap[]; >>>> +#endif >>> >>> I'm not sure why that's needed if the config files have to put the >>> adpater list into a #define: >>> >>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >>> >>>> +#define CONFIG_SYS_I2C >>>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >>> >>> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >>> functions (which presumably would be called from board code or as a >>> result of DT parsing) dynamically register themselves? >> >> This would not work, if we are running for example from flash on >> powerpc before relocation. There some boards need to read RAM >> infos from an SPD Eeprom through i2c ... so we must have very early >> i2c bus ready and usable ... >> >> It maybe a good plus to add i2c busses/adapters dynamically after >> relocation is done and we have enough RAM ... I tried such an >> approach here: >> >> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4 >> >> but this is an old state (only rebased a long time), >> but it is maybe a good base for doing such an feature >> if it is needed ... > > I mainly ask because Simon is pushing to have Tegra's U-Boot completely > driven by device tree. If we need to hard-code the list of enabled I2C > adapters in the U-Boot config file, and don't also support dynamically > added I2C drivers, then that will be incompatible with device tree. Mostly, although with the serial console (which had a similar problem) we just decoded the information onto the stack as needed. It was inefficient, but worked fine for a small number of operations. We might be able to do better with pre-relocation malloc() soon. Regards, Simon
On 10/31/2012 09:56 AM, Simon Glass wrote: > Hi Stephen, > > On Wed, Oct 31, 2012 at 8:41 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 10/31/2012 12:00 AM, Heiko Schocher wrote: >>> Hello Stephen, >>> >>> On 30.10.2012 23:32, Stephen Warren wrote: >>>> On 10/30/2012 11:28 AM, Simon Glass wrote: ... >>>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h ... >>>>> +#define CONFIG_SYS_I2C >>>>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>>>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >>>> >>>> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >>>> functions (which presumably would be called from board code or as a >>>> result of DT parsing) dynamically register themselves? ... >> I mainly ask because Simon is pushing to have Tegra's U-Boot completely >> driven by device tree. If we need to hard-code the list of enabled I2C >> adapters in the U-Boot config file, and don't also support dynamically >> added I2C drivers, then that will be incompatible with device tree. > > Mostly, although with the serial console (which had a similar problem) > we just decoded the information onto the stack as needed. It was > inefficient, but worked fine for a small number of operations. We > might be able to do better with pre-relocation malloc() soon. It's more complex than that. If we use DT, we must be able to: a) Configure exactly which I2C instances the board uses from DT. b) Provide configuration (e.g. max clock rate) for those instances from DT. If all in-use I2C adapters must be specified statically in CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any arbitrary subset of Tegra's I2C adapters, then we must always set CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining the maximal set of I2C adapters that a board can enable, which means DT isn't specifying that, but rather the board config file is, and hence it's pointless to even use DT for this purpose. Now, most boards won't use all I2C adapters, so presumably the Tegra I2C init routine will look for status="disabled" (or the inverse) in DT, and only initialize if the DT node for the adapter is present and enabled. However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not enabled, which will presumably influence the I2C bus numbering in the U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.
Hello Stephen, On 31.10.2012 17:25, Stephen Warren wrote: > On 10/31/2012 09:56 AM, Simon Glass wrote: >> Hi Stephen, >> >> On Wed, Oct 31, 2012 at 8:41 AM, Stephen Warren<swarren@wwwdotorg.org> wrote: >>> On 10/31/2012 12:00 AM, Heiko Schocher wrote: >>>> Hello Stephen, >>>> >>>> On 30.10.2012 23:32, Stephen Warren wrote: >>>>> On 10/30/2012 11:28 AM, Simon Glass wrote: > ... >>>>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h > ... >>>>>> +#define CONFIG_SYS_I2C >>>>>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>>>>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >>>>> >>>>> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >>>>> functions (which presumably would be called from board code or as a >>>>> result of DT parsing) dynamically register themselves? > ... >>> I mainly ask because Simon is pushing to have Tegra's U-Boot completely >>> driven by device tree. If we need to hard-code the list of enabled I2C >>> adapters in the U-Boot config file, and don't also support dynamically >>> added I2C drivers, then that will be incompatible with device tree. >> >> Mostly, although with the serial console (which had a similar problem) >> we just decoded the information onto the stack as needed. It was >> inefficient, but worked fine for a small number of operations. We >> might be able to do better with pre-relocation malloc() soon. > > It's more complex than that. > > If we use DT, we must be able to: > > a) Configure exactly which I2C instances the board uses from DT. > b) Provide configuration (e.g. max clock rate) for those instances from DT. > > If all in-use I2C adapters must be specified statically in > CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any > arbitrary subset of Tegra's I2C adapters, then we must always set > CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we > put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining > the maximal set of I2C adapters that a board can enable, which means DT > isn't specifying that, but rather the board config file is, and hence > it's pointless to even use DT for this purpose. My current approach needs static specific adapters, yes. But I see not the problem, if we define all tegra adapters per default and ... > Now, most boards won't use all I2C adapters, so presumably the Tegra I2C > init routine will look for status="disabled" (or the inverse) in DT, and > only initialize if the DT node for the adapter is present and enabled. ... and it should here be possible to add the used "i2c busses"! dynamically from the info in the DT, or? We use in U-Boot not direct the i2c adapters, instead i2c busses ... so if we define all 4 tegra i2c adapters per default, but using on one board only adapter 1 and 3 we have two i2c busses: 0 (= adapter 1) and 1 (= adapter 3) ... no gaps between ...) > However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not > enabled, which will presumably influence the I2C bus numbering in the No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS define, without really use them in CONFIG_SYS_I2C_BUSSES. And it should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ... something like I did in http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 function: int i2c_add_one_bus(char *buf) -> while interpreting DT i2c info for above board will result in calling: i2c_add_one_bus("tegra-i2c-1"); i2c_add_one_bus("tegra-i2c-3"); and results in two new i2c busses 0 and 1 ... Maybe this is a way to go? > U-Boot shell; there will be I2C busses that exist but cannot be used. Is > this what we want? Perhaps it is in fact a good idea to always make the Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ... > U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps > when some ports aren't enabled? That would be a departure from the way > USB is handled today though. Hmm.. but is this possible? If we have a board with 2 (or more) different i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ... bye, Heiko
On 11/01/2012 01:42 AM, Heiko Schocher wrote: > Hello Stephen, > > On 31.10.2012 17:25, Stephen Warren wrote: >> On 10/31/2012 09:56 AM, Simon Glass wrote: >>> Hi Stephen, >>> >>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen >>> Warren<swarren@wwwdotorg.org> wrote: >>>> On 10/31/2012 12:00 AM, Heiko Schocher wrote: >>>>> Hello Stephen, >>>>> >>>>> On 30.10.2012 23:32, Stephen Warren wrote: >>>>>> On 10/30/2012 11:28 AM, Simon Glass wrote: >> ... >>>>>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >> ... >>>>>>> +#define CONFIG_SYS_I2C >>>>>>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>>>>>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >>>>>> >>>>>> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >>>>>> functions (which presumably would be called from board code or as a >>>>>> result of DT parsing) dynamically register themselves? >> ... >>>> I mainly ask because Simon is pushing to have Tegra's U-Boot completely >>>> driven by device tree. If we need to hard-code the list of enabled I2C >>>> adapters in the U-Boot config file, and don't also support dynamically >>>> added I2C drivers, then that will be incompatible with device tree. >>> >>> Mostly, although with the serial console (which had a similar problem) >>> we just decoded the information onto the stack as needed. It was >>> inefficient, but worked fine for a small number of operations. We >>> might be able to do better with pre-relocation malloc() soon. >> >> It's more complex than that. >> >> If we use DT, we must be able to: >> >> a) Configure exactly which I2C instances the board uses from DT. >> b) Provide configuration (e.g. max clock rate) for those instances >> from DT. >> >> If all in-use I2C adapters must be specified statically in >> CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any >> arbitrary subset of Tegra's I2C adapters, then we must always set >> CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we >> put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining >> the maximal set of I2C adapters that a board can enable, which means DT >> isn't specifying that, but rather the board config file is, and hence >> it's pointless to even use DT for this purpose. > > My current approach needs static specific adapters, yes. But I see not > the problem, if we define all tegra adapters per default and ... > >> Now, most boards won't use all I2C adapters, so presumably the Tegra I2C >> init routine will look for status="disabled" (or the inverse) in DT, and >> only initialize if the DT node for the adapter is present and enabled. > > ... and it should here be possible to add the used "i2c busses"! > dynamically from the info in the DT, or? Does the I2C adapter registration function call an I2C core function to dynamically register the actual buses that exist on the system? If so, then everything seems fine. However, your later responses re: CONFIG_SYS_I2C_BUSSES then would confuse me... > We use in U-Boot not direct the i2c adapters, instead i2c busses ... > so if we define all 4 tegra i2c adapters per default, but using on > one board only adapter 1 and 3 we have two i2c busses: > 0 (= adapter 1) and > 1 (= adapter 3) ... > no gaps between ...) > >> However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not >> enabled, which will presumably influence the I2C bus numbering in the > > No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS > define, without really use them in CONFIG_SYS_I2C_BUSSES. And it Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized, and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a particular adapter, or could end up defining multiple buses that use a particular bus (in the presence of a mux). That all seems fine, but... However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at compile time, not something dynamic. If that's true, then I'll just re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I said "CONFIG_SYS_I2C_ADAPTERS"... > should be possible (not yet coded, but tried in an older version) to add > i2c busses after relocation, or while interpret DT ... > > something like I did in > http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this dynamic in the long run we should just make it dynamic from the start as part of the API rework. If we don't, we'll just have to do another pass over all the drivers converting them to dynamic registration later anyway. I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization. We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions. Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. BTW, isn't buses spelled "buses" not "busses"? Thunderbird's spell-checker certainly thinks so! > function: > int i2c_add_one_bus(char *buf) > > -> while interpreting DT i2c info for above board will result in calling: > i2c_add_one_bus("tegra-i2c-1"); > i2c_add_one_bus("tegra-i2c-3"); > > and results in two new i2c busses 0 and 1 ... > Maybe this is a way to go? Yes, that sounds like the right kind of direction. >> U-Boot shell; there will be I2C busses that exist but cannot be used. Is >> this what we want? Perhaps it is in fact a good idea to always make the > > Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have > some i2c adapters which are defined but (maybe) not used ... > >> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps >> when some ports aren't enabled? That would be a departure from the way >> USB is handled today though. > > Hmm.. but is this possible? If we have a board with 2 (or more) different > i2c adapters (which is now possible with the new framework), for example > 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra > adapters are the first 4 i2c busses, so we cannot longer say the > two soft i2c adapters are starting from 0 too (and vice versa) ... I was talking about the I2C HW in the SoC itself. There isn't any concept of an expected I2C bus/adapter ID for anything outside the SoC.
Hi, On Thu, Nov 1, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 11/01/2012 01:42 AM, Heiko Schocher wrote: >> Hello Stephen, >> >> On 31.10.2012 17:25, Stephen Warren wrote: >>> On 10/31/2012 09:56 AM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen >>>> Warren<swarren@wwwdotorg.org> wrote: >>>>> On 10/31/2012 12:00 AM, Heiko Schocher wrote: >>>>>> Hello Stephen, >>>>>> >>>>>> On 30.10.2012 23:32, Stephen Warren wrote: >>>>>>> On 10/30/2012 11:28 AM, Simon Glass wrote: >>> ... >>>>>>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >>> ... >>>>>>>> +#define CONFIG_SYS_I2C >>>>>>>> +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} >>>>>>>> +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS >>>>>>> >>>>>>> But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init >>>>>>> functions (which presumably would be called from board code or as a >>>>>>> result of DT parsing) dynamically register themselves? >>> ... >>>>> I mainly ask because Simon is pushing to have Tegra's U-Boot completely >>>>> driven by device tree. If we need to hard-code the list of enabled I2C >>>>> adapters in the U-Boot config file, and don't also support dynamically >>>>> added I2C drivers, then that will be incompatible with device tree. >>>> >>>> Mostly, although with the serial console (which had a similar problem) >>>> we just decoded the information onto the stack as needed. It was >>>> inefficient, but worked fine for a small number of operations. We >>>> might be able to do better with pre-relocation malloc() soon. >>> >>> It's more complex than that. >>> >>> If we use DT, we must be able to: >>> >>> a) Configure exactly which I2C instances the board uses from DT. >>> b) Provide configuration (e.g. max clock rate) for those instances >>> from DT. >>> >>> If all in-use I2C adapters must be specified statically in >>> CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any >>> arbitrary subset of Tegra's I2C adapters, then we must always set >>> CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we >>> put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining >>> the maximal set of I2C adapters that a board can enable, which means DT >>> isn't specifying that, but rather the board config file is, and hence >>> it's pointless to even use DT for this purpose. >> >> My current approach needs static specific adapters, yes. But I see not >> the problem, if we define all tegra adapters per default and ... >> >>> Now, most boards won't use all I2C adapters, so presumably the Tegra I2C >>> init routine will look for status="disabled" (or the inverse) in DT, and >>> only initialize if the DT node for the adapter is present and enabled. >> >> ... and it should here be possible to add the used "i2c busses"! >> dynamically from the info in the DT, or? > > Does the I2C adapter registration function call an I2C core function to > dynamically register the actual buses that exist on the system? If so, > then everything seems fine. > > However, your later responses re: CONFIG_SYS_I2C_BUSSES then would > confuse me... > >> We use in U-Boot not direct the i2c adapters, instead i2c busses ... >> so if we define all 4 tegra i2c adapters per default, but using on >> one board only adapter 1 and 3 we have two i2c busses: >> 0 (= adapter 1) and >> 1 (= adapter 3) ... >> no gaps between ...) >> >>> However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not >>> enabled, which will presumably influence the I2C bus numbering in the >> >> No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS >> define, without really use them in CONFIG_SYS_I2C_BUSSES. And it > > Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized, > and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so > CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a > particular adapter, or could end up defining multiple buses that use a > particular bus (in the presence of a mux). That all seems fine, but... > > However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at > compile time, not something dynamic. If that's true, then I'll just > re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I > said "CONFIG_SYS_I2C_ADAPTERS"... > >> should be possible (not yet coded, but tried in an older version) to add >> i2c busses after relocation, or while interpret DT ... >> >> something like I did in >> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 > > I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or > CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this > dynamic in the long run we should just make it dynamic from the start as > part of the API rework. If we don't, we'll just have to do another pass > over all the drivers converting them to dynamic registration later anyway. > > I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each > driver registers an arbitrary number of adapters and/or buses during its > initialization. > > We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; > just have each driver define a structure that gets put into a special > linker section, so that the I2C core can iterate over all linked drivers > at boot time and call their initialization functions. > > Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we > should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. > > BTW, isn't buses spelled "buses" not "busses"? Thunderbird's > spell-checker certainly thinks so! > >> function: >> int i2c_add_one_bus(char *buf) >> >> -> while interpreting DT i2c info for above board will result in calling: >> i2c_add_one_bus("tegra-i2c-1"); >> i2c_add_one_bus("tegra-i2c-3"); >> >> and results in two new i2c busses 0 and 1 ... >> Maybe this is a way to go? > > Yes, that sounds like the right kind of direction. Well it certainly is useful to have dynamic I2C, but it might be tricky until we have a pre-reloc malloc()? We don't need to do everything at once. It would be nice to get the adapter parameter in there though, to avoid two steps of driver change. But I would encourage you Heiko to get this tidied up a bit and merged, since it is already a big step forward. > >>> U-Boot shell; there will be I2C busses that exist but cannot be used. Is >>> this what we want? Perhaps it is in fact a good idea to always make the >> >> Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have >> some i2c adapters which are defined but (maybe) not used ... >> >>> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps >>> when some ports aren't enabled? That would be a departure from the way >>> USB is handled today though. >> >> Hmm.. but is this possible? If we have a board with 2 (or more) different >> i2c adapters (which is now possible with the new framework), for example >> 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra >> adapters are the first 4 i2c busses, so we cannot longer say the >> two soft i2c adapters are starting from 0 too (and vice versa) ... > > I was talking about the I2C HW in the SoC itself. There isn't any > concept of an expected I2C bus/adapter ID for anything outside the SoC. Well the device tree should take care of this (in the alias section). The numbering of i2c ports should be done that way. See Tegra USB for how this works, as an example (board_usb_init() calls fdtdec_find_aliases_for_id(), then add_port() to add each of the ports it finds). Regards, Simon
Hello Stephen, On 01.11.2012 18:03, Stephen Warren wrote: > On 11/01/2012 01:42 AM, Heiko Schocher wrote: >> Hello Stephen, >> >> On 31.10.2012 17:25, Stephen Warren wrote: >>> On 10/31/2012 09:56 AM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen >>>> Warren<swarren@wwwdotorg.org> wrote: >>>>> On 10/31/2012 12:00 AM, Heiko Schocher wrote: [...] >>> If all in-use I2C adapters must be specified statically in >>> CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any >>> arbitrary subset of Tegra's I2C adapters, then we must always set >>> CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we >>> put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining >>> the maximal set of I2C adapters that a board can enable, which means DT >>> isn't specifying that, but rather the board config file is, and hence >>> it's pointless to even use DT for this purpose. >> >> My current approach needs static specific adapters, yes. But I see not >> the problem, if we define all tegra adapters per default and ... >> >>> Now, most boards won't use all I2C adapters, so presumably the Tegra I2C >>> init routine will look for status="disabled" (or the inverse) in DT, and >>> only initialize if the DT node for the adapter is present and enabled. >> >> ... and it should here be possible to add the used "i2c busses"! >> dynamically from the info in the DT, or? > > Does the I2C adapter registration function call an I2C core function to > dynamically register the actual buses that exist on the system? If so, > then everything seems fine. > > However, your later responses re: CONFIG_SYS_I2C_BUSSES then would > confuse me... > >> We use in U-Boot not direct the i2c adapters, instead i2c busses ... >> so if we define all 4 tegra i2c adapters per default, but using on >> one board only adapter 1 and 3 we have two i2c busses: >> 0 (= adapter 1) and >> 1 (= adapter 3) ... >> no gaps between ...) >> >>> However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not >>> enabled, which will presumably influence the I2C bus numbering in the >> >> No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS >> define, without really use them in CONFIG_SYS_I2C_BUSSES. And it > > Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized, CONFIG_SYS_I2C_ADAPTERS defines which i2c adapters (=drivers) (maybe) used on this board. > and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so > CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a > particular adapter, or could end up defining multiple buses that use a > particular bus (in the presence of a mux). That all seems fine, but... > > However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at > compile time, not something dynamic. If that's true, then I'll just > re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I > said "CONFIG_SYS_I2C_ADAPTERS"... Yes, currently static ... >> should be possible (not yet coded, but tried in an older version) to add >> i2c busses after relocation, or while interpret DT ... >> >> something like I did in >> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 > > I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or > CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this > dynamic in the long run we should just make it dynamic from the start as > part of the API rework. If we don't, we'll just have to do another pass > over all the drivers converting them to dynamic registration later anyway. This patchserie is thought only for static configuration (as current state is) ... But you can post patches for such a solution ... and we can move this way! > I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each > driver registers an arbitrary number of adapters and/or buses during its > initialization. Why should an i2c driver register buses? That is board specific. > We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; > just have each driver define a structure that gets put into a special > linker section, so that the I2C core can iterate over all linked drivers > at boot time and call their initialization functions. > > Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we > should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. How do we this, for example on some powerpc boards, which boot from NOR flash and read a SPD EEprom data for RAM initialization (Note: we have here not a lot of stack, nor RAM)? Maybe we should wait with this hole patchserie until DM is ready? (And maybe rework it completly?) > BTW, isn't buses spelled "buses" not "busses"? Thunderbird's > spell-checker certainly thinks so! > >> function: >> int i2c_add_one_bus(char *buf) >> >> -> while interpreting DT i2c info for above board will result in calling: >> i2c_add_one_bus("tegra-i2c-1"); >> i2c_add_one_bus("tegra-i2c-3"); >> >> and results in two new i2c busses 0 and 1 ... >> Maybe this is a way to go? > > Yes, that sounds like the right kind of direction. Ok. >>> U-Boot shell; there will be I2C busses that exist but cannot be used. Is >>> this what we want? Perhaps it is in fact a good idea to always make the >> >> Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have >> some i2c adapters which are defined but (maybe) not used ... >> >>> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps >>> when some ports aren't enabled? That would be a departure from the way >>> USB is handled today though. >> >> Hmm.. but is this possible? If we have a board with 2 (or more) different >> i2c adapters (which is now possible with the new framework), for example >> 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra >> adapters are the first 4 i2c busses, so we cannot longer say the >> two soft i2c adapters are starting from 0 too (and vice versa) ... > > I was talking about the I2C HW in the SoC itself. There isn't any > concept of an expected I2C bus/adapter ID for anything outside the SoC. What do you mean here? There is such a concept when using multibus. We have an ID for each bus. This patchserie just add the possibility that we can use now more than one hw/sw adapter, introduce an i2c_core file and reworks the i2c mux functionality ... and in the end we will get rid of the defines HARD_I2C/SOFT_I2C, all i2c driver moved to drivers/i2c ... bye, Heiko
Hello Simon, On 05.11.2012 21:39, Simon Glass wrote: > Hi, > > On Thu, Nov 1, 2012 at 10:03 AM, Stephen Warren<swarren@wwwdotorg.org> wrote: >> On 11/01/2012 01:42 AM, Heiko Schocher wrote: >>> Hello Stephen, >>> >>> On 31.10.2012 17:25, Stephen Warren wrote: >>>> On 10/31/2012 09:56 AM, Simon Glass wrote: >>>>> Hi Stephen, >>>>> >>>>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen [...] >>> should be possible (not yet coded, but tried in an older version) to add >>> i2c busses after relocation, or while interpret DT ... >>> >>> something like I did in >>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 >> >> I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or >> CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this >> dynamic in the long run we should just make it dynamic from the start as >> part of the API rework. If we don't, we'll just have to do another pass >> over all the drivers converting them to dynamic registration later anyway. >> >> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each >> driver registers an arbitrary number of adapters and/or buses during its >> initialization. >> >> We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; >> just have each driver define a structure that gets put into a special >> linker section, so that the I2C core can iterate over all linked drivers >> at boot time and call their initialization functions. >> >> Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we >> should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. >> >> BTW, isn't buses spelled "buses" not "busses"? Thunderbird's >> spell-checker certainly thinks so! >> >>> function: >>> int i2c_add_one_bus(char *buf) >>> >>> -> while interpreting DT i2c info for above board will result in calling: >>> i2c_add_one_bus("tegra-i2c-1"); >>> i2c_add_one_bus("tegra-i2c-3"); >>> >>> and results in two new i2c busses 0 and 1 ... >>> Maybe this is a way to go? >> >> Yes, that sounds like the right kind of direction. > > Well it certainly is useful to have dynamic I2C, but it might be > tricky until we have a pre-reloc malloc()? Yep ... not only this, we maybe have not enough "RAM" for it, because RAM is not initialized and we are using some SRAM, cache, ... for stack ... (/dreaming on Sometimes I think, we should switch completely to boot basically with SPL for all boards (in SPL we do not need mutliadapter/mutlibus support), so we could start U-Boot directly relocated in RAM from SPL with an arch independent board.c and all RAM availiable, which would solve some problems ... /dreaming off) > We don't need to do everything at once. It would be nice to get the > adapter parameter in there though, to avoid two steps of driver did this, see: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20121030 > change. But I would encourage you Heiko to get this tidied up a bit > and merged, since it is already a big step forward. Ok, we should vote here, if we a) go ahead with this patchset b) wait for DM is ready c) rework this patchserie here completely new (dynamic registration) as stephen suggested >>>> U-Boot shell; there will be I2C busses that exist but cannot be used. Is >>>> this what we want? Perhaps it is in fact a good idea to always make the >>> >>> Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have >>> some i2c adapters which are defined but (maybe) not used ... >>> >>>> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps >>>> when some ports aren't enabled? That would be a departure from the way >>>> USB is handled today though. >>> >>> Hmm.. but is this possible? If we have a board with 2 (or more) different >>> i2c adapters (which is now possible with the new framework), for example >>> 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra >>> adapters are the first 4 i2c busses, so we cannot longer say the >>> two soft i2c adapters are starting from 0 too (and vice versa) ... >> >> I was talking about the I2C HW in the SoC itself. There isn't any >> concept of an expected I2C bus/adapter ID for anything outside the SoC. > > Well the device tree should take care of this (in the alias section). > The numbering of i2c ports should be done that way. See Tegra USB for > how this works, as an example (board_usb_init() calls > fdtdec_find_aliases_for_id(), then add_port() to add each of the ports > it finds). bye, Heiko
On 11/07/2012 11:47 PM, Heiko Schocher wrote: > On 01.11.2012 18:03, Stephen Warren wrote: ... >> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each >> driver registers an arbitrary number of adapters and/or buses during its >> initialization. > > Why should an i2c driver register buses? That is board specific. I don't entirely agree here. Certainly the information is board-specific, but I don't believe that precludes bus registration occurring from the I2C adapter drivers themselves, based on information passed from a board file or device tree. If a particular adapter is instantiated by the board, then there is clearly an I2C bus attached to that adapter. Hence, it's quite reasonable for the adapter itself to register the bus directly attached to it. Following on from there, if there's an I2C bus mux attached to some I2C bus, then there are clearly I2C buses downstream from the bus mux, and the bus mux driver knows exactly how many there are, and can register those buses. This can continue through a whole tree of I2C adapters/muxes/buses. The above model fits into device tree very well; in that case, you only find out about the existence of downstream muxes etc. once you've initialized the I2C adapter for the parent bus. Equally, I think this model can work very well for manually coded board files; the board file can directly instantiate all top-level I2C adapters, and pass information to the I2C driver for those adapters indicating which I2C devices are attached to the buses. Then, if one of those devices happens to be an I2C bus mux, it can instantiate further I2C buses, which in turn instantiate more devices based on the parameters passed to the I2C bus mux driver. >> We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; >> just have each driver define a structure that gets put into a special >> linker section, so that the I2C core can iterate over all linked drivers >> at boot time and call their initialization functions. >> >> Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we >> should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. > > How do we this, for example on some powerpc boards, which boot from > NOR flash and read a SPD EEprom data for RAM initialization (Note: we > have here not a lot of stack, nor RAM)? Maybe we should wait with this > hole patchserie until DM is ready? (And maybe rework it completly?) I'd expect SPD initialization to be a special case in a U-Boot SPL; isn't that its purpose?
Hi Heiko, On Wed, Nov 7, 2012 at 11:02 PM, Heiko Schocher <hs@denx.de> wrote: > Hello Simon, > > > On 05.11.2012 21:39, Simon Glass wrote: >> >> Hi, >> >> On Thu, Nov 1, 2012 at 10:03 AM, Stephen Warren<swarren@wwwdotorg.org> >> wrote: >>> >>> On 11/01/2012 01:42 AM, Heiko Schocher wrote: >>>> >>>> Hello Stephen, >>>> >>>> On 31.10.2012 17:25, Stephen Warren wrote: >>>>> >>>>> On 10/31/2012 09:56 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Stephen, >>>>>> >>>>>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen > > [...] > >>>> should be possible (not yet coded, but tried in an older version) to add >>>> i2c busses after relocation, or while interpret DT ... >>>> >>>> something like I did in >>>> >>>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13 >>> >>> >>> I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or >>> CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this >>> dynamic in the long run we should just make it dynamic from the start as >>> part of the API rework. If we don't, we'll just have to do another pass >>> over all the drivers converting them to dynamic registration later >>> anyway. >>> >>> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each >>> driver registers an arbitrary number of adapters and/or buses during its >>> initialization. >>> >>> We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; >>> just have each driver define a structure that gets put into a special >>> linker section, so that the I2C core can iterate over all linked drivers >>> at boot time and call their initialization functions. >>> >>> Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we >>> should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. >>> >>> BTW, isn't buses spelled "buses" not "busses"? Thunderbird's >>> spell-checker certainly thinks so! >>> >>>> function: >>>> int i2c_add_one_bus(char *buf) >>>> >>>> -> while interpreting DT i2c info for above board will result in >>>> calling: >>>> i2c_add_one_bus("tegra-i2c-1"); >>>> i2c_add_one_bus("tegra-i2c-3"); >>>> >>>> and results in two new i2c busses 0 and 1 ... >>>> Maybe this is a way to go? >>> >>> >>> Yes, that sounds like the right kind of direction. >> >> >> Well it certainly is useful to have dynamic I2C, but it might be >> tricky until we have a pre-reloc malloc()? > > > Yep ... not only this, we maybe have not enough "RAM" for it, because > RAM is not initialized and we are using some SRAM, cache, ... for > stack ... > > (/dreaming on > Sometimes I think, we should switch completely to boot basically with > SPL for all boards (in SPL we do not need mutliadapter/mutlibus support), > so we could start U-Boot directly relocated in RAM from SPL with an arch > independent board.c and all RAM availiable, which would solve some > problems ... > /dreaming off) > > >> We don't need to do everything at once. It would be nice to get the >> adapter parameter in there though, to avoid two steps of driver > > > did this, see: > > http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20121030 > > >> change. But I would encourage you Heiko to get this tidied up a bit >> and merged, since it is already a big step forward. > > > Ok, we should vote here, if we > > a) go ahead with this patchset > b) wait for DM is ready > c) rework this patchserie here completely new (dynamic registration) > as stephen suggested I vote for a. It is a big step forward, and we can then build on that for dynamic registration, possibly making a few more changes as needed. Since most drivers will not convert immediately, this should have little impact on everyone else. I don't see a huge benefit to doing dynamic registration until DM is ready. At best it will just hold us up, at worst it will be thrown out when DM arrives. Regards, Simon > > >>>>> U-Boot shell; there will be I2C busses that exist but cannot be used. >>>>> Is >>>>> this what we want? Perhaps it is in fact a good idea to always make the >>>> >>>> >>>> Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have >>>> some i2c adapters which are defined but (maybe) not used ... >>>> >>>>> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave >>>>> gaps >>>>> when some ports aren't enabled? That would be a departure from the way >>>>> USB is handled today though. >>>> >>>> >>>> Hmm.. but is this possible? If we have a board with 2 (or more) >>>> different >>>> i2c adapters (which is now possible with the new framework), for example >>>> 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra >>>> adapters are the first 4 i2c busses, so we cannot longer say the >>>> two soft i2c adapters are starting from 0 too (and vice versa) ... >>> >>> >>> I was talking about the I2C HW in the SoC itself. There isn't any >>> concept of an expected I2C bus/adapter ID for anything outside the SoC. >> >> >> Well the device tree should take care of this (in the alias section). >> The numbering of i2c ports should be done that way. See Tegra USB for >> how this works, as an example (board_usb_init() calls >> fdtdec_find_aliases_for_id(), then add_port() to add each of the ports >> it finds). > > > bye, > Heiko > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hello Stephen, On 08.11.2012 18:05, Stephen Warren wrote: > On 11/07/2012 11:47 PM, Heiko Schocher wrote: >> On 01.11.2012 18:03, Stephen Warren wrote: > ... >>> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each >>> driver registers an arbitrary number of adapters and/or buses during its >>> initialization. >> >> Why should an i2c driver register buses? That is board specific. > > I don't entirely agree here. Certainly the information is > board-specific, but I don't believe that precludes bus registration > occurring from the I2C adapter drivers themselves, based on information > passed from a board file or device tree. > > If a particular adapter is instantiated by the board, then there is > clearly an I2C bus attached to that adapter. Hence, it's quite > reasonable for the adapter itself to register the bus directly attached > to it. But some i2c drivers have more than one instance... would you for all boards register all possible instances of a driver? ... A lot of boards do not use the multibus feature, and so they use only one i2c driver ... on this boards we would register maybe a lot of buses for nothing ... > Following on from there, if there's an I2C bus mux attached to some I2C > bus, then there are clearly I2C buses downstream from the bus mux, and > the bus mux driver knows exactly how many there are, and can register > those buses. Here again, why register all possible buses? We are just a bootloader ... > This can continue through a whole tree of I2C adapters/muxes/buses. > > The above model fits into device tree very well; in that case, you only > find out about the existence of downstream muxes etc. once you've > initialized the I2C adapter for the parent bus. > > Equally, I think this model can work very well for manually coded board > files; the board file can directly instantiate all top-level I2C > adapters, and pass information to the I2C driver for those adapters > indicating which I2C devices are attached to the buses. Then, if one of > those devices happens to be an I2C bus mux, it can instantiate further > I2C buses, which in turn instantiate more devices based on the > parameters passed to the I2C bus mux driver. Yep, possible ... but another approach ... >>> We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; >>> just have each driver define a structure that gets put into a special >>> linker section, so that the I2C core can iterate over all linked drivers >>> at boot time and call their initialization functions. >>> >>> Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we >>> should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically. >> >> How do we this, for example on some powerpc boards, which boot from >> NOR flash and read a SPD EEprom data for RAM initialization (Note: we >> have here not a lot of stack, nor RAM)? Maybe we should wait with this >> hole patchserie until DM is ready? (And maybe rework it completly?) > > I'd expect SPD initialization to be a special case in a U-Boot SPL; > isn't that its purpose? We have boards which are booting from NOR flash and reading a SPD EEprom without using SPL ... bye, Heiko
Dear Heiko, In message <50A1E780.4090807@denx.de> you wrote: > > > Equally, I think this model can work very well for manually coded board > > files; the board file can directly instantiate all top-level I2C > > adapters, and pass information to the I2C driver for those adapters > > indicating which I2C devices are attached to the buses. Then, if one of > > those devices happens to be an I2C bus mux, it can instantiate further > > I2C buses, which in turn instantiate more devices based on the > > parameters passed to the I2C bus mux driver. > > Yep, possible ... but another approach ... Thanks for pointing this out. Stephen, the old design principle [1] to initialize devices only when they are needed within U-Boot itself still applies. We do not want to initialize everything we can, but only what we really need ourself. Quote: Perfection is reached, not when there is no longer anything to add, but when there is no longer anything to take away. - Antoine de Saint-Exupery [1] See bullet # 2 at http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast Best regards, Wolfgang Denk
On 11/12/2012 11:24 PM, Heiko Schocher wrote: > Hello Stephen, > > On 08.11.2012 18:05, Stephen Warren wrote: >> On 11/07/2012 11:47 PM, Heiko Schocher wrote: >>> On 01.11.2012 18:03, Stephen Warren wrote: >> ... >>>> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each >>>> driver registers an arbitrary number of adapters and/or buses during >>>> its >>>> initialization. >>> >>> Why should an i2c driver register buses? That is board specific. >> >> I don't entirely agree here. Certainly the information is >> board-specific, but I don't believe that precludes bus registration >> occurring from the I2C adapter drivers themselves, based on information >> passed from a board file or device tree. >> >> If a particular adapter is instantiated by the board, then there is >> clearly an I2C bus attached to that adapter. Hence, it's quite >> reasonable for the adapter itself to register the bus directly attached >> to it. > > But some i2c drivers have more than one instance... would you for all > boards register all possible instances of a driver? ... Register, but perhaps not initialize, seems fine to me. After all, what if the user wishes to use the I2C adapters for some custom command; up-front registration of all valid adapters is required for that, although actual initialization can always be deferred until if/when the adapter is actually used. The other issue is that as I pointed out before, the move to define which I2C (and all other peripheral) adapters are used via device tree rather than board/config files on some U-Boot platforms seems completely at odds with not always registering all I2C adapters, at least on the platforms that use device tree. >> Following on from there, if there's an I2C bus mux attached to some I2C >> bus, then there are clearly I2C buses downstream from the bus mux, and >> the bus mux driver knows exactly how many there are, and can register >> those buses. > > Here again, why register all possible buses? We are just a bootloader ... I don't see how being a bootloader is relevant. After all, people can use for example U-Boot I2C or GPIO commands for board bringup, to implement HW initialization via custom boot scripts, etc.
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index 5a738d5..44f1297 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -9,6 +9,9 @@ #ifdef CONFIG_SYS_I2C_SOFT extern struct i2c_adapter soft_i2c_adap[]; #endif +#ifdef CONFIG_TEGRA_I2C +extern struct i2c_adapter tegra_i2c_adap[]; +#endif struct i2c_adapter *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] = CONFIG_SYS_I2C_ADAPTERS; diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index 3e157f4..b92bcf1 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -35,8 +35,6 @@ DECLARE_GLOBAL_DATA_PTR; -static unsigned int i2c_bus_num; - /* Information about i2c controller */ struct i2c_bus { int id; @@ -327,21 +325,11 @@ static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num) return bus; } -unsigned int i2c_get_bus_speed(void) +static unsigned int tegra_i2c_set_bus_speed(unsigned int speed) { struct i2c_bus *bus; - bus = tegra_i2c_get_bus(i2c_bus_num); - if (!bus) - return 0; - return bus->speed; -} - -int i2c_set_bus_speed(unsigned int speed) -{ - struct i2c_bus *bus; - - bus = tegra_i2c_get_bus(i2c_bus_num); + bus = tegra_i2c_get_bus(i2c_get_bus_num()); if (!bus) return 0; bus->speed = speed; @@ -450,7 +438,7 @@ void i2c_init_board(void) return; } -void i2c_init(int speed, int slaveaddr) +static void tegra_i2c_init(int speed, int slaveaddr) { /* This will override the speed selected in the fdt for that port */ debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr); @@ -500,7 +488,7 @@ int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) } /* Probe to see if a chip is present. */ -int i2c_probe(uchar chip) +static int tegra_i2c_probe(uchar chip) { struct i2c_bus *bus; int rc; @@ -526,7 +514,8 @@ static int i2c_addr_ok(const uint addr, const int alen) } /* Read bytes */ -int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) +static int tegra_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, + int len) { struct i2c_bus *bus; uint offset; @@ -534,7 +523,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n", chip, addr, len); - bus = tegra_i2c_get_bus(i2c_bus_num); + bus = tegra_i2c_get_bus(i2c_get_bus_num()); if (!bus) return 1; if (!i2c_addr_ok(addr, alen)) { @@ -564,7 +553,8 @@ 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) +static int tegra_i2c_write(uchar chip, uint addr, int alen, uchar *buffer, + int len) { struct i2c_bus *bus; uint offset; @@ -572,7 +562,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n", chip, addr, len); - bus = tegra_i2c_get_bus(i2c_bus_num); + bus = tegra_i2c_get_bus(i2c_get_bus_num()); if (!bus) return 1; if (!i2c_addr_ok(addr, alen)) { @@ -593,25 +583,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) return 0; } -#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 (bus >= TEGRA_I2C_NUM_CONTROLLERS || !i2c_controllers[bus].inited) - return -1; - i2c_bus_num = bus; - - return 0; -} -#endif - int tegra_i2c_get_dvc_bus_num(void) { int i; @@ -625,3 +596,18 @@ int tegra_i2c_get_dvc_bus_num(void) return -1; } + +struct i2c_adapter tegra_i2c_adap[] = { + { + .init = tegra_i2c_init, + .probe = tegra_i2c_probe, + .read = tegra_i2c_read, + .write = tegra_i2c_write, + .set_bus_speed = tegra_i2c_set_bus_speed, + .speed = 100000, + .slaveaddr = 0, + .name = "tegra-i2c", + .init_done = 0, + .hwadapnr = 0, + } +}; diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h index 0727a4c..61aca66 100644 --- a/include/configs/seaboard.h +++ b/include/configs/seaboard.h @@ -58,10 +58,12 @@ /* I2C */ #define CONFIG_TEGRA_I2C #define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS #define CONFIG_SYS_MAX_I2C_BUS 4 #define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C +#define CONFIG_SYS_I2C +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS /* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/trimslice.h b/include/configs/trimslice.h index eeb0dbe..333930b 100644 --- a/include/configs/trimslice.h +++ b/include/configs/trimslice.h @@ -56,10 +56,12 @@ /* I2C */ #define CONFIG_TEGRA_I2C #define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS #define CONFIG_SYS_MAX_I2C_BUS 4 #define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C +#define CONFIG_SYS_I2C +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS /* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/whistler.h b/include/configs/whistler.h index 1c7803b..22aff0b 100644 --- a/include/configs/whistler.h +++ b/include/configs/whistler.h @@ -48,10 +48,12 @@ /* I2C */ #define CONFIG_TEGRA_I2C #define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS #define CONFIG_SYS_MAX_I2C_BUS 4 #define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C +#define CONFIG_SYS_I2C +#define CONFIG_SYS_I2C_ADAPTERS {&tegra_i2c_adap[0]} +#define CONFIG_SYS_NUM_I2C_ADAPTERS TEGRA_I2C_NUM_CONTROLLERS /* SD/MMC */ #define CONFIG_MMC
(just for illustration, please don't merge) This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra i2c driver to support this. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/i2c/i2c_core.c | 3 ++ drivers/i2c/tegra_i2c.c | 64 +++++++++++++++++-------------------------- include/configs/seaboard.h | 4 ++- include/configs/trimslice.h | 4 ++- include/configs/whistler.h | 4 ++- 5 files changed, 37 insertions(+), 42 deletions(-)