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

[Marek Vasut]

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 :)

> 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
> 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]);
>>>
>>
>>


-- 
Best regards,
Marek Vasut