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

[Heiko Schocher]

Hello Stephen,

Am 01.08.2013 07:39, schrieb Stephen Warren:
> On 07/31/2013 10:32 PM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> Am 31.07.2013 21:31, schrieb Stephen Warren:
>>> On 07/30/2013 11:46 PM, Heiko Schocher wrote:
>>> ...
>>>> Hmm.. each i2c adapter has its own init function ... why the tegra
>>>> driver do not use it? And do the necessary inits in it? So we
>>>> initialize an adapater only if we use it, which is also a rule
>>>> for u-boot ...
>>>>
>>>> I have no hw, but it should be possible to add to process_nodes()
>>>> a parameter "controller_number" and call
>>>> process_nodes(controller_number, ...) from the i2c drivers init
>>>> function ...
>>>
>>> There are two steps to initializing I2C on a Tegra system:
>>>
>>> 1) Parsing the DT to determine which/how-many I2C adapters exist in the
>>> SoC. This will yield information such as the register address of the I2C
>>> adapters, which clock/reset signal they rely on, perhaps the max bus
>>> clock rate, etc.
>>>
>>> This is a single global operation that needs to happen one single time
>>> before anything else.
>>
>> Why?
>
> That's simply the way the code is written.

Ok.

>>> This stage initializes the i2c_controllers[] array, since that's what
>>> stores all the data parsed from DT.
>>>
>>> 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?

> 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?

If this "2" is used on different boards with the same u-boot code only
different in dt, bus 2 maybe not exist ...
or if you change the order of i2c nodes in the dt "2" is no longer
"2" ... or?

>> saying something like this (just as an example, should be tested):
>>
>> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
>> index 9ac3969..7bbd3c7 100644
>> --- a/drivers/i2c/tegra_i2c.c
>> +++ b/drivers/i2c/tegra_i2c.c
>> @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int
>> node, struct i2c_bus *i2c_bus)
>>    * @param is_scs       1 if this HW uses a single clock source (T114+)
>>    * @return 0 if ok, -1 on error
>>    */
>> -static int process_nodes(const void *blob, int node_list[], int count,
>> +static int process_nodes(const void *blob, int node_list[],
>> +                        struct i2c_adapter *adap, int count,
>>                           int is_dvc, int is_scs)
>>   {
>>          struct i2c_bus *i2c_bus;
>> -       int i;
>> -
>> -       /* build the i2c_controllers[] for each controller */
>> -       for (i = 0; i<  count; i++) {
>> -               int node = node_list[i];
>> +       int node = node_list[i];
>
> How do you determine i here? There's no guarantee that all I2C DT nodes

adap->hwadapnr

as in drivers/i2c/tegra_i2c.c:

static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)
{
         struct i2c_bus *bus;

         bus = &i2c_controllers[adap->hwadapnr];
[...]

> end up being processed in a single call to process_nodes(); there might
> be some "Tegra20 I2C", some "Tegra20 I2C DVC", some "Tegra30 I2C"
> modules in the same SoC.

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