[RFC][PATCH] misc: Introduce reboot_reason driver

[RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Arnd Bergmann]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Bjorn Andersson]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Bjorn Andersson]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Bjorn Andersson]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Bjorn Andersson]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Rob Herring]

On Tue, Dec 8, 2015 at 3:29 PM, John Stultz <john.stultz@linaro.org> 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>".

What are some examples of OEM specific commands?

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

Starting with what does the h/w look like, this is typically
implemented with some sort of persistent register(s) either in the SOC
or PMIC. So I think persistent memory/registers is what we need to
describe in DT. Perhaps this could be tied into pstore (an overkill
for a single register, but useful if you already have pstore support
for persistent memory)? The 2nd part is which register to use and the
mapping of values to reboot reason. Do these vary within SOC families?
If not, then we should just hardcode them in the reboot drivers which
are already vendor specific.

Also, while trying to standardize the values for reboot reason
probably won't work short term, we should define something so people
will start using it. We also should consider any implications with
PSCI.

Rob

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Rob Herring]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Tue, Dec 8, 2015 at 1:52 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
<bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Tue, Dec 8, 2015 at 2:26 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Dec 8, 2015 at 3:29 PM, John Stultz <john.stultz@linaro.org> 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>".
>
> What are some examples of OEM specific commands?

Actually, I don't know. That bit was preserved from the 3.4 based
logic in a vendor tree.

I can drop it for now if you'd rather.


>> 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.
>
> Starting with what does the h/w look like, this is typically
> implemented with some sort of persistent register(s) either in the SOC
> or PMIC. So I think persistent memory/registers is what we need to
> describe in DT. Perhaps this could be tied into pstore (an overkill
> for a single register, but useful if you already have pstore support
> for persistent memory)?

Hrm. Yea. I'm hesitant to tie it into pstore, but that's partly due to
not wanting the bootloader to have to parse the pstore area (ideally
the bootloader doesn't touch it).

To me having the bootloader reserve a page of memory and having the
kernel map and write that reserved page makes the most sense to me.
It then being special memory mapped registers or just a reserved page
can be somewhat equivalent.

But maybe I'm being naive?

> The 2nd part is which register to use and the
> mapping of values to reboot reason. Do these vary within SOC families?
> If not, then we should just hardcode them in the reboot drivers which
> are already vendor specific.

I'm not aware of differences between SOC families for the values,
though the addresses might change.

> Also, while trying to standardize the values for reboot reason
> probably won't work short term, we should define something so people
> will start using it.

Ack. Though if the values are mostly custom/magic, is having them
defined in the DT problematic? Or do you just want  macros for
something like something like
   reason,bootloader = <GENERIC_REASON_BOOTLOADER>; ?

> We also should consider any implications with
> PSCI.

Sorry. I'm ignorant here. What would those implications possibly be?

thanks
-john

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Tue, Dec 8, 2015 at 2:26 PM, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Dec 8, 2015 at 3:29 PM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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>".
>
> What are some examples of OEM specific commands?

Actually, I don't know. That bit was preserved from the 3.4 based
logic in a vendor tree.

I can drop it for now if you'd rather.


>> 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.
>
> Starting with what does the h/w look like, this is typically
> implemented with some sort of persistent register(s) either in the SOC
> or PMIC. So I think persistent memory/registers is what we need to
> describe in DT. Perhaps this could be tied into pstore (an overkill
> for a single register, but useful if you already have pstore support
> for persistent memory)?

Hrm. Yea. I'm hesitant to tie it into pstore, but that's partly due to
not wanting the bootloader to have to parse the pstore area (ideally
the bootloader doesn't touch it).

To me having the bootloader reserve a page of memory and having the
kernel map and write that reserved page makes the most sense to me.
It then being special memory mapped registers or just a reserved page
can be somewhat equivalent.

But maybe I'm being naive?

> The 2nd part is which register to use and the
> mapping of values to reboot reason. Do these vary within SOC families?
> If not, then we should just hardcode them in the reboot drivers which
> are already vendor specific.

I'm not aware of differences between SOC families for the values,
though the addresses might change.

> Also, while trying to standardize the values for reboot reason
> probably won't work short term, we should define something so people
> will start using it.

Ack. Though if the values are mostly custom/magic, is having them
defined in the DT problematic? Or do you just want  macros for
something like something like
   reason,bootloader = <GENERIC_REASON_BOOTLOADER>; ?

> We also should consider any implications with
> PSCI.

Sorry. I'm ignorant here. What would those implications possibly be?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Sascha Hauer]

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 |

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Sascha Hauer]

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-r2nGTMty4D4@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Sascha Hauer]

