| Message ID | 1372797539-13111-2-git-send-email-lho@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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
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);