[U-Boot] [PATCH v3] arm: socfpga: fix issue with warm reset when CSEL is 0

[Dalon Westergreen]

On Mon, 2017-02-20 at 15:14 +0100, Marek Vasut wrote:
> On 02/20/2017 03:10 PM, Dalon Westergreen wrote:
> > 
> > On Mon, 2017-02-20 at 10:07 +0100, Marek Vasut wrote:
> > > 
> > > On 02/18/2017 02:34 AM, Dalon Westergreen wrote:
> > > > 
> > > > 
> > > > When CSEL=0x0 the socfpga bootrom does not touch the clock
> > > > configuration for the device.  This can lead to a boot failure
> > > > on warm resets. This patch disables warm resets when CSEL=0.
> > > > This results in the clock and pll configurations being reset
> > > > on any reset issued when CSEL=0.
> > > > 
> > > > Signed-off-by: Dalon Westergreen <dwesterg at gmail.com>
> > > 
> > > What about my suggestion for V2 about just loading function pointer into
> > > the reset jump address register ?
> > 
> > Frankly, i really dont like relying on the existence of a snippet of code in
> > the
> > onchip ram being untouched to ensure a reboot/reset will occur for this
> > csel=0
> > case.  i am certain this case is rarely used, and confident that it isnt
> > being
> > used while trying to preserve sdram contents.
> 
> Well, you already rely on such snippet, it's SPL. If you corrupt SPL and
> do warm reset, your system hangs, I had that multiple times :)

True.  I would argue to just use cold resets but i think arria 10 has more use
for the warm reset case.

> > 
> > the downside is that the scorecard is reset every boot. so the bootrom will
> > retry all the spl images again resulting in possibly longer boot times.
> 
> Is that significant ?

The watchdog timeout is on the order of 1.5 seconds.  That would be for each
failed spl.

> > 
> > The
> > other is that things like sdram content preservation is not likely to work
> > or
> > the csel=0 case (but as i mentioned, i dont see this used often in cyclone5,
> > and never have i seen it when csel=0).
> 
> OK, I didn't see this requirement yet.
> 
> > 
> > > 
> > > btw --- missing before the changelog, without it the changelog will land
> > > in git history.
> > 
> > Thanks
> > 
> > > 
> > > > 
> > > > 
> > > > Changes in v3:
> > > >  - Change implementation to rely on cold reset for CSEL=0. Which
> > > >    is a much simpler approach to dealing with this special case
> > > >    during boot.
> > > > Changes in v2:
> > > >  - Fix checkpatch issues predominently due to whitespace issues
> > > > ---
> > > >  arch/arm/mach-socfpga/include/mach/system_manager.h |  3 +++
> > > >  arch/arm/mach-socfpga/misc.c                        | 13 ++++++++++++-
> > > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/arm/mach-socfpga/include/mach/system_manager.h
> > > > b/arch/arm/mach-socfpga/include/mach/system_manager.h
> > > > index c45edea..c9c0b33 100644
> > > > --- a/arch/arm/mach-socfpga/include/mach/system_manager.h
> > > > +++ b/arch/arm/mach-socfpga/include/mach/system_manager.h
> > > > @@ -137,6 +137,9 @@ struct socfpga_system_manager {
> > > >  
> > > >  #define SYSMGR_SDMMC_DRVSEL_SHIFT	0
> > > >  
> > > > +#define SYSMGR_BOOTINFO_CSEL_MASK	0x18
> > > > +#define SYSMGR_BOOTINFO_CSEL_LSB	3
> > > > +
> > > >  /* EMAC Group Bit definitions */
> > > >  #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII	0x0
> > > >  #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII		0x1
> > > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> > > > index dd6b53b..9792138 100644
> > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > @@ -356,6 +356,7 @@ static uint32_t iswgrp_handoff[8];
> > > >  int arch_early_init_r(void)
> > > >  {
> > > >  	int i;
> > > > +	unsigned int csel;
> > > >  
> > > >  	/*
> > > >  	 * Write magic value into magic register to unlock support for
> > > > @@ -363,8 +364,18 @@ int arch_early_init_r(void)
> > > >  	 * value to be written into the register by the bootloader, so
> > > >  	 * to support that old code, we write it here instead of in the
> > > >  	 * reset_cpu() function just before resetting the CPU.
> > > > +	 *
> > > > +	 * For CSEL = 0 we do not want to enable warm resets to ensure
> > > > that
> > > > +	 * on reset the clocks and plls are reset to their default
> > > > states
> > > > as
> > > > +	 * the bootrom, for CSEL=0, leaves the clocks untouched.  If
> > > > the
> > > > clocks
> > > > +	 * and plls are not reset, the bootrom will fail to load the
> > > > spl
> > > > image.
> > > >  	 */
> > > > -	writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
> > > > +
> > > > +	csel = (readl(&sysmgr_regs->bootinfo) &
> > > > SYSMGR_BOOTINFO_CSEL_MASK)
> > > > > 
> > > > > > 
> > > > > > 
> > > > +		SYSMGR_BOOTINFO_CSEL_LSB;
> > > > +
> > > > +	if (csel)
> > > > +		writel(0xae9efebc, &sysmgr_regs-
> > > > > 
> > > > > romcodegrp_warmramgrp_enable);
> > > >  
> > > >  	for (i = 0; i < 8; i++)	/* Cache initial SW setting regs
> > > > */
> > > >  		iswgrp_handoff[i] = readl(&sysmgr_regs-
> > > > >iswgrp_handoff[i]);
> > > > 
> > > 
> > > 
> 
>