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

[Heiko Schocher]

Hello Simon,

Am 31.07.2013 14:30, schrieb Simon Glass:
> Hi,
>
> On Wed, Jul 31, 2013 at 3:38 AM, Albert ARIBAUD
> <albert.u.boot at aribaud.net>wrote:
>
>> Hi Heiko,
>>
>> On Wed, 31 Jul 2013 10:31:12 +0200, Heiko Schocher<hs at denx.de>  wrote:
>>
>>> Hello Albert,
>>>
>>> Am 31.07.2013 10:16, schrieb Albert ARIBAUD:
>>>> Hi Heiko,
>>>>
>>>> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher<hs at denx.de>   wrote:
>>>>
>>>>>> On the other hand, it may be hard to immediately know what functions
>>>>>> throughout U-boot are safe to call from within board_init_f(); maybe
>> we
>>>>>> should start thinking about checking and marking these, the simplest
>>>>>> way being to suffix them with "_f" once we have made sure they are
>> safe
>>>>>> to call from within board_init_f().
>>>>>
>>>>> Hmmm... Maybe instead we should think (also in thinking common bring
>>>>> up for all boards) about:
>>>>>
>>>>> getting rid of board_init_f in u-boot code, instead use for all
>>>>> boards spl code to init needed things and copy and relocate u-boot
>>>>> to ram in spl code ... so we have in u-boot no longer such
>>>>> restictions ... but thats just an idea which whirs in my head ...
>>>>> without thinking to deep in it.
>>>>>
>>>>> But this approach would have some advantages ...
>>>>
>>>> Well, the original SPL was basically board_init_f() plus some code to
>>>> copy U-Boot from wherever it was to DDR, so it was tightly linked to
>>>> board_init_f(). But... first, SPL has evolved into a "U-Boot lite"
>>>> where much can happen beyond board_init_f() -- think Falcon mode, for
>>>> instance -- and second, there are boards which do not have SPL at all,
>>>> and their board_init_f() can thus not be "moved to SPL".
>>>
>>> Hmm... all boards use board_init_f ... and spl do pieces from
>> board_init_f
>>> So why should it not be possible to do all init things in spl code?
>>> Code beyond board_init_f is optional ...
>>
>> It is, in the original "SPL is just board_init_f plus some copying"
>> view. In the current "SPL is U-boot only not full-featured", it becomes
>> false.
>>
>>> And yes, there are a lot of boards, which have no spl, but they
>>> can execute spl code (thinking of the lof of powerpc boards which
>>> booting from nor flash ... spl code can also run from nor ... and
>>> copy the u-boot piece of the image to ram, relocate it ...)
>>>
>>> And yes, a side effect could be, that all boards can use Falcon boot
>> mode.
>>>
>>> ;-)
>>>
>>>> So no, I don't think we can move U-Boot's design from "_f/_r" to
>>>> "SPL/U-Boot".
>>>
>>> I am not sure ...
>>
>> I see this approach of likening SPL to _f and U-boot to _r as forcing a
>> dual-binary model onto all boards whereas not all boards require it. I
>> prefer a model where _f can exist, _r can exist, and for each target,
>> the maintainer decides which binaries are built and for each one,
>> whether _f and/or _r is present and what _r does.
>>
>
> I am not really any clearer as to what should be done here.
>
> Previously on ARM i2c init generally happened in board_init_r(). This has

really? The init_func_i2c() call is not introduced through the
i2c multibus approach ...

> changed now, and so boards which need to do some init (e.g. reading and
> storing DT information) to make i2c work are going to have problems if they
> cannot access any memory (yes we could put it in global_data I suppose).
>
> It sounds like the need for early i2c is rare, so we could perhaps create
> an option like CONFIG_SYS_EARLY_I2C to enable this?
>
> While I agree that minimising code in board_init_f() is a great idea, if we
> have one case that needs it, then we need to deal with the problem.

 From my side, yes. But this different to powerpc!

> Although did I mention that it does seem silly to me to solve what is an
> entirely hypothetical problem on Tegra and (I think) any other modern ARM
> SOC that uses SPL? After all, SDRAM is fully available on these SOCs and in
> fact setting that up and getting U-Boot loaded into RAM is the purpose of
> the SPL stage! To my mind, SPL has taken over this responsibility of
> board_init_f(). Thoughts? Maybe a minor rethink of
> SPL/board_init_f()/board_init_r() is in order?

Yes, but board_init_f is used in spl code, or? And it is not only
ram settings, maybe read baudrate setting in an i2c epprom ...

We can make init_func_i2c() weak, and in the first step a
dummy function and see, which boards really need it.

Nevertheless, why the tegra i2c driver do not call process_node()
from the i2c_init function? This must be fixed I think, as it
is not good to do some setup needed for i2c in board_init()
and other in i2c_init to have a working i2c adapter ...

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