Re: [PATCH v4 1/4] dt-bindings: mediatek: watchdog: Fix compatible fallbacks and example

[Allen-KH Cheng]

Hi Nícolas,

On Mon, 2022-08-08 at 13:03 -0400, Nícolas F. R. A. Prado wrote:
> Hi,
> 
> On Wed, Aug 03, 2022 at 05:23:57PM +0800, Allen-KH Cheng wrote:
> > Hi Chen-Yu and Mathias,
> > 
> > Sincere apologies for the delayed response.
> > 
> > On Tue, 2022-08-02 at 13:04 +0200, Matthias Brugger wrote:
> > > 
> > > On 01/08/2022 11:29, Chen-Yu Tsai wrote:
> > > > On Thu, Jul 21, 2022 at 10:50 AM Allen-KH Cheng
> > > > <allen-kh.cheng@xxxxxxxxxxxx> wrote:
> > > > > 
> > > > > The watchdog timer of mt8186. mt8195 and mt7986 have their DT
> > > > > data.
> > > > > We should not use 'mediatek,mt6589-wdt' as fallback.
> > > > > 
> > > > > For mediatek,wdt example of mt8183, We remove
> > > > > mediatek,mt6589-wdt 
> > > > > fallback.
> > > > 
> > > > I think this needs some more information.
> > > > 
> > > > Right now on the kernel side, mt6589-wdt provides just watchdog
> > > > support.
> > > > The SoC-specific compatibles that are touched by this patch
> > > > provide
> > > > reset
> > > > controls in addition to the standard watchdog, which remains
> > > > the
> > > > same.
> > > > 
> > > > If that is the case, then the fallback compatibles are correct.
> > > > A
> > > > fallback
> > > > says that the new hardware is compatible with some older
> > > > hardware,
> > > > and
> > > > can be run with the driver supporting that older hardware,
> > > > likely
> > > > with
> > > > reduced functionality.
> > > > 
> > > 
> > > My understanding is, that we add a fallback because although at
> > > the
> > > time we 
> > > entered the compatible, the functionality of the device is the
> > > same
> > > as the 
> > > fallback. Nonetheless we add a compatible specific for the device
> > > in
> > > case in the 
> > > future we realize that the device has some functionality that is
> > > not
> > > and can not 
> > > be covered by the fallback.
> > > 
> > > This is the case here. Actually adding the fallback in the first
> > > place was 
> > > wrong, because the driver since ever supports the extra function
> > > for
> > > the device, 
> > > the reset.
> > > 
> > > So this is a mere cleanup of the binding to reflect what was
> > > always
> > > present in 
> > > the driver.
> > > 
> > > Regards,
> > > Matthias
> > > 
> > 
> > mt6589-wdt dosen't contains a reset control for other modules, like
> > chen-yu mention "mt6589-wdt provides just watchdog support."
> > 
> > For instance, there is a reset control in mt8195-wdt and we have a
> > DT
> > data to define its reset number of TOPRGU. I thought it's better
> > not
> > use mt6589-wdt as fallback.
> > 
> > Please let me know if this works and if you have any suggestions or
> > comments.
> 
> The only practical usecase that I can think of that relies on keeping
> the
> fallback compatibles is using the current DT with an older kernel
> that didn't
> yet support the specific watchdog compatibles. In this case, dropping
> the
> fallback compatibles would make the watchdog not work at all in such
> a kernel.
> 
> I'm not sure how relevant/common of a usecase that would be, but
> maybe it's
> worth considering given that the advantage of removing the fallback
> compatible
> is purely aesthetic?
> 
> Thanks,
> Nícolas
> 

Thanks for your comments.

I agree the advantage of patch is aesthetic. Since I also want to send
another "watchdog: Convert binding to YAML" PATCH, it's better let all
wdt compatibles in the binding match the contents of mtk_wdt_dt_ids in
drivers/watchdog/mtk_wdt.c

static const struct of_device_id mtk_wdt_dt_ids[] = {
	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
	{ .compatible = "mediatek,mt6589-wdt" },
	{ .compatible = "mediatek,mt7986-wdt", .data = &mt7986_data },
	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
	{ .compatible = "mediatek,mt8186-wdt", .data = &mt8186_data },
	{ .compatible = "mediatek,mt8192-wdt", .data = &mt8192_data },
	{ .compatible = "mediatek,mt8195-wdt", .data = &mt8195_data },
	{ /* sentinel */ }
};

