[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Simon Goldschmidt]

This removes the code that enables the Boot ROM to just jump to SRAM
instead of loading SPL from the original boot source on warm reboot.

The reason for removing this is that it is insecure: SRAM might be
overwritten at the time the warm reboot is done. Instead, use the default
behaviour of loading SPL from the configured boot source medium.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

Changes in v2:
- remove the compatibility code restoring the old "reboot from SRAM"
  behaviour via an env var callback as it turned out such a hack should
  not be included by default
- (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
  callback")

 arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
index 71547d81ab..38bff8a450 100644
--- a/arch/arm/mach-socfpga/misc_gen5.c
+++ b/arch/arm/mach-socfpga/misc_gen5.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <errno.h>
+#include <environment.h>
 #include <fdtdec.h>
 #include <linux/libfdt.h>
 #include <altera.h>
@@ -182,15 +183,6 @@ int arch_early_init_r(void)
 {
 	int i;
 
-	/*
-	 * Write magic value into magic register to unlock support for
-	 * issuing warm reset. The ancient kernel code expects this
-	 * 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.
-	 */
-	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]);
 
-- 
2.20.1

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Marek Vasut]

On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
> This removes the code that enables the Boot ROM to just jump to SRAM
> instead of loading SPL from the original boot source on warm reboot.
> 
> The reason for removing this is that it is insecure: SRAM might be
> overwritten at the time the warm reboot is done. Instead, use the default
> behaviour of loading SPL from the configured boot source medium.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
> Changes in v2:
> - remove the compatibility code restoring the old "reboot from SRAM"
>   behaviour via an env var callback as it turned out such a hack should
>   not be included by default
> - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
>   callback")
> 
>  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> index 71547d81ab..38bff8a450 100644
> --- a/arch/arm/mach-socfpga/misc_gen5.c
> +++ b/arch/arm/mach-socfpga/misc_gen5.c
> @@ -6,6 +6,7 @@
>  #include <common.h>
>  #include <asm/io.h>
>  #include <errno.h>
> +#include <environment.h>
>  #include <fdtdec.h>
>  #include <linux/libfdt.h>
>  #include <altera.h>
> @@ -182,15 +183,6 @@ int arch_early_init_r(void)
>  {
>  	int i;
>  
> -	/*
> -	 * Write magic value into magic register to unlock support for
> -	 * issuing warm reset. The ancient kernel code expects this
> -	 * 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.
> -	 */
> -	writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);

Does this break ancient altera kernel versions ? Do we care ?

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Simon Goldschmidt]

On Fri, Jul 12, 2019 at 7:15 AM Marek Vasut <marex@denx.de> wrote:
>
> On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
> > This removes the code that enables the Boot ROM to just jump to SRAM
> > instead of loading SPL from the original boot source on warm reboot.
> >
> > The reason for removing this is that it is insecure: SRAM might be
> > overwritten at the time the warm reboot is done. Instead, use the default
> > behaviour of loading SPL from the configured boot source medium.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> > Changes in v2:
> > - remove the compatibility code restoring the old "reboot from SRAM"
> >   behaviour via an env var callback as it turned out such a hack should
> >   not be included by default
> > - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
> >   callback")
> >
> >  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> > index 71547d81ab..38bff8a450 100644
> > --- a/arch/arm/mach-socfpga/misc_gen5.c
> > +++ b/arch/arm/mach-socfpga/misc_gen5.c
> > @@ -6,6 +6,7 @@
> >  #include <common.h>
> >  #include <asm/io.h>
> >  #include <errno.h>
> > +#include <environment.h>
> >  #include <fdtdec.h>
> >  #include <linux/libfdt.h>
> >  #include <altera.h>
> > @@ -182,15 +183,6 @@ int arch_early_init_r(void)
> >  {
> >       int i;
> >
> > -     /*
> > -      * Write magic value into magic register to unlock support for
> > -      * issuing warm reset. The ancient kernel code expects this
> > -      * 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.
> > -      */
> > -     writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
>
> Does this break ancient altera kernel versions ? Do we care ?

You mean ancient kernels built from non-mainline sources? I don't really care,
no.

However, this *might* break boards like my older hw revisions that set qspi into
four byte mode. But given that, depending on the situation, those *are* already
broken, I think removing this code is still the correct thing to do.

The bonus is that you'll notice on the very first try that 'reboot' doesn't
work. Whereas before,t it worked at the start and then might break in some
specific situation you'll not be able to test.

Regards,
Simon

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Marek Vasut]

