Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 15:17, Daniel Golle wrote:
> > On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
> >> Il 10/11/23 01:30, Daniel Golle ha scritto:
> >>> Add binding description for mediatek,mt7988-wdt.
> >>>
> >>> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> >>> ---
> >>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
> >>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
> >>>   2 files changed, 13 insertions(+)
> >>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> index cc502838bc398..8d2520241e37f 100644
> >>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> @@ -25,6 +25,7 @@ properties:
> >>>             - mediatek,mt6735-wdt
> >>>             - mediatek,mt6795-wdt
> >>>             - mediatek,mt7986-wdt
> >>> +          - mediatek,mt7988-wdt
> >>>             - mediatek,mt8183-wdt
> >>>             - mediatek,mt8186-wdt
> >>>             - mediatek,mt8188-wdt
> >>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> new file mode 100644
> >>> index 0000000000000..fa7c937505e08
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> @@ -0,0 +1,12 @@
> >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>> +
> >>> +/* TOPRGU resets */
> >>
> >> The first reset is zero, the second reset is one.
> >>
> >> Where's the zero'th reset? :-)
> > 
> > Currently the reset numbers represent the corresponding bit positions in
> > the toprgu register, as this is how the mtk-wdt driver is organized.
> > 
> > So there is probably something at bit 0, and also at bit 3~11 and
> > maybe also 17~23, but it's unknown and may be added later once known
> > and/or needed.
> 
> There is no need to put register bits, which are not used by the driver,
> in the bindings.

There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).

Or should the driver be reorganized to provide a mapping of logical to
physical resets, and then have only the needed once present and start
counting logical resets from 0? This is doable, of course, but it's a
bit of effort just for the aesthetical goal of starting to count from
zero and continous in header file.

And, of course, chances are that other currently still unused bits
will be needed at a later point which then would mean having to add
them in at least 2 places (header file and mapping logical<->physical)
where as currently it would just mean adding a line defining it in the
header file.

A quick looks at all the other headers in
include/dt-binding/reset/mt*-resets.h also shows that currently all of
them have unused bits and e.g. infracfg on MT7986 starts counting from
6.

> 
> Best regards,
> Krzysztof
>