We have "mediatek,mt8186-wdt" "mediatek,mt8195-wdt" and
"mediatek,mt7986-wdt" now and they have their DT data for the reset
control.

It's weird and unuseful to add "mediatek,mt6589-wdt" as fallback. 


Please kindly let me know if I missed anything

BRs,
Allen

> > 
> > Thanks,
> > Allen
> > 
> > > > As an example, if mt8195-wdt is backward compatible with
> > > > mt6589-
> > > > wdt,
> > > > then it should run as mt6589-wdt, and would just be missing new
> > > > functionality, in this case the reset controls.
> > > > 
> > > > So either mt6589-wdt also contains a reset control that is not
> > > > the
> > > > same
> > > > as the other newer chips, or has some other functionality that
> > > > the
> > > > other
> > > > chips contain, and justifies the removal of the fallback, or
> > > > this
> > > > patch
> > > > is incorrect. Note that mt2701-wdt and mt762*-wdt are still
> > > > listed
> > > > as
> > > > compatible with mt6589-wdt. So I think a better explanation is
> > > > required.
> > > > 
> > > > 
> > > > Regards
> > > > ChenYu
> > > > 
> > > > 
> > > > > Fixes:a45b408a020b("dt-bindings: watchdog: Add compatible for
> > > > > MediaTek MT8186")
> > > > > Fixes:b326f2c85f3d("dt-bindings: watchdog: Add compatible for
> > > > > Mediatek MT8195")
> > > > > Fixes:41e73feb1024("dt-bindings: watchdog: Add compatible for
> > > > > Mediatek MT7986")
> > > > > Fixes:f43f97a0fc0e("dt-bindings: mediatek: mt8183: Add
> > > > > #reset-
> > > > > cells")
> > > > > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@xxxxxxxxxxxx>
> > > > > Reviewed-by: AngeloGioacchino Del Regno <
> > > > > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > > > ---
> > > > >   Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 9
> > > > > ++++-----
> > > > >   1 file changed, 4 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-
> > > > > wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-
> > > > > wdt.txt
> > > > > index 762c62e428ef..67ef991ec4cf 100644
> > > > > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > > > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > > > @@ -14,12 +14,12 @@ Required properties:
> > > > >          "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for
> > > > > MT7622
> > > > >          "mediatek,mt7623-wdt", "mediatek,mt6589-wdt": for
> > > > > MT7623
> > > > >          "mediatek,mt7629-wdt", "mediatek,mt6589-wdt": for
> > > > > MT7629
> > > > > -       "mediatek,mt7986-wdt", "mediatek,mt6589-wdt": for
> > > > > MT7986
> > > > > +       "mediatek,mt7986-wdt": for MT7986
> > > > >          "mediatek,mt8183-wdt": for MT8183
> > > > > -       "mediatek,mt8186-wdt", "mediatek,mt6589-wdt": for
> > > > > MT8186
> > > > > +       "mediatek,mt8186-wdt": for MT8186
> > > > >          "mediatek,mt8516-wdt", "mediatek,mt6589-wdt": for
> > > > > MT8516
> > > > >          "mediatek,mt8192-wdt": for MT8192
> > > > > -       "mediatek,mt8195-wdt", "mediatek,mt6589-wdt": for
> > > > > MT8195
> > > > > +       "mediatek,mt8195-wdt": for MT8195
> > > > > 
> > > > >   - reg : Specifies base physical address and size of the
> > > > > registers.
> > > > > 
> > > > > @@ -32,8 +32,7 @@ Optional properties:
> > > > >   Example:
> > > > > 
> > > > >   watchdog: watchdog@10007000 {
> > > > > -       compatible = "mediatek,mt8183-wdt",
> > > > > -                    "mediatek,mt6589-wdt";
> > > > > +       compatible = "mediatek,mt8183-wdt";
> > > > >          mediatek,disable-extrst;
> > > > >          reg = <0 0x10007000 0 0x100>;
> > > > >          interrupts = <GIC_SPI 139 IRQ_TYPE_NONE>;
> > > > > --
> > > > > 2.18.0
> > > > > 
> > > > > 
> > 
> >