| Message ID | 1431715428-23385-1-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Hans de Goede |
| Headers | show |
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.
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.
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
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
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(-)