On 10/29/21 9:45 AM, Florian Fainelli wrote:
On 10/29/21 6:03 AM, Rob Herring wrote:
On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
[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.
Yes, that's the correct thing to do.
So in the future if we happen to want to manage the hardware timers in
that block, they would be part of the watchdog driver? I am fairly sure
they won't be, so you will be creating a composite driver/MFD to
separate out the functions, more likely. So you might as well create
sub-nodes.
The question is whether you'd need sub nodes for the other functions.
Folks tend to want to have sub nodes for convenience which isn't really
needed and then requires a DT update ('cause they add nodes as adding
drivers).
Sorry but not, this is getting completely ridiculous, the
Based on the registers, you really don't need sub nodes here.
I sort of disagree here, the watchdog is a part of a sundry timer block
here, but it is logically broken up into different parts and if if I
were to imagine how this would map into different drivers, I can easily
see that we would have:
- a driver to manage the timer interrupt controller
- a driver to manage each of the 3 hardware timers, be they clockevent
or else
- a driver to manage the watchdog
The simplest way to get there, and also because these same timer blocks
are actually spread in other parts of STB chips just like the watchdog
is, but in a different layout where they stand on their own was the main
drive for defining the bcm7038_wdt binding the way it was.
Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
driver and this is better than the previous patch set, but I really do
not see why having the watchdog driver not manage the *exact* subset of
the register space needed (starting at 0x28) is not being done.
Agreed, especially since other sub-devices of bcm4908 are alredy modeled
this way. See arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi.
At this point, before accepting anything, I'll want to have an explanation
how and why the watchdog interface is handled differently than, say,
its reset controller. Also, I'd like to understand the memory region
assigned to bcm7038, which happens to be something like:
compatible = "brcm,bcm7038-wdt";
reg = <0xf040a7e8 0x16>;
because it seems unlikely that this is a chip subsystem that just happens
to start at such an odd boundary. More specifically, I see in actual
.dtsi files data such as:
watchdog: watchdog@4066a8 {
clocks = <&upg_clk>;
compatible = "brcm,bcm7038-wdt";
reg = <0x4066a8 0x14>;
status = "disabled";
};
...
timers: timer@406680 {
compatible = "brcm,brcmstb-timers";
reg = <0x406680 0x40>;
};
So there happen to be timers in the same region, and the offset
between timer and watchdog registers is 0x28. Coincidentally, that
just happens to be the extra offset defined in this patch for the
bcm4908 watchdog. Really ? Sorry, this sounds very inconsistent
and arbitrary to me.
Overall, I suspect I'll have to see datasheets if we really end up
having different offsets for each chip, because I'll want to confirm
that the watchdog subsystem isn't treated differently than other
subsystems, and that the offset calculations are appropriate and
consistent across the different chips.
Guenter
