diff mbox series

[22/26,v6] spl: spl_legacy: Add cache flush after reading U-Boot image

Message ID 20200408080942.7694-23-sr@denx.de
State New
Headers show
Series Refactor the architecture parts of mt7628 | expand

Commit Message

Stefan Roese April 8, 2020, 8:09 a.m. UTC 
From: Weijie Gao <weijie.gao at mediatek.com>

Flush the cache after reading of the U-Boot proper into SDRAM so that
it can be started.

This is needed on some platforms, e.g. MT76x8.

Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
Signed-off-by: Stefan Roese <sr at denx.de>
Cc: Weijie Gao <weijie.gao at mediatek.com>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
---
Changes in v6:
- New patch

 common/spl/spl_legacy.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Simon Goldschmidt April 9, 2020, 7:29 a.m. UTC | #1 
Am 08.04.2020 um 10:09 schrieb Stefan Roese:
> From: Weijie Gao <weijie.gao at mediatek.com>
> 
> Flush the cache after reading of the U-Boot proper into SDRAM so that
> it can be started.
> 
> This is needed on some platforms, e.g. MT76x8.
> 
> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Weijie Gao <weijie.gao at mediatek.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> ---
> Changes in v6:
> - New patch
> 
>  common/spl/spl_legacy.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> index 2cd2a74a4c..e320206098 100644
> --- a/common/spl/spl_legacy.c
> +++ b/common/spl/spl_legacy.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <common.h>
> +#include <cpu_func.h>
>  #include <malloc.h>
>  #include <spl.h>
>  
> @@ -108,5 +109,8 @@ int spl_load_legacy_img(struct spl_image_info *spl_image,
>  		return -EINVAL;
>  	}
>  
> +	/* Flush cache of loaded U-Boot image */
> +	flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
> +

I failed to find the mail, but haven't we discussed moving this cache
flush to your arch before starting a binary?

I cannot see this being required or implemented for non-legacy images,
and it still seems wrong here.

Regards,
Simon

