From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dalon Westergreen Date: Mon, 20 Feb 2017 06:07:47 -0800 Subject: [U-Boot] [PATCH v2] arm: socfpga: fix issue with warm reset when CSEL is 0 In-Reply-To: <0c7683b6-00b3-0922-6a80-746f296ec832@denx.de> References: <1487096912-18457-1-git-send-email-dwesterg@gmail.com> <1487195313.6617.291.camel@gmail.com> <1487213598.6617.304.camel@gmail.com> <1487354722.6998.35.camel@gmail.com> <1487373879.9640.15.camel@gmail.com> <0c7683b6-00b3-0922-6a80-746f296ec832@denx.de> Message-ID: <1487599667.6111.16.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 Sun, 2017-02-19 at 22:31 +0100, Marek Vasut wrote: > On 02/18/2017 12:24 AM, Dalon Westergreen wrote: > > > > On Fri, 2017-02-17 at 22:16 +0100, Marek Vasut wrote: > > > > > > On 02/17/2017 07:05 PM, Dalon Westergreen wrote: > > > > > > > > > > > > On Wed, 2017-02-15 at 18:53 -0800, Dalon Westergreen wrote: > > > > > > > > > > > > > > > On Wed, 2017-02-15 at 23:20 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 02/15/2017 10:48 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2017-02-15 at 22:15 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 02/14/2017 07:28 PM, 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.??To address this, the bootrom is configured to > > > > > > > > > run a bit of code in the last 4KB of onchip ram on a warm > > > > > > > > > reset. > > > > > > > > > This code puts the PLLs in bypass, disables the bootrom > > > > > > > > > configuration > > > > > > > > > to run the code snippet, and issues a warm reset to run the > > > > > > > > > bootrom. > > > > > > > > > > > > > > > > > > Signed-off-by: Dalon Westergreen > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Changes in V2: > > > > > > > > > ?- Fix checkpatch issues predominently due to whitespace > > > > > > > > > issues > > > > > > > > > --- > > > > > > > > > ?arch/arm/mach-socfpga/Makefile?????????????????????|??2 +- > > > > > > > > > ?arch/arm/mach-socfpga/include/mach/clock_manager.h | 26 > > > > > > > > > +++++++- > > > > > > > > > ?arch/arm/mach-socfpga/include/mach/reset_manager.h |??4 ++ > > > > > > > > > ?.../arm/mach-socfpga/include/mach/system_manager.h |??7 ++- > > > > > > > > > ?arch/arm/mach-socfpga/misc.c???????????????????????| 27 > > > > > > > > > ++++++++ > > > > > > > > > ?arch/arm/mach-socfpga/reset_clock_manager.S????????| 71 > > > > > > > > > ++++++++++++++++++++++ > > > > > > > > > ?6 files changed, 134 insertions(+), 3 deletions(-) > > > > > > > > > ?create mode 100644 arch/arm/mach- > > > > > > > > > socfpga/reset_clock_manager.S > > > > > > > > > > > > > > > > > > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach- > > > > > > > > > socfpga/Makefile > > > > > > > > > index 809cd47..6876ccf 100644 > > > > > > > > > --- a/arch/arm/mach-socfpga/Makefile > > > > > > > > > +++ b/arch/arm/mach-socfpga/Makefile > > > > > > > > > @@ -8,7 +8,7 @@ > > > > > > > > > ?# > > > > > > > > > ? > > > > > > > > > ?obj-y += misc.o timer.o reset_manager.o > > > > > > > > > system_manager.o > > > > > > > > > clock_manager.o \ > > > > > > > > > - ???fpga_manager.o board.o > > > > > > > > > + ???fpga_manager.o board.o reset_clock_manager.o > > > > > > > > > ? > > > > > > > > > ?obj-$(CONFIG_SPL_BUILD) += spl.o freeze_controller.o > > > > > > > > > ? > > > > > > > > > diff --git a/arch/arm/mach- > > > > > > > > > socfpga/include/mach/clock_manager.h > > > > > > > > > b/arch/arm/mach-socfpga/include/mach/clock_manager.h > > > > > > > > > index 803c926..78f63a4 100644 > > > > > > > > > --- a/arch/arm/mach-socfpga/include/mach/clock_manager.h > > > > > > > > > +++ b/arch/arm/mach-socfpga/include/mach/clock_manager.h > > > > > > > > > @@ -19,9 +19,12 @@ const unsigned int cm_get_osc_clk_hz(const > > > > > > > > > int > > > > > > > > > osc); > > > > > > > > > ?const unsigned int cm_get_f2s_per_ref_clk_hz(void); > > > > > > > > > ?const unsigned int cm_get_f2s_sdr_ref_clk_hz(void); > > > > > > > > > ? > > > > > > > > > +/* Onchip RAM functions for CSEL=0 */ > > > > > > > > > +void reset_clock_manager(void); > > > > > > > > > +extern unsigned reset_clock_manager_size; > > > > > > > > > + > > > > > > > > > ?/* Clock configuration accessors */ > > > > > > > > > ?const struct cm_config * const cm_get_default_config(void); > > > > > > > > > -#endif > > > > > > > > > ? > > > > > > > > > ?struct cm_config { > > > > > > > > > ? /* main group */ > > > > > > > > > @@ -127,6 +130,19 @@ struct socfpga_clock_manager { > > > > > > > > > ? struct socfpga_clock_manager_altera altera; > > > > > > > > > ? u32 _pad_0xe8_0x200[70]; > > > > > > > > > ?}; > > > > > > > > > +#endif > > > > > > > > > + > > > > > > > > > +#define CLKMGR_CTRL_ADDRESS 0x0 > > > > > > > > > > > > > > > > Is this the same as struct socfpga_clock_manager {} ? > > > > > > > > Why ? > > > > > > > It is, just defining offsets to use in the assembly calls > > > > > > > > > > > > The asm is IMO not needed > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +#define CLKMGR_BYPASS_ADDRESS 0x4 > > > > > > > > > +#define CLKMGR_INTER_ADDRESS 0x8 > > > > > > > > > +#define CLKMGR_INTREN_ADDRESS 0xc > > > > > > > > > +#define CLKMGR_DBCTRL_ADDRESS 0x10 > > > > > > > > > +#define CLKMGR_STAT_ADDRESS 0x14 > > > > > > > > > +#define CLKMGR_MAINPLLGRP_MAINQSPICLK_ADDRESS 0x54 > > > > > > > > > +#define CLKMGR_MAINPLLGRP_MAINNANDSDMMCCLK_ADDRESS 0x58 > > > > > > > > > +#define CLKMGR_PERPLLGRP_PERQSPICLK_ADDRESS 0x90 > > > > > > > > > +#define CLKMGR_PERPLLGRP_PERNANDSDMMCCLK_ADDRESS 0x94 > > > > > > > > > + > > > > > > > > > ? > > > > > > > > > ?#define CLKMGR_CTRL_SAFEMODE ( > > > > > > > > > 1 << > > > > > > > > > 0) > > > > > > > > > ?#define CLKMGR_CTRL_SAFEMODE_OFFSET 0 > > > > > > > > > @@ -314,4 +330,12 @@ struct socfpga_clock_manager { > > > > > > > > > ?#define CLKMGR_SDRPLLGRP_S2FUSER2CLK_PHASE_OFFSET 9 > > > > > > > > > ?#define CLKMGR_SDRPLLGRP_S2FUSER2CLK_PHASE_MASK > > > > > > > > > 0x > > > > > > > > > 0000 > > > > > > > > > 0e > > > > > > > > > 00 > > > > > > > > > ? > > > > > > > > > +/* Bypass Main and Per PLL, bypass source per input mux */ > > > > > > > > > +#define CLKMGR_BYPASS_MAIN_PER_PLL_MASK?????????0x19 > > > > > > > > > +????????????????????????????????????????????????????????????? > > > > > > > > > ???? > > > > > > > > > ???? > > > > > > > > > ?? > > > > > > > > > ???? > > > > > > > > > ????? > > > > > > > > > +#define CLKMGR_MAINQSPICLK_RESET_VALUE??????????0x3 > > > > > > > > > +#define CLKMGR_MAINNANDSDMMCCLK_RESET_VALUE?????0x3 > > > > > > > > > +#define CLKMGR_PERQSPICLK_RESET_VALUE???????????0x1 > > > > > > > > > +#define CLKMGR_PERNANDSDMMCCLK_RESET_VALUE??????0x1 > > > > > > > > > + > > > > > > > > > ?#endif /* _CLOCK_MANAGER_H_ */ > > > > > > > > > diff --git a/arch/arm/mach- > > > > > > > > > socfpga/include/mach/reset_manager.h > > > > > > > > > b/arch/arm/mach-socfpga/include/mach/reset_manager.h > > > > > > > > > index 2f070f2..58d77fb 100644 > > > > > > > > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h > > > > > > > > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h > > > > > > > > > @@ -7,6 +7,7 @@ > > > > > > > > > ?#ifndef _RESET_MANAGER_H_ > > > > > > > > > ?#define _RESET_MANAGER_H_ > > > > > > > > > ? > > > > > > > > > +#ifndef __ASSEMBLY__ > > > > > > > > > ?void reset_cpu(ulong addr); > > > > > > > > > ?void reset_deassert_peripherals_handoff(void); > > > > > > > > > ? > > > > > > > > > @@ -28,6 +29,8 @@ struct socfpga_reset_manager { > > > > > > > > > ? u32 padding2[12]; > > > > > > > > > ? u32 tstscratch; > > > > > > > > > ?}; > > > > > > > > > +#endif > > > > > > > > > + > > > > > > > > > ? > > > > > > > > > ?#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > > > > > > > > > ?#define RSTMGR_CTRL_SWWARMRSTREQ_LSB 2 > > > > > > > > > @@ -40,6 +43,7 @@ struct socfpga_reset_manager { > > > > > > > > > ? * and reset ID can be extracted using the subsequent macros > > > > > > > > > ? * RSTMGR_RESET() and RSTMGR_BANK(). > > > > > > > > > ? */ > > > > > > > > > +#define RSTMGR_CTRL_OFFSET 4 > > > > > > > > > ?#define RSTMGR_BANK_OFFSET 8 > > > > > > > > > ?#define RSTMGR_BANK_MASK 0x7 > > > > > > > > > ?#define RSTMGR_RESET_OFFSET 0 > > > > > > > > > diff --git a/arch/arm/mach- > > > > > > > > > socfpga/include/mach/system_manager.h > > > > > > > > > b/arch/arm/mach-socfpga/include/mach/system_manager.h > > > > > > > > > index c45edea..b89f269 100644 > > > > > > > > > --- a/arch/arm/mach-socfpga/include/mach/system_manager.h > > > > > > > > > +++ b/arch/arm/mach-socfpga/include/mach/system_manager.h > > > > > > > > > @@ -13,7 +13,6 @@ void sysmgr_pinmux_init(void); > > > > > > > > > ?void sysmgr_config_warmrstcfgio(int enable); > > > > > > > > > ? > > > > > > > > > ?void sysmgr_get_pinmux_table(const u8 **table, unsigned int > > > > > > > > > *table_len); > > > > > > > > > -#endif > > > > > > > > > ? > > > > > > > > > ?struct socfpga_system_manager { > > > > > > > > > ? /* System Manager Module */ > > > > > > > > > @@ -115,6 +114,12 @@ struct socfpga_system_manager { > > > > > > > > > ? u32 _pad_0x734; > > > > > > > > > ? u32 spim0usefpga; /* > > > > > > > > > 0x738 > > > > > > > > > */ > > > > > > > > > ?}; > > > > > > > > > +#endif > > > > > > > > > + > > > > > > > > > +#define CONFIG_SYSMGR_WARMRAMGRP_ENABLE (SOCFP > > > > > > > > > GA_S > > > > > > > > > YSMG > > > > > > > > > R_ > > > > > > > > > ADDR > > > > > > > > > ESS + 0xe0) > > > > > > > > > + > > > > > > > > > +#define SYSMGR_BOOTINFO_CSEL_MASK 0x18 > > > > > > > > > +#define SYSMGR_BOOTINFO_CSEL_LSB 3 > > > > > > > > > ? > > > > > > > > > ?#define SYSMGR_ROMCODEGRP_CTRL_WARMRSTCFGPINMUX (1 << > > > > > > > > > 0) > > > > > > > > > ?#define SYSMGR_ROMCODEGRP_CTRL_WARMRSTCFGIO (1 << 1) > > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- > > > > > > > > > socfpga/misc.c > > > > > > > > > index dd6b53b..57e3334 100644 > > > > > > > > > --- a/arch/arm/mach-socfpga/misc.c > > > > > > > > > +++ b/arch/arm/mach-socfpga/misc.c > > > > > > > > > @@ -16,6 +16,7 @@ > > > > > > > > > ?#include > > > > > > > > > ?#include > > > > > > > > > ?#include > > > > > > > > > +#include > > > > > > > > > ?#include > > > > > > > > > ?#include > > > > > > > > > ?#include > > > > > > > > > @@ -356,6 +357,32 @@ static uint32_t iswgrp_handoff[8]; > > > > > > > > > ?int arch_early_init_r(void) > > > > > > > > > ?{ > > > > > > > > > ? int i; > > > > > > > > > + unsigned csel, ramboot_addr; > > > > > > > > > + > > > > > > > > > + /* Check the CSEL value */ > > > > > > > > > + csel = (readl(&sysmgr_regs->bootinfo) & > > > > > > > > > SYSMGR_BOOTINFO_CSEL_MASK) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + SYSMGR_BOOTINFO_CSEL_LSB; > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + ?* For CSEL = 0 the bootrom does not configure the > > > > > > > > > clocks > > > > > > > > > which > > > > > > > > > can > > > > > > > > > + ?* result in a boot failure on warm resets.??To > > > > > > > > > remedy > > > > > > > > > this a > > > > > > > > > small > > > > > > > > > + ?* bit of code is placed at the end of the onchip ram > > > > > > > > > and > > > > > > > > > run > > > > > > > > > on > > > > > > > > > + ?* a warm reset.??It puts the PLLs in bypass and > > > > > > > > > issues > > > > > > > > > another > > > > > > > > > warm > > > > > > > > > + ?* reset to get back to the bootrom. > > > > > > > > > + ?*/ > > > > > > > > > + if (!csel) { > > > > > > > > > + /* Put the code snippet in the last 4KB of > > > > > > > > > the > > > > > > > > > onchip > > > > > > > > > ram > > > > > > > > > */ > > > > > > > > > + ramboot_addr = CONFIG_SYS_INIT_RAM_ADDR + > > > > > > > > > + CONFIG_SYS_INIT_RAM_SIZE - 0x1000; > > > > > > > > > + > > > > > > > > > + /* Copy the code to the onchip ramlocation */ > > > > > > > > > + memcpy((void *)ramboot_addr, > > > > > > > > > reset_clock_manager, > > > > > > > > > + ??????reset_clock_manager_size); > > > > > > > > > > > > > > > > So uh, why don't you put this code into SPL and execute it from > > > > > > > > there ? > > > > > > > > This is b/s ... > > > > > > > > > > > > > > you are correct, it should be setup in the SPL.??that said though, > > > > > > > should the entire setup of the warm reset in this function be > > > > > > > moved > > > > > > > to spl???the warm reset is enabled by writing that "magic" number > > > > > > > into a "magic" register.??currently this is not done in SPL which > > > > > > > is why i put this where i did. > > > > > > > > > > > > Well yes, the SPL does the magic init of the platform. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + /* Set the bootrom to run the code snippet on > > > > > > > > > reset > > > > > > > > > */ > > > > > > > > > + writel(ramboot_addr, > > > > > > > > > + ??????&sysmgr_regs- > > > > > > > > > > > > > > > > > > > > romcodegrp_warmramgrp_execution); > > > > > > > > > + } > > > > > > > > > ? > > > > > > > > > ? /* > > > > > > > > > ? ?* Write magic value into magic register to unlock > > > > > > > > > support > > > > > > > > > for > > > > > > > > > diff --git a/arch/arm/mach-socfpga/reset_clock_manager.S > > > > > > > > > b/arch/arm/mach- > > > > > > > > > socfpga/reset_clock_manager.S > > > > > > > > > new file mode 100644 > > > > > > > > > index 0000000..1818b2d > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/arch/arm/mach-socfpga/reset_clock_manager.S > > > > > > > > > @@ -0,0 +1,71 @@ > > > > > > > > > +/* > > > > > > > > > + * Copyright (C) 2017, Intel Corporation > > > > > > > > > + * > > > > > > > > > + * SPDX-License-Identifier:?????GPL-2.0+ > > > > > > > > > + */ > > > > > > > > > + > > > > > > > > > +#include > > > > > > > > > +#include > > > > > > > > > +#include > > > > > > > > > +#include > > > > > > > > > +#include > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + */ > > > > > > > > > +ENTRY(reset_clock_manager) > > > > > > > > > > > > > > > > This is just a few writel() calls in SPL , right ? Put it > > > > > > > > there... > > > > > > > > > > > > > > Although this is just a few write calls, i dont believe just doing > > > > > > > this in SPL will work.??The intent is to copy the code snippet > > > > > > > to onchip ram so on a warm reset the code below is executed. > > > > > > > > > > > > SPL is running from SRAM already , so what's the problem here ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > If it > > > > > > > is not executed, it is possible that the bootrom (when CSEL=0) > > > > > > > will > > > > > > > be unable to fetch SPL at all. > > > > > > > > > > > > Why ? And how will you be able to enter this code (which is running > > > > > > from > > > > > > actual u-boot, which is itself loaded by SPL probably ...) if > > > > > > the bootrom cannot fetch SPL??? > > > > > > > > > > I think i am not being clear.??This issue is not power on reset, it > > > > > is after a warm reset.??When CSEL=0 the bootrom does no configuration > > > > > or changes of the pll/clock settings.??On power up, this isnt an > > > > > issue as the plls are bypased.??On a warm reset, the clocks and > > > > > plls are left alone with csel=0, and as a result they are left running > > > > > at whatever speed they were set at during the initial boot.??The > > > > > bootrom makes assumptions about the clock state and does not setup > > > > > the sdcard/qspi/nand appropriately for the clock configuration. as > > > > > a result, the bootrom will be unable to load the spl image on this > > > > > second warm reset.?? > > > > > > > > > > The work around is to place a code snippet ( the asm stuff below ) > > > > > in the onchip ram in a "reserved" area, allowing use of the reset > > > > > of the onchip ram for any purpose.??The bootrom is then configured > > > > > to run this code snippet on a warm reset which could occur post > > > > > boot.??The code snippet places the clocks in a known state (bypass) > > > > > and resets again to initiate the bootrom. > > > > > > > > > > I am not sure how plausible it is just to, on warm reset, have the > > > > > bootrom run the spl image in onchip ram and just reserve the entire > > > > > ram for that purpose when csel=0. > > > > > > > > > > i hope this clears up the problem, again, any thoughts are welcome. > > > > > > > > > > --dalon > > > > > > > > Any comments on this Marek???I am not sure there is a reasonable way > > > > do this without the assembly snippet.??The snippet is not run during > > > > uboot or spl, it is un on a warm reset.??I do agree this setup during > > > > spl, before i do that though i would like to understand if you have > > > > any better ways to do this???When CSEL=0 code needs to be run after > > > > a warm reset and before the bootrom code is run again to place > > > > the clocks in a known configuration. > > > > > > I just got back from the airport, catching up on email. > > > What about doing cold reset, SoCFPGA is capable of that. I was actually > > > pondering why the heck do we do warm reset at all ... > > > > Okay, after much discussion and debate with a colleague..\ > > > > Warm reset is preferred as the bootrom keeps a score card to determine > > whether an spl image in the boot media failed or not.??If it failed, > > on a warm reset it will not retry the failed image. > > So what will it do ? Try a valid image from another slot at offset > +n*64kiB ? Yes. ?Or when the scorecard indicates 4 failures it will just stop. > > > > The other reason warm resets are preferred is for preservation of the > > dram contents.??On a warm reset it is possible to skip io configuration > > and dram calibration so that the contents can be saved. > > That's a good point. > > But here's a counterargument, what if you upgrade U-Boot ? Warm reset > will use the old SPL and the system will likely hang upon reboot ;-) > > > > > > > > > How do you point bootrom to run that snippet instead of whatever is in > > > the OCRAM ? > > > > > > > This code here > > > > > > > > ?> > > > > > + writel(ramboot_addr, > > > ?> > > > > > + ??????&sysmgr_regs- > > > ?> > > > > > >romcodegrp_warmramgrp_execution); > > Can't you just feed a function pointer pointing into some function which > is part of the SPL into that register then ? I think that'd do the same > trick, no ? Yes, you could, but the idea of putting the code at the end of memory is to allow the onchip ram to be used for other things. > > > > writes the address to jump to on warm reset.??The register value > > is preserved through a warm reset. > > That's neat, I didn't know that. > > > > > > > > > > > > > All that said, i frankly do not believe for the CSEL=0 case > > there is any merit to doing the above.??Although it "preserves" > > the behaviour as compared to other CSEL values, i think a much > > simpler method is to, for the CSEL=0 case, just issue a cold reset. > > > > As in this case we are touching the clocks, i am not sure the > > use cases for a warm reset even hold (sdram preservation, etc). > > So i agree with you, and suggest only enabling the warm reset > > for cases where CSEL != 0. > > > > Unless there are objections, I will do just that and resubmit a > > patch. (which should now be just a few lines of code) > > See above, if this actually fixes issue, let's get it in. But in a > civilized fashion, no random ad-hoc asm if possible please :) In v3 i just used the simpler method of not allowing warm resets for csel=0. This is far cleaner, and likely more reliable. --dalon