devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
@ 2022-07-25  8:24 Pin-yen Lin
  2022-07-25  8:39 ` AngeloGioacchino Del Regno
  0 siblings, 1 reply; 7+ messages in thread
From: Pin-yen Lin @ 2022-07-25  8:24 UTC (permalink / raw)
  To: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel
  Cc: Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Pin-yen Lin,
	Krzysztof Kozlowski, Rob Herring, devicetree

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.

Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

 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 {
-- 
2.37.1.359.gd136c6c3e2-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-25  8:24 [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog Pin-yen Lin
@ 2022-07-25  8:39 ` AngeloGioacchino Del Regno
  2022-07-25 10:19   ` Pin-yen Lin
  0 siblings, 1 reply; 7+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-25  8:39 UTC (permalink / raw)
  To: Pin-yen Lin, Matthias Brugger, linux-arm-kernel, linux-mediatek,
	linux-kernel
  Cc: Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

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?

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.

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.

Regards,
Angelo


> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
> 
>   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 {
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-25  8:39 ` AngeloGioacchino Del Regno
@ 2022-07-25 10:19   ` Pin-yen Lin
  2022-07-25 10:31     ` AngeloGioacchino Del Regno
  0 siblings, 1 reply; 7+ messages in thread
From: Pin-yen Lin @ 2022-07-25 10:19 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
	Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

On Mon, Jul 25, 2022 at 4:39 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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.

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?

>
> 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?
>
> Regards,
> Angelo
>
>
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> >   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 {
> >
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-25 10:19   ` Pin-yen Lin
@ 2022-07-25 10:31     ` AngeloGioacchino Del Regno
  2022-07-26  5:55       ` Pin-yen Lin
  0 siblings, 1 reply; 7+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-25 10:31 UTC (permalink / raw)
  To: Pin-yen Lin
  Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
	Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

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@collabora.com> 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?

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

>>
>> 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@chromium.org>
>>> ---
>>>
>>>    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 {
>>>
>>
>>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-25 10:31     ` AngeloGioacchino Del Regno
@ 2022-07-26  5:55       ` Pin-yen Lin
  2022-07-26  8:19         ` AngeloGioacchino Del Regno
  0 siblings, 1 reply; 7+ messages in thread
From: Pin-yen Lin @ 2022-07-26  5:55 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
	Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

On Mon, Jul 25, 2022 at 6:31 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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@collabora.com> 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?

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.
>
> >>
> >> 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@chromium.org>
> >>> ---
> >>>
> >>>    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 {
> >>>
> >>
> >>
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-26  5:55       ` Pin-yen Lin
@ 2022-07-26  8:19         ` AngeloGioacchino Del Regno
  2022-07-26  8:27           ` Pin-yen Lin
  0 siblings, 1 reply; 7+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-26  8:19 UTC (permalink / raw)
  To: Pin-yen Lin
  Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
	Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

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@collabora.com> 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@collabora.com> 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

>>
>>>>
>>>> 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@chromium.org>
>>>>> ---
>>>>>
>>>>>     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 {
>>>>>
>>>>
>>>>
>>
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog
  2022-07-26  8:19         ` AngeloGioacchino Del Regno
@ 2022-07-26  8:27           ` Pin-yen Lin
  0 siblings, 0 replies; 7+ messages in thread
From: Pin-yen Lin @ 2022-07-26  8:27 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
	Hsin-Yi Wang, Eizan Miyamoto, Evan Benn, Krzysztof Kozlowski,
	Rob Herring, devicetree

On Tue, Jul 26, 2022 at 4:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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@collabora.com> 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@collabora.com> 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@chromium.org>
> >>>>> ---
> >>>>>
> >>>>>     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 {
> >>>>>
> >>>>
> >>>>
> >>
> >>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-07-26  8:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-25  8:24 [PATCH] arm64: dts: mt8173-oak: Switch to SMC watchdog Pin-yen Lin
2022-07-25  8:39 ` AngeloGioacchino Del Regno
2022-07-25 10:19   ` Pin-yen Lin
2022-07-25 10:31     ` AngeloGioacchino Del Regno
2022-07-26  5:55       ` Pin-yen Lin
2022-07-26  8:19         ` AngeloGioacchino Del Regno
2022-07-26  8:27           ` Pin-yen Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).