[U-Boot] [PATCH 10/10] arm: socfpga: arria10: Added Arria10 critical HW initialization to spl

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Sel, 2016-12-20 at 09:17 -0600, Dinh Nguyen wrote:
> 
> On 12/06/2016 02:11 AM, Chee Tien Fong wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > This patch adding the Arria10 critical hardware initialization
> > before
> > enabling console print out in spl.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Dinh Nguyen <dinguyen@kernel.org>
> > Cc: Chin Liang See <chin.liang.see@intel.com>
> > Cc: Tien Fong <skywindctf@gmail.com>
> > ---
> > ?arch/arm/mach-socfpga/spl.c |???86
> > +++++++++++++++++++++++++++++++++++++++++-
> > ?1 files changed, 83 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-socfpga/spl.c b/arch/arm/mach-
> > socfpga/spl.c
> > index fec4c7a..9375514 100644
> > --- a/arch/arm/mach-socfpga/spl.c
> > +++ b/arch/arm/mach-socfpga/spl.c
> > @@ -1,7 +1,7 @@
> > ?/*
> > - *??Copyright (C) 2012 Altera Corporation <www.altera.com>
> > + *??Copyright (C) 2012-2016 Altera Corporation <www.altera.com>
> > ? *
> > - * SPDX-License-Identifier:	GPL-2.0+
> > + * SPDX-License-Identifier:	GPL-2.0
> > ? */
> Shouldn't be changing the license.
> 
Yeah, i will revert it.
> > 
> > 
> > ?#include <common.h>
> > @@ -19,22 +19,32 @@
> > ?#include <asm/arch/sdram.h>
> > ?#include <asm/arch/scu.h>
> > ?#include <asm/arch/nic301.h>
> > +#include <asm/sections.h>
> > +#include <watchdog.h>
> > +#include <fdtdec.h>
> > +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +#include <asm/arch/pinmux.h>
> > +#endif
> I don't know about all these includes, do you really need them? I
> don't?
> see where you would need pinmux.h in this patch.
> 
Yeah, we those header, otherwise compilation failed.
we need pinmux.h also, because there is a
function?config_dedicated_pins(gd->fdt_blob); in void
board_init_f(ulong dummy). This function would configure the dedicated
IO such as UART IO before enable print console.
> > 
> > 
> > ?DECLARE_GLOBAL_DATA_PTR;
> > 
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > ?static struct pl310_regs *const pl310 =
> > ?	(struct pl310_regs *)CONFIG_SYS_PL310_BASE;
> > ?static struct scu_registers *scu_regs =
> > ?	(struct scu_registers *)SOCFPGA_MPUSCU_ADDRESS;
> > ?static struct nic301_registers *nic301_regs =
> > ?	(struct nic301_registers *)SOCFPGA_L3REGS_ADDRESS;
> > -static struct socfpga_system_manager *sysmgr_regs =
> > +#endif
> > +
> > +static const struct socfpga_system_manager *sysmgr_regs =
> > ?	(struct socfpga_system_manager *)SOCFPGA_SYSMGR_ADDRESS;
> > 
> > ?u32 spl_boot_device(void)
> > ?{
> > ?	const u32 bsel = readl(&sysmgr_regs->bootinfo);
> > 
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > ?	switch (bsel & 0x7) {
> > ?	case 0x1:	/* FPGA (HPS2FPGA Bridge) */
> > ?		return BOOT_DEVICE_RAM;
> > @@ -55,6 +65,24 @@ u32 spl_boot_device(void)
> > ?		printf("Invalid boot device (bsel=%08x)!\n",
> > bsel);
> > ?		hang();
> > ?	}
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +	switch ((bsel>>12) & 0x7) {
> > +	case 0x1:	/* FPGA (HPS2FPGA Bridge) */
> > +		return BOOT_DEVICE_RAM;
> > +	case 0x2:	/* NAND Flash (1.8V) */
> > +	case 0x3:	/* NAND Flash (3.0V) */
> > +		return BOOT_DEVICE_NAND;
> > +	case 0x4:	/* SD/MMC External Transceiver (1.8V) */
> > +	case 0x5:	/* SD/MMC Internal Transceiver (3.0V) */
> > +		return BOOT_DEVICE_MMC1;
> > +	case 0x6:	/* QSPI Flash (1.8V) */
> > +	case 0x7:	/* QSPI Flash (3.0V) */
> > +		return BOOT_DEVICE_SPI;
> > +	default:
> > +		printf("Invalid boot device (bsel=%08x)!\n",
> > bsel);
> > +		hang();
> > +	}
> > +#endif
> > ?}
> You should just do a shift define??here, so you don't have to add
> all?
> this extra code here. Something like
> 
> 	switch ((bsel >> BOOTINFO_BSEL_SHIFT) & 0x7)
> 
> > 
> > 
> > ?#ifdef CONFIG_SPL_MMC_SUPPORT
> > @@ -68,6 +96,7 @@ u32 spl_boot_mode(const u32 boot_device)
> > ?}
> > ?#endif
> > 
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > ?static void socfpga_nic301_slave_ns(void)
> > ?{
> > ?	writel(0x1, &nic301_regs->lwhps2fpgaregs);
> > @@ -182,3 +211,54 @@ void board_init_f(ulong dummy)
> > ?	/* Configure simple malloc base pointer into RAM. */
> > ?	gd->malloc_base = CONFIG_SYS_TEXT_BASE + (1024 * 1024);
> > ?}
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +void board_init_f(ulong dummy)
> > +{
> > +	memset(__bss_start, 0, __bss_end - __bss_start);
> > +	/*
> > +	?* Configure Clock Manager to use intosc clock instead
> > external osc to
> > +	?* ensure success watchdog operation. We do it as early as
> > possible.
> > +	?*/
> > +	cm_use_intosc();
> > +
> > +	watchdog_disable();
> Why?
> 
Disabled watchdog for watchdog configuration, this cater for scenario
watchdog already starting running such as warm reset.
> > 
> > +
> > +	arch_early_init_r();
> I don't think you should be calling this here.
> 
This function contains security policies initialization(before
accessing any slaves), asserting reset to all timers(L4) before HW
configuration. This is to ensure no firewall blocking on slave, timer
in idle state after power up/warm reset.
> > 
> > +
> > +#ifdef CONFIG_HW_WATCHDOG
> > +	/* release osc1 watchdog timer 0 from reset */
> > +	reset_deassert_osc1wd0();
> > +
> > +	/* reconfigure and enable the watchdog */
> > +	hw_watchdog_init();
> > +	WATCHDOG_RESET();
> Do you really need all these WATCHDOG_RESET()'s?
> > 
> > +#endif /* CONFIG_HW_WATCHDOG */
> > +
> > +#ifdef CONFIG_OF_CONTROL
> There's no need for this #ifdef. If you build for ARCH_SOCFPGA is?
> OF_CONTROL selected.
> 
> > 
> > +	/* We need to access to FDT as this stage */
> Why?
> 
we have dedicated IO configuration and clock configuration based on
handoff data in FDT.
> Dinh