[PATCH 0/2] power: Add APM X-Gene SoC system reboot driver.

[PATCH 0/2] power: Add APM X-Gene SoC system reboot driver.

[Loc Ho]

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

[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 <lho@apm.com>
Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
---
 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

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 349e9ae..c41a7de 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -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.
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 372807f..e75d54c 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -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
diff --git a/drivers/power/reset/xgene-reboot.c b/drivers/power/reset/xgene-reboot.c
new file mode 100755
index 0000000..e6e4178
--- /dev/null
+++ b/drivers/power/reset/xgene-reboot.c
@@ -0,0 +1,101 @@
+/*
+ * xgene-reboot.c - AppliedMicro X-Gene SoC Reboot Driver
+ *
+ * Copyright (c) 2013, Applied Micro Circuits Corporation
+ * Author: Feng Kan <fkan@apm.com>
+ * Author: Loc Ho <lho@apm.com>
+ *
+ * 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();
+
+	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) {
+		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);
-- 
1.5.5

[PATCH 2/2] power: arm64: Add DTS entry for APM X-Gene reboot driver.

[Loc Ho]

power: arm64: Add DTS entry for APM X-Gene reboot driver.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..e686919 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -178,6 +178,12 @@
 			};
 		};
 
+                reboot@0x17000014 {
+                        compatible = "apm,xgene-reboot";
+                        reg = <0x0 0x17000014 0x0 0x100>;
+                        mask = <0x1>;
+                };
+
 		serial0: serial@1c020000 {
 			device_type = "serial";
 			compatible = "ns16550";
-- 
1.5.5

Re: [PATCH 2/2] power: arm64: Add DTS entry for APM X-Gene reboot driver.

[Anton Vorontsov]

On Tue, Jul 02, 2013 at 02:38:59PM -0600, Loc Ho wrote:
> power: arm64: Add DTS entry for APM X-Gene reboot driver.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
> ---

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.

>  arch/arm64/boot/dts/apm-storm.dtsi |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
> index d37d736..e686919 100644
> --- a/arch/arm64/boot/dts/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
> @@ -178,6 +178,12 @@
>  			};
>  		};
>  
> +                reboot@0x17000014 {
> +                        compatible = "apm,xgene-reboot";
> +                        reg = <0x0 0x17000014 0x0 0x100>;
> +                        mask = <0x1>;
> +                };

There should be tabs instead of whitespaces.

> +
>  		serial0: serial@1c020000 {
>  			device_type = "serial";
>  			compatible = "ns16550";
> -- 
> 1.5.5

Re: [PATCH 2/2] power: arm64: Add DTS entry for APM X-Gene reboot driver.

[Olof Johansson]

On Fri, Aug 9, 2013 at 2:31 PM, Anton Vorontsov <anton@enomsg.org> wrote:
> On Tue, Jul 02, 2013 at 02:38:59PM -0600, Loc Ho wrote:
>> power: arm64: Add DTS entry for APM X-Gene reboot driver.
>>
>> Signed-off-by: Loc Ho <lho@apm.com>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
>> ---
>
> 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.
>
>>  arch/arm64/boot/dts/apm-storm.dtsi |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
>> index d37d736..e686919 100644
>> --- a/arch/arm64/boot/dts/apm-storm.dtsi
>> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
>> @@ -178,6 +178,12 @@
>>                       };
>>               };
>>
>> +                reboot@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

Re: [PATCH 1/2] power: Add APM X-Gene system reboot driver

[Anton Vorontsov]

On Tue, Jul 02, 2013 at 02:38:58PM -0600, Loc Ho wrote:
> 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 <lho@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
> ---

The patch looks great, thanks for it! Just a few minor issues I noticed...

>  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
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 349e9ae..c41a7de 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -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.

> +        depends on POWER_RESET	
> +        help
> +          Reboot support for the APM SoC X-Gene Eval boards.

Some whitespace issues here...

> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 372807f..e75d54c 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -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
> diff --git a/drivers/power/reset/xgene-reboot.c b/drivers/power/reset/xgene-reboot.c
> new file mode 100755

Executable bit on the source file?..

> index 0000000..e6e4178
> --- /dev/null
> +++ b/drivers/power/reset/xgene-reboot.c
> @@ -0,0 +1,101 @@
> +/*
> + * xgene-reboot.c - AppliedMicro X-Gene SoC Reboot Driver

No need for the file name in the file itself.

> + *
> + * Copyright (c) 2013, Applied Micro Circuits Corporation
> + * Author: Feng Kan <fkan@apm.com>
> + * Author: Loc Ho <lho@apm.com>
> + *
> + * 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();
> +
> +	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

> +		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);
> -- 
> 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

Re: [PATCH 1/2] power: Add APM X-Gene system reboot driver

[Stephen Boyd]

On 08/09/13 14:28, Anton Vorontsov wrote:
> On Tue, Jul 02, 2013 at 02:38:58PM -0600, Loc Ho wrote:
>> + *
>> + * Copyright (c) 2013, Applied Micro Circuits Corporation
>> + * Author: Feng Kan <fkan@apm.com>
>> + * Author: Loc Ho <lho@apm.com>
>> + *
>> + * 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;

__iomem. Please run sparse.

>> +	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();

Maybe this should go into the arm64 layer. It doesn't seem that xgene
specific.

>> +
>> +	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?

>> +}
>> +
>> +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
>
>> +		dev_err(&pdev->dev, "out of memory for context\n");

kmalloc prints an error on failure so this print is unnecessary.

>> +		return -ENODEV;
>> +	}
>> +	ctx->csr = of_iomap(pdev->dev.of_node, 0);

You should use platform functions instead of of_*() functions.

>> +	if (ctx->csr == NULL) {
>> +		devm_kfree(&pdev->dev, ctx);

This isn't necessary.

>> +		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.

>> +
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id xgene_reboot_of_match[] = {

const?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/2] power: Add APM X-Gene system reboot driver

[Christopher Covington]

On 08/09/2013 05:28 PM, Anton Vorontsov wrote:
> On Tue, Jul 02, 2013 at 02:38:58PM -0600, Loc Ho wrote:
>> 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 <lho@apm.com>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> Signed-off-by: Kumar Sankaran <ksankaran@apm.com>
>> ---
> 
> The patch looks great, thanks for it! Just a few minor issues I noticed...
> 
>>  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
>>
>> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
>> index 349e9ae..c41a7de 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -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.

I thought the desired default was to have a single arm64 kernel binary that
could run on all platforms.

> 
>> +        depends on POWER_RESET	
>> +        help
>> +          Reboot support for the APM SoC X-Gene Eval boards.

[...]

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.