[U-Boot] [PATCH v3 16/26] sunxi: H3: add DRAM controller single bit delay support

[Maxime Ripard]

On Mon, Dec 19, 2016 at 10:53:32AM +0000, Andre Przywara wrote:
> Hi,
> 
> On 19/12/16 09:57, Maxime Ripard wrote:
> > On Mon, Dec 19, 2016 at 01:50:06AM +0000, Andre Przywara wrote:
> >> From: Jens Kuske <jenskuske at gmail.com>
> >>
> >> So far the DRAM driver for the H3 SoC (and apparently boot0/libdram as
> >> well) only applied coarse delay line settings, with one delay value for
> >> all the data lines in each byte lane and one value for the control lines.
> >>
> >> Instead of setting the delays for whole bytes only allow setting it for
> >> each individual bit. Also add support for address/command lane delays.
> >>
> >> For the purpose of this patch the rules for the existing coarse settings
> >> were just applied to the new scheme, so the actual register writes don't
> >> change for the H3. Other SoCs will utilize this feature later properly.
> >>
> >> With a stock GCC 5.3.0 this increases the dram_sun8i_h3.o code size from
> >> 2296 to 2344 Bytes.
> >>
> >> [Andre: move delay parameters into macros to ease later sharing, use
> >> 	defines for numbers of delay registers, extend commit message]
> >>
> >> Signed-off-by: Jens Kuske <jenskuske at gmail.com>
> >> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > 
> > I said it earlier, but some comments on these new fields would really
> > be welcome to document the structure and what values they're supposed
> > to hold.
> 
> I guess you know as much as I do on this topic.

I'd say that you know much more than I do on this one ;)

> Apparently there are delays to compensate for differing trace lengths on
> the PCB, one for every bit line. On the 32-bit DRAM controller these are
> grouped in four groups of 8 bits each (hence byte lane).
> 
> Please keep in mind that there is no easily available documentation,
> some parts are just copying what boot0/libdram does.
> As we don't know the exact meaning of these fields, I prefer to not add
> any guesses here.
> I was hoping that the defines I added in this version would shed some
> light on this? I am pretty sure of their meaning, but that's as far as
> it goes. For instance I have no idea what the units are, for cycles they
> are quite big.
> 
> So frankly I don't really know what to add here still.

Ok.

> I can elaborate on how to get these actual values from an existing
> boot0/libdram, but that would be more of a documentation patch than
> actual code.

That would still be very valuable, if it makes sense, you always can
put that as a separate patch.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161220/b497d8e7/attachment.sig>