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.