Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1286860 > unrolled thread

[RFC][PATCH] misc: Introduce reboot_reason driver

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-08 22:30 +0100
    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Arnd Bergmann <arnd@arndb.de> - 2015-12-08 23:00 +0100
      Re: [RFC][PATCH] misc: Introduce reboot_reason driver Bjorn Andersson <bjorn.andersson@sonymobile.com> - 2015-12-08 23:20 +0100
        Re: [RFC][PATCH] misc: Introduce reboot_reason driver Rob Herring <robh+dt@kernel.org> - 2015-12-08 23:50 +0100
      Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-09 01:20 +0100
        Re: [RFC][PATCH] misc: Introduce reboot_reason driver Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-09 10:00 +0100
          Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-09 22:50 +0100
    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Bjorn Andersson <bjorn.andersson@sonymobile.com> - 2015-12-08 23:10 +0100
      Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-09 01:30 +0100
        Re: [RFC][PATCH] misc: Introduce reboot_reason driver Arnd Bergmann <arnd@arndb.de> - 2015-12-09 11:10 +0100
          Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 02:30 +0100
            Re: [RFC][PATCH] misc: Introduce reboot_reason driver Arnd Bergmann <arnd@arndb.de> - 2015-12-10 16:00 +0100
              Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 20:00 +0100
                Re: [RFC][PATCH] misc: Introduce reboot_reason driver Rob Herring <robh+dt@kernel.org> - 2015-12-10 21:30 +0100
                  Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 22:50 +0100
                    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Arnd Bergmann <arnd@arndb.de> - 2015-12-10 23:20 +0100
      Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 02:40 +0100
        Re: [RFC][PATCH] misc: Introduce reboot_reason driver Arnd Bergmann <arnd@arndb.de> - 2015-12-10 10:10 +0100
          Re: [RFC][PATCH] misc: Introduce reboot_reason driver Tomas Winkler <tomasw@gmail.com> - 2015-12-10 10:30 +0100
            Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 20:10 +0100
              Re: [RFC][PATCH] misc: Introduce reboot_reason driver One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-10 21:00 +0100
                Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-10 21:10 +0100
    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Rob Herring <robh+dt@kernel.org> - 2015-12-08 23:30 +0100
      Re: [RFC][PATCH] misc: Introduce reboot_reason driver John Stultz <john.stultz@linaro.org> - 2015-12-09 01:40 +0100
    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-09 10:00 +0100
    Re: [RFC][PATCH] misc: Introduce reboot_reason driver Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-09 10:10 +0100

Page 1 of 2  [1] 2  Next page →

#1286860 — [RFC][PATCH] misc: Introduce reboot_reason driver

This patch adds a basic driver to allow for commands like
"reboot bootloader" and "reboot recovery" to communicate this
reboot-reason to the bootloader.

This is commonly done on Android devices, in order to reboot
the device into fastboot or recovery mode. It also supports
custom OEM specific commands, via "reboot oem-<value>".

This driver pulls the phys memory address from DT as well as
the magic reason values that are written to the address for
each mode.

For an example, this patch also adds the DT support for
the nexus7 device via its dts (which is not yet upstream).

Thoughts and feedback would be appreciated!

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Vinay Simha BN <simhavcs@gmail.com>
Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts |   9 ++
 drivers/misc/Kconfig                          |   8 ++
 drivers/misc/Makefile                         |   1 +
 drivers/misc/reboot_reason.c                  | 117 ++++++++++++++++++++++++++
 4 files changed, 135 insertions(+)
 create mode 100644 drivers/misc/reboot_reason.c

diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
index 5183d18..ee5dcb7 100644
--- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
+++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
@@ -282,6 +282,15 @@
 			};
 		};
 
