[PATCH 0/2] socfpga: fix hotplug/add suspend to ram

[PATCH 0/2] socfpga: fix hotplug/add suspend to ram

[atull]

From: Alan Tull <atull@opensource.altera.com>

patch 1: socfpga: hotplug: put cpu1 in wfi
  Fix how cpu1 is hotplugged to prevent increaced power
  consumption.

patch 2: socfpga: support suspend to ram
  * Initialize ocram
  * Add a function to do suspend to ram and
    place DDR in self-refresh
  * This function lives in ocram

Alan Tull (2):
  socfpga: hotplug: put cpu1 in wfi
  socfpga: support suspend to ram

 arch/arm/mach-socfpga/Makefile       |    1 +
 arch/arm/mach-socfpga/core.h         |    6 ++
 arch/arm/mach-socfpga/platsmp.c      |   12 ++-
 arch/arm/mach-socfpga/pm.c           |  141 ++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/self-refresh.S |  148 ++++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/socfpga.c      |   10 +++
 6 files changed, 315 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-socfpga/pm.c
 create mode 100644 arch/arm/mach-socfpga/self-refresh.S

-- 
1.7.9.5

[PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[atull]

From: Alan Tull <atull@opensource.altera.com>

Use WFI when putting CPU1 to sleep.  Don't hold CPU1 in reset
since that results in increased power consumption.

Reset CPU1 briefly during CPU1 bootup.

This has been tested for hotplug and suspend/resume and results
in no increased power consumption.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 arch/arm/mach-socfpga/core.h    |    2 ++
 arch/arm/mach-socfpga/platsmp.c |   12 +++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
index 572b8f7..c4a0929 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -28,6 +28,8 @@
 #define RSTMGR_CTRL_SWCOLDRSTREQ	0x1	/* Cold Reset */
 #define RSTMGR_CTRL_SWWARMRSTREQ	0x2	/* Warm Reset */
 
+#define RSTMGR_MPUMODRST_CPU1		0x2     /*CPU1 Reset*/
+
 extern void socfpga_secondary_startup(void);
 extern void __iomem *socfpga_scu_base_addr;
 
diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
index 5356a72..1d5f8ad 100644
--- a/arch/arm/mach-socfpga/platsmp.c
+++ b/arch/arm/mach-socfpga/platsmp.c
@@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	int trampoline_size = &secondary_trampoline_end - &secondary_trampoline;
 
 	if (cpu1start_addr) {
+		/* This will put CPU #1 into reset.*/
+		__raw_writel(RSTMGR_MPUMODRST_CPU1,
+			     rst_manager_base_addr + 0x10);
+
 		memcpy(phys_to_virt(0), &secondary_trampoline, trampoline_size);
 
 		__raw_writel(virt_to_phys(socfpga_secondary_startup),
@@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus)
  */
 static void socfpga_cpu_die(unsigned int cpu)
 {
-	cpu_do_idle();
+	/* Flush the L1 data cache. */
+	flush_cache_all();
 
-	/* We should have never returned from idle */
-	panic("cpu %d unexpectedly exit from shutdown\n", cpu);
+	/* Do WFI. If we wake up early, go back into WFI */
+	while (1)
+		cpu_do_idle();
 }
 
 struct smp_operations socfpga_smp_ops __initdata = {
-- 
1.7.9.5

[PATCH 2/2] socfpga: support suspend to ram

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add code that requests that the sdr controller go into
self-refresh mode.  This code is run from ocram.

This patch assumes that u-boot has already configured sdr:
  sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
  sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
  sdr.ctrlcfg.dramtiming4.selfrfshexit = 512

How to suspend to ram:
 $ echo enabled > \
/sys/devices/soc/ffc02000.serial0/tty/ttyS0/power/wakeup

 $ echo -n mem > /sys/power/state

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 arch/arm/mach-socfpga/Makefile       |    1 +
 arch/arm/mach-socfpga/core.h         |    4 +
 arch/arm/mach-socfpga/pm.c           |  141 ++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/self-refresh.S |  148 ++++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/socfpga.c      |   10 +++
 5 files changed, 304 insertions(+)
 create mode 100644 arch/arm/mach-socfpga/pm.c
 create mode 100644 arch/arm/mach-socfpga/self-refresh.S

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 6dd7a93..0591927 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -4,3 +4,4 @@
 
 obj-y					:= socfpga.o
 obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
+obj-$(CONFIG_SUSPEND)	+= pm.o self-refresh.o
diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
index c4a0929..cc1a2fb 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -38,6 +38,7 @@ extern void socfpga_sysmgr_init(void);
 
 extern void __iomem *sys_manager_base_addr;
 extern void __iomem *rst_manager_base_addr;
+extern void __iomem *sdr_ctl_base_addr;
 
 extern struct smp_operations socfpga_smp_ops;
 extern char secondary_trampoline, secondary_trampoline_end;
@@ -46,4 +47,7 @@ extern unsigned long cpu1start_addr;
 
 #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
 
+u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
+extern unsigned int socfpga_sdram_self_refresh_sz;
+
 #endif
diff --git a/arch/arm/mach-socfpga/pm.c b/arch/arm/mach-socfpga/pm.c
new file mode 100644
index 0000000..02c3719
--- /dev/null
+++ b/arch/arm/mach-socfpga/pm.c
@@ -0,0 +1,141 @@
+/*
+ *  arch/arm/mach-socfpga/pm.c
+ *
+ * Copyright (C) 2014 Altera Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitops.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of_platform.h>
+#include <linux/suspend.h>
+#include <asm/suspend.h>
+#include <asm/fncpy.h>
+#include "core.h"
+
+/* Pointer to function copied to ocram */
+static u32 (*socfpga_sdram_self_refresh_in_ocram)(u32 sdr_base, u32 scu_base);
+
+/* Round up a pointer address to fix aligment for fncpy() */
+static void *fncpy_align(void *ptr)
+{
+	u32 value = (u32)ptr;
+
+	if ((value & (FNCPY_ALIGN - 1)) != 0)
+		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
+
+	return (void *)value;
+}
+
+static void *socfpga_init_ocram_exec(void)
+{
+	struct device_node *np;
+	const __be32 *prop;
+	u32 ocram_hwaddr, len;
+	void __iomem  *iomem_exec_ocram;
+	size_t size;
+
+	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
+		return 0;
+	}
+
+	/* Determine the OCRAM address and size */
+	prop = of_get_property(np, "reg", &size);
+	ocram_hwaddr = be32_to_cpup(prop++);
+	len = be32_to_cpup(prop);
+
+	if (!prop || size < sizeof(*prop)) {
+		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
+		return 0;
+	}
+
+	iomem_exec_ocram = __arm_ioremap_exec(ocram_hwaddr, len, 0);
+
+	/* Fix alignment to work with fncpy */
+	iomem_exec_ocram = fncpy_align(iomem_exec_ocram);
+
+	return iomem_exec_ocram;
+}
+
+static int socfpga_setup_ocram_self_refresh(void)
+{
+	void *ocram_addr;
+
+	/* Configure ocram and make it executable */
+	ocram_addr = socfpga_init_ocram_exec();
+	WARN(!ocram_addr, "Unable to initialize ocram for pm");
+	if (!ocram_addr)
+		return -EFAULT;
+
+	/* Copy the code that puts DDR in self refresh to ocram */
+	socfpga_sdram_self_refresh_in_ocram =
+		(void *)fncpy((void *)ocram_addr,
+		      &socfpga_sdram_self_refresh,
+		      socfpga_sdram_self_refresh_sz);
+
+	WARN(!socfpga_sdram_self_refresh_in_ocram,
+	     "could not copy function to ocram");
+	if (!socfpga_sdram_self_refresh_in_ocram)
+		return -EFAULT;
+
+	return 0;
+}
+
+static int socfpga_pm_suspend(unsigned long arg)
+{
+	u32 ret;
+
+	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
+						  (u32)socfpga_scu_base_addr);
+
+	pr_debug("%s self-refresh loops request=%d exit=%d\n", __func__,
+		 ret & 0xffff, (ret >> 16) & 0xffff);
+
+	return 0;
+}
+
+static int socfpga_pm_enter(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+	case PM_SUSPEND_MEM:
+		outer_disable();
+		cpu_suspend(0, socfpga_pm_suspend);
+		outer_resume();
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static const struct platform_suspend_ops socfpga_pm_ops = {
+	.valid	= suspend_valid_only_mem,
+	.enter	= socfpga_pm_enter,
+};
+
+static int __init socfpga_pm_init(void)
+{
+	if (socfpga_setup_ocram_self_refresh())
+		return -EFAULT;
+
+	suspend_set_ops(&socfpga_pm_ops);
+	pr_info("SoCFPGA initialized for DDR self-refresh during suspend.\n");
+
+	return 0;
+}
+arch_initcall(socfpga_pm_init);
diff --git a/arch/arm/mach-socfpga/self-refresh.S b/arch/arm/mach-socfpga/self-refresh.S
new file mode 100644
index 0000000..a58b58f
--- /dev/null
+++ b/arch/arm/mach-socfpga/self-refresh.S
@@ -0,0 +1,148 @@
+/*
+ * Copyright (C) 2014 Altera Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+
+#define SOCFPGA_SCU_CTRL_OFFS	0
+#define SCU_STANDBY_ENA		0x20
+#define MAX_LOOP_COUNT		1000
+
+/* Register offset */
+#define SDR_CTRLGRP_LOWPWREQ_ADDR       0x54
+#define SDR_CTRLGRP_LOWPWRACK_ADDR      0x58
+
+/* Bitfield positions */
+#define SELFRSHREQ_POS                  3
+#define SELFRSHREQ_MASK                 0x8
+
+#define SELFRFSHACK_POS                 1
+#define SELFRFSHACK_MASK                0x2
+
+	.arch   armv7-a
+	.text
+	.align 3
+
+	/*
+	 * socfpga_sdram_self_refresh
+	 *
+	 *  r0 : sdr_ctl_base_addr
+	 *  r1 : socfpga_scu_base_addr
+	 *  r2 : temp storage of register values
+	 *  r3 : loop counter
+	 *  r4 : temp storage of return value
+	 *
+	 *  return value: lower 16 bits: loop count going into self refresh
+	 *                upper 16 bits: loop count exiting self refresh
+	 */
+ENTRY(socfpga_sdram_self_refresh)
+	stmfd	sp!, {r4}
+
+	/*
+	 * Enable SCU (snoop) standby mode.
+	 *
+	 * From the ARM Cortex-A9 MPCore Technical Reference Manual:
+	 * "When set, SCU CLK is turned off when all processors are in WFI
+	 *  mode, there is no pending request on the ACP (if implemented), and
+	 *  there is no remaining activity in the SCU."
+	 */
+	ldr	r2, [r1, #SOCFPGA_SCU_CTRL_OFFS]
+	orr	r2, r2, #SCU_STANDBY_ENA
+	str	r2, [r1, #SOCFPGA_SCU_CTRL_OFFS]
+
+	/* Enable dynamic clock gating in the Power Control Register. */
+	mrc	p15, 0, r2, c15, c0, 0
+	orr	r2, r2, #1
+	mcr	p15, 0, r2, c15, c0, 0
+
+	/* Enable self refresh: set sdr.ctrlgrp.lowpwreq.selfrshreq = 1 */
+	ldr	r2, [r0, #SDR_CTRLGRP_LOWPWREQ_ADDR]
+	orr	r2, r2, #SELFRSHREQ_MASK
+	str	r2, [r0, #SDR_CTRLGRP_LOWPWREQ_ADDR]
+
+	/* Poll until sdr.ctrlgrp.lowpwrack.selfrfshack == 1 or hit max loops */
+	mov	r3, #0
+while_ack_0:
+	ldr	r2, [r0, #SDR_CTRLGRP_LOWPWRACK_ADDR]
+	and	r2, r2, #SELFRFSHACK_MASK
+	cmp	r2, #SELFRFSHACK_MASK
+	beq	ack_1
+
+	add	r3, #1
+	cmp	r3, #MAX_LOOP_COUNT
+	bne	while_ack_0
+
+ack_1:
+	mov	r4, r3
+
+	/*
+	 * Execute an ISB instruction to ensure that all of the
+	 * CP15 register changes have been committed.
+	 */
+	isb
+
+	/*
+	 * Execute a barrier instruction to ensure that all cache,
+	 * TLB and branch predictor maintenance operations issued
+	 * by any CPU in the cluster have completed.
+	 */
+	dsb
+	dmb
+
+	wfi
+
+	/* Disable self-refresh: set sdr.ctrlgrp.lowpwreq.selfrshreq = 0 */
+	ldr	r2, [r0, #SDR_CTRLGRP_LOWPWREQ_ADDR]
+	bic	r2, r2, #SELFRSHREQ_MASK
+	str	r2, [r0, #SDR_CTRLGRP_LOWPWREQ_ADDR]
+
+	/* Poll until sdr.ctrlgrp.lowpwrack.selfrfshack == 0 or hit max loops */
+	mov	r3, #0
+while_ack_1:
+	ldr	r2, [r0, #SDR_CTRLGRP_LOWPWRACK_ADDR]
+	and	r2, r2, #SELFRFSHACK_MASK
+	cmp	r2, #SELFRFSHACK_MASK
+	bne	ack_0
+
+	add	r3, #1
+	cmp	r3, #MAX_LOOP_COUNT
+	bne	while_ack_1
+
+ack_0:
+	/*
+	 * Prepare return value:
+	 * Shift loop count for exiting self refresh into upper 16 bits.
+	 * Leave loop count for requesting self refresh in lower 16 bits.
+	 */
+	mov	r3, r3, lsl #16
+	add	r4, r4, r3
+
+	/* Disable dynamic clock gating in the Power Control Register. */
+	mrc	p15, 0, r2, c15, c0, 0
+	bic	r2, r2, #1
+	mcr	p15, 0, r2, c15, c0, 0
+
+	/* Disable SCU standby mode */
+	ldr	r2, [r1, #SOCFPGA_SCU_CTRL_OFFS]
+	bic	r2, r2, #SCU_STANDBY_ENA
+	str	r2, [r1, #SOCFPGA_SCU_CTRL_OFFS]
+
+	mov     r0, r4                  @ return value
+	ldmfd	sp!, {r4}
+	bx	lr			@ return
+
+ENDPROC(socfpga_sdram_self_refresh)
+ENTRY(socfpga_sdram_self_refresh_sz)
+	.word	. - socfpga_sdram_self_refresh
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index adbf383..34b4cc5 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -30,6 +30,7 @@ void __iomem *socfpga_scu_base_addr = ((void __iomem *)(SOCFPGA_SCU_VIRT_BASE));
 void __iomem *sys_manager_base_addr;
 void __iomem *rst_manager_base_addr;
 unsigned long cpu1start_addr;
+void __iomem *sdr_ctl_base_addr;
 
 static struct map_desc scu_io_desc __initdata = {
 	.virtual	= SOCFPGA_SCU_VIRT_BASE,
@@ -77,6 +78,15 @@ void __init socfpga_sysmgr_init(void)
 
 	np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");
 	rst_manager_base_addr = of_iomap(np, 0);
+
+	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
+		return;
+	}
+
+	sdr_ctl_base_addr = of_iomap(np, 0);
+	WARN_ON(!sdr_ctl_base_addr);
 }
 
 static void __init socfpga_init_irq(void)
-- 
1.7.9.5

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Russell King - ARM Linux]

On Wed, Sep 24, 2014 at 03:27:28PM -0500, atull@opensource.altera.com wrote:
> diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
> index 5356a72..1d5f8ad 100644
> --- a/arch/arm/mach-socfpga/platsmp.c
> +++ b/arch/arm/mach-socfpga/platsmp.c
> @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	int trampoline_size = &secondary_trampoline_end - &secondary_trampoline;
>  
>  	if (cpu1start_addr) {
> +		/* This will put CPU #1 into reset.*/
> +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> +			     rst_manager_base_addr + 0x10);

If you can place CPU1 into reset, then why not place it into reset during
hot unplug?

> @@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus)
>   */
>  static void socfpga_cpu_die(unsigned int cpu)
>  {
> -	cpu_do_idle();
> +	/* Flush the L1 data cache. */
> +	flush_cache_all();

Why do you think that's necessary?

This potentially flushes *all* levels of the cache, including L2, which
is not a nice thing to do if you have another CPU running.  Secondly,
the core code has already called flush_cache_louis() _twice_ for you
immediately prior to calling your cpu_die() function explicitly to
remove any L1 data.

The only data which should remain are speculative prefetches and stack
data specific to _this_ CPU (which could include dirty cache lines
associated with the stack frame to enter your cpu_die function.)
None of these cache lines are of any interest to other CPUs in the
system, so there's no need for them to be written back prior to the
CPU being reset or powered down for hot unplug.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2] socfpga: support suspend to ram

[Steffen Trumtrar]

Hi!

On Wed, Sep 24, 2014 at 03:27:29PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add code that requests that the sdr controller go into
> self-refresh mode.  This code is run from ocram.
> 
> This patch assumes that u-boot has already configured sdr:
>   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
>   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
>   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512
> 
> How to suspend to ram:
>  $ echo enabled > \
> /sys/devices/soc/ffc02000.serial0/tty/ttyS0/power/wakeup
> 
>  $ echo -n mem > /sys/power/state
> 

(...)

Never looked into that, so maybe a stupid question:
What happens if the bootloader (u-boot or other) didn't configure the sdr?
Will it "just" not wake up again?

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[atull]

On Wed, 24 Sep 2014, Russell King - ARM Linux wrote:

Hi Russell,

> On Wed, Sep 24, 2014 at 03:27:28PM -0500, atull@opensource.altera.com wrote:
> > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
> > index 5356a72..1d5f8ad 100644
> > --- a/arch/arm/mach-socfpga/platsmp.c
> > +++ b/arch/arm/mach-socfpga/platsmp.c
> > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle)
> >  	int trampoline_size = &secondary_trampoline_end - &secondary_trampoline;
> >  
> >  	if (cpu1start_addr) {
> > +		/* This will put CPU #1 into reset.*/
> > +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> > +			     rst_manager_base_addr + 0x10);
> 
> If you can place CPU1 into reset, then why not place it into reset during
> hot unplug?

It's a chip weirdness.  We can reset CPU1 briefly, but if we leave it in
reset, it results in power consumption going up.

> 
> > @@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus)
> >   */
> >  static void socfpga_cpu_die(unsigned int cpu)
> >  {
> > -	cpu_do_idle();
> > +	/* Flush the L1 data cache. */
> > +	flush_cache_all();
> 
> Why do you think that's necessary?
> 
> This potentially flushes *all* levels of the cache, including L2, which
> is not a nice thing to do if you have another CPU running.  Secondly,
> the core code has already called flush_cache_louis() _twice_ for you
> immediately prior to calling your cpu_die() function explicitly to
> remove any L1 data.
> 
> The only data which should remain are speculative prefetches and stack
> data specific to _this_ CPU (which could include dirty cache lines
> associated with the stack frame to enter your cpu_die function.)
> None of these cache lines are of any interest to other CPUs in the
> system, so there's no need for them to be written back prior to the
> CPU being reset or powered down for hot unplug.
> 

Then it's clear we don't need that.  I'll take it out in v2.

Thanks for the review!

Alan

Re: [PATCH 2/2] socfpga: support suspend to ram

[atull]

Hi Steffen,

On Thu, 25 Sep 2014, Steffen Trumtrar wrote:

> Hi!
> 
> On Wed, Sep 24, 2014 at 03:27:29PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add code that requests that the sdr controller go into
> > self-refresh mode.  This code is run from ocram.
> > 
> > This patch assumes that u-boot has already configured sdr:
> >   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
> >   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
> >   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512
> > 
> > How to suspend to ram:
> >  $ echo enabled > \
> > /sys/devices/soc/ffc02000.serial0/tty/ttyS0/power/wakeup
> > 
> >  $ echo -n mem > /sys/power/state
> > 
> 
> (...)
> 
> Never looked into that, so maybe a stupid question:
> What happens if the bootloader (u-boot or other) didn't configure the sdr?
> Will it "just" not wake up again?

Waking up won't be the problem.  Linux won't boot.

Alternatively, if this Linux kernel is paired with a version of the
bootloader that does most of the initializion, but not the settings
that we want here, I expect there could be issues.

I don't want to duplicate the u-boot sdr configuration code, instead
I document my assumptions here here.  For future generations who take
this patch.  If they have a problem with s2r, they will look through
the git logs and find this helpful note of what their bootloader
was supposed to do.

Alan

> 
> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [PATCH 2/2] socfpga: support suspend to ram

[Dinh Nguyen]

On 09/24/2014 03:27 PM, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add code that requests that the sdr controller go into
> self-refresh mode.  This code is run from ocram.
> 
> This patch assumes that u-boot has already configured sdr:
>   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
>   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
>   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512
> 
> How to suspend to ram:
>  $ echo enabled > \
> /sys/devices/soc/ffc02000.serial0/tty/ttyS0/power/wakeup
> 
>  $ echo -n mem > /sys/power/state
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
>  arch/arm/mach-socfpga/Makefile       |    1 +
>  arch/arm/mach-socfpga/core.h         |    4 +
>  arch/arm/mach-socfpga/pm.c           |  141 ++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/self-refresh.S |  148 ++++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/socfpga.c      |   10 +++
>  5 files changed, 304 insertions(+)
>  create mode 100644 arch/arm/mach-socfpga/pm.c
>  create mode 100644 arch/arm/mach-socfpga/self-refresh.S
> 
> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
> index 6dd7a93..0591927 100644
> --- a/arch/arm/mach-socfpga/Makefile
> +++ b/arch/arm/mach-socfpga/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-y					:= socfpga.o
>  obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
> +obj-$(CONFIG_SUSPEND)	+= pm.o self-refresh.o
> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> index c4a0929..cc1a2fb 100644
> --- a/arch/arm/mach-socfpga/core.h
> +++ b/arch/arm/mach-socfpga/core.h
> @@ -38,6 +38,7 @@ extern void socfpga_sysmgr_init(void);
>  
>  extern void __iomem *sys_manager_base_addr;
>  extern void __iomem *rst_manager_base_addr;
> +extern void __iomem *sdr_ctl_base_addr;
>  
>  extern struct smp_operations socfpga_smp_ops;
>  extern char secondary_trampoline, secondary_trampoline_end;
> @@ -46,4 +47,7 @@ extern unsigned long cpu1start_addr;
>  
>  #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
>  
> +u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
> +extern unsigned int socfpga_sdram_self_refresh_sz;
> +
>  #endif
> diff --git a/arch/arm/mach-socfpga/pm.c b/arch/arm/mach-socfpga/pm.c
> new file mode 100644
> index 0000000..02c3719
> --- /dev/null
> +++ b/arch/arm/mach-socfpga/pm.c
> @@ -0,0 +1,141 @@
> +/*
> + *  arch/arm/mach-socfpga/pm.c
> + *
> + * Copyright (C) 2014 Altera Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/suspend.h>
> +#include <asm/suspend.h>
> +#include <asm/fncpy.h>
> +#include "core.h"
> +
> +/* Pointer to function copied to ocram */
> +static u32 (*socfpga_sdram_self_refresh_in_ocram)(u32 sdr_base, u32 scu_base);
> +
> +/* Round up a pointer address to fix aligment for fncpy() */
> +static void *fncpy_align(void *ptr)
> +{
> +	u32 value = (u32)ptr;
> +
> +	if ((value & (FNCPY_ALIGN - 1)) != 0)
> +		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
> +
> +	return (void *)value;
> +}
> +
> +static void *socfpga_init_ocram_exec(void)
> +{
> +	struct device_node *np;
> +	const __be32 *prop;
> +	u32 ocram_hwaddr, len;
> +	void __iomem  *iomem_exec_ocram;
> +	size_t size;
> +
> +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
> +		return 0;
> +	}
> +
> +	/* Determine the OCRAM address and size */
> +	prop = of_get_property(np, "reg", &size);
> +	ocram_hwaddr = be32_to_cpup(prop++);
> +	len = be32_to_cpup(prop);
> +
> +	if (!prop || size < sizeof(*prop)) {
> +		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
> +		return 0;
> +	}
> +
> +	iomem_exec_ocram = __arm_ioremap_exec(ocram_hwaddr, len, 0);

The call to __arm_ioremap_exec can fail. Also, this doesn't seem right,
what if a good chunk of OCRAM has been used by some other driver in the
system? From what I've seen, OCRAM should be using the generic allocator
framework.

> +
> +	/* Fix alignment to work with fncpy */
> +	iomem_exec_ocram = fncpy_align(iomem_exec_ocram);
> +
> +	return iomem_exec_ocram;
> +}
> +
> +static int socfpga_setup_ocram_self_refresh(void)
> +{
> +	void *ocram_addr;
> +
> +	/* Configure ocram and make it executable */
> +	ocram_addr = socfpga_init_ocram_exec();
> +	WARN(!ocram_addr, "Unable to initialize ocram for pm");
> +	if (!ocram_addr)
> +		return -EFAULT;
> +
> +	/* Copy the code that puts DDR in self refresh to ocram */
> +	socfpga_sdram_self_refresh_in_ocram =
> +		(void *)fncpy((void *)ocram_addr,
> +		      &socfpga_sdram_self_refresh,
> +		      socfpga_sdram_self_refresh_sz);
> +
> +	WARN(!socfpga_sdram_self_refresh_in_ocram,
> +	     "could not copy function to ocram");
> +	if (!socfpga_sdram_self_refresh_in_ocram)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int socfpga_pm_suspend(unsigned long arg)
> +{
> +	u32 ret;
> +
> +	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
> +						  (u32)socfpga_scu_base_addr);

What if sdr_ctl_base_addr is not valid?

BR,
Dinh

Re: [PATCH 2/2] socfpga: support suspend to ram

[atull]

On Fri, 26 Sep 2014, Dinh Nguyen wrote:

> On 09/24/2014 03:27 PM, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add code that requests that the sdr controller go into
> > self-refresh mode.  This code is run from ocram.
> > 
> > This patch assumes that u-boot has already configured sdr:
> >   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
> >   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
> >   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512
> > 
> > How to suspend to ram:
> >  $ echo enabled > \
> > /sys/devices/soc/ffc02000.serial0/tty/ttyS0/power/wakeup
> > 
> >  $ echo -n mem > /sys/power/state
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> >  arch/arm/mach-socfpga/Makefile       |    1 +
> >  arch/arm/mach-socfpga/core.h         |    4 +
> >  arch/arm/mach-socfpga/pm.c           |  141 ++++++++++++++++++++++++++++++++
> >  arch/arm/mach-socfpga/self-refresh.S |  148 ++++++++++++++++++++++++++++++++++
> >  arch/arm/mach-socfpga/socfpga.c      |   10 +++
> >  5 files changed, 304 insertions(+)
> >  create mode 100644 arch/arm/mach-socfpga/pm.c
> >  create mode 100644 arch/arm/mach-socfpga/self-refresh.S
> > 
> > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
> > index 6dd7a93..0591927 100644
> > --- a/arch/arm/mach-socfpga/Makefile
> > +++ b/arch/arm/mach-socfpga/Makefile
> > @@ -4,3 +4,4 @@
> >  
> >  obj-y					:= socfpga.o
> >  obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
> > +obj-$(CONFIG_SUSPEND)	+= pm.o self-refresh.o
> > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> > index c4a0929..cc1a2fb 100644
> > --- a/arch/arm/mach-socfpga/core.h
> > +++ b/arch/arm/mach-socfpga/core.h
> > @@ -38,6 +38,7 @@ extern void socfpga_sysmgr_init(void);
> >  
> >  extern void __iomem *sys_manager_base_addr;
> >  extern void __iomem *rst_manager_base_addr;
> > +extern void __iomem *sdr_ctl_base_addr;
> >  
> >  extern struct smp_operations socfpga_smp_ops;
> >  extern char secondary_trampoline, secondary_trampoline_end;
> > @@ -46,4 +47,7 @@ extern unsigned long cpu1start_addr;
> >  
> >  #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
> >  
> > +u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
> > +extern unsigned int socfpga_sdram_self_refresh_sz;
> > +
> >  #endif
> > diff --git a/arch/arm/mach-socfpga/pm.c b/arch/arm/mach-socfpga/pm.c
> > new file mode 100644
> > index 0000000..02c3719
> > --- /dev/null
> > +++ b/arch/arm/mach-socfpga/pm.c
> > @@ -0,0 +1,141 @@
> > +/*
> > + *  arch/arm/mach-socfpga/pm.c
> > + *
> > + * Copyright (C) 2014 Altera Corporation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/suspend.h>
> > +#include <asm/suspend.h>
> > +#include <asm/fncpy.h>
> > +#include "core.h"
> > +
> > +/* Pointer to function copied to ocram */
> > +static u32 (*socfpga_sdram_self_refresh_in_ocram)(u32 sdr_base, u32 scu_base);
> > +
> > +/* Round up a pointer address to fix aligment for fncpy() */
> > +static void *fncpy_align(void *ptr)
> > +{
> > +	u32 value = (u32)ptr;
> > +
> > +	if ((value & (FNCPY_ALIGN - 1)) != 0)
> > +		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
> > +
> > +	return (void *)value;
> > +}
> > +
> > +static void *socfpga_init_ocram_exec(void)
> > +{
> > +	struct device_node *np;
> > +	const __be32 *prop;
> > +	u32 ocram_hwaddr, len;
> > +	void __iomem  *iomem_exec_ocram;
> > +	size_t size;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
> > +	if (!np) {
> > +		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
> > +		return 0;
> > +	}
> > +
> > +	/* Determine the OCRAM address and size */
> > +	prop = of_get_property(np, "reg", &size);
> > +	ocram_hwaddr = be32_to_cpup(prop++);
> > +	len = be32_to_cpup(prop);
> > +
> > +	if (!prop || size < sizeof(*prop)) {
> > +		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
> > +		return 0;
> > +	}
> > +
> > +	iomem_exec_ocram = __arm_ioremap_exec(ocram_hwaddr, len, 0);
> 
> The call to __arm_ioremap_exec can fail. Also, this doesn't seem right,
> what if a good chunk of OCRAM has been used by some other driver in the
> system? From what I've seen, OCRAM should be using the generic allocator
> framework.

Good catch.  I will check for this faiure in v2.  Also, yes, use the generic
allocator framework.

> 
> > +
> > +	/* Fix alignment to work with fncpy */
> > +	iomem_exec_ocram = fncpy_align(iomem_exec_ocram);
> > +
> > +	return iomem_exec_ocram;
> > +}
> > +
> > +static int socfpga_setup_ocram_self_refresh(void)
> > +{
> > +	void *ocram_addr;
> > +
> > +	/* Configure ocram and make it executable */
> > +	ocram_addr = socfpga_init_ocram_exec();
> > +	WARN(!ocram_addr, "Unable to initialize ocram for pm");
> > +	if (!ocram_addr)
> > +		return -EFAULT;
> > +
> > +	/* Copy the code that puts DDR in self refresh to ocram */
> > +	socfpga_sdram_self_refresh_in_ocram =
> > +		(void *)fncpy((void *)ocram_addr,
> > +		      &socfpga_sdram_self_refresh,
> > +		      socfpga_sdram_self_refresh_sz);
> > +
> > +	WARN(!socfpga_sdram_self_refresh_in_ocram,
> > +	     "could not copy function to ocram");
> > +	if (!socfpga_sdram_self_refresh_in_ocram)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int socfpga_pm_suspend(unsigned long arg)
> > +{
> > +	u32 ret;
> > +
> > +	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
> > +						  (u32)socfpga_scu_base_addr);
> 
> What if sdr_ctl_base_addr is not valid?

I will check for NULL in v2.

Alan

> 
> BR,
> Dinh
> 
> 

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Pavel Machek]

Hi!

> From: Alan Tull <atull@opensource.altera.com>
> 
> Use WFI when putting CPU1 to sleep.  Don't hold CPU1 in reset
> since that results in increased power consumption.
> 
> Reset CPU1 briefly during CPU1 bootup.
> 
> This has been tested for hotplug and suspend/resume and results
> in no increased power consumption.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
>  arch/arm/mach-socfpga/core.h    |    2 ++
>  arch/arm/mach-socfpga/platsmp.c |   12 +++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> index 572b8f7..c4a0929 100644
> --- a/arch/arm/mach-socfpga/core.h
> +++ b/arch/arm/mach-socfpga/core.h
> @@ -28,6 +28,8 @@
>  #define RSTMGR_CTRL_SWCOLDRSTREQ	0x1	/* Cold Reset */
>  #define RSTMGR_CTRL_SWWARMRSTREQ	0x2	/* Warm Reset */
>  
> +#define RSTMGR_MPUMODRST_CPU1		0x2     /*CPU1 Reset*/
> +

It would be nice to have space after /* and before */.

> diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
> index 5356a72..1d5f8ad 100644
> --- a/arch/arm/mach-socfpga/platsmp.c
> +++ b/arch/arm/mach-socfpga/platsmp.c
> @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	int trampoline_size = &secondary_trampoline_end - &secondary_trampoline;
>  
>  	if (cpu1start_addr) {
> +		/* This will put CPU #1 into reset.*/

Same here.

> +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> +			     rst_manager_base_addr + 0x10);

Would it be possible to copy reset manager description struct from
u-boot and use it here, instead of raw offset?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] socfpga: support suspend to ram

[Pavel Machek]

Hi!

> Add code that requests that the sdr controller go into
> self-refresh mode.  This code is run from ocram.
> 
> This patch assumes that u-boot has already configured sdr:
>   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
>   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
>   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512

I'm not sure if we should make assumptions like that. u-boot is not
the only bootloader.

At the very least, it should go to comment in the code, not to changelog.

> +u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
> +extern unsigned int socfpga_sdram_self_refresh_sz;

_sz -> size.

Is it ok to just copy code around?

> +/* Round up a pointer address to fix aligment for fncpy() */
> +static void *fncpy_align(void *ptr)
> +{
> +	u32 value = (u32)ptr;
> +
> +	if ((value & (FNCPY_ALIGN - 1)) != 0)
> +		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
> +
> +	return (void *)value;
> +}

Don't we have a nice macro doing aligning?

I guess the if() is not neccessary.

> +static int socfpga_pm_suspend(unsigned long arg)
> +{
> +	u32 ret;
> +
> +	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
> +						  (u32)socfpga_scu_base_addr);
> +
> +	pr_debug("%s self-refresh loops request=%d exit=%d\n", __func__,
> +		 ret & 0xffff, (ret >> 16) & 0xffff);
> +
> +	return 0;
> +}

return ret?


> +	.arch   armv7-a
> +	.text
> +	.align 3
> +
> +	/*
> +	 * socfpga_sdram_self_refresh
> +	 *
> +	 *  r0 : sdr_ctl_base_addr
> +	 *  r1 : socfpga_scu_base_addr
> +	 *  r2 : temp storage of register values
> +	 *  r3 : loop counter
> +	 *  r4 : temp storage of return value
> +	 *
> +	 *  return value: lower 16 bits: loop count going into self refresh
> +	 *                upper 16 bits: loop count exiting self refresh
> +	 */
> +ENTRY(socfpga_sdram_self_refresh)

r0, r1 are the parameters?

> @@ -77,6 +78,15 @@ void __init socfpga_sysmgr_init(void)
>  
>  	np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");
>  	rst_manager_base_addr = of_iomap(np, 0);
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
> +		return;
> +	}
> +
> +	sdr_ctl_base_addr = of_iomap(np, 0);
> +	WARN_ON(!sdr_ctl_base_addr);
>  }
>  
>  static void __init socfpga_init_irq(void)

Actually, "sdr-ctl" is quite hard to understand. I guess it means
"sdram-control"? Should we do something like altr,sdram-ctrl-1.0, so
that we have way forward if hardware changes in future?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[atull]

On Wed, 1 Oct 2014, Pavel Machek wrote:

> Hi!
> 
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Use WFI when putting CPU1 to sleep.  Don't hold CPU1 in reset
> > since that results in increased power consumption.
> > 
> > Reset CPU1 briefly during CPU1 bootup.
> > 
> > This has been tested for hotplug and suspend/resume and results
> > in no increased power consumption.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> >  arch/arm/mach-socfpga/core.h    |    2 ++
> >  arch/arm/mach-socfpga/platsmp.c |   12 +++++++++---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> > index 572b8f7..c4a0929 100644
> > --- a/arch/arm/mach-socfpga/core.h
> > +++ b/arch/arm/mach-socfpga/core.h
> > @@ -28,6 +28,8 @@
> >  #define RSTMGR_CTRL_SWCOLDRSTREQ	0x1	/* Cold Reset */
> >  #define RSTMGR_CTRL_SWWARMRSTREQ	0x2	/* Warm Reset */
> >  
> > +#define RSTMGR_MPUMODRST_CPU1		0x2     /*CPU1 Reset*/
> > +
> 
> It would be nice to have space after /* and before */.

Hi Pavel,

I will fix the comment space here and the other place you pointed out.

> 
> > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
> > index 5356a72..1d5f8ad 100644
> > --- a/arch/arm/mach-socfpga/platsmp.c
> > +++ b/arch/arm/mach-socfpga/platsmp.c
> > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle)
> >  	int trampoline_size = &secondary_trampoline_end - &secondary_trampoline;
> >  
> >  	if (cpu1start_addr) {
> > +		/* This will put CPU #1 into reset.*/
> 
> Same here.
> 
> > +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> > +			     rst_manager_base_addr + 0x10);
> 
> Would it be possible to copy reset manager description struct from
> u-boot and use it here, instead of raw offset?

I will replace this 0x10 with a macro that reflects how the register is 
named in the register map.

Thanks for the review!

Alan

> 								Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Pavel Machek]

Hi!

> > > +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> > > +			     rst_manager_base_addr + 0x10);
> > 
> > Would it be possible to copy reset manager description struct from
> > u-boot and use it here, instead of raw offset?
> 
> I will replace this 0x10 with a macro that reflects how the register is 
> named in the register map.

That would be better than 0x10, but even better would be just copying

struct socfpga_reset_manager {
        u32     status;
        u32     ctrl;
        u32     counts;
        u32     padding1;
        u32     mpu_mod_reset;
        u32     per_mod_reset;
        u32     per2_mod_reset;
        u32     brg_mod_reset;
};

from u-boot. Unlike macros, structs have advantages that typos lead to
easier-to-see failure modes... (And they are easier to read/parse,
too).

Thanks,
									Pavel 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Dinh Nguyen]

