On Thu, Aug 30, 2018 at 08:42:23PM +0200, Marek Behun wrote:
> On Thu, 30 Aug 2018 09:28:53 -0700
> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> > > .../bindings/watchdog/armada-37xx-wdt.txt | 23 ++
> >
> > Deviceptree properties need to be in a separate patch and have to be
> > approved by devicetree maintainers.
>
> OK :) I will divide the patch. Should I add devicetree maintainers to
> Cc? Who is then responsible for applying the devicetree property, them
> or you?
>
I (or rather Wim) will appy it, it just needs a Reviewed-by: from a DT
maintainer.
> > > +config ARMADA_37XX_WATCHDOG
> > > + tristate "Armada 37xx watchdog"
> > > + depends on ARCH_MVEBU || COMPILE_TEST
> >
> > Did you test this with a couple of architectures ?
>
> This watchdog si specific to Armada 3720, which currently means
> EspressoBin and Turris Mox. I don't think another vendor would create a
> SOC with same structure for watchdog as this. I also don't have access
> to other boards. This works on EspressoBin and Turris Mox.
>
I should have said "build test". Did you test-build the driver with other
architectures ?
> > > +static unsigned int timeout = WATCHDOG_TIMEOUT;
> >
> > This defeats the purpose of using watchdog_init_timeout() to get a
> > value from devicetree. That value will never be used.
>
> I shall rewrite this in the next version.
>
> > > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > > +{
> > > + u64 val;
> > > +
> > > + val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > + val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);
> >
> > Is this guaranteed to be consistent ? What happens if there is a
> > 32-bit wrap between those two operations ?
>
> hmmm. The address is not divisible by 8, so I can't use readq :( what
> do you propose?
>
Re-read high after reading low ?
> > > +static int armada_37xx_wdt_ping(struct watchdog_device *wdt)
> > > +{
> > > + struct armada_37xx_watchdog *dev =
> > > watchdog_get_drvdata(wdt); +
> > > + armada_37xx_wdt_counter_disable(dev);
> > > + set_counter_value(dev);
> > > + armada_37xx_wdt_counter_enable(dev);
> > > +
> >
> > Is it really necessary to stop the watchdog for each ping ? That
> > leaves the system in a vulnerable state. Or does CNTR_CTRL_ENABLE
> > only enable writing into the counter register ?
>
> Well, it probably is possible to do this without stopping the watchdog,
> but two counters would need to be used for this. A trigger can be set
> to counter 1 to start counting from initial value when counter 2 ended.
> I will have to try if it works, though.
>
> > > +static int armada_37xx_wdt_start(struct watchdog_device *wdt)
> > > +{
> > > + struct armada_37xx_watchdog *dev =
> > > watchdog_get_drvdata(wdt);
> > > + u32 reg;
> > > +
> > > + reg = readl(dev->reg + CNTR_CTRL);
> > > +
> > > + if (reg & CNTR_CTRL_ACTIVE)
> > > + return -EBUSY;
> >
> > This is highly unusual. What problem are you solving here ?
>
> Hmm, you are right, this should not happen if the rest of this driver
> works well so that code is redundant.
>
> > > + ret = clk_prepare_enable(dev->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + dev->clk_rate = clk_get_rate(dev->clk);
> >
> > Some clock drivers can return 0 here, which would result in a
> > division by 0 later. It is prudent to check the return value.
>
> Yes, I shall do that.
>
> > > +
> > > + /*
> > > + * Since the timeout in seconds is given as 32 bit
> > > unsigned int, and
> > > + * the counters hold 64 bit values, even after
> > > multiplication by clock
> > > + * rate the counter can hold timeout of UINT_MAX seconds.
> > > + */
> > > + dev->wdt.min_timeout = 0;
> >
> > That value is set to 1 above, which makes more sense. What is the
> > point of setting it to 0 here, and what is the impact of setting the
> > actual timeout to 0 ? Are you sure that doesn't result in an
> > immediate reboot ?
>
> Yes, setting it to 0 does immediate reset. The = 1 line is redundant,
> sorry.
This does not make sense. If you want to implement a reset function,
do it. Don't mis-use the watchdog API for it.
>
> > > + armada_37xx_wdt_set_timeout(&dev->wdt, dev->wdt.timeout);
> > > +
> > > + if (armada_37xx_wdt_is_running(dev))
> > > + set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
> > > + else
> > > + armada_37xx_wdt_stop(&dev->wdt);
> >
> > Why stop it if it isn't running ?
>
> There are 4 counters and every one of them can be set to be a watchdog
> counter. This driver, as described in the commit message, works with
> counter ID 1. The armada_37xx_wdt_stop unsets all 4 counters from
> being watchdog counters, so that if something, for example u-boot, set
> another counter as watchdog counter, the system would not reboot.
>
> If you think this shouldn't be done I shall take it away, and rewrite
> armada_37xx_wdt_stop to only touch counter ID 1.
>
If anything, the counter to be used should be specified in devicetree
instead of being fixed. You should not touch any other counters.
Guenter
