[PATCH 0/5] ARM: sunxi: Machine code cleanup

[PATCH 0/5] ARM: sunxi: Machine code cleanup

[Maxime Ripard]

Hi,

This serie moves the restart code out of the mach-sunxi directory to
either the watchdog driver or to a new driver in drivers/power/reset.

Since the reset code was pretty much all the code left in the
mach-sunxi directory for all the SoCs but the A31, we also can remove
the whole machine definitions, and use the generic machine.

Thanks,
Maxime

Maxime Ripard (5):
  wdt: sunxi: Move restart code to the watchdog driver
  power: reset: Add Allwinner A31 reset code
  ARM: sunxi: Select restart drivers
  ARM: sunxi: Remove reset code from the platform
  ARM: sunxi: Remove sun4i and sun7i machine definitions

 arch/arm/mach-sunxi/Kconfig        |   4 ++
 arch/arm/mach-sunxi/sunxi.c        | 127 -------------------------------------
 drivers/power/reset/Kconfig        |   7 ++
 drivers/power/reset/Makefile       |   1 +
 drivers/power/reset/sun6i-reboot.c |  85 +++++++++++++++++++++++++
 drivers/watchdog/sunxi_wdt.c       |  33 ++++++++++
 6 files changed, 130 insertions(+), 127 deletions(-)
 create mode 100644 drivers/power/reset/sun6i-reboot.c

-- 
1.9.1

[PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

[Maxime Ripard]

Most of the watchdog code is duplicated between the machine restart code and
the watchdog driver. Add the restart hook to the watchdog driver, to be able to
remove it from the machine code eventually.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/watchdog/sunxi_wdt.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index cd00a7836cdc..0644c45d2b60 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -22,9 +23,12 @@
 #include <linux/moduleparam.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
+#include <asm/system_misc.h>
+
 #define WDT_MAX_TIMEOUT         16
 #define WDT_MIN_TIMEOUT         1
 #define WDT_MODE_TIMEOUT(n)     ((n) << 3)
@@ -70,6 +74,30 @@ static const int wdt_timeout_map[] = {
 	[16] = 0b1011, /* 16s */
 };
 
+static struct sunxi_wdt_dev *sunxi_restart_ctx;
+
+static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
+{
+	if (!sunxi_restart_ctx)
+		return;
+
+	/* Enable timer and set reset bit in the watchdog */
+	writel(WDT_MODE_EN | WDT_MODE_RST_EN,
+	       sunxi_restart_ctx->wdt_base + WDT_MODE);
+
+	/*
+	 * Restart the watchdog. The default (and lowest) interval
+	 * value for the watchdog is 0.5s.
+	 */
+	writel(WDT_CTRL_RELOAD, sunxi_restart_ctx->wdt_base + WDT_CTRL);
+
+	while (1) {
+		mdelay(5);
+		writel(WDT_MODE_EN | WDT_MODE_RST_EN,
+		       sunxi_restart_ctx->wdt_base + WDT_MODE);
+	}
+}
+
 static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
@@ -181,6 +209,9 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
 	if (unlikely(err))
 		return err;
 
+	sunxi_restart_ctx = sunxi_wdt;
+	arm_pm_restart = sun4i_wdt_restart;
+
 	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
 			sunxi_wdt->wdt_dev.timeout, nowayout);
 
@@ -191,6 +222,8 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
 {
 	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
 
+	arm_pm_restart = NULL;
+
 	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
 	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);
 
-- 
1.9.1

[PATCH 2/5] power: reset: Add Allwinner A31 reset code

[Maxime Ripard]

That code used to be in the machine code, but it's more fit here with other
restart hooks.

That will allow to cleanup the machine directory, while waiting for a proper
watchdog driver for the A31.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/power/reset/Kconfig        |  7 ++++
 drivers/power/reset/Makefile       |  1 +
 drivers/power/reset/sun6i-reboot.c | 85 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/power/reset/sun6i-reboot.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index fa0e4e057b99..67aeb6ec08f9 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -43,6 +43,13 @@ config POWER_RESET_RESTART
 	  Instead they restart, and u-boot holds the SoC until the
 	  user presses a key. u-boot then boots into Linux.
 
+config POWER_RESET_SUN6I
+	bool "Allwinner A31 SoC reset driver"
+	depends on ARCH_SUNXI
+	depends on POWER_RESET
+	help
+	  Reboot support for the Allwinner A31 SoCs.
+
 config POWER_RESET_VEXPRESS
 	bool "ARM Versatile Express power-off and reset driver"
 	depends on ARM || ARM64
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index a5b4a77d1a41..950fdc011c7a 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
 obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