+		reboot_reason: reboot_reason@2a03f65c {
+			compatible		= "reboot_reason";
+			reg			= <0x2A03F65C 0x4>;
+			reason,none		= <0x77665501>;
+			reason,bootloader	= <0x77665500>;
+			reason,recovery		= <0x77665502>;
+			reason,oem		= <0x6f656d00>;
+		};
+
 		gpio-keys {
 			compatible = "gpio-keys";
 			power {
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 22892c7..b5c141b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -525,6 +525,14 @@ config VEXPRESS_SYSCFG
 	  bus. System Configuration interface is one of the possible means
 	  of generating transactions on this bus.
 
+config REBOOT_REASON
+	bool "Pass reboot reason to bootloader"
+	default n
+	help
+	  On many systems there is a desire to provide a reboot reason to
+	  the bootloader, so that the bootloader can boot into a desired
+	  mode on the next boot.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 537d7f3..4581e62 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
+obj-$(CONFIG_REBOOT_REASON)	+= reboot_reason.o
diff --git a/drivers/misc/reboot_reason.c b/drivers/misc/reboot_reason.c
new file mode 100644
index 0000000..5c9b55eb
--- /dev/null
+++ b/drivers/misc/reboot_reason.c
@@ -0,0 +1,117 @@
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/reboot.h>
+
+/* Types of reasons */
+static enum {
+	NONE,
+	BOOTLOADER,
+	RECOVERY,
+	OEM,
+	MAX_REASONS
+} __maybe_unused reason_types;
+
+static u32			reasons[MAX_REASONS];
+static void __iomem		*reboot_reason_addr;
+static struct notifier_block	restart_nb;
+
+static int reboot_reason(struct notifier_block *nb, unsigned long action,
+								void *data)
+{
+	char *cmd = (char *)data;
+	long reason = reasons[NONE];
+
+	if (!reboot_reason_addr)
+		return NOTIFY_DONE;
+
+	if (cmd != NULL) {
+		if (!strncmp(cmd, "bootloader", 10))
+			reason = reasons[BOOTLOADER];
+		else if (!strncmp(cmd, "recovery", 8))
+			reason = reasons[RECOVERY];
+		else if (!strncmp(cmd, "oem-", 4)) {
+			unsigned long code;
+
+			if (!kstrtoul(cmd+4, 0, &code))
+				reason = reasons[OEM] | (code & 0xff);
+		}
+	}
+
+	if (reason != -1)
+		writel(reason, reboot_reason_addr);
+	return NOTIFY_DONE;
+}
+
+static int reboot_reason_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	u32 val;
+	int i;
+
+	/* initialize the reasons */
+	for (i = 0; i < MAX_REASONS; i++)
+		reasons[i] = -1;
+
+	/* Try to grab the reason io address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reboot_reason_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reboot_reason_addr))
+		return PTR_ERR(reboot_reason_addr);
+
+	/* initialize specified reasons from DT */
+	if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
+		reasons[NONE] = val;
+	if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
+		reasons[BOOTLOADER] = val;
+	if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
+		reasons[RECOVERY] = val;
+	if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
+		reasons[OEM] = val;
+
+	/* Install the notifier */
+	restart_nb.notifier_call = reboot_reason;
+	restart_nb.priority = 256;
+	if (register_restart_handler(&restart_nb)) {
+		dev_err(&pdev->dev,
+			"failed to setup restart handler.\n");
+	}
+	return 0;
+}
+
+int reboot_reason_remove(struct platform_device *pdev)
+{
+	unregister_restart_handler(&restart_nb);
+	return 0;
+}
+
+static const struct of_device_id reboot_reason_of_match[] = {
+	{ .compatible = "reboot_reason", },
+	{ },
+};
+
+static struct platform_driver reboot_reason_driver = {
+	.driver = {
+		.name = "reboot_reason",
+		.of_match_table = reboot_reason_of_match,
+	},
+	.probe = reboot_reason_probe,
+	.remove = reboot_reason_remove,
+};
+
+static int __init reboot_reason_init(void)
+{
+	return platform_driver_register(&reboot_reason_driver);
+}
+arch_initcall(reboot_reason_init);
+
+static void __exit reboot_reason_exit(void)
+{
+	platform_driver_unregister(&reboot_reason_driver);
+}
+module_exit(reboot_reason_exit);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1286878

On Tuesday 08 December 2015 13:29:22 John Stultz wrote:

> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> index 5183d18..ee5dcb7 100644
> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> @@ -282,6 +282,15 @@
>  			};
>  		};
>  
> +		reboot_reason: reboot_reason@2a03f65c {
> +			compatible		= "reboot_reason";

This is not a good compatible string. There should generally be a vendor
name associated with it (use "linux," if nothing else, and you should have
'-' instead of '_'.

> +			reg			= <0x2A03F65C 0x4>;

This may easily conflict with the device it is part of. We should have
non-overlapping register areas in general. For the example you are
looking at, which register block is this?

> +
> +/* Types of reasons */
> +static enum {
> +	NONE,
> +	BOOTLOADER,
> +	RECOVERY,
> +	OEM,
> +	MAX_REASONS
> +} __maybe_unused reason_types;

The variable seems to always be unused, not just "__maybe_unused". Maybe remove it?

> +static int reboot_reason(struct notifier_block *nb, unsigned long action,
> +								void *data)
> +{
> +	char *cmd = (char *)data;
> +	long reason = reasons[NONE];
> +
> +	if (!reboot_reason_addr)
> +		return NOTIFY_DONE;
> +
> +	if (cmd != NULL) {
> +		if (!strncmp(cmd, "bootloader", 10))
> +			reason = reasons[BOOTLOADER];
> +		else if (!strncmp(cmd, "recovery", 8))
> +			reason = reasons[RECOVERY];
> +		else if (!strncmp(cmd, "oem-", 4)) {
> +			unsigned long code;
> +
> +			if (!kstrtoul(cmd+4, 0, &code))
> +				reason = reasons[OEM] | (code & 0xff);
> +		}
> +	}
> +
> +	if (reason != -1)
> +		writel(reason, reboot_reason_addr);
> +	return NOTIFY_DONE;
> +}

Will this reboot the machine?

> +	/* Install the notifier */
> +	restart_nb.notifier_call = reboot_reason;
> +	restart_nb.priority = 256;
> +	if (register_restart_handler(&restart_nb)) {

If not, you should use register_reboot_notifier() rather than
register_restart_handler(). The former gets called to do something
just before rebooting, while the latter attempts to actually reboot
the machine.

> +static int __init reboot_reason_init(void)
> +{
> +	return platform_driver_register(&reboot_reason_driver);
> +}
> +arch_initcall(reboot_reason_init);

Why this early? If it can be a normal device_initcall, you can use
module_platform_driver().

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286895

On Tue 08 Dec 13:52 PST 2015, Arnd Bergmann wrote:

> On Tuesday 08 December 2015 13:29:22 John Stultz wrote:
> 
[..]
> > +static int reboot_reason(struct notifier_block *nb, unsigned long action,
> > +								void *data)
> > +{
> > +	char *cmd = (char *)data;
> > +	long reason = reasons[NONE];
> > +
> > +	if (!reboot_reason_addr)
> > +		return NOTIFY_DONE;
> > +
> > +	if (cmd != NULL) {
> > +		if (!strncmp(cmd, "bootloader", 10))
> > +			reason = reasons[BOOTLOADER];
> > +		else if (!strncmp(cmd, "recovery", 8))
> > +			reason = reasons[RECOVERY];
> > +		else if (!strncmp(cmd, "oem-", 4)) {
> > +			unsigned long code;
> > +
> > +			if (!kstrtoul(cmd+4, 0, &code))
> > +				reason = reasons[OEM] | (code & 0xff);
> > +		}
> > +	}
> > +
> > +	if (reason != -1)
> > +		writel(reason, reboot_reason_addr);
> > +	return NOTIFY_DONE;
> > +}
> 
> Will this reboot the machine?
> 

It will store the magic in IMEM, that is then read by the boot loader
after the reboot to make it trigger alternative boot flows - e.g. the
recovery/update path or enter fastboot mode.

> > +	/* Install the notifier */
> > +	restart_nb.notifier_call = reboot_reason;
> > +	restart_nb.priority = 256;
> > +	if (register_restart_handler(&restart_nb)) {
> 
> If not, you should use register_reboot_notifier() rather than
> register_restart_handler(). The former gets called to do something
> just before rebooting, while the latter attempts to actually reboot
> the machine.
> 

It should be a reboot_notifier, sorry for missing this in my answer.

> > +static int __init reboot_reason_init(void)
> > +{
> > +	return platform_driver_register(&reboot_reason_driver);
> > +}
> > +arch_initcall(reboot_reason_init);
> 
> Why this early? If it can be a normal device_initcall, you can use
> module_platform_driver().
> 

Not represented in this patch is that several Android vendors uses this
mechanism to communicate a panic() to the boot loader; to let the system
enter some sort of memory dump state.

Adding support for that would be convenient and mandate the early,
otherwise it's just userspace that will trigger this so module would be
fine.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286904

On Tue, Dec 8, 2015 at 4:15 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Tue 08 Dec 13:52 PST 2015, Arnd Bergmann wrote:
>
>> On Tuesday 08 December 2015 13:29:22 John Stultz wrote:

[...]

>> > +static int __init reboot_reason_init(void)
>> > +{
>> > +   return platform_driver_register(&reboot_reason_driver);
>> > +}
>> > +arch_initcall(reboot_reason_init);
>>
>> Why this early? If it can be a normal device_initcall, you can use
>> module_platform_driver().
>>
>
> Not represented in this patch is that several Android vendors uses this
> mechanism to communicate a panic() to the boot loader; to let the system
> enter some sort of memory dump state.

They could also just parse pstore and look for a panic(). Or the
bootloader should set the reason to enter memory dump and the kernel
should clear it once it is up enough to handle resets. You have a
window of time already that a panic will hang. Only a watchdog will
help there and that would need a different solution.

Are panics in early boot really common outside of development?

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286926

On Tue, Dec 8, 2015 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tuesday 08 December 2015 13:29:22 John Stultz wrote:
>
> > diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > index 5183d18..ee5dcb7 100644
> > --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > @@ -282,6 +282,15 @@
> >                       };
> >               };
> >
> > +             reboot_reason: reboot_reason@2a03f65c {
> > +                     compatible              = "reboot_reason";
>
> This is not a good compatible string. There should generally be a vendor
> name associated with it (use "linux," if nothing else, and you should have
> '-' instead of '_'.


Ack.


>
>
> > +                     reg                     = <0x2A03F65C 0x4>;
>
> This may easily conflict with the device it is part of. We should have
> non-overlapping register areas in general. For the example you are
> looking at, which register block is this?

So Bjorn says its IMEM, but I was assuming it was just a reserved
magic phys addr from the bootloader.

Ideally I'm hoping to use this same driver for another device, which
plans to reserve a page from memory that the bootloader won't clear.


> > +/* Types of reasons */
> > +static enum {
> > +     NONE,
> > +     BOOTLOADER,
> > +     RECOVERY,
> > +     OEM,
> > +     MAX_REASONS
> > +} __maybe_unused reason_types;
>
> The variable seems to always be unused, not just "__maybe_unused". Maybe remove it?


Yea. I initially just had the empty enum, but the compiler was giving
me "useless class storage specifier in empty declaration" warnings. So
I added a variable to it, but then I got unused variable warnings. So
I ended up with this. :P

Is there a better way? Are enums for array indexes out of fashion?
Just a list of #defines ?


> > +static int reboot_reason(struct notifier_block *nb, unsigned long action,
> > +                                                             void *data)
> > +{
> > +     char *cmd = (char *)data;
> > +     long reason = reasons[NONE];
> > +
> > +     if (!reboot_reason_addr)
> > +             return NOTIFY_DONE;
> > +
> > +     if (cmd != NULL) {
> > +             if (!strncmp(cmd, "bootloader", 10))
> > +                     reason = reasons[BOOTLOADER];
> > +             else if (!strncmp(cmd, "recovery", 8))
> > +                     reason = reasons[RECOVERY];
> > +             else if (!strncmp(cmd, "oem-", 4)) {
> > +                     unsigned long code;
> > +
> > +                     if (!kstrtoul(cmd+4, 0, &code))
> > +                             reason = reasons[OEM] | (code & 0xff);
> > +             }
> > +     }
> > +
> > +     if (reason != -1)
> > +             writel(reason, reboot_reason_addr);
> > +     return NOTIFY_DONE;
> > +}
>
> Will this reboot the machine?


No. Just sets the data for the bootloader before the reboot occurs.

> > +     /* Install the notifier */
> > +     restart_nb.notifier_call = reboot_reason;
> > +     restart_nb.priority = 256;
> > +     if (register_restart_handler(&restart_nb)) {
>
> If not, you should use register_reboot_notifier() rather than
> register_restart_handler(). The former gets called to do something
> just before rebooting, while the latter attempts to actually reboot
> the machine.

Ah. Thanks.

> > +static int __init reboot_reason_init(void)
> > +{
> > +     return platform_driver_register(&reboot_reason_driver);
> > +}
> > +arch_initcall(reboot_reason_init);
>
> Why this early? If it can be a normal device_initcall, you can use
> module_platform_driver().

Yea. Mostly its just from the driver I stole the init path from, but I
thought it might be useful to have early. But I'm not passionate about
it one way or the other, so module_platform_driver is fine.


Thanks so much for the review and feedback!

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287202

On Tue, Dec 08, 2015 at 04:13:35PM -0800, John Stultz wrote:
> On Tue, Dec 8, 2015 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tuesday 08 December 2015 13:29:22 John Stultz wrote:
> >
> > > diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > > index 5183d18..ee5dcb7 100644
> > > --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > > +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> > > @@ -282,6 +282,15 @@
> > >                       };
> > >               };
> > >
> > > +             reboot_reason: reboot_reason@2a03f65c {
> > > +                     compatible              = "reboot_reason";
> >
> > This is not a good compatible string. There should generally be a vendor
> > name associated with it (use "linux," if nothing else, and you should have
> > '-' instead of '_'.
> 
> 
> Ack.
> 
> 
> >
> >
> > > +                     reg                     = <0x2A03F65C 0x4>;
> >
> > This may easily conflict with the device it is part of. We should have
> > non-overlapping register areas in general. For the example you are
> > looking at, which register block is this?
> 
> So Bjorn says its IMEM, but I was assuming it was just a reserved
> magic phys addr from the bootloader.
> 
> Ideally I'm hoping to use this same driver for another device, which
> plans to reserve a page from memory that the bootloader won't clear.
> 
> 
> > > +/* Types of reasons */
> > > +static enum {
> > > +     NONE,
> > > +     BOOTLOADER,
> > > +     RECOVERY,
> > > +     OEM,
> > > +     MAX_REASONS
> > > +} __maybe_unused reason_types;
> >
> > The variable seems to always be unused, not just "__maybe_unused". Maybe remove it?
> 
> 
> Yea. I initially just had the empty enum, but the compiler was giving
> me "useless class storage specifier in empty declaration" warnings. So
> I added a variable to it, but then I got unused variable warnings. So
> I ended up with this. :P
> 
> Is there a better way? Are enums for array indexes out of fashion?

They are not, but you have declared a variable (reason_types) which you
don't use. You probably meant to create a enum named reason_types, like
this:

enum reason_types {
	NONE,
	BOOTLOADER,
	RECOVERY,
	OEM,
	MAX_REASONS
};

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287902

On Wed, Dec 9, 2015 at 12:50 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Dec 08, 2015 at 04:13:35PM -0800, John Stultz wrote:
>>
>> Is there a better way? Are enums for array indexes out of fashion?
>
> They are not, but you have declared a variable (reason_types) which you
> don't use. You probably meant to create a enum named reason_types, like
> this:
>
> enum reason_types {
>         NONE,
>         BOOTLOADER,
>         RECOVERY,
>         OEM,
>         MAX_REASONS
> };

So I had tried using a enum name as well, close to what you suggest
here, and still got the "warning: useless storage class specifier in
empty declaration" build warning.

Though trying again, it seems the problem was I was declaring it as
"static enum ...". Removing the static allows it to build w/o warnings
as a unnamed enum structure.  Why, I don't know. :P

Thanks for the pointer!
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286889

On Tue 08 Dec 13:29 PST 2015, John Stultz wrote:

> This patch adds a basic driver to allow for commands like
> "reboot bootloader" and "reboot recovery" to communicate this
> reboot-reason to the bootloader.
> 
> This is commonly done on Android devices, in order to reboot
> the device into fastboot or recovery mode. It also supports
> custom OEM specific commands, via "reboot oem-<value>".
> 
> This driver pulls the phys memory address from DT as well as
> the magic reason values that are written to the address for
> each mode.
> 
> For an example, this patch also adds the DT support for
> the nexus7 device via its dts (which is not yet upstream).
> 
> Thoughts and feedback would be appreciated!
> 

Good to see some work on this, I want it too :)

I do however think this is Qualcomm specific in its implementation, so
adding Andy and linux-arm-msm.

[..]
> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> index 5183d18..ee5dcb7 100644
> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> @@ -282,6 +282,15 @@
>  			};
>  		};
>  
> +		reboot_reason: reboot_reason@2a03f65c {
> +			compatible		= "reboot_reason";
> +			reg			= <0x2A03F65C 0x4>;
> +			reason,none		= <0x77665501>;
> +			reason,bootloader	= <0x77665500>;
> +			reason,recovery		= <0x77665502>;
> +			reason,oem		= <0x6f656d00>;
> +		};
> +

This address refers to IMEM, which is shared with a number of other
uses. So I think we should have a simple-mfd (and syscon) with this
within.


I like the fact that you don't hard code the magics in the
implementation, as I've seen additions from multiple places - so perhaps
it should be made even more flexible.

OMAP seems to use strings here instead of magics, but the delivery
mechanism looks to be the same. But I think of this as Qualcomm
specific.

>  		gpio-keys {
>  			compatible = "gpio-keys";
>  			power {
[..]
> diff --git a/drivers/misc/reboot_reason.c b/drivers/misc/reboot_reason.c
[..]
> +
> +static int reboot_reason(struct notifier_block *nb, unsigned long action,
> +								void *data)
> +{
> +	char *cmd = (char *)data;
> +	long reason = reasons[NONE];
> +
> +	if (!reboot_reason_addr)
> +		return NOTIFY_DONE;
> +
> +	if (cmd != NULL) {
> +		if (!strncmp(cmd, "bootloader", 10))
> +			reason = reasons[BOOTLOADER];
> +		else if (!strncmp(cmd, "recovery", 8))
> +			reason = reasons[RECOVERY];
> +		else if (!strncmp(cmd, "oem-", 4)) {
> +			unsigned long code;
> +
> +			if (!kstrtoul(cmd+4, 0, &code))
> +				reason = reasons[OEM] | (code & 0xff);
> +		}
> +	}

In the case where we didn't find a match you should write the "normal"
reason here, otherwise I think you will end up in recovery when recovery
issues a reboot (in Android that is).

> +
> +	if (reason != -1)
> +		writel(reason, reboot_reason_addr);
> +	return NOTIFY_DONE;
> +}
> +
> +static int reboot_reason_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 val;
> +	int i;
> +
> +	/* initialize the reasons */
> +	for (i = 0; i < MAX_REASONS; i++)
> +		reasons[i] = -1;
> +
> +	/* Try to grab the reason io address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	reboot_reason_addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reboot_reason_addr))
> +		return PTR_ERR(reboot_reason_addr);
> +

Please acquire this memory from a syscon (preferably the the parent
simple-mfd).

> +	/* initialize specified reasons from DT */
> +	if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
> +		reasons[NONE] = val;
> +	if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
> +		reasons[BOOTLOADER] = val;
> +	if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
> +		reasons[RECOVERY] = val;
> +	if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
> +		reasons[OEM] = val;

I would like for this to be less hard coded.

> +
> +	/* Install the notifier */
> +	restart_nb.notifier_call = reboot_reason;
> +	restart_nb.priority = 256;
> +	if (register_restart_handler(&restart_nb)) {
> +		dev_err(&pdev->dev,
> +			"failed to setup restart handler.\n");
> +	}
> +	return 0;
> +}
> +

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286930

On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Tue 08 Dec 13:29 PST 2015, John Stultz wrote:
>
>> This patch adds a basic driver to allow for commands like
>> "reboot bootloader" and "reboot recovery" to communicate this
>> reboot-reason to the bootloader.
>>
>> This is commonly done on Android devices, in order to reboot
>> the device into fastboot or recovery mode. It also supports
>> custom OEM specific commands, via "reboot oem-<value>".
>>
>> This driver pulls the phys memory address from DT as well as
>> the magic reason values that are written to the address for
>> each mode.
>>
>> For an example, this patch also adds the DT support for
>> the nexus7 device via its dts (which is not yet upstream).
>>
>> Thoughts and feedback would be appreciated!
>>
>
> Good to see some work on this, I want it too :)
>
> I do however think this is Qualcomm specific in its implementation, so
> adding Andy and linux-arm-msm.

Right. So this is based off of the qualcomm implementation. But I'm
hoping to reuse it for other hardware.


> [..]
>> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> index 5183d18..ee5dcb7 100644
>> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> @@ -282,6 +282,15 @@
>>                       };
>>               };
>>
>> +             reboot_reason: reboot_reason@2a03f65c {
>> +                     compatible              = "reboot_reason";
>> +                     reg                     = <0x2A03F65C 0x4>;
>> +                     reason,none             = <0x77665501>;
>> +                     reason,bootloader       = <0x77665500>;
>> +                     reason,recovery         = <0x77665502>;
>> +                     reason,oem              = <0x6f656d00>;
>> +             };
>> +
>
> This address refers to IMEM, which is shared with a number of other
> uses. So I think we should have a simple-mfd (and syscon) with this
> within.

