Dne ponedeljek, 02. oktober 2023 ob 13:26:26 CEST je Andre Przywara napisal(a):
On Sun, 1 Oct 2023 21:43:32 +0530 Gunjan Gupta viraniac@gmail.com wrote:
(fixing Jernej's email)
Hi Gunjan,
thanks for sending a patch!
On some H6 boards like Orange Pi 3 LTS, some times U-Boot fails to detect ram size correctly. Instead of 2GB thats available, it detects 4GB of ram and then SPL just hangs there making board not to boot further.
On debugging, I found that the rows value were being determined correctly, but columns were sometimes off by one value. I found that adding some delay after the mctl_core_init call along with making use of dsb in the start of the mctl_mem_matches solves the issue.
Signed-off-by: Gunjan Gupta viraniac@gmail.com
arch/arm/mach-sunxi/dram_helpers.c | 1 + arch/arm/mach-sunxi/dram_sun50i_h6.c | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index cdf2750f1c..5758c58e07 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -32,6 +32,7 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) #ifndef CONFIG_MACH_SUNIV bool mctl_mem_matches(u32 offset) {
- dsb();
This looks a bit odd, do you have an explanation for that? And are you sure that is really needed? I understand why we need the DSB after the writel's below, but before that? The only thing I could think of is that we are missing a barrier in mctl_core_init() - which is the function called before mctl_mem_matches(). Can you move that dsb(); into mctl_auto_detect_dram_size(), right after the mctl_core_init() call (where you add the udelay() below)? And I wonder if a dmb() would already be sufficient? I noticed recently that the clr/setbit_le32() functions don't have a barrier at all, maybe that should be fixed instead?
Looking at original BSP DRAM code, there is no data barriers that I can find. Cache shouldn't be a thing before DRAM is initialized, right? Conversely, I suggest adding memory barriers before each udelay(), as it is there for a reason.
/* Try to write different values to RAM at two addresses */ writel(0, CFG_SYS_SDRAM_BASE); writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c b/arch/arm/mach-sunxi/dram_sun50i_h6.c index bff2e42513..a031a845f5 100644 --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c @@ -623,6 +623,8 @@ static void mctl_auto_detect_dram_size(struct dram_para *para) para->cols = 11; mctl_core_init(para);
- udelay(50);
The location of that udelay() looks a bit odd, any chance that belongs at the end of mctl_channel_init() instead? And how did you come up with that and the value of 50? Just pure experimentation? I think the original BSP DRAM code has plenty of delay calls, so we might have missed this one here, but I would love to see some explanation here.
I checked original driver and we have *almost* all delays. There are two missing. Both in mctl_phy_pir_init(). One before mctl_await_completion() and another after it. Both are 1 us long.
Maybe that solves it (in combination with data barriers)?
Best regards, Jernej
Cheers, Andre
for (para->cols = 8; para->cols < 11; para->cols++) { /* 8 bits per byte and 16/32 bit width */ if (mctl_mem_matches(1 << (para->cols + 1 +