Re: [PATCH 1/2] watchdog: Add Cadence WDT driver

[Harini Katakam]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/