From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dalon Westergreen Date: Mon, 20 Feb 2017 06:21:47 -0800 Subject: [U-Boot] [PATCH v3] arm: socfpga: fix issue with warm reset when CSEL is 0 In-Reply-To: <3f945117-5b43-8f71-0ae8-f2f42f821484@denx.de> References: <1487381696-6409-1-git-send-email-dwesterg@gmail.com> <086715c8-bb1b-6bd2-f585-fedcacbde47f@denx.de> <1487599850.6111.20.camel@gmail.com> <3f945117-5b43-8f71-0ae8-f2f42f821484@denx.de> Message-ID: <1487600507.6111.26.camel@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > > > > > 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]); > > > > > > > > > > > >