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

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

[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

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

[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

[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

[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

[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

[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