sunxi: Fix dram initialization not working on some a33 devices
Subsystems (2)
PATCH 1/2 sunxi: Fix dram initialization not working on some a33 devices
When porting the allwinner dram init code to u-boot we missed some code setting an extra bit when doing auto dram config.
This commits add this bit, fixing dram init not working on the ga10h 10" a33 tablet which I'm bringing up atm.
--- arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +1 -1
@@ -195,7 +195,7 @@ static int mctl_train_dram(struct dram_para *para) (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE ;mctl_data_train_cfg (para); -mctl_set_pir (0x1f3 ); +mctl_set_pir (0x5f3 );return ((readl (&mctl_ctl->pgsr0) >>20 ) &0xff ) ? -EIO :0 ; }
Signature · 1 line
2.4.0
PATCH 2/2 sunxi: Make dram odt-en configurable through Kconfig for A33 based boards
Some A33 based boards use odt, while others do not, so make this configurable.
--- arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c | 3 +-- board/sunxi/Kconfig | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-)
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +1 -2
@@ -19,7 +19,6 @@#define DRAM_CLK_MUL 2#define DRAM_CLK_DIV 4#define DRAM_SIGMA_DELTA_ENABLE 1 -#define DRAM_ODT_EN 0struct 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 {
board/sunxi/Kconfig +8 -0
@@ -95,6 +95,14 @@ config DRAM_ZQ ---help--- Set the dram zq value. +if MACH_SUN8I_A33 +config DRAM_ODT_EN + int "sunxi dram odt enable" + default 0 + ---help--- + Set this to 1 to enable dram odt (on die termination) +endif + if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I config DRAM_EMR1 int "sunxi dram emr1 value"
Signature · 1 line
2.4.0
PATCH 2/2 sunxi: Make dram odt-en configurable through Kconfig for A33 based boards
board/sunxi/Kconfig config DRAM_ZQ · line 98
Quoted 9 lines · Hans de Goede
board/sunxi/Kconfig +1
-0@@ -95,6 +95,14 @@ config DRAM_ZQ ---help--- Set the dram zq value. +if MACH_SUN8I_A33
Shouldn't this be a "depends on MACH_SUN8I_A33" in the entry itself? I see we use if a lot in this file, is there a reason for that or just how it has been done?
In any case using if here is at least consistent so no strong objection on that grounds.
board/sunxi/Kconfig config DRAM_ZQ · line 103
Quoted 5 lines
board/sunxi/Kconfig +5
-0+config DRAM_ODT_EN + int "sunxi dram odt enable" + default 0 + ---help--- + Set this to 1 to enable dram odt (on die termination)
Why is this an int rather than a bool?
PATCH 2/2 sunxi: Make dram odt-en configurable through Kconfig for A33 based boards
Hi,
Quoted 9 lines · Ian Campbell
Quoted 8 lines · Hans de Goede
board/sunxi/Kconfig +0
-0@@ -95,6 +95,14 @@ config DRAM_ZQ ---help--- Set the dram zq value.
Quoted 5 lines
board/sunxi/Kconfig config DRAM_ZQ · line 98
Quoted 1 line
board/sunxi/Kconfig +1
-0+if MACH_SUN8I_A33Shouldn't this be a "depends on
MACH_SUN8I_A33" in the entry itself? I see we use if a lot in this file, is there a reason for that or just how it has been done?
No special reason, just copy paste from elsewhere.
Quoted 11 lines
In any case using if here is at least consistent so no strong objection on that grounds.
board/sunxi/Kconfig config DRAM_ZQ · line 103
Quoted 5 lines
board/sunxi/Kconfig +5
-0+config DRAM_ODT_EN + int "sunxi dram odt enable" + default 0 + ---help--- + Set this to 1 to enable dram odt (on die termination)Why is this an int rather than a bool?
Because it is used directly as an int in the code, otherwise I need to add #ifdef-ery. I guess I could make it a bool and use IS_ENABLED(), but that does lead to slightly less readable code IMHO.
Regards,
Hans
PATCH 2/2 sunxi: Make dram odt-en configurable through Kconfig for A33 based boards
Quoted 8 lines · Hans de Goede
Quoted 7 lines
board/sunxi/Kconfig config DRAM_ZQ · line 103
Quoted 5 lines
board/sunxi/Kconfig +5
-0+config DRAM_ODT_EN + int "sunxi dram odt enable" + default 0 + ---help--- + Set this to 1 to enable dram odt (on die termination)Why is this an int rather than a bool?
Quoted 4 lines
Because it is used directly as an int in the code, otherwise I need to add #ifdef-ery. I guess I could make it a bool and use
IS_ENABLED(), but that does lead to slightly less readable code IMHO.
I'm afraid I think IS_ENABLED is the way to go though, it's the lesser of two evils compared with using an int for a boolean option.
Ian.
PATCH 2/2 sunxi: Make dram odt-en configurable through Kconfig for A33 based boards
Hi,
Quoted 9 lines · Ian Campbell
Quoted 8 lines · Hans de Goede
Quoted 7 lines
board/sunxi/Kconfig config DRAM_ZQ · line 103
Quoted 5 lines
board/sunxi/Kconfig +5
-0+config DRAM_ODT_EN + int "sunxi dram odt enable" + default 0 + ---help--- + Set this to 1 to enable dram odt (on die termination)Why is this an int rather than a bool?
Quoted 7 lines
Quoted 4 lines
Because it is used directly as an int in the code, otherwise I need to add #ifdef-ery. I guess I could make it a bool and use
IS_ENABLED(), but that does lead to slightly less readable code IMHO.I'm afraid I think
IS_ENABLEDis the way to go though, it's the lesser of two evils compared with using an int for a boolean option.
Ok, so while trying to fix this I noticed that there already is a definition of CONFIG_DRAM_ODT_EN in board/sunxi/Kconfig inside a
#if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
block, and that one actually really is an int (it goes from 0-3). so I'm going to leave this as an int, and use the existing option moving it outside of the #if MACH_SUN4I || MACH_SUN5I || MACH_SUN7I block.
v2 coming up.
Regards,
Hans
PATCH 1/2 sunxi: Fix dram initialization not working on some a33 devices
In reply to Hans de GoedeQuoted 7 lines · Hans de Goede
When porting the allwinner dram init code to u-boot we missed some code setting an extra bit when doing auto dram config.
This commits add this bit, fixing dram init not working on the ga10h 10" a33 tablet which I'm bringing up atm.
Signed-off-by Hans de Goede <[email protected]>
PATCH 1/2 sunxi: Fix dram initialization not working on some a33 devices
In reply to Hans de GoedeHello Hans,
Quoted 2 lines · On Wed, 13 May 2015 17:09:33 +0200 Hans de Goede <[email protected]> wrote:
When porting the allwinner dram init code to u-boot we missed some code setting an extra bit when doing auto dram config.
This "extra bit" actually has a name. Not using named constants instead of the magic numbers makes the code rather difficult to review. Your persistent refusal to use the nicely available DRAM controller documentation may be interpreted as an act of disrespect towards the reviewers of your code.
Basically, the A31, A23 and now A33 DRAM initialization code was (and still is) a quick and dirty copy/paste hack. And you made exactly no effort to make it reviewable, despite having been requested to do so multiple times. Yes, it is nice that this code is at least able to boot. But this alone can't tell us anything about its quality (after all, the Allwinner's 3.4 kernel is able to boot a pretty functional Android system too, but it contains a lot of rather questionable or outright buggy code under the hood). The DRAM setup is really critical for the system reliability and has long lasting effects. The bugs in the vast majority of the rest of the U-Boot code are much less important because, because they have no chance to cause any troubles after the OS takes over. The DRAM initialization is different and the "reckless cowboy" approach is simply very dangerous (assuming that the end users may want to use their Allwinner based devices for something more serious than just toying with them for fun).
Quoted 4 lines
This commits add this bit, fixing dram init not working on the ga10h 10" a33 tablet which I'm bringing up atm.
Signed-off-by Hans de Goede <[email protected]>
We were just lucky that this particular bug was too easy to notice and fix. Are you sure that there are no other similar, but more subtle problems, which are just more difficult to reproduce and may remain unnoticed longer?
Based on a quick search in the mailing list archives, I have already provided you with a link to the "spruhn7a.pdf" document 4 (!) times. This is the fifth attempt and I'm seriously starting to lose patience. No offense intended, but you are probably the most dense person that I have ever met :-) There is even an updated C revision of this document:
http://www.ti.com/lit/ug/spruhn7c/spruhn7c.pdf
Quoted 9 lines
--- arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +0
-0
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c static int mctl_train_dram(struct dram_para *para) · line 201
Quoted 8 lines
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +1
-1@@ -195,7 +195,7 @@ static int mctl_train_dram(struct dram_para *para) (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE ;mctl_data_train_cfg (para); -mctl_set_pir (0x1f3 ); +mctl_set_pir (0x5f3 );return ((readl (&mctl_ctl->pgsr0) >>20 ) &0xff ) ? -EIO :0 ; }
Now some questions to Ian Campbell. Are you really happy about the non-descriptive commit messages like "Initial u-boot port by Vishnu Patekar, major cleanup / rewrite by Hans de Goede" in
http://git.denx.de/?p=u-boot.git;a=commit;h=ffc0ae0c70decbe5a910 ?
And are you also happy with neglecting the documentation and keeping all these magic constants in the code? Have you in fact done the review of the code logic? Or at least verified that it had been transplanted from boot0 correctly without introducing any potentially problematic functional changes?
Signature · 2 lines
Best regards, Siarhei Siamashka
PATCH 1/2 sunxi: Fix dram initialization not working on some a33 devices
Hi Sairhei,
Quoted 40 lines · Siarhei Siamashka
Hello Hans,
Quoted 2 lines · On Wed, 13 May 2015 17:09:33 +0200 Hans de Goede <[email protected]> wrote:
When porting the allwinner dram init code to u-boot we missed some code setting an extra bit when doing auto dram config.
This "extra bit" actually has a name. Not using named constants instead of the magic numbers makes the code rather difficult to review. Your persistent refusal to use the nicely available DRAM controller documentation may be interpreted as an act of disrespect towards the reviewers of your code.
Basically, the A31, A23 and now A33 DRAM initialization code was (and still is) a quick and dirty copy/paste hack. And you made exactly no effort to make it reviewable, despite having been requested to do so multiple times. Yes, it is nice that this code is at least able to boot. But this alone can't tell us anything about its quality (after all, the Allwinner's 3.4 kernel is able to boot a pretty functional Android system too, but it contains a lot of rather questionable or outright buggy code under the hood). The DRAM setup is really critical for the system reliability and has long lasting effects. The bugs in the vast majority of the rest of the U-Boot code are much less important because, because they have no chance to cause any troubles after the OS takes over. The DRAM initialization is different and the "reckless cowboy" approach is simply very dangerous (assuming that the end users may want to use their Allwinner based devices for something more serious than just toying with them for fun).
Quoted 4 lines
This commits add this bit, fixing dram init not working on the ga10h 10" a33 tablet which I'm bringing up atm.
Signed-off-by Hans de Goede <[email protected]>We were just lucky that this particular bug was too easy to notice and fix. Are you sure that there are no other similar, but more subtle problems, which are just more difficult to reproduce and may remain unnoticed longer?
Quoted 6 lines
Based on a quick search in the mailing list archives, I have already provided you with a link to the "spruhn7a.pdf" document 4 (!) times. This is the fifth attempt and I'm seriously starting to lose patience. No offense intended, but you are probably the most dense person that I have ever met :-) There is even an updated C revision of this document:
http://www.ti.com/lit/ug/spruhn7c/spruhn7c.pdf
Last time you provided that link it was when discussing the a23 (or was it the a31?) dram controller, and now you provide the same link for the a33, this makes no sense since all 3 dram controllers are actually quite different.
We simply do not have reliable dram controller documentation. Yes there may be some common ancestry between the ti dram controller you're linking to and the allwinner ones, but allwinner has been consistently tweaking there dram controller with each new soc generation, so even if that document is correct for one generation it certainly will not be for other generations.
Quoted 16 lines
Quoted 16 lines
--- arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +1
-1@@ -195,7 +195,7 @@ static int mctl_train_dram(struct dram_para *para) (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE ;mctl_data_train_cfg (para); -mctl_set_pir (0x1f3 ); +mctl_set_pir (0x5f3 );
Quoted 8 lines
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c static int mctl_train_dram(struct dram_para *para) · line 201
Quoted 2 lines
arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c +0
-0return ((readl (&mctl_ctl->pgsr0) >>20 ) &0xff ) ? -EIO :0 ; }Now some questions to Ian Campbell. Are you really happy about the non-descriptive commit messages like "Initial u-boot port by Vishnu Patekar, major cleanup / rewrite by Hans de Goede" in
http://git.denx.de/?p=u-boot.git;a=commit;h=ffc0ae0c70decbe5a910 ?And are you also happy with neglecting the documentation
There is NO documentation. What you're doing is the same as giving someone the manual for a sony video recorder and then telling him to use that to program recording of his favorite show into a jvc recorder, yes there are going to be similarities, that does not make the manual from manufacturer a going to apply to a device from manufacturer b.
Last time we had this discussion it was trivial to point out various places where the documentation did not match up, and that was like 3 generations of dram controllers ago ...
With all that said you're welcome to find things which do match and provide patches to convert magic constants to useful symbolic names.
What you're not welcome to do is trash my work and throw mud at me, it would greatly help if you were to use a more constructive tone in future email.
Regards,
Hans