Dne ponedeljek, 02. oktober 2023 ob 14:42:40 CEST je Gunjan Gupta napisal(a):
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?
I started with Ondrej Jirman's patch from LibreELEC's tree that had a dsb call added after the first writel call. That reduced the frequency of the errors but didn't removed it completely. My reason for moving it before the writel was to make sure any memory access is completed before starting the actual logic for the test. That reduced the frequency even further, but didn't resolve the issue. I did try removing it leaving only udelay added to the code, but that brings back the issue.
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?
Sure, I will try experimenting with it.
I noticed recently that the clr/setbit_le32() functions don't have a barrier at all, maybe that should be fixed instead?
I haven't done much of the low level programming myself. Mostly have done user space programming along with fixing minor kernel module compilation issues due to kernel api changes. So I wasn't sure what all places to debug. But if you point me to places with things to try, I surely can give time playing around testing the proposed fixes.
@@ -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?
Before adding the udelay, I added 7 print statements to print all the members of the para struct. That itself solved the issue along with the dsb added to the top of the mctl_mem_matches function. This is what gave me the clue that a delay is needed there. The value of 50 is indeed from pure experimentation
Oh, I found one major difference between BSP and mainline driver. Please test patch attached below. I don't know if this path is always taken when wrong configuration is tested or not.
Best regards, Jernej
--- a/arch/arm/mach-sunxi/dram_sun50i_h6.c +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c @@ -420,6 +420,7 @@ static bool mctl_channel_init(struct dram_para *para) (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE; struct sunxi_mctl_phy_reg * const mctl_phy = (struct sunxi_mctl_phy_reg *)SUNXI_DRAM_PHY0_BASE; + bool ret = true; int i; u32 val;
@@ -537,7 +538,7 @@ static bool mctl_channel_init(struct dram_para *para) debug("DRAM PHY DX%dRSR0 = %x\n", i, readl(&mctl_phy->dx[i].rsr[0])); debug("Error while initializing DRAM PHY!\n");
- return false; + ret = false; }
if (sunxi_dram_is_lpddr(para->type)) @@ -553,7 +554,7 @@ static bool mctl_channel_init(struct dram_para *para) writel(0x7ff, &mctl_com->maer1); writel(0xffff, &mctl_com->maer2);
- return true; + return ret; }
static void mctl_auto_detect_rank_width(struct dram_para *para)