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 <dwest...@gmail.com>
>>>>>>>>
>>>>>>>> --
>>>>>>>> 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 (SOCFPGA_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 <asm/arch/reset_manager.h>
>>>>>>>> #include <asm/arch/scan_manager.h>
>>>>>>>> #include <asm/arch/system_manager.h>
>>>>>>>> +#include <asm/arch/clock_manager.h>
>>>>>>>> #include <asm/arch/nic301.h>
>>>>>>>> #include <asm/arch/scu.h>
>>>>>>>> #include <asm/pl310.h>
>>>>>>>> @@ -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 <config.h>
>>>>>>>> +#include <linux/linkage.h>
>>>>>>>> +#include <asm/arch/system_manager.h>
>>>>>>>> +#include <asm/arch/reset_manager.h>
>>>>>>>> +#include <asm/arch/clock_manager.h>
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + */
>>>>>>>> +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 ?
> 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 ?
> 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 :)
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
