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

[Simon Glass]

Hi Heiko,

On Thu, Aug 1, 2013 at 10:40 PM, Heiko Schocher <hs at denx.de> wrote:

> 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=**
> ccb680c8cf9002232bc459e4003e3b**47db5e1bf4<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?


I don't remember it listing all the devices. It was a very long time ago,
but when I sent my patch everything seemed to work find on Seaboard. Time
is not my friend at the moment but I will see if I can make time this
weekend to try this out and come up with a patch.

Regards,
Simon