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)
