Re: [PATCH 2/3] mpc52xx/wdt: merge WDT code into the GPT

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Wed, Nov 11, 2009 at 1:27 AM, Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org> wrote:
> Hi Grant:
>
> Thanks a lot for your comments!
>
>> On Tue, Nov 10, 2009 at 12:41 PM, Albrecht Dreß <albrecht.dress@arcor.de>
>> > +#if defined(HAVE_MPC5200_WDT)
>> > +       /* check if this device could be a watchdog */
>> > +       if (of_get_property(ofdev->node, "fsl,has-wdt", NULL) ||
>> > +           of_get_property(ofdev->node, "has-wdt", NULL)) {
>> > +               const u32 *on_boot_wdt;
>> > +
>> > +               gpt->wdt_mode = MPC52xx_GPT_CAN_WDT;
>> > +
>> > +               /* check if the device shall be used as on-boot watchdog
>> */
>> > +               on_boot_wdt = of_get_property(ofdev->node, "wdt,on-boot",
>> NULL);
>> > +               if (on_boot_wdt) {
>> > +                       gpt->wdt_mode |= MPC52xx_GPT_IS_WDT;
>> > +                       if (*on_boot_wdt > 0 &&
>> > +                           mpc52xx_gpt_wdt_start(gpt, *on_boot_wdt) ==
>> 0)
>> > +                               dev_info(gpt->dev,
>> > +                                        "running as wdt, timeout %us\n",
>> > +                                        *on_boot_wdt);
>> > +                       else
>> > +                               dev_info(gpt->dev, "reserved as wdt\n");
>> > +               } else
>> > +                       dev_info(gpt->dev, "can function as wdt\n");
>> > +       }
>> > +#endif
>>
>> Ditto on the #if/endif
>
> I don't think it can be removed here.  Think about the following:
> - user has a dtb with fsl,has-wdt and fsl,wdt-on-boot properties and
> - disables 5200 WDT in the kernel config.
>
> Now the system refuses to use GPT0 as GPT, although the WDT functionality has been disabled.  Of course, the dts doesn't fit the kernel config now, but I think we should catch this case.

I think the behaviour should be consistent regardless of what support
is compiled into the kernel.  If the wdt-on-boot property is set, the
it does make sense for the kernel to never use it as a GPT.  Otherwise
the kernel behaviour becomes subtly different in ways non-obvious to
the user.

> Actually I tried to implement your requirements from <http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-August/074595.html>:
> - fsl,has-wdt indicates that the GPT can serve as WDT;
> - any access through the wdt api to a wdt-capable gpt actually reserves it as wdt;
> - the new of property forces reserving the wdt before any gpt api function has chance to grab it as gpt.
>
> Or did I get something wrong here?

No, you got it right...  Well, I can't even remember what I said
yesterday, but this list of requirements looks sane.  :-)

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.