watchdog: introduce the ARM64 SBSA watchdog driver
PATCH v2 1/1 watchdog: introduce the ARM64 SBSA watchdog driver
The ARM Server Base System Architecture is a specification for ARM-based server systems. Among other things, it defines the behavior and register interface for a watchdog timer.
---
[v2]
added PM support
now depends on Fu Wei's "ACPI: import watchdog info of GTDT into platform device"
patch (GTDT platform code removed)
driver compiles as a module
replaced ARM-specific timer references
hardware registers now marked as little-endian
add OF support
drivers/watchdog/Kconfig | 9 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/arm_sbsa_wdt.c | 303 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 313 insertions(+) create mode 100644 drivers/watchdog/arm_sbsa_wdt.c
drivers/watchdog/Kconfig +9 -0
@@ -514,6 +514,15 @@ config MEDIATEK_WATCHDOG To compile this driver as a module, choose M here: the module will be called mtk_wdt. +config ARM_SBSA_WDT + tristate "ARM Server Base System Architecture watchdog" + depends on ARM64 || COMPILE_TEST + depends on ARCH_TIMER + select WATCHDOG_CORE + help + Say Y here to include watchdog timer support for ARM Server Base + System Architecture (SBSA) systems. + # AVR32 Architecture config AT32AP700X_WDT
drivers/watchdog/Makefile +1 -0
@@ -64,6 +64,7 @@ obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o +obj-$(CONFIG_ARM_SBSA_WDT) += arm_sbsa_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
drivers/watchdog/arm_sbsa_wdt.c new file +303 -0
303 lines not on this page · raw message
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
drivers/watchdog/arm_sbsa_wdt.c line 100
Quoted 11 lines · On Thursday 14 May 2015 11:52:33 Timur Tabi wrote:
drivers/watchdog/arm_sbsa_wdt.c new file +11
-0+static int arm_sbsa_wdt_set_timeout (struct watchdog_device *wdev, +unsigned int timeout) +{ +struct arm_sbsa_watchdog_data *data = + container_of(wdev, struct arm_sbsa_watchdog_data, wdev); +uint32_t wor; + + wdev->timeout = timeout; + + wor =arch_timer_get_cntfrq () * wdev->timeout; +writel (cpu_to_le32 (wor), &data->control->wor);
Why the double endianess swap? Are the registers defined to be CPU-endian?
You probably mean le32_to_cpu() here, not the other way round, although the effect is the same.
Arnd
Quoted 5 lines · Arnd Bergmann
Quoted 1 line
Quoted 1 line
+
writel(cpu_to_le32(wor), &data->control->wor);Why the double endianess swap? Are the registers defined to be CPU-endian?
You probably mean
le32_to_cpu()here, not the other way round, although the effect is the same.
Maybe I'd doing the whole __le32 thing wrong, but I think cpu_to_le32(wor) is correct. 'wor' is a normal integer, so it's in "cpu" format. I want to convert it to le32 format so that I write an le32 value to memory. Therefore, cpu_to_le32(). Most calls to writel() use cpu_to_le32():
$ git grep -l "writel(cpu_to_le32" arch/m68k/coldfire/pci.c arch/mips/pci/ops-bcm63xx.c arch/mips/pci/ops-bonito64.c arch/mips/pci/ops-loongson2.c arch/parisc/include/asm/io.h drivers/atm/fore200e.c drivers/block/umem.c drivers/isdn/hardware/mISDN/hfcmulti.c drivers/mtd/nand/r852.c drivers/net/ethernet/brocade/bna/bfa_ioc.c drivers/parisc/eisa.c drivers/scsi/bfa/bfa_ioc.c drivers/scsi/bnx2fc/bnx2fc_hwi.c drivers/scsi/bnx2i/bnx2i_hwi.c drivers/scsi/hptiop.c drivers/scsi/mpt2sas/mpt2sas_base.c drivers/scsi/mpt3sas/mpt3sas_base.c drivers/scsi/nsp32_io.h drivers/staging/i2o/i2o.h drivers/usb/isp1760/isp1760-hcd.c sound/pci/mixart/mixart_hwdep.h
$ git grep -l "writel(le32_to_cpu" drivers/scsi/hpsa.c drivers/scsi/megaraid/megaraid_sas_fusion.c drivers/virtio/virtio_mmio.c
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Guenter Roeck
maintainerQuoted 4 lines · Timur Tabi
Quoted 3 lines · Arnd Bergmann
Quoted 1 line
Quoted 1 line
+
writel(cpu_to_le32(wor), &data->control->wor);Why the double endianess swap? Are the registers defined to be CPU-endian?
Quoted 8 lines
Quoted 2 lines
You probably mean
le32_to_cpu()here, not the other way round, although the effect is the same.Maybe I'd doing the whole
__le32thing wrong, but I thinkcpu_to_le32(wor)is correct. 'wor' is a normal integer, so it's in "cpu" format. I want to convert it to le32 format so that I write an le32 value to memory. Therefore,cpu_to_le32(). Most calls towritel()usecpu_to_le32():
What other drivers do is completely irrelevant. The key question is what _this_ hardware does, and you should know that.
It concerns me that you point to other drivers as argument for using the endianness conversion your code. That raises the question if you really understand the hardware. Is the register known to be a little endian register, even on a big endian machine, or is it in host byte order ? You don't really answer that question.
Guenter
Quoted 3 lines · Guenter Roeck
It concerns me that you point to other drivers as argument for using the endianness conversion your code. That raises the question if you really understand the hardware.
The only information I have on how the hardware works is the SBSA document and the internal documentation for our ARM64 server chip.
Quoted 3 lines
Is the register known to be a little endian register,
even on a big endian machine, or is it in host byte order ? You don't really answer that question.
The ARM SBSA says in section 4.1.1:
* CPUs shall implement little-endian support.
That's the only reference to endianness in the document. I don't know if that means that the device registers must also be little-endian. Does Linux even support running ARM64 in big-endian mode?
So maybe it's safe to say that everything must be little endian? If so, do I still need the __le32 in the structs, and should I still use cpu_to_le32()?
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Guenter Roeck
maintainerQuoted 7 lines · Timur Tabi
Quoted 3 lines · Guenter Roeck
It concerns me that you point to other drivers as argument for using the endianness conversion your code. That raises the question if you really understand the hardware.
The only information I have on how the hardware works is the SBSA document and the internal documentation for our ARM64 server chip.
Quoted 8 lines
Quoted 3 lines
Is the register known to be a little endian register,
even on a big endian machine, or is it in host byte order ? You don't really answer that question.The ARM SBSA says in section 4.1.1:
* CPUs shall implement little-endian support.
That doesn't really say anything. It neither says that the CPU must _run_ in little endian mode, nor does it say anything about its register byte order.
Quoted 2 lines
That's the only reference to endianness in the document. I don't know if that means that the device registers must also be little-endian. Does Linux even support running ARM64 in big-endian mode?
Interesting question. You should be able to answer it yourself.
Hint: Look in arch/arm64/Kconfig.
Quoted 2 lines
So maybe it's safe to say that everything must be little endian? If so, do I still need the
__le32in the structs, and should I still usecpu_to_le32()?
We still don't know if the registers are in host byte order or in little endian order. If registers are in host byte order, there is no need for a conversion.
Guenter
Quoted 5 lines · Guenter Roeck
We still don't know if the registers are in host byte order or in little endian
order. If registers are in host byte order, there is no need for a conversion.
Well, if I compile and boot a big-endian ARM64 kernel, then I don't expect the hardware devices to magically switch all their registers into big endian. The devices will remain little-endian.
I presume that this is true on all ARM and ARM64 systems. Switching the CPU into big-endian mode (when it normally would run in little-endian) does not also switch the hardware registers.
Therefore, if we want to support big-endian kernels on ARM64, then every readl/writel in every driver must use cpu_to_le32/etc.
I think this conversation has gone off-track. I don't see how the SBSA watchdog device is any different from any other device on an ARM64 platform.
I think it's safe to say that the endian order is not specified anywhere, but on my hardware, everything is little-endian.
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Quoted 15 lines · On Thursday 14 May 2015 18:49:17 Timur Tabi wrote:
Quoted 5 lines · Guenter Roeck
We still don't know if the registers are in host byte order or in little endian
order. If registers are in host byte order, there is no need for a conversion.Well, if I compile and boot a big-endian ARM64 kernel, then I don't expect the hardware devices to magically switch all their registers into big endian. The devices will remain little-endian.
I presume that this is true on all ARM and ARM64 systems. Switching the CPU into big-endian mode (when it normally would run in little-endian) does not also switch the hardware registers.
Quoted 8 lines
Therefore, if we want to support big-endian kernels on ARM64, then every readl/writel in every driver must use cpu_to_le32/etc.
I think this conversation has gone off-track. I don't see how the SBSA watchdog device is any different from any other device on an ARM64 platform.
I think it's safe to say that the endian order is not specified anywhere, but on my hardware, everything is little-endian.
Then please remove the double-swap. If the device works like any other device in the system, then the byteswap that is implied by readl/writel will do the right thing, and swapping twice will break big-endian kernels.
Arnd
Quoted 4 lines · Arnd Bergmann
Then please remove the double-swap. If the device works like any other device in the system, then the byteswap that is implied by readl/writel will do the right thing, and swapping twice will break big-endian kernels.
Should I still keep the __le32 in the structs?
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Quoted 7 lines · On Friday 15 May 2015 07:14:08 Timur Tabi wrote:
Quoted 4 lines · Arnd Bergmann
Then please remove the double-swap. If the device works like any other device in the system, then the byteswap that is implied by readl/writel will do the right thing, and swapping twice will break big-endian kernels.
Should I still keep the
__le32in the structs?
Not sure about that. Most people use offsets with a void * pointer, so they would not care.
Do you get a sparse warning if you either leave the __le32, or if you turn it into u32? The correct behavior is probably to leave __le32 here, and ensure that readl() produces a sparse warning when fed with a pointer to u32, but not void* or __le32*. It would need some experimenting to find if that catches real bugs or just starts warning about correct code and not about incorrect code.
FWIW, the drivers you cited that use le32_to_cpu(readl(p)) are probably all wrong, and it would be nice to have a sparse warning for that.
Arnd
Quoted 2 lines · Arnd Bergmann
Do you get a sparse warning if you either leave the
__le32, or if you turn it into u32?
After removing the cpu_to_le32 calls, but keeping the __le32 in the structs, I get no sparse warnings on my driver.
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 7 lines · On Friday 15 May 2015 11:21:05 Timur Tabi wrote:
Quoted 2 lines · Arnd Bergmann
Do you get a sparse warning if you either leave the
__le32, or if you turn it into u32?After removing the
cpu_to_le32calls, but keeping the__le32in the structs, I get no sparse warnings on my driver.
Ok, good. I've also checked the readl() definition and found that it has a (__force __le32) cast, while __raw_readl() takes a const volatile void __iomem *addr) pointer, so it sparse won't warn either way, but that's probably ok.
Arnd