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