power: Add APM X-Gene SoC system reboot driver.
Subsystems (2)
cover power: Add APM X-Gene SoC system reboot driver.
Add APM X-Gene SoC system reboot driver. This driver only supports system reboot. System shutdown is board specific and will be handled by board specific driver or GPIO based driver.
Loc Ho (2):
power: Add APM X-Gene SoC system reboot driver. This driver handles
only
power: arm64: Add DTS entry for APM X-Gene reboot driver.
arch/arm64/boot/dts/apm-storm.dtsi | 6 ++ drivers/power/reset/Kconfig | 7 +++ drivers/power/reset/Makefile | 1 + drivers/power/reset/xgene-reboot.c | 101 ++++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 0 deletions(-) create mode 100755 drivers/power/reset/xgene-reboot.c
PATCH 1/2 power: Add APM X-Gene system reboot driver
power: Add APM X-Gene SoC system reboot driver. This driver handles only system reboot. System shutdown is board specific and can be handled by board driver or GPIO based shutdown driver.
--- drivers/power/reset/Kconfig | 7 +++ drivers/power/reset/Makefile | 1 + drivers/power/reset/xgene-reboot.c | 101 ++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 0 deletions(-) create mode 100755 drivers/power/reset/xgene-reboot.c
drivers/power/reset/Kconfig +7 -0
@@ -37,3 +37,10 @@ config POWER_RESET_VEXPRESS help Power off and reset support for the ARM Ltd. Versatile Express boards. + +config POWER_RESET_XGENE + bool + default y if ARM64 + depends on POWER_RESET + help + Reboot support for the APM SoC X-Gene Eval boards.
drivers/power/reset/Makefile +1 -0
@@ -2,3 +2,4 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o +obj-$(CONFIG_POWER_RESET_XGENE) += xgene-reboot.o
drivers/power/reset/xgene-reboot.c new file +101 -0
101 lines not on this page · raw message
Signature · 1 line
1.5.5
PATCH 2/2 power: arm64: Add DTS entry for APM X-Gene reboot driver.
power: arm64: Add DTS entry for APM X-Gene reboot driver.
--- arch/arm64/boot/dts/apm-storm.dtsi | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
arch/arm64/boot/dts/apm-storm.dtsi +6 -0
@@ -178,6 +178,12 @@ }; }; + reboot at 0x17000014 { + compatible = "apm,xgene-reboot"; + reg = <0x0 0x17000014 0x0 0x100>; + mask = <0x1>; + }; + serial0: serial at 1c020000 { device_type = "serial"; compatible = "ns16550";
Signature · 1 line
1.5.5
PATCH 2/2 power: arm64: Add DTS entry for APM X-Gene reboot driver.
Quoted 6 lines · Loc Ho
power: arm64: Add DTS entry for APM X-Gene reboot driver.
Signed-off-by Loc Ho <[email protected]>Signed-off-by Feng Kan <[email protected]>Signed-off-by Kumar Sankaran <[email protected]>---
I've applied patch 1/2 to battery-2.6.git tree. I need an ack from ARM maintainers to grab this together with the first one.
Quoted 8 lines
arch/arm64/boot/dts/apm-storm.dtsi | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)arch/arm64/boot/dts/apm-storm.dtsi +0
-0
arch/arm64/boot/dts/apm-storm.dtsi line 185
Quoted 8 lines
arch/arm64/boot/dts/apm-storm.dtsi +5
-0@@ -178,6 +178,12 @@ }; }; + reboot at 0x17000014 { + compatible = "apm,xgene-reboot"; + reg = <0x0 0x17000014 0x0 0x100>; + mask = <0x1>; + };
There should be tabs instead of whitespaces.
Quoted 6 lines
arch/arm64/boot/dts/apm-storm.dtsi +1
-0+ serial0: serial at 1c020000 { device_type = "serial"; compatible = "ns16550";Signature · 1 line
1.5.5
PATCH 2/2 power: arm64: Add DTS entry for APM X-Gene reboot driver.
Quoted 22 lines · Anton Vorontsov
Quoted 6 lines · Loc Ho
power: arm64: Add DTS entry for APM X-Gene reboot driver.
Signed-off-by Loc Ho <[email protected]>Signed-off-by Feng Kan <[email protected]>Signed-off-by Kumar Sankaran <[email protected]>---
I've applied patch 1/2 to battery-2.6.git tree. I need an ack from ARM maintainers to grab this together with the first one.
Quoted 11 lines
arch/arm64/boot/dts/apm-storm.dtsi | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)arch/arm64/boot/dts/apm-storm.dtsi +0
-0@@ -178,6 +178,12 @@ }; };
Quoted 7 lines
arch/arm64/boot/dts/apm-storm.dtsi line 185
Quoted 5 lines
arch/arm64/boot/dts/apm-storm.dtsi +5
-0+ reboot at 0x17000014 { + compatible = "apm,xgene-reboot"; + reg = <0x0 0x17000014 0x0 0x100>; + mask = <0x1>; + };There should be tabs instead of whitespaces.
Binding is undocumented, as far as I can tell. Please add documentation of this under Documentation/devicetree/bindings.
In particular, the 'mask' property is nonstandard and it needs to be documented what part of the reboot controller hardware it is describing.
-Olof
PATCH 1/2 power: Add APM X-Gene system reboot driver
In reply to Loc HoQuoted 8 lines · Loc Ho
power: Add APM X-Gene SoC system reboot driver. This driver handles only system reboot. System shutdown is board specific and can be handled by board driver or GPIO based shutdown driver.
Signed-off-by Loc Ho <[email protected]>Signed-off-by Feng Kan <[email protected]>Signed-off-by Kumar Sankaran <[email protected]>---
The patch looks great, thanks for it! Just a few minor issues I noticed...
Quoted 11 lines
drivers/power/reset/Kconfig | 7 +++ drivers/power/reset/Makefile | 1 + drivers/power/reset/xgene-reboot.c | 101 ++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 0 deletions(-) create mode 100755 drivers/power/reset/xgene-reboot.cdrivers/power/reset/Kconfig +0
-0
drivers/power/reset/Kconfig config POWER_RESET_VEXPRESS · line 43
Quoted 7 lines
drivers/power/reset/Kconfig +4
-0@@ -37,3 +37,10 @@ config POWER_RESET_VEXPRESS help Power off and reset support for the ARM Ltd. Versatile Express boards. + +config POWER_RESET_XGENE + bool + default y if ARM64
This is not good. You don't want to select the driver for all ARM64 builds. I changed it to 'depends on' and made the driver optionally selectable.
drivers/power/reset/Kconfig config POWER_RESET_VEXPRESS · line 46
Quoted 3 lines
drivers/power/reset/Kconfig +3
-0+ depends on POWER_RESET + help + Reboot support for the APM SoC X-Gene Eval boards.
Some whitespace issues here...
drivers/power/reset/xgene-reboot.c
Quoted 11 lines
drivers/power/reset/Makefile +1
-0@@ -2,3 +2,4 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o +obj-$(CONFIG_POWER_RESET_XGENE) += xgene-reboot.odrivers/power/reset/xgene-reboot.c new file +0
-0
Executable bit on the source file?..
drivers/power/reset/xgene-reboot.c line 2
Quoted 6 lines
index 0000000..e6e4178
drivers/power/reset/xgene-reboot.c new file +2
-0@@ -0,0 +1,101 @@ +/* +* xgene-reboot.c - AppliedMicro X-Gene SoC Reboot Driver
No need for the file name in the file itself.
Quoted 53 lines
drivers/power/reset/xgene-reboot.c new file +53
-0+* +* Copyright (c) 2013, Applied Micro Circuits Corporation +* Author: Feng Kan <[email protected]> +* Author: Loc Ho <[email protected]> +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that 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, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, +* MA 02111-1307 USA +* +* This driver provides system reboot functionality for APM X-Gene SoC. +* For system shutdown, this is board specify. If a board designer +* implements GPIO shutdown, use the gpio-poweroff.c driver. +* +*/ +#include <linux/io.h> +#include <linux/of_device.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/stat.h> +#include <linux/slab.h> +#include <asm/system_misc.h> + +struct xgene_reboot_context { +struct platform_device *pdev; +void *csr; +u32 mask; +}; + +static struct xgene_reboot_context *xgene_restart_ctx; + +static void xgene_restart (char str,const char *cmd) +{ +struct xgene_reboot_context *ctx = xgene_restart_ctx; +unsigned long timeout; + +/* Issue the reboot */ +if (ctx) +writel (ctx->mask, ctx->csr); + + timeout = jiffies +HZ ; +while (time_before (jiffies, timeout)) +cpu_relax ();
drivers/power/reset/xgene-reboot.c line 65
Quoted 10 lines
drivers/power/reset/xgene-reboot.c new file +10
-0+ +dev_emerg (&ctx->pdev->dev,"Unable to restart system\n" ); +} + +static int xgene_reboot_probe (struct platform_device *pdev) +{ +struct xgene_reboot_context *ctx; + + ctx =devm_kzalloc (&pdev->dev,sizeof (*ctx),GFP_KERNEL ); +if (ctx ==NULL ) {
!ctx is shorter
Quoted 38 lines
drivers/power/reset/xgene-reboot.c new file +36
-0+dev_err (&pdev->dev,"out of memory for context\n" ); +return -ENODEV ; + } + ctx->csr =of_iomap (pdev->dev.of_node,0 ); +if (ctx->csr ==NULL ) { +devm_kfree (&pdev->dev, ctx); +dev_err (&pdev->dev,"can not map resource\n" ); +return -ENODEV ; + } +if (of_property_read_u32 (pdev->dev.of_node,"mask" , &ctx->mask)) + ctx->mask =0xFFFFFFFF ; + ctx->pdev = pdev; + arm_pm_restart = xgene_restart; + xgene_restart_ctx = ctx; + +return 0 ; +} + +static struct of_device_id xgene_reboot_of_match[] = { + { .compatible ="apm,xgene-reboot" }, + {} +}; + +static struct platform_driver xgene_reboot_driver = { + .probe = xgene_reboot_probe, + .driver = { + .name ="xgene-reboot" , + .of_match_table = xgene_reboot_of_match, + }, +}; + +static int __init xgene_reboot_init (void ) +{ +return platform_driver_register (&xgene_reboot_driver); +} +device_initcall (xgene_reboot_init);Signature · 1 line
1.5.5
Wow! This is an ancient git release, 5 years old. Just saying... :)
Anyways, I fixed all the nits and applied the patch to the battery-2.6.git tree.
Thanks!
Anton
PATCH 1/2 power: Add APM X-Gene system reboot driver
Quoted 29 lines · Anton Vorontsov
Quoted 28 lines · Loc Ho
+ *
+ * Copyright (c) 2013, Applied Micro Circuits Corporation
+ * Author: Feng Kan <[email protected]>
+ * Author: Loc Ho <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ * This driver provides system reboot functionality for APM X-Gene SoC.
+ * For system shutdown, this is board specify. If a board designer
+ * implements GPIO shutdown, use thegpio-poweroff.cdriver.
+ *
+ */drivers/power/reset/xgene-reboot.c new file +3
-0+#include <linux/io.h> +#include <linux/of_device.h> +#include <linux/of_address.h>
Quoted 8 lines
drivers/power/reset/xgene-reboot.c line 38
Quoted 8 lines
drivers/power/reset/xgene-reboot.c new file +8
-0+#include <linux/platform_device.h> +#include <linux/stat.h> +#include <linux/slab.h> +#include <asm/system_misc.h> + +struct xgene_reboot_context { +struct platform_device *pdev; +void *csr;
__iomem. Please run sparse.
Quoted 9 lines
Quoted 9 lines
drivers/power/reset/xgene-reboot.c new file +9
-0+u32 mask; +}; + +static struct xgene_reboot_context *xgene_restart_ctx; + +static void xgene_restart (char str,const char *cmd) +{ +struct xgene_reboot_context *ctx = xgene_restart_ctx; +unsigned long timeout;
Quoted 8 lines
drivers/power/reset/xgene-reboot.c line 55
Quoted 8 lines
drivers/power/reset/xgene-reboot.c new file +8
-0+ +/* Issue the reboot */ +if (ctx) +writel (ctx->mask, ctx->csr); + + timeout = jiffies +HZ ; +while (time_before (jiffies, timeout)) +cpu_relax ();
Maybe this should go into the arm64 layer. It doesn't seem that xgene specific.
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + dev_emerg(&ctx->pdev->dev, "Unable to restart system\n");
If ctx is NULL here this will blow up so why check for ctx before the writel?
Quoted 11 lines
drivers/power/reset/xgene-reboot.c line 65
Quoted 8 lines
drivers/power/reset/xgene-reboot.c new file +8
-0+} + +static int xgene_reboot_probe (struct platform_device *pdev) +{ +struct xgene_reboot_context *ctx; + + ctx =devm_kzalloc (&pdev->dev,sizeof (*ctx),GFP_KERNEL ); +if (ctx ==NULL ) {!ctx is shorter
drivers/power/reset/xgene-reboot.c line 66
Quoted 1 line
drivers/power/reset/xgene-reboot.c new file +1
-0+dev_err (&pdev->dev,"out of memory for context\n" );
kmalloc prints an error on failure so this print is unnecessary.
Quoted 3 lines
drivers/power/reset/xgene-reboot.c line 69
Quoted 3 lines
drivers/power/reset/xgene-reboot.c new file +3
-0+return -ENODEV ; + } + ctx->csr =of_iomap (pdev->dev.of_node,0 );
You should use platform functions instead of of_*() functions.
Quoted 2 lines
drivers/power/reset/xgene-reboot.c line 71
Quoted 2 lines
drivers/power/reset/xgene-reboot.c new file +2
-0+if (ctx->csr ==NULL ) { +devm_kfree (&pdev->dev, ctx);
This isn't necessary.
Quoted 8 lines
drivers/power/reset/xgene-reboot.c line 79
Quoted 8 lines
drivers/power/reset/xgene-reboot.c new file +8
-0+dev_err (&pdev->dev,"can not map resource\n" ); +return -ENODEV ; + } +if (of_property_read_u32 (pdev->dev.of_node,"mask" , &ctx->mask)) + ctx->mask =0xFFFFFFFF ; + ctx->pdev = pdev; + arm_pm_restart = xgene_restart; + xgene_restart_ctx = ctx;
Although it's unlikely, this exposes a race condition where the arm_pm_restart is assigned before ctx and if a restart happens in between we won't actually restart. The two should probably be swapped.
Quoted 5 lines
drivers/power/reset/xgene-reboot.c line 84
Quoted 5 lines
drivers/power/reset/xgene-reboot.c new file +5
-0+ +return 0 ; +} + +static struct of_device_id xgene_reboot_of_match[] = {
const?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/2 power: Add APM X-Gene system reboot driver
In reply to Anton VorontsovQuoted 18 lines · Anton Vorontsov
Quoted 8 lines · Loc Ho
power: Add APM X-Gene SoC system reboot driver. This driver handles only system reboot. System shutdown is board specific and can be handled by board driver or GPIO based shutdown driver.
Signed-off-by Loc Ho <[email protected]>Signed-off-by Feng Kan <[email protected]>Signed-off-by Kumar Sankaran <[email protected]>---
The patch looks great, thanks for it! Just a few minor issues I noticed...
Quoted 6 lines
drivers/power/reset/Kconfig | 7 +++ drivers/power/reset/Makefile | 1 + drivers/power/reset/xgene-reboot.c | 101 ++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 0 deletions(-) create mode 100755 drivers/power/reset/xgene-reboot.c
Quoted 16 lines
drivers/power/reset/Kconfig config POWER_RESET_VEXPRESS · line 43
Quoted 12 lines
drivers/power/reset/Kconfig +4
-0@@ -37,3 +37,10 @@ config POWER_RESET_VEXPRESS help Power off and reset support for the ARM Ltd. Versatile Express boards. + +config POWER_RESET_XGENE + bool + default y if ARM64This is not good. You don't want to select the driver for all ARM64 builds. I changed it to 'depends on' and made the driver optionally selectable.
I thought the desired default was to have a single arm64 kernel binary that could run on all platforms.
Quoted 4 lines
drivers/power/reset/Kconfig config POWER_RESET_VEXPRESS · line 46
Quoted 4 lines
drivers/power/reset/Kconfig +3
-0+ depends on POWER_RESET + help + Reboot support for the APM SoC X-Gene Eval boards.
[...]
Thanks,
Christopher
Signature · 3 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.