On 10/1/14, 10:04 AM, Pavel Machek wrote:
> Hi!
> 
>>>> +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
>>>> +			     rst_manager_base_addr + 0x10);
>>>
>>> Would it be possible to copy reset manager description struct from
>>> u-boot and use it here, instead of raw offset?
>>
>> I will replace this 0x10 with a macro that reflects how the register is 
>> named in the register map.
> 
> That would be better than 0x10, but even better would be just copying
> 
> struct socfpga_reset_manager {
>         u32     status;
>         u32     ctrl;
>         u32     counts;
>         u32     padding1;
>         u32     mpu_mod_reset;
>         u32     per_mod_reset;
>         u32     per2_mod_reset;
>         u32     brg_mod_reset;
> };
> 
> from u-boot. Unlike macros, structs have advantages that typos lead to
> easier-to-see failure modes... (And they are easier to read/parse,
> too).
> 

Copying from uboot sounds good, but I already know that the CPU reset
offset is different for our next SOC, Arria 10. The Arria 10 SOC should
still be able to use the same MSL as Cyclone5 and Arria5, but with a few
differences. One of them being, the CPU1 reset offset is at 0x20 instead
of 0x10. So I think having a macro for this one register is a bit
cleaner than having to define a whole new struct for Arria10.

if (of_machine_is_compatible("altr,socfpga-arria10"))
	__raw_writel(0, rst_manager_base_addr + 0x20);