+obj-$(CONFIG_POWER_RESET_SUN6I) += sun6i-reboot.o
 obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o
 obj-$(CONFIG_POWER_RESET_XGENE) += xgene-reboot.o
diff --git a/drivers/power/reset/sun6i-reboot.c b/drivers/power/reset/sun6i-reboot.c
new file mode 100644
index 000000000000..af2cd7ff2fe8
--- /dev/null
+++ b/drivers/power/reset/sun6i-reboot.c
@@ -0,0 +1,85 @@
+/*
+ * Allwinner A31 SoCs reset code
+ *
+ * Copyright (C) 2012-2014 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+
+#include <asm/system_misc.h>
+
+#define SUN6I_WATCHDOG1_IRQ_REG		0x00
+#define SUN6I_WATCHDOG1_CTRL_REG	0x10
+#define SUN6I_WATCHDOG1_CTRL_RESTART		BIT(0)
+#define SUN6I_WATCHDOG1_CONFIG_REG	0x14
+#define SUN6I_WATCHDOG1_CONFIG_RESTART		BIT(0)
+#define SUN6I_WATCHDOG1_CONFIG_IRQ		BIT(1)
+#define SUN6I_WATCHDOG1_MODE_REG	0x18
+#define SUN6I_WATCHDOG1_MODE_ENABLE		BIT(0)
+
+static void __iomem *wdt_base;
+
+static void sun6i_wdt_restart(enum reboot_mode mode, const char *cmd)
+{
+	if (!wdt_base)
+		return;
+
+	/* Disable interrupts */
+	writel(0, wdt_base + SUN6I_WATCHDOG1_IRQ_REG);
+
+	/* We want to disable the IRQ and just reset the whole system */
+	writel(SUN6I_WATCHDOG1_CONFIG_RESTART,
+		wdt_base + SUN6I_WATCHDOG1_CONFIG_REG);
+
+	/* Enable timer. The default and lowest interval value is 0.5s */
+	writel(SUN6I_WATCHDOG1_MODE_ENABLE,
+		wdt_base + SUN6I_WATCHDOG1_MODE_REG);
+
+	/* Restart the watchdog. */
+	writel(SUN6I_WATCHDOG1_CTRL_RESTART,
+		wdt_base + SUN6I_WATCHDOG1_CTRL_REG);
+
+	while (1) {
+		mdelay(5);
+		writel(SUN6I_WATCHDOG1_MODE_ENABLE,
+			wdt_base + SUN6I_WATCHDOG1_MODE_REG);
+	}
+}
+
+static int sun6i_reboot_probe(struct platform_device *pdev)
+{
+	wdt_base = of_iomap(pdev->dev.of_node, 0);
+	if (!wdt_base) {
+		WARN(1, "failed to map watchdog base address");
+		return -ENODEV;
+	}
+
+	arm_pm_restart = sun6i_wdt_restart;
+
+	return 0;
+}
+
+static struct of_device_id sun6i_reboot_of_match[] = {
+	{ .compatible = "allwinner,sun6i-a31-wdt" },
+	{}
+};
+
+static struct platform_driver sun6i_reboot_driver = {
+	.probe = sun6i_reboot_probe,
+	.driver = {
+		.name = "sun6i-reboot",
+		.of_match_table = sun6i_reboot_of_match,
+	},
+};
+module_platform_driver(sun6i_reboot_driver);
-- 
1.9.1

[PATCH 3/5] ARM: sunxi: Select restart drivers

[Maxime Ripard]

Make sure we have the restart hooks in the kernel by selecting them in Kconfig.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/mach-sunxi/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index b57d7d53b9d3..5fe80cc9b524 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -9,6 +9,10 @@ config ARCH_SUNXI
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL
 	select PINCTRL_SUNXI
+	select POWER_RESET
+	select POWER_RESET_SUN6I
+	select POWER_SUPPLY
 	select RESET_CONTROLLER
 	select SUN4I_TIMER
 	select SUN5I_HSTIMER
+	select SUNXI_WATCHDOG
-- 
1.9.1

[PATCH 4/5] ARM: sunxi: Remove reset code from the platform

[Maxime Ripard]

Now that reset is handled either by the watchdog driver for the sun4i, sun5i
and sun7i, and by a driver of its own for sun6i, we can remove it from the
platform code.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/mach-sunxi/sunxi.c | 98 ---------------------------------------------
 1 file changed, 98 deletions(-)

diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
index 460b5a4962ef..1c62a0a021d7 100644
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -12,109 +12,14 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
-#include <linux/delay.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
 #include <linux/of_platform.h>
-#include <linux/io.h>
-#include <linux/reboot.h>
 
 #include <asm/mach/arch.h>
