[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Hans de Goede]

Make DRAM_ODT_EN Kconfig setting a bool, add a separate DRAM_ODT_CORRECTION
setting for A23 SoCs and use DRAM_ODT_EN Kconfig everywhere instead of
only in dram_sun4i.c and hardcoding odt_en elsewhere.

Note this commit makes no functional changes for existing boards,
its purpose is to allow changing the odt_en value on future A33 boards.

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
Changes in v3:
-Complete rewrite of the patch
---
 arch/arm/cpu/armv7/sunxi/dram_sun4i.c            |  4 ++--
 arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c        | 19 +++++++++----------
 arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c        |  4 ++--
 arch/arm/include/asm/arch-sunxi/dram_sun4i.h     |  3 +--
 arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h |  1 +
 board/sunxi/Kconfig                              | 22 +++++++++++++++-------
 board/sunxi/dram_sun4i_auto.c                    |  3 ++-
 board/sunxi/dram_sun5i_auto.c                    |  3 ++-
 8 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
index c736fa3..f7b4915 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
@@ -508,7 +508,7 @@ static void mctl_ddr3_initialize(void)
 /*
  * Perform impedance calibration on the DRAM controller side of the wire.
  */
-static void mctl_set_impedance(u32 zq, u32 odt_en)
+static void mctl_set_impedance(u32 zq, bool odt_en)
 {
 	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
 	u32 reg_val;
@@ -556,7 +556,7 @@ static void mctl_set_impedance(u32 zq, u32 odt_en)
 	clrbits_le32(&dram->zqcr0, DRAM_ZQCR0_ZCAL);
 
 	/* Set I/O configure register */
-	writel(DRAM_IOCR_ODT_EN(odt_en), &dram->iocr);
+	writel(DRAM_IOCR_ODT_EN, &dram->iocr);
 }
 
 static unsigned long dramc_init_helper(struct dram_para *para)
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
index 3d7964d..165c052 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a23.c
@@ -26,12 +26,14 @@
 #include <asm/arch/clock.h>
 #include <asm/arch/dram.h>
 #include <asm/arch/prcm.h>
