* [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
@ 2022-07-27 9:40 Pin-yen Lin
2022-07-28 11:21 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 6+ messages in thread
From: Pin-yen Lin @ 2022-07-27 9:40 UTC (permalink / raw)
To: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel
Cc: Eizan Miyamoto, Hsin-Yi Wang, 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>
---
Changes in v2:
- Move the modifications to mt8173-elm.dtsi and add some comments.
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index e21feb85d822..b2269770abc3 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
};
};
};
+
+ soc {
+ /*
+ * Disable the original MMIO watch dog and switch to the SMC watchdog,
+ * which operates on the same MMIO.
+ */
+ /delete-node/ watchdog@10007000;
+
+ watchdog {
+ compatible = "arm,smc-wdt";
+ };
+ };
};
&mfg_async {
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
2022-07-27 9:40 [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog Pin-yen Lin
@ 2022-07-28 11:21 ` AngeloGioacchino Del Regno
2022-07-28 15:39 ` Pin-yen Lin
0 siblings, 1 reply; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-28 11:21 UTC (permalink / raw)
To: Pin-yen Lin, Matthias Brugger, linux-arm-kernel, linux-mediatek,
linux-kernel
Cc: Eizan Miyamoto, Hsin-Yi Wang, Evan Benn, Krzysztof Kozlowski,
Rob Herring, devicetree
Il 27/07/22 11:40, 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.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> Changes in v2:
> - Move the modifications to mt8173-elm.dtsi and add some comments.
>
> arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> index e21feb85d822..b2269770abc3 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
> };
> };
> };
> +
> + soc {
> + /*
> + * Disable the original MMIO watch dog and switch to the SMC watchdog,
> + * which operates on the same MMIO.
> + */
> + /delete-node/ watchdog@10007000;
Unfortunately, we're not quite there yet.
The comment is fine, but...
There's no need to /delete-node/: you can just do it like
/*
* Disable the original MMIO watch dog and switch to the SMC watchdog,
* which operates on the same MMIO.
*/
&watchdog {
status = "disabled";
};
and...
> +
> + watchdog {
This isn't addressable, hence it belongs to the root node, not to soc.
If you did that because of naming issues, I would propose to call it
smc-watchdog instead of watchdog.
> + compatible = "arm,smc-wdt";
P.S.: No timeout-sec?
Regards,
Angelo
> + };
> + };
> };
>
> &mfg_async {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
2022-07-28 11:21 ` AngeloGioacchino Del Regno
@ 2022-07-28 15:39 ` Pin-yen Lin
2022-07-28 15:51 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 6+ messages in thread
From: Pin-yen Lin @ 2022-07-28 15:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
Eizan Miyamoto, Hsin-Yi Wang, Evan Benn, Krzysztof Kozlowski,
Rob Herring, devicetree
On Thu, Jul 28, 2022 at 7:21 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 27/07/22 11:40, 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.
> >
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Move the modifications to mt8173-elm.dtsi and add some comments.
> >
> > arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > index e21feb85d822..b2269770abc3 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
> > };
> > };
> > };
> > +
> > + soc {
> > + /*
> > + * Disable the original MMIO watch dog and switch to the SMC watchdog,
> > + * which operates on the same MMIO.
> > + */
> > + /delete-node/ watchdog@10007000;
>
> Unfortunately, we're not quite there yet.
> The comment is fine, but...
>
> There's no need to /delete-node/: you can just do it like
>
> /*
> * Disable the original MMIO watch dog and switch to the SMC watchdog,
> * which operates on the same MMIO.
> */
> &watchdog {
> status = "disabled";
> };
>
> and...
>
> > +
> > + watchdog {
>
> This isn't addressable, hence it belongs to the root node, not to soc.
> If you did that because of naming issues, I would propose to call it
> smc-watchdog instead of watchdog.
>
>
> > + compatible = "arm,smc-wdt";
>
Thanks for the suggestion. I'll modify it accordingly in v3.
> P.S.: No timeout-sec?
The example in the binding file has a timeout-sec property, but it is
not defined in the binding nor used in the driver...
The driver seems to talk with the firmware to get a timeout value[1]
instead of reading it from the devicetree.
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/arm_smc_wdt.c#L138
>
> Regards,
> Angelo
>
> > + };
> > + };
> > };
> >
> > &mfg_async {
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
2022-07-28 15:39 ` Pin-yen Lin
@ 2022-07-28 15:51 ` AngeloGioacchino Del Regno
2022-07-29 2:59 ` Pin-yen Lin
0 siblings, 1 reply; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-28 15:51 UTC (permalink / raw)
To: Pin-yen Lin
Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
Eizan Miyamoto, Hsin-Yi Wang, Evan Benn, Krzysztof Kozlowski,
Rob Herring, devicetree
Il 28/07/22 17:39, Pin-yen Lin ha scritto:
> On Thu, Jul 28, 2022 at 7:21 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 27/07/22 11:40, 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.
>>>
>>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Move the modifications to mt8173-elm.dtsi and add some comments.
>>>
>>> arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
>>> index e21feb85d822..b2269770abc3 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
>>> @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
>>> };
>>> };
>>> };
>>> +
>>> + soc {
>>> + /*
>>> + * Disable the original MMIO watch dog and switch to the SMC watchdog,
>>> + * which operates on the same MMIO.
>>> + */
>>> + /delete-node/ watchdog@10007000;
>>
>> Unfortunately, we're not quite there yet.
>> The comment is fine, but...
>>
>> There's no need to /delete-node/: you can just do it like
>>
>> /*
>> * Disable the original MMIO watch dog and switch to the SMC watchdog,
>> * which operates on the same MMIO.
>> */
>> &watchdog {
>> status = "disabled";
>> };
>>
>> and...
>>
>>> +
>>> + watchdog {
>>
>> This isn't addressable, hence it belongs to the root node, not to soc.
>> If you did that because of naming issues, I would propose to call it
>> smc-watchdog instead of watchdog.
>>
>>
>>> + compatible = "arm,smc-wdt";
>>
> Thanks for the suggestion. I'll modify it accordingly in v3.
>
>> P.S.: No timeout-sec?
>
> The example in the binding file has a timeout-sec property, but it is
> not defined in the binding nor used in the driver...
> The driver seems to talk with the firmware to get a timeout value[1]
> instead of reading it from the devicetree.
>
Oh. I admit I trusted the binding blindly, didn't check the actual driver code.
On a note, we should check why is that binding partially wrong and eventually
fix it (remove the property), or add the capability to the driver, but feel free
to ignore that for now, as this is not relevant for the context of this specific
change that you're trying to do here.
P.S.: I just noticed that the commit title is also wrong. s/mt8173-oak/mt8173-elm/g
Waiting for a v3!
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/arm_smc_wdt.c#L138
>>
>> Regards,
>> Angelo
>>
>>> + };
>>> + };
>>> };
>>>
>>> &mfg_async {
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
2022-07-28 15:51 ` AngeloGioacchino Del Regno
@ 2022-07-29 2:59 ` Pin-yen Lin
2022-07-29 4:00 ` Pin-yen Lin
0 siblings, 1 reply; 6+ messages in thread
From: Pin-yen Lin @ 2022-07-29 2:59 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
Eizan Miyamoto, Hsin-Yi Wang, Evan Benn, Krzysztof Kozlowski,
Rob Herring, devicetree
On Thu, Jul 28, 2022 at 11:51 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 28/07/22 17:39, Pin-yen Lin ha scritto:
> > On Thu, Jul 28, 2022 at 7:21 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Il 27/07/22 11:40, 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.
> >>>
> >>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Move the modifications to mt8173-elm.dtsi and add some comments.
> >>>
> >>> arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
> >>> 1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> >>> index e21feb85d822..b2269770abc3 100644
> >>> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> >>> @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
> >>> };
> >>> };
> >>> };
> >>> +
> >>> + soc {
> >>> + /*
> >>> + * Disable the original MMIO watch dog and switch to the SMC watchdog,
> >>> + * which operates on the same MMIO.
> >>> + */
> >>> + /delete-node/ watchdog@10007000;
> >>
> >> Unfortunately, we're not quite there yet.
> >> The comment is fine, but...
> >>
> >> There's no need to /delete-node/: you can just do it like
> >>
> >> /*
> >> * Disable the original MMIO watch dog and switch to the SMC watchdog,
> >> * which operates on the same MMIO.
> >> */
> >> &watchdog {
> >> status = "disabled";
> >> };
> >>
> >> and...
> >>
> >>> +
> >>> + watchdog {
> >>
> >> This isn't addressable, hence it belongs to the root node, not to soc.
> >> If you did that because of naming issues, I would propose to call it
> >> smc-watchdog instead of watchdog.
> >>
> >>
> >>> + compatible = "arm,smc-wdt";
> >>
> > Thanks for the suggestion. I'll modify it accordingly in v3.
> >
> >> P.S.: No timeout-sec?
> >
> > The example in the binding file has a timeout-sec property, but it is
> > not defined in the binding nor used in the driver...
> > The driver seems to talk with the firmware to get a timeout value[1]
> > instead of reading it from the devicetree.
> >
>
> Oh. I admit I trusted the binding blindly, didn't check the actual driver code.
>
> On a note, we should check why is that binding partially wrong and eventually
> fix it (remove the property), or add the capability to the driver, but feel free
> to ignore that for now, as this is not relevant for the context of this specific
> change that you're trying to do here.
It looks like the timeout-sec property was not defined nor referenced
from the very first commit [2][3]. I'll send out another patch to
remove it.
[2]: dt-bindings:
https://lore.kernel.org/all/20200505131242.v6.1.Id96574f1f52479d7a2f3b866b8a0552ab8c03d7f@changeid/
[3]: watchdog driver:
https://lore.kernel.org/all/20200505131242.v6.2.Ia92bb4d4ce84bcefeba1d00aaa1c1e919b6164ef@changeid/
>
> P.S.: I just noticed that the commit title is also wrong. s/mt8173-oak/mt8173-elm/g
I sent out the v3 before I saw this reply. I'll send out a v4 for
this and address Krzysztof's comment as well.
>
> Waiting for a v3!
>
>
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/arm_smc_wdt.c#L138
> >>
> >> Regards,
> >> Angelo
> >>
> >>> + };
> >>> + };
> >>> };
> >>>
> >>> &mfg_async {
> >>>
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog
2022-07-29 2:59 ` Pin-yen Lin
@ 2022-07-29 4:00 ` Pin-yen Lin
0 siblings, 0 replies; 6+ messages in thread
From: Pin-yen Lin @ 2022-07-29 4:00 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Matthias Brugger, linux-arm-kernel, linux-mediatek, linux-kernel,
Eizan Miyamoto, Hsin-Yi Wang, Evan Benn, Krzysztof Kozlowski,
Rob Herring, devicetree
On Fri, Jul 29, 2022 at 10:59 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> On Thu, Jul 28, 2022 at 11:51 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> >
> > Il 28/07/22 17:39, Pin-yen Lin ha scritto:
> > > On Thu, Jul 28, 2022 at 7:21 PM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@collabora.com> wrote:
> > >>
> > >> Il 27/07/22 11:40, 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.
> > >>>
> > >>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > >>> ---
> > >>>
> > >>> Changes in v2:
> > >>> - Move the modifications to mt8173-elm.dtsi and add some comments.
> > >>>
> > >>> arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 12 ++++++++++++
> > >>> 1 file changed, 12 insertions(+)
> > >>>
> > >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> index e21feb85d822..b2269770abc3 100644
> > >>> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > >>> @@ -161,6 +161,18 @@ hdmi_connector_in: endpoint {
> > >>> };
> > >>> };
> > >>> };
> > >>> +
> > >>> + soc {
> > >>> + /*
> > >>> + * Disable the original MMIO watch dog and switch to the SMC watchdog,
> > >>> + * which operates on the same MMIO.
> > >>> + */
> > >>> + /delete-node/ watchdog@10007000;
> > >>
> > >> Unfortunately, we're not quite there yet.
> > >> The comment is fine, but...
> > >>
> > >> There's no need to /delete-node/: you can just do it like
> > >>
> > >> /*
> > >> * Disable the original MMIO watch dog and switch to the SMC watchdog,
> > >> * which operates on the same MMIO.
> > >> */
> > >> &watchdog {
> > >> status = "disabled";
> > >> };
> > >>
> > >> and...
> > >>
> > >>> +
> > >>> + watchdog {
> > >>
> > >> This isn't addressable, hence it belongs to the root node, not to soc.
> > >> If you did that because of naming issues, I would propose to call it
> > >> smc-watchdog instead of watchdog.
> > >>
> > >>
> > >>> + compatible = "arm,smc-wdt";
> > >>
> > > Thanks for the suggestion. I'll modify it accordingly in v3.
> > >
> > >> P.S.: No timeout-sec?
> > >
> > > The example in the binding file has a timeout-sec property, but it is
> > > not defined in the binding nor used in the driver...
> > > The driver seems to talk with the firmware to get a timeout value[1]
> > > instead of reading it from the devicetree.
> > >
> >
> > Oh. I admit I trusted the binding blindly, didn't check the actual driver code.
> >
> > On a note, we should check why is that binding partially wrong and eventually
> > fix it (remove the property), or add the capability to the driver, but feel free
> > to ignore that for now, as this is not relevant for the context of this specific
> > change that you're trying to do here.
>
> It looks like the timeout-sec property was not defined nor referenced
> from the very first commit [2][3]. I'll send out another patch to
> remove it.
>
> [2]: dt-bindings:
> https://lore.kernel.org/all/20200505131242.v6.1.Id96574f1f52479d7a2f3b866b8a0552ab8c03d7f@changeid/
> [3]: watchdog driver:
> https://lore.kernel.org/all/20200505131242.v6.2.Ia92bb4d4ce84bcefeba1d00aaa1c1e919b6164ef@changeid/
I checked the drivers again, and I realized that the `timeout-sec`
property overrides the default timeout from the firmware.
But for MT8173 chromebooks, I think we can just leave it blank and use
the default value.
> >
> > P.S.: I just noticed that the commit title is also wrong. s/mt8173-oak/mt8173-elm/g
>
> I sent out the v3 before I saw this reply. I'll send out a v4 for
> this and address Krzysztof's comment as well.
>
> >
> > Waiting for a v3!
> >
> >
> > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/arm_smc_wdt.c#L138
> > >>
> > >> Regards,
> > >> Angelo
> > >>
> > >>> + };
> > >>> + };
> > >>> };
> > >>>
> > >>> &mfg_async {
> > >>>
> > >>
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-29 4:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 9:40 [PATCH v2] arm64: dts: mt8173-oak: Switch to SMC watchdog Pin-yen Lin
2022-07-28 11:21 ` AngeloGioacchino Del Regno
2022-07-28 15:39 ` Pin-yen Lin
2022-07-28 15:51 ` AngeloGioacchino Del Regno
2022-07-29 2:59 ` Pin-yen Lin
2022-07-29 4:00 ` 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).