From: Siarhei Siamashka <siarhei.siamashka@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [linux-sunxi] Re: [PATCH 05/14] sunxi: dram: Code cleanup for the impedance calibration
Date: Fri, 25 Jul 2014 06:44:41 +0300 [thread overview]
Message-ID: <20140725064441.1b685c6c@i7> (raw)
In-Reply-To: <1405970420.4100.11.camel@hastur.hellion.org.uk>
On Mon, 21 Jul 2014 20:20:20 +0100
Ian Campbell <ijc@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
next prev parent reply other threads:[~2014-07-25 3:44 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-18 16:22 [U-Boot] [PATCH 00/14] sunxi: Allwinner A10/A13/A20 DRAM controller fixes Siarhei Siamashka
2014-07-18 16:22 ` [U-Boot] [PATCH 01/14] sunxi: dram: Remove useless 'dramc_scan_dll_para()' function Siarhei Siamashka
2014-07-21 18:42 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 02/14] sunxi: dram: Remove broken super-standby remnants Siarhei Siamashka
2014-07-21 18:45 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 03/14] sunxi: dram: Respect the DDR3 reset timing requirements Siarhei Siamashka
2014-07-21 18:46 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 04/14] sunxi: dram: Code cleanup and comments for the CKE delay handling Siarhei Siamashka
2014-07-21 18:51 ` Ian Campbell
2014-07-25 1:41 ` Siarhei Siamashka
2014-07-25 7:27 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 05/14] sunxi: dram: Code cleanup for the impedance calibration Siarhei Siamashka
2014-07-21 19:20 ` Ian Campbell
2014-07-25 3:44 ` Siarhei Siamashka [this message]
2014-07-25 7:30 ` [U-Boot] [linux-sunxi] " Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 06/14] sunxi: dram: Configurable MBUS clock speed (use PLL5 or PLL6) Siarhei Siamashka
2014-07-21 19:31 ` Ian Campbell
2014-07-25 4:00 ` Siarhei Siamashka
2014-07-25 7:31 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 07/14] sunxi: dram: Use divisor P=1 for PLL5 Siarhei Siamashka
2014-07-21 19:35 ` Ian Campbell
2014-07-18 16:22 ` [U-Boot] [PATCH 08/14] sunxi: dram: Improve DQS gate data training error handling Siarhei Siamashka
2014-07-21 19:36 ` Ian Campbell
2014-07-18 16:23 ` [U-Boot] [PATCH 09/14] sunxi: dram: Add a helper function 'mctl_get_number_of_lanes' Siarhei Siamashka
2014-07-21 19:41 ` Ian Campbell
2014-07-25 4:26 ` Siarhei Siamashka
2014-07-25 7:33 ` Ian Campbell
2014-07-18 16:23 ` [U-Boot] [PATCH 10/14] sunxi: dram: Configurable DQS gating window mode and delay Siarhei Siamashka
2014-07-18 16:23 ` [U-Boot] [PATCH 11/14] sunxi: dram: Support sun4i (Allwinner A10) and sun5i (Allwinner A13) Siarhei Siamashka
2014-07-21 19:49 ` Ian Campbell
2014-07-18 16:23 ` [U-Boot] [PATCH 12/14] sunxi: dram: Drop DDR2 support and assume only single rank DDR3 memory Siarhei Siamashka
2014-07-21 19:51 ` Ian Campbell
2014-07-25 4:36 ` Siarhei Siamashka
2014-07-18 16:23 ` [U-Boot] [PATCH 13/14] sunxi: dram: Derive write recovery delay from DRAM clock speed Siarhei Siamashka
2014-07-21 19:52 ` Ian Campbell
2014-07-18 16:23 ` [U-Boot] [PATCH 14/14] sunxi: dram: Autodetect DDR3 bus width and density Siarhei Siamashka
2014-07-21 19:54 ` Ian Campbell
2014-07-19 10:59 ` [U-Boot] [PATCH 00/14] sunxi: Allwinner A10/A13/A20 DRAM controller fixes Hans de Goede
2014-07-21 19:58 ` Ian Campbell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140725064441.1b685c6c@i7 \
--to=siarhei.siamashka@gmail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox