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

[Simon Goldschmidt]

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
>