Re: [PATCH] [v3] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi <timur@codeaurora.org>]

Guenter Roeck wrote:

>> +config ARM_SBSA_WDT
>> +    tristate "ARM Server Base System Architecture watchdog"
>> +    depends on ARM64 || COMPILE_TEST
>
> COMPILE_TEST doesn't work with the current code - see below.

Ok, I'll remove it.  I never liked it anyway.

> I still don't understand why you can not use the standard arch_sys_counter
> provided for arm but have to call arm-internal functions directly.
> That seems quite broken to me.

I tried to use clk_get_sys(), but that failed because there are no 
clocks defined on my platform (the 'clocks' list in clkdev.c is empty). 
  I don't think the clock API is fully functional on an ARM ACPI system.

I was originally using functions like arch_timer_read_counter(), but 
that function is not exported, so I couldn't compile my driver as module 
if I used it.

>> +#include <linux/acpi.h>
>> +
>> +/* Watchdog Interface Identification Registers */
>> +struct arm_sbsa_watchdog_ident {
>> +    __le32 w_iidr;    /* Watchdog Interface Identification Register */
>> +    uint8_t res2[0xFE8 - 0xFD0];
>> +    __le32 w_pidr2;    /* Peripheral ID2 Register */
>
> Does this register have any use ?

I don't use it in this driver, but it is part of the hardware.

>> +struct arm_sbsa_watchdog_data {
>> +    struct watchdog_device wdev;
>> +    unsigned int pm_status;
>
> You can use bool here.

Ok.

>> +static int arm_sbsa_wdt_start(struct watchdog_device *wdev)
>> +{
>> +    struct arm_sbsa_watchdog_data *data =
>> +        container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
>> +
>> +    /* Writing to the control register will also reset the counter */
>> +    writel(1, &data->control->wcs);
>> +
>
> Is it ok to always overwrite the entire wcs register ?

Yes.  Writes to bits 1-31 have no affect.

>> +static unsigned int arm_sbsa_wdt_timeleft(struct watchdog_device *wdev)
>> +{
>> +    struct arm_sbsa_watchdog_data *data =
>> +        container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
>> +    uint64_t diff = readq(&data->control->wcv) -
>> arch_counter_get_cntvct();
>> +
> Due to this readq the code doesn't compile on arm,
> nor on any other architecture which does not implement readq.

>> +    return diff / arch_timer_get_cntfrq();
>
> .. and as mentioned before this won't compile on 32 bit targets.

That's why I have in my Kconfig:

	depends on ARM64

It will never be compiled on a 32-bit ARM platform.

>> +static struct watchdog_info arm_sbsa_wdt_info = {
>> +    .options = WDIOF_SETTIMEOUT | _WDIOF_KEEPALIVEPING,
>
>      WDIOF_MAGICCLOSE ?

Is it preferred to enable this feature?  It seems like a policy setting 
that shouldn't be defined by the driver.

>> +    /* We only support architecture version 0 */
>> +    iidr = readl(&data->control->ident.w_iidr);
>> +    if (((iidr >> 16) & 0xf) != 0) {
>> +        dev_info(&pdev->dev,
>> +             "only architecture version 0 is supported\n");
>
> dev_err ?
> It might make sense to display the detected version as well.

Ok.

>> +    watchdog_set_drvdata(&data->wdev, data);
>> +
> You don't ever call watchdog_get_drvdata(), so this is unnecessary.

Ok

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.