-#include <asm/mach/map.h>
-#include <asm/system_misc.h>
 
 #include "common.h"
 
-#define SUN4I_WATCHDOG_CTRL_REG		0x00
-#define SUN4I_WATCHDOG_CTRL_RESTART		BIT(0)
-#define SUN4I_WATCHDOG_MODE_REG		0x04
-#define SUN4I_WATCHDOG_MODE_ENABLE		BIT(0)
-#define SUN4I_WATCHDOG_MODE_RESET_ENABLE	BIT(1)
-
-#define SUN6I_WATCHDOG1_IRQ_REG		0x00
-#define SUN6I_WATCHDOG1_CTRL_REG	0x10
-#define SUN6I_WATCHDOG1_CTRL_RESTART		BIT(0)
-#define SUN6I_WATCHDOG1_CONFIG_REG	0x14
-#define SUN6I_WATCHDOG1_CONFIG_RESTART		BIT(0)
-#define SUN6I_WATCHDOG1_CONFIG_IRQ		BIT(1)
-#define SUN6I_WATCHDOG1_MODE_REG	0x18
-#define SUN6I_WATCHDOG1_MODE_ENABLE		BIT(0)
-
-static void __iomem *wdt_base;
-
-static void sun4i_restart(enum reboot_mode mode, const char *cmd)
-{
-	if (!wdt_base)
-		return;
-
-	/* Enable timer and set reset bit in the watchdog */
-	writel(SUN4I_WATCHDOG_MODE_ENABLE | SUN4I_WATCHDOG_MODE_RESET_ENABLE,
-	       wdt_base + SUN4I_WATCHDOG_MODE_REG);
-
-	/*
-	 * Restart the watchdog. The default (and lowest) interval
-	 * value for the watchdog is 0.5s.
-	 */
-	writel(SUN4I_WATCHDOG_CTRL_RESTART, wdt_base + SUN4I_WATCHDOG_CTRL_REG);
-
-	while (1) {
-		mdelay(5);
-		writel(SUN4I_WATCHDOG_MODE_ENABLE | SUN4I_WATCHDOG_MODE_RESET_ENABLE,
-		       wdt_base + SUN4I_WATCHDOG_MODE_REG);
-	}
-}
-
-static void sun6i_restart(enum reboot_mode mode, const char *cmd)
-{
-	if (!wdt_base)
-		return;
-
-	/* Disable interrupts */
-	writel(0, wdt_base + SUN6I_WATCHDOG1_IRQ_REG);
-
-	/* We want to disable the IRQ and just reset the whole system */
-	writel(SUN6I_WATCHDOG1_CONFIG_RESTART,
-		wdt_base + SUN6I_WATCHDOG1_CONFIG_REG);
-
-	/* Enable timer. The default and lowest interval value is 0.5s */
-	writel(SUN6I_WATCHDOG1_MODE_ENABLE,
-		wdt_base + SUN6I_WATCHDOG1_MODE_REG);
-
-	/* Restart the watchdog. */
-	writel(SUN6I_WATCHDOG1_CTRL_RESTART,
-		wdt_base + SUN6I_WATCHDOG1_CTRL_REG);
-
-	while (1) {
-		mdelay(5);
-		writel(SUN6I_WATCHDOG1_MODE_ENABLE,
-			wdt_base + SUN6I_WATCHDOG1_MODE_REG);
-	}
-}
-
-static struct of_device_id sunxi_restart_ids[] = {
-	{ .compatible = "allwinner,sun4i-a10-wdt" },
-	{ .compatible = "allwinner,sun6i-a31-wdt" },
-	{ /*sentinel*/ }
-};
-
-static void sunxi_setup_restart(void)
-{
-	struct device_node *np;
-
-	np = of_find_matching_node(NULL, sunxi_restart_ids);
-	if (WARN(!np, "unable to setup watchdog restart"))
-		return;
-
-	wdt_base = of_iomap(np, 0);
-	WARN(!wdt_base, "failed to map watchdog base address");
-}
-
 static void __init sunxi_dt_init(void)
 {
-	sunxi_setup_restart();
-
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
@@ -128,7 +33,6 @@ static const char * const sunxi_board_dt_compat[] = {
 DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
 	.init_machine	= sunxi_dt_init,
 	.dt_compat	= sunxi_board_dt_compat,
-	.restart	= sun4i_restart,
 MACHINE_END
 
 static const char * const sun6i_board_dt_compat[] = {
@@ -148,7 +52,6 @@ DT_MACHINE_START(SUN6I_DT, "Allwinner sun6i (A31) Family")
 	.init_machine	= sunxi_dt_init,
 	.init_time	= sun6i_timer_init,
 	.dt_compat	= sun6i_board_dt_compat,
-	.restart	= sun6i_restart,
 	.smp		= smp_ops(sun6i_smp_ops),
 MACHINE_END
 
@@ -160,5 +63,4 @@ static const char * const sun7i_board_dt_compat[] = {
 DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
 	.init_machine	= sunxi_dt_init,
 	.dt_compat	= sun7i_board_dt_compat,
-	.restart	= sun4i_restart,
 MACHINE_END
-- 
1.9.1

[PATCH 5/5] ARM: sunxi: Remove sun4i and sun7i machine definitions

[Maxime Ripard]

The sun4i and sun7i machines are nothing more than a generic machine now, we
can completely remove them.

Only stays the sun6i machine, that needs to have its reset controller enabled
before the timers.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/mach-sunxi/sunxi.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
index 1c62a0a021d7..205cd75f63f5 100644
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -12,29 +12,11 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
-#include <linux/of_platform.h>
 
 #include <asm/mach/arch.h>
 
 #include "common.h"
 
-static void __init sunxi_dt_init(void)
-{
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-}
-
-static const char * const sunxi_board_dt_compat[] = {
-	"allwinner,sun4i-a10",
-	"allwinner,sun5i-a10s",
-	"allwinner,sun5i-a13",
-	NULL,
-};
-
-DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
-	.init_machine	= sunxi_dt_init,
-	.dt_compat	= sunxi_board_dt_compat,
-MACHINE_END
-
 static const char * const sun6i_board_dt_compat[] = {
 	"allwinner,sun6i-a31",
 	NULL,
@@ -49,18 +31,7 @@ static void __init sun6i_timer_init(void)
 }
 
 DT_MACHINE_START(SUN6I_DT, "Allwinner sun6i (A31) Family")
-	.init_machine	= sunxi_dt_init,
 	.init_time	= sun6i_timer_init,
 	.dt_compat	= sun6i_board_dt_compat,
 	.smp		= smp_ops(sun6i_smp_ops),
 MACHINE_END
-
-static const char * const sun7i_board_dt_compat[] = {
-	"allwinner,sun7i-a20",
-	NULL,
-};
-
-DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
-	.init_machine	= sunxi_dt_init,
-	.dt_compat	= sun7i_board_dt_compat,
-MACHINE_END
-- 
1.9.1

Re: [PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

[Arnd Bergmann]

On Wednesday 23 April 2014 17:04:32 Maxime Ripard wrote:
> Most of the watchdog code is duplicated between the machine restart code and
> the watchdog driver. Add the restart hook to the watchdog driver, to be able to
> remove it from the machine code eventually.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>


> @@ -181,6 +209,9 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>  	if (unlikely(err))
>  		return err;
>  
> +	sunxi_restart_ctx = sunxi_wdt;
> +	arm_pm_restart = sun4i_wdt_restart;
> +
>  	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
>  			sunxi_wdt->wdt_dev.timeout, nowayout);
>  
> @@ -191,6 +222,8 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
>  {
>  	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
>  
> +	arm_pm_restart = NULL;
> +
>  	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
>  	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);

I think we will eventually want a more sophisticated method of registering
and unregistering reset handlers in order to deal with multiple ways of
triggering reset. For now, your approach seems sufficient.

	Arnd

Re: [PATCH 2/5] power: reset: Add Allwinner A31 reset code

[Arnd Bergmann]

On Wednesday 23 April 2014 17:04:33 Maxime Ripard wrote:
> That code used to be in the machine code, but it's more fit here with other
> restart hooks.
> 
> That will allow to cleanup the machine directory, while waiting for a proper
> watchdog driver for the A31.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/5] ARM: sunxi: Select restart drivers

[Arnd Bergmann]

On Wednesday 23 April 2014 17:04:34 Maxime Ripard wrote:
> Make sure we have the restart hooks in the kernel by selecting them in Kconfig.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/mach-sunxi/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index b57d7d53b9d3..5fe80cc9b524 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -9,6 +9,10 @@ config ARCH_SUNXI
>         select HAVE_ARM_ARCH_TIMER
>         select PINCTRL
>         select PINCTRL_SUNXI
> +       select POWER_RESET
> +       select POWER_RESET_SUN6I
> +       select POWER_SUPPLY
>         select RESET_CONTROLLER
>         select SUN4I_TIMER
>         select SUN5I_HSTIMER
> +       select SUNXI_WATCHDOG

I think you can't select SUNXI_WATCHDOG without also selecting WATCHDOG
first. I would prefer not to do that however, and leave it up to the
defconfig to enable it, loading the reset/watchdog driver as a module
seems entirely reasonable in a multiplatform distro kernel.

	Arnd

Re: [PATCH 4/5] ARM: sunxi: Remove reset code from the platform

[Arnd Bergmann]

On Wednesday 23 April 2014 17:04:35 Maxime Ripard wrote:
> Now that reset is handled either by the watchdog driver for the sun4i, sun5i
> and sun7i, and by a driver of its own for sun6i, we can remove it from the
> platform code.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/mach-sunxi/sunxi.c | 98 ---------------------------------------------
>  1 file changed, 98 deletions(-)
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 5/5] ARM: sunxi: Remove sun4i and sun7i machine definitions

[Arnd Bergmann]

On Wednesday 23 April 2014 17:04:36 Maxime Ripard wrote:
>  
> -static void __init sunxi_dt_init(void)
> -{
> -       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> -}
> -
> -static const char * const sunxi_board_dt_compat[] = {
> -       "allwinner,sun4i-a10",
> -       "allwinner,sun5i-a10s",
> -       "allwinner,sun5i-a13",
> -       NULL,
> -};
> -
> -DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
> -       .init_machine   = sunxi_dt_init,
> -       .dt_compat      = sunxi_board_dt_compat,
> -MACHINE_END
> -
>  static const char * const sun6i_board_dt_compat[] = {
>         "allwinner,sun6i-a31",
>         NULL,

I'd like to hear more opinions on this. We could either rely
on the generic code, or we could keep the entry with just
the .dt_compat line and the name, so /proc/cpuinfo contains
a meaningful platform name.

Either approach works for me, but I think we should do this
consistent across platforms. Olof, do you have an opinion?

	Arnd

Re: [PATCH 3/5] ARM: sunxi: Select restart drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Wed, Apr 23, 2014 at 06:00:03PM +0200, Arnd Bergmann wrote:
> On Wednesday 23 April 2014 17:04:34 Maxime Ripard wrote:
> > Make sure we have the restart hooks in the kernel by selecting them in Kconfig.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  arch/arm/mach-sunxi/Kconfig | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index b57d7d53b9d3..5fe80cc9b524 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -9,6 +9,10 @@ config ARCH_SUNXI
> >         select HAVE_ARM_ARCH_TIMER
> >         select PINCTRL
> >         select PINCTRL_SUNXI
> > +       select POWER_RESET
> > +       select POWER_RESET_SUN6I
> > +       select POWER_SUPPLY
> >         select RESET_CONTROLLER
> >         select SUN4I_TIMER
> >         select SUN5I_HSTIMER
> > +       select SUNXI_WATCHDOG
> 
> I think you can't select SUNXI_WATCHDOG without also selecting WATCHDOG
> first. I would prefer not to do that however, and leave it up to the
> defconfig to enable it, loading the reset/watchdog driver as a module
> seems entirely reasonable in a multiplatform distro kernel.

Ok. Let's drop this patch, and I'll send some patches for the
defconfig.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] ARM: sunxi: Remove sun4i and sun7i machine definitions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

On Wed, Apr 23, 2014 at 06:02:51PM +0200, Arnd Bergmann wrote:
> On Wednesday 23 April 2014 17:04:36 Maxime Ripard wrote:
> >  
> > -static void __init sunxi_dt_init(void)
> > -{
> > -       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> > -}
> > -
> > -static const char * const sunxi_board_dt_compat[] = {
> > -       "allwinner,sun4i-a10",
> > -       "allwinner,sun5i-a10s",
> > -       "allwinner,sun5i-a13",
> > -       NULL,
> > -};
> > -
> > -DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
> > -       .init_machine   = sunxi_dt_init,
> > -       .dt_compat      = sunxi_board_dt_compat,
> > -MACHINE_END
> > -
> >  static const char * const sun6i_board_dt_compat[] = {
> >         "allwinner,sun6i-a31",
> >         NULL,
> 
> I'd like to hear more opinions on this. We could either rely
> on the generic code, or we could keep the entry with just
> the .dt_compat line and the name, so /proc/cpuinfo contains
> a meaningful platform name.

Ah! I haven't thought of /proc/cpuinfo. I agree that having something
meaningful in there would be much better.

I'll respin the patch if Olof agrees.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

[Guenter Roeck]

On 04/23/2014 08:04 AM, Maxime Ripard wrote:
> Most of the watchdog code is duplicated between the machine restart code and
> the watchdog driver. Add the restart hook to the watchdog driver, to be able to
> remove it from the machine code eventually.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>   drivers/watchdog/sunxi_wdt.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index cd00a7836cdc..0644c45d2b60 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -14,6 +14,7 @@
>    */
>
>   #include <linux/clk.h>
> +#include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
> @@ -22,9 +23,12 @@
>   #include <linux/moduleparam.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
>
> +#include <asm/system_misc.h>
> +
>   #define WDT_MAX_TIMEOUT         16
>   #define WDT_MIN_TIMEOUT         1
>   #define WDT_MODE_TIMEOUT(n)     ((n) << 3)
> @@ -70,6 +74,30 @@ static const int wdt_timeout_map[] = {
>   	[16] = 0b1011, /* 16s */
>   };
>
> +static struct sunxi_wdt_dev *sunxi_restart_ctx;
> +
> +static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	if (!sunxi_restart_ctx)
> +		return;
> +
The only condition where this can happen if is there is a race between the calling
code and the remove function below. If that race really exists, it could as well
happen after this check, so the check does not really provide any value and can
be removed.

Note that I find the variable name a bit misleading. It isn't a context,
it is the poitner to sunxi_wdt_dev. What is really needed is wdt_base,
not this pointer. Would it make more sense to provide that pointer
directly instead, like you did in the code for the A31 ?

Guenter

> +	/* Enable timer and set reset bit in the watchdog */
> +	writel(WDT_MODE_EN | WDT_MODE_RST_EN,
> +	       sunxi_restart_ctx->wdt_base + WDT_MODE);
> +
> +	/*
> +	 * Restart the watchdog. The default (and lowest) interval
> +	 * value for the watchdog is 0.5s.
> +	 */
> +	writel(WDT_CTRL_RELOAD, sunxi_restart_ctx->wdt_base + WDT_CTRL);
> +
> +	while (1) {
> +		mdelay(5);
> +		writel(WDT_MODE_EN | WDT_MODE_RST_EN,
> +		       sunxi_restart_ctx->wdt_base + WDT_MODE);
> +	}
> +}
> +
>   static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
>   {
>   	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> @@ -181,6 +209,9 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>   	if (unlikely(err))
>   		return err;
>
> +	sunxi_restart_ctx = sunxi_wdt;
> +	arm_pm_restart = sun4i_wdt_restart;
> +
>   	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
>   			sunxi_wdt->wdt_dev.timeout, nowayout);
>
> @@ -191,6 +222,8 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
>   {
>   	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
>
> +	arm_pm_restart = NULL;
> +
>   	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
>   	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);
>
>

Re: [PATCH 2/5] power: reset: Add Allwinner A31 reset code

[Guenter Roeck]

On 04/23/2014 08:04 AM, Maxime Ripard wrote:
> That code used to be in the machine code, but it's more fit here with other
> restart hooks.
>
> That will allow to cleanup the machine directory, while waiting for a proper
> watchdog driver for the A31.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

I am a bit lost here. Why is this a separate driver, accessing watchdog registers,
while the other reset functions are being moved into the watchdog code ?

Any chance to handle all platforms the same ? Seems to me that would be less messy.

Guenter

> ---
>   drivers/power/reset/Kconfig        |  7 ++++
>   drivers/power/reset/Makefile       |  1 +
>   drivers/power/reset/sun6i-reboot.c | 85 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 93 insertions(+)
>   create mode 100644 drivers/power/reset/sun6i-reboot.c
>
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index fa0e4e057b99..67aeb6ec08f9 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -43,6 +43,13 @@ config POWER_RESET_RESTART
>   	  Instead they restart, and u-boot holds the SoC until the
>   	  user presses a key. u-boot then boots into Linux.
>
> +config POWER_RESET_SUN6I
> +	bool "Allwinner A31 SoC reset driver"
> +	depends on ARCH_SUNXI
> +	depends on POWER_RESET
> +	help
> +	  Reboot support for the Allwinner A31 SoCs.
> +
>   config POWER_RESET_VEXPRESS
>   	bool "ARM Versatile Express power-off and reset driver"
>   	depends on ARM || ARM64
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index a5b4a77d1a41..950fdc011c7a 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -3,5 +3,6 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>   obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>   obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
> +obj-$(CONFIG_POWER_RESET_SUN6I) += sun6i-reboot.o
>   obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o
>   obj-$(CONFIG_POWER_RESET_XGENE) += xgene-reboot.o
> diff --git a/drivers/power/reset/sun6i-reboot.c b/drivers/power/reset/sun6i-reboot.c
> new file mode 100644
> index 000000000000..af2cd7ff2fe8
> --- /dev/null
> +++ b/drivers/power/reset/sun6i-reboot.c
> @@ -0,0 +1,85 @@
> +/*
> + * Allwinner A31 SoCs reset code
> + *
> + * Copyright (C) 2012-2014 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +
> +#include <asm/system_misc.h>
> +
> +#define SUN6I_WATCHDOG1_IRQ_REG		0x00
> +#define SUN6I_WATCHDOG1_CTRL_REG	0x10
> +#define SUN6I_WATCHDOG1_CTRL_RESTART		BIT(0)
> +#define SUN6I_WATCHDOG1_CONFIG_REG	0x14
> +#define SUN6I_WATCHDOG1_CONFIG_RESTART		BIT(0)
> +#define SUN6I_WATCHDOG1_CONFIG_IRQ		BIT(1)
> +#define SUN6I_WATCHDOG1_MODE_REG	0x18
> +#define SUN6I_WATCHDOG1_MODE_ENABLE		BIT(0)
> +
> +static void __iomem *wdt_base;
> +
> +static void sun6i_wdt_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	if (!wdt_base)
> +		return;
> +
> +	/* Disable interrupts */
> +	writel(0, wdt_base + SUN6I_WATCHDOG1_IRQ_REG);
> +
> +	/* We want to disable the IRQ and just reset the whole system */
> +	writel(SUN6I_WATCHDOG1_CONFIG_RESTART,
> +		wdt_base + SUN6I_WATCHDOG1_CONFIG_REG);
> +
> +	/* Enable timer. The default and lowest interval value is 0.5s */
> +	writel(SUN6I_WATCHDOG1_MODE_ENABLE,
> +		wdt_base + SUN6I_WATCHDOG1_MODE_REG);
> +
> +	/* Restart the watchdog. */
> +	writel(SUN6I_WATCHDOG1_CTRL_RESTART,
> +		wdt_base + SUN6I_WATCHDOG1_CTRL_REG);
> +
> +	while (1) {
> +		mdelay(5);
> +		writel(SUN6I_WATCHDOG1_MODE_ENABLE,
> +			wdt_base + SUN6I_WATCHDOG1_MODE_REG);
> +	}
> +}
> +
> +static int sun6i_reboot_probe(struct platform_device *pdev)
> +{
> +	wdt_base = of_iomap(pdev->dev.of_node, 0);
> +	if (!wdt_base) {
> +		WARN(1, "failed to map watchdog base address");
> +		return -ENODEV;
> +	}
> +
> +	arm_pm_restart = sun6i_wdt_restart;
> +
> +	return 0;
> +}
> +
> +static struct of_device_id sun6i_reboot_of_match[] = {
> +	{ .compatible = "allwinner,sun6i-a31-wdt" },
> +	{}
> +};
> +
> +static struct platform_driver sun6i_reboot_driver = {
> +	.probe = sun6i_reboot_probe,
> +	.driver = {
> +		.name = "sun6i-reboot",
> +		.of_match_table = sun6i_reboot_of_match,
> +	},
> +};
> +module_platform_driver(sun6i_reboot_driver);
>

Re: [PATCH 1/5] wdt: sunxi: Move restart code to the watchdog driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

Hi,

On Sat, Apr 26, 2014 at 08:31:07AM -0700, Guenter Roeck wrote:
> On 04/23/2014 08:04 AM, Maxime Ripard wrote:
> >Most of the watchdog code is duplicated between the machine restart code and
> >the watchdog driver. Add the restart hook to the watchdog driver, to be able to
> >remove it from the machine code eventually.
> >
> >Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >---
> >  drivers/watchdog/sunxi_wdt.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> >diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> >index cd00a7836cdc..0644c45d2b60 100644
> >--- a/drivers/watchdog/sunxi_wdt.c
> >+++ b/drivers/watchdog/sunxi_wdt.c
> >@@ -14,6 +14,7 @@
> >   */
> >
> >  #include <linux/clk.h>
> >+#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> >@@ -22,9 +23,12 @@
> >  #include <linux/moduleparam.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >+#include <linux/reboot.h>
> >  #include <linux/types.h>
> >  #include <linux/watchdog.h>
> >
> >+#include <asm/system_misc.h>
> >+
> >  #define WDT_MAX_TIMEOUT         16
> >  #define WDT_MIN_TIMEOUT         1
> >  #define WDT_MODE_TIMEOUT(n)     ((n) << 3)
> >@@ -70,6 +74,30 @@ static const int wdt_timeout_map[] = {
> >  	[16] = 0b1011, /* 16s */
> >  };
> >
> >+static struct sunxi_wdt_dev *sunxi_restart_ctx;
> >+
> >+static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
> >+{
> >+	if (!sunxi_restart_ctx)
> >+		return;
> >+
> The only condition where this can happen if is there is a race between the calling
> code and the remove function below. If that race really exists, it could as well
> happen after this check, so the check does not really provide any value and can
> be removed.
> 
> Note that I find the variable name a bit misleading. It isn't a context,
> it is the poitner to sunxi_wdt_dev. What is really needed is wdt_base,
> not this pointer. Would it make more sense to provide that pointer
> directly instead, like you did in the code for the A31 ?

Hmm, right, I'll change this.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] power: reset: Add Allwinner A31 reset code

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

Hi,

On Sat, Apr 26, 2014 at 08:32:27AM -0700, Guenter Roeck wrote:
> On 04/23/2014 08:04 AM, Maxime Ripard wrote:
> >That code used to be in the machine code, but it's more fit here with other
> >restart hooks.
> >
> >That will allow to cleanup the machine directory, while waiting for a proper
> >watchdog driver for the A31.
> >
> >Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> I am a bit lost here. Why is this a separate driver, accessing watchdog registers,
> while the other reset functions are being moved into the watchdog code ?
> 
> Any chance to handle all platforms the same ? Seems to me that would be less messy.

The A31 watchdog is actually a different watchdog from the one in the
other Allwinner SoCs, that probably needs a driver of its own, or at
least, a significant refactoring.

No one did this yet, but eventually this will probably happen.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] ARM: sunxi: Remove sun4i and sun7i machine definitions

[Olof Johansson]

On Wed, Apr 23, 2014 at 9:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 23 April 2014 17:04:36 Maxime Ripard wrote:
>>
>> -static void __init sunxi_dt_init(void)
>> -{
>> -       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>> -}
>> -
>> -static const char * const sunxi_board_dt_compat[] = {
>> -       "allwinner,sun4i-a10",
>> -       "allwinner,sun5i-a10s",
>> -       "allwinner,sun5i-a13",
>> -       NULL,
>> -};
>> -
>> -DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
>> -       .init_machine   = sunxi_dt_init,
>> -       .dt_compat      = sunxi_board_dt_compat,
>> -MACHINE_END
>> -
>>  static const char * const sun6i_board_dt_compat[] = {
>>         "allwinner,sun6i-a31",
>>         NULL,
>
> I'd like to hear more opinions on this. We could either rely
> on the generic code, or we could keep the entry with just
> the .dt_compat line and the name, so /proc/cpuinfo contains
> a meaningful platform name.
>
> Either approach works for me, but I think we should do this
> consistent across platforms. Olof, do you have an opinion?

In reality, today, most platforms still need some out-of-tree stuff
that usually goes into the mach directory on out of tree kernels. It
also gives a place to stick the Kconfig entries, it's been nice to
have them split out in per-platform Kconfigs instead of having them
all modify and conflict the shared one.

I know those aren't strong arguments to keep it, but given that all
other things are more or less equal, it's a good a reason as any.

But, I'm not picky either way.


-Olof

Re: [PATCH 5/5] ARM: sunxi: Remove sun4i and sun7i machine definitions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

Hi,

On Mon, Apr 28, 2014 at 06:08:53PM -0700, Olof Johansson wrote:
> On Wed, Apr 23, 2014 at 9:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 23 April 2014 17:04:36 Maxime Ripard wrote:
> >>
> >> -static void __init sunxi_dt_init(void)
> >> -{
> >> -       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >> -}
> >> -
> >> -static const char * const sunxi_board_dt_compat[] = {
> >> -       "allwinner,sun4i-a10",
> >> -       "allwinner,sun5i-a10s",
> >> -       "allwinner,sun5i-a13",
> >> -       NULL,
> >> -};
> >> -
> >> -DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)")
> >> -       .init_machine   = sunxi_dt_init,
> >> -       .dt_compat      = sunxi_board_dt_compat,
> >> -MACHINE_END
> >> -
> >>  static const char * const sun6i_board_dt_compat[] = {
> >>         "allwinner,sun6i-a31",
> >>         NULL,
> >
> > I'd like to hear more opinions on this. We could either rely
> > on the generic code, or we could keep the entry with just
> > the .dt_compat line and the name, so /proc/cpuinfo contains
> > a meaningful platform name.
> >
> > Either approach works for me, but I think we should do this
> > consistent across platforms. Olof, do you have an opinion?
> 
> In reality, today, most platforms still need some out-of-tree stuff
> that usually goes into the mach directory on out of tree kernels. It
> also gives a place to stick the Kconfig entries, it's been nice to
> have them split out in per-platform Kconfigs instead of having them
> all modify and conflict the shared one.
> 
> I know those aren't strong arguments to keep it, but given that all
> other things are more or less equal, it's a good a reason as any.

I guess the /proc/cpuinfo thing just tip the scales to keeping the
minimal machines. I'll update the patches.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]