Re: [PATCH v1] common/memsize.c: Fix get_ram_size() original data restore

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5351 bytes --]

On Fri, Feb 27, 2026 at 11:39:44AM +0100, Emanuele Ghidoli wrote:
> 
> 
> On 2/27/26 11:13, Francis, Neha wrote:
> > 
> > 
> > On 2/26/2026 10:01 PM, Tom Rini wrote:
> >> On Thu, Feb 26, 2026 at 05:30:06PM +0100, Stefan Eichenberger wrote:
> >>> Hi Francesco and Tom,
> >>>
> >>> On Thu, Feb 26, 2026 at 05:11:49PM +0100, Francesco Dolcini wrote:
> >>>> +Emanuele
> >>>>
> >>>> Hello Tom,
> >>>>
> >>>> On Thu, Feb 26, 2026 at 08:23:45AM -0600, Tom Rini wrote:
> >>>>> On Thu, Feb 26, 2026 at 08:05:02AM +0100, Francesco Dolcini wrote:
> >>>>>> Hello Tom,
> >>>>>>
> >>>>>> On Fri, Mar 14, 2025 at 11:06:49AM +0100, Stefan Eichenberger wrote:
> >>>>>>> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >>>>>>>
> >>>>>>> The get_ram_size() function fails to restore the original RAM data when
> >>>>>>> the data cache is enabled. This issue was observed on an AM625 R5 SPL
> >>>>>>> with 512MB of RAM and is a regression that became visible with
> >>>>>>> commit bc07851897bd ("board: ti: Pull redundant DDR functions to a common
> >>>>>>> location and Fixup DDR size when ECC is enabled").
> >>>>>>>
> >>>>>>> Observed boot failure messages:
> >>>>>>>   Warning: Did not detect image signing certificate. Skipping authentication to prevent boot failure. This will fail on Security Enforcing(HS-SE) devices
> >>>>>>>   Authentication passed
> >>>>>>>   Starting ATF on ARM64 core...
> >>>>>>>
> >>>>>>> The system then hangs. This indicates that without a data cache flush,
> >>>>>>> data in the cache is not coherent with RAM, preventing the system from
> >>>>>>> booting. This was verified by printing the content of this address when
> >>>>>>> the issue occurs.
> >>>>>>>
> >>>>>>> Add a data cache flush after each restore operation to resolve this
> >>>>>>> issue.
> >>>>>>>
> >>>>>>> Fixes: bc07851897bd ("board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled")
> >>>>>>> Fixes: 1c64b98c1ec4 ("common/memsize.c: Fix get_ram_size() when cache is enabled")
> >>>>>>> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >>>>>>
> >>>>>> Tom, can we merge this?
> >>>>>> This is the last bit to solve the regression reported here,
> >>>>>> https://lore.kernel.org/all/20260224152405.GD340942@francesco-nb/
> >>>>>
> >>>>> I wasn't happy with this at the time, and Stefan's last email in the
> >>>>> thread left me with the impression more investigation was needed and
> >>>>> likely something else was the root cause.
> >>>>
> >>>> I believe that this patch is needed.
> >>>>
> >>>> On AM62 what is happening is the following.
> >>>>
> >>>> We have a cortex-R5 that is the first core booting (there is also a
> >>>> cortex-m4, but it's not relevant for this discussion).
> >>>>
> >>>> It runs from internal memory and it configures the DDR ram
> >>>>
> >>>> We load to DDR memory various pieces of firmware (TFA, U-Boot for the
> >>>> cortex A53, ...)
> >>>>
> >>>> We do execute get_ram_size(), that read/write the memory, and it is
> >>>> supposed to restore it back the original content
> >>>>
> >>>> However when we have the cache enabled, we might miss to write back the
> >>>> original memory content, where the other pieces of firmware are.
> >>>>
> >>>> And after that we start the cortex A53, running in DDR, and there the
> >>>> memory content might not be correct, because there is no cache coherency
> >>>> between the cortex-A and the cortex-R. And because of that we have
> >>>> crashes.
> >>>>
> >>>> Stefan: any comment here? Can you help?
> >>>
> >>> I think what you wrote summarises the issue well. If I recall correctly,
> >>> I "fixed" the issue last time by simply calling get_ram_size() once
> >>> before enabling the cache. This was in commit 4164289db882e. The SPL
> >>> then informs U-Boot of the memory size via fdt fixup. However, something
> >>> has probably changed now (possibly in the R5 SPL), meaning the cache is
> >>> enabled earlier, so the cache is enabled again when get_ram_size() is
> >>> called.
> >>>
> >>> For the AMP use case, either "get_ram_size" should not be called once
> >>> the cache is enabled, or a similar patch to the one I proposed is
> >>> required.
> >>
> >> I would lean towards the former if at all possible.
> >>
> > 
> > Just trying to understand, what is the reasoning behind ensuring get_ram_size is
> > not called if cache is not enabled? Wasn't get_ram_size written with the
> > possibility of cache being enabled (existence of dcache_en logic); then this
> > patch is a valid fix right?
> > 
> > In parallel, I do agree we need to have a code analysis w.r.t dram_init, we are
> > making certain cache and dram calls spuriously making this confusing.
> > 
> 
> Hello Tom,
> I agree with Francis.
> 
> When I proposed commit 1c64b98c1ec4 ("common/memsize.c: Fix get_ram_size()
> when cache is enabled"), I was not considering the presence of other actors
> (other cores, DMA engines, etc.).
> 
> That patch fixes what I had overlooked at the time. We need to restore the
> actual RAM contents, not only what is perceived by the core executing
> get_ram_size().
> 
> To me this patch sounds intrinsically correct.

Alright. Can I please get some Reviewed / Tested by tags? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]