Re: [RFC v4 1/4] watchdog: renesas-rwdt: add driver

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]


> > +Required properties:
> > +- compatible : Should be "renesas,rwdt-r8a7795";
> 
> "renesas,r8a7795-rwdt", and "renesas,rcar-gen3-rwdt" as a fallback, to match
> current best practices?

What about "renesas,rcar-gen2-rwdt" since Gen2 and Gen3 are identical?
But why "r8a7795-rwdt" with SoC first? Looking at the r8a7795.dtsi,
"<ip_core>-<soc_type>" seems to be more dominant than
"<soc_type>-<ip_core>"? Ah, confusing again...

> > +Optional properties:
> > +- timeout-sec : Contains the watchdog timeout in seconds
> 
> Isn't this a software configuration thingy? I.e. does it belong it DT?

This is a generic watchdog binding handled by the watchdog core.

> > +#define RWDT_DEFAULT_TIMEOUT 60
> 
> "60U", so no more (hidden) casts needed.

Ah, yes, I can do this now since the module parameter is gone \o/

> > +static int rwdt_start(struct watchdog_device *wdev)
> > +{
> > +       struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +       clk_prepare_enable(priv->clk);
> 
> pm_runtime_get_sync()?
> 
> You also have to call pm_runtime_enable() etc. in probe()/remove().

I'd prefer putting those into probe/remove as well. The restart handler
also accesses the registers, so IMO the clock should be always on.

> > +       clk_prepare_enable(priv->clk);
> > +       rate = clk_get_rate(priv->clk);
> > +       clk_disable_unprepare(priv->clk);
> 
> I think the clk enablement was needed in v2, because you wanted to read the
> RWTCSRA register, but that was already removed in v3?

Aha, clk_get_rate doesn't need the clock prepared? Didn't know that.

> > +       if (!rate)
> > +               return -ENOENT;
> 
> -EINVAL?

Too generic.

> > +       for (i = ARRAY_SIZE(clk_divs); i >= 0; i--) {
> > +               clks_per_sec = rate / clk_divs[i];
> > +               if (clks_per_sec) {
> > +                       priv->clks_per_sec = clks_per_sec;
> > +                       priv->cks = i;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!clks_per_sec) {
> > +               dev_err(&pdev->dev, "Can't find suitable clock divider!\n");
> > +               return -ERANGE;
> 
> -EINVAL?

Ditto.

> > + * 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.
> 
> Isn't there some setup needed on R-Car Gen3, too?

Good news. The latest bootloader update made the Watchdog work on Gen3!
It sets WDTRSTCR (chapter 11.2.4) to 0x8002. Clearing bit 0 unmasks the
reset signal from the RWDT watchdog. Setting bit 15 enforces
initializing BARs on WDT reset. This is the bit we are missing badly on
Gen2 for proper WDT reset AFAICT.

This register can only be changed in secure mode. I recall that those
register are left to the firmware to be setup, or am I wrong? At least
this explains why the BSP team could test my driver without changing
kernel code. They had early access to the new bootloader ;)

However, while testing I found a glitch to be fixed. I hope to have this
tackled today, so I can resend the series.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]