Hi,
On Tue, Apr 1, 2014 at 6:02 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 03/28/2014 03:31 AM, Harini Katakam wrote:
>>
>> Add Cadence WDT driver. This is used by Xilinx Zynq.
>>
>> Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx>
>> ---
>> drivers/watchdog/Kconfig | 7 +
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/cadence_wdt.c | 530
>> ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 538 insertions(+)
>> create mode 100644 drivers/watchdog/cadence_wdt.c
>>
<snip>
>> +
>> + spin_lock(&wdt->io_lock);
>> + cdns_wdt_writereg(wdt->regs + CDNS_WDT_ZMR_OFFSET,
>> + CDNS_WDT_ZMR_ZKEY_VAL);
>> +
>> + /* Shift the count value to correct bit positions */
>> + count = (count << 2) & CDNS_WDT_CCR_CRV_MASK;
>> +
>> + /* Write counter access key first to be able write to register */
>> + data = count | CDNS_WDT_REGISTER_ACCESS_KEY | wdt->ctrl_clksel;
>> + cdns_wdt_writereg(wdt->regs + CDNS_WDT_CCR_OFFSET, data);
>> + data = CDNS_WDT_ZMR_WDEN_MASK | CDNS_WDT_ZMR_RSTLEN_16 |
>> + CDNS_WDT_ZMR_ZKEY_VAL;
>> +
>> + /* Reset on timeout if specified in device tree. */
>> + if (wdt->rst) {
>> + data |= CDNS_WDT_ZMR_RSTEN_MASK;
>> + data &= ~CDNS_WDT_ZMR_IRQEN_MASK;
>> + } else {
>> + data &= ~CDNS_WDT_ZMR_RSTEN_MASK;
>> + data |= CDNS_WDT_ZMR_IRQEN_MASK;
>> + }
>> + cdns_wdt_writereg(wdt->regs + CDNS_WDT_ZMR_OFFSET, data);
>> + spin_unlock(&wdt->io_lock);
>> + cdns_wdt_writereg(wdt->regs + CDNS_WDT_RESTART_OFFSET,
>> + CDNS_WDT_RESTART_KEY);
>> +
>
> Sometimes you write into this register with lock protection, here without.
>
> How comes (or, in other words, why does the write have to be spinlock
> protected above but not here) ?
>
CDNS_WDT_RESTART_OFFSET needs to be written here with lock protection.
I missed it because it is in wdt_start, but i realise it should be protected.
Will correct that.
<snip>
>> + /* Initialize the members of cdns_wdt structure */
>> + cdns_wdt_device->parent = &pdev->dev;
>> + of_get_property(pdev->dev.of_node, "timeout-sec",
>> + &cdns_wdt_device->timeout);
>> + if (wdt_timeout < CDNS_WDT_MAX_TIMEOUT &&
>> + wdt_timeout > CDNS_WDT_MIN_TIMEOUT)
>
>
> Why are CDNS_WDT_MAX_TIMEOUT and CDNS_WDT_MIN_TIMEOUT not acceptable ?
> Why don't you use watchdog_init_timeout() anyway ?
>
I'll use watchdog_init_timeout.
>
>> + cdns_wdt_device->timeout = wdt_timeout;
>> + else
>> + dev_info(&pdev->dev,
>> + "timeout limited to 1 - %d sec, using
>> default=%d\n",
>> + CDNS_WDT_MAX_TIMEOUT, CDNS_WDT_DEFAULT_TIMEOUT);
>> +
>> + watchdog_set_nowayout(cdns_wdt_device, nowayout);
>> + watchdog_set_drvdata(cdns_wdt_device, wdt);
>> +
>
>
> Set too late (see other e-mail)
>
Will correct order in probe as pointed in other mails.
Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