Errr.. I'm going to have to read up there. I'm not super familiar with
any of those drivers, so I'll try to see how exactly they would map in
here.


> I like the fact that you don't hard code the magics in the
> implementation, as I've seen additions from multiple places - so perhaps
> it should be made even more flexible.
>
> OMAP seems to use strings here instead of magics, but the delivery
> mechanism looks to be the same. But I think of this as Qualcomm
> specific.

Yea. This is good feedback. EFI bootloaders have their own
implementations as well.  I suspect we'll need a few different driver
"types" to handle these differences, ie: value vs string.

I'll maybe extend the compatible string to make it clear that this is
a "value" style, and we can extend it with a string type later if
folks care?

>> +static int reboot_reason(struct notifier_block *nb, unsigned long action,
>> +                                                             void *data)
>> +{
>> +     char *cmd = (char *)data;
>> +     long reason = reasons[NONE];
>> +
>> +     if (!reboot_reason_addr)
>> +             return NOTIFY_DONE;
>> +
>> +     if (cmd != NULL) {
>> +             if (!strncmp(cmd, "bootloader", 10))
>> +                     reason = reasons[BOOTLOADER];
>> +             else if (!strncmp(cmd, "recovery", 8))
>> +                     reason = reasons[RECOVERY];
>> +             else if (!strncmp(cmd, "oem-", 4)) {
>> +                     unsigned long code;
>> +
>> +                     if (!kstrtoul(cmd+4, 0, &code))
>> +                             reason = reasons[OEM] | (code & 0xff);
>> +             }
>> +     }
>
> In the case where we didn't find a match you should write the "normal"
> reason here, otherwise I think you will end up in recovery when recovery
> issues a reboot (in Android that is).

So, reason is initialized to NONE here. So that should handle this,
no? Or do you mean something more subtle?

>
>> +
>> +     if (reason != -1)
>> +             writel(reason, reboot_reason_addr);
>> +     return NOTIFY_DONE;
>> +}
>> +
>> +static int reboot_reason_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     u32 val;
>> +     int i;
>> +
>> +     /* initialize the reasons */
>> +     for (i = 0; i < MAX_REASONS; i++)
>> +             reasons[i] = -1;
>> +
>> +     /* Try to grab the reason io address */
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     reboot_reason_addr = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(reboot_reason_addr))
>> +             return PTR_ERR(reboot_reason_addr);
>> +
>
> Please acquire this memory from a syscon (preferably the the parent
> simple-mfd).

