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

[John Stultz]

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

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


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

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


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

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

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

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

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

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

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


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

Any tips here on how to do so?

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