else
	__raw_writel(0, rst_manager_base_addr + 0x10);

Dinh

Re: [PATCH 2/2] socfpga: support suspend to ram

[atull]

On Wed, 1 Oct 2014, Pavel Machek wrote:

> Hi!
> 
> > Add code that requests that the sdr controller go into
> > self-refresh mode.  This code is run from ocram.
> > 
> > This patch assumes that u-boot has already configured sdr:
> >   sdr.ctrlcfg.lowpwreq.selfrfshmask = 3
> >   sdr.ctrlcfg.lowpwrtiming.clkdisablecycles = 8
> >   sdr.ctrlcfg.dramtiming4.selfrfshexit = 512
> 
> I'm not sure if we should make assumptions like that. u-boot is not
> the only bootloader.

Yes, that's why I wanted to document it.

> 
> At the very least, it should go to comment in the code, not to changelog.
> 

I agree.  I'm about to post the next version.  I will add this in
the code.

> > +u32 socfpga_sdram_self_refresh(u32 sdr_base, u32 scu_base);
> > +extern unsigned int socfpga_sdram_self_refresh_sz;
> 
> _sz -> size.
> 
> Is it ok to just copy code around?
> 

You have to use fncpy to do it.

> > +/* Round up a pointer address to fix aligment for fncpy() */
> > +static void *fncpy_align(void *ptr)
> > +{
> > +	u32 value = (u32)ptr;
> > +
> > +	if ((value & (FNCPY_ALIGN - 1)) != 0)
> > +		value = ((value & ~(FNCPY_ALIGN - 1)) + FNCPY_ALIGN);
> > +
> > +	return (void *)value;
> > +}
> 
> Don't we have a nice macro doing aligning?

