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 { > >>>>> > >>>> > >>>> > >> > >> >