Hi John,

Only a small comment

On Tue, Dec 08, 2015 at 01:29:22PM -0800, John Stultz wrote:
> +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;

can be simplified to:

	of_property_read_u32(pdev->dev.of_node, "reason,none", &reasons[NONE]);

No need to check first, &reasons[NONE] will only be modified when the
property exists.

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 |

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Sascha Hauer]

Hi John,

On Tue, Dec 08, 2015 at 01:29:22PM -0800, 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!

I wonder if 'reason' is the correct word. This driver says nothing about
the reason why we reboot, but more what we should do next. Maybe
something like purpose, scope or intention is more appropriate?

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 |

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Arnd Bergmann]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Wed, Dec 9, 2015 at 2:07 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Arnd Bergmann]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Tomas Winkler]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Arnd Bergmann]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[One Thousand Gnomes]

On Thu, 10 Dec 2015 11:04:03 -0800
John Stultz <john.stultz@linaro.org> wrote:

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

Other than providing a reason (which could be via sysfs) I don't actually
see what in the rest of it doesn't either live in the platform arch/ code
or for standardised firmware in the EFI / ACPI or some other drivers.

sysfs node provides the reason string, reboot notifiers get run before
reboot, and it's up to the platform to decide if it wants to do anything
with the reason string before it hits the switch.

I don't see the need for anything but an extra /sys/power node in the core
kernel ? The rest from a core kernel perspective is already there.

Alan

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

On Thu, Dec 10, 2015 at 11:57 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Thu, 10 Dec 2015 11:04:03 -0800
> John Stultz <john.stultz@linaro.org> wrote:
>> On Thu, Dec 10, 2015 at 1:20 AM, Tomas Winkler <tomasw@gmail.com> wrote:
>> > 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?
>
> Other than providing a reason (which could be via sysfs) I don't actually
> see what in the rest of it doesn't either live in the platform arch/ code
> or for standardised firmware in the EFI / ACPI or some other drivers.

Ok. Will move to drivers/firmware unless suggested otherwise.

thanks
-john

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Rob Herring]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[John Stultz]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Arnd Bergmann]

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

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Rob Herring]

On Thu, Dec 10, 2015 at 4:11 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 10 December 2015 13:43:16 John Stultz wrote:
>> On Thu, Dec 10, 2015 at 12:24 PM, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

Exactly.

> 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:

If only there was a standard interface for firmware variable storage...

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

A major distinction here is many of these would be persistent (i.e. in
flash). As much of this goes into to the DT anyway, a standard way for
userspace to update the DT could solve this. That would not really
work for handling a one time case unless you supported boot with an
alternate DT.

I've also seen storing of SD tuning parameters to avoid tuning at boot
up. Although, the tuning doesn't add that much to the boot time
relative to Android boot, so I don't really see the point.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Rob Herring]

On Thu, Dec 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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.

Exactly.

> 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:

If only there was a standard interface for firmware variable storage...

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

A major distinction here is many of these would be persistent (i.e. in
flash). As much of this goes into to the DT anyway, a standard way for
userspace to update the DT could solve this. That would not really
work for handling a one time case unless you supported boot with an
alternate DT.

I've also seen storing of SD tuning parameters to avoid tuning at boot
up. Although, the tuning doesn't add that much to the boot time
relative to Android boot, so I don't really see the point.

Rob

Re: [RFC][PATCH] misc: Introduce reboot_reason driver

[Bjorn Andersson]

On Wed 09 Dec 17:32 PST 2015, 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.
> 

I'm pretty sure it's just SRAM, but I hadn't looked at that binding
before, sounds like a conceptually better fit.

The part that I was looking for was the convenience of having a regmap
available for the uses that we will find later on, but I guess sram
provides similar means of accessing various pieces of the memory.

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

As these are normally just ascii strings I was thinking we could have
them as individual properties and then use for_each_property_of_node()
on the implementation side. But it doesn't really matter.

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

If we have a reasonably dynamic way of defining these there's little to
no reason to treat oem-N specially from the others.

Regards,
Bjorn