Messages in this thread | | | Subject | Re: [PATCH] watchdog: mpc8xxx: provide boot status | From | Christophe LEROY <> | Date | Fri, 14 Sep 2018 15:48:11 +0200 |
| |
Le 13/09/2018 à 22:25, Guenter Roeck a écrit : > On Thu, Sep 13, 2018 at 08:07:21AM +0000, Christophe Leroy wrote: >> mpc8xxx watchdog driver supports the following platforms: >> - mpc8xx >> - mpc83xx >> - mpc86xx >> >> Those three platforms have a 32 bits register which provides the >> reason of the last boot, including whether it was caused by the >> watchdog. >> >> mpc8xx: Register RSR, bit SWRS (bit 3) >> mpc83xx: Register RSR, bit SWRS (bit 28) >> mpc86xx: Register RSTRSCR, bit WDT_RR (bit 11) >> >> This patch maps the register as defined in the device tree and updates >> wdt.bootstatus based on the value of the watchdog related bit. Then >> the information can be retrieved via the WDIOC_GETBOOTSTATUS ioctl. >> >> Hereunder is an exemple of devicetree for mpc8xx, > > example
ok
> >> the Reset Status Register being at offset 0x288: >> >> WDT: watchdog@0 { >> compatible = "fsl,mpc823-wdt"; >> reg = <0x0 0x10 0x288 0x4>; > > This isn't documented anywhere, and no one wil know how to use it. > So far that was grandfathered in, but with more complex usage > it really needs to be documented.
Ok, added a binding
> >> }; >> >> On the mpc83xx, RSR is at offset 0x910 >> On the mpc86xx, RSTRSCR is at offset 0xe0094 >> >> Suggested-by: Radu Rendec <radu.rendec@gmail.com> >> Tested-by: Christophe Leroy <christophe.leroy@c-s.fr> # On mpc885 >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> --- >> drivers/watchdog/mpc8xxx_wdt.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c >> index aca2d6323f8a..2951a485a6b4 100644 >> --- a/drivers/watchdog/mpc8xxx_wdt.c >> +++ b/drivers/watchdog/mpc8xxx_wdt.c >> @@ -49,10 +49,12 @@ struct mpc8xxx_wdt { >> struct mpc8xxx_wdt_type { >> int prescaler; >> bool hw_enabled; >> + u32 rsr_mask; >> }; >> >> struct mpc8xxx_wdt_ddata { >> struct mpc8xxx_wdt __iomem *base; >> + u32 __iomem *rsr; >> struct watchdog_device wdd; >> spinlock_t lock; >> u16 swtc; >> @@ -137,6 +139,7 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev) >> struct mpc8xxx_wdt_ddata *ddata; >> u32 freq = fsl_get_sys_freq(); >> bool enabled; >> + struct device *dev = &ofdev->dev; > > If you introduce this variable, please use it everywhere > in the function.
ok, introduced it and changed every pr_xxx() to dev_xxx() in a preceeding patch.
> >> >> wdt_type = of_device_get_match_data(&ofdev->dev); >> if (!wdt_type) >> @@ -160,6 +163,22 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev) >> return -ENODEV; >> } >> >> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 1); >> + ddata->rsr = devm_ioremap_resource(dev, res); >> + if (IS_ERR(ddata->rsr)) { >> + dev_info(dev, "Could not map reset status register"); > > Please, no such message. It would start to show up everywhere unless > devicetree files are updated, which likely won't happen. Then we get > bogged down by people asking where this message suddenly comes from.
ok
> >> + } else { >> + u32 rsr_v = in_be32(ddata->rsr); >> + bool status = rsr_v & wdt_type->rsr_mask; >> + >> + ddata->wdd.bootstatus = status ? WDIOF_CARDRESET : 0; >> + /* clear reset status bits related to watchdog time */ >> + out_be32(ddata->rsr, wdt_type->rsr_mask); >> + >> + dev_info(dev, "Last boot was %s by watchdog (RSR = 0x%8.8x)\n", >> + status ? "caused" : "not caused", rsr_v); > > The hex value of RSR may be interesting for developers, but not for users. > Please drop. > > Also, "caused" is redundant. Add it to the base string and add "not " > when needed.
Ok, I did it in v2, allthought I find the code less readable.
> >> + } >> + >> spin_lock_init(&ddata->lock); >> >> ddata->wdd.info = &mpc8xxx_wdt_info, >> @@ -216,6 +235,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = { >> .compatible = "mpc83xx_wdt", >> .data = &(struct mpc8xxx_wdt_type) { >> .prescaler = 0x10000, >> + .rsr_mask = BIT(3), /* RSR Bit 28 */ > > The comment is quite useless. How does BIT(3) match RSR bit 28 ? > I am sure it is because the HW manual counts bits the other way, > but here it is just confusing and thus doesn't add value unless > you provide additional context.
Ok, put the BIT names instead.
Thanks for the review Christophe
> >> }, >> }, >> { >> @@ -223,6 +243,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = { >> .data = &(struct mpc8xxx_wdt_type) { >> .prescaler = 0x10000, >> .hw_enabled = true, >> + .rsr_mask = BIT(20), /* RSTRSCR Bit 11 */ >> }, >> }, >> { >> @@ -230,6 +251,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = { >> .data = &(struct mpc8xxx_wdt_type) { >> .prescaler = 0x800, >> .hw_enabled = true, >> + .rsr_mask = BIT(28), /* RSR Bit 3 */ >> }, >> }, >> {}, >> -- >> 2.13.3 >>
| |