Ok. Will look into this. Sorry for my unfamiliarity with these details.


>> +     /* initialize specified reasons from DT */
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
>> +             reasons[NONE] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
>> +             reasons[BOOTLOADER] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
>> +             reasons[RECOVERY] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
>> +             reasons[OEM] = val;
>
> I would like for this to be less hard coded.

Any tips here on how to do so?

thanks for all the feedback!
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287322

On Tuesday 08 December 2015 16:22:40 John Stultz wrote:
> >> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> index 5183d18..ee5dcb7 100644
> >> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> @@ -282,6 +282,15 @@
> >>                       };
> >>               };
> >>
> >> +             reboot_reason: reboot_reason@2a03f65c {
> >> +                     compatible              = "reboot_reason";
> >> +                     reg                     = <0x2A03F65C 0x4>;
> >> +                     reason,none             = <0x77665501>;
> >> +                     reason,bootloader       = <0x77665500>;
> >> +                     reason,recovery         = <0x77665502>;
> >> +                     reason,oem              = <0x6f656d00>;
> >> +             };
> >> +
> >
> > This address refers to IMEM, which is shared with a number of other
> > uses. So I think we should have a simple-mfd (and syscon) with this
> > within.
> 
> Errr.. I'm going to have to read up there. I'm not super familiar with
> any of those drivers, so I'll try to see how exactly they would map in
> here.