+#include <linux/kconfig.h>
 
 static const struct dram_para dram_para = {
 	.clock = CONFIG_DRAM_CLK,
 	.type = 3,
 	.zq = CONFIG_DRAM_ZQ,
-	.odt_en = 1,
+	.odt_en = IS_ENABLED(CONFIG_DRAM_ODT_EN),
+	.odt_correction = CONFIG_DRAM_ODT_CORRECTION,
 	.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,
@@ -97,7 +99,6 @@ static void mctl_init(u32 *bus_width)
 		(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
 	struct sunxi_mctl_phy_reg * const mctl_phy =
 		(struct sunxi_mctl_phy_reg *)SUNXI_DRAM_PHY0_BASE;
-	int correction;
 
 	if (dram_para.tpr13 & 0x20)
 		writel(0x40b, &mctl_phy->dcr);
@@ -138,7 +139,7 @@ static void mctl_init(u32 *bus_width)
 
 	writel(0x01000081, &mctl_phy->dtcr);
 
-	if (dram_para.clock <= 240 || !(dram_para.odt_en & 0x01)) {
+	if (dram_para.clock <= 240 || !dram_para.odt_en) {
 		clrbits_le32(&mctl_phy->dx0gcr, 0x600);
 		clrbits_le32(&mctl_phy->dx1gcr, 0x600);
 	}
@@ -251,13 +252,11 @@ static void mctl_init(u32 *bus_width)
 	} else
 		*bus_width = 16;
 
-	correction = (dram_para.odt_en >> 8) & 0xff;
-	if (correction) {
-		if (dram_para.odt_en & 0x80000000)
-			correction = -correction;
-
-		mctl_apply_odt_correction(&mctl_phy->dx0lcdlr1, correction);
-		mctl_apply_odt_correction(&mctl_phy->dx1lcdlr1, correction);
+	if (dram_para.odt_correction) {
+		mctl_apply_odt_correction(&mctl_phy->dx0lcdlr1,
+					  dram_para.odt_correction);
+		mctl_apply_odt_correction(&mctl_phy->dx1lcdlr1,
+					  dram_para.odt_correction);
 	}
 
 	mctl_await_completion(&mctl_ctl->statr, 0x01, 0x01);
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
index 979bb3c..ebba438 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
@@ -14,12 +14,12 @@
 #include <asm/arch/clock.h>
 #include <asm/arch/dram.h>
 #include <asm/arch/prcm.h>
+#include <linux/kconfig.h>
 
 /* PLL runs at 2x dram-clk, controller runs at PLL / 4 (dram-clk / 2) */
 #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 +215,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) && IS_ENABLED(CONFIG_DRAM_ODT_EN)) {
 		setbits_le32(DXnGCR0(0), 0x3 << 9);
 		setbits_le32(DXnGCR0(1), 0x3 << 9);
 	} else {
diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun4i.h b/arch/arm/include/asm/arch-sunxi/dram_sun4i.h
index 40c385a..139e336 100644
--- a/arch/arm/include/asm/arch-sunxi/dram_sun4i.h
+++ b/arch/arm/include/asm/arch-sunxi/dram_sun4i.h
@@ -164,8 +164,7 @@ struct dram_para {
 
 #define DRAM_ZQSR_ZDONE (1 << 31) /* ZQ calibration completion flag */
 
-#define DRAM_IOCR_ODT_EN(n) ((((n) & 0x3) << 30) | ((n) & 0x3) << 0)
-#define DRAM_IOCR_ODT_EN_MASK DRAM_IOCR_ODT_EN(0x3)
+#define DRAM_IOCR_ODT_EN ((3 << 30) | (3 << 0))
 
 #define DRAM_MR_BURST_LENGTH(n) (((n) & 0x7) << 0)
 #define DRAM_MR_BURST_LENGTH_MASK DRAM_MR_BURST_LENGTH(0x7)
diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
index 06adee2..316a333 100644
--- a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
+++ b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
@@ -19,6 +19,7 @@ struct dram_para {
 	u32 type;
 	u32 zq;
 	u32 odt_en;
+	s32 odt_correction;
 	u32 para1;
 	u32 para2;
 	u32 mr0;
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 7368d4d..3012346 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -96,6 +96,13 @@ config DRAM_ZQ
 	---help---
 	Set the dram zq value.
 
+config DRAM_ODT_EN
+	bool "sunxi dram odt enable"
+	default n if !MACH_SUN8I_A23
+	default y if MACH_SUN8I_A23
+	---help---
+	Select this to enable dram odt (on die termination).
+
 if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
 config DRAM_EMR1
 	int "sunxi dram emr1 value"
@@ -104,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
@@ -171,6 +171,14 @@ endchoice
 
 endif
 
+if MACH_SUN8I_A23
+config DRAM_ODT_CORRECTION
+	int "sunxi dram odt correction value"
+	default 0
+	---help---
+	Set the dram odt correction value (range -255 - 255).
+endif
+
 config SYS_CLK_FREQ
 	default 912000000 if MACH_SUN7I
 	default 1008000000 if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I || MACH_SUN8I
diff --git a/board/sunxi/dram_sun4i_auto.c b/board/sunxi/dram_sun4i_auto.c
index 09e0c9a..149bb51 100644
--- a/board/sunxi/dram_sun4i_auto.c
+++ b/board/sunxi/dram_sun4i_auto.c
@@ -1,5 +1,6 @@
 #include <common.h>
 #include <asm/arch/dram.h>
+#include <linux/kconfig.h>
 
 static struct dram_para dram_para = {
 	.clock = CONFIG_DRAM_CLK,
@@ -9,7 +10,7 @@ static struct dram_para dram_para = {
 	.io_width = 0,
 	.bus_width = 0,
 	.zq = CONFIG_DRAM_ZQ,
-	.odt_en = CONFIG_DRAM_ODT_EN,
+	.odt_en = IS_ENABLED(CONFIG_DRAM_ODT_EN),
 	.size = 0,
 #ifdef CONFIG_DRAM_TIMINGS_VENDOR_MAGIC
 	.cas = 6,
diff --git a/board/sunxi/dram_sun5i_auto.c b/board/sunxi/dram_sun5i_auto.c
index 660b18e..596a206 100644
--- a/board/sunxi/dram_sun5i_auto.c
+++ b/board/sunxi/dram_sun5i_auto.c
@@ -2,6 +2,7 @@
 
 #include <common.h>
 #include <asm/arch/dram.h>
+#include <linux/kconfig.h>
 
 static struct dram_para dram_para = {
 	.clock = CONFIG_DRAM_CLK,
@@ -12,7 +13,7 @@ static struct dram_para dram_para = {
 	.io_width = 0,
 	.bus_width = 0,
 	.zq = CONFIG_DRAM_ZQ,
-	.odt_en = CONFIG_DRAM_ODT_EN,
+	.odt_en = IS_ENABLED(CONFIG_DRAM_ODT_EN),
 	.size = 0,
 #ifdef CONFIG_DRAM_TIMINGS_VENDOR_MAGIC
 	.cas = 9,
-- 
2.4.1

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Ian Campbell]

On Tue, 2015-05-19 at 14:56 +0200, Hans de Goede wrote:
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> index c736fa3..f7b4915 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> @@ -508,7 +508,7 @@ static void mctl_ddr3_initialize(void)
>  /*
>   * Perform impedance calibration on the DRAM controller side of the wire.
>   */
> -static void mctl_set_impedance(u32 zq, u32 odt_en)
> +static void mctl_set_impedance(u32 zq, bool odt_en)
>  {
>  	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
>  	u32 reg_val;
> @@ -556,7 +556,7 @@ static void mctl_set_impedance(u32 zq, u32 odt_en)
>  	clrbits_le32(&dram->zqcr0, DRAM_ZQCR0_ZCAL);
>  
>  	/* Set I/O configure register */
> -	writel(DRAM_IOCR_ODT_EN(odt_en), &dram->iocr);
> +	writel(DRAM_IOCR_ODT_EN, &dram->iocr);

I think at this point previously odt_en would always be 0x1 here (0x0
having been short circuited earlier).

Whereas this...

> -#define DRAM_IOCR_ODT_EN(n) ((((n) & 0x3) << 30) | ((n) & 0x3) << 0)
> -#define DRAM_IOCR_ODT_EN_MASK DRAM_IOCR_ODT_EN(0x3)
> +#define DRAM_IOCR_ODT_EN ((3 << 30) | (3 << 0))

... now behaves as if it were always 0x3.

AFAICT up until now,@least with the in-tree defconfigs, odt_en was
always 0 for sun4i anyway, but this is a surprising change I think.

DRAM_IOCR_ODT_EN_MASK (which did use 0x3) seems to have been unused. in
tree.

> diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
> index 06adee2..316a333 100644
> --- a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
> +++ b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
> @@ -19,6 +19,7 @@ struct dram_para {
>  	u32 type;
>  	u32 zq;
>  	u32 odt_en;

Make this (and maybe others) an actual bool?

Keeping it a u32 might make hex editing easier though?

Ian.

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Siarhei Siamashka]

On Tue, 19 May 2015 14:56:39 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Make DRAM_ODT_EN Kconfig setting a bool, add a separate DRAM_ODT_CORRECTION
> setting for A23 SoCs and use DRAM_ODT_EN Kconfig everywhere instead of
> only in dram_sun4i.c and hardcoding odt_en elsewhere.
> 
> Note this commit makes no functional changes for existing boards,
> its purpose is to allow changing the odt_en value on future A33 boards.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

The sun4i part is fine.

Regarding the A23 part, the only nitpick from me is the newly added
CONFIG_DRAM_ODT_CORRECTION option. The description does not seem
to be very informative in Kconfig:

> +if MACH_SUN8I_A23
> +config DRAM_ODT_CORRECTION
> +	int "sunxi dram odt correction value"
> +	default 0
> +	---help---
> +	Set the dram odt correction value (range -255 - 255).
> +endif

Since the right correction value is extracted from the FEX file (or
where are we expected to get it from?), a short instruction about
converting the 'dram_odt_en' parameter from FEX into the
DRAM_ODT_CORRECTION option for U-Boot would be quite useful here.

Other than this, looks good and

Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Siarhei Siamashka]

On Tue, 19 May 2015 15:13:18 +0100
Ian Campbell <ijc+uboot@hellion.org.uk> wrote:

> On Tue, 2015-05-19 at 14:56 +0200, Hans de Goede wrote:
> > diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> > index c736fa3..f7b4915 100644
> > --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> > +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> > @@ -508,7 +508,7 @@ static void mctl_ddr3_initialize(void)
> >  /*
> >   * Perform impedance calibration on the DRAM controller side of the wire.
> >   */
> > -static void mctl_set_impedance(u32 zq, u32 odt_en)
> > +static void mctl_set_impedance(u32 zq, bool odt_en)
> >  {
> >  	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
> >  	u32 reg_val;
> > @@ -556,7 +556,7 @@ static void mctl_set_impedance(u32 zq, u32 odt_en)
> >  	clrbits_le32(&dram->zqcr0, DRAM_ZQCR0_ZCAL);
> >  
> >  	/* Set I/O configure register */
> > -	writel(DRAM_IOCR_ODT_EN(odt_en), &dram->iocr);
> > +	writel(DRAM_IOCR_ODT_EN, &dram->iocr);
> 
> I think at this point previously odt_en would always be 0x1 here (0x0
> having been short circuited earlier).

In fact, only 0 and 3 were the realistic practical choices. The commit
message says that no functional changes are introduced for the
existing boards. And this is true because these boards all use 0
by default.

But setting this option to 3 (which enables ODT for both DQ and DQS
lines), combined with picking good ZQ settings, can do wonders and
significantly improve reliability at high DRAM clock speeds. And
this is not just an "armchair expert" opinion :-) The 'highspeedtruck'
branch specifically took advantage of this configuration knob:
    http://lists.denx.de/pipermail/u-boot/2014-July/183981.html
If I understand it correctly, you do have a Cubietruck board, right?
So if you are really interested, then you should be able to easily
verify this yourself.

With a bit of ZQ tuning, I also had DRAM successfully running at
648 MHz on my A13-OLinuXino-MICRO. And Adam Sampson could reach 648 MHz
DRAM clock speeds on his two LinkSprite pcDuino3 Nano boards
too.

> Whereas this...
> 
> > -#define DRAM_IOCR_ODT_EN(n) ((((n) & 0x3) << 30) | ((n) & 0x3) << 0)
> > -#define DRAM_IOCR_ODT_EN_MASK DRAM_IOCR_ODT_EN(0x3)
> > +#define DRAM_IOCR_ODT_EN ((3 << 30) | (3 << 0))
> 
> ... now behaves as if it were always 0x3.

> AFAICT up until now, at least with the in-tree defconfigs, odt_en was
> always 0 for sun4i anyway, but this is a surprising change I think.

Do you mean that you want a better description of this change in the
commit message?

I myself can only welcome the change of this option to the boolean type,
because this way we avoid any need to answer quite a predictable
question "why 3 and not 1 or 2?" at:
   http://linux-sunxi.org/A10_DRAM_Controller_Calibration#Finding_good_impedance_settings

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Hans de Goede]

Hi,

On 05/19/2015 04:13 PM, Ian Campbell wrote:
> On Tue, 2015-05-19 at 14:56 +0200, Hans de Goede wrote:
>> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
>> index c736fa3..f7b4915 100644
>> --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
>> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
>> @@ -508,7 +508,7 @@ static void mctl_ddr3_initialize(void)
>>   /*
>>    * Perform impedance calibration on the DRAM controller side of the wire.
>>    */
>> -static void mctl_set_impedance(u32 zq, u32 odt_en)
>> +static void mctl_set_impedance(u32 zq, bool odt_en)
>>   {
>>   	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
>>   	u32 reg_val;
>> @@ -556,7 +556,7 @@ static void mctl_set_impedance(u32 zq, u32 odt_en)
>>   	clrbits_le32(&dram->zqcr0, DRAM_ZQCR0_ZCAL);
>>
>>   	/* Set I/O configure register */
>> -	writel(DRAM_IOCR_ODT_EN(odt_en), &dram->iocr);
>> +	writel(DRAM_IOCR_ODT_EN, &dram->iocr);
>
> I think at this point previously odt_en would always be 0x1 here (0x0
> having been short circuited earlier).
>
> Whereas this...
>
>> -#define DRAM_IOCR_ODT_EN(n) ((((n) & 0x3) << 30) | ((n) & 0x3) << 0)
>> -#define DRAM_IOCR_ODT_EN_MASK DRAM_IOCR_ODT_EN(0x3)
>> +#define DRAM_IOCR_ODT_EN ((3 << 30) | (3 << 0))
>
> ... now behaves as if it were always 0x3.
>
> AFAICT up until now, at least with the in-tree defconfigs, odt_en was
> always 0 for sun4i anyway, but this is a surprising change I think.

This is deliberate, as Siarhei explained in his reply to v2 the 2 bits
we are setting here enable odt for the DQ resp. DQS lines, having them
enabled for one but not the other does not really make sense.

I'll amend the commit message to explicitly mention this.

Regards,

Hans


>
> DRAM_IOCR_ODT_EN_MASK (which did use 0x3) seems to have been unused. in
> tree.
>
>> diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
>> index 06adee2..316a333 100644
>> --- a/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
>> +++ b/arch/arm/include/asm/arch-sunxi/dram_sun8i_a23.h
>> @@ -19,6 +19,7 @@ struct dram_para {
>>   	u32 type;
>>   	u32 zq;
>>   	u32 odt_en;
>
> Make this (and maybe others) an actual bool?
>
> Keeping it a u32 might make hex editing easier though?
>
> Ian.
>

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Hans de Goede]

Hi,

On 05/19/2015 04:54 PM, Siarhei Siamashka wrote:
> On Tue, 19 May 2015 14:56:39 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Make DRAM_ODT_EN Kconfig setting a bool, add a separate DRAM_ODT_CORRECTION
>> setting for A23 SoCs and use DRAM_ODT_EN Kconfig everywhere instead of
>> only in dram_sun4i.c and hardcoding odt_en elsewhere.
>>
>> Note this commit makes no functional changes for existing boards,
>> its purpose is to allow changing the odt_en value on future A33 boards.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> The sun4i part is fine.
>
> Regarding the A23 part, the only nitpick from me is the newly added
> CONFIG_DRAM_ODT_CORRECTION option. The description does not seem
> to be very informative in Kconfig:
>
>> +if MACH_SUN8I_A23
>> +config DRAM_ODT_CORRECTION
>> +	int "sunxi dram odt correction value"
>> +	default 0
>> +	---help---
>> +	Set the dram odt correction value (range -255 - 255).
>> +endif
>
> Since the right correction value is extracted from the FEX file (or
> where are we expected to get it from?), a short instruction about
> converting the 'dram_odt_en' parameter from FEX into the
> DRAM_ODT_CORRECTION option for U-Boot would be quite useful here.

Thanks for the review, adding a blurb to the Kconfig help on how
to get the correction value from a fex file is a good idea, I've
added such a blurb to the version in my personal tree.

Regards,

Hans

[U-Boot] [PATCH v3] sunxi: Make DRAM_ODT_EN Kconfig setting a bool

[Ian Campbell]

On Tue, 2015-05-19 at 18:39 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05/19/2015 04:13 PM, Ian Campbell wrote:
> > On Tue, 2015-05-19 at 14:56 +0200, Hans de Goede wrote:
> >> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> >> index c736fa3..f7b4915 100644
> >> --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> >> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c
> >> @@ -508,7 +508,7 @@ static void mctl_ddr3_initialize(void)
> >>   /*
> >>    * Perform impedance calibration on the DRAM controller side of the wire.
> >>    */
> >> -static void mctl_set_impedance(u32 zq, u32 odt_en)
> >> +static void mctl_set_impedance(u32 zq, bool odt_en)
> >>   {
> >>   	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
> >>   	u32 reg_val;
> >> @@ -556,7 +556,7 @@ static void mctl_set_impedance(u32 zq, u32 odt_en)
> >>   	clrbits_le32(&dram->zqcr0, DRAM_ZQCR0_ZCAL);
> >>
> >>   	/* Set I/O configure register */
> >> -	writel(DRAM_IOCR_ODT_EN(odt_en), &dram->iocr);
> >> +	writel(DRAM_IOCR_ODT_EN, &dram->iocr);
> >
> > I think at this point previously odt_en would always be 0x1 here (0x0
> > having been short circuited earlier).
> >
> > Whereas this...
> >
> >> -#define DRAM_IOCR_ODT_EN(n) ((((n) & 0x3) << 30) | ((n) & 0x3) << 0)
> >> -#define DRAM_IOCR_ODT_EN_MASK DRAM_IOCR_ODT_EN(0x3)
> >> +#define DRAM_IOCR_ODT_EN ((3 << 30) | (3 << 0))
> >
> > ... now behaves as if it were always 0x3.
> >
> > AFAICT up until now, at least with the in-tree defconfigs, odt_en was
> > always 0 for sun4i anyway, but this is a surprising change I think.
> 
> This is deliberate, as Siarhei explained in his reply to v2 the 2 bits
> we are setting here enable odt for the DQ resp. DQS lines, having them
> enabled for one but not the other does not really make sense.
> 
> I'll amend the commit message to explicitly mention this.

Thanks.

Ian.