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

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

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

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

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

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


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

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

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

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

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

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

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

I would like for this to be less hard coded.

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

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to