Is this an SRAM? We already have a generic SRAM binding, and I think this
would fit in there.

> > I like the fact that you don't hard code the magics in the
> > implementation, as I've seen additions from multiple places - so perhaps
> > it should be made even more flexible.
> >
> > OMAP seems to use strings here instead of magics, but the delivery
> > mechanism looks to be the same. But I think of this as Qualcomm
> > specific.
> 
> Yea. This is good feedback. EFI bootloaders have their own
> implementations as well.  I suspect we'll need a few different driver
> "types" to handle these differences, ie: value vs string.
> 
> I'll maybe extend the compatible string to make it clear that this is
> a "value" style, and we can extend it with a string type later if
> folks care?

If the two known implementations are already fundamentally different,
there is a good chance that other vendors have found some more ways
to do the same thing, so we might need to do this as a framework,
or merge it into an existing framework.

Maybe it can be done in the same driver that also handles rebooting
the machine? Those are typically in drivers/power/reset or
in drivers/watchdog/ for machines that use the watchdog as the only
way to reboot the machine. We can have additional device specific
properties along with the reboot method (e.g. a reference to the
SRAM or some syscon node) and add common code in another file if
we need it.

> >> +     /* initialize specified reasons from DT */
> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
> >> +             reasons[NONE] = val;
> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
> >> +             reasons[BOOTLOADER] = val;
> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
> >> +             reasons[RECOVERY] = val;
> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
> >> +             reasons[OEM] = val;
> >
> > I would like for this to be less hard coded.
> 
> Any tips here on how to do so?

If we move this logic into the qcom reset driver in
drivers/power/reset/msm-poweroff.c, we should be good.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288109