On 7/12/19 7:20 AM, Simon Goldschmidt wrote:
> On Fri, Jul 12, 2019 at 7:15 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
>>> This removes the code that enables the Boot ROM to just jump to SRAM
>>> instead of loading SPL from the original boot source on warm reboot.
>>>
>>> The reason for removing this is that it is insecure: SRAM might be
>>> overwritten at the time the warm reboot is done. Instead, use the default
>>> behaviour of loading SPL from the configured boot source medium.
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> ---
>>>
>>> Changes in v2:
>>> - remove the compatibility code restoring the old "reboot from SRAM"
>>>   behaviour via an env var callback as it turned out such a hack should
>>>   not be included by default
>>> - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
>>>   callback")
>>>
>>>  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
>>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
>>> index 71547d81ab..38bff8a450 100644
>>> --- a/arch/arm/mach-socfpga/misc_gen5.c
>>> +++ b/arch/arm/mach-socfpga/misc_gen5.c
>>> @@ -6,6 +6,7 @@
>>>  #include <common.h>
>>>  #include <asm/io.h>
>>>  #include <errno.h>
>>> +#include <environment.h>
>>>  #include <fdtdec.h>
>>>  #include <linux/libfdt.h>
>>>  #include <altera.h>
>>> @@ -182,15 +183,6 @@ int arch_early_init_r(void)
>>>  {
>>>       int i;
>>>
>>> -     /*
>>> -      * Write magic value into magic register to unlock support for
>>> -      * issuing warm reset. The ancient kernel code expects this
>>> -      * 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.
>>> -      */
>>> -     writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
>>
>> Does this break ancient altera kernel versions ? Do we care ?
> 
> You mean ancient kernels built from non-mainline sources? I don't really care,
> no.
> 
> However, this *might* break boards like my older hw revisions that set qspi into
> four byte mode. But given that, depending on the situation, those *are* already
> broken, I think removing this code is still the correct thing to do.
> 
> The bonus is that you'll notice on the very first try that 'reboot' doesn't
> work. Whereas before,t it worked at the start and then might break in some
> specific situation you'll not be able to test.

Does reboot still work in mainline Linux with this ?
I am somewhat reluctant to apply this patch, since I recall there was
some weird reason this write was added, but I don't remember what it was.

-- 
Best regards,
Marek Vasut

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-56 Email: marex at denx.de

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Simon Goldschmidt]

On Fri, Jul 12, 2019 at 7:27 AM Marek Vasut <marex@denx.de> wrote:
>
> On 7/12/19 7:20 AM, Simon Goldschmidt wrote:
> > On Fri, Jul 12, 2019 at 7:15 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
> >>> This removes the code that enables the Boot ROM to just jump to SRAM
> >>> instead of loading SPL from the original boot source on warm reboot.
> >>>
> >>> The reason for removing this is that it is insecure: SRAM might be
> >>> overwritten at the time the warm reboot is done. Instead, use the default
> >>> behaviour of loading SPL from the configured boot source medium.
> >>>
> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - remove the compatibility code restoring the old "reboot from SRAM"
> >>>   behaviour via an env var callback as it turned out such a hack should
> >>>   not be included by default
> >>> - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
> >>>   callback")
> >>>
> >>>  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
> >>>  1 file changed, 1 insertion(+), 9 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> >>> index 71547d81ab..38bff8a450 100644
> >>> --- a/arch/arm/mach-socfpga/misc_gen5.c
> >>> +++ b/arch/arm/mach-socfpga/misc_gen5.c
> >>> @@ -6,6 +6,7 @@
> >>>  #include <common.h>
> >>>  #include <asm/io.h>
> >>>  #include <errno.h>
> >>> +#include <environment.h>
> >>>  #include <fdtdec.h>
> >>>  #include <linux/libfdt.h>
> >>>  #include <altera.h>
> >>> @@ -182,15 +183,6 @@ int arch_early_init_r(void)
> >>>  {
> >>>       int i;
> >>>
> >>> -     /*
> >>> -      * Write magic value into magic register to unlock support for
> >>> -      * issuing warm reset. The ancient kernel code expects this
> >>> -      * 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.
> >>> -      */
> >>> -     writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
> >>
> >> Does this break ancient altera kernel versions ? Do we care ?
> >
> > You mean ancient kernels built from non-mainline sources? I don't really care,
> > no.
> >
> > However, this *might* break boards like my older hw revisions that set qspi into
> > four byte mode. But given that, depending on the situation, those *are* already
> > broken, I think removing this code is still the correct thing to do.
> >
> > The bonus is that you'll notice on the very first try that 'reboot' doesn't
> > work. Whereas before,t it worked at the start and then might break in some
> > specific situation you'll not be able to test.
>
> Does reboot still work in mainline Linux with this ?
> I am somewhat reluctant to apply this patch, since I recall there was
> some weird reason this write was added, but I don't remember what it was.

I can understand being reluctant about this. I can tell you my board reboots OK,
so does socfpga_socrates. There are, however, two types of boards that might not
reboot ok:

a) boards with broken reset circuits (where the qspi chip does not get
reset on reboot) that set the qspi into 4 byte mode (so the boot rom cannot load
SPL from flash)

