[U-Boot] [PATCH V2 3/4] common: introduce board_preboot_os hook

[Nikita Kiryanov]

Hi Stefano,

On 11/05/2014 02:32 PM, Stefano Babic wrote:
> Hi Nikita,
>
> On 29/10/2014 16:56, Nikita Kiryanov wrote:
>> Introduce board specific function board_preboot_os() to allow for board
>> specific config before we boot.
>>
>> Signed-off-by: Nikita Kiryanov <nikita at compulab.co.il>
>> Cc: Igor Grinberg <grinberg at compulab.co.il>
>> Cc: Stefano Babic <sbabic at denx.de>
>> Cc: Tom Rini <trini at ti.com>
>> Cc: Jeroen Hofstee <jeroen at myspectrum.nl>
>> Cc: Otavio Salvador <otavio at ossystems.com.br>
>> ---
>> Changes in V2:
>> 	- Added board_preboot_os to bootm.h
>> 	- Split cm_fx6 stuff into a separate patch
>>
>>   common/bootm_os.c | 7 +++++++
>>   include/bootm.h   | 1 +
>>   2 files changed, 8 insertions(+)
>>
>
> There is something that does not convince me. Really, the general
> statement should be to turn all devices off before booting the kernel,
> because this is what Linux expects. That said, this could be later
> solved with dm, because we will can iterate through all drivers and call
> a shutdown function.
>
> Currently, "preboot" functions are turning off the hardware, so the name
> is already misleading. And I see that arch_preboot_os() was also
> convinced (because it is weak) to become a board_preboot, for example
> here: board/renesas/koelsch/koelsch.c (and in other renesas
> implementation, too).
>
> I have some concerns adding a new weak function, that enables some
> further hooks inside board code. I find it not well scalable.
> In your case, you need such as sata_shutdown(), and it should be
> responsibility of the general sata code to call something specific for
> the board, if necessary.
>
> In current code, bootm_disable_interrupts() calls also usb_stop() and
> eth_halt(), - nothing to do with the name of the function. IMHO it
> should be better to move the code to shutdown interface inside
> boot_selected_os(), or having a common function "disable_hardware()"
> that calls usb_stop(), eth_halt() and then, why not, a sata_stop().
>
> There is also in arch/arm/imx-common/cpu.c a derived implementation for
> arch_preboot_os(). Really it has nothing to do with arch, as I can see:
> it shuts down only the IPU. But it is another hook, and IMHO it is
> better stopping sata here as adding a hidden callback.

You make some good points.
I'll switch to using arch_preboot_os until there's a better way of doing
this.

-- 
Regards,
Nikita Kiryanov