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

[Arnd Bergmann <arnd@arndb.de>]

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