On Wed, Dec 9, 2015 at 2:07 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 08 December 2015 16:22:40 John Stultz wrote:
>> >> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> index 5183d18..ee5dcb7 100644
>> >> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> @@ -282,6 +282,15 @@
>> >>                       };
>> >>               };
>> >>
>> >> +             reboot_reason: reboot_reason@2a03f65c {
>> >> +                     compatible              = "reboot_reason";
>> >> +                     reg                     = <0x2A03F65C 0x4>;
>> >> +                     reason,none             = <0x77665501>;
>> >> +                     reason,bootloader       = <0x77665500>;
>> >> +                     reason,recovery         = <0x77665502>;
>> >> +                     reason,oem              = <0x6f656d00>;
>> >> +             };
>> >> +
>> >
>> > This address refers to IMEM, which is shared with a number of other
>> > uses. So I think we should have a simple-mfd (and syscon) with this
>> > within.
>>
>> Errr.. I'm going to have to read up there. I'm not super familiar with
>> any of those drivers, so I'll try to see how exactly they would map in
>> here.
>
> Is this an SRAM? We already have a generic SRAM binding, and I think this
> would fit in there.
>
>> > I like the fact that you don't hard code the magics in the
>> > implementation, as I've seen additions from multiple places - so perhaps
>> > it should be made even more flexible.
>> >
>> > OMAP seems to use strings here instead of magics, but the delivery
>> > mechanism looks to be the same. But I think of this as Qualcomm
>> > specific.
>>
>> Yea. This is good feedback. EFI bootloaders have their own
>> implementations as well.  I suspect we'll need a few different driver
>> "types" to handle these differences, ie: value vs string.
>>
>> I'll maybe extend the compatible string to make it clear that this is
>> a "value" style, and we can extend it with a string type later if
>> folks care?
>
> If the two known implementations are already fundamentally different,
> there is a good chance that other vendors have found some more ways
> to do the same thing, so we might need to do this as a framework,
> or merge it into an existing framework.
>
> Maybe it can be done in the same driver that also handles rebooting
> the machine? Those are typically in drivers/power/reset or
> in drivers/watchdog/ for machines that use the watchdog as the only
> way to reboot the machine. We can have additional device specific
> properties along with the reboot method (e.g. a reference to the
> SRAM or some syscon node) and add common code in another file if
> we need it.

Heh. So my original patch to support this was actually tied into the
ps_hold reboot logic in the pinctrl-msm code:
https://git.linaro.org/people/john.stultz/flo.git/commitdiff/55f28281306af2cc6c61aa001030cb90da096ffa?hp=ad39413ecde7acd39c1f017249b1443ce4ef6812

But realizing I'd like to support this same feature on other hardware,
and we'd be duplicating the logic over and over for each device/reboot
method, it seemed having a fairly generic driver would be better.

Looking at a few other devices, I saw one example that wanted both a
string and a value at the same time, so I probably could extend the
driver to have both reason strings and values, and would set which
ever (or both) were specified. That would cover all the cases I've
seen except the UEFI methods.

(Though I suspect I still have to wrap my head more around the DT
bindings side of things)

>> >> +     /* initialize specified reasons from DT */
>> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
>> >> +             reasons[NONE] = val;
>> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
>> >> +             reasons[BOOTLOADER] = val;
>> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
>> >> +             reasons[RECOVERY] = val;
>> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
>> >> +             reasons[OEM] = val;
>> >
>> > I would like for this to be less hard coded.
>>
>> Any tips here on how to do so?
>
> If we move this logic into the qcom reset driver in
> drivers/power/reset/msm-poweroff.c, we should be good.

If the concern is that since DT is basically ABI, one might not want
to have such a wide interface that specifies all the different
reasons, I can understand that. Though I'm really not sure how else we
would be able to specify the device supported the reboot reason logic
w/o having something in the DT (since some device may use the same soc
w/  the same reboot logic may use a different bootloader which doesn't
support the reason methods). At that point if we don't describe the
method clearly, it ends up being something closer to just a quirks
list which we'd have to map internally to behavior, which doesn't seem
great.

Should we run into hardware that the proposed driver doesn't handle,
we can introduce a new driver for those specific semantics, but this
way we can share at least most of the logic, no?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288548

On Wednesday 09 December 2015 17:19:52 John Stultz wrote:
> On Wed, Dec 9, 2015 at 2:07 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 08 December 2015 16:22:40 John Stultz wrote:
> >> >> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >
> > If the two known implementations are already fundamentally different,
> > there is a good chance that other vendors have found some more ways
> > to do the same thing, so we might need to do this as a framework,
> > or merge it into an existing framework.
> >
> > Maybe it can be done in the same driver that also handles rebooting
> > the machine? Those are typically in drivers/power/reset or
> > in drivers/watchdog/ for machines that use the watchdog as the only
> > way to reboot the machine. We can have additional device specific
> > properties along with the reboot method (e.g. a reference to the
> > SRAM or some syscon node) and add common code in another file if
> > we need it.
> 
> Heh. So my original patch to support this was actually tied into the
> ps_hold reboot logic in the pinctrl-msm code:
> https://git.linaro.org/people/john.stultz/flo.git/commitdiff/55f28281306af2cc6c61aa001030cb90da096ffa?hp=ad39413ecde7acd39c1f017249b1443ce4ef6812
> 
> But realizing I'd like to support this same feature on other hardware,
> and we'd be duplicating the logic over and over for each device/reboot
> method, it seemed having a fairly generic driver would be better.
> 
> Looking at a few other devices, I saw one example that wanted both a
> string and a value at the same time, so I probably could extend the
> driver to have both reason strings and values, and would set which
> ever (or both) were specified. That would cover all the cases I've
> seen except the UEFI methods.
> 
> (Though I suspect I still have to wrap my head more around the DT
> bindings side of things)

The problem is actually something else: from the two machines we looked
at, it's clear that the reboot-reason is not actually something that
is hardware specific, but instead depends on the bootloader. HTC
has its own loader for Tegra, so we can't put the implementation into
the Tegra reboot driver because that wouldn't work on non-HTC machines,
and conversely, another device from HTC that uses a Qualcomm chip might
use the machine vendor specific method rather than the SoC vendor designed
one.

There are actually tons of different ways to do the same thing, including
the PC nvram, the Nokia N900 method that has been discussed to no
end because it relies on ATAGS, IBM has a key-value pair method for
open firmware using NVRAM, and Broadcom uses their own layout on a
number of different devices (nvram, eeprom, NOR-flash, NAND-flash).

