[Rob: please kindly comment on this]
On 28.10.2021 18:29, Florian Fainelli wrote:
On 10/28/21 2:30 AM, Rafał Miłecki wrote:
From: Rafał Miłecki <rafal@xxxxxxxxxx>
Hardware supported by this driver goes back to the old bcm63xx days. It
was then reused in BCM7038 and later also in BCM4908.
Depending on SoC model registers layout differs a bit. This commit
introduces support for per-chipset registers offsets & adds BCM4908
layout.
Later on BCM63xx SoCs support should be added too (probably as platform
devices due to missing DT). Eventually this driver should replace
bcm63xx_wdt.c.
Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
---
[snip]
+
+static const u16 bcm7038_wdt_regs_bcm4908[] = {
+ [BCM63XX_WDT_REG_DEFVAL] = 0x28,
+ [BCM63XX_WDT_REG_CTL] = 0x2c,
+ [BCM63XX_WDT_REG_SOFTRESET] = 0x34,
I don't understand what you are doing here and why you are not
offsetting the "reg" property appropriately when you create your
bcm4908-wdt Device Tree node such that the base starts at 0, and the
existing driver becomes usable as-is. This does not make any sense to me
when it is obviously the simplest way to make the driver "accept" the
resource being passed.
I believe that DT binding should cover the whole hardware block and
describe it (here: use proper compatible to allow recognizing block
variant).
That's because (as far as I understand) DT should be used to describe
hardware as closely as possible. I think it shouldn't be adjusted to
make mapping match Linux's driver implementation.
So if:
1. Hardware block is mapped at 0xff800400
2. It has interesting registers at 0xff800428 and 0xff80042c
I think mapping should use:
reg = <0xff800400 0x3c>;
even if we don't use the first N registers.
That way, at some point, you can extend Linux (or whatever) driver to
use extra registers without reworking the whole binding. That's why I
think we need to map whole hardware block & handle different registers
layouts in a driver.
Now, that is something I learnt from various DT discussions but I still
may got it wrong. I'd like to ask Rob to comment on this.
Let me also paste my summary of BCM4908's block I extracted from
Broadcom's header:
typedef struct Timer {
uint32 TimerCtl0; /* 0x00 */
uint32 TimerCtl1; /* 0x04 */
uint32 TimerCtl2; /* 0x08 */
uint32 TimerCtl3; /* 0x0c */
uint32 TimerCnt0; /* 0x10 */
uint32 TimerCnt1; /* 0x14 */
uint32 TimerCnt2; /* 0x18 */
uint32 TimerCnt3; /* 0x1c */
uint32 TimerMask; /* 0x20 */
uint32 TimerInts; /* 0x24 */
uint32 WatchDogDefCount; /* 0x28 */
uint32 WatchDogCtl; /* 0x2c */
uint32 WDResetCount; /* 0x30 */
uint32 SoftRst; /* 0x34 */
uint32 ResetStatus; /* 0x38 */
uint32 ResetReason; /* 0x3c */
uint32 spare[3]; /* 0x40-0x4b */
};