Actually the next version is going to fix some of these other comments by 
using the ocram sram driver to allocate ocram space.  

> 
> I guess the if() is not neccessary.
> 
> > +static int socfpga_pm_suspend(unsigned long arg)
> > +{
> > +	u32 ret;
> > +
> > +	ret = socfpga_sdram_self_refresh_in_ocram((u32)sdr_ctl_base_addr,
> > +						  (u32)socfpga_scu_base_addr);
> > +
> > +	pr_debug("%s self-refresh loops request=%d exit=%d\n", __func__,
> > +		 ret & 0xffff, (ret >> 16) & 0xffff);
> > +
> > +	return 0;
> > +}
> 
> return ret?
> 
> 
> > +	.arch   armv7-a
> > +	.text
> > +	.align 3
> > +
> > +	/*
> > +	 * socfpga_sdram_self_refresh
> > +	 *
> > +	 *  r0 : sdr_ctl_base_addr
> > +	 *  r1 : socfpga_scu_base_addr
> > +	 *  r2 : temp storage of register values
> > +	 *  r3 : loop counter
> > +	 *  r4 : temp storage of return value
> > +	 *
> > +	 *  return value: lower 16 bits: loop count going into self refresh
> > +	 *                upper 16 bits: loop count exiting self refresh
> > +	 */
> > +ENTRY(socfpga_sdram_self_refresh)
> 
> r0, r1 are the parameters?
> 
> > @@ -77,6 +78,15 @@ void __init socfpga_sysmgr_init(void)
> >  
> >  	np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");
> >  	rst_manager_base_addr = of_iomap(np, 0);
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
> > +	if (!np) {
> > +		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
> > +		return;
> > +	}
> > +
> > +	sdr_ctl_base_addr = of_iomap(np, 0);
> > +	WARN_ON(!sdr_ctl_base_addr);
> >  }
> >  
> >  static void __init socfpga_init_irq(void)
> 
> Actually, "sdr-ctl" is quite hard to understand. I guess it means
> "sdram-control"? Should we do something like altr,sdram-ctrl-1.0, so
> that we have way forward if hardware changes in future?
> 
> 									Pavel
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Pavel Machek]