> >> >> +     /* initialize specified reasons from DT */
> >> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
> >> >> +             reasons[NONE] = val;
> >> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
> >> >> +             reasons[BOOTLOADER] = val;
> >> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
> >> >> +             reasons[RECOVERY] = val;
> >> >> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
> >> >> +             reasons[OEM] = val;
> >> >
> >> > I would like for this to be less hard coded.
> >>
> >> Any tips here on how to do so?
> >
> > If we move this logic into the qcom reset driver in
> > drivers/power/reset/msm-poweroff.c, we should be good.
> 
> If the concern is that since DT is basically ABI, one might not want
> to have such a wide interface that specifies all the different
> reasons, I can understand that. Though I'm really not sure how else we
> would be able to specify the device supported the reboot reason logic
> w/o having something in the DT (since some device may use the same soc
> w/  the same reboot logic may use a different bootloader which doesn't
> support the reason methods). At that point if we don't describe the
> method clearly, it ends up being something closer to just a quirks
> list which we'd have to map internally to behavior, which doesn't seem
> great.
> 
> Should we run into hardware that the proposed driver doesn't handle,
> we can introduce a new driver for those specific semantics, but this
> way we can share at least most of the logic, no?

I think we need a layered approach, with some high-level code to
store the boot reason, but then support firmware specific backends
to that. If we just need a phandle for an SRAM partition and an offset
within it, that can be done by the high-level driver, but not
any of the more sophisticated communication methods.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288844

On Thu, Dec 10, 2015 at 6:52 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 09 December 2015 17:19:52 John Stultz wrote:
>>
>> If the concern is that since DT is basically ABI, one might not want
>> to have such a wide interface that specifies all the different
>> reasons, I can understand that. Though I'm really not sure how else we
>> would be able to specify the device supported the reboot reason logic
>> w/o having something in the DT (since some device may use the same soc
>> w/  the same reboot logic may use a different bootloader which doesn't
>> support the reason methods). At that point if we don't describe the
>> method clearly, it ends up being something closer to just a quirks
>> list which we'd have to map internally to behavior, which doesn't seem
>> great.
>>
>> Should we run into hardware that the proposed driver doesn't handle,
>> we can introduce a new driver for those specific semantics, but this
>> way we can share at least most of the logic, no?
>
> I think we need a layered approach, with some high-level code to
> store the boot reason, but then support firmware specific backends
> to that. If we just need a phandle for an SRAM partition and an offset
> within it, that can be done by the high-level driver, but not
> any of the more sophisticated communication methods.

Hrm. This feels to me like over-design, though. We already have the
restart notifiers to hook into, which provide the command string. So
its just a matter of parsing the string and writing the appropriate
magic in the appropriate way (to memory, registers, efi, whatever).
The amount of code we'd be dealing with to have a front end and 3-4
back-ends, vs having 3-4 separate drivers seems like it would almost
be the same. So why try to make a more complicated infrastructure?

Simply renaming this driver to reboot_reason_sram.c or something makes
it easy to add reboot_reason_efi.c later.

The other part is, while there are many bits of hardware that have
done varied things in the past, I'm not sure how hard we should try to
design a super-infrastructure to support all these different solutions
if no one is pushing them upstream. I'd rather design for what people
are working to merge (admittedly, that's a bit selfish of a statement
here ;), and then hope future hardware chooses to use the same
mechanism, or adapt the infrastructure as folks try to merge new
approaches.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288903

On Thu, Dec 10, 2015 at 12:56 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Dec 10, 2015 at 6:52 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wednesday 09 December 2015 17:19:52 John Stultz wrote:
>>>
>>> If the concern is that since DT is basically ABI, one might not want
>>> to have such a wide interface that specifies all the different
>>> reasons, I can understand that. Though I'm really not sure how else we
>>> would be able to specify the device supported the reboot reason logic
>>> w/o having something in the DT (since some device may use the same soc
>>> w/  the same reboot logic may use a different bootloader which doesn't
>>> support the reason methods). At that point if we don't describe the
>>> method clearly, it ends up being something closer to just a quirks
>>> list which we'd have to map internally to behavior, which doesn't seem
>>> great.
>>>
>>> Should we run into hardware that the proposed driver doesn't handle,
>>> we can introduce a new driver for those specific semantics, but this
>>> way we can share at least most of the logic, no?
>>
>> I think we need a layered approach, with some high-level code to
>> store the boot reason, but then support firmware specific backends
>> to that. If we just need a phandle for an SRAM partition and an offset
>> within it, that can be done by the high-level driver, but not
>> any of the more sophisticated communication methods.
>
> Hrm. This feels to me like over-design, though. We already have the
> restart notifiers to hook into, which provide the command string. So
> its just a matter of parsing the string and writing the appropriate
> magic in the appropriate way (to memory, registers, efi, whatever).
> The amount of code we'd be dealing with to have a front end and 3-4
> back-ends, vs having 3-4 separate drivers seems like it would almost
> be the same. So why try to make a more complicated infrastructure?

The fact that we are using notifiers for reset reason and triggering
is probably some indication that some infrastructure is needed. But I
don't think you need to do that here as long as it is all kernel
internals. We'll make the 2nd guy do it. ;)

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288950

On Thu, Dec 10, 2015 at 12:24 PM, Rob Herring <robh+dt@kernel.org> wrote:
> The fact that we are using notifiers for reset reason and triggering
> is probably some indication that some infrastructure is needed. But I
> don't think you need to do that here as long as it is all kernel
> internals. We'll make the 2nd guy do it. ;)

:)

Though, just so I understand better, what is problematic w/ the reset
notifiers? They provide the reboot command argument, which is the core
of what is needed. It actually seemed like it was almost designed with
this problem in mind.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288972

On Thursday 10 December 2015 13:43:16 John Stultz wrote:
> On Thu, Dec 10, 2015 at 12:24 PM, Rob Herring <robh+dt@kernel.org> wrote:
> > The fact that we are using notifiers for reset reason and triggering
> > is probably some indication that some infrastructure is needed. But I
> > don't think you need to do that here as long as it is all kernel
> > internals. We'll make the 2nd guy do it. 
> 
> 
> 
> Though, just so I understand better, what is problematic w/ the reset
> notifiers? They provide the reboot command argument, which is the core
> of what is needed. It actually seemed like it was almost designed with
> this problem in mind.

Notifiers in general are a bit of a kludge. We often use them in places
that have not been abstracted well enough yet, and they make it
less obvious what is actually going on when something happens, or
in what order things are called.

I'm actually less worried about the notifier side here than about
the general problem of the communication channel. The reboot reason
is only one of a number of things that the kernel needs to communicate
to the boot loader. Other things may include:

- boot device
- location of the kernel
- command line
- properties of the /chosen DT node in general
- boot scripts
- ethernet MAC addresses
- bootloader console configuration

