[U-Boot] [PATCH v2 0/5] sun6i: A31s / CSQ_CS908 board support

[U-Boot] [PATCH v2 0/5] sun6i: A31s / CSQ_CS908 board support

[Hans de Goede]

Hi,

Here is v2 of my sun6i: A31s / CSQ_CS908 board support series.

Changes since v1:
-"sun6i: Make dram clk and zq value Kconfig options"
 -Mention changing of default zq value in commit message
 -Drop "if EXPERT" usage, as that breaks setting things through defconfig files
-"sun6i: Drop some "unknown magic" from dram init"
 -Mention that the info on what the dropped unknown magic did comes from
  Allwinner
-"sun6i: Add new CSQ_CS908 board"
 -Add setting of LDO for phy, so that network works
 -Correct usb vbus settings

Regards,

Hans

[U-Boot] [PATCH v2 1/5] sun6i: Make dram clk and zq value Kconfig options

[Hans de Goede]

It turns out that there is a too large spread between boards to handle this
with a default value, turn this into Kconfig options, and set the values
the factory images are using for the Colombus and Mele_M9 boards.

Note this changes the ZQ default when not overriden through defconfig from
120 to 123, as that is what most boards seem to actually use.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 12 +++++-------
 board/sunxi/Kconfig                   | 17 +++++++++++++++++
 configs/Colombus_defconfig            |  2 ++
 configs/Mele_M9_defconfig             |  2 ++
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
index 10a6241..30439dc 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
@@ -17,9 +17,7 @@
 #include <asm/arch/dram.h>
 #include <asm/arch/prcm.h>
 
-/* DRAM clk & zq defaults, maybe turn these into Kconfig options ? */
-#define DRAM_CLK_DEFAULT 312000000
-#define DRAM_ZQ_DEFAULT 0x78
+#define DRAM_CLK (CONFIG_DRAM_CLK * 1000000)
 
 struct dram_sun6i_para {
 	u8 bus_width;
@@ -48,7 +46,7 @@ static void mctl_sys_init(void)
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
 	const int dram_clk_div = 2;
 
-	clock_set_pll5(DRAM_CLK_DEFAULT * dram_clk_div);
+	clock_set_pll5(DRAM_CLK * dram_clk_div);
 
 	clrsetbits_le32(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_DIV0_MASK,
 		CCM_DRAMCLK_CFG_DIV0(dram_clk_div) | CCM_DRAMCLK_CFG_RST |
@@ -173,7 +171,7 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para)
 
 	await_completion(&mctl_phy->pgsr, 0x03, 0x03);
 
-	writel(DRAM_ZQ_DEFAULT, &mctl_phy->zq0cr1);
+	writel(CONFIG_DRAM_ZQ, &mctl_phy->zq0cr1);
 
 	setbits_le32(&mctl_phy->pir, MCTL_PIR_CLEAR_STATUS);
 	writel(MCTL_PIR_STEP1, &mctl_phy->pir);
@@ -219,9 +217,9 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para)
 	await_completion(&mctl_ctl->sstat, 0x07, 0x01);
 
 	/* Set number of clks per micro-second */
-	writel(DRAM_CLK_DEFAULT / 1000000, &mctl_ctl->togcnt1u);
+	writel(DRAM_CLK / 1000000, &mctl_ctl->togcnt1u);
 	/* Set number of clks per 100 nano-seconds */
-	writel(DRAM_CLK_DEFAULT / 10000000, &mctl_ctl->togcnt100n);
+	writel(DRAM_CLK / 10000000, &mctl_ctl->togcnt100n);
 	/* Set memory timing registers */
 	writel(MCTL_TREFI, &mctl_ctl->trefi);
 	writel(MCTL_TMRD, &mctl_ctl->tmrd);
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 246cd9a..6162227 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -32,6 +32,23 @@ config MACH_SUN8I
 
 endchoice
 
+if MACH_SUN6I
+
+config DRAM_CLK
+	int "sun6i dram clock speed"
+	default 312
+	---help---
+	Set the dram clock speed, valid range 240 - 480, must be a multiple
+	of 24.
+
+config DRAM_ZQ
+	int "sun6i dram zq value"
+	default 123
+	---help---
+	Set the dram zq value.
+
+endif
+
 config SYS_CONFIG_NAME
 	string
 	default "sun4i" if MACH_SUN4I
diff --git a/configs/Colombus_defconfig b/configs/Colombus_defconfig
index de78a01..9b4968f 100644
--- a/configs/Colombus_defconfig
+++ b/configs/Colombus_defconfig
@@ -5,3 +5,5 @@ CONFIG_USB_KEYBOARD=n
 +S:CONFIG_ARCH_SUNXI=y
 +S:CONFIG_MACH_SUN6I=y
 +S:CONFIG_TARGET_COLOMBUS=y
++S:CONFIG_DRAM_CLK=288
++S:CONFIG_DRAM_ZQ=379
diff --git a/configs/Mele_M9_defconfig b/configs/Mele_M9_defconfig
index 40eabce..740b931 100644
--- a/configs/Mele_M9_defconfig
+++ b/configs/Mele_M9_defconfig
@@ -5,6 +5,8 @@ CONFIG_FDTFILE="sun6i-a31-m9.dtb"
 +S:CONFIG_ARCH_SUNXI=y
 +S:CONFIG_MACH_SUN6I=y
 +S:CONFIG_TARGET_MELE_M9=y
++S:CONFIG_DRAM_CLK=312
++S:CONFIG_DRAM_ZQ=120
 # Ethernet phy power
 +S:CONFIG_AXP221_DLDO1_VOLT=3300
 # USB hub power
-- 
2.1.0

[U-Boot] [PATCH v2 1/5] sun6i: Make dram clk and zq value Kconfig options

[Ian Campbell]

On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
> It turns out that there is a too large spread between boards to handle this
> with a default value, turn this into Kconfig options, and set the values
> the factory images are using for the Colombus and Mele_M9 boards.
> 
> Note this changes the ZQ default when not overriden through defconfig from
> 120 to 123, as that is what most boards seem to actually use.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

With one minor query:

> +config DRAM_CLK
> +	int "sun6i dram clock speed"
> +	default 312
> +	---help---
> +	Set the dram clock speed, valid range 240 - 480, must be a multiple
> +	of 24.
> [...]
> diff --git a/configs/Mele_M9_defconfig b/configs/Mele_M9_defconfig
> index 40eabce..740b931 100644
> --- a/configs/Mele_M9_defconfig
> +++ b/configs/Mele_M9_defconfig
> @@ -5,6 +5,8 @@ CONFIG_FDTFILE="sun6i-a31-m9.dtb"
>  +S:CONFIG_ARCH_SUNXI=y
>  +S:CONFIG_MACH_SUN6I=y
>  +S:CONFIG_TARGET_MELE_M9=y
> ++S:CONFIG_DRAM_CLK=312

You are overriding this to be the default, is that deliberate/necessary?

I suspect it's just to keep both or neither of CLK and ZQ explicitly
give, which is a fine reason.

Ian.

[U-Boot] [PATCH v2 1/5] sun6i: Make dram clk and zq value Kconfig options

[Hans de Goede]

Hi,

On 11/25/2014 09:55 AM, Ian Campbell wrote:
> On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
>> It turns out that there is a too large spread between boards to handle this
>> with a default value, turn this into Kconfig options, and set the values
>> the factory images are using for the Colombus and Mele_M9 boards.
>>
>> Note this changes the ZQ default when not overriden through defconfig from
>> 120 to 123, as that is what most boards seem to actually use.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Ian Campbell <ijc@hellion.org.uk>
>
> With one minor query:
>
>> +config DRAM_CLK
>> +	int "sun6i dram clock speed"
>> +	default 312
>> +	---help---
>> +	Set the dram clock speed, valid range 240 - 480, must be a multiple
>> +	of 24.
>> [...]
>> diff --git a/configs/Mele_M9_defconfig b/configs/Mele_M9_defconfig
>> index 40eabce..740b931 100644
>> --- a/configs/Mele_M9_defconfig
>> +++ b/configs/Mele_M9_defconfig
>> @@ -5,6 +5,8 @@ CONFIG_FDTFILE="sun6i-a31-m9.dtb"
>>   +S:CONFIG_ARCH_SUNXI=y
>>   +S:CONFIG_MACH_SUN6I=y
>>   +S:CONFIG_TARGET_MELE_M9=y
>> ++S:CONFIG_DRAM_CLK=312
>
> You are overriding this to be the default, is that deliberate/necessary?
>
> I suspect it's just to keep both or neither of CLK and ZQ explicitly
> give, which is a fine reason.

Yes, this is just here to make it easier to see what CLK and ZQ is used without
needing to look in the Kconfig, it is also here to make it a better template
for other sun6i defconfig-s

Regards,

Hans

[U-Boot] [PATCH v2 1/5] sun6i: Make dram clk and zq value Kconfig options

[Siarhei Siamashka]

On Sun, 23 Nov 2014 14:43:11 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> It turns out that there is a too large spread between boards to handle this
> with a default value, turn this into Kconfig options, and set the values
> the factory images are using for the Colombus and Mele_M9 boards.
> 
> Note this changes the ZQ default when not overriden through defconfig from
> 120 to 123, as that is what most boards seem to actually use.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

0x7b (or 123 in the decimal) form is a default reset value in the
hardware register (at least on A10/A13/A20 and also on TI Keystone2
hardware). So it indeed makes sense to have it as the default value for
u-boot. And also very likely shows that whoever provided the DRAM
configurations with this value, did not in fact bother to really
configure ZQ to something meaningful :-)

In general, if we read the JEDEC booklets, then ZQ calibration is the
feature, which allows to improve reliability and increase DRAM clock
speeds. The DRAM clock speeds, which are well beyond what the
Allwinner A31 boards seem to be using at the moment.

I hope that 0x78 value for ZQ on Mele M9 is really there for a reason,
and not something like actually 0x7B that got misread from the printed
documentation or from screen :-) As the DRAM clock speeds on A31
hardware seem to be really very low, I would suspect that almost any
ZQ value would be probably good enough in practice.

For A10/A13/A20 hardware we actually have easy tools to measure DRAM
reliability. These tools can help to pick good ZQ settings. Something
similar may be potentially implemented for A31 too.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2 2/5] sun6i: Add sunxi_get_ss_bonding_id() function

[Hans de Goede]

Add a sunxi_get_ss_bonding_id() function, and use it to differentiate between
the A31s and the A31.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ian Campbell <ijc@hellion.org.uk>
---
 arch/arm/cpu/armv7/sunxi/cpu_info.c           | 38 ++++++++++++++++++++++++++-
 arch/arm/include/asm/arch-sunxi/clock_sun6i.h |  4 ++-
 arch/arm/include/asm/arch-sunxi/cpu.h         |  5 ++++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/cpu_info.c b/arch/arm/cpu/armv7/sunxi/cpu_info.c
index 41b9add..5146dc4 100644
--- a/arch/arm/cpu/armv7/sunxi/cpu_info.c
+++ b/arch/arm/cpu/armv7/sunxi/cpu_info.c
@@ -9,6 +9,32 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
+#include <asm/arch/clock.h>
+
+#ifdef CONFIG_MACH_SUN6I
+int sunxi_get_ss_bonding_id(void)
+{
+	struct sunxi_ccm_reg * const ccm =
+		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	static int bonding_id = -1;
+
+	if (bonding_id != -1)
+		return bonding_id;
+
+	/* Enable Security System */
+	setbits_le32(&ccm->ahb_reset0_cfg, 1 << AHB_RESET_OFFSET_SS);
+	setbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_SS);
+
+	bonding_id = readl(SUNXI_SS_BASE);
+	bonding_id = (bonding_id >> 16) & 0x7;
+
+	/* Disable Security System again */
+	clrbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_SS);
+	clrbits_le32(&ccm->ahb_reset0_cfg, 1 << AHB_RESET_OFFSET_SS);
+
+	return bonding_id;
+}
+#endif
 
 #ifdef CONFIG_DISPLAY_CPUINFO
 int print_cpuinfo(void)
@@ -24,7 +50,17 @@ int print_cpuinfo(void)
 	default: puts("CPU:   Allwinner A1X (SUN5I)\n");
 	}
 #elif defined CONFIG_MACH_SUN6I
-	puts("CPU:   Allwinner A31 (SUN6I)\n");
+	switch (sunxi_get_ss_bonding_id()) {
+	case SUNXI_SS_BOND_ID_A31:
+		puts("CPU:   Allwinner A31 (SUN6I)\n");
+		break;
+	case SUNXI_SS_BOND_ID_A31S:
+		puts("CPU:   Allwinner A31s (SUN6I)\n");
+		break;
+	default:
+		printf("CPU:   Allwinner A31? (SUN6I, id: %d)\n",
+		       sunxi_get_ss_bonding_id());
+	}
 #elif defined CONFIG_MACH_SUN7I
 	puts("CPU:   Allwinner A20 (SUN7I)\n");
 #elif defined CONFIG_MACH_SUN8I
diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
index 50a4b69..7e810bb 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
@@ -209,6 +209,7 @@ struct sunxi_ccm_reg {
 #define AHB_GATE_OFFSET_MMC1		9
 #define AHB_GATE_OFFSET_MMC0		8
 #define AHB_GATE_OFFSET_MMC(n)		(AHB_GATE_OFFSET_MMC0 + (n))
+#define AHB_GATE_OFFSET_SS		5
 
 /* ahb_gate1 offsets */
 #define AHB_GATE_OFFSET_DRC0		25
@@ -270,8 +271,9 @@ struct sunxi_ccm_reg {
 #define AHB_RESET_OFFSET_MMC1		9
 #define AHB_RESET_OFFSET_MMC0		8
 #define AHB_RESET_OFFSET_MMC(n)		(AHB_RESET_OFFSET_MMC0 + (n))
+#define AHB_RESET_OFFSET_SS		5
 
-/* ahb_reset0 offsets */
+/* ahb_reset1 offsets */
 #define AHB_RESET_OFFSET_DRC0		25
 #define AHB_RESET_OFFSET_DE_BE0		12
 #define AHB_RESET_OFFSET_HDMI		11
diff --git a/arch/arm/include/asm/arch-sunxi/cpu.h b/arch/arm/include/asm/arch-sunxi/cpu.h
index bdee89e..90e06c0 100644
--- a/arch/arm/include/asm/arch-sunxi/cpu.h
+++ b/arch/arm/include/asm/arch-sunxi/cpu.h
@@ -135,9 +135,14 @@
 
 #define SUNXI_CPU_CFG			(SUNXI_TIMER_BASE + 0x13c)
 
+/* SS bonding ids used for cpu identification */
+#define SUNXI_SS_BOND_ID_A31		4
+#define SUNXI_SS_BOND_ID_A31S		5
+
 #ifndef __ASSEMBLY__
 void sunxi_board_init(void);
 void sunxi_reset(void);
+int sunxi_get_ss_bonding_id(void);
 #endif /* __ASSEMBLY__ */
 
 #endif /* _CPU_H */
-- 
2.1.0

[U-Boot] [PATCH v2 3/5] sun6i: dram: Do not try to initialize a second dram chan on A31s

[Hans de Goede]

The A31s only has one dram channel, so do not bother with trying to initialize
a second channel.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/Makefile     |  2 +-
 arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
index 3b6ae47..1337b60 100644
--- a/arch/arm/cpu/armv7/sunxi/Makefile
+++ b/arch/arm/cpu/armv7/sunxi/Makefile
@@ -10,6 +10,7 @@
 obj-y	+= timer.o
 obj-y	+= board.o
 obj-y	+= clock.o
+obj-y	+= cpu_info.o
 obj-y	+= pinmux.o
 obj-$(CONFIG_MACH_SUN6I)	+= prcm.o
 obj-$(CONFIG_MACH_SUN8I)	+= prcm.o
@@ -21,7 +22,6 @@ obj-$(CONFIG_MACH_SUN7I)	+= clock_sun4i.o
 obj-$(CONFIG_MACH_SUN8I)	+= clock_sun6i.o
 
 ifndef CONFIG_SPL_BUILD
-obj-y	+= cpu_info.o
 ifdef CONFIG_ARMV7_PSCI
 obj-y	+= psci.o
 endif
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
index 30439dc..2ac0b58 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
@@ -372,10 +372,15 @@ unsigned long sunxi_dram_init(void)
 		.rows = 16,
 	};
 
+	/* A31s only has one channel */
+	if (sunxi_get_ss_bonding_id() == SUNXI_SS_BOND_ID_A31S)
+		para.chan = 1;
+
 	mctl_sys_init();
 
 	mctl_dll_init(0, &para);
-	mctl_dll_init(1, &para);
+	if (para.chan == 2)
+		mctl_dll_init(1, &para);
 
 	setbits_le32(&mctl_com->ccr,
 		     MCTL_CCR_MASTER_CLK_EN |
@@ -383,7 +388,9 @@ unsigned long sunxi_dram_init(void)
 		     MCTL_CCR_CH1_CLK_EN);
 
 	mctl_channel_init(0, &para);
-	mctl_channel_init(1, &para);
+	if (para.chan == 2)
+		mctl_channel_init(1, &para);
+
 	mctl_com_init(&para);
 	mctl_port_cfg();
 
-- 
2.1.0

[U-Boot] [PATCH v2 3/5] sun6i: dram: Do not try to initialize a second dram chan on A31s

[Ian Campbell]

On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
> The A31s only has one dram channel, so do not bother with trying to initialize
> a second channel.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH v2 3/5] sun6i: dram: Do not try to initialize a second dram chan on A31s

[Siarhei Siamashka]

On Sun, 23 Nov 2014 14:43:13 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> The A31s only has one dram channel, so do not bother with trying to initialize
> a second channel.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Ian Campbell has already noticed in the earlier review. In the case *if*
the automatic detection works correctly, then why bother adding this
*extra* code? Based on your description, it looks like a superfluous
band-aid and increases the number of "moving parts". Which generally
makes the code less maintainable.

There was a talk about calling it a "performance optimization" earlier,
but the v2 commit message does not mention this. Neither does it present
any benchmark numbers.

Basically, it boils down to whether we can trust the automatic detection
code to do a proper job or not. Normally, if the automatic detection
does not work correctly in all cases, then it needs to be fixed in the
long run. But I can clearly understand if you are not willing to take
any risks and just want to deploy something that somehow works to the
end users sooner.

Either way, I have nothing against these patches. Just would prefer a
little bit better transparency and more clear commit messages. Keep up
the good job.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2 3/5] sun6i: dram: Do not try to initialize a second dram chan on A31s

[Hans de Goede]

Hi,

On 12-12-14 21:25, Siarhei Siamashka wrote:
> On Sun, 23 Nov 2014 14:43:13 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> The A31s only has one dram channel, so do not bother with trying to initialize
>> a second channel.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Ian Campbell has already noticed in the earlier review. In the case *if*
> the automatic detection works correctly,

It does work correctly in my testing.

> then why bother adding this
> *extra* code?

I'm just mimicking what the boot0 code does here, and the boot0 code does
detect the SoC type and not bother with the second channel on A31s, and that
seems sensible to do, nothing good will come out of trying to use the second
channel on A31s.

> Based on your description, it looks like a superfluous
> band-aid and increases the number of "moving parts". Which generally
> makes the code less maintainable.
>
> There was a talk about calling it a "performance optimization" earlier,
> but the v2 commit message does not mention this. Neither does it present
> any benchmark numbers.
>
> Basically, it boils down to whether we can trust the automatic detection
> code to do a proper job or not. Normally, if the automatic detection
> does not work correctly in all cases, then it needs to be fixed in the
> long run. But I can clearly understand if you are not willing to take
> any risks and just want to deploy something that somehow works to the
> end users sooner.

In my testing the auto-detect code worked fine for automatically disabling
the 2nd channel on on A31s, but as said it seems sensible to just not bother
with the 2nd channel at all on A31s.

Regards,

Hans

[U-Boot] [PATCH v2 3/5] sun6i: dram: Do not try to initialize a second dram chan on A31s

[Siarhei Siamashka]

On Sat, 13 Dec 2014 12:00:44 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 12-12-14 21:25, Siarhei Siamashka wrote:
> > On Sun, 23 Nov 2014 14:43:13 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >
> >> The A31s only has one dram channel, so do not bother with trying to initialize
> >> a second channel.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Ian Campbell has already noticed in the earlier review. In the case *if*
> > the automatic detection works correctly,
> 
> It does work correctly in my testing.

Thanks. Yes, I have seen your confirmation earlier:
    http://lists.denx.de/pipermail/u-boot/2014-November/196198.html

"My assumption was that it would not work, as the A31s has only one
channel, or so the datasheets claim. But it turned out it does work"

> > then why bother adding this
> > *extra* code?
> 
> I'm just mimicking what the boot0 code does here, and the boot0 code does
> detect the SoC type and not bother with the second channel on A31s, and that
> seems sensible to do, nothing good will come out of trying to use the second
> channel on A31s.

That's a good point. If boot0 is doing this, then Allwinner developers
probably had some valid reason. Too bad that we don't know this reason.

You are currently transplanting the Allwinner code into u-boot. And
in the short run it is indeed safer to keep it as is. Especially
considering that you are apparently not doing any extensive testing
of the individual features and hardware registers, but instead heavily
relying on assumptions.

> > Based on your description, it looks like a superfluous
> > band-aid and increases the number of "moving parts". Which generally
> > makes the code less maintainable.
> >
> > There was a talk about calling it a "performance optimization" earlier,
> > but the v2 commit message does not mention this. Neither does it present
> > any benchmark numbers.
> >
> > Basically, it boils down to whether we can trust the automatic detection
> > code to do a proper job or not. Normally, if the automatic detection
> > does not work correctly in all cases, then it needs to be fixed in the
> > long run. But I can clearly understand if you are not willing to take
> > any risks and just want to deploy something that somehow works to the
> > end users sooner.
> 
> In my testing the auto-detect code worked fine for automatically disabling
> the 2nd channel on on A31s, but as said it seems sensible to just not bother
> with the 2nd channel at all on A31s.

In what way is it sensible to have three possible code paths (A31s, A31
with two channels, A31 with a single channel) instead of two before
this change (A31/A31s with a single channel, A31 with two channels)?
This just results in a worse testing coverage situation.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2 4/5] sun6i: Drop some "unknown magic" from dram init

[Hans de Goede]

Allwinner tells us that this bit of code is the rtc ram being used to detect
coming out of "super-standby" mode, and if that is the case, going out of
self-refresh mode.

Since we do not support "super-standby" mode, this can be dropped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ian Campbell <ijc@hellion.org.uk>
---
 arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
index 2ac0b58..5e163cb 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
@@ -140,9 +140,6 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para)
 
 	writel((MCTL_TITMSRST << 18) | (MCTL_TDLLLOCK << 6) | MCTL_TDLLSRST,
 	       &mctl_phy->ptr0);
-	/* Unknown magic performed by boot0 */
-	if ((readl(SUNXI_RTC_BASE + 0x20c) & 3) == 2)
-		setbits_le32(&mctl_phy->ptr0, 1 << 18);
 
 	writel((MCTL_TDINIT1 << 19) | MCTL_TDINIT0, &mctl_phy->ptr1);
 	writel((MCTL_TDINIT3 << 17) | MCTL_TDINIT2, &mctl_phy->ptr2);
-- 
2.1.0

[U-Boot] [PATCH v2 4/5] sun6i: Drop some "unknown magic" from dram init

[Siarhei Siamashka]

On Sun, 23 Nov 2014 14:43:14 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

Sorry for a late reply. I was not on CC for these patches and don't
properly keep track of the u-boot mailing list activity lately.

> Allwinner tells us that this bit of code is the rtc ram being used to detect
> coming out of "super-standby" mode, and if that is the case, going out of
> self-refresh mode.

If I understand this paragraph correctly, you seem to have a privileged
communication channel with Allwinner. And I wonder if you are keeping
track of this information somehow and willing to share it with the
others?

For sun4i/sun5i/sun7i DRAM controller, we actively used a wiki page:
    http://linux-sunxi.org/A10_DRAM_Controller_Register_Guide
Basically, whenever something new is found about some hardware
register, just do a quick edit to make sure that this information
is not forgotten or lost. This does not really take much time.
Trying to remember something later and searching it in the scattered
old e-mails is usually a bigger waste of time.

The most interesting question is of course whether Allwinner has any
plans to provide real DRAM controller documentation. If they do it,
then we don't have to waste time on finding and documenting all the
relevant information.

> Since we do not support "super-standby" mode, this can be dropped.

This patch seems to have a similar purpose to what we did for
sun4i/sun5i/sun7i dram controller earlier:
    http://git.denx.de/?p=u-boot.git;a=commitdiff;h=f2577967738f923571b7156ad46ef91d9fa8d9f8

A somewhat tricky part about this stuff is that while we don't support
the "super-standby" mode, we still have to somehow deal with it whenever
we actually encounter it in the wild. It is easy to reproduce if you
have an Allwinner tablet with Android firmware (just disable WLAN, let
it sleep for a while, insert an SD card with u-boot and GNU/Linux, try
to press the power button to wake the device from sleep).

For example, on sun5i/sun7i hardware, doing "writel(0x16510000,
&dram->ppwrsctl)" part is important if we want to really discard the
RAM data and boot normally. A failure to do so just transitions the
device into and unbootable state. Moreover, the device may look as
"bricked" to the end user until the RTC battery runs out of juice or
some special action is taken. I think that this scenario is exactly
what was described in the NOTICE section:
    http://cubieboard.org/2014/01/13/upgrade-new-android-for-cubietruckv1-01/

I'm not sure if all the same applies to A31 hardware (does having a
dedicated OpenRISC power management chip change anything?), but just
chopping off the code and hoping for the best might be not the best
action.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Ian Campbell <ijc@hellion.org.uk>
> ---
>  arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> index 2ac0b58..5e163cb 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> @@ -140,9 +140,6 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para)
>  
>  	writel((MCTL_TITMSRST << 18) | (MCTL_TDLLLOCK << 6) | MCTL_TDLLSRST,
>  	       &mctl_phy->ptr0);
> -	/* Unknown magic performed by boot0 */
> -	if ((readl(SUNXI_RTC_BASE + 0x20c) & 3) == 2)
> -		setbits_le32(&mctl_phy->ptr0, 1 << 18);

Regarding the "unknown magic". In fact we already have quite a lot of
information about various obscure parts of the Allwinner DRAM code:
    http://lists.denx.de/pipermail/u-boot/2014-September/189199.html

For example, for this PTR0 PHY register, we have the following
information from the RK3288 header files:

/* PTR0 */
#define tITMSRST(n)          ((n)<<18)
#define tDLLLOCK(n)          ((n)<<6)
#define tDLLSRST(n)          ((n)<<0)

Having the bit fields as named identifiers instead of Allwinner magic
numbers makes the code more readable.

And we also have the "4.41 PHY Timing Register 0 (PTR0)" section in
http://www.ti.com/lit/ug/spruhn7a/spruhn7a.pdf for some general
explanations.

The RK3288 is almost a perfect match, because all the hardware register
addresses and the register names are nearly identical. The TI Keystone2
is a lot more distant relative, so the information in its documentation
is less trustworthy for us.

The actual functionality of these bits still needs to be confirmed in
an experimental way (instead of just making theoretical assumptions and
hoping for the best). But again, wasting time on doing this only makes
sense if Allwinner in fact has no plans to release proper documentation
for the DRAM controllers used in their SoCs.
 
-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2 4/5] sun6i: Drop some "unknown magic" from dram init

[Hans de Goede]

Hi,

On 12-12-14 21:24, Siarhei Siamashka wrote:
> On Sun, 23 Nov 2014 14:43:14 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
> Sorry for a late reply. I was not on CC for these patches and don't
> properly keep track of the u-boot mailing list activity lately.
>
>> Allwinner tells us that this bit of code is the rtc ram being used to detect
>> coming out of "super-standby" mode, and if that is the case, going out of
>> self-refresh mode.
>
> If I understand this paragraph correctly, you seem to have a privileged
> communication channel with Allwinner. And I wonder if you are keeping
> track of this information somehow and willing to share it with the
> others?

I do not really have a private communication channel, as Simos Xenitellis
who has been working on getting beter collaboration between Allwinner and the
linux-sunxi community has mentioned in the
"[linux-sunxi] Introductions and Allwinner documentation update"
thread, kevin at allwinnertech.com and shuge at allwinnertech.com are available to
ask questions we may have and I've been doing that.

Note that I had multiple questions about the sun6i DRAM controller, and
the info in this commit is all I got unfortunately they are not able to
share anything wrt the DRAM controller.

>
> For sun4i/sun5i/sun7i DRAM controller, we actively used a wiki page:
>      http://linux-sunxi.org/A10_DRAM_Controller_Register_Guide
> Basically, whenever something new is found about some hardware
> register, just do a quick edit to make sure that this information
> is not forgotten or lost. This does not really take much time.
> Trying to remember something later and searching it in the scattered
> old e-mails is usually a bigger waste of time.
>
> The most interesting question is of course whether Allwinner has any
> plans to provide real DRAM controller documentation. If they do it,
> then we don't have to waste time on finding and documenting all the
> relevant information.

AFAIK there are no plans to provide any documentation, or any info at all,
as said I had multiple questions and the info in this commit is all the
info I got.

The A23 dram support I'm about to post is also all my own work with no
help from Allwinner.

>
>> Since we do not support "super-standby" mode, this can be dropped.
>
> This patch seems to have a similar purpose to what we did for
> sun4i/sun5i/sun7i dram controller earlier:
>      http://git.denx.de/?p=u-boot.git;a=commitdiff;h=f2577967738f923571b7156ad46ef91d9fa8d9f8
>
> A somewhat tricky part about this stuff is that while we don't support
> the "super-standby" mode, we still have to somehow deal with it whenever
> we actually encounter it in the wild. It is easy to reproduce if you
> have an Allwinner tablet with Android firmware (just disable WLAN, let
> it sleep for a while, insert an SD card with u-boot and GNU/Linux, try
> to press the power button to wake the device from sleep).
>
> For example, on sun5i/sun7i hardware, doing "writel(0x16510000,
> &dram->ppwrsctl)" part is important if we want to really discard the
> RAM data and boot normally. A failure to do so just transitions the
> device into and unbootable state. Moreover, the device may look as
> "bricked" to the end user until the RTC battery runs out of juice or
> some special action is taken. I think that this scenario is exactly
> what was described in the NOTICE section:
>      http://cubieboard.org/2014/01/13/upgrade-new-android-for-cubietruckv1-01/
>
> I'm not sure if all the same applies to A31 hardware (does having a
> dedicated OpenRISC power management chip change anything?), but just
> chopping off the code and hoping for the best might be not the best
> action.

A valid point, unfortunately I only have A31 based top set boxes and those
do not do suspend / resume AFAIK...

But definitely something to keep in mind.

Regards,

Hans


>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Ian Campbell <ijc@hellion.org.uk>
>> ---
>>   arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> index 2ac0b58..5e163cb 100644
>> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> @@ -140,9 +140,6 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para)
>>
>>   	writel((MCTL_TITMSRST << 18) | (MCTL_TDLLLOCK << 6) | MCTL_TDLLSRST,
>>   	       &mctl_phy->ptr0);
>> -	/* Unknown magic performed by boot0 */
>> -	if ((readl(SUNXI_RTC_BASE + 0x20c) & 3) == 2)
>> -		setbits_le32(&mctl_phy->ptr0, 1 << 18);
>
> Regarding the "unknown magic". In fact we already have quite a lot of
> information about various obscure parts of the Allwinner DRAM code:
>      http://lists.denx.de/pipermail/u-boot/2014-September/189199.html
>
> For example, for this PTR0 PHY register, we have the following
> information from the RK3288 header files:
>
> /* PTR0 */
> #define tITMSRST(n)          ((n)<<18)
> #define tDLLLOCK(n)          ((n)<<6)
> #define tDLLSRST(n)          ((n)<<0)
>
> Having the bit fields as named identifiers instead of Allwinner magic
> numbers makes the code more readable.
>
> And we also have the "4.41 PHY Timing Register 0 (PTR0)" section in
> http://www.ti.com/lit/ug/spruhn7a/spruhn7a.pdf for some general
> explanations.
>
> The RK3288 is almost a perfect match, because all the hardware register
> addresses and the register names are nearly identical. The TI Keystone2
> is a lot more distant relative, so the information in its documentation
> is less trustworthy for us.
>
> The actual functionality of these bits still needs to be confirmed in
> an experimental way (instead of just making theoretical assumptions and
> hoping for the best). But again, wasting time on doing this only makes
> sense if Allwinner in fact has no plans to release proper documentation
> for the DRAM controllers used in their SoCs.
>
>

[U-Boot] [PATCH v2 5/5] sun6i: Add new CSQ_CS908 board

[Hans de Goede]

The CSQ CS908 is an A31s based top-set box, with 1G RAM, 8G NAND,
rtl8188etv usb wifi, 2 USB A receptacles (1 connected through the OTG
controller), ethernet, 3.5 mm jack with a/v out and hdmi out:

http://www.geekbuying.com/item/CS908-Allwinner-A31S-Quad-Core-1-2GHz-Android-4-4-Mini-TV-Box-HDMI-HDD-Player-1G-8G-WIFI-Miracast---Black-333395.html

Note it has no sdcard slot and therefore can only be fel booted.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ian Campbell <ijc@hellion.org.uk>
---
 configs/CSQ_CS908_defconfig | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 configs/CSQ_CS908_defconfig

diff --git a/configs/CSQ_CS908_defconfig b/configs/CSQ_CS908_defconfig
new file mode 100644
index 0000000..18fe1f3
--- /dev/null
+++ b/configs/CSQ_CS908_defconfig
@@ -0,0 +1,19 @@
+CONFIG_SPL=y
+CONFIG_SYS_EXTRA_OPTIONS="USB_EHCI"
+CONFIG_FDTFILE="sun6i-a31s-cs908.dtb"
++S:CONFIG_ARM=y
++S:CONFIG_ARCH_SUNXI=y
++S:CONFIG_MACH_SUN6I=y
++S:CONFIG_TARGET_CSQ_CS908=y
++S:CONFIG_DRAM_CLK=432
++S:CONFIG_DRAM_ZQ=123
+# Ethernet phy power
++S:CONFIG_AXP221_DLDO1_VOLT=3300
+# Wifi power
++S:CONFIG_AXP221_ALDO1_VOLT=3300
+# HDMI power ?
++S:CONFIG_AXP221_ALDO2_VOLT=1800
++S:CONFIG_AXP221_ALDO3_VOLT=3000
+# No Vbus gpio for either usb
++S:CONFIG_USB1_VBUS_PIN=""
++S:CONFIG_USB2_VBUS_PIN=""
-- 
2.1.0