On Wed 2014-10-01 11:07:18, Dinh Nguyen wrote:
> 
> 
> On 10/1/14, 10:04 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
> >>>> +			     rst_manager_base_addr + 0x10);
> >>>
> >>> Would it be possible to copy reset manager description struct from
> >>> u-boot and use it here, instead of raw offset?
> >>
> >> I will replace this 0x10 with a macro that reflects how the register is 
> >> named in the register map.
> > 
> > That would be better than 0x10, but even better would be just copying
> > 
> > struct socfpga_reset_manager {
> >         u32     status;
> >         u32     ctrl;
> >         u32     counts;
> >         u32     padding1;
> >         u32     mpu_mod_reset;
> >         u32     per_mod_reset;
> >         u32     per2_mod_reset;
> >         u32     brg_mod_reset;
> > };
> > 
> > from u-boot. Unlike macros, structs have advantages that typos lead to
> > easier-to-see failure modes... (And they are easier to read/parse,
> > too).
> > 
> 
> Copying from uboot sounds good, but I already know that the CPU reset
> offset is different for our next SOC, Arria 10. The Arria 10 SOC should
> still be able to use the same MSL as Cyclone5 and Arria5, but with a few
> differences. One of them being, the CPU1 reset offset is at 0x20 instead
> of 0x10. So I think having a macro for this one register is a bit
> cleaner than having to define a whole new struct for Arria10.

