Re: [PATCH 1/1] sunxi: dram: Fix incorrect ram size detection for some H6 boards

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

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)