>  	return 0;
>  }
>
Stefan Roese April 9, 2020, 7:43 a.m. UTC | #2 
On 09.04.20 09:29, Simon Goldschmidt wrote:
> Am 08.04.2020 um 10:09 schrieb Stefan Roese:
>> From: Weijie Gao <weijie.gao at mediatek.com>
>>
>> Flush the cache after reading of the U-Boot proper into SDRAM so that
>> it can be started.
>>
>> This is needed on some platforms, e.g. MT76x8.
>>
>> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> ---
>> Changes in v6:
>> - New patch
>>
>>   common/spl/spl_legacy.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>> index 2cd2a74a4c..e320206098 100644
>> --- a/common/spl/spl_legacy.c
>> +++ b/common/spl/spl_legacy.c
>> @@ -4,6 +4,7 @@
>>    */
>>   
>>   #include <common.h>
>> +#include <cpu_func.h>
>>   #include <malloc.h>
>>   #include <spl.h>
>>   
>> @@ -108,5 +109,8 @@ int spl_load_legacy_img(struct spl_image_info *spl_image,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Flush cache of loaded U-Boot image */
>> +	flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
>> +
> 
> I failed to find the mail, but haven't we discussed moving this cache
> flush to your arch before starting a binary?

I don't remember such an agreement. But I don't object in general.

> I cannot see this being required or implemented for non-legacy images,
> and it still seems wrong here.

Its pretty common when an OS image is loaded and booted, that the cache
is flushed before running code from it.

But I can rework this to add some empty weak function (I don't see an
easy better way) to do this platform specific image handling before its
booted. Or do you have a better idea on how to handle this?

Thanks,
Stefan
Daniel Schwierzeck April 9, 2020, 4:47 p.m. UTC | #3 
Am 09.04.20 um 09:43 schrieb Stefan Roese:
> On 09.04.20 09:29, Simon Goldschmidt wrote:
>> Am 08.04.2020 um 10:09 schrieb Stefan Roese:
>>> From: Weijie Gao <weijie.gao at mediatek.com>
>>>
>>> Flush the cache after reading of the U-Boot proper into SDRAM so that
>>> it can be started.
>>>
>>> This is needed on some platforms, e.g. MT76x8.
>>>
>>> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>> ---
>>> Changes in v6:
>>> - New patch
>>>
>>> ? common/spl/spl_legacy.c | 4 ++++
>>> ? 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>>> index 2cd2a74a4c..e320206098 100644
>>> --- a/common/spl/spl_legacy.c
>>> +++ b/common/spl/spl_legacy.c
>>> @@ -4,6 +4,7 @@
>>> ?? */
>>> ? ? #include <common.h>
>>> +#include <cpu_func.h>
>>> ? #include <malloc.h>
>>> ? #include <spl.h>
>>> ? @@ -108,5 +109,8 @@ int spl_load_legacy_img(struct spl_image_info
>>> *spl_image,
>>> ????????? return -EINVAL;
>>> ????? }
>>> ? +??? /* Flush cache of loaded U-Boot image */
>>> +??? flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
>>> +
>>
>> I failed to find the mail, but haven't we discussed moving this cache
>> flush to your arch before starting a binary?
> 
> I don't remember such an agreement. But I don't object in general.
> 
>> I cannot see this being required or implemented for non-legacy images,
>> and it still seems wrong here.
> 
> Its pretty common when an OS image is loaded and booted, that the cache
> is flushed before running code from it.
> 
> But I can rework this to add some empty weak function (I don't see an
> easy better way) to do this platform specific image handling before its
> booted. Or do you have a better idea on how to handle this?
> 
> Thanks,
> Stefan

actually all MIPS platforms with non-coherent cache need that flush
before jumping to another U-Boot or OS. Thus currently random crashes
can occur on all MIPS boards with generic SPL not just mtmips.

But jump_to_image_no_args() in spl.c is already declared as weak, so we
should implement that unconditionally for MIPS similar to your patch for
do_go_exec(). It would be great if you could prepare a patch to replace
this one, thanks.
Simon Goldschmidt April 9, 2020, 6:15 p.m. UTC | #4 
Am 09.04.2020 um 09:43 schrieb Stefan Roese:
> On 09.04.20 09:29, Simon Goldschmidt wrote:
>> Am 08.04.2020 um 10:09 schrieb Stefan Roese:
>>> From: Weijie Gao <weijie.gao at mediatek.com>
>>>
>>> Flush the cache after reading of the U-Boot proper into SDRAM so that
>>> it can be started.
>>>
>>> This is needed on some platforms, e.g. MT76x8.
>>>
>>> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>> ---
>>> Changes in v6:
>>> - New patch
>>>
>>>   common/spl/spl_legacy.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>>> index 2cd2a74a4c..e320206098 100644
>>> --- a/common/spl/spl_legacy.c
>>> +++ b/common/spl/spl_legacy.c
>>> @@ -4,6 +4,7 @@
>>>    */
>>>   
>>>   #include <common.h>
>>> +#include <cpu_func.h>
>>>   #include <malloc.h>
>>>   #include <spl.h>
>>>   
>>> @@ -108,5 +109,8 @@ int spl_load_legacy_img(struct spl_image_info *spl_image,
>>>   		return -EINVAL;
>>>   	}
>>>   
>>> +	/* Flush cache of loaded U-Boot image */
>>> +	flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
>>> +
>>
>> I failed to find the mail, but haven't we discussed moving this cache
>> flush to your arch before starting a binary?
> 
> I don't remember such an agreement. But I don't object in general.

Ok, maybe I remember wrong then.

> 
>> I cannot see this being required or implemented for non-legacy images,
>> and it still seems wrong here.
> 
> Its pretty common when an OS image is loaded and booted, that the cache
> is flushed before running code from it.

Yes, of course. I can follow you there. I'm just curious why it's added
here and only for legacy images.

> 
> But I can rework this to add some empty weak function (I don't see an
> easy better way) to do this platform specific image handling before its
> booted. Or do you have a better idea on how to handle this?

I don't know. And I'm not totally opposed to this patch. I just think
it's a strange thing to do here. If you need it, I think it should be in
a more central place. Surely, you'd need it for non-legacy images, too?

I could imagine this being added to jump_to_image_no_args(). That way,
we would have the cache flush in a central place instead of stumbling
accross it again in the future (e.g. for non-legacy images). OTOH, I'm
not sure that would be free of side-effects...?

Regards,
Simon


> 
> Thanks,
> Stefan
>
Stefan Roese April 10, 2020, 7:50 a.m. UTC | #5 
On 09.04.20 18:47, Daniel Schwierzeck wrote:
> 
> 
> Am 09.04.20 um 09:43 schrieb Stefan Roese:
>> On 09.04.20 09:29, Simon Goldschmidt wrote:
>>> Am 08.04.2020 um 10:09 schrieb Stefan Roese:
>>>> From: Weijie Gao <weijie.gao at mediatek.com>
>>>>
>>>> Flush the cache after reading of the U-Boot proper into SDRAM so that
>>>> it can be started.
>>>>
>>>> This is needed on some platforms, e.g. MT76x8.
>>>>
>>>> Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>>>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>>> ---
>>>> Changes in v6:
>>>> - New patch
>>>>
>>>>  ? common/spl/spl_legacy.c | 4 ++++
>>>>  ? 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>>>> index 2cd2a74a4c..e320206098 100644
>>>> --- a/common/spl/spl_legacy.c
>>>> +++ b/common/spl/spl_legacy.c
>>>> @@ -4,6 +4,7 @@
>>>>  ?? */
>>>>  ? ? #include <common.h>
>>>> +#include <cpu_func.h>
>>>>  ? #include <malloc.h>
>>>>  ? #include <spl.h>
>>>>  ? @@ -108,5 +109,8 @@ int spl_load_legacy_img(struct spl_image_info
>>>> *spl_image,
>>>>  ????????? return -EINVAL;
>>>>  ????? }
>>>>  ? +??? /* Flush cache of loaded U-Boot image */
>>>> +??? flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
>>>> +
>>>
>>> I failed to find the mail, but haven't we discussed moving this cache
>>> flush to your arch before starting a binary?
>>
>> I don't remember such an agreement. But I don't object in general.
>>
>>> I cannot see this being required or implemented for non-legacy images,
>>> and it still seems wrong here.
>>
>> Its pretty common when an OS image is loaded and booted, that the cache
>> is flushed before running code from it.
>>
>> But I can rework this to add some empty weak function (I don't see an
>> easy better way) to do this platform specific image handling before its
>> booted. Or do you have a better idea on how to handle this?
>>
>> Thanks,
>> Stefan
> 
> actually all MIPS platforms with non-coherent cache need that flush
> before jumping to another U-Boot or OS. Thus currently random crashes
> can occur on all MIPS boards with generic SPL not just mtmips.
> 
> But jump_to_image_no_args() in spl.c is already declared as weak, so we
> should implement that unconditionally for MIPS similar to your patch for
> do_go_exec(). It would be great if you could prepare a patch to replace
> this one, thanks.

Sure, good idea. I'll drop this patch and will add the cache flush to
the newly created MIPS specfic version of jump_to_image_no_args().

Thanks,
Stefan
diff mbox series

Patch

diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
index 2cd2a74a4c..e320206098 100644
--- a/common/spl/spl_legacy.c
+++ b/common/spl/spl_legacy.c
@@ -4,6 +4,7 @@ 
  */
 
 #include <common.h>
+#include <cpu_func.h>
 #include <malloc.h>
 #include <spl.h>
 
@@ -108,5 +109,8 @@  int spl_load_legacy_img(struct spl_image_info *spl_image,
 		return -EINVAL;
 	}
 
+	/* Flush cache of loaded U-Boot image */
+	flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
+
 	return 0;
 }