I don't think "whole new struct" is a problem. At least it will be
plain to see what changed (which will get easily lost in ifdefs.

struct cyclone5_reset_manager {
	struct socfpga_reset_manager common;
	u32 brg_mod_reset;
}

struct aria10_reset_manager {
	struct socfpga_reset_manager common;
	char filler[0x10];
	u32 brg_mod_reset;
}

if (of_machine_is_compatible("altr,socfpga-arria10"))
	__raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset));
else
	__raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset));

...does not sound that bad. (And you'll need some nice solution for
u-boot, anyway...)

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Dinh Nguyen]

On 10/1/14, 6:16 PM, Pavel Machek wrote:
> On Wed 2014-10-01 11:07:18, Dinh Nguyen wrote:
>>
>>
>> On 10/1/14, 10:04 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> +		__raw_writel(RSTMGR_MPUMODRST_CPU1,
>>>>>> +			     rst_manager_base_addr + 0x10);
>>>>>
>>>>> Would it be possible to copy reset manager description struct from
>>>>> u-boot and use it here, instead of raw offset?
>>>>
>>>> I will replace this 0x10 with a macro that reflects how the register is 
>>>> named in the register map.
>>>
>>> That would be better than 0x10, but even better would be just copying
>>>
>>> struct socfpga_reset_manager {
>>>         u32     status;
>>>         u32     ctrl;
>>>         u32     counts;
>>>         u32     padding1;
>>>         u32     mpu_mod_reset;
>>>         u32     per_mod_reset;
>>>         u32     per2_mod_reset;
>>>         u32     brg_mod_reset;
>>> };
>>>
>>> from u-boot. Unlike macros, structs have advantages that typos lead to
>>> easier-to-see failure modes... (And they are easier to read/parse,
>>> too).
>>>
>>
>> Copying from uboot sounds good, but I already know that the CPU reset
>> offset is different for our next SOC, Arria 10. The Arria 10 SOC should
>> still be able to use the same MSL as Cyclone5 and Arria5, but with a few
>> differences. One of them being, the CPU1 reset offset is at 0x20 instead
>> of 0x10. So I think having a macro for this one register is a bit
>> cleaner than having to define a whole new struct for Arria10.
> 
> I don't think "whole new struct" is a problem. At least it will be
> plain to see what changed (which will get easily lost in ifdefs.
> 
> struct cyclone5_reset_manager {
> 	struct socfpga_reset_manager common;
> 	u32 brg_mod_reset;
> }
> 
> struct aria10_reset_manager {
> 	struct socfpga_reset_manager common;
> 	char filler[0x10];
> 	u32 brg_mod_reset;
> }
> 
> if (of_machine_is_compatible("altr,socfpga-arria10"))
> 	__raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> else
> 	__raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> 
> ...does not sound that bad. (And you'll need some nice solution for
> u-boot, anyway...)
> 

That's fine.

Dinh

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[Arnd Bergmann]

On Thursday 02 October 2014 01:16:46 Pavel Machek wrote:
> > > 
> > > struct socfpga_reset_manager {
> > >         u32     status;
> > >         u32     ctrl;
> > >         u32     counts;
> > >         u32     padding1;
> > >         u32     mpu_mod_reset;
> > >         u32     per_mod_reset;
> > >         u32     per2_mod_reset;
> > >         u32     brg_mod_reset;
> > > };
> > > 
> > > from u-boot. Unlike macros, structs have advantages that typos lead to
> > > easier-to-see failure modes... (And they are easier to read/parse,
> > > too).
> > > 
> > 
> > Copying from uboot sounds good, but I already know that the CPU reset
> > offset is different for our next SOC, Arria 10. The Arria 10 SOC should
> > still be able to use the same MSL as Cyclone5 and Arria5, but with a few
> > differences. One of them being, the CPU1 reset offset is at 0x20 instead
> > of 0x10. So I think having a macro for this one register is a bit
> > cleaner than having to define a whole new struct for Arria10.
> 
> I don't think "whole new struct" is a problem. At least it will be
> plain to see what changed (which will get easily lost in ifdefs.
> 
> struct cyclone5_reset_manager {
>         struct socfpga_reset_manager common;
>         u32 brg_mod_reset;
> }
> 
> struct aria10_reset_manager {
>         struct socfpga_reset_manager common;
>         char filler[0x10];
>         u32 brg_mod_reset;
> }
> 
> if (of_machine_is_compatible("altr,socfpga-arria10"))
>         __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> else
>         __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> 
> ...does not sound that bad. (And you'll need some nice solution for
> u-boot, anyway...)

I think it would be better to just add more fields and access a different
field based on the SoC type than cast the structs around.

Also, never use __raw_writel unless you know exactly what you are doing.
This should use writel, or possibly writel_relaxed.

Finally, don't sprinkle of_machine_is_compatible() checks all over the
place. Make the decision once when you initially probe the machine.

	Arnd

Re: [PATCH 1/2] socfpga: hotplug: put cpu1 in wfi

[atull]

On Thu, 2 Oct 2014, Arnd Bergmann wrote:

> On Thursday 02 October 2014 01:16:46 Pavel Machek wrote:
> > > > 
> > > > struct socfpga_reset_manager {
> > > >         u32     status;
> > > >         u32     ctrl;
> > > >         u32     counts;
> > > >         u32     padding1;
> > > >         u32     mpu_mod_reset;
> > > >         u32     per_mod_reset;
> > > >         u32     per2_mod_reset;
> > > >         u32     brg_mod_reset;
> > > > };
> > > > 
> > > > from u-boot. Unlike macros, structs have advantages that typos lead to
> > > > easier-to-see failure modes... (And they are easier to read/parse,
> > > > too).
> > > > 
> > > 
> > > Copying from uboot sounds good, but I already know that the CPU reset
> > > offset is different for our next SOC, Arria 10. The Arria 10 SOC should
> > > still be able to use the same MSL as Cyclone5 and Arria5, but with a few
> > > differences. One of them being, the CPU1 reset offset is at 0x20 instead
> > > of 0x10. So I think having a macro for this one register is a bit
> > > cleaner than having to define a whole new struct for Arria10.
> > 
> > I don't think "whole new struct" is a problem. At least it will be
> > plain to see what changed (which will get easily lost in ifdefs.
> > 
> > struct cyclone5_reset_manager {
> >         struct socfpga_reset_manager common;
> >         u32 brg_mod_reset;
> > }
> > 
> > struct aria10_reset_manager {
> >         struct socfpga_reset_manager common;
> >         char filler[0x10];
> >         u32 brg_mod_reset;
> > }
> > 
> > if (of_machine_is_compatible("altr,socfpga-arria10"))
> >         __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> > else
> >         __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset));
> > 
> > ...does not sound that bad. (And you'll need some nice solution for
> > u-boot, anyway...)
> 
> I think it would be better to just add more fields and access a different
> field based on the SoC type than cast the structs around.
> 
> Also, never use __raw_writel unless you know exactly what you are doing.
> This should use writel, or possibly writel_relaxed.

Arnd, Pavel,

I appreciate the comments.

I will fix this to not be a __raw_writel.

> 
> Finally, don't sprinkle of_machine_is_compatible() checks all over the
> place. Make the decision once when you initially probe the machine.
> 
> 	Arnd
> 

The changes for aria10 are minor: a different DT plus two register changes.  
I'm not introducing aria10 support in this patch.  This is a 16 line patch
for fixing something in an established machine layer.  If I have to come up
with a new scheme for accessing registers, then I will need to touch other
code that this patch does not intend to change.

Alan