[U-Boot] [linux-sunxi] Re: [PATCH 05/14] sunxi: dram: Code cleanup for the impedance calibration

[Siarhei Siamashka]

On Mon, 21 Jul 2014 20:20:20 +0100
Ian Campbell <ijc at hellion.org.uk> wrote:

> On Fri, 2014-07-18 at 19:22 +0300, Siarhei Siamashka wrote:
> > Moved the impedance setup code part into a separate function. Added explicit
> > wait for ZQ calibration completion before proceeding to the next initialization
> > steps. Removed the CONFIG_SUN7I ifdef guard around the code, which has identical
> > behaviour on sun4i/sun5i/sun7i. And if 'odt_en' is set in the 'dram_para' struct,
> > then ODT now actually gets enabled in the DRAM_IOCR register (which the older
> > code failed to do and was always running without ODT no matter the settings).
> 
> There's a few aspects of this code which don't seem to be explained
> here.
> 
> Firstly if odt_en is not enabled we now skip setting the impedance.
> Which seems logical but should me mentioned.

Right. The commit message needs to be rewritten to address the
fact that we are introducing the new ZQ calibration code and
just removing the old one.

> It's also worth noting that none of the platforms in u-boot-sunxi.git#master
> set odt_en

None of the sunxi devices are using it for anything meaningful (even
though some of them attempt to set odt_en in the non-mainline sunxi
u-boot, they are actually very lucky if they don't suffer from adverse
effects doing this).

The demonstration of the usefulness of this patch is only presented in
the 'highspeedtruck' branches, referenced from the cover letter. We
know that this code works, because we can get a major DRAM clock speed
uplift. Which is simply impossible without doing impedance configuration
correctly.

> Secondly the weird sun7i magic has changed from
> -       setbits_le32(&dram->zqcr1, (0x1 << 24) | (0x1 << 1));
> -       if (para->tpr4 & 0x2)
> -               clrsetbits_le32(&dram->zqcr1, (0x1 << 24), (0x1 << 1));

If you want to have some extra fun, then you can compare this fragment
with the original code from boot0:

    //SDR_ZQCR1 set bit24 to 1
    reg_val = mctl_read_w(SDR_ZQCR1);
    reg_val |= (0x1<<24) | (0x1<<1);
    if(para->dram_tpr4 & 0x2)
    {
        reg_val &= ~((0x1<<24) | (0x1<<1));
    }
    mctl_write_w(SDR_ZQCR1, reg_val);

As you can see, the original boot0 logic already got mangled somewhat.
But since we have no boards, which would pass the (para->dram_tpr4 &
0x2) check, it is not really important in practice.

> into just a write of the raw value. This should be mentioned. Also this
> now occurs after the call to dramc_clock_output_en().

The old code is basically a non-working gibberish. And it is not very
useful as a reference.

We had to first figure out how the hardware works (taking advantage
of the fact that Rockchip and TI Keystone2 DRAM controllers are rather
similar and we can get some hints from them):
    http://linux-sunxi.org/A10_DRAM_Controller_Register_Guide#SDR_ZQCR0
And then reimplement the ZQ calibration code using this information.

Only the ZPROG and ZDATA bits placement is kept the same in the 'zq'
parameter for "backwards compatibility" reasons. But there is no
real backwards compatibility, because it is difficult to be exactly
compatible with something that is broken.

> Thirdly why do we not wait for ZQ calibration on sun7i?

On sun4i and sun5i hardware, we see that the ZQ calibration has been
already initiated by something and is in progress. So it is reasonable
to wait until it finishes doing its things and only then start the real
ZQ calibration with our settings.

On sun7i hardware, there are no signs of this auto-initiated
calibration. So if we add a wait, it will only result in a timeout
panic.

> Lastly it now seems to support calibration using an external resistor.
> And neither half of that new if (zdata) seems to correspond to the old
> code.

That's right.
 
> I think part of the problem here is that this patch is trying to do too
> much in one go.

The patch is rather small in terms of LOC and added functionality. The
ZDATA handling can be split into a separate patch though.

> If separating things out isn't possible (e.g. because these changes are
> all interdependent) then it is important that the commit message describes
> them.

If this would make you more happy, it is also possible to just remove
all the old impedance code in one commit (since nothing is really using
it yet). And then introduce the new impedance code in another commit.

Luckily, in the mainline u-boot we still don't have any copy-pasted
board configs, which happen to pretend to be using the 'odt_en'
parameter in the 'dram_para' struct.

> I'd also steer clear of describing this as a code cleanup when it
> also has functional changes.

The 'cleanup' was just a bad choice of word. It is a reimplementation.

> > + * Wait up to 1s for mask to be set in given reg.
> > + */
> > +static void await_bits_set(u32 *reg, u32 mask)
> 
> This could be combined with the existing await_completion into a
> function which takes a mask and a val. Perhaps with convenience wrappers
> for the two cases.

That's a good idea.

-- 
Best regards,
Siarhei Siamashka