* [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
@ 2015-03-10 23:21 ` Moritz Fischer
0 siblings, 0 replies; 10+ messages in thread
From: Moritz Fischer @ 2015-03-10 23:21 UTC (permalink / raw)
To: sre; +Cc: dbaryshkov, dwmw2, devicetree, linux-kernel, linux-pm,
Moritz Fischer
Add a generic SYSCON register mapped poweroff mechanism.
Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
drivers/power/reset/Kconfig | 7 +++
drivers/power/reset/Makefile | 1 +
drivers/power/reset/syscon-poweroff.c | 97 +++++++++++++++++++++++++++++++++++
3 files changed, 105 insertions(+)
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 27f6646..4bae079 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -151,6 +151,13 @@ config POWER_RESET_SYSCON
help
Reboot support for generic SYSCON mapped register reset.
+config POWER_RESET_SYSCON_POWEROFF
+ bool "Generic SYSCON regmap poweroff driver"
+ depends on OF
+ select MFD_SYSCON
+ help
+ Poweroff support for generic SYSCON mapped register poweroff.
+
config POWER_RESET_RMOBILE
tristate "Renesas R-Mobile reset driver"
depends on ARCH_RMOBILE || COMPILE_TEST
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 11de15b..dbe06c3 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -17,4 +17,5 @@ obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o
obj-$(CONFIG_POWER_RESET_XGENE) += xgene-reboot.o
obj-$(CONFIG_POWER_RESET_KEYSTONE) += keystone-reset.o
obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
+obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
diff --git a/drivers/power/reset/syscon-poweroff.c b/drivers/power/reset/syscon-poweroff.c
new file mode 100644
index 0000000..fee88e2
--- /dev/null
+++ b/drivers/power/reset/syscon-poweroff.c
@@ -0,0 +1,97 @@
+/*
+ * Generic Syscon Poweroff Driver
+ *
+ * Copyright (c) 2015, National Instruments Corp.
+ * Author: Moritz Fischer <moritz.fischer@ettus.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.
+ */
+
+#include <linux/kallsyms.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/notifier.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+
+static struct regmap *map;
+static u32 offset;
+static u32 mask;
+
+void syscon_poweroff(void)
+{
+ /* Issue the poweroff */
+ regmap_write(map, offset, mask);
+
+ mdelay(1000);
+
+ pr_emerg("Unable to poweroff system\n");
+}
+
+static int syscon_poweroff_probe(struct platform_device *pdev)
+{
+ char symname[KSYM_NAME_LEN];
+
+ map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
+ if (IS_ERR(map)) {
+ dev_err(&pdev->dev, "unable to get syscon");
+ return PTR_ERR(map);
+ }
+
+ if (of_property_read_u32(pdev->dev.of_node, "offset", &offset)) {
+ dev_err(&pdev->dev, "unable to read 'offset'");
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(pdev->dev.of_node, "mask", &mask)) {
+ dev_err(&pdev->dev, "unable to read 'mask'");
+ return -EINVAL;
+ }
+
+ if (pm_power_off) {
+ lookup_symbol_name((ulong)pm_power_off, symname);
+ dev_err(&pdev->dev,
+ "pm_power_off already claimed %p %s",
+ pm_power_off, symname);
+ return -EBUSY;
+ }
+
+ pm_power_off = syscon_poweroff;
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id syscon_poweroff_of_match[] = {
+ { .compatible = "syscon-poweroff" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
+#endif
+
+static struct platform_driver syscon_poweroff_driver = {
+ .probe = syscon_poweroff_probe,
+ .driver = {
+ .name = "syscon-poweroff",
+ .of_match_table = syscon_poweroff_of_match,
+ },
+};
+module_platform_driver(syscon_poweroff_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
+MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
+MODULE_ALIAS("platform:syscon-poweroff");
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
2015-03-10 23:21 ` Moritz Fischer
(?)
@ 2015-03-11 1:39 ` Sebastian Reichel
-1 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2015-03-11 1:39 UTC (permalink / raw)
To: Moritz Fischer; +Cc: dbaryshkov, dwmw2, devicetree, linux-kernel, linux-pm
[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]
Hi,
On Tue, Mar 10, 2015 at 04:21:09PM -0700, Moritz Fischer wrote:
> Add a generic SYSCON register mapped poweroff mechanism.
Driver looks mostly fine.
> [...]
> +#ifdef CONFIG_OF
> +static const struct of_device_id syscon_poweroff_of_match[] = {
> + { .compatible = "syscon-poweroff" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
> +#endif
You can remove the #ifdef, since the driver depends on OF.
> +static struct platform_driver syscon_poweroff_driver = {
> + .probe = syscon_poweroff_probe,
You should set pm_power_off = NULL in .remove
> + .driver = {
> + .name = "syscon-poweroff",
> + .of_match_table = syscon_poweroff_of_match,
This would fail for !CONFIG_OF. If the driver has optional OF
support you should use of_match_ptr(syscon_poweroff_of_match),
which is a preprocessor macro returning NULL for !CONFIG_OF.
> + },
> +};
> +module_platform_driver(syscon_poweroff_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> +MODULE_ALIAS("platform:syscon-poweroff");
> --
> 1.9.3
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
2015-03-10 23:21 ` Moritz Fischer
(?)
(?)
@ 2015-03-11 9:31 ` Paul Bolle
2015-03-11 22:00 ` Moritz Fischer
-1 siblings, 1 reply; 10+ messages in thread
From: Paul Bolle @ 2015-03-11 9:31 UTC (permalink / raw)
To: Moritz Fischer; +Cc: sre, dbaryshkov, dwmw2, devicetree, linux-kernel, linux-pm
On Tue, 2015-03-10 at 16:21 -0700, Moritz Fischer wrote:
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> +config POWER_RESET_SYSCON_POWEROFF
> + bool "Generic SYSCON regmap poweroff driver"
This adds a bool symbol.
> + depends on OF
> + select MFD_SYSCON
> + help
> + Poweroff support for generic SYSCON mapped register poweroff.
> +
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
So this objectfile can never be part of a module.
> --- /dev/null
> +++ b/drivers/power/reset/syscon-poweroff.c
> @@ -0,0 +1,97 @@
> +/*
> + * Generic Syscon Poweroff Driver
> + *
> + * Copyright (c) 2015, National Instruments Corp.
> + * Author: Moritz Fischer <moritz.fischer@ettus.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.
> + */
> +#include <linux/module.h>
Is that include needed?
> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
This will be preprocessed away.
> +module_platform_driver(syscon_poweroff_driver);
I think the built-in equivalent of this would be adding a wrapper that
only does
platform_driver_register(&syscon_poweroff_driver);
and have that wrapper be marked with device_initcall(). Apparently
there's no macro that does all that in one line.
> +MODULE_LICENSE("GPL v2");
You probably meant
MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> +MODULE_ALIAS("platform:syscon-poweroff");
But these four macros will all be effectively preprocessed away, anyhow.
Paul Bolle
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
2015-03-11 9:31 ` Paul Bolle
@ 2015-03-11 22:00 ` Moritz Fischer
2015-03-11 23:18 ` Paul Bolle
0 siblings, 1 reply; 10+ messages in thread
From: Moritz Fischer @ 2015-03-11 22:00 UTC (permalink / raw)
To: Paul Bolle
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
devicetree, linux-kernel, linux-pm
Paul,
thanks for your feedback. While developing this I looked at other drivers in
the tree and many of your comments would apply to them, too?
On Wed, Mar 11, 2015 at 2:31 AM, Paul Bolle <pebolle@tiscali
>> +config POWER_RESET_SYSCON_POWEROFF
>> + bool "Generic SYSCON regmap poweroff driver"
>
> This adds a bool symbol.
Is that an issue or is it just to substantiate the rest of your comments below?
>> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>
> So this objectfile can never be part of a module.
Same comment would apply for power/reset/qnap-poweroff.c, right?
>> +#include <linux/module.h>
>
> Is that include needed?
Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is used?
>> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
>
> This will be preprocessed away.
In my experiments it didn't work without this, are you sure it's not needed?
>
>> +module_platform_driver(syscon_poweroff_driver);
>
> I think the built-in equivalent of this would be adding a wrapper that
> only does
> platform_driver_register(&syscon_poweroff_driver);
I couldn't find other examples in the tree doing this. Any pointers?
>> +MODULE_LICENSE("GPL v2");
>
> You probably meant
> MODULE_LICENSE("GPL");
Sorry about that. Will fix.
>
>> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
>> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
>> +MODULE_ALIAS("platform:syscon-poweroff");
>
> But these four macros will all be effectively preprocessed away, anyhow.
I'll remove them.
Thanks,
Moritz
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
2015-03-11 22:00 ` Moritz Fischer
@ 2015-03-11 23:18 ` Paul Bolle
2015-03-12 0:52 ` Moritz Fischer
0 siblings, 1 reply; 10+ messages in thread
From: Paul Bolle @ 2015-03-11 23:18 UTC (permalink / raw)
To: Moritz Fischer
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
devicetree, linux-kernel, linux-pm
Moritz,
On Wed, 2015-03-11 at 15:00 -0700, Moritz Fischer wrote:
> thanks for your feedback. While developing this I looked at other drivers in
> the tree and many of your comments would apply to them, too?
Could be. I didn't check. It's basically stuff that get's silently
handled by the preprocessor. So I wouldn't be surprised if a lot of
drivers do similar things.
> On Wed, Mar 11, 2015 at 2:31 AM, Paul Bolle <pebolle@tiscali
>
> >> +config POWER_RESET_SYSCON_POWEROFF
> >> + bool "Generic SYSCON regmap poweroff driver"
> >
> > This adds a bool symbol.
>
> Is that an issue or is it just to substantiate the rest of your comments below?
It's just an observation.
> >> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
> >
> > So this objectfile can never be part of a module.
>
> Same comment would apply for power/reset/qnap-poweroff.c, right?
That could be. I didn't check it while looking at this patch.
> >> +#include <linux/module.h>
> >
> > Is that include needed?
>
> Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is used?
Perhaps, but I don't think those macros are used.
> >> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
> >
> > This will be preprocessed away.
>
> In my experiments it didn't work without this, are you sure it's not needed?
MODULE_DEVICE_TABLE() is defined only in include/linux/module.h. The few
related lines read (in next-20150311):
#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name) \
extern const typeof(name) __mod_##type##__##name##_device_table \
__attribute__ ((unused, alias(__stringify(name))))
#else /* !MODULE */
#define MODULE_DEVICE_TABLE(type, name)
#endif
So I think MODULE_DEVICE_TABLE() is always preprocessed away for boolean
code. Did I get that right?
> >> +module_platform_driver(syscon_poweroff_driver);
> >
> > I think the built-in equivalent of this would be adding a wrapper that
> > only does
> > platform_driver_register(&syscon_poweroff_driver);
>
> I couldn't find other examples in the tree doing this. Any pointers?
Please note "think". But see drivers/cpufreq/dbx500-cpufreq.c:
static int __init dbx500_cpufreq_register(void)
{
return platform_driver_register(&dbx500_cpufreq_plat_driver);
}
device_initcall(dbx500_cpufreq_register);
or drivers/gpio/gpio-vf610.c:
static int __init gpio_vf610_init(void)
{
return platform_driver_register(&vf610_gpio_driver);
}
device_initcall(gpio_vf610_init);
(There are probably more.) Please note that both files use MODULE_*
macros but can actually only be built-in.
> >> +MODULE_LICENSE("GPL v2");
> >
> > You probably meant
> > MODULE_LICENSE("GPL");
>
> Sorry about that. Will fix.
That's a very easy mistake to make anyway. There's currently a thread
were (another) Dmitry and I discuss the merits of differentiating
between "GPL" and "GPL v2" in MODULE_LICENSE().
> >> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> >> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> >> +MODULE_ALIAS("platform:syscon-poweroff");
> >
> > But these four macros will all be effectively preprocessed away, anyhow.
>
> I'll remove them.
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.
2015-03-11 23:18 ` Paul Bolle
@ 2015-03-12 0:52 ` Moritz Fischer
0 siblings, 0 replies; 10+ messages in thread
From: Moritz Fischer @ 2015-03-12 0:52 UTC (permalink / raw)
To: Paul Bolle
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
devicetree, linux-kernel, linux-pm
Paul,
On Wed, Mar 11, 2015 at 4:18 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> MODULE_DEVICE_TABLE() is defined only in include/linux/module.h. The few
> related lines read (in next-20150311):
> #ifdef MODULE
> /* Creates an alias so file2alias.c can find device table. */
> #define MODULE_DEVICE_TABLE(type, name) \
> extern const typeof(name) __mod_##type##__##name##_device_table \
> __attribute__ ((unused, alias(__stringify(name))))
> #else /* !MODULE */
> #define MODULE_DEVICE_TABLE(type, name)
> #endif
>
> So I think MODULE_DEVICE_TABLE() is always preprocessed away for boolean
> code. Did I get that right?
Yeah, looking at that convinced me :) I have it already removed in v2.
>
>> >> +module_platform_driver(syscon_poweroff_driver);
>> >
>> > I think the built-in equivalent of this would be adding a wrapper that
>> > only does
>> > platform_driver_register(&syscon_poweroff_driver);
>>
>> I couldn't find other examples in the tree doing this. Any pointers?
>
> Please note "think". But see drivers/cpufreq/dbx500-cpufreq.c:
> static int __init dbx500_cpufreq_register(void)
> {
> return platform_driver_register(&dbx500_cpufreq_plat_driver);
> }
> device_initcall(dbx500_cpufreq_register);
>
> or drivers/gpio/gpio-vf610.c:
> static int __init gpio_vf610_init(void)
> {
> return platform_driver_register(&vf610_gpio_driver);
> }
> device_initcall(gpio_vf610_init);
I included it into v3. Thanks.
Thanks again for your feedback,
Moritz
^ permalink raw reply [flat|nested] 10+ messages in thread