[U-Boot] [PATCH v2] sunxi: Make dram odt-en configurable through Kconfig for A33 based boards

[U-Boot] [PATCH v2] sunxi: Make dram odt-en configurable through Kconfig for A33 based boards

[Hans de Goede]

Some A33 based boards use odt, while others do not, so make odt_en
configurable for sun8i too by moving the existing Kconfig option for it out
of the #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block it was in.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use existing DRAM_ODT_EN Kconfig block moving it out of the
 #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block it was in
---
 arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c |  2 +-
 arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c |  3 +--
 board/sunxi/Kconfig                       | 15 ++++++++-------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
index 3d7964d..9cb42d5 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
@@ -31,7 +31,7 @@ static const struct dram_para dram_para = {
 	.clock = CONFIG_DRAM_CLK,
 	.type = 3,
 	.zq = CONFIG_DRAM_ZQ,
-	.odt_en = 1,
+	.odt_en = CONFIG_DRAM_ODT_EN,
 	.para1 = 0, /* not used (only used when tpr13 bit 31 is set */
 	.para2 = 0, /* not used (only used when tpr13 bit 31 is set */
 	.mr0 = 6736,
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
index 979bb3c..a7a0a2e 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
@@ -19,7 +19,6 @@
 #define DRAM_CLK_MUL 2
 #define DRAM_CLK_DIV 4
 #define DRAM_SIGMA_DELTA_ENABLE 1
-#define DRAM_ODT_EN 0
 
 struct dram_para {
 	u8 cs1;
@@ -215,7 +214,7 @@ static int mctl_channel_init(struct dram_para *para)
 	clrbits_le32(&mctl_ctl->pgcr0, 0x3f << 0);
 
 	/* Set ODT */
-	if ((CONFIG_DRAM_CLK > 400) && DRAM_ODT_EN) {
+	if ((CONFIG_DRAM_CLK > 400) && CONFIG_DRAM_ODT_EN) {
 		setbits_le32(DXnGCR0(0), 0x3 << 9);
 		setbits_le32(DXnGCR0(1), 0x3 << 9);
 	} else {
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 940b6c7..e0c832b 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -95,6 +95,14 @@ config DRAM_ZQ
 	---help---
 	Set the dram zq value.
 
+config DRAM_ODT_EN
+	int "sunxi dram odt_en value"
+	default 0 if !MACH_SUN8I_A23
+	default 1 if MACH_SUN8I_A23
+	---help---
+	Set the dram controller odt_en parameter. This can be used to
+	enable/disable the ODT feature.
+
 if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
 config DRAM_EMR1
 	int "sunxi dram emr1 value"
@@ -103,13 +111,6 @@ config DRAM_EMR1
 	---help---
 	Set the dram controller emr1 value.
 
-config DRAM_ODT_EN
-	int "sunxi dram odt_en value"
-	default 0
-	---help---
-	Set the dram controller odt_en parameter. This can be used to
-	enable/disable the ODT feature.
-
 config DRAM_TPR3
 	hex "sunxi dram tpr3 value"
 	default 0
-- 
2.4.0

[U-Boot] [PATCH v2] sunxi: Make dram odt-en configurable through Kconfig for A33 based boards

[Ian Campbell]

On Fri, 2015-05-15 at 20:43 +0200, Hans de Goede wrote:
> Some A33 based boards use odt, while others do not, so make odt_en
> configurable for sun8i too by moving the existing Kconfig option for it out
> of the #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block it was in.

I'm still not mad-keen on overloading an int Kconfig with boolean
semantics. _Especially_ if it has different semantics on different
generations of DRAM controller, that's even worse than before IMHO.

I think we should either:

Have two Kconfig options an int on SUN[457]I and a bool on SUN8I.
Whether or not they have the same name I don't know, consider adding
GENx to it perhaps?

-or-

Have a single option which is an int which has similar semantics on
both. This could be ODT_EN[0] == enable, and:
        ODT_EN[31:1]==reserved on SUN8I
but
        ODT_EN[7:1]==reserved,[15:8]==correction,[31:16]==reserved on
        the others.
The main upshot here, apart from improved help text for the option,
would be adding &0x1 to the usage in SUN8I.

Is there any reason not to push odt_en through dram_para like on other
subarches? Or conversely any reason for those others not to use the
Kconfig directly?

The second option sounds like a simpler change from where the code is
today, but perhaps we prefer not to invent semantics in this way. FWIW
if you were to prefer the first option above then going via dram_param
would make the use of IS_ENABLED (to assign to a boolean field in the
param struct) much less ugly than with the existing structure of the
code.

> +	Set the dram controller odt_en parameter. This can be used to
> +	enable/disable the ODT feature.

This isn't strictly correct/true on SUN8I since we don't set odt_en in
the params.

Ian.

[U-Boot] [PATCH v2] sunxi: Make dram odt-en configurable through Kconfig for A33 based boards

[Siarhei Siamashka]

On Sun, 17 May 2015 11:25:42 +0100
Ian Campbell <ijc+uboot@hellion.org.uk> wrote:

> On Fri, 2015-05-15 at 20:43 +0200, Hans de Goede wrote:
> > Some A33 based boards use odt, while others do not, so make odt_en
> > configurable for sun8i too by moving the existing Kconfig option for it out
> > of the #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block it was in.
> 
> I'm still not mad-keen on overloading an int Kconfig with boolean
> semantics. _Especially_ if it has different semantics on different
> generations of DRAM controller, that's even worse than before IMHO.

In fact, it appears to have exactly the same semantics on different
generations of the DRAM controller (if only hardware is concerned):

In the case of sun4i/sun5i/sun7i, this configuration variable encodes
two bits (values from 0 to 3). These bits are responsible for
separately enabling ODT for DQ and DQS lines. Please have a look at
the U-Boot code:
    http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-sunxi/dram_sun4i.h;h=40c385a5bc8025e5#l167
And also at the documentation (bits IOCR_DQS_RTT / IOCR_DQ_RTT /
IOCR_DQS_ODT / IOCR_DQ_ODT):
    http://linux-sunxi.org/A10_DRAM_Controller_Register_Guide#SDR_IOCR

Likewise, the U-Boot code for A33:
    http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c;h=d03f00dc4bc60a8b#l218
And the documentation for bits 9 (DQSRTT), 10 (DQRTT), 1 (DQSODT)
and 2 (DQODT) of the DXnGCR register:
    http://www.ti.com/lit/ug/spruhn7c/spruhn7c.pdf

As we can see, the A33 treats the 'odt_en' flag as boolean and
interprets it as setting the same value for both DQ and DQS lines
(either both are set or both are clear). Now an interesting thing
is that the A33 DRAM init code only configures the DQSRTT and DQRTT
bits, but ignores the DQSODT and DQODT! This in fact might mean that
the ODT support is non-functional for A33. It is not a totally
unlikely scenario, considering that the ODT feature used to be
also broken in the sun4i/sun5i/sun7i DRAM code before:
    http://git.denx.de/?p=u-boot.git;a=commitdiff;h=7e40e1926a237ad2
Another possible explanation could be that the A33 variant of the DRAM
controller simply makes two of these four configuration bits redundant.
These possible explanations can be confirmed or debunked by running
reliability tests with different DRAM configurations and checking if
changing these settings has any effect.

Either way, there are definitely major similarities in the ODT
configuration between all these DRAM controller variants.

As a side note, this is how the actual work should be done. It takes a
bit of time and effort to implement the DRAM controller support
properly in order to ensure device reliability and good performance.
And it does not make me happy to see the Allwinner's boo0 code just
getting mindlessly transplanted into U-Boot without even trying to
understand what it does, introducing some changes mostly just for the
sake of adjusting the coding style but doing exactly nothing to make it
reviewable (the commit messages are barely informative, the existence
of any documentation or the other code for similar hardware is denied,
all the magic constants are preserved, etc.). Furthermore, this nearly
pointless bikeshedding about a mostly cosmetic config option (again,
apparently even without trying to understand how this option affects
the hardware) makes me even less happy :-(

> 
> I think we should either:
> 
> Have two Kconfig options an int on SUN[457]I and a bool on SUN8I.
> Whether or not they have the same name I don't know, consider adding
> GENx to it perhaps?
> 
> -or-
> 
> Have a single option which is an int which has similar semantics on
> both. This could be ODT_EN[0] == enable, and:
>         ODT_EN[31:1]==reserved on SUN8I
> but
>         ODT_EN[7:1]==reserved,[15:8]==correction,[31:16]==reserved on
>         the others.
> The main upshot here, apart from improved help text for the option,
> would be adding &0x1 to the usage in SUN8I.

BTW, it looks like the recent boot0 sources for A20 now treat
the 'odt_en' parameter as boolean in the same way as on A33:
    https://github.com/allwinner-zh/bootloader/blob/a447eef53990f591/basic_loader/bsp/bsp_for_a20/init_dram/dram_init.c#L426

And then the 'trp5' config variable is used to tune the individual bits
(enabling/disabling ODT individually for DQ and DQS lines). And on A13,
the ODT support is now commented out completely:
    https://github.com/allwinner-zh/bootloader/blob/a447eef53990f591/basic_loader/bsp/bsp_for_a13/init_dram/dram_init.c#L334

It just shows how random is the evolution of the Allwinner code :-)

I would not mind if the 'odt_en' option just gets changed to boolean on
sun4i/sun5i/sun7i hardware in U-Boot and unified with A33. I have
only ever tried 0 and 3 values for the 'odt_en' parameter, with pretty
good results so far (there was no need for 1 and 2):

   http://linux-sunxi.org/A10_DRAM_Controller_Calibration#Finding_good_impedance_settings

The separate ODT configuration for DQ and DQS can be probably safely
dropped. This can be always introduced back if necessary.

> Is there any reason not to push odt_en through dram_para like on other
> subarches? Or conversely any reason for those others not to use the
> Kconfig directly?

Having the dram_para structure is convenient, because it keeps all the
DRAM settings in a single place in the SPL binary. It can be then easily
inspected or patched. For example, sometimes hardware vendors offer only
bootable images, but no U-Boot sources and don't respond to e-mails.
It is currently easy to extract the DRAM settings from such binaries
(HYNIX H5TQ2G83CFR vs. SAMSUNG K4B2G0846Q for A13-OLinuXino):

    https://www.olimex.com/wiki/A13-OLinuXino#Linux

The other potential use may involve reading the DRAM settings from
NAND (from the header of the Android boot0 bootloader). Having a
structure in memory, where these settings can be written at runtime,
is better than dealing with compile time constants.
 
> The second option sounds like a simpler change from where the code is
> today, but perhaps we prefer not to invent semantics in this way. FWIW
> if you were to prefer the first option above then going via dram_param
> would make the use of IS_ENABLED (to assign to a boolean field in the
> param struct) much less ugly than with the existing structure of the
> code.
> 
> > +	Set the dram controller odt_en parameter. This can be used to
> > +	enable/disable the ODT feature.
> 
> This isn't strictly correct/true on SUN8I since we don't set odt_en in
> the params.

I think that this comment in the Kconfig actually refers to the
FEX name of this parameter:
    https://github.com/linux-sunxi/sunxi-boards/blob/master/sys_config/a33/ga10h_v1.1.fex#L99

And this makes sense. Because the information from device FEX files is
still widely used for populating the settings in U-Boot configs.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2] sunxi: Make dram odt-en configurable through Kconfig for A33 based boards

[Hans de Goede]

Hi,

On 17-05-15 12:25, Ian Campbell wrote:
> On Fri, 2015-05-15 at 20:43 +0200, Hans de Goede wrote:
>> Some A33 based boards use odt, while others do not, so make odt_en
>> configurable for sun8i too by moving the existing Kconfig option for it out
>> of the #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block it was in.
>
> I'm still not mad-keen on overloading an int Kconfig with boolean
> semantics. _Especially_ if it has different semantics on different
> generations of DRAM controller, that's even worse than before IMHO.
>
> I think we should either:
>
> Have two Kconfig options an int on SUN[457]I and a bool on SUN8I.
> Whether or not they have the same name I don't know, consider adding
> GENx to it perhaps?
>
> -or-
>
> Have a single option which is an int which has similar semantics on
> both. This could be ODT_EN[0] == enable, and:
>          ODT_EN[31:1]==reserved on SUN8I
> but
>          ODT_EN[7:1]==reserved,[15:8]==correction,[31:16]==reserved on
>          the others.
> The main upshot here, apart from improved help text for the option,
> would be adding &0x1 to the usage in SUN8I.
>
> Is there any reason not to push odt_en through dram_para like on other
> subarches? Or conversely any reason for those others not to use the
> Kconfig directly?
>
> The second option sounds like a simpler change from where the code is
> today, but perhaps we prefer not to invent semantics in this way. FWIW
> if you were to prefer the first option above then going via dram_param
> would make the use of IS_ENABLED (to assign to a boolean field in the
> param struct) much less ugly than with the existing structure of the
> code.
>
>> +	Set the dram controller odt_en parameter. This can be used to
>> +	enable/disable the ODT feature.
>
> This isn't strictly correct/true on SUN8I since we don't set odt_en in
> the params.

Ok, I think I've a good solution for this now, which actually makes
it a bool, v3 of this patch is coming up.

Regards,

Hans