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.