Re: [PATCH] sunxi: restore modified memory

[Andrey Skvortsov <andrej.skvortzov@gmail.com>]

Hi Andre,

On 23-11-01 09:50, Andre Przywara wrote:
> On Fri, 21 Jul 2023 17:57:21 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> 
> Hi Andrey,
> 
> sorry for the late reply!
> 
> > On A64 with 2G of RAM words at following addresses were modified with
> > 'aa55aa55' value:
> >  - 50000000
> >  - 60000000
> >  - 84000000
> >  - 88000000
> >  - 90000000
> >  - A0000000
> >  - A0000200
> > 
> > That made harder to pick memory range for persistent storage in RAM.
> 
> In general I now think this patch is fine, since DRAM content can indeed
> be preserved across some reboot types.
> Some things below:
> 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  arch/arm/mach-sunxi/dram_helpers.c  | 16 ++++++++++++++--
> >  arch/arm/mach-sunxi/dram_sunxi_dw.c | 16 ++++++++++++++--
> >  2 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c
> > index 2c873192e6..e3d236a4b3 100644
> > --- a/arch/arm/mach-sunxi/dram_helpers.c
> > +++ b/arch/arm/mach-sunxi/dram_helpers.c
> > @@ -32,12 +32,24 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val)
> >  #ifndef CONFIG_MACH_SUNIV
> >  bool mctl_mem_matches(u32 offset)
> >  {
> > +	unsigned long tmp_base;
> > +	unsigned long tmp_offset;
> 
> The type doesn't match the return type of readl.
> Please use uint32_t here.
> And the names are somewhat misleading, at least to my mind, since they are
> not a base address and an offset, but rather just values at a base
> address and at an offset from that.
> So I think val_base and val_offset would be better.

Thanks for the review.

> > +	bool ret;
> > +
> > +        /* Save original values */
> > +	tmp_base = readl(CFG_SYS_SDRAM_BASE);
> > +	tmp_offset = readl((ulong)CFG_SYS_SDRAM_BASE + offset);
> > +
> >  	/* Try to write different values to RAM at two addresses */
> >  	writel(0, CFG_SYS_SDRAM_BASE);
> >  	writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset);
> >  	dsb();
> >  	/* Check if the same value is actually observed when reading back */
> > -	return readl(CFG_SYS_SDRAM_BASE) ==
> > -	       readl((ulong)CFG_SYS_SDRAM_BASE + offset);
> > +	ret = readl(CFG_SYS_SDRAM_BASE) == readl((ulong)CFG_SYS_SDRAM_BASE + offset);
> > +
> > +	/* Restore original values */
> > +	writel(tmp_base, CFG_SYS_SDRAM_BASE);
> > +	writel(tmp_offset, (ulong)CFG_SYS_SDRAM_BASE + offset);
> > +	return ret;
> >  }
> >  #endif
> > diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c
> > index 9107b114df..6245815fa2 100644
> > --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c
> > +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c
> > @@ -657,13 +657,25 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para)
> >   */
> >  static bool mctl_mem_matches_base(u32 offset, ulong base)
> 
> Mmh, somehow I dimly remember commenting on this before (some other
> patch?), but this function is essentially the same as above, isn't it?
> If someone has some spare cycles, they could be merged, implementing
> mctl_mem_matches() as a tiny wrapper around mctl_mem_matches_base(), to
> preserve all callers.
> 
> Shouldn't block this patch, though.
Good point. I'll do this as part of v2 as a separate patch.

-- 
Best regards,
Andrey Skvortsov