[U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework

[Heiko Schocher]

Hello Stephen,

Am 01.08.2013 22:32, schrieb Stephen Warren:
> On 08/01/2013 12:02 AM, Heiko Schocher wrote:
>> Am 01.08.2013 07:39, schrieb Stephen Warren:
>>> On 07/31/2013 10:32 PM, Heiko Schocher wrote:
>>>> Am 31.07.2013 21:31, schrieb Stephen Warren:
>>>>> On 07/30/2013 11:46 PM, Heiko Schocher wrote:
> ...
>>>>> 2) Actually initializing or using the I2C HW. That could certainly be
>>>>> part of the per-I2C-controller init() function you mention.
>>>>
>>>> For that purpose is the i2c_init function.
>>>>
>>>> And why we could not do step 1 when we do step 2? Why doing step 1
>>>> for hw we later do not use?
>>>
>>> I suppose you could. It seems conceptually /far/ simpler to just scan
>>> the DT once up-front rather than having to defer all this stuff until
>>
>> on the other hand we ring for every ms boot time ... and here we want
>> to scan a complete dt with maybe a lot of nodes, we do not want to
>> use?
>>
>>> you actually use an I2C bus. If you do that, how can you know how many
>>> I2C buses exist without trying to use every possible one and seeing
>>
>> Do I really need to know that?
>
> Well, with the recent patches, U-Boot prints out a list of valid I2C

which patches? I am trying current U-Boot on some boards, I do
not see such a list ... you mean on tegra based boards?

> adapters at boot. So, I suppose someone must have thought it a good idea!

Ok, but then, printing this "list" should done after relocation.
And the me known "I2C:   ready" print on U-Boot boot, comes from
init_func_i2c() ... which is ancient code from powerpc times ...

> Perhaps the most relevant reason: why on earth wouldn't a user want to
> run e.g. "i2c list" or "i2c dev" and get back a list of valid I2C
> adapters, as opposed to poking around in the dark to see which exist?

Didn't you need also a list of devices on each i2c bus, if you use
this argument?

But for this reason we have in the new framework the "i2c bus"
command, which shows all busses. This could be adapted in the DT case,
to look, which buses really exist and print them. No need to do this
at boottime!

Also, in an old version of the multibus/multiadapter approach
I made a option to add dynamically new i2c buses for example
from the environment or from the "i2c bus" command ... maybe
it is worth to think about such an option in the DT case?

see here:
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4

Patches welcome ;-)

>>> which fail? Also, the mapping from I2C bus number to register address is
>>
>> Hmm.. there are max TEGRA_I2C_NUM_CONTROLLERS. If one gets used,
>> it scans the dt ... if only 1 adapter is used, only one is activated
>> through i2c_init ...
>>
>>> only created by actually scanning the whole DT; there's no need to every
>>> I2C DT node to have a /aliases entry that dictates its U-Boot device ID,
>>> so you really do have to scan everything completely up-front before you
>>> can determine which registers to use.
>>
>> But the i2c bus number is coded static all over the u-boot code (Should
>> be changed) in the code. Saying a board has an i2c eeprom on bus "2",
>> it calls i2c_set_bus_number(2) ... this "2" must be somewhere in the dt
>> or?
>
> Yes, the "2" must come from DT, or DT is pointless. We simply have to

Ok, puuh...

> get rid of all the hard-coding. If we can't do that, then I'd strongly
> prefer to revert all the DT mess, and put the list of HW modules back
> into the source code. DT isn't adding anything at all unless we can
> actually use it exclusively.
>
> Given how long this discussion is going on, can we please just revert
> the commit so that the code works for everyone who's trying to use it,
> then fix the problem later?

Yes, but not reverting the hole commit, please just remove the
i2c_init_board() call in i2c_init_all() and test it, and send a
new patch, thanks!

diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
index d1072e8..5f10eb3 100644
--- a/drivers/i2c/i2c_core.c
+++ b/drivers/i2c/i2c_core.c
@@ -246,7 +246,6 @@ void i2c_init_board(void)
   */
  void i2c_init_all(void)
  {
-       i2c_init_board();
         i2c_set_bus_num(CONFIG_SYS_SPD_BUS_NUM);
         return;
  }

Or we make the "init_func_i2c()" function weak, and as the
first step it is a dummy function, and boards which really need
it, currently fail and must code it in board code? I would prefer
this way, as it is anyway the goal to get rid of this function
if all boards are using the new framework ...)

Can you or Simon try this?

@Simon:
Hmm... just looking in an older version of the new framework...
i2c_init_board() was from the beginning in i2c_init_all()... why
didn't this poped up when you tested the tegra patches? Are there
some changes in the DT/tegra code in the meantime?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany