[U-Boot] [PATCH v3 00/10] sunxi: sun5/7i: add the psci suspend function

[U-Boot] [PATCH v3 00/10] sunxi: sun5/7i: add the psci suspend function

[Antoine Tenart]

Hi all,

This series adds an implementation of the psci suspend function for both
sun5i and sun7i. This was tested on Nextthing's CHIP and on a A20, using
cpuidle in Linux. My tests showed a power consumption reduced by nearly
a factor 2 on the CHIP when the system was idle. It went from ~1W without
cpuidle enabled to ~0.6W.

As the sun5i does not have a GIC, I needed to remove some code from the
generic psci code. To do this I added an ARM_GIC Kconfig option which
needs to be selected by each SoC having a GIC. I already added the
relevant lines for SoCs using the PSCI ARMv7 code. As this Kconfig
option is currently only used in the psci code, it should be safe to add
it even if all the SoCs having a GIC do not select it, as long as they
don't have psci functions.

On multi-core systems, we need to ensure all CPU are in psci suspend
function to switch the cpu clk source. Likewise, the first CPU to exit
the psci suspend function should restore the cpu clk source; and only
the first one. This series adds a few atomic operation (with return
support) in order to solve this problem. Please review them carefully as
I'm not sure about the implementation of such functions.

Thanks!

Antoine

Since v2:
  - Stopped putting the dram into self-refresh.
  - Removed pll2-7 disabling.
  - Added an atomic variable to only change the cpuclk source when all
    CPUs are in idle.
  - Did a similar implementation to be sure the first CPU to leave the
    idle mode switches back the cpuclk source to pll1.
  - Rebased on the latest master and updated the patches.

Since v1:
  - Rebased on the latest master and updated the patches.
  - Fixed a compile warning I introduced in virt-v7.c
  - Added the missing ARM_GIC Kconfig options.
  - Fixed a commit message (removed 'HYP').

Antoine Tenart (10):
  arm: add atomic functions with return support
  ARM: add the ARM_GIC configuration option
  sunxi: select ARM_GIC for sun[6789]i
  ARM: select ARM_GIC for SoCs having a psci implementation
  exynos: select ARM_GIC for TARGET_ARNDALE
  tegra: select ARM_GIC for Tegra TK1s
  ARM: PSCI: protect GIC specific code with ARM_GIC
  sun5/7i: add an implementation of the psci suspend function
  sun5i: add defines used by the PSCI code
  sun5i: boot in non-secure mode by default

 arch/arm/Kconfig                              |  11 +++
 arch/arm/cpu/armv7/nonsec_virt.S              |   6 ++
 arch/arm/cpu/armv7/sunxi/Makefile             |   9 ++-
 arch/arm/cpu/armv7/sunxi/psci.c               |   2 +-
 arch/arm/cpu/armv7/sunxi/psci_suspend.c       | 108 ++++++++++++++++++++++++++
 arch/arm/cpu/armv7/virt-v7.c                  |  42 ++++++----
 arch/arm/include/asm/arch-sunxi/clock_sun4i.h |   6 ++
 arch/arm/include/asm/atomic.h                 |  34 ++++++++
 arch/arm/mach-exynos/Kconfig                  |   1 +
 arch/arm/mach-tegra/tegra124/Kconfig          |   2 +
 board/sunxi/Kconfig                           |  10 +++
 include/configs/sun5i.h                       |   3 +
 12 files changed, 218 insertions(+), 16 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/sunxi/psci_suspend.c

-- 
2.10.1

[U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Antoine Tenart]

Implement three atomic functions to allow making an atomic operation
that returns the value. Adds: atomic_add_return(), atomic_sub_return(),
atomic_inc_return() and atomic_dec_return().

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/include/asm/atomic.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 171f4d979281..0d9a6e3901e2 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -46,6 +46,18 @@ static inline void atomic_add(int i, volatile atomic_t *v)
 	local_irq_restore(flags);
 }
 
+static inline int atomic_add_return(int i, volatile atomic_t *v)
+{
+	unsigned long flags = 0;
+	int ret;
+
+	local_irq_save(flags);
+	ret = (v->counter += i);
+	local_irq_restore(flags);
+
+	return ret;
+}
+
 static inline void atomic_sub(int i, volatile atomic_t *v)
 {
 	unsigned long flags = 0;
@@ -55,6 +67,18 @@ static inline void atomic_sub(int i, volatile atomic_t *v)
 	local_irq_restore(flags);
 }
 
+static inline int atomic_sub_return(int i, volatile atomic_t *v)
+{
+	unsigned long flags = 0;
+	int ret;
+
+	local_irq_save(flags);
+	ret = (v->counter -= i);
+	local_irq_restore(flags);
+
+	return ret;
+}
+
 static inline void atomic_inc(volatile atomic_t *v)
 {
 	unsigned long flags = 0;
@@ -64,6 +88,11 @@ static inline void atomic_inc(volatile atomic_t *v)
 	local_irq_restore(flags);
 }
 
+static inline int atomic_inc_return(volatile atomic_t *v)
+{
+	return atomic_add_return(1, v);
+}
+
 static inline void atomic_dec(volatile atomic_t *v)
 {
 	unsigned long flags = 0;
@@ -73,6 +102,11 @@ static inline void atomic_dec(volatile atomic_t *v)
 	local_irq_restore(flags);
 }
 
+static inline int atomic_dec_return(volatile atomic_t *v)
+{
+	return atomic_sub_return(1, v);
+}
+
 static inline int atomic_dec_and_test(volatile atomic_t *v)
 {
 	unsigned long flags = 0;
-- 
2.10.1

[U-Boot] [PATCH v3 02/10] ARM: add the ARM_GIC configuration option

[Antoine Tenart]

Some SoC does not have a GIC. Adds a configuration option to denote
this, allowing to remove code configuring the GIC when it's not
possible.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d7a9b11c766a..6a5e53280679 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -13,6 +13,9 @@ config DMA_ADDR_T_64BIT
 	bool
 	default y if ARM64
 
+config ARM_GIC
+	bool
+
 config HAS_VBAR
 	bool
 
-- 
2.10.1

[U-Boot] [PATCH v3 03/10] sunxi: select ARM_GIC for sun[6789]i

[Antoine Tenart]

Select the newly introduced ARM_GIC option to the relevant sunxi
MACH configurations.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 board/sunxi/Kconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index c0ffeb3333fc..cd8330c3944f 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -61,6 +61,7 @@ config MACH_SUN5I
 
 config MACH_SUN6I
 	bool "sun6i (Allwinner A31)"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -71,6 +72,7 @@ config MACH_SUN6I
 
 config MACH_SUN7I
 	bool "sun7i (Allwinner A20)"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -81,6 +83,7 @@ config MACH_SUN7I
 
 config MACH_SUN8I_A23
 	bool "sun8i (Allwinner A23)"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -91,6 +94,7 @@ config MACH_SUN8I_A23
 
 config MACH_SUN8I_A33
 	bool "sun8i (Allwinner A33)"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -101,12 +105,14 @@ config MACH_SUN8I_A33
 
 config MACH_SUN8I_A83T
 	bool "sun8i (Allwinner A83T)"
+	select ARM_GIC
 	select CPU_V7
 	select SUNXI_GEN_SUN6I
 	select SUPPORT_SPL
 
 config MACH_SUN8I_H3
 	bool "sun8i (Allwinner H3)"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -117,6 +123,7 @@ config MACH_SUN8I_H3
 
 config MACH_SUN9I
 	bool "sun9i (Allwinner A80)"
+	select ARM_GIC
 	select CPU_V7
 	select SUNXI_GEN_SUN6I
 
-- 
2.10.1

[U-Boot] [PATCH v3 04/10] ARM: select ARM_GIC for SoCs having a psci implementation

[Antoine Tenart]

Select the newly introduced ARM_GIC option to the relevant MACH
configurations.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 6a5e53280679..b329d3c29fce 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -316,6 +316,7 @@ config ARCH_BCM283X
 
 config TARGET_VEXPRESS_CA15_TC2
 	bool "Support vexpress_ca15_tc2"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -402,14 +403,17 @@ config TARGET_BCM23550_W1D
 
 config TARGET_BCM28155_AP
 	bool "Support bcm28155_ap"
+	select ARM_GIC
 	select CPU_V7
 
 config TARGET_BCMCYGNUS
 	bool "Support bcmcygnus"
+	select ARM_GIC
 	select CPU_V7
 
 config TARGET_BCMNSP
 	bool "Support bcmnsp"
+	select ARM_GIC
 	select CPU_V7
 
 config ARCH_EXYNOS
@@ -452,6 +456,7 @@ config ARCH_MESON
 
 config ARCH_MX7
 	bool "Freescale MX7"
+	select ARM_GIC
 	select CPU_V7
 
 config ARCH_MX6
@@ -746,6 +751,7 @@ config TARGET_LS1012AFRDM
 
 config TARGET_LS1021AQDS
 	bool "Support ls1021aqds"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -756,6 +762,7 @@ config TARGET_LS1021AQDS
 
 config TARGET_LS1021ATWR
 	bool "Support ls1021atwr"
+	select ARM_GIC
 	select CPU_V7
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
@@ -822,6 +829,7 @@ config TARGET_COLIBRI_PXA270
 
 config ARCH_UNIPHIER
 	bool "Socionext UniPhier SoCs"
+	select ARM_GIC
 	select BLK
 	select CLK_UNIPHIER
 	select DM
-- 
2.10.1

[U-Boot] [PATCH v3 05/10] exynos: select ARM_GIC for TARGET_ARNDALE

[Antoine Tenart]

Select the newly introduced ARM_GIC option to the relevant configuration
which also have a psci implementation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/mach-exynos/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index ce2a16f95b02..f976d10eaee7 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -79,6 +79,7 @@ config TARGET_ODROID_XU3
 
 config TARGET_ARNDALE
 	bool "Exynos5250 Arndale board"
+	select ARM_GIC
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
 	select SUPPORT_SPL
-- 
2.10.1

[U-Boot] [PATCH v3 06/10] tegra: select ARM_GIC for Tegra TK1s

[Antoine Tenart]

Select the newly introduced ARM_GIC option to the relevant configuration
which also have a psci implementation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/mach-tegra/tegra124/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-tegra/tegra124/Kconfig b/arch/arm/mach-tegra/tegra124/Kconfig
index df7746228386..41d75bd2f321 100644
--- a/arch/arm/mach-tegra/tegra124/Kconfig
+++ b/arch/arm/mach-tegra/tegra124/Kconfig
@@ -6,12 +6,14 @@ choice
 
 config TARGET_JETSON_TK1
 	bool "NVIDIA Tegra124 Jetson TK1 board"
+	select ARM_GIC
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
 	select ARCH_SUPPORT_PSCI
 
 config TARGET_CEI_TK1_SOM
 	bool "Colorado Engineering Inc Tegra124 TK1-som board"
+	select ARM_GIC
 	select CPU_V7_HAS_NONSEC if !SPL_BUILD
 	select CPU_V7_HAS_VIRT if !SPL_BUILD
 	help
-- 
2.10.1

[U-Boot] [PATCH v3 07/10] ARM: PSCI: protect GIC specific code with ARM_GIC

[Antoine Tenart]

Introducing the ARM_GIC configuration option, use it to only use GIC
specific code in ARM PSCI function when the SoC has a GIC.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/cpu/armv7/nonsec_virt.S |  6 ++++++
 arch/arm/cpu/armv7/virt-v7.c     | 42 ++++++++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
index 95ce9387b83e..2a11d2e83b80 100644
--- a/arch/arm/cpu/armv7/nonsec_virt.S
+++ b/arch/arm/cpu/armv7/nonsec_virt.S
@@ -164,6 +164,7 @@ ENDPROC(_smp_pen)
  * though, but we check this in C before calling this function.
  */
 ENTRY(_nonsec_init)
+#ifdef CONFIG_ARM_GIC
 	get_gicd_addr	r3
 
 	mvn	r1, #0				@ all bits to 1
@@ -175,6 +176,7 @@ ENTRY(_nonsec_init)
 	str	r1, [r3, #GICC_CTLR]		@ and clear all other bits
 	mov	r1, #0xff
 	str	r1, [r3, #GICC_PMR]		@ set priority mask register
+#endif
 
 	mrc	p15, 0, r0, c1, c1, 2
 	movw	r1, #0x3fff
@@ -200,7 +202,11 @@ ENTRY(_nonsec_init)
 	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR to secure vectors
 	isb
 
+#ifdef CONFIG_ARM_GIC
 	mov	r0, r3				@ return GICC address
+#else
+	mov	r0, #0
+#endif
 	bx	lr
 ENDPROC(_nonsec_init)
 
diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
index d33e5c61a9c2..a34305853a34 100644
--- a/arch/arm/cpu/armv7/virt-v7.c
+++ b/arch/arm/cpu/armv7/virt-v7.c
@@ -82,22 +82,11 @@ void __weak smp_kick_all_cpus(void)
 	kick_secondary_cpus_gic(gic_dist_addr);
 }
 
-__weak void psci_board_init(void)
-{
-}
-
-int armv7_init_nonsec(void)
+#ifdef CONFIG_ARM_GIC
+static int psci_gic_setup(void)
 {
-	unsigned int reg;
-	unsigned itlinesnr, i;
 	unsigned long gic_dist_addr;
-
-	/* check whether the CPU supports the security extensions */
-	reg = read_id_pfr1();
-	if ((reg & 0xF0) == 0) {
-		printf("nonsec: Security extensions not implemented.\n");
-		return -1;
-	}
+	unsigned itlinesnr, i;
 
 	/* the SCR register will be set directly in the monitor mode handler,
 	 * according to the spec one should not tinker with it in secure state
@@ -123,6 +112,31 @@ int armv7_init_nonsec(void)
 	for (i = 1; i <= itlinesnr; i++)
 		writel((unsigned)-1, gic_dist_addr + GICD_IGROUPRn + 4 * i);
 
+	return 0;
+}
+#endif
+
+__weak void psci_board_init(void)
+{
+}
+
+int armv7_init_nonsec(void)
+{
+	unsigned int reg;
+
+	/* check whether the CPU supports the security extensions */
+	reg = read_id_pfr1();
+	if ((reg & 0xF0) == 0) {
+		printf("nonsec: Security extensions not implemented.\n");
+		return -1;
+	}
+
+#ifdef CONFIG_ARM_GIC
+	int ret = psci_gic_setup();
+	if (ret)
+		return ret;
+#endif
+
 	psci_board_init();
 
 	/*
-- 
2.10.1

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Antoine Tenart]

Add the suspend psci function for sun5i and sun7i. Thus function
switches the cpu clk source to osc24M or to losc depending on the
SoC family.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/cpu/armv7/sunxi/Makefile             |   9 ++-
 arch/arm/cpu/armv7/sunxi/psci.c               |   2 +-
 arch/arm/cpu/armv7/sunxi/psci_suspend.c       | 108 ++++++++++++++++++++++++++
 arch/arm/include/asm/arch-sunxi/clock_sun4i.h |   6 ++
 4 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/sunxi/psci_suspend.c

diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
index b35b9df4a9d6..80667268a0fc 100644
--- a/arch/arm/cpu/armv7/sunxi/Makefile
+++ b/arch/arm/cpu/armv7/sunxi/Makefile
@@ -13,7 +13,14 @@ obj-$(CONFIG_MACH_SUN6I)	+= tzpc.o
 obj-$(CONFIG_MACH_SUN8I_H3)	+= tzpc.o
 
 ifndef CONFIG_SPL_BUILD
-obj-$(CONFIG_ARMV7_PSCI)	+= psci.o
+ifdef CONFIG_ARMV7_PSCI
+obj-$(CONFIG_MACH_SUN6I)	+= psci.o
+obj-$(CONFIG_MACH_SUN7I)	+= psci.o
+obj-$(CONFIG_MACH_SUN8I)	+= psci.o
+
+obj-$(CONFIG_MACH_SUN5I)	+= psci_suspend.o
+obj-$(CONFIG_MACH_SUN7I)	+= psci_suspend.o
+endif
 endif
 
 ifdef CONFIG_SPL_BUILD
diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
index 766b8c79d93d..4525bc7bf26b 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.c
+++ b/arch/arm/cpu/armv7/sunxi/psci.c
@@ -48,7 +48,7 @@ static u32 __secure cp15_read_cntp_ctl(void)
 
 #define ONE_MS (CONFIG_TIMER_CLK_FREQ / 1000)
 
-static void __secure __mdelay(u32 ms)
+void __secure __mdelay(u32 ms)
 {
 	u32 reg = ONE_MS * ms;
 
diff --git a/arch/arm/cpu/armv7/sunxi/psci_suspend.c b/arch/arm/cpu/armv7/sunxi/psci_suspend.c
new file mode 100644
index 000000000000..e5c000ff2d3d
--- /dev/null
+++ b/arch/arm/cpu/armv7/sunxi/psci_suspend.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2016 Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * Based on Allwinner code.
+ * Copyright 2007-2012 (C) Allwinner Technology Co., Ltd.
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ */
+#include <config.h>
+#include <common.h>
+
+#include <asm/atomic.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/dram.h>
+#include <asm/armv7.h>
+#include <asm/io.h>
+#include <asm/psci.h>
+#include <asm/secure.h>
+#include <asm/system.h>
+
+#include <linux/bitops.h>
+
+void __mdelay(u32);
+
+#if defined(CONFIG_MACH_SUN5I)
+#define NR_CPUS		1
+#elif defined(CONFIG_MACH_SUN7I)
+#define NR_CPUS		2
+#endif
+
+/*
+ * The PSCI suspend function switch cpuclk to another source and disable
+ * pll1. As this function is called per-CPU, it should only do this when
+ * all the CPUs are in idle state.
+ *
+ * The 'cnt' variable keeps track of the number of CPU which are in the idle
+ * state. The last one setup cpuclk for idle.
+ *
+ * The 'clk_state' varibale holds the cpu clk state (idle or normal).
+ */
+atomic_t __secure_data cnt, clk_state;
+
+#define CLK_NORMAL	0
+#define CLK_IDLE	1
+
+static void __secure sunxi_clock_enter_idle(struct sunxi_ccm_reg *ccm)
+{
+	/* switch cpuclk to osc24m */
+	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
+			CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
+
+	/* disable pll1 */
+	clrbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
+
+#ifndef CONFIG_MACH_SUN7I
+	/* switch cpuclk to losc */
+	clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
+#endif
+
+	/* disable ldo */
+	clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
+}
+
+static void __secure sunxi_clock_leave_idle(struct sunxi_ccm_reg *ccm)
+{
+	/* enable ldo */
+	setbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
+
+#ifndef CONFIG_MACH_SUN7I
+	/* switch cpuclk to osc24m */
+	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
+			CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
+#endif
+
+	/* enable pll1 */
+	setbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
+
+	/* switch cpuclk to pll1 */
+	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
+			CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT);
+}
+
+void __secure psci_cpu_suspend(void)
+{
+	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+	if (atomic_inc_return(&cnt) == NR_CPUS) {
+		/* wait for any sunxi_clock_leave_idle() to finish */
+		while (atomic_read(&clk_state) != CLK_NORMAL)
+			__mdelay(1);
+
+		sunxi_clock_enter_idle(ccm);
+		atomic_set(&clk_state, CLK_IDLE);
+	}
+
+	/* idle */
+	DSB;
+	wfi();
+
+	if (atomic_dec_return(&cnt) == NR_CPUS - 1) {
+		/* wait for any sunxi_clock_enter_idle() to finish */
+		while (atomic_read(&clk_state) != CLK_IDLE)
+			__mdelay(1);
+
+		sunxi_clock_leave_idle(ccm);
+		atomic_set(&clk_state, CLK_NORMAL);
+	}
+}
diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
index d1c5ad0a739b..0d7df5bba543 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
@@ -90,6 +90,10 @@ struct sunxi_ccm_reg {
 	u32 gmac_clk_cfg;	/* 0x164 */
 };
 
+/* osc24m_cfg bit field */
+#define OSC24M_EN		(0x1 << 0)
+#define OSC24M_LDO_EN		(0x1 << 16)
+
 /* apb1 bit field */
 #define APB1_CLK_SRC_OSC24M		(0x0 << 24)
 #define APB1_CLK_SRC_PLL6		(0x1 << 24)
@@ -208,6 +212,8 @@ struct sunxi_ccm_reg {
 #define CCM_AHB_GATE_DLL (0x1 << 15)
 #define CCM_AHB_GATE_ACE (0x1 << 16)
 
+#define CCM_PLL1_CTRL_EN		(0x1 << 31)
+
 #define CCM_PLL3_CTRL_M_SHIFT		0
 #define CCM_PLL3_CTRL_M_MASK		(0x7f << CCM_PLL3_CTRL_M_SHIFT)
 #define CCM_PLL3_CTRL_M(n)		(((n) & 0x7f) << 0)
-- 
2.10.1

[U-Boot] [PATCH v3 09/10] sun5i: add defines used by the PSCI code

[Antoine Tenart]

The sun5i SoCs can take advantage of the newly introduce PSCI suspend
function. Add defines used by the PSCI code.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 include/configs/sun5i.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/configs/sun5i.h b/include/configs/sun5i.h
index d2576599036a..86eeb847ac42 100644
--- a/include/configs/sun5i.h
+++ b/include/configs/sun5i.h
@@ -19,6 +19,9 @@
 
 #define CONFIG_SUNXI_USB_PHYS	2
 
+#define CONFIG_ARMV7_SECURE_BASE	SUNXI_SRAM_A1_BASE
+#define CONFIG_TIMER_CLK_FREQ		24000000
+
 /*
  * Include common sunxi configuration where most the settings are
  */
-- 
2.10.1

[U-Boot] [PATCH v3 10/10] sun5i: boot in non-secure mode by default

[Antoine Tenart]

sun5i now implements the psci suspend function. In order to be used by
the kernel, we should now boot in non-secure mode. Enable it by default.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 board/sunxi/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index cd8330c3944f..b330137db64d 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -56,8 +56,11 @@ config MACH_SUN4I
 config MACH_SUN5I
 	bool "sun5i (Allwinner A13)"
 	select CPU_V7
+	select CPU_V7_HAS_NONSEC
+	select ARCH_SUPPORT_PSCI
 	select SUNXI_GEN_SUN4I
 	select SUPPORT_SPL
+	select ARMV7_BOOT_SEC_DEFAULT if OLD_SUNXI_KERNEL_COMPAT
 
 config MACH_SUN6I
 	bool "sun6i (Allwinner A31)"
-- 
2.10.1

[U-Boot] [PATCH v3 09/10] sun5i: add defines used by the PSCI code

[Maxime Ripard]

Hi,

On Wed, Oct 26, 2016 at 02:10:32PM +0200, Antoine Tenart wrote:
> The sun5i SoCs can take advantage of the newly introduce PSCI suspend
> function. Add defines used by the PSCI code.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  include/configs/sun5i.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/configs/sun5i.h b/include/configs/sun5i.h
> index d2576599036a..86eeb847ac42 100644
> --- a/include/configs/sun5i.h
> +++ b/include/configs/sun5i.h
> @@ -19,6 +19,9 @@
>  
>  #define CONFIG_SUNXI_USB_PHYS	2
>  
> +#define CONFIG_ARMV7_SECURE_BASE	SUNXI_SRAM_A1_BASE
> +#define CONFIG_TIMER_CLK_FREQ		24000000

That seems wrong. AFAIK, the TIMER_CLK_FREQ is only used to set CNTFRQ
in the arch timers, and the A13 (and related) doesn't have them.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161026/8e603961/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Maxime Ripard]

Hi,

On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
> Add the suspend psci function for sun5i and sun7i. Thus function
> switches the cpu clk source to osc24M or to losc depending on the
> SoC family.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  arch/arm/cpu/armv7/sunxi/Makefile             |   9 ++-
>  arch/arm/cpu/armv7/sunxi/psci.c               |   2 +-
>  arch/arm/cpu/armv7/sunxi/psci_suspend.c       | 108 ++++++++++++++++++++++++++
>  arch/arm/include/asm/arch-sunxi/clock_sun4i.h |   6 ++
>  4 files changed, 123 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/cpu/armv7/sunxi/psci_suspend.c
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
> index b35b9df4a9d6..80667268a0fc 100644
> --- a/arch/arm/cpu/armv7/sunxi/Makefile
> +++ b/arch/arm/cpu/armv7/sunxi/Makefile
> @@ -13,7 +13,14 @@ obj-$(CONFIG_MACH_SUN6I)	+= tzpc.o
>  obj-$(CONFIG_MACH_SUN8I_H3)	+= tzpc.o
>  
>  ifndef CONFIG_SPL_BUILD
> -obj-$(CONFIG_ARMV7_PSCI)	+= psci.o
> +ifdef CONFIG_ARMV7_PSCI
> +obj-$(CONFIG_MACH_SUN6I)	+= psci.o
> +obj-$(CONFIG_MACH_SUN7I)	+= psci.o
> +obj-$(CONFIG_MACH_SUN8I)	+= psci.o
> +
> +obj-$(CONFIG_MACH_SUN5I)	+= psci_suspend.o
> +obj-$(CONFIG_MACH_SUN7I)	+= psci_suspend.o
> +endif
>  endif
>  
>  ifdef CONFIG_SPL_BUILD
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index 766b8c79d93d..4525bc7bf26b 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -48,7 +48,7 @@ static u32 __secure cp15_read_cntp_ctl(void)
>  
>  #define ONE_MS (CONFIG_TIMER_CLK_FREQ / 1000)
>  
> -static void __secure __mdelay(u32 ms)
> +void __secure __mdelay(u32 ms)
>  {
>  	u32 reg = ONE_MS * ms;
>  
> diff --git a/arch/arm/cpu/armv7/sunxi/psci_suspend.c b/arch/arm/cpu/armv7/sunxi/psci_suspend.c
> new file mode 100644
> index 000000000000..e5c000ff2d3d
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/sunxi/psci_suspend.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (C) 2016 Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * Based on Allwinner code.
> + * Copyright 2007-2012 (C) Allwinner Technology Co., Ltd.
> + *
> + * SPDX-License-Identifier:	GPL-2.0
> + */
> +#include <config.h>
> +#include <common.h>
> +
> +#include <asm/atomic.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/dram.h>
> +#include <asm/armv7.h>
> +#include <asm/io.h>
> +#include <asm/psci.h>
> +#include <asm/secure.h>
> +#include <asm/system.h>
> +
> +#include <linux/bitops.h>
> +
> +void __mdelay(u32);
> +
> +#if defined(CONFIG_MACH_SUN5I)
> +#define NR_CPUS		1
> +#elif defined(CONFIG_MACH_SUN7I)
> +#define NR_CPUS		2
> +#endif
> +
> +/*
> + * The PSCI suspend function switch cpuclk to another source and disable
> + * pll1. As this function is called per-CPU, it should only do this when
> + * all the CPUs are in idle state.
> + *
> + * The 'cnt' variable keeps track of the number of CPU which are in the idle
> + * state. The last one setup cpuclk for idle.
> + *
> + * The 'clk_state' varibale holds the cpu clk state (idle or normal).
> + */
> +atomic_t __secure_data cnt, clk_state;
> +
> +#define CLK_NORMAL	0
> +#define CLK_IDLE	1
> +
> +static void __secure sunxi_clock_enter_idle(struct sunxi_ccm_reg *ccm)
> +{
> +	/* switch cpuclk to osc24m */
> +	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
> +			CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
> +
> +	/* disable pll1 */
> +	clrbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
> +
> +#ifndef CONFIG_MACH_SUN7I
> +	/* switch cpuclk to losc */
> +	clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
> +#endif

Some kind of comment here would be nice.

> +
> +	/* disable ldo */
> +	clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);

I realise I'm the one who suggested doing this, but that might turn
out to be wrong. Have you tested devices that use the oscillator
directly, like the PWM?

> +}
> +
> +static void __secure sunxi_clock_leave_idle(struct sunxi_ccm_reg *ccm)
> +{
> +	/* enable ldo */
> +	setbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
> +
> +#ifndef CONFIG_MACH_SUN7I
> +	/* switch cpuclk to osc24m */
> +	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
> +			CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
> +#endif
> +
> +	/* enable pll1 */
> +	setbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);

It might be worth waiting for the PLL to lock by polling the bit 28
before switching the mux to it.

> +
> +	/* switch cpuclk to pll1 */
> +	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
> +			CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT);
> +}
> +
> +void __secure psci_cpu_suspend(void)
> +{
> +	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +
> +	if (atomic_inc_return(&cnt) == NR_CPUS) {
> +		/* wait for any sunxi_clock_leave_idle() to finish */
> +		while (atomic_read(&clk_state) != CLK_NORMAL)
> +			__mdelay(1);
> +
> +		sunxi_clock_enter_idle(ccm);
> +		atomic_set(&clk_state, CLK_IDLE);
> +	}
> +
> +	/* idle */
> +	DSB;
> +	wfi();
> +
> +	if (atomic_dec_return(&cnt) == NR_CPUS - 1) {
> +		/* wait for any sunxi_clock_enter_idle() to finish */
> +		while (atomic_read(&clk_state) != CLK_IDLE)
> +			__mdelay(1);
> +
> +		sunxi_clock_leave_idle(ccm);
> +		atomic_set(&clk_state, CLK_NORMAL);
> +	}
> +}
> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> index d1c5ad0a739b..0d7df5bba543 100644
> --- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> @@ -90,6 +90,10 @@ struct sunxi_ccm_reg {
>  	u32 gmac_clk_cfg;	/* 0x164 */
>  };
>  
> +/* osc24m_cfg bit field */
> +#define OSC24M_EN		(0x1 << 0)
> +#define OSC24M_LDO_EN		(0x1 << 16)
> +
>  /* apb1 bit field */
>  #define APB1_CLK_SRC_OSC24M		(0x0 << 24)
>  #define APB1_CLK_SRC_PLL6		(0x1 << 24)
> @@ -208,6 +212,8 @@ struct sunxi_ccm_reg {
>  #define CCM_AHB_GATE_DLL (0x1 << 15)
>  #define CCM_AHB_GATE_ACE (0x1 << 16)
>  
> +#define CCM_PLL1_CTRL_EN		(0x1 << 31)
> +
>  #define CCM_PLL3_CTRL_M_SHIFT		0
>  #define CCM_PLL3_CTRL_M_MASK		(0x7f << CCM_PLL3_CTRL_M_SHIFT)
>  #define CCM_PLL3_CTRL_M(n)		(((n) & 0x7f) << 0)
> -- 
> 2.10.1

Looks good otherwise,
Thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161026/6deb8447/attachment.sig>

[U-Boot] [PATCH v3 06/10] tegra: select ARM_GIC for Tegra TK1s

[Stephen Warren]

On 10/26/2016 06:10 AM, Antoine Tenart wrote:
> Select the newly introduced ARM_GIC option to the relevant configuration
> which also have a psci implementation.

This doesn't look right; all Tegras have a GIC, so it's not a 
board-specific option. Perhaps TEGRA_COMMON or TEGRA_ARMV[78]_COMMON 
should select this?

[U-Boot] [PATCH v3 06/10] tegra: select ARM_GIC for Tegra TK1s

[Antoine Tenart]

Hello,

On Wed, Oct 26, 2016 at 08:55:27AM -0600, Stephen Warren wrote:
> On 10/26/2016 06:10 AM, Antoine Tenart wrote:
> > Select the newly introduced ARM_GIC option to the relevant configuration
> > which also have a psci implementation.
> 
> This doesn't look right; all Tegras have a GIC, so it's not a board-specific
> option. Perhaps TEGRA_COMMON or TEGRA_ARMV[78]_COMMON should select this?

Oops, I wanted to do this for the v3, but apparently forgot. Sorry.

Which one do you want me to use (TEGRA_COMMON vs TEGRA_ARMV[78]_COMMON)?

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161026/e2eb9cd5/attachment.sig>

[U-Boot] [PATCH v3 06/10] tegra: select ARM_GIC for Tegra TK1s

[Stephen Warren]

On 10/26/2016 08:59 AM, Antoine Tenart wrote:
> Hello,
>
> On Wed, Oct 26, 2016 at 08:55:27AM -0600, Stephen Warren wrote:
>> On 10/26/2016 06:10 AM, Antoine Tenart wrote:
>>> Select the newly introduced ARM_GIC option to the relevant configuration
>>> which also have a psci implementation.
>>
>> This doesn't look right; all Tegras have a GIC, so it's not a board-specific
>> option. Perhaps TEGRA_COMMON or TEGRA_ARMV[78]_COMMON should select this?
>
> Oops, I wanted to do this for the v3, but apparently forgot. Sorry.
>
> Which one do you want me to use (TEGRA_COMMON vs TEGRA_ARMV[78]_COMMON)?

Assuming the code enabled by CONFIG_ARM_GIC doesn't have any 
dependencies, then CONFIG_TEGRA_COMMON is probably the best place since 
it applies to all Tegra systems. I'd suggest "select ARM_GIC if XXX" 
where "XXX" is something like PSCI being enabled (or whatever other 
functionality needs ARM_GIC), to make sure we don't bring in any new 
code or behaviour for boards that don't require it.

[U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Mark Rutland]

Hi,

On Wed, Oct 26, 2016 at 02:10:24PM +0200, Antoine Tenart wrote:
> Implement three atomic functions to allow making an atomic operation
> that returns the value. Adds: atomic_add_return(), atomic_sub_return(),
> atomic_inc_return() and atomic_dec_return().
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

In the cover letter, you mentioned that these are needed for SMP
systems (for managing common suspend entry/exit management).

The below operations are *not* atomic in SMP systems, and can only work
in UP. The same is true of all the existing code in the same file.

> +static inline int atomic_add_return(int i, volatile atomic_t *v)
> +{
> +	unsigned long flags = 0;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	ret = (v->counter += i);
> +	local_irq_restore(flags);
> +
> +	return ret;
> +}

local_irq_{save,restore}() won't serialize two CPUs. Consider two CPUs
executing this in parallel (assuming the compiler chooses a register rX
as a temporary):

	CPU0				CPU1

	local_irq_save()		local_irq_save()
	rX = v->counter			rX = v->counter
	rX += i				rX += i
	v->counter = rX
					v->counter = rX
	local_irq_restore()		local_irq_restore()

At the end of this, CPU0's increment of v->counter is lost.

If you need atomics on SMP, you'll need to use the
{load,store}-exclusive instructions, which come with a number of
additional requirements (e.g. the memory being operated on must be
mapped as write-back cacheable, the entire retry loop needs to be
writtten in asm, etc).

Thanks,
Mark.

[U-Boot] [PATCH v3 09/10] sun5i: add defines used by the PSCI code

[Antoine Tenart]

Hi,

On Wed, Oct 26, 2016 at 02:34:30PM +0200, Maxime Ripard wrote:
> On Wed, Oct 26, 2016 at 02:10:32PM +0200, Antoine Tenart wrote:
> > The sun5i SoCs can take advantage of the newly introduce PSCI suspend
> > function. Add defines used by the PSCI code.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> >  include/configs/sun5i.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/configs/sun5i.h b/include/configs/sun5i.h
> > index d2576599036a..86eeb847ac42 100644
> > --- a/include/configs/sun5i.h
> > +++ b/include/configs/sun5i.h
> > @@ -19,6 +19,9 @@
> >  
> >  #define CONFIG_SUNXI_USB_PHYS	2
> >  
> > +#define CONFIG_ARMV7_SECURE_BASE	SUNXI_SRAM_A1_BASE
> > +#define CONFIG_TIMER_CLK_FREQ		24000000
> 
> That seems wrong. AFAIK, the TIMER_CLK_FREQ is only used to set CNTFRQ
> in the arch timers, and the A13 (and related) doesn't have them.

That's right the A13 doesn't have one. I'll remove this define then.

Thanks!

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/fb239ac0/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Antoine Tenart]

Hi,

On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
> On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
> > +
> > +#ifndef CONFIG_MACH_SUN7I
> > +	/* switch cpuclk to losc */
> > +	clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
> > +#endif
> 
> Some kind of comment here would be nice.

That's based on my experiments, switching the cpu clk to losc wasn't
working (the board hanged). I agree that's not the best explanation
ever...

> > +
> > +	/* disable ldo */
> > +	clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
> 
> I realise I'm the one who suggested doing this, but that might turn
> out to be wrong. Have you tested devices that use the oscillator
> directly, like the PWM?

No, and good point.

> > +static void __secure sunxi_clock_leave_idle(struct sunxi_ccm_reg *ccm)
> > +{
> > +	/* enable ldo */
> > +	setbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
> > +
> > +#ifndef CONFIG_MACH_SUN7I
> > +	/* switch cpuclk to osc24m */
> > +	clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
> > +			CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
> > +#endif
> > +
> > +	/* enable pll1 */
> > +	setbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
> 
> It might be worth waiting for the PLL to lock by polling the bit 28
> before switching the mux to it.

I was doing this before, but it turned out this wasn't necessary to have
the psci suspend function working. It would probably be safer though.

> Looks good otherwise,

Thanks for the review!

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/c8c162d6/attachment.sig>

[U-Boot] [PATCH v3 06/10] tegra: select ARM_GIC for Tegra TK1s

[Antoine Tenart]

On Wed, Oct 26, 2016 at 09:01:59AM -0600, Stephen Warren wrote:
> On 10/26/2016 08:59 AM, Antoine Tenart wrote:
> > On Wed, Oct 26, 2016 at 08:55:27AM -0600, Stephen Warren wrote:
> > > On 10/26/2016 06:10 AM, Antoine Tenart wrote:
> > > > Select the newly introduced ARM_GIC option to the relevant configuration
> > > > which also have a psci implementation.
> > > 
> > > This doesn't look right; all Tegras have a GIC, so it's not a board-specific
> > > option. Perhaps TEGRA_COMMON or TEGRA_ARMV[78]_COMMON should select this?
> > 
> > Oops, I wanted to do this for the v3, but apparently forgot. Sorry.
> > 
> > Which one do you want me to use (TEGRA_COMMON vs TEGRA_ARMV[78]_COMMON)?
> 
> Assuming the code enabled by CONFIG_ARM_GIC doesn't have any dependencies,
> then CONFIG_TEGRA_COMMON is probably the best place since it applies to all
> Tegra systems. I'd suggest "select ARM_GIC if XXX" where "XXX" is something
> like PSCI being enabled (or whatever other functionality needs ARM_GIC), to
> make sure we don't bring in any new code or behaviour for boards that don't
> require it.

This configuration option currently only protect some portions of code,
so there's no dependency involved. I'll select ARM_GIC from
CONFIG_TEGRA_COMMON, but without "where XXXX" as this does not brings
new code.

Thanks!

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/76540a53/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Maxime Ripard]

Hi,

On Thu, Oct 27, 2016 at 03:10:58PM +0200, Antoine Tenart wrote:
> Hi,
> 
> On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
> > On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
> > > +
> > > +#ifndef CONFIG_MACH_SUN7I
> > > +	/* switch cpuclk to losc */
> > > +	clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
> > > +#endif
> > 
> > Some kind of comment here would be nice.
> 
> That's based on my experiments, switching the cpu clk to losc wasn't
> working (the board hanged). I agree that's not the best explanation
> ever...

Still, even if that's only to say that it was based on experiments and
you don't really know why. It's definitely not obvious to anyone, so
it deserves a comment to explain why you did it that way.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/1c135fb1/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Chen-Yu Tsai]

On Thu, Oct 27, 2016 at 9:10 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hi,
>
> On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
>> On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
>> > +
>> > +#ifndef CONFIG_MACH_SUN7I
>> > +   /* switch cpuclk to losc */
>> > +   clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
>> > +#endif
>>
>> Some kind of comment here would be nice.
>
> That's based on my experiments, switching the cpu clk to losc wasn't
> working (the board hanged). I agree that's not the best explanation
> ever...
>
>> > +
>> > +   /* disable ldo */
>> > +   clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
>>
>> I realise I'm the one who suggested doing this, but that might turn
>> out to be wrong. Have you tested devices that use the oscillator
>> directly, like the PWM?
>
> No, and good point.

I think the sun4i-timer and ARM arch timer are also clocked from OSC24M,
so turning it off is really not a good idea.

ChenYu

>> > +static void __secure sunxi_clock_leave_idle(struct sunxi_ccm_reg *ccm)
>> > +{
>> > +   /* enable ldo */
>> > +   setbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
>> > +
>> > +#ifndef CONFIG_MACH_SUN7I
>> > +   /* switch cpuclk to osc24m */
>> > +   clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
>> > +                   CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
>> > +#endif
>> > +
>> > +   /* enable pll1 */
>> > +   setbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
>>
>> It might be worth waiting for the PLL to lock by polling the bit 28
>> before switching the mux to it.
>
> I was doing this before, but it turned out this wasn't necessary to have
> the psci suspend function working. It would probably be safer though.
>
>> Looks good otherwise,
>
> Thanks for the review!
>
> Antoine
>
> --
> Antoine T?nart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Antoine Tenart]

HI Mark,

On Wed, Oct 26, 2016 at 04:14:31PM +0100, Mark Rutland wrote:
> On Wed, Oct 26, 2016 at 02:10:24PM +0200, Antoine Tenart wrote:
> > Implement three atomic functions to allow making an atomic operation
> > that returns the value. Adds: atomic_add_return(), atomic_sub_return(),
> > atomic_inc_return() and atomic_dec_return().
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> 
> In the cover letter, you mentioned that these are needed for SMP
> systems (for managing common suspend entry/exit management).
> 
> The below operations are *not* atomic in SMP systems, and can only work
> in UP. The same is true of all the existing code in the same file.

That's what I feared...

> > +static inline int atomic_add_return(int i, volatile atomic_t *v)
> > +{
> > +	unsigned long flags = 0;
> > +	int ret;
> > +
> > +	local_irq_save(flags);
> > +	ret = (v->counter += i);
> > +	local_irq_restore(flags);
> > +
> > +	return ret;
> > +}
> 
> local_irq_{save,restore}() won't serialize two CPUs. Consider two CPUs
> executing this in parallel (assuming the compiler chooses a register rX
> as a temporary):
> 
> 	CPU0				CPU1
> 
> 	local_irq_save()		local_irq_save()
> 	rX = v->counter			rX = v->counter
> 	rX += i				rX += i
> 	v->counter = rX
> 					v->counter = rX
> 	local_irq_restore()		local_irq_restore()
> 
> At the end of this, CPU0's increment of v->counter is lost.
> 
> If you need atomics on SMP, you'll need to use the
> {load,store}-exclusive instructions, which come with a number of
> additional requirements (e.g. the memory being operated on must be
> mapped as write-back cacheable, the entire retry loop needs to be
> writtten in asm, etc).

Thanks a lot for the review and for the explanation.

So I need to do something like what's done in the kernel, at
http://lxr.free-electrons.com/source/arch/arm/include/asm/atomic.h#L60
for ARM; by using ldrex and strex.

Thanks,

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/baec227b/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Antoine Tenart]

On Thu, Oct 27, 2016 at 03:20:02PM +0200, Maxime Ripard wrote:
> On Thu, Oct 27, 2016 at 03:10:58PM +0200, Antoine Tenart wrote:
> > On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
> > > On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
> > > > +
> > > > +#ifndef CONFIG_MACH_SUN7I
> > > > +	/* switch cpuclk to losc */
> > > > +	clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
> > > > +#endif
> > > 
> > > Some kind of comment here would be nice.
> > 
> > That's based on my experiments, switching the cpu clk to losc wasn't
> > working (the board hanged). I agree that's not the best explanation
> > ever...
> 
> Still, even if that's only to say that it was based on experiments and
> you don't really know why. It's definitely not obvious to anyone, so
> it deserves a comment to explain why you did it that way.

Sure, I wasn't arguing about the need of a comment :)

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/aec3c7ea/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Antoine Tenart]

Hi,

On Thu, Oct 27, 2016 at 09:21:10PM +0800, Chen-Yu Tsai wrote:
> On Thu, Oct 27, 2016 at 9:10 PM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> > On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
> >> On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
> >
> >> > +
> >> > +   /* disable ldo */
> >> > +   clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
> >>
> >> I realise I'm the one who suggested doing this, but that might turn
> >> out to be wrong. Have you tested devices that use the oscillator
> >> directly, like the PWM?
> >
> > No, and good point.
> 
> I think the sun4i-timer and ARM arch timer are also clocked from OSC24M,
> so turning it off is really not a good idea.

You're right, sun4i-timer is clocked from osc24m. Turning off LDO doesn't
turn off OSC24M, but that might disturb it if I understood correctly.
Anyway I won't disable LDO in v4.

Thanks!

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/e9631f5f/attachment.sig>

[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Chen-Yu Tsai]

On Thu, Oct 27, 2016 at 9:41 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hi,
>
> On Thu, Oct 27, 2016 at 09:21:10PM +0800, Chen-Yu Tsai wrote:
>> On Thu, Oct 27, 2016 at 9:10 PM, Antoine Tenart
>> <antoine.tenart@free-electrons.com> wrote:
>> > On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
>> >> On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
>> >
>> >> > +
>> >> > +   /* disable ldo */
>> >> > +   clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
>> >>
>> >> I realise I'm the one who suggested doing this, but that might turn
>> >> out to be wrong. Have you tested devices that use the oscillator
>> >> directly, like the PWM?
>> >
>> > No, and good point.
>>
>> I think the sun4i-timer and ARM arch timer are also clocked from OSC24M,
>> so turning it off is really not a good idea.
>
> You're right, sun4i-timer is clocked from osc24m. Turning off LDO doesn't
> turn off OSC24M, but that might disturb it if I understood correctly.
> Anyway I won't disable LDO in v4.

That makes me wonder what the LDO is actually for...

ChenYu

>
> Thanks!
>
> Antoine
>
> --
> Antoine T?nart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com