Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

["Rafał Miłecki" <zajec5@gmail.com>]

[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@milecki.pl>
>>
>> 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@milecki.pl>
>> ---
> 
> [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 */
};