Re: [PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
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 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 Signed-off-by: Stefan Roese Cc: Weijie Gao Cc: Simon Goldschmidt --- 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 +#include #include #include @@ -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
Re: [PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
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 >>> >>> 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 >>> Signed-off-by: Stefan Roese >>> Cc: Weijie Gao >>> Cc: Simon Goldschmidt >>> --- >>> 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 >>> +#include >>> #include >>> #include >>> >>> @@ -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 >
Re: [PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
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 >>> >>> 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 >>> Signed-off-by: Stefan Roese >>> Cc: Weijie Gao >>> Cc: Simon Goldschmidt >>> --- >>> 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 >>> +#include >>> #include >>> #include >>> @@ -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. -- - Daniel
Re: [PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
On 09.04.20 09:29, Simon Goldschmidt wrote: Am 08.04.2020 um 10:09 schrieb Stefan Roese: From: Weijie Gao 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 Signed-off-by: Stefan Roese Cc: Weijie Gao Cc: Simon Goldschmidt --- 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 +#include #include #include @@ -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
Re: [PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
Am 08.04.2020 um 10:09 schrieb Stefan Roese: > From: Weijie Gao > > 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 > Signed-off-by: Stefan Roese > Cc: Weijie Gao > Cc: Simon Goldschmidt > --- > 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 > +#include > #include > #include > > @@ -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; > } >
[PATCH 22/26 v6] spl: spl_legacy: Add cache flush after reading U-Boot image
From: Weijie Gao 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 Signed-off-by: Stefan Roese Cc: Weijie Gao Cc: Simon Goldschmidt --- 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 +#include #include #include @@ -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; } -- 2.26.0