b) if I remember correctly the discussion with Dalon some months ago: boards
that have 'csel=0' and set a very high clock rate as input clock to the qspi
hardware: boot rom sees 'csel=0' and thinks the input clock rate is slow (while
it actually is high since warm reboot doesn't change the clocks). So it uses a
small divider and this might result in a transfer speed higher than the flash
chip can handle.

My position to 'a' is that yes, we break such boards but they should just write
the magic on their own, which can even be done with a U-Boot script.

'b' boards are a bit more delicate. I can't tell how widely this is used. I
think all mainline boards other than socfgpa_socrates boot from mmc, not from
qspi. And socfgpa_socrates should not be affected as it has csel='11'.

One fix for these 'b' boards is to reboot 'cold', not 'warm'. Unfortunately,
up to now, warm reboot is the default for Linux. It can be overridden by passing
"reboot=hard" on the kernel command line. I think I've reached consent with Dinh
to change the behaviour to let 'cold' reboot be the standard (which does reset
clocks, so boot rom correctly handles 'csel=0'), but that won't be active until
Linux v5.4, I guess (haven't found the time to post the patch, yet).

However, the current state is just not safe: if reboot is triggered by a
watchdog, e.g. because the cpu runs invalid code, how can we rely on the SRAM
being in the correct state? We'll want to load SPL from flash.

Regards,
Simon

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Marek Vasut]

On 7/12/19 8:00 AM, Simon Goldschmidt wrote:
> On Fri, Jul 12, 2019 at 7:27 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/12/19 7:20 AM, Simon Goldschmidt wrote:
>>> On Fri, Jul 12, 2019 at 7:15 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
>>>>> This removes the code that enables the Boot ROM to just jump to SRAM
>>>>> instead of loading SPL from the original boot source on warm reboot.
>>>>>
>>>>> The reason for removing this is that it is insecure: SRAM might be
>>>>> overwritten at the time the warm reboot is done. Instead, use the default
>>>>> behaviour of loading SPL from the configured boot source medium.
>>>>>
>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - remove the compatibility code restoring the old "reboot from SRAM"
>>>>>   behaviour via an env var callback as it turned out such a hack should
>>>>>   not be included by default
>>>>> - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
>>>>>   callback")
>>>>>
>>>>>  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
>>>>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
>>>>> index 71547d81ab..38bff8a450 100644
>>>>> --- a/arch/arm/mach-socfpga/misc_gen5.c
>>>>> +++ b/arch/arm/mach-socfpga/misc_gen5.c
>>>>> @@ -6,6 +6,7 @@
>>>>>  #include <common.h>
>>>>>  #include <asm/io.h>
>>>>>  #include <errno.h>
>>>>> +#include <environment.h>
>>>>>  #include <fdtdec.h>
>>>>>  #include <linux/libfdt.h>
>>>>>  #include <altera.h>
>>>>> @@ -182,15 +183,6 @@ int arch_early_init_r(void)
>>>>>  {
>>>>>       int i;
>>>>>
>>>>> -     /*
>>>>> -      * Write magic value into magic register to unlock support for
>>>>> -      * issuing warm reset. The ancient kernel code expects this
>>>>> -      * 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.
>>>>> -      */
>>>>> -     writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
>>>>
>>>> Does this break ancient altera kernel versions ? Do we care ?
>>>
>>> You mean ancient kernels built from non-mainline sources? I don't really care,
>>> no.
>>>
>>> However, this *might* break boards like my older hw revisions that set qspi into
>>> four byte mode. But given that, depending on the situation, those *are* already
>>> broken, I think removing this code is still the correct thing to do.
>>>
>>> The bonus is that you'll notice on the very first try that 'reboot' doesn't
>>> work. Whereas before,t it worked at the start and then might break in some
>>> specific situation you'll not be able to test.
>>
>> Does reboot still work in mainline Linux with this ?
>> I am somewhat reluctant to apply this patch, since I recall there was
>> some weird reason this write was added, but I don't remember what it was.
> 
> I can understand being reluctant about this. I can tell you my board reboots OK,
> so does socfpga_socrates. There are, however, two types of boards that might not
> reboot ok:
> 
> a) boards with broken reset circuits (where the qspi chip does not get
> reset on reboot) that set the qspi into 4 byte mode (so the boot rom cannot load
> SPL from flash)
> 
> b) if I remember correctly the discussion with Dalon some months ago: boards
> that have 'csel=0' and set a very high clock rate as input clock to the qspi
> hardware: boot rom sees 'csel=0' and thinks the input clock rate is slow (while
> it actually is high since warm reboot doesn't change the clocks). So it uses a
> small divider and this might result in a transfer speed higher than the flash
> chip can handle.
> 
> My position to 'a' is that yes, we break such boards but they should just write
> the magic on their own, which can even be done with a U-Boot script.
> 
> 'b' boards are a bit more delicate. I can't tell how widely this is used. I
> think all mainline boards other than socfgpa_socrates boot from mmc, not from
> qspi. And socfgpa_socrates should not be affected as it has csel='11'.

At least vining-fpga boots from QSPI NOR. IS1 I think too. csel=0 is
quite common, because it's recommended by Altera.

> One fix for these 'b' boards is to reboot 'cold', not 'warm'. Unfortunately,
> up to now, warm reboot is the default for Linux. It can be overridden by passing
> "reboot=hard" on the kernel command line. I think I've reached consent with Dinh
> to change the behaviour to let 'cold' reboot be the standard (which does reset
> clocks, so boot rom correctly handles 'csel=0'), but that won't be active until
> Linux v5.4, I guess (haven't found the time to post the patch, yet).
> 
> However, the current state is just not safe: if reboot is triggered by a
> watchdog, e.g. because the cpu runs invalid code, how can we rely on the SRAM
> being in the correct state? We'll want to load SPL from flash.

Doesn't the bootrom check some checksum of the blob in SRAM, and reload
the SRAM code from storage if the checksum doesn't match ?

[U-Boot] [PATCH v2] arm: socfpga: do not reboot via SRAM

[Simon Goldschmidt]

On Fri, Jul 12, 2019 at 12:29 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/12/19 8:00 AM, Simon Goldschmidt wrote:
> > On Fri, Jul 12, 2019 at 7:27 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/12/19 7:20 AM, Simon Goldschmidt wrote:
> >>> On Fri, Jul 12, 2019 at 7:15 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 7/10/19 9:06 PM, Simon Goldschmidt wrote:
> >>>>> This removes the code that enables the Boot ROM to just jump to SRAM
> >>>>> instead of loading SPL from the original boot source on warm reboot.
> >>>>>
> >>>>> The reason for removing this is that it is insecure: SRAM might be
> >>>>> overwritten at the time the warm reboot is done. Instead, use the default
> >>>>> behaviour of loading SPL from the configured boot source medium.
> >>>>>
> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >>>>> ---
> >>>>>
> >>>>> Changes in v2:
> >>>>> - remove the compatibility code restoring the old "reboot from SRAM"
> >>>>>   behaviour via an env var callback as it turned out such a hack should
> >>>>>   not be included by default
> >>>>> - (v1 patch subject was: "arm: socfpga: control reboot from SRAM via env
> >>>>>   callback")
> >>>>>
> >>>>>  arch/arm/mach-socfpga/misc_gen5.c | 10 +---------
> >>>>>  1 file changed, 1 insertion(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> >>>>> index 71547d81ab..38bff8a450 100644
> >>>>> --- a/arch/arm/mach-socfpga/misc_gen5.c
> >>>>> +++ b/arch/arm/mach-socfpga/misc_gen5.c
> >>>>> @@ -6,6 +6,7 @@
> >>>>>  #include <common.h>
> >>>>>  #include <asm/io.h>
> >>>>>  #include <errno.h>
> >>>>> +#include <environment.h>
> >>>>>  #include <fdtdec.h>
> >>>>>  #include <linux/libfdt.h>
> >>>>>  #include <altera.h>
> >>>>> @@ -182,15 +183,6 @@ int arch_early_init_r(void)
> >>>>>  {
> >>>>>       int i;
> >>>>>
> >>>>> -     /*
> >>>>> -      * Write magic value into magic register to unlock support for
> >>>>> -      * issuing warm reset. The ancient kernel code expects this
> >>>>> -      * 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.
> >>>>> -      */
> >>>>> -     writel(0xae9efebc, &sysmgr_regs->romcodegrp_warmramgrp_enable);
> >>>>
> >>>> Does this break ancient altera kernel versions ? Do we care ?
> >>>
> >>> You mean ancient kernels built from non-mainline sources? I don't really care,
> >>> no.
> >>>
> >>> However, this *might* break boards like my older hw revisions that set qspi into
> >>> four byte mode. But given that, depending on the situation, those *are* already
> >>> broken, I think removing this code is still the correct thing to do.
> >>>
> >>> The bonus is that you'll notice on the very first try that 'reboot' doesn't
> >>> work. Whereas before,t it worked at the start and then might break in some
> >>> specific situation you'll not be able to test.
> >>
> >> Does reboot still work in mainline Linux with this ?
> >> I am somewhat reluctant to apply this patch, since I recall there was
> >> some weird reason this write was added, but I don't remember what it was.
> >
> > I can understand being reluctant about this. I can tell you my board reboots OK,
> > so does socfpga_socrates. There are, however, two types of boards that might not
> > reboot ok:
> >
> > a) boards with broken reset circuits (where the qspi chip does not get
> > reset on reboot) that set the qspi into 4 byte mode (so the boot rom cannot load
> > SPL from flash)
> >
> > b) if I remember correctly the discussion with Dalon some months ago: boards
> > that have 'csel=0' and set a very high clock rate as input clock to the qspi
> > hardware: boot rom sees 'csel=0' and thinks the input clock rate is slow (while
> > it actually is high since warm reboot doesn't change the clocks). So it uses a
> > small divider and this might result in a transfer speed higher than the flash
> > chip can handle.
> >
> > My position to 'a' is that yes, we break such boards but they should just write
> > the magic on their own, which can even be done with a U-Boot script.
> >
> > 'b' boards are a bit more delicate. I can't tell how widely this is used. I
> > think all mainline boards other than socfgpa_socrates boot from mmc, not from
> > qspi. And socfgpa_socrates should not be affected as it has csel='11'.
>
> At least vining-fpga boots from QSPI NOR. IS1 I think too. csel=0 is
> quite common, because it's recommended by Altera.

Oh, ok.

>
> > One fix for these 'b' boards is to reboot 'cold', not 'warm'. Unfortunately,
> > up to now, warm reboot is the default for Linux. It can be overridden by passing
> > "reboot=hard" on the kernel command line. I think I've reached consent with Dinh
> > to change the behaviour to let 'cold' reboot be the standard (which does reset
> > clocks, so boot rom correctly handles 'csel=0'), but that won't be active until
> > Linux v5.4, I guess (haven't found the time to post the patch, yet).
> >
> > However, the current state is just not safe: if reboot is triggered by a
> > watchdog, e.g. because the cpu runs invalid code, how can we rely on the SRAM
> > being in the correct state? We'll want to load SPL from flash.
>
> Doesn't the bootrom check some checksum of the blob in SRAM, and reload
> the SRAM code from storage if the checksum doesn't match ?

It can, but it's not configured to do so. I started to implement that
a while ago,
but the result was that current SPL implementation does not support it for 2
reasons:

1) memory layout with external DTB interleaves writable BSS with read-only
sections while the CRC-checked region must be in one piece (offset + length)

2) U-Boot DM stores drivers and uclass information in lists sections. However,
these entries are not readonly but initialized and later written to
(partly). So that
means we cannot check CRC on them on warm reboot.

OK, so you think there *are* boards where we need this behaviour?
I just think it's bad design to reboot via SRAM (you also lose the possibility
to use this SRAM for something else during runtime), so how do we continue?

Regards,
Simon