[PATCH 0/2] Add support for generic register mapped poweroff.

[PATCH 0/2] Add support for generic register mapped poweroff.

[Moritz Fischer]

This patchset adds support for generic register mapped poweroff via SYCON.

Earlier versions of this were specific to our product, however,
as Sebastian suggested, it can be generalized into a generic driver.

If an entry in the MAINTAINERS file is required I can resend a corrected version.

Moritz Fischer (2):
  power: reset: Add generic SYSCON register mapped poweroff.
  dt: power: Add docs for generic SYSCON poweroff driver.

 .../bindings/power/reset/syscon-poweroff.txt       | 23 +++++
 drivers/power/reset/Kconfig                        |  7 ++
 drivers/power/reset/Makefile                       |  1 +
 drivers/power/reset/syscon-poweroff.c              | 97 ++++++++++++++++++++++
 4 files changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt
 create mode 100644 drivers/power/reset/syscon-poweroff.c

-- 
1.9.3

[PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[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

[PATCH 2/2] dt: power: Add docs for generic SYSCON poweroff driver.

[Moritz Fischer]

Add documentation for generic SYSCON poweroff driver.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 .../bindings/power/reset/syscon-poweroff.txt       | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt b/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt
new file mode 100644
index 0000000..1422cee
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt
@@ -0,0 +1,23 @@
+Generic SYSCON mapped register poweroff driver
+
+This is a generic poweroff driver using syscon to map the poweroff register.
+The poweroff is generally performed with a write to the poweroff register
+defined by the register map pointed by syscon reference plus the offset
+with the mask defined in the poweroff node.
+
+Required properties:
+- compatible: should contain "syscon-poweroff"
+- regmap: this is phandle to the register map node
+- offset: offset in the register map for the poweroff register (in bytes)
+- mask: the poweroff value written to the poweroff register (32 bit access)
+
+Default will be little endian mode, 32 bit access only.
+
+Examples:
+
+	reboot {
+	   compatible = "syscon-reboot";
+	   regmap = <&regmapnode>;
+	   offset = <0x0>;
+	   mask = <0x7a>;
+	};
-- 
1.9.3

Re: [PATCH 2/2] dt: power: Add docs for generic SYSCON poweroff driver.

[Moritz Fischer]

On Tue, Mar 10, 2015 at 4:21 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> Add documentation for generic SYSCON poweroff driver.
>
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  .../bindings/power/reset/syscon-poweroff.txt       | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt b/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt
> new file mode 100644
> index 0000000..1422cee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-poweroff.txt
> @@ -0,0 +1,23 @@
> +Generic SYSCON mapped register poweroff driver
> +
> +This is a generic poweroff driver using syscon to map the poweroff register.
> +The poweroff is generally performed with a write to the poweroff register
> +defined by the register map pointed by syscon reference plus the offset
> +with the mask defined in the poweroff node.
> +
> +Required properties:
> +- compatible: should contain "syscon-poweroff"
> +- regmap: this is phandle to the register map node
> +- offset: offset in the register map for the poweroff register (in bytes)
> +- mask: the poweroff value written to the poweroff register (32 bit access)
> +
> +Default will be little endian mode, 32 bit access only.
> +
> +Examples:
> +
> +       reboot {

Arghhh ...
> +          compatible = "syscon-reboot";
Arghhhh.....
> +          regmap = <&regmapnode>;
> +          offset = <0x0>;
> +          mask = <0x7a>;
> +       };
> --
> 1.9.3
>

Will collect feedback and resubmit, sorry about that.

Moritz

Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Sebastian Reichel]

[-- 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 --]

Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Paul Bolle]

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

Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Moritz Fischer]

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

Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Paul Bolle]

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

Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Moritz Fischer]

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