Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On 10/29/2012 11:57 PM, Heiko Schocher wrote:
Hello Stephen,
On 29.10.2012 16:34, Stephen Warren wrote:
...
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
Ah, do you mean we should change the i2c adapter struct from:
struct i2c_adapter {
void(*init)(int speed, int slaveaddr);
int (*probe)(uint8_t chip);
int (*read)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(uint speed);
[...]
to
struct i2c_adapter {
void(*init)(struct i2c_adapter *adap, int speed, int
slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(struct i2c_adapter *adap, uint
speed);
[...]
?
We can do this. Simon suggested this too ...
Yes, exactly. (the functions will need some way to get information out
of the struct i2c_adapter; I assume there's some kind of driver_private
field in there too).
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hi,
On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 10/29/2012 11:57 PM, Heiko Schocher wrote:
Hello Stephen,
On 29.10.2012 16:34, Stephen Warren wrote:
...
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
Ah, do you mean we should change the i2c adapter struct from:
struct i2c_adapter {
void(*init)(int speed, int slaveaddr);
int (*probe)(uint8_t chip);
int (*read)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(uint speed);
[...]
to
struct i2c_adapter {
void(*init)(struct i2c_adapter *adap, int speed, int
slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(struct i2c_adapter *adap, uint
speed);
[...]
?
We can do this. Simon suggested this too ...
Yes, exactly. (the functions will need some way to get information out
of the struct i2c_adapter; I assume there's some kind of driver_private
field in there too).
Yes.
I will post a few patches to move Tegra over to the new framework as
it is, so that people can see the impact. Given that every driver has
to change, it would be my preference to set the i2c API once. But I
suppose within a short while everything will move to the device model,
so perhaps the job of this series is just to move things in the right
direction?
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On Mon, Oct 29, 2012 at 10:53:10AM +0100, Heiko Schocher wrote: Hello Tom, On 26.10.2012 20:44, Tom Rini wrote: On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote: [snip] Later, I wonder whether the concept of a 'current' i2c bus should be maintained by the command line interpreter, rather than the i2c system. Because to be honest, most of the drivers I see have to save the current bus number, set the current bus, do the operation, then set the bus back how they found it (to preserve whatever the user things is the current bus). I agree. Lets move the notion of current to cmd_i2c and make everything internally pass around the bus to operate on. Or try going down this path and find a fatal problem :) As I wrote to simon, stephen, this is an independent problem from the new framework patches! OK. I was hoping we could just: - Change init_func_i2c (for ARM/m68k/ppc/nds32, equiv elsewhere) to init all configured busses. - Change the CLI part to track what bus it is operating on. It sounds like it's more work than just this, really. Since there is agreement to push things further after this update, yes, OK, we can go one step at a time here. Thanks! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Simon,
On 30.10.2012 18:22, Simon Glass wrote:
Hi,
On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warrenswar...@wwwdotorg.org wrote:
On 10/29/2012 11:57 PM, Heiko Schocher wrote:
Hello Stephen,
On 29.10.2012 16:34, Stephen Warren wrote:
...
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
Ah, do you mean we should change the i2c adapter struct from:
struct i2c_adapter {
void(*init)(int speed, int slaveaddr);
int (*probe)(uint8_t chip);
int (*read)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(uint speed);
[...]
to
struct i2c_adapter {
void(*init)(struct i2c_adapter *adap, int speed, int
slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(struct i2c_adapter *adap, uint
speed);
[...]
?
We can do this. Simon suggested this too ...
Yes, exactly. (the functions will need some way to get information out
of the struct i2c_adapter; I assume there's some kind of driver_private
field in there too).
Yes.
I will post a few patches to move Tegra over to the new framework as
it is, so that people can see the impact. Given that every driver has
to change, it would be my preference to set the i2c API once. But I
suppose within a short while everything will move to the device model,
so perhaps the job of this series is just to move things in the right
direction?
Yes, but not only, I think the i2c framework needs an update.
bye,
heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On Tue, Oct 30, 2012 at 10:22:54AM -0700, Simon Glass wrote:
Hi,
On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 10/29/2012 11:57 PM, Heiko Schocher wrote:
Hello Stephen,
On 29.10.2012 16:34, Stephen Warren wrote:
...
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
Ah, do you mean we should change the i2c adapter struct from:
struct i2c_adapter {
void(*init)(int speed, int slaveaddr);
int (*probe)(uint8_t chip);
int (*read)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(uint speed);
[...]
to
struct i2c_adapter {
void(*init)(struct i2c_adapter *adap, int speed, int
slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(struct i2c_adapter *adap, uint
speed);
[...]
?
We can do this. Simon suggested this too ...
Yes, exactly. (the functions will need some way to get information out
of the struct i2c_adapter; I assume there's some kind of driver_private
field in there too).
Yes.
I will post a few patches to move Tegra over to the new framework as
it is, so that people can see the impact. Given that every driver has
to change, it would be my preference to set the i2c API once. But I
suppose within a short while everything will move to the device model,
so perhaps the job of this series is just to move things in the right
direction?
Funny enough I don't see a UDM-i2c.txt file, so I guess the community
gets to think about that one :)
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Simon, On 26.10.2012 18:08, Simon Glass wrote: On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocherh...@denx.de wrote: Hello Simon, On 25.10.2012 23:37, Simon Glass wrote: On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote: [...] 2. The init is a bit odd, in that we can call init() repeatedly. Perhaps that function should be renamed to reset, and the init should be used for calling just once at the start? What do you mean here exactly? I couldn´t parse this ... Well there is start-of-day setup, which I think should be called init. This is done once on boot for each i2c adapter. Hmm... I am not sure if this is only done on boot, because we should close or deinit an adapter if not used any more in U-Boot as the U-Boot principle says: http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast So I want to add in future some deinit to every adapter, and call it from i2c_set_bus() when switching to another i2c adapter ... And then there is the i2c_init() which seems to be called whenever we feel like it - e.g. to change speed. I suggest that we use init() to mean start-of-day init and reset(), or similar, to mean any post-init. I am not suggest that for this series, just as a future effort. Yes, that should be changed. We do not need an init() in the i2c API, as i2c_set_bus_num() do this for us (and later also the deinit()) We just need a set/get_speed() and a deblock()/reset() ? Maybe a step in the API cleanup? 3. Rather than each device having to call i2c_get_bus_num() to find out what bus is referred to, why not just pass this information in? In fact the adapter pointer can serve for this. Not the struct i2c_adapter must passed, but the struct i2c_bus! And each device should know, which i2c bus it uses, or? So at the end we should have something like i2c_read(struct i2c_bus *bus, ...) calls ... and the i2c core can detect, if this bus is the current, if so go on, if not switch to this bus. So at the end i2c_set_bus_num would be go static ... 4. So perhaps the i2c read/write functions should change from: int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) to: int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar *buffer, int len) Yep, exactly, see comments to point 3 ... That would be the best (and I think in previous discussions I defined this as one goal ...), but this would be (another) big change, because this is an *API* change, with maybe a lot of config file changes ... Let us bring in the new i2c framework with all i2c drivers converted, and then do the next step ... maybe one step more, if mareks device model is ready, we can switch easy to DM ... and maybe get this API change for free ... Yes. I certainly understand the need to fit in with what is already there, and avoid a massive API change, which can be performed as a follow-on patch anyway. With your info above I will adjust the tegra driver to work with this and test it. Ok, great! So I post v2 patches after you tested ... And Yes, we should do this API change, but I tend to do this step after the new i2c framework is stable and all i2c drivers are converted to it ... Later, I wonder whether the concept of a 'current' i2c bus should be maintained by the command line interpreter, rather than the i2c system. Because to be honest, most of the drivers I see have to save the current bus number, set the current bus, do the operation, then set the bus back how they found it (to preserve whatever the user things is the current bus). Yes, suboptimal ... but this is independent from the new i2c framework patches! It is possible (with old/new i2c bus framework) to introduce a current commandline i2c bus, and then, before calling i2c_read/write from the commandline, call a i2c_set_bus_num() ... then we can get rid of this store/restore current i2c bus ... waiting for patches ;-) OK. Granted there is overhead with i2c muxes, but the i2c core can remember the state of these muxes and doesn't have to switch things until there has been a change since the last transaction. This exactly do the i2c_set_bus_num() now! Great. This last suggestion can be dealt with later, but I thought I would bring it up. I am happy about every comment! :-) Thanks, Simon bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Stephen,
On 26.10.2012 18:07, Stephen Warren wrote:
On 10/25/2012 11:48 PM, Heiko Schocher wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few
points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
That rather relies on their being a concept of a current I2C adapter.
It seems a little limiting to require that. What if the current
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of current adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.
Yes, you are right, but just the same problem with current code!
You mixed here two things!
The idea behind the current i2c adapter was/is, that the i2c
core need to know, which bus is current because there is the
possibility that on one adapter are more i2c busses, because
of using i2c muxes ... and we must know, on which bus we are
currently, because if we want to switch to another bus, we must
first disable the old way (and maybe disable the i2c adapter too).
- If we want this feature, we need a current adapter. If we say,
Ok, we do not want this disabling... we can get rid of it, yes!
But I think it is safer to disable the i2c muxes, before
switching to another bus ... so this current i2c adapter
is an i2c core internal! We should of course change the i2c
API that we pass the bus to the i2c API, but, as I said this
to simon, this is another big change, and I want to have this
step after getting in the new i2c framework (and maybe hope,
that when we convert to mareks DM, we get this for free),
because we must also adapt for example all dtt, RTC, because
they need to know on which bus they resist, and we have to
config this ...
The problem from storing/restoring the current cmdline i2c
bus, is another problem, which is an independent patch!
You can make with current code a patch, which holds the current
i2c cmdline bus in a variable, and then get rid of this store/
restore calls as for example found in cmd_date.c ...
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Tom, On 26.10.2012 20:44, Tom Rini wrote: On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote: [snip] Later, I wonder whether the concept of a 'current' i2c bus should be maintained by the command line interpreter, rather than the i2c system. Because to be honest, most of the drivers I see have to save the current bus number, set the current bus, do the operation, then set the bus back how they found it (to preserve whatever the user things is the current bus). I agree. Lets move the notion of current to cmd_i2c and make everything internally pass around the bus to operate on. Or try going down this path and find a fatal problem :) As I wrote to simon, stephen, this is an independent problem from the new framework patches! - There are two steps to do: - save the curent cmdline bus in a variable, and call i2c_set_bus_num() before you call i2c_* from the commandline, get rid of the store/restore calls ... - change the i2c API to pass the i2c bus in the i2c_* functions - in the new i2c framework, i2c_set_bus() gets static and the gd-current_i2c_bus is used only in i2c_core.c This two steps can be done in one step, but the second step is complicated enough, so it is better to do it in two steps (I think)! Waiting for patches ;-) The i2c framework change is independent from this! The current_i2c_bus is used i2c_core internally for storing the current active i2c bus, based on the idea of having only one i2c bus active in U-Boot, see therefore the U-Boot principle: http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast Initialize devices only when they are needed within U-Boot, i.e. don't initialize the Ethernet interface(s) unless U-Boot performs [...], etc. (and don't forget to shut down these devices after using them - otherwise nasty things may happen when you try to boot your OS). therefore the current i2c bus is needed! If we want to drop this U-Boot principle, we can get rid of current_i2c_bus ... (Ok, currently U-Boot do not deactivate not used i2c adapters ...) bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hi Heiko, On Mon, Oct 29, 2012 at 2:44 AM, Heiko Schocher h...@denx.de wrote: Hello Simon, On 26.10.2012 18:08, Simon Glass wrote: On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocherh...@denx.de wrote: Hello Simon, On 25.10.2012 23:37, Simon Glass wrote: On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote: [...] 2. The init is a bit odd, in that we can call init() repeatedly. Perhaps that function should be renamed to reset, and the init should be used for calling just once at the start? What do you mean here exactly? I couldn´t parse this ... Well there is start-of-day setup, which I think should be called init. This is done once on boot for each i2c adapter. Hmm... I am not sure if this is only done on boot, because we should close or deinit an adapter if not used any more in U-Boot as the U-Boot principle says: http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast So I want to add in future some deinit to every adapter, and call it from i2c_set_bus() when switching to another i2c adapter ... Well deinit() should be probably be done before finishing U-Boot, not after every transaction, since you don't know that the current transaction will be the last. When using FDT, you need to look up the available i2c ports in the driver, and this should be done once at the start. If the i2c core can call a suitable init function then this is easier, rather than us having to keep track of whether the init is done or not. And then there is the i2c_init() which seems to be called whenever we feel like it - e.g. to change speed. I suggest that we use init() to mean start-of-day init and reset(), or similar, to mean any post-init. I am not suggest that for this series, just as a future effort. Yes, that should be changed. We do not need an init() in the i2c API, as i2c_set_bus_num() do this for us (and later also the deinit()) We just need a set/get_speed() and a deblock()/reset() ? Maybe a step in the API cleanup? Yes, a future step I think. I feel that i2c is one of the darker corners of U-Boot and so am keen to get this tidied up. 3. Rather than each device having to call i2c_get_bus_num() to find out what bus is referred to, why not just pass this information in? In fact the adapter pointer can serve for this. Not the struct i2c_adapter must passed, but the struct i2c_bus! And each device should know, which i2c bus it uses, or? So at the end we should have something like i2c_read(struct i2c_bus *bus, ...) calls ... and the i2c core can detect, if this bus is the current, if so go on, if not switch to this bus. So at the end i2c_set_bus_num would be go static ... 4. So perhaps the i2c read/write functions should change from: int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) to: int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar *buffer, int len) Yep, exactly, see comments to point 3 ... That would be the best (and I think in previous discussions I defined this as one goal ...), but this would be (another) big change, because this is an *API* change, with maybe a lot of config file changes ... Let us bring in the new i2c framework with all i2c drivers converted, and then do the next step ... maybe one step more, if mareks device model is ready, we can switch easy to DM ... and maybe get this API change for free ... Yes. I certainly understand the need to fit in with what is already there, and avoid a massive API change, which can be performed as a follow-on patch anyway. With your info above I will adjust the tegra driver to work with this and test it. Ok, great! So I post v2 patches after you tested ... And Yes, we should do this API change, but I tend to do this step after the new i2c framework is stable and all i2c drivers are converted to it ... [snip] Regards, Simon bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On 10/29/2012 03:47 AM, Heiko Schocher wrote:
Hello Stephen,
On 26.10.2012 18:07, Stephen Warren wrote:
On 10/25/2012 11:48 PM, Heiko Schocher wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few
points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR +
CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR +
CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
That rather relies on their being a concept of a current I2C adapter.
It seems a little limiting to require that. What if the current
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of current adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.
Yes, you are right, but just the same problem with current code!
You mixed here two things!
I think you're reading more into what I was saying than what I actually
said.
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
This is all unrelated to I2C bus muxes; they shouldn't be implemented as
part of an SoC I2C driver anyway, so the driver shouldn't know about bus
muxes before or after this patch - the I2C core should manage that.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hi Stephen,
On Mon, Oct 29, 2012 at 8:34 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 10/29/2012 03:47 AM, Heiko Schocher wrote:
Hello Stephen,
On 26.10.2012 18:07, Stephen Warren wrote:
On 10/25/2012 11:48 PM, Heiko Schocher wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few
points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR +
CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR +
CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
That rather relies on their being a concept of a current I2C adapter.
It seems a little limiting to require that. What if the current
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of current adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.
Yes, you are right, but just the same problem with current code!
You mixed here two things!
I think you're reading more into what I was saying than what I actually
said.
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
I think Heiko agreed with this, just that he wants to take things a
step at a time.
This is all unrelated to I2C bus muxes; they shouldn't be implemented as
part of an SoC I2C driver anyway, so the driver shouldn't know about bus
muxes before or after this patch - the I2C core should manage that.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Simon, On 29.10.2012 14:48, Simon Glass wrote: Hi Heiko, On Mon, Oct 29, 2012 at 2:44 AM, Heiko Schocherh...@denx.de wrote: Hello Simon, On 26.10.2012 18:08, Simon Glass wrote: On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocherh...@denx.de wrote: Hello Simon, On 25.10.2012 23:37, Simon Glass wrote: On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.dewrote: [...] 2. The init is a bit odd, in that we can call init() repeatedly. Perhaps that function should be renamed to reset, and the init should be used for calling just once at the start? What do you mean here exactly? I couldn´t parse this ... Well there is start-of-day setup, which I think should be called init. This is done once on boot for each i2c adapter. Hmm... I am not sure if this is only done on boot, because we should close or deinit an adapter if not used any more in U-Boot as the U-Boot principle says: http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast So I want to add in future some deinit to every adapter, and call it from i2c_set_bus() when switching to another i2c adapter ... Well deinit() should be probably be done before finishing U-Boot, not after every transaction, since you don't know that the current transaction will be the last. Not after every transaction, only when switching to another adapter, or before booting linux ... When using FDT, you need to look up the available i2c ports in the driver, and this should be done once at the start. If the i2c core can call a suitable init function then this is easier, rather than us having to keep track of whether the init is done or not. Dummy question, why is this needed when using FDT? And then there is the i2c_init() which seems to be called whenever we feel like it - e.g. to change speed. I suggest that we use init() to mean start-of-day init and reset(), or similar, to mean any post-init. I am not suggest that for this series, just as a future effort. Yes, that should be changed. We do not need an init() in the i2c API, as i2c_set_bus_num() do this for us (and later also the deinit()) We just need a set/get_speed() and a deblock()/reset() ? Maybe a step in the API cleanup? Yes, a future step I think. I feel that i2c is one of the darker corners of U-Boot and so am keen to get this tidied up. Patches are welcome! Let us bring in the new framework, clean up the i2c API / maybe DM integration, then we are on a good way I think ... [...] bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Stephen,
On 29.10.2012 16:34, Stephen Warren wrote:
On 10/29/2012 03:47 AM, Heiko Schocher wrote:
Hello Stephen,
On 26.10.2012 18:07, Stephen Warren wrote:
On 10/25/2012 11:48 PM, Heiko Schocher wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.dewrote:
[...]
That rather relies on their being a concept of a current I2C adapter.
It seems a little limiting to require that. What if the current
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of current adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.
Yes, you are right, but just the same problem with current code!
You mixed here two things!
I think you're reading more into what I was saying than what I actually
said.
Sorry, maybe I misunderstood you ...
If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.
Ah, do you mean we should change the i2c adapter struct from:
struct i2c_adapter {
void(*init)(int speed, int slaveaddr);
int (*probe)(uint8_t chip);
int (*read)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
int (*write)(uint8_t chip, uint addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(uint speed);
[...]
to
struct i2c_adapter {
void(*init)(struct i2c_adapter *adap, int speed, int
slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip, uint
addr, int alen,
uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip, uint
addr, int alen,
uint8_t *buffer, int len);
uint(*set_bus_speed)(struct i2c_adapter *adap, uint speed);
[...]
?
We can do this. Simon suggested this too ...
@Simon: Is this Ok with you?
Nevertheless, we need a cur_i2c_bus pointer, as the i2c core, needs
to know the current i2c bus for detecting if the bus, which whould be
accessed is the current or not and a switching to another bus is needed.
This is all unrelated to I2C bus muxes; they shouldn't be implemented as
part of an SoC I2C driver anyway, so the driver shouldn't know about bus
muxes before or after this patch - the I2C core should manage that.
Exactly! And that do the new i2c framework in i2c_core.c!
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On 10/25/2012 11:48 PM, Heiko Schocher wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few
points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
That rather relies on their being a concept of a current I2C adapter.
It seems a little limiting to require that. What if the current
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of current adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hi Heiko,
On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher h...@denx.de wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few
points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
2. The init is a bit odd, in that we can call init() repeatedly.
Perhaps that function should be renamed to reset, and the init should
be used for calling just once at the start?
What do you mean here exactly? I couldn´t parse this ...
Well there is start-of-day setup, which I think should be called init.
This is done once on boot for each i2c adapter.
And then there is the i2c_init() which seems to be called whenever we
feel like it - e.g. to change speed. I suggest that we use init() to
mean start-of-day init and reset(), or similar, to mean any post-init.
I am not suggest that for this series, just as a future effort.
3. Rather than each device having to call i2c_get_bus_num() to find
out what bus is referred to, why not just pass this information in? In
fact the adapter pointer can serve for this.
Not the struct i2c_adapter must passed, but the struct
i2c_bus!
And each device should know, which i2c bus it uses, or? So at
the end we should have something like i2c_read(struct i2c_bus *bus, ...)
calls ... and the i2c core can detect, if this bus is the
current, if so go on, if not switch to this bus. So at the
end i2c_set_bus_num would be go static ...
4. So perhaps the i2c read/write functions should change from:
int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
to:
int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
*buffer, int len)
Yep, exactly, see comments to point 3 ...
That would be the best (and I think in previous discussions I defined
this as one goal ...), but this would be (another) big change,
because this is an *API* change, with maybe a lot of config file
changes ...
Let us bring in the new i2c framework with all i2c drivers converted,
and then do the next step ... maybe one step more, if mareks device
model is ready, we can switch easy to DM ... and maybe get this API
change for free ...
Yes. I certainly understand the need to fit in with what is already
there, and avoid a massive API change, which can be performed as a
follow-on patch anyway. With your info above I will adjust the tegra
driver to work with this and test it.
Later, I wonder whether the concept of a 'current' i2c bus should be
maintained by the command line interpreter, rather than the i2c
system. Because to be honest, most of the drivers I see have to save
the current bus number, set the current bus, do the operation, then
set the bus back how they found it (to preserve whatever the user
things is the current bus).
Yes, suboptimal ... but this is independent from the new i2c framework
patches!
It is possible (with old/new i2c bus framework) to introduce a
current commandline i2c bus, and then, before calling i2c_read/write
from the commandline, call a i2c_set_bus_num() ... then we can get rid
of this store/restore current i2c bus ... waiting for patches ;-)
OK.
Granted there is overhead with i2c muxes, but the i2c core can
remember the state of these muxes and doesn't have to switch things
until there has been a
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote: [snip] Later, I wonder whether the concept of a 'current' i2c bus should be maintained by the command line interpreter, rather than the i2c system. Because to be honest, most of the drivers I see have to save the current bus number, set the current bus, do the operation, then set the bus back how they found it (to preserve whatever the user things is the current bus). I agree. Lets move the notion of current to cmd_i2c and make everything internally pass around the bus to operate on. Or try going down this path and find a fatal problem :) -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hi Heiko, On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher h...@denx.de wrote: rebased/reworked the I2C multibus patches from Simon Glass found here: http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html It seems the timing is coming, to bring this in mainline and move boards over to the new i2c framework. As an example I converted the soft-i2c driver (and all boards using it) to the new framework, so this patchseries has to be tested intensively, as I can check compile only ... I am very happy to see this, thank you. I have brought this in and tried to get it running for Tegra. A few points: 1. The methods in struct i2c_adapter should IMO be passed a struct i2c_adapter *, so they can determine which adapter is being accessed. Otherwise I can't see how they would know. 2. The init is a bit odd, in that we can call init() repeatedly. Perhaps that function should be renamed to reset, and the init should be used for calling just once at the start? 3. Rather than each device having to call i2c_get_bus_num() to find out what bus is referred to, why not just pass this information in? In fact the adapter pointer can serve for this. 4. So perhaps the i2c read/write functions should change from: int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) to: int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar *buffer, int len) Later, I wonder whether the concept of a 'current' i2c bus should be maintained by the command line interpreter, rather than the i2c system. Because to be honest, most of the drivers I see have to save the current bus number, set the current bus, do the operation, then set the bus back how they found it (to preserve whatever the user things is the current bus). Granted there is overhead with i2c muxes, but the i2c core can remember the state of these muxes and doesn't have to switch things until there has been a change since the last transaction. This last suggestion can be dealt with later, but I thought I would bring it up. Regards, Simon Cc: Simon Glass s...@chromium.org Cc: Piotr Wilczek p.wilc...@samsung.com Heiko Schocher (3): i2c: add i2c_core and prepare for new multibus support i2c: common changes for multibus/multiadapter support i2c, soft-i2c: switch to new multibus/multiadapter support README| 89 +++- arch/arm/include/asm/global_data.h|3 + arch/arm/lib/board.c | 15 +- arch/avr32/include/asm/global_data.h |3 + arch/blackfin/include/asm/global_data.h |4 +- arch/blackfin/lib/board.c |7 + arch/m68k/include/asm/global_data.h |3 + arch/m68k/lib/board.c | 15 +- arch/microblaze/include/asm/global_data.h |3 + arch/mips/include/asm/global_data.h |3 + arch/mips/lib/board.c |7 + arch/nds32/include/asm/global_data.h |3 + arch/nds32/lib/board.c| 10 +- arch/nios2/include/asm/global_data.h |3 + arch/powerpc/cpu/mpc8xx/video.c |4 + arch/powerpc/include/asm/global_data.h|3 + arch/powerpc/lib/board.c | 16 +- arch/sh/include/asm/global_data.h |3 + arch/sparc/include/asm/global_data.h |3 + arch/x86/include/asm/global_data.h|3 + board/BuS/eb_cpux9k2/cpux9k2.c|2 +- board/BuS/vl_ma2sc/vl_ma2sc.c |2 +- board/atc/atc.c |2 +- board/bluewater/snapper9260/snapper9260.c |2 +- board/cm5200/cm5200.c |4 +- board/cpu86/cpu86.c |2 +- board/cpu87/cpu87.c |2 +- board/emk/top9000/top9000.c |8 +- board/eukrea/cpuat91/cpuat91.c|2 +- board/freescale/m52277evb/README |2 +- board/freescale/m53017evb/README |2 +- board/freescale/m5373evb/README |2 +- board/freescale/m54455evb/README |2 +- board/freescale/m547xevb/README |2 +- board/ids8247/ids8247.c |2 +- board/keymile/common/common.c |3 +- board/keymile/common/ivm.c| 15 +- board/keymile/km82xx/km82xx.c |2 +- board/keymile/km_arm/km_arm.c |8 +- board/lwmon/lwmon.c |2 +- board/lwmon/pcmcia.c |4 +- board/pm826/pm826.c |2 +- board/pm828/pm828.c |2 +- board/sacsng/ioconfig.h |2 +- board/sandburst/karef/karef.c |1 - board/siemens/SCM/scm.c |2 +- board/tqc/tqm8260/tqm8260.c |2 +- board/tqc/tqm8272/tqm8272.c |2 +- board/tqc/tqm8272/tqm8272.h |2 +- common/cmd_date.c
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocherh...@denx.de wrote:
rebased/reworked the I2C multibus patches from Simon Glass found
here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...
I am very happy to see this, thank you.
I am too ;-) and Sorry that I am only now ready ...
I have brought this in and tried to get it running for Tegra. A few points:
1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.
They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS ((struct i2c_bus_hose *)gd-cur_i2c_bus)
#define I2C_ADAPi2c_adap[I2C_BUS-adapter]
#define I2C_ADAP_HWNR (I2C_ADAP-hwadapnr)
preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
(struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]
but of course, we still can change the struct i2c_adapter if
needed ... but we have one more parameter ... Ok, not really a bad
problem.
2. The init is a bit odd, in that we can call init() repeatedly.
Perhaps that function should be renamed to reset, and the init should
be used for calling just once at the start?
What do you mean here exactly? I couldn´t parse this ...
3. Rather than each device having to call i2c_get_bus_num() to find
out what bus is referred to, why not just pass this information in? In
fact the adapter pointer can serve for this.
Not the struct i2c_adapter must passed, but the struct
i2c_bus!
And each device should know, which i2c bus it uses, or? So at
the end we should have something like i2c_read(struct i2c_bus *bus, ...)
calls ... and the i2c core can detect, if this bus is the
current, if so go on, if not switch to this bus. So at the
end i2c_set_bus_num would be go static ...
4. So perhaps the i2c read/write functions should change from:
int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
to:
int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
*buffer, int len)
Yep, exactly, see comments to point 3 ...
That would be the best (and I think in previous discussions I defined
this as one goal ...), but this would be (another) big change,
because this is an *API* change, with maybe a lot of config file
changes ...
Let us bring in the new i2c framework with all i2c drivers converted,
and then do the next step ... maybe one step more, if mareks device
model is ready, we can switch easy to DM ... and maybe get this API
change for free ...
Later, I wonder whether the concept of a 'current' i2c bus should be
maintained by the command line interpreter, rather than the i2c
system. Because to be honest, most of the drivers I see have to save
the current bus number, set the current bus, do the operation, then
set the bus back how they found it (to preserve whatever the user
things is the current bus).
Yes, suboptimal ... but this is independent from the new i2c framework
patches!
It is possible (with old/new i2c bus framework) to introduce a
current commandline i2c bus, and then, before calling i2c_read/write
from the commandline, call a i2c_set_bus_num() ... then we can get rid
of this store/restore current i2c bus ... waiting for patches ;-)
Granted there is overhead with i2c muxes, but the i2c core can
remember the state of these muxes and doesn't have to switch things
until there has been a change since the last transaction.
This exactly do the i2c_set_bus_num() now!
This last suggestion can be dealt with later, but I thought I would bring it up.
I am happy about every comment! :-)
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 0/3] Bring in new I2C framework
rebased/reworked the I2C multibus patches from Simon Glass found here: http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html It seems the timing is coming, to bring this in mainline and move boards over to the new i2c framework. As an example I converted the soft-i2c driver (and all boards using it) to the new framework, so this patchseries has to be tested intensively, as I can check compile only ... Cc: Simon Glass s...@chromium.org Cc: Piotr Wilczek p.wilc...@samsung.com Heiko Schocher (3): i2c: add i2c_core and prepare for new multibus support i2c: common changes for multibus/multiadapter support i2c, soft-i2c: switch to new multibus/multiadapter support README| 89 +++- arch/arm/include/asm/global_data.h|3 + arch/arm/lib/board.c | 15 +- arch/avr32/include/asm/global_data.h |3 + arch/blackfin/include/asm/global_data.h |4 +- arch/blackfin/lib/board.c |7 + arch/m68k/include/asm/global_data.h |3 + arch/m68k/lib/board.c | 15 +- arch/microblaze/include/asm/global_data.h |3 + arch/mips/include/asm/global_data.h |3 + arch/mips/lib/board.c |7 + arch/nds32/include/asm/global_data.h |3 + arch/nds32/lib/board.c| 10 +- arch/nios2/include/asm/global_data.h |3 + arch/powerpc/cpu/mpc8xx/video.c |4 + arch/powerpc/include/asm/global_data.h|3 + arch/powerpc/lib/board.c | 16 +- arch/sh/include/asm/global_data.h |3 + arch/sparc/include/asm/global_data.h |3 + arch/x86/include/asm/global_data.h|3 + board/BuS/eb_cpux9k2/cpux9k2.c|2 +- board/BuS/vl_ma2sc/vl_ma2sc.c |2 +- board/atc/atc.c |2 +- board/bluewater/snapper9260/snapper9260.c |2 +- board/cm5200/cm5200.c |4 +- board/cpu86/cpu86.c |2 +- board/cpu87/cpu87.c |2 +- board/emk/top9000/top9000.c |8 +- board/eukrea/cpuat91/cpuat91.c|2 +- board/freescale/m52277evb/README |2 +- board/freescale/m53017evb/README |2 +- board/freescale/m5373evb/README |2 +- board/freescale/m54455evb/README |2 +- board/freescale/m547xevb/README |2 +- board/ids8247/ids8247.c |2 +- board/keymile/common/common.c |3 +- board/keymile/common/ivm.c| 15 +- board/keymile/km82xx/km82xx.c |2 +- board/keymile/km_arm/km_arm.c |8 +- board/lwmon/lwmon.c |2 +- board/lwmon/pcmcia.c |4 +- board/pm826/pm826.c |2 +- board/pm828/pm828.c |2 +- board/sacsng/ioconfig.h |2 +- board/sandburst/karef/karef.c |1 - board/siemens/SCM/scm.c |2 +- board/tqc/tqm8260/tqm8260.c |2 +- board/tqc/tqm8272/tqm8272.c |2 +- board/tqc/tqm8272/tqm8272.h |2 +- common/cmd_date.c |9 + common/cmd_dtt.c |9 + common/cmd_eeprom.c |3 +- common/cmd_i2c.c | 118 ++--- common/env_eeprom.c | 14 + common/stdio.c| 14 +- drivers/i2c/Makefile |3 +- drivers/i2c/i2c_core.c| 367 + drivers/i2c/soft_i2c.c| 122 ++ include/configs/A3000.h |4 +- include/configs/BSC9131RDB.h |1 - include/configs/CANBT.h |4 + include/configs/CPU86.h |9 +- include/configs/CPU87.h |9 +- include/configs/DU440.h |1 - include/configs/GEN860T.h | 36 ++-- include/configs/HIDDEN_DRAGON.h | 10 +- include/configs/IAD210.h | 12 +- include/configs/ICU862.h | 14 +- include/configs/IDS8247.h | 10 +- include/configs/IP860.h | 10 +- include/configs/IPHASE4539.h | 12 +- include/configs/JSE.h |1 - include/configs/KAREF.h |1 - include/configs/KUP4K.h | 12 +- include/configs/KUP4X.h | 14 +- include/configs/M5208EVBE.h |1 - include/configs/M52277EVB.h |1 - include/configs/M5235EVB.h|1 - include/configs/M5271EVB.h|1 - include/configs/M5275EVB.h|1 - include/configs/M53017EVB.h |1
Re: [U-Boot] [PATCH 0/3] Bring in new I2C framework
Hello Simon, Simon Glass wrote: This series provides Heiko's upgraded I2C framework from a few years ago. I hope that we can bring this in and move boards over to it as time permits, rather than switching everything in one fell swoop which never happens. Ok, lets try it! To show it working I have enabled it for Tegra in a very rough way. It seems fine with my limited testing. Great! Thanks! Patches for other i2c drivers can be found here: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_2012 They just need a rebase and an update to your changes (and of course some tests) In terms of changes, I have just fixed some checkpatch errors and fiddled with a couple of function signatures. I will start a thread on the list with a few thoughts on this series at some point. Ok, thanks. Here some thoughts comming in my mind: - Why a cur_adap added in gd_t: - This points allways to the current used i2c adapter. - because gd_t is writeable when running in flash, complete multiadapter/multibus functionality is usable, when running in flash, which is needed for some SoCs. - using a pointer brings faster accesses to the i2c_adapter_t struct and saves some bytes here and there. - init from a i2c controller: In current code all i2c controllers, as a precaution, getting initialized. In the new code, a i2c controller gets only initialized if it is used. This is done in i2c_set_bus_num(). Also, with this approach, we can easy add in a second step, a i2c_deinit() function, called from i2c_set_bus_num(), so we can easy deactivate a no longer used controller. - added hw_adapnr in i2c_adapter_t struct: when for example a CPU has more then one i2c controller we can use this variable to differentiate which controller the actual i2c adapter uses. - Maybe we should add a base_addr field in struct i2c_adapter? This would help for SoCs, who have more then one identical controller, just differ in their base_addr... (Currently I made a function, or an array which returns the base_addr, dependend on hw_adapnr). We should drop this, and introduce a base_addr field. [...] bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 0/3] Bring in new I2C framework
This series provides Heiko's upgraded I2C framework from a few years ago. I hope that we can bring this in and move boards over to it as time permits, rather than switching everything in one fell swoop which never happens. To show it working I have enabled it for Tegra in a very rough way. It seems fine with my limited testing. In terms of changes, I have just fixed some checkpatch errors and fiddled with a couple of function signatures. I will start a thread on the list with a few thoughts on this series at some point. Heiko Schocher (2): i2c: add i2c_core and prepare for new multibus support i2c: common changes for multibus/multiadapter support Simon Glass (1): WIP: tegra: i2c: Enable new I2C framework README| 82 +++- arch/arm/include/asm/global_data.h|3 + arch/arm/lib/board.c |3 +- arch/avr32/include/asm/global_data.h |3 + arch/blackfin/include/asm/global_data.h |4 +- arch/blackfin/lib/board.c |7 + arch/m68k/include/asm/global_data.h |3 + arch/m68k/lib/board.c | 18 ++- arch/microblaze/include/asm/global_data.h |3 + arch/mips/include/asm/global_data.h |3 + arch/mips/lib/board.c |7 + arch/nios2/include/asm/global_data.h |3 + arch/powerpc/cpu/mpc8xx/video.c |4 + arch/powerpc/include/asm/global_data.h|3 + arch/powerpc/lib/board.c | 18 ++- arch/sh/include/asm/global_data.h |3 + arch/sparc/include/asm/global_data.h |3 + arch/x86/include/asm/global_data.h|3 + common/cmd_date.c |9 + common/cmd_dtt.c |9 + common/cmd_i2c.c | 127 +++ common/stdio.c| 13 +- drivers/i2c/Makefile |1 + drivers/i2c/i2c_core.c| 360 + drivers/i2c/tegra2_i2c.c | 53 ++--- include/configs/seaboard.h|2 + include/i2c.h | 190 +++- 27 files changed, 842 insertions(+), 95 deletions(-) create mode 100644 drivers/i2c/i2c_core.c -- 1.7.7.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
