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

On Tue, Dec 8, 2015 at 2:07 PM, Bjorn Andersson
[off-list ref] wrote:

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

quoted

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.

[..]

quoted

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?

quoted

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

quoted

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

quoted

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