From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikita Kiryanov Date: Thu, 06 Nov 2014 13:31:36 +0200 Subject: [U-Boot] [PATCH V2 3/4] common: introduce board_preboot_os hook In-Reply-To: <545A18CF.6080604@denx.de> References: <1414598184-1963-1-git-send-email-nikita@compulab.co.il> <1414598184-1963-4-git-send-email-nikita@compulab.co.il> <545A18CF.6080604@denx.de> Message-ID: <545B5C18.3020401@compulab.co.il> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> Cc: Igor Grinberg >> Cc: Stefano Babic >> Cc: Tom Rini >> Cc: Jeroen Hofstee >> Cc: Otavio Salvador >> --- >> 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