| Message ID | 1517343778-27902-12-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote: > On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore > when suspending to RAM we need to explicitly disable the counting by > clearing TME from RWTCSRA. > Also, on some systems RWDT is the only piece of HW that allows the SoC > to be restarted, therefore this patch implements the restart callback. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> > --- > Wolfram, was the restart callback implementation missing for a reason? > Is its implementation going to break any Gen3 platform? > The changes clearly are way more than claimed in the subject. Adding restart handler and PM support may be prerequisites for Gen2, but the changes apply to Gen3 as well. What happened to "one patch per logical change" ? > v1->v3: > * unified Gen2 and Gen3 drivers. > > drivers/watchdog/renesas_wdt.c | 61 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 56 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c > index 831ef83..eedb016 100644 > --- a/drivers/watchdog/renesas_wdt.c > +++ b/drivers/watchdog/renesas_wdt.c > @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev) > return DIV_BY_CLKS_PER_SEC(priv, 65536 - val); > } > > +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, > + void *data) > +{ > + struct rwdt_priv *priv = watchdog_get_drvdata(wdev); > + > + pm_runtime_get_sync(wdev->parent); > + > + rwdt_write(priv, 0x00, RWTCSRB); > + rwdt_write(priv, 0x00, RWTCSRA); > + rwdt_write(priv, 0xffff, RWTCNT); > + > + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) > + cpu_relax(); > + > + rwdt_write(priv, 0x80, RWTCSRA); > + return 0; > +} > + > static const struct watchdog_info rwdt_ident = { > .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > .identity = "Renesas WDT Watchdog", > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { > .stop = rwdt_stop, > .ping = rwdt_init_timeout, > .get_timeleft = rwdt_get_timeleft, > + .restart = rwdt_restart, > }; > > static int rwdt_probe(struct platform_device *pdev) > @@ -203,13 +222,42 @@ static int rwdt_remove(struct platform_device *pdev) > return 0; > } > > -/* > - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP > - * to work there, one also needs a RESET (RST) driver which does not exist yet > - * due to HW issues. This needs to be solved before adding compatibles here. > - */ Have those issues been resolved ? > +#ifdef CONFIG_PM > +static int rwdt_suspend(struct device *dev) > +{ > + struct platform_device *pdev; > + struct rwdt_priv *priv; > + > + pdev = to_platform_device(dev); > + priv = platform_get_drvdata(pdev); > + if (watchdog_active(&priv->wdev)) { > + rwdt_write(priv, priv->cks, RWTCSRA); > + } > + return 0; > +} > + > +static int rwdt_resume(struct device *dev) > +{ > + struct platform_device *pdev; > + struct rwdt_priv *priv; > + > + pdev = to_platform_device(dev); > + priv = platform_get_drvdata(pdev); > + if (watchdog_active(&priv->wdev)) { > + rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA); > + } > + return 0; > +} > + > +static const struct dev_pm_ops rwdt_pm = { > + .suspend = rwdt_suspend, > + .resume = rwdt_resume, > +}; > +#endif > + > static const struct of_device_id rwdt_ids[] = { > { .compatible = "renesas,rcar-gen3-wdt", }, > + { .compatible = "renesas,rcar-gen2-wdt", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, rwdt_ids); > @@ -218,6 +266,9 @@ static struct platform_driver rwdt_driver = { > .driver = { > .name = "renesas_wdt", > .of_match_table = rwdt_ids, > +#ifdef CONFIG_PM > + .pm = &rwdt_pm, > +#endif > }, > .probe = rwdt_probe, > .remove = rwdt_remove, > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guenter, thank you for your feedback. > Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support > > On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote: > > On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore > > when suspending to RAM we need to explicitly disable the counting by > > clearing TME from RWTCSRA. > > Also, on some systems RWDT is the only piece of HW that allows the SoC > > to be restarted, therefore this patch implements the restart callback. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> > > --- > > Wolfram, was the restart callback implementation missing for a reason? > > Is its implementation going to break any Gen3 platform? > > > > The changes clearly are way more than claimed in the subject. Adding > restart handler and PM support may be prerequisites for Gen2, but the > changes apply to Gen3 as well. What happened to "one patch per logical > change" ? The PM implementation should be ok for Gen3 too, without this patch series the kernel would disable the rwdt clock when suspending to RAM, this would "pause" the counting. This patch series declares the rwdt clock as critical for Gen2 and RZ/G1, which means we need to explicitly disable the counting to keep the same behaviour in place. With respect to the restart callback implementation, that may well have consequences on Gen3, hopefully Wolfram will give us a feedback on this. In particular I would like to know if we should be installing the restart callback only when we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make it available for Gen3 as well. The only way to reboot iWave's boards iwg20d and iwg22d is to use the internal watchdog, that's why the restart implementation was merged into this patch. I can split this patch in two, one for PM support and one for restart support for the next version, what do you think about this? > > > v1->v3: > > * unified Gen2 and Gen3 drivers. > > > > drivers/watchdog/renesas_wdt.c | 61 ++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 56 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c > > index 831ef83..eedb016 100644 > > --- a/drivers/watchdog/renesas_wdt.c > > +++ b/drivers/watchdog/renesas_wdt.c > > @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev) > > return DIV_BY_CLKS_PER_SEC(priv, 65536 - val); > > } > > > > +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, > > +void *data) > > +{ > > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev); > > + > > +pm_runtime_get_sync(wdev->parent); > > + > > +rwdt_write(priv, 0x00, RWTCSRB); > > +rwdt_write(priv, 0x00, RWTCSRA); > > +rwdt_write(priv, 0xffff, RWTCNT); > > + > > +while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) > > +cpu_relax(); > > + > > +rwdt_write(priv, 0x80, RWTCSRA); > > +return 0; > > +} > > + > > static const struct watchdog_info rwdt_ident = { > > .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > > .identity = "Renesas WDT Watchdog", > > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { > > .stop = rwdt_stop, > > .ping = rwdt_init_timeout, > > .get_timeleft = rwdt_get_timeleft, > > +.restart = rwdt_restart, > > }; > > > > static int rwdt_probe(struct platform_device *pdev) > > @@ -203,13 +222,42 @@ static int rwdt_remove(struct platform_device *pdev) > > return 0; > > } > > > > -/* > > - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP > > - * to work there, one also needs a RESET (RST) driver which does not exist yet > > - * due to HW issues. This needs to be solved before adding compatibles here. > > - */ > > Have those issues been resolved ? This patch series addresses precisely those issues. The first implementation used a flag that the watchdog driver needed to explicitly set to make everything work as expected. This new version uses a different concept that doesn't require such an hack, and that's why the proposal now is to merge Gen2 and Gen3 drivers. Thanks, Fab > > > +#ifdef CONFIG_PM > > +static int rwdt_suspend(struct device *dev) > > +{ > > +struct platform_device *pdev; > > +struct rwdt_priv *priv; > > + > > +pdev = to_platform_device(dev); > > +priv = platform_get_drvdata(pdev); > > +if (watchdog_active(&priv->wdev)) { > > +rwdt_write(priv, priv->cks, RWTCSRA); > > +} > > +return 0; > > +} > > + > > +static int rwdt_resume(struct device *dev) > > +{ > > +struct platform_device *pdev; > > +struct rwdt_priv *priv; > > + > > +pdev = to_platform_device(dev); > > +priv = platform_get_drvdata(pdev); > > +if (watchdog_active(&priv->wdev)) { > > +rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA); > > +} > > +return 0; > > +} > > + > > +static const struct dev_pm_ops rwdt_pm = { > > +.suspend = rwdt_suspend, > > +.resume = rwdt_resume, > > +}; > > +#endif > > + > > static const struct of_device_id rwdt_ids[] = { > > { .compatible = "renesas,rcar-gen3-wdt", }, > > +{ .compatible = "renesas,rcar-gen2-wdt", }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, rwdt_ids); > > @@ -218,6 +266,9 @@ static struct platform_driver rwdt_driver = { > > .driver = { > > .name = "renesas_wdt", > > .of_match_table = rwdt_ids, > > +#ifdef CONFIG_PM > > +.pm = &rwdt_pm, > > +#endif > > }, > > .probe = rwdt_probe, > > .remove = rwdt_remove, > > -- > > 2.7.4 > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fabrizio, On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: >> Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support >> On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote: >> > On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore >> > when suspending to RAM we need to explicitly disable the counting by >> > clearing TME from RWTCSRA. >> > Also, on some systems RWDT is the only piece of HW that allows the SoC >> > to be restarted, therefore this patch implements the restart callback. >> > >> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> >> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> >> > --- >> > Wolfram, was the restart callback implementation missing for a reason? >> > Is its implementation going to break any Gen3 platform? >> >> The changes clearly are way more than claimed in the subject. Adding >> restart handler and PM support may be prerequisites for Gen2, but the >> changes apply to Gen3 as well. What happened to "one patch per logical >> change" ? > > The PM implementation should be ok for Gen3 too, without this patch series the kernel > would disable the rwdt clock when suspending to RAM, this would "pause" the counting. > This patch series declares the rwdt clock as critical for Gen2 and RZ/G1, which means we > need to explicitly disable the counting to keep the same behaviour in place. Note that if the kernel crashes after rwdt_suspend(), the watchdog won't restart the system. But that's an issue common to many watchdog driver, right? > With respect to the restart callback implementation, that may well have consequences on > Gen3, hopefully Wolfram will give us a feedback on this. > In particular I would like to know if we should be installing the restart callback only when > we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make it available for > Gen3 as well. IIRC, the reason we don't have the restart callback on R-Car Gen3 is the arm64 maintainers insisting on using PSCI, and vetoing other means, to restart the system. Which leaves us with a few boards where we have to use the watchdog, and wait until the timeout triggers... > The only way to reboot iWave's boards iwg20d and iwg22d is to use the internal watchdog, > that's why the restart implementation was merged into this patch. > I can split this patch in two, one for PM support and one for restart support for the next > version, what do you think about this? Splitting in individual logical changes sounds like a good idea to me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGkgR2VlcnQsDQoNCnRoYW5rIHlvdSBmb3IgeW91ciBmZWVkYmFjayENCg0KPiBTdWJqZWN0OiBS ZTogW1JGQyB2MyAxMS8yNV0gd2F0Y2hkb2c6IHJlbmVzYXNfd2R0OiBBZGQgUi1DYXIgR2VuMiBz dXBwb3J0DQo+DQo+IEhpIEZhYnJpemlvLA0KPg0KPiBPbiBXZWQsIEphbiAzMSwgMjAxOCBhdCAx MTo0NyBBTSwgRmFicml6aW8gQ2FzdHJvDQo+IDxmYWJyaXppby5jYXN0cm9AYnAucmVuZXNhcy5j b20+IHdyb3RlOg0KPiA+PiBTdWJqZWN0OiBSZTogW1JGQyB2MyAxMS8yNV0gd2F0Y2hkb2c6IHJl bmVzYXNfd2R0OiBBZGQgUi1DYXIgR2VuMiBzdXBwb3J0DQo+ID4+IE9uIFR1ZSwgSmFuIDMwLCAy MDE4IGF0IDA4OjIyOjQ0UE0gKzAwMDAsIEZhYnJpemlvIENhc3RybyB3cm90ZToNCj4gPj4gPiBP biBSLUNhciBHZW4yIGFuZCBSWi9HMSB0aGUgcndkdCBjbG9jayBuZWVkcyB0byBiZSBhbHdheXMg T04sIHRoZXJlZm9yZQ0KPiA+PiA+IHdoZW4gc3VzcGVuZGluZyB0byBSQU0gd2UgbmVlZCB0byBl eHBsaWNpdGx5IGRpc2FibGUgdGhlIGNvdW50aW5nIGJ5DQo+ID4+ID4gY2xlYXJpbmcgVE1FIGZy b20gUldUQ1NSQS4NCj4gPj4gPiBBbHNvLCBvbiBzb21lIHN5c3RlbXMgUldEVCBpcyB0aGUgb25s eSBwaWVjZSBvZiBIVyB0aGF0IGFsbG93cyB0aGUgU29DDQo+ID4+ID4gdG8gYmUgcmVzdGFydGVk LCB0aGVyZWZvcmUgdGhpcyBwYXRjaCBpbXBsZW1lbnRzIHRoZSByZXN0YXJ0IGNhbGxiYWNrLg0K PiA+PiA+DQo+ID4+ID4gU2lnbmVkLW9mZi1ieTogRmFicml6aW8gQ2FzdHJvIDxmYWJyaXppby5j YXN0cm9AYnAucmVuZXNhcy5jb20+DQo+ID4+ID4gU2lnbmVkLW9mZi1ieTogUmFtZXNoIFNoYW5t dWdhc3VuZGFyYW0gPHJhbWVzaC5zaGFubXVnYXN1bmRhcmFtQGJwLnJlbmVzYXMuY29tPg0KPiA+ PiA+IC0tLQ0KPiA+PiA+IFdvbGZyYW0sIHdhcyB0aGUgcmVzdGFydCBjYWxsYmFjayBpbXBsZW1l bnRhdGlvbiBtaXNzaW5nIGZvciBhIHJlYXNvbj8NCj4gPj4gPiBJcyBpdHMgaW1wbGVtZW50YXRp b24gZ29pbmcgdG8gYnJlYWsgYW55IEdlbjMgcGxhdGZvcm0/DQo+ID4+DQo+ID4+IFRoZSBjaGFu Z2VzIGNsZWFybHkgYXJlIHdheSBtb3JlIHRoYW4gY2xhaW1lZCBpbiB0aGUgc3ViamVjdC4gQWRk aW5nDQo+ID4+IHJlc3RhcnQgaGFuZGxlciBhbmQgUE0gc3VwcG9ydCBtYXkgYmUgcHJlcmVxdWlz aXRlcyBmb3IgR2VuMiwgYnV0IHRoZQ0KPiA+PiBjaGFuZ2VzIGFwcGx5IHRvIEdlbjMgYXMgd2Vs bC4gV2hhdCBoYXBwZW5lZCB0byAib25lIHBhdGNoIHBlciBsb2dpY2FsDQo+ID4+IGNoYW5nZSIg Pw0KPiA+DQo+ID4gVGhlIFBNIGltcGxlbWVudGF0aW9uIHNob3VsZCBiZSBvayBmb3IgR2VuMyB0 b28sIHdpdGhvdXQgdGhpcyBwYXRjaCBzZXJpZXMgdGhlIGtlcm5lbA0KPiA+IHdvdWxkIGRpc2Fi bGUgdGhlIHJ3ZHQgY2xvY2sgd2hlbiBzdXNwZW5kaW5nIHRvIFJBTSwgdGhpcyB3b3VsZCAicGF1 c2UiIHRoZSBjb3VudGluZy4NCj4gPiBUaGlzIHBhdGNoIHNlcmllcyBkZWNsYXJlcyB0aGUgcndk dCBjbG9jayBhcyBjcml0aWNhbCBmb3IgR2VuMiBhbmQgUlovRzEsIHdoaWNoIG1lYW5zIHdlDQo+ ID4gbmVlZCB0byBleHBsaWNpdGx5IGRpc2FibGUgdGhlIGNvdW50aW5nIHRvIGtlZXAgdGhlIHNh bWUgYmVoYXZpb3VyIGluIHBsYWNlLg0KPg0KPiBOb3RlIHRoYXQgaWYgdGhlIGtlcm5lbCBjcmFz aGVzIGFmdGVyIHJ3ZHRfc3VzcGVuZCgpLCB0aGUgd2F0Y2hkb2cgd29uJ3QNCj4gcmVzdGFydCB0 aGUgc3lzdGVtLiBCdXQgdGhhdCdzIGFuIGlzc3VlIGNvbW1vbiB0byBtYW55IHdhdGNoZG9nIGRy aXZlciwgcmlnaHQ/DQoNCnllYWgsIHRoZXJlIGlzbid0IG11Y2ggd2UgY2FuIGRvIGFib3V0IHRo aXMuIEV2ZW4gd2l0aG91dCByd2R0X3N1c3BlbmQgaW4gcGxhY2UsIGlmIHRoZQ0Ka2VybmVsIGNy YXNoZXMgYWZ0ZXIgZGlzYWJsaW5nIHRoZSByd2R0IGNsb2NrIHRoZSB3YXRjaGRvZyB3b24ndCBy ZXN0YXJ0IHRoZSBzeXN0ZW0uDQoNCj4NCj4gPiBXaXRoIHJlc3BlY3QgdG8gdGhlIHJlc3RhcnQg Y2FsbGJhY2sgaW1wbGVtZW50YXRpb24sIHRoYXQgbWF5IHdlbGwgaGF2ZSBjb25zZXF1ZW5jZXMg b24NCj4gPiBHZW4zLCBob3BlZnVsbHkgV29sZnJhbSB3aWxsIGdpdmUgdXMgYSBmZWVkYmFjayBv biB0aGlzLg0KPiA+IEluIHBhcnRpY3VsYXIgSSB3b3VsZCBsaWtlIHRvIGtub3cgaWYgd2Ugc2hv dWxkIGJlIGluc3RhbGxpbmcgdGhlIHJlc3RhcnQgY2FsbGJhY2sgb25seSB3aGVuDQo+ID4gd2Ug dXNlICJyZW5lc2FzLHJjYXItZ2VuMi13ZHQiIGFzIGNvbXBhdGlibGUgc3RyaW5nLCBvciBpdCdz IG9rIHRvIG1ha2UgaXQgYXZhaWxhYmxlIGZvcg0KPiA+IEdlbjMgYXMgd2VsbC4NCj4NCj4gSUlS QywgdGhlIHJlYXNvbiB3ZSBkb24ndCBoYXZlIHRoZSByZXN0YXJ0IGNhbGxiYWNrIG9uIFItQ2Fy IEdlbjMgaXMgdGhlDQo+IGFybTY0IG1haW50YWluZXJzIGluc2lzdGluZyBvbiB1c2luZyBQU0NJ LCBhbmQgdmV0b2luZyBvdGhlciBtZWFucywNCj4gdG8gcmVzdGFydCB0aGUgc3lzdGVtLg0KPg0K PiBXaGljaCBsZWF2ZXMgdXMgd2l0aCBhIGZldyBib2FyZHMgd2hlcmUgd2UgaGF2ZSB0byB1c2Ug dGhlIHdhdGNoZG9nLCBhbmQNCj4gd2FpdCB1bnRpbCB0aGUgdGltZW91dCB0cmlnZ2Vycy4uLg0K DQpvaywgZm9yIHRoZSBuZXh0IHZlcnNpb24gSSdsbCByZWdpc3RlciB0aGUgcmVzdGFydCBjYWxs YmFjayBvbmx5IHdoZW4gd2UgbWF0Y2gNCiJyZW5lc2FzLHJjYXItZ2VuMi13ZHQiLiBTaW5jZSBj dXJyZW50bHkgdGhlcmUgaXMgbm8gd2F0Y2hkb2cgc3VwcG9ydCBvbiBHZW4yDQpJIGJlbGlldmUg c3VjaCBhIGNoYW5nZSBpcyBwZXJmZWN0bHkgc2FmZS4NCg0KPg0KPiA+IFRoZSBvbmx5IHdheSB0 byByZWJvb3QgaVdhdmUncyBib2FyZHMgaXdnMjBkIGFuZCBpd2cyMmQgaXMgdG8gdXNlIHRoZSBp bnRlcm5hbCB3YXRjaGRvZywNCj4gPiB0aGF0J3Mgd2h5IHRoZSByZXN0YXJ0IGltcGxlbWVudGF0 aW9uIHdhcyBtZXJnZWQgaW50byB0aGlzIHBhdGNoLg0KPiA+IEkgY2FuIHNwbGl0IHRoaXMgcGF0 Y2ggaW4gdHdvLCBvbmUgZm9yIFBNIHN1cHBvcnQgYW5kIG9uZSBmb3IgcmVzdGFydCBzdXBwb3J0 IGZvciB0aGUgbmV4dA0KPiA+IHZlcnNpb24sIHdoYXQgZG8geW91IHRoaW5rIGFib3V0IHRoaXM/ DQo+DQo+IFNwbGl0dGluZyBpbiBpbmRpdmlkdWFsIGxvZ2ljYWwgY2hhbmdlcyBzb3VuZHMgbGlr ZSBhIGdvb2QgaWRlYSB0byBtZS4NCg0KYWdyZWVkIHRoZW4sIEknbGwgc3BsaXQgdGhlIGNvbW1p dCBpbiB0d28gZm9yIHRoZSBuZXh0IHZlcnNpb24uDQoNClRoYW5rcywNCkZhYg0KDQo+DQo+IEdy e29ldGplLGVldGluZ31zLA0KPg0KPiAgICAgICAgICAgICAgICAgICAgICAgICBHZWVydA0KPg0K PiAtLQ0KPiBHZWVydCBVeXR0ZXJob2V2ZW4gLS0gVGhlcmUncyBsb3RzIG9mIExpbnV4IGJleW9u ZCBpYTMyIC0tIGdlZXJ0QGxpbnV4LW02OGsub3JnDQo+DQo+IEluIHBlcnNvbmFsIGNvbnZlcnNh dGlvbnMgd2l0aCB0ZWNobmljYWwgcGVvcGxlLCBJIGNhbGwgbXlzZWxmIGEgaGFja2VyLiBCdXQN Cj4gd2hlbiBJJ20gdGFsa2luZyB0byBqb3VybmFsaXN0cyBJIGp1c3Qgc2F5ICJwcm9ncmFtbWVy IiBvciBzb21ldGhpbmcgbGlrZSB0aGF0Lg0KPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgIC0tIExpbnVzIFRvcnZhbGRzDQoNCg0KDQpSZW5lc2FzIEVsZWN0cm9uaWNzIEV1cm9wZSBM dGQsIER1a2VzIE1lYWRvdywgTWlsbGJvYXJkIFJvYWQsIEJvdXJuZSBFbmQsIEJ1Y2tpbmdoYW1z aGlyZSwgU0w4IDVGSCwgVUsuIFJlZ2lzdGVyZWQgaW4gRW5nbGFuZCAmIFdhbGVzIHVuZGVyIFJl Z2lzdGVyZWQgTm8uIDA0NTg2NzA5Lg0K -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/31/2018 04:13 AM, Geert Uytterhoeven wrote: > Hi Fabrizio, > > On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: >>> Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support >>> On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote: >>>> On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore >>>> when suspending to RAM we need to explicitly disable the counting by >>>> clearing TME from RWTCSRA. >>>> Also, on some systems RWDT is the only piece of HW that allows the SoC >>>> to be restarted, therefore this patch implements the restart callback. >>>> >>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> >>>> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> >>>> --- >>>> Wolfram, was the restart callback implementation missing for a reason? >>>> Is its implementation going to break any Gen3 platform? >>> >>> The changes clearly are way more than claimed in the subject. Adding >>> restart handler and PM support may be prerequisites for Gen2, but the >>> changes apply to Gen3 as well. What happened to "one patch per logical >>> change" ? >> >> The PM implementation should be ok for Gen3 too, without this patch series the kernel >> would disable the rwdt clock when suspending to RAM, this would "pause" the counting. >> This patch series declares the rwdt clock as critical for Gen2 and RZ/G1, which means we >> need to explicitly disable the counting to keep the same behaviour in place. > > Note that if the kernel crashes after rwdt_suspend(), the watchdog won't > restart the system. But that's an issue common to many watchdog driver, right? > If so, that would be buggy. >> With respect to the restart callback implementation, that may well have consequences on >> Gen3, hopefully Wolfram will give us a feedback on this. >> In particular I would like to know if we should be installing the restart callback only when >> we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make it available for >> Gen3 as well. > > IIRC, the reason we don't have the restart callback on R-Car Gen3 is the > arm64 maintainers insisting on using PSCI, and vetoing other means, > to restart the system. > You could just give it lower priority than PSCI. > Which leaves us with a few boards where we have to use the watchdog, and > wait until the timeout triggers... > Which means the veto is counter-productive and thus meaningless. Guenter >> The only way to reboot iWave's boards iwg20d and iwg22d is to use the internal watchdog, >> that's why the restart implementation was merged into this patch. >> I can split this patch in two, one for PM support and one for restart support for the next >> version, what do you think about this? > > Splitting in individual logical changes sounds like a good idea to me. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guenter, Geert thank you for your feedback! > > > > IIRC, the reason we don't have the restart callback on R-Car Gen3 is the > > arm64 maintainers insisting on using PSCI, and vetoing other means, > > to restart the system. > > > > You could just give it lower priority than PSCI. I am looking at the implementation of machine_restart() for both arm and arm64, and it looks like that if arm_pm_restart gets set, then arm_pm_restart has the highest priority of all. If arm_pm_restart is not set, then the kernel will go through the restart_handler_list, and the registered priority for the restart method will matter at that point. My understanding is that Gen3 does set arm_pm_restart due to drivers/firmware/psci.c, and therefore other restart handlers shouldn't matter too much, but I am not familiar with Gen3 at all. Shall I just set the restart priority to 128 and leave the callback in place for both Gen2 and Gen3? Thanks, Fab > > > Which leaves us with a few boards where we have to use the watchdog, and > > wait until the timeout triggers... > > > > Which means the veto is counter-productive and thus meaningless. > > Guenter > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hi Günter, On Wed, Jan 31, 2018 at 3:48 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 01/31/2018 04:13 AM, Geert Uytterhoeven wrote: >> On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro >> <fabrizio.castro@bp.renesas.com> wrote: >>>> Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 >>>> support >>>> On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote: >>>>> >>>>> On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore >>>>> when suspending to RAM we need to explicitly disable the counting by >>>>> clearing TME from RWTCSRA. >>>>> Also, on some systems RWDT is the only piece of HW that allows the SoC >>>>> to be restarted, therefore this patch implements the restart callback. >>>>> >>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> >>>>> Signed-off-by: Ramesh Shanmugasundaram >>>>> <ramesh.shanmugasundaram@bp.renesas.com> >>>>> --- >>>>> Wolfram, was the restart callback implementation missing for a reason? >>>>> Is its implementation going to break any Gen3 platform? >>>> >>>> The changes clearly are way more than claimed in the subject. Adding >>>> restart handler and PM support may be prerequisites for Gen2, but the >>>> changes apply to Gen3 as well. What happened to "one patch per logical >>>> change" ? >>> >>> The PM implementation should be ok for Gen3 too, without this patch >>> series the kernel >>> would disable the rwdt clock when suspending to RAM, this would "pause" >>> the counting. >>> This patch series declares the rwdt clock as critical for Gen2 and RZ/G1, >>> which means we >>> need to explicitly disable the counting to keep the same behaviour in >>> place. >> >> Note that if the kernel crashes after rwdt_suspend(), the watchdog won't >> restart the system. But that's an issue common to many watchdog driver, >> right? >> > If so, that would be buggy. All watchdog drivers implementing a system suspend handler stop the watchdog before suspending the system. I guess if they wouldn't do that, the watchdog would restart the system while asleep? Exceptions are: - atlas7_wdt, sirfsoc_wdt: these seem to rely on another driver stopping the watchdog clock, so it behaves the same as all above, - diag288_wdt: this returns -EBUSY when trying to suspend the system while the watchdog is enabled, to put the burden of disabling the watchdog on the user, - ux500_wdt: this seems to use a hardware feature to automatically disable the watchdog during suspend. So only the last one seems to protect against kernel crashes after the watchdog's suspend method is called... Note that two drivers (iTCO_wdt and wdat_wdt) implements suspend_noirq instead of suspend, which decreases the window of running without a watchdog a bit. >>> With respect to the restart callback implementation, that may well have >>> consequences on >>> Gen3, hopefully Wolfram will give us a feedback on this. >>> In particular I would like to know if we should be installing the restart >>> callback only when >>> we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make >>> it available for >>> Gen3 as well. >> >> IIRC, the reason we don't have the restart callback on R-Car Gen3 is the >> arm64 maintainers insisting on using PSCI, and vetoing other means, >> to restart the system. > > You could just give it lower priority than PSCI. > >> Which leaves us with a few boards where we have to use the watchdog, and >> wait until the timeout triggers... > > Which means the veto is counter-productive and thus meaningless. I do welcome that point of view ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c index 831ef83..eedb016 100644 --- a/drivers/watchdog/renesas_wdt.c +++ b/drivers/watchdog/renesas_wdt.c @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev) return DIV_BY_CLKS_PER_SEC(priv, 65536 - val); } +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, + void *data) +{ + struct rwdt_priv *priv = watchdog_get_drvdata(wdev); + + pm_runtime_get_sync(wdev->parent); + + rwdt_write(priv, 0x00, RWTCSRB); + rwdt_write(priv, 0x00, RWTCSRA); + rwdt_write(priv, 0xffff, RWTCNT); + + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) + cpu_relax(); + + rwdt_write(priv, 0x80, RWTCSRA); + return 0; +} + static const struct watchdog_info rwdt_ident = { .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, .identity = "Renesas WDT Watchdog", @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { .stop = rwdt_stop, .ping = rwdt_init_timeout, .get_timeleft = rwdt_get_timeleft, + .restart = rwdt_restart, }; static int rwdt_probe(struct platform_device *pdev) @@ -203,13 +222,42 @@ static int rwdt_remove(struct platform_device *pdev) return 0; } -/* - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP - * to work there, one also needs a RESET (RST) driver which does not exist yet - * due to HW issues. This needs to be solved before adding compatibles here. - */ +#ifdef CONFIG_PM +static int rwdt_suspend(struct device *dev) +{ + struct platform_device *pdev; + struct rwdt_priv *priv; + + pdev = to_platform_device(dev); + priv = platform_get_drvdata(pdev); + if (watchdog_active(&priv->wdev)) { + rwdt_write(priv, priv->cks, RWTCSRA); + } + return 0; +} + +static int rwdt_resume(struct device *dev) +{ + struct platform_device *pdev; + struct rwdt_priv *priv; + + pdev = to_platform_device(dev); + priv = platform_get_drvdata(pdev); + if (watchdog_active(&priv->wdev)) { + rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA); + } + return 0; +} + +static const struct dev_pm_ops rwdt_pm = { + .suspend = rwdt_suspend, + .resume = rwdt_resume, +}; +#endif + static const struct of_device_id rwdt_ids[] = { { .compatible = "renesas,rcar-gen3-wdt", }, + { .compatible = "renesas,rcar-gen2-wdt", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, rwdt_ids); @@ -218,6 +266,9 @@ static struct platform_driver rwdt_driver = { .driver = { .name = "renesas_wdt", .of_match_table = rwdt_ids, +#ifdef CONFIG_PM + .pm = &rwdt_pm, +#endif }, .probe = rwdt_probe, .remove = rwdt_remove,