Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog

[Pin-yen Lin]

On Tue, Jul 26, 2022 at 4:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 26/07/22 07:55, Pin-yen Lin ha scritto:
> > On Mon, Jul 25, 2022 at 6:31 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>
> >> Il 25/07/22 12:19, Pin-yen Lin ha scritto:
> >>> On Mon, Jul 25, 2022 at 4:39 PM AngeloGioacchino Del Regno
> >>> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>>>
> >>>> Il 25/07/22 10:24, Pin-yen Lin ha scritto:
> >>>>> Switch to SMC watchdog because we need direct control of HW watchdog
> >>>>> registers from kernel. The corresponding firmware was uploaded in
> >>>>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405.
> >>>>>
> >>>>
> >>>> There's a fundamental issue with this change, I think.
> >>>>
> >>>> What happens if we run this devicetree on a device that does *not* have
> >>>> the new(er) firmware?
> >>>
> >>> I haven't tried this patch with an older firmware. I'll manage to
> >>> build one for this.
> >>>>
> >>>> The kernel *shall not* get broken when running on devices that are running
> >>>> on older firmware, especially because that's what was initially supported
> >>>> and what is working right now.
> >>>
> >>> Actually the current approach does not work *right*. The device boots,
> >>> but the watchdog does not work properly.
> >>>
> >>
> >> Is this a Chromebook firmware specific issue?
> >
> > I'm not sure if this is a Chromebook-specific issue. The internal
> > issue thread only discussed this for the Chromebook firmware.
> >>
> >>> Also, all MT8173 ChromeOS devices have this firmware updated, and we
> >>> don't have other upstream users apart from mt8173-evb. Do we want to
> >>> support the developers that are running upstream linux with their
> >>> MT8173 boards?
> >>>
> >>
> >> Upstream shall not be just about one machine: if we add support for a SoC there,
> >> we shall support the SoC-generic things in the SoC-specific DTSI, and the machine
> >> specific things in the machine-specific devicetrees.
> >>
> >> Chromebooks are not the only machines using the MT8173 SoC (Chuwi, Amazon also do
> >> have products using MT8173), so we shall not make the main mt8173.dtsi incompatible
> >> with these machines.
> >
> > I don't see their DTS files uploaded to the upstream kernel. So we
> > still want to support them even if they didn't upstream their changes?
> >
>
> The point is not about having to support them, but about not making things
> harder for them, in case any community person wants to add support for these
> upstream.
>
> By rule, everything SoC-generic goes to soc.dtsi; everything board-specific
> goes to board.dts(i).
>
> > Does it make sense if we move the modification to mt8173-elm.dtsi? The
> > device should be running ChromeOS AP firmware if it uses or references
> > mt8173-elm.dtsi. Also, all the MT8173 Chromebooks were shipped with
> > the "new" firmware from the very beginning. We just somehow didn't
> > upstream this around the time.
>
> Moving this to mt8173-elm.dtsi is the only sensible option, as this is something
> that was addressed in Chromebooks' firmwares and it's not a SoC hardware spec.
>
> So yes, please.
>
> The disablement of the MMIO watchdog should also be done in mt8173-elm.dtsi, and
> please please please, make sure to add a comment in the devicetree saying that
> we're disabling that one because the SMC wdog operates on the same MMIO.
>
> Regards,
> Angelo

Thanks for your detailed explanation. I'll move the modifications to
mt8173-elm.dtsi and add comments in v2.

Regards,
Pin-yen
>
> >>
> >>>>
> >>>> For this reason, I think that we should get some code around that checks
> >>>> if the SMC watchdog is supported and, if not, resort to MMIO wdog.
> >>>
> >>> What is the expected way to support this backward compatibility? Do we
> >>> put the old compatible strings ("mediatek,mt8173-wdt" and
> >>> "mediatek,mt6589-wdt") after "arm,smc-wdt" and reject it in the
> >>> drivers if the firmware does not support it?
> >>
> >> I don't know what's the best option to support both cases... Perhaps a good one
> >> would be to check (in mtk_wdt? or in arm_smc_wdt?) if the arm_smc_wdt is actually
> >> supported in firmware, so if the SMC one is registered, we skip the other.
> >>
> >>>>
> >>>> Regards,
> >>>> Angelo
> >>>>
> >>>>
> >>>>> Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>>     arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++----
> >>>>>     1 file changed, 2 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>>> index a2aef5aa67c1..2d1c776740a5 100644
> >>>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>>> @@ -528,10 +528,8 @@ power-domain@MT8173_POWER_DOMAIN_MFG {
> >>>>>                         };
> >>>>>                 };
> >>>>>
> >>>>> -             watchdog: watchdog@10007000 {
> >>>>> -                     compatible = "mediatek,mt8173-wdt",
> >>>>> -                                  "mediatek,mt6589-wdt";
> >>>>> -                     reg = <0 0x10007000 0 0x100>;
> >>>>> +             watchdog {
> >>>>> +                     compatible = "arm,smc-wdt";
> >>>>>                 };
> >>>>>
> >>>>>                 timer: timer@10008000 {
> >>>>>
> >>>>
> >>>>
> >>
> >>
>