On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> Hi Uwe,
>
> thanks for looking into this.
>
> Uwe Kleine-König writes:
> > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> > > This device doesn't provide any information about boot status.
> > > As workaround we use a persitent bit to track watchdog activity.
> >
> > Hmm, so you set a bit in a persistent register iff the watchdog is
> > running. And if that bit is set during probe you assume the watchdog
> > reset the machine. But this also happens, when the user presses the
> > reset button which makes the detection unreliable.
>
> Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> reset button causes the device to power cycle and thus clears the
> persitent bit. So I can't test your case, but surely there are some paths
> into reset, that don't clear the bit and cause false positives.
OK, I found a i.MX28 machine in our repository:
barebox# mw 0x80056080 0xbabecafe
barebox# boot
...
linux# reboot
barebox# md 0x80056080+4
80056080: babecafe
After a power cycle the value disappears (I didn't check the details, if
there is a battery it might be empty).
> I have considered gathering some additional information from the
> power subsystem to refine the result, but decided that it's not
> worth the complexity. Since the power system is a different silicon
> block, we can't rely on it being available and would need to provide
> a fall back implementation anyway.
It would be really nice to get some details about boot rom usage of the
persistent flags in use. I'd expect that these allow to determine the
boot source.
> > > */
> > >
> > > static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> > > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
> > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
> > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> > > } else {
> > > writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
> > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
> > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
> > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> >
> > checkpatch tells:
> >
> > WARNING: line over 80 characters
> > #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115:
> > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> >
> > WARNING: line over 80 characters
> > #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122:
> > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
>
> I tried to follow the style of the surrounding code. If there is a clear
> statement about the preferred style for this driver from whoever is
> responsible, I will happily change my patch to whatever it is.
Yeah, I don't care much. I would have broken the newly introduced lines,
but I'd not go so far to nack your patch when you don't.
> > > }
> > > }
> > >
> > > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
> > > +{
> > > + struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> > > +
> > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> > > +}
> > > +
> > > static struct stmp3xxx_wdt_pdata wdt_pdata = {
> > > .wdt_set_timeout = stmp3xxx_wdt_set_timeout,
> > > + .wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
> > > + .bootstatus = 0,
> > > };
> >
> > This is out of the scope of your patch, but IMHO wdt_pdata should be
> > const in case it is used for >1 device.
>
> Hm, if you want to change wdt_pdata to const, then I can't add the
> bootstatus field. But I think you can't change this to const anyway,
> because the platform_data pointer of struct device is not a const void
> pointer. However didn't look into this in detail, so maybe I'm missing
> something ...
Conceptually platform data should be const for a driver, not necessarily
for the code that sets it up. So the right thing to do is to add a const
to struct device::platform_data. That is usually not in the way during
creation of the device, but stops drivers to modify it.
Then you can do in stmp3xxx_wdt_register:
struct ... pdata = {
.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
.bootstatus = whatever;
};
struct platform_device_info pdevinfo = {
.parent = &rtc_pdev->dev;
.data = pdata;
.size_data = sizeof(pdata);
}
platform_device_register_full(&pdevinfo);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
