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

[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
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.

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?

Regards,
Simon