[U-Boot] [PATCH v2 5/5] sun6i: Add new CSQ_CS908 board

[Ian Campbell]

On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
> The CSQ CS908 is an A31s based top-set box, with 1G RAM, 8G NAND,
> rtl8188etv usb wifi, 2 USB A receptacles (1 connected through the OTG
> controller), ethernet, 3.5 mm jack with a/v out and hdmi out:
> 
> http://www.geekbuying.com/item/CS908-Allwinner-A31S-Quad-Core-1-2GHz-Android-4-4-Mini-TV-Box-HDMI-HDD-Player-1G-8G-WIFI-Miracast---Black-333395.html
> 
> Note it has no sdcard slot and therefore can only be fel booted.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Ian Campbell <ijc@hellion.org.uk>
> ---
>  configs/CSQ_CS908_defconfig | 19 +++++++++++++++++++

Apparently I'm terrible at remembering this during review: this is
missing a MAINTAINERS entry. (MAKEALL warns about this when regenerating
boards.cfg).

Since this patch is only in u-boot-sunxi#next so far do you just want to
fold in an update to add it to your stanza (or add a new appropriate
stanza) and force push? Ack to that plan if you do.

FYI I've just force pushed a similar change adding Chen-Yu's entry to
the Hummingbird A31.

Ian.

[U-Boot] [PATCH v2 5/5] sun6i: Add new CSQ_CS908 board

[Hans de Goede]

Hi,

On 14-12-14 15:57, Ian Campbell wrote:
> On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
>> The CSQ CS908 is an A31s based top-set box, with 1G RAM, 8G NAND,
>> rtl8188etv usb wifi, 2 USB A receptacles (1 connected through the OTG
>> controller), ethernet, 3.5 mm jack with a/v out and hdmi out:
>>
>> http://www.geekbuying.com/item/CS908-Allwinner-A31S-Quad-Core-1-2GHz-Android-4-4-Mini-TV-Box-HDMI-HDD-Player-1G-8G-WIFI-Miracast---Black-333395.html
>>
>> Note it has no sdcard slot and therefore can only be fel booted.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Ian Campbell <ijc@hellion.org.uk>
>> ---
>>   configs/CSQ_CS908_defconfig | 19 +++++++++++++++++++
>
> Apparently I'm terrible at remembering this during review: this is
> missing a MAINTAINERS entry. (MAKEALL warns about this when regenerating
> boards.cfg).

Yeah, I'm bad at remembering to update MAINTAINERS too...

> Since this patch is only in u-boot-sunxi#next so far do you just want to
> fold in an update to add it to your stanza (or add a new appropriate
> stanza) and force push? Ack to that plan if you do.

Done (fixed, forced-push).

I've also fixed the same problem for the new Ippo_q8h_v1.2_defconfig in
my sun8i spl / draminit set in my personal tree.

Regards,

Hans

[U-Boot] [PATCH v2 5/5] sun6i: Add new CSQ_CS908 board

[Ian Campbell]

On Thu, 2014-12-18 at 11:39 +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-12-14 15:57, Ian Campbell wrote:
> > On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote:
> >> The CSQ CS908 is an A31s based top-set box, with 1G RAM, 8G NAND,
> >> rtl8188etv usb wifi, 2 USB A receptacles (1 connected through the OTG
> >> controller), ethernet, 3.5 mm jack with a/v out and hdmi out:
> >>
> >> http://www.geekbuying.com/item/CS908-Allwinner-A31S-Quad-Core-1-2GHz-Android-4-4-Mini-TV-Box-HDMI-HDD-Player-1G-8G-WIFI-Miracast---Black-333395.html
> >>
> >> Note it has no sdcard slot and therefore can only be fel booted.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> Acked-by: Ian Campbell <ijc@hellion.org.uk>
> >> ---
> >>   configs/CSQ_CS908_defconfig | 19 +++++++++++++++++++
> >
> > Apparently I'm terrible at remembering this during review: this is
> > missing a MAINTAINERS entry. (MAKEALL warns about this when regenerating
> > boards.cfg).
> 
> Yeah, I'm bad at remembering to update MAINTAINERS too...
> 
> > Since this patch is only in u-boot-sunxi#next so far do you just want to
> > fold in an update to add it to your stanza (or add a new appropriate
> > stanza) and force push? Ack to that plan if you do.
> 
> Done (fixed, forced-push).

Thanks.

> I've also fixed the same problem for the new Ippo_q8h_v1.2_defconfig in
> my sun8i spl / draminit set in my personal tree.

Great, that's one less to forget then!

Ian.