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

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

So no, I don't think we can move U-Boot's design from "_f/_r" to
"SPL/U-Boot".

> > But we should strictly limit the scope of board_init_f() or we'll find
> > the board_init_f()/board_init_r() pair following a patch similar to the
> > SPL/U-Boot pair, where SPL started out as a tiny helper piece of code
> > and ending up a resizeable (and, I dare to say, sizeable as well) kind
> > of U-boot. If we let too many features slip in board_init_f(), it'll
> > blur into a board_init_r() like and before we know it, it'll *require*
> > DDR, and write access to it too...
> >
> > So, board_init_() should *strictly* be limited to setting up a console
> > (for information purposes) and giving access to DDR while in the same
> > time never writing to it itself. Bonus points if it can limit itself to
> > *enabling* and postpone any *optimizing*(I am thinking of DDR settings
> > here and no, I don't have specific existing cases in mind; just sayin').
> >
> > In the present instance, I'd rather we either:
> >
> > - removed dependency on DT etc. by using "hard-coded" low level I2C
> >    reads for those boards that need it (I assume that for each of these
> >    boards the I2C slave, offset, and length to read are constant) in _f
> >    phase, or
> 
> But DT is used for initializing the i2c driver in tegra ...

Alright, out goes this proposal. Anyway, I didn't like it best. :)

> > - parsed the _f phase looking for offending functions or calls which
> >    write to .data or .bss and fix them, suffixing them with _f; in
> >    essence, that amounts to starting the implementation of my suggestion
> >    above alongside fixing the issue at hand.
> >
> > The first approach is rather "let's bring the thing back up first", so
> > it does not have my preference, but I would understand the need to
> > quickly fix things.
> 
> Yes.
> 
> > The second approach seems to be going in the same direction as Heiko's
> > proposal of 07:52 +0200, which I thus second provided it is applicable
> > to all the boards Wolfgang had in mind.
> 
> Lets do us this step as fixup ;-)

Alright too. :)

> bye,
> Heiko

Amicalement,
-- 
Albert.