| Message ID | 20231006042901.6474-6-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ti: k3-j7: Add the ESM & main domain watchdog nodes | expand |
On 09:58-20231006, Keerthy wrote: > There are totally 2 instances of watchdog module in MCU domain. > These instances are coupled with the MCU domain R5F instances. > Disabling them as they are not used by Linux. Device tree is hardware description - not tied to how Linux uses it. Reason these wdts are disabled by default is because they are tightly coupled with R5Fs. > > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > index a7b5c4cb7d3e..809a0b1cf038 100644 > --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > @@ -714,4 +714,28 @@ > ti,esm-pins = <63>; > bootph-pre-ram; > }; > + > + /* > + * The 2 RTI instances are couple with MCU R5Fs so keeping them > + * disabled as these will be used by their respective firmware > + */ > + mcu_watchdog0: watchdog@40600000 { > + compatible = "ti,j7-rti-wdt"; > + reg = <0x00 0x40600000 0x00 0x100>; > + clocks = <&k3_clks 367 1>; > + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; > + assigned-clocks = <&k3_clks 367 0>; > + assigned-clock-parents = <&k3_clks 367 4>; > + status = "disabled"; > + }; > + > + mcu_watchdog1: watchdog@40610000 { > + compatible = "ti,j7-rti-wdt"; > + reg = <0x00 0x40610000 0x00 0x100>; > + clocks = <&k3_clks 368 1>; > + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; > + assigned-clocks = <&k3_clks 368 0>; > + assigned-clock-parents = <&k3_clks 368 4>; Please DONOT ignore the review comments - I did ask the documentation in dts as well. reason being that this is what people will see rather than dig up the commit log. it should be intutive when reading the dts why nodes are disabled by default Vs the standard of leaving it enabled by default. Given esp that these peripherals do not have anything to do with board semantics (pinmux or something similar) to be complete. > + status = "disabled"; > + }; > }; > -- > 2.17.1 >
> > + mcu_watchdog1: watchdog@40610000 { > > + compatible = "ti,j7-rti-wdt"; > > + reg = <0x00 0x40610000 0x00 0x100>; > > + clocks = <&k3_clks 368 1>; > > + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; > > + assigned-clocks = <&k3_clks 368 0>; > > + assigned-clock-parents = <&k3_clks 368 4>; > > Please DONOT ignore the review comments - I did ask the documentation in > dts as well. reason being that this is what people will see rather than > dig up the commit log. it should be intutive when reading the dts why > nodes are disabled by default Vs the standard of leaving it enabled by > default. Given esp that these peripherals do not have anything to do > with board semantics (pinmux or something similar) to be complete. > > > + status = "disabled"; Just providing clarifying comment - something like this is probably more appropriate: /* Tightly coupled to R5F */ status = "reserved"; The rti needs to be handled by R5F and is reserved for firmware usage.
On 10/6/2023 5:04 PM, Nishanth Menon wrote: > On 09:58-20231006, Keerthy wrote: >> There are totally 2 instances of watchdog module in MCU domain. >> These instances are coupled with the MCU domain R5F instances. > >> Disabling them as they are not used by Linux. > Device tree is hardware description - not tied to how Linux uses it. > > Reason these wdts are disabled by default is because they are tightly > coupled with R5Fs. > >> >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> --- >> .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> index a7b5c4cb7d3e..809a0b1cf038 100644 >> --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> @@ -714,4 +714,28 @@ >> ti,esm-pins = <63>; >> bootph-pre-ram; >> }; >> + Nishanth, Below i have addressed the coupling with R5Fs & MCU domains watcdogs. >> + /* >> + * The 2 RTI instances are couple with MCU R5Fs so keeping them >> + * disabled as these will be used by their respective firmware >> + */ >> + mcu_watchdog0: watchdog@40600000 { >> + compatible = "ti,j7-rti-wdt"; >> + reg = <0x00 0x40600000 0x00 0x100>; >> + clocks = <&k3_clks 367 1>; >> + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; >> + assigned-clocks = <&k3_clks 367 0>; >> + assigned-clock-parents = <&k3_clks 367 4>; >> + status = "disabled"; >> + }; >> + >> + mcu_watchdog1: watchdog@40610000 { >> + compatible = "ti,j7-rti-wdt"; >> + reg = <0x00 0x40610000 0x00 0x100>; >> + clocks = <&k3_clks 368 1>; >> + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; >> + assigned-clocks = <&k3_clks 368 0>; >> + assigned-clock-parents = <&k3_clks 368 4>; > > Please DONOT ignore the review comments - I did ask the documentation in > dts as well. reason being that this is what people will see rather than > dig up the commit log. it should be intutive when reading the dts why > nodes are disabled by default Vs the standard of leaving it enabled by > default. Given esp that these peripherals do not have anything to do > with board semantics (pinmux or something similar) to be complete. As mentioned above. I added single comment for addressing both the watchdogs. - Keerthy > >> + status = "disabled"; >> + }; >> }; >> -- >> 2.17.1 >> >
On 10/6/2023 5:04 PM, Nishanth Menon wrote: > On 09:58-20231006, Keerthy wrote: >> There are totally 2 instances of watchdog module in MCU domain. >> These instances are coupled with the MCU domain R5F instances. > >> Disabling them as they are not used by Linux. > Device tree is hardware description - not tied to how Linux uses it. > > Reason these wdts are disabled by default is because they are tightly > coupled with R5Fs. Okay. I will rephrase that. > >> >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> --- >> .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> index a7b5c4cb7d3e..809a0b1cf038 100644 >> --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >> @@ -714,4 +714,28 @@ >> ti,esm-pins = <63>; >> bootph-pre-ram; >> }; >> + >> + /* >> + * The 2 RTI instances are couple with MCU R5Fs so keeping them >> + * disabled as these will be used by their respective firmware >> + */ >> + mcu_watchdog0: watchdog@40600000 { >> + compatible = "ti,j7-rti-wdt"; >> + reg = <0x00 0x40600000 0x00 0x100>; >> + clocks = <&k3_clks 367 1>; >> + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; >> + assigned-clocks = <&k3_clks 367 0>; >> + assigned-clock-parents = <&k3_clks 367 4>; >> + status = "disabled"; >> + }; >> + >> + mcu_watchdog1: watchdog@40610000 { >> + compatible = "ti,j7-rti-wdt"; >> + reg = <0x00 0x40610000 0x00 0x100>; >> + clocks = <&k3_clks 368 1>; >> + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; >> + assigned-clocks = <&k3_clks 368 0>; >> + assigned-clock-parents = <&k3_clks 368 4>; > > Please DONOT ignore the review comments - I did ask the documentation in > dts as well. reason being that this is what people will see rather than > dig up the commit log. it should be intutive when reading the dts why > nodes are disabled by default Vs the standard of leaving it enabled by > default. Given esp that these peripherals do not have anything to do > with board semantics (pinmux or something similar) to be complete. > >> + status = "disabled"; >> + }; >> }; >> -- >> 2.17.1 >> >
On 17:22-20231006, J, KEERTHY wrote: > > > On 10/6/2023 5:04 PM, Nishanth Menon wrote: > > On 09:58-20231006, Keerthy wrote: > > > There are totally 2 instances of watchdog module in MCU domain. > > > These instances are coupled with the MCU domain R5F instances. > > > > > Disabling them as they are not used by Linux. > > Device tree is hardware description - not tied to how Linux uses it. > > > > Reason these wdts are disabled by default is because they are tightly > > coupled with R5Fs. > > > > > > > > Signed-off-by: Keerthy <j-keerthy@ti.com> > > > --- > > > .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > > > index a7b5c4cb7d3e..809a0b1cf038 100644 > > > --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > > > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi > > > @@ -714,4 +714,28 @@ > > > ti,esm-pins = <63>; > > > bootph-pre-ram; > > > }; > > > + > > Nishanth, > > Below i have addressed the coupling with R5Fs & MCU domains watcdogs. > > > > + /* > > > + * The 2 RTI instances are couple with MCU R5Fs so keeping them > > > + * disabled as these will be used by their respective firmware This description is best in the commit message > > > + */ > > > + mcu_watchdog0: watchdog@40600000 { > > > + compatible = "ti,j7-rti-wdt"; > > > + reg = <0x00 0x40600000 0x00 0x100>; > > > + clocks = <&k3_clks 367 1>; > > > + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; > > > + assigned-clocks = <&k3_clks 367 0>; > > > + assigned-clock-parents = <&k3_clks 367 4>; > > > + status = "disabled"; > > > + }; > > > + > > > + mcu_watchdog1: watchdog@40610000 { > > > + compatible = "ti,j7-rti-wdt"; > > > + reg = <0x00 0x40610000 0x00 0x100>; > > > + clocks = <&k3_clks 368 1>; > > > + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; > > > + assigned-clocks = <&k3_clks 368 0>; > > > + assigned-clock-parents = <&k3_clks 368 4>; > > > > Please DONOT ignore the review comments - I did ask the documentation in > > dts as well. reason being that this is what people will see rather than > > dig up the commit log. it should be intutive when reading the dts why > > nodes are disabled by default Vs the standard of leaving it enabled by > > default. Given esp that these peripherals do not have anything to do > > with board semantics (pinmux or something similar) to be complete. > > As mentioned above. I added single comment for addressing both the > watchdogs. I missed it completely. Now that I think of it, I seem to have missed having seen it in previous rev reviews as well, and there is a reason for it: See [1] clarifying comment - nodes reserved for firmware usage have convention of "reserved" as status and documentation immediately above the status to help clarify the reason in-context. That is more readable than having to scroll up to find the rationale. [1] https://lore.kernel.org/all/20231006114422.avymeap7h5ocs6zq@dreadlock/
On 10/6/2023 5:31 PM, Nishanth Menon wrote: > On 17:22-20231006, J, KEERTHY wrote: >> >> >> On 10/6/2023 5:04 PM, Nishanth Menon wrote: >>> On 09:58-20231006, Keerthy wrote: >>>> There are totally 2 instances of watchdog module in MCU domain. >>>> These instances are coupled with the MCU domain R5F instances. >>> >>>> Disabling them as they are not used by Linux. >>> Device tree is hardware description - not tied to how Linux uses it. >>> >>> Reason these wdts are disabled by default is because they are tightly >>> coupled with R5Fs. >>> >>>> >>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>> --- >>>> .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >>>> index a7b5c4cb7d3e..809a0b1cf038 100644 >>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi >>>> @@ -714,4 +714,28 @@ >>>> ti,esm-pins = <63>; >>>> bootph-pre-ram; >>>> }; >>>> + >> >> Nishanth, >> >> Below i have addressed the coupling with R5Fs & MCU domains watcdogs. >> >>>> + /* >>>> + * The 2 RTI instances are couple with MCU R5Fs so keeping them >>>> + * disabled as these will be used by their respective firmware > > This description is best in the commit message > >>>> + */ >>>> + mcu_watchdog0: watchdog@40600000 { >>>> + compatible = "ti,j7-rti-wdt"; >>>> + reg = <0x00 0x40600000 0x00 0x100>; >>>> + clocks = <&k3_clks 367 1>; >>>> + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; >>>> + assigned-clocks = <&k3_clks 367 0>; >>>> + assigned-clock-parents = <&k3_clks 367 4>; >>>> + status = "disabled"; >>>> + }; >>>> + >>>> + mcu_watchdog1: watchdog@40610000 { >>>> + compatible = "ti,j7-rti-wdt"; >>>> + reg = <0x00 0x40610000 0x00 0x100>; >>>> + clocks = <&k3_clks 368 1>; >>>> + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; >>>> + assigned-clocks = <&k3_clks 368 0>; >>>> + assigned-clock-parents = <&k3_clks 368 4>; >>> >>> Please DONOT ignore the review comments - I did ask the documentation in >>> dts as well. reason being that this is what people will see rather than >>> dig up the commit log. it should be intutive when reading the dts why >>> nodes are disabled by default Vs the standard of leaving it enabled by >>> default. Given esp that these peripherals do not have anything to do >>> with board semantics (pinmux or something similar) to be complete. >> >> As mentioned above. I added single comment for addressing both the >> watchdogs. > > I missed it completely. Now that I think of it, I seem to have missed > having seen it in previous rev reviews as well, and there is a reason > for it: See [1] clarifying comment - nodes reserved for firmware usage > have convention of "reserved" as status and documentation immediately > above the status to help clarify the reason in-context. That is more > readable than having to scroll up to find the rationale. > > [1] https://lore.kernel.org/all/20231006114422.avymeap7h5ocs6zq@dreadlock/ Thanks Nishanth. I agree reserved is better and I will add a comment something like below: /* Tightly coupled to R5F */ status = "reserved"; - Keerthy >
diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi index a7b5c4cb7d3e..809a0b1cf038 100644 --- a/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi @@ -714,4 +714,28 @@ ti,esm-pins = <63>; bootph-pre-ram; }; + + /* + * The 2 RTI instances are couple with MCU R5Fs so keeping them + * disabled as these will be used by their respective firmware + */ + mcu_watchdog0: watchdog@40600000 { + compatible = "ti,j7-rti-wdt"; + reg = <0x00 0x40600000 0x00 0x100>; + clocks = <&k3_clks 367 1>; + power-domains = <&k3_pds 367 TI_SCI_PD_EXCLUSIVE>; + assigned-clocks = <&k3_clks 367 0>; + assigned-clock-parents = <&k3_clks 367 4>; + status = "disabled"; + }; + + mcu_watchdog1: watchdog@40610000 { + compatible = "ti,j7-rti-wdt"; + reg = <0x00 0x40610000 0x00 0x100>; + clocks = <&k3_clks 368 1>; + power-domains = <&k3_pds 368 TI_SCI_PD_EXCLUSIVE>; + assigned-clocks = <&k3_clks 368 0>; + assigned-clock-parents = <&k3_clks 368 4>; + status = "disabled"; + }; };
There are totally 2 instances of watchdog module in MCU domain. These instances are coupled with the MCU domain R5F instances. Disabling them as they are not used by Linux. Signed-off-by: Keerthy <j-keerthy@ti.com> --- .../boot/dts/ti/k3-j784s4-mcu-wakeup.dtsi | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+)