[PATCH] sunxi: restore modified memory

[PATCH] sunxi: restore modified memory

[Andrey Skvortsov]

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.

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;
+	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)
 {
+	unsigned long tmp_base;
+	unsigned long tmp_offset;
+	bool ret;
+
+	/* Save original values */
+	tmp_base = readl(base);
+	tmp_offset = readl((ulong)base + offset);
+
 	/* Try to write different values to RAM at two addresses */
 	writel(0, base);
 	writel(0xaa55aa55, base + offset);
 	dsb();
 	/* Check if the same value is actually observed when reading back */
-	return readl(base) ==
-	       readl(base + offset);
+	ret = readl(base) == readl(base + offset);
+
+	/* Restore original values */
+	writel(tmp_base, base);
+	writel(tmp_offset, (ulong)base + offset);
+	return ret;
 }
 
 static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank)
-- 
2.39.2

Re: [PATCH] sunxi: restore modified memory

[Andre Przywara]

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.

> +	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.

Cheers,
Andre


>  {
> +	unsigned long tmp_base;
> +	unsigned long tmp_offset;
> +	bool ret;
> +
> +	/* Save original values */
> +	tmp_base = readl(base);
> +	tmp_offset = readl((ulong)base + offset);
> +
>  	/* Try to write different values to RAM at two addresses */
>  	writel(0, base);
>  	writel(0xaa55aa55, base + offset);
>  	dsb();
>  	/* Check if the same value is actually observed when reading back */
> -	return readl(base) ==
> -	       readl(base + offset);
> +	ret = readl(base) == readl(base + offset);
> +
> +	/* Restore original values */
> +	writel(tmp_base, base);
> +	writel(tmp_offset, (ulong)base + offset);
> +	return ret;
>  }
>  
>  static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank)

Re: [PATCH] sunxi: restore modified memory

[Andrey Skvortsov]

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