Every bootloader is different here regarding what can be configured
and how we do it. Often the configuration is done entirely from user
space, but some platforms have kernel support. So picking one particular
aspect and trying to unify that one with a common kernel interface
but ignoring all the others may cause problems if we later want to
add a more general abstraction.

It also looks like at least some of the interfaces require a checksum
to be updated, or are based on variable-length entries, both of which
require a proper driver.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288113

On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Tue 08 Dec 13:29 PST 2015, John Stultz wrote:
>> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> index 5183d18..ee5dcb7 100644
>> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> @@ -282,6 +282,15 @@
>>                       };
>>               };
>>
>> +             reboot_reason: reboot_reason@2a03f65c {
>> +                     compatible              = "reboot_reason";
>> +                     reg                     = <0x2A03F65C 0x4>;
>> +                     reason,none             = <0x77665501>;
>> +                     reason,bootloader       = <0x77665500>;
>> +                     reason,recovery         = <0x77665502>;
>> +                     reason,oem              = <0x6f656d00>;
>> +             };
>> +
>
> This address refers to IMEM, which is shared with a number of other
> uses. So I think we should have a simple-mfd (and syscon) with this
> within.

So talking with Arnd some more it looked like IMEM was really just
SRAM. Is that not the case, or is there something else special about
it?  Does it really need simple-mfd and syscon? I'm still fuzzy on how
to use those for this.

>> +     /* initialize specified reasons from DT */
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,none", &val))
>> +             reasons[NONE] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,bootloader", &val))
>> +             reasons[BOOTLOADER] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,recovery", &val))
>> +             reasons[RECOVERY] = val;
>> +     if (!of_property_read_u32(pdev->dev.of_node, "reason,oem", &val))
>> +             reasons[OEM] = val;
>
> I would like for this to be less hard coded.

So thinking of this more. Is having something like:

cmds = "default", "bootloader", "recovery";
vals  = <0xmagic1>, <0xmagic2>, <0xmagic3>;

what you're thinking about?

This wouldn't quite handle the "oem-N" options as simply, but they
could define each oem- case explicitly in the DT to support it.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288374

On Wednesday 09 December 2015 17:32:02 John Stultz wrote:
> On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > On Tue 08 Dec 13:29 PST 2015, John Stultz wrote:
> >> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> index 5183d18..ee5dcb7 100644
> >> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> >> @@ -282,6 +282,15 @@
> >>                       };
> >>               };
> >>
> >> +             reboot_reason: reboot_reason@2a03f65c {
> >> +                     compatible              = "reboot_reason";
> >> +                     reg                     = <0x2A03F65C 0x4>;
> >> +                     reason,none             = <0x77665501>;
> >> +                     reason,bootloader       = <0x77665500>;
> >> +                     reason,recovery         = <0x77665502>;
> >> +                     reason,oem              = <0x6f656d00>;
> >> +             };
> >> +
> >
> > This address refers to IMEM, which is shared with a number of other
> > uses. So I think we should have a simple-mfd (and syscon) with this
> > within.
> 
> So talking with Arnd some more it looked like IMEM was really just
> SRAM. Is that not the case, or is there something else special about
> it?  Does it really need simple-mfd and syscon? I'm still fuzzy on how
> to use those for this.

If it's SRAM, we should use the SRAM binding and not make it a syscon
device. What we can have however, is a mostly somewhat reboot-reason
driver that is able to access an SRAM device or something else,
depending on what the platform and/or bootloader has.

HTC's Nexus 9 apparently uses a section of normal RAM for communication
between bootloader and kernel, so we'd also need a way to hook into
a driver for that.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288396

On Thu, Dec 10, 2015 at 11:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 09 December 2015 17:32:02 John Stultz wrote:
>> On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
>> <bjorn.andersson@sonymobile.com> wrote:
>> > On Tue 08 Dec 13:29 PST 2015, John Stultz wrote:
>> >> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> index 5183d18..ee5dcb7 100644
>> >> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
>> >> @@ -282,6 +282,15 @@
>> >>                       };
>> >>               };
>> >>
>> >> +             reboot_reason: reboot_reason@2a03f65c {
>> >> +                     compatible              = "reboot_reason";
>> >> +                     reg                     = <0x2A03F65C 0x4>;
>> >> +                     reason,none             = <0x77665501>;
>> >> +                     reason,bootloader       = <0x77665500>;
>> >> +                     reason,recovery         = <0x77665502>;
>> >> +                     reason,oem              = <0x6f656d00>;
>> >> +             };
>> >> +
>> >
>> > This address refers to IMEM, which is shared with a number of other
>> > uses. So I think we should have a simple-mfd (and syscon) with this
>> > within.
>>
>> So talking with Arnd some more it looked like IMEM was really just
>> SRAM. Is that not the case, or is there something else special about
>> it?  Does it really need simple-mfd and syscon? I'm still fuzzy on how
>> to use those for this.
>
> If it's SRAM, we should use the SRAM binding and not make it a syscon
> device. What we can have however, is a mostly somewhat reboot-reason
> driver that is able to access an SRAM device or something else,
> depending on what the platform and/or bootloader has.
>
> HTC's Nexus 9 apparently uses a section of normal RAM for communication
> between bootloader and kernel, so we'd also need a way to hook into
> a driver for that.
>
>         Arnd

Intel uses EFI variables for that on  some AOS platforms. There is a
need for persistent storage abstraction and generalize the reboot
reasons strings.
Second, I wonder why this is submitted under drivers/misc when it
doesn't bind the misc API.

Thanks
Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288856

On Thu, Dec 10, 2015 at 1:20 AM, Tomas Winkler <tomasw@gmail.com> wrote:
> Intel uses EFI variables for that on  some AOS platforms. There is a
> need for persistent storage abstraction and generalize the reboot
> reasons strings.

Yea. I've been told there isn't any sort of standardized method for
EFI here. But I have seen a few different implementations floating
around.

One of the machines I want to support with this driver is actually
using a UEFI bootloader, but we don't have separate storage to use to
communicate back via the UEFI methods, so the ram based approach looks
like the best solution.

> Second, I wonder why this is submitted under drivers/misc when it
> doesn't bind the misc API.

Heh. Apologies. Its more of a "where do I put this?", misc rather then
"this should be part of the established misc infrastructure!"
Suggestions for alternative locations?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web