| Message ID | 20200923232535.241437-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: stm32: Enable thermal sensor support on stm32mp15xx-dhcor | expand |
On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> wrote: >Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor SoMs. > >Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree support >based on STM32MP157A") The change looks good but what does this patch fixes? Thanks, Mani >Signed-off-by: Marek Vasut <marex@denx.de> >Cc: Alexandre Torgue <alexandre.torgue@st.com> >Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >Cc: linux-stm32@st-md-mailman.stormreply.com >To: linux-arm-kernel@lists.infradead.org >--- > arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi >b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi >index 04fbb324a541..803eb8bc9c85 100644 >--- a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi >+++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi >@@ -21,6 +21,10 @@ memory@c0000000 { > }; > }; > >+&dts { >+ status = "okay"; >+}; >+ > &i2c4 { > pinctrl-names = "default"; > pinctrl-0 = <&i2c4_pins_a>;
On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: > > > On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> wrote: >> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor SoMs. >> >> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree support >> based on STM32MP157A") > > The change looks good but what does this patch fixes? The missing temp sensor, which helps you detect overheat of the SoC. That is esp. important on the 800 MHz AV96.
On 24 September 2020 4:11:11 PM IST, Marek Vasut <marex@denx.de> wrote: >On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: >> >> >> On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> >wrote: >>> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor >SoMs. >>> >>> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree >support >>> based on STM32MP157A") >> >> The change looks good but what does this patch fixes? > >The missing temp sensor, which helps you detect overheat of the SoC. >That is esp. important on the 800 MHz AV96. This doesn't quality as a "fix". Essentially you're just adding a missing feature and not fixing any issues. So please remove the fixes tag and resubmit. Thanks, Mani
On 9/25/20 4:21 AM, Manivannan Sadhasivam wrote: > > > On 24 September 2020 4:11:11 PM IST, Marek Vasut <marex@denx.de> wrote: >> On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: >>> >>> >>> On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> >> wrote: >>>> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor >> SoMs. >>>> >>>> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree >> support >>>> based on STM32MP157A") >>> >>> The change looks good but what does this patch fixes? >> >> The missing temp sensor, which helps you detect overheat of the SoC. >> That is esp. important on the 800 MHz AV96. > > This doesn't quality as a "fix". Essentially you're just adding a missing feature and not fixing any issues. So please remove the fixes tag and resubmit. I would argue that if the system overheats and crashes, we want to know about that, possibly in advance so thermal throttling can be applied. Currently this is not possible and I think that is a bug.
On Fri, Sep 25, 2020 at 01:12:12PM +0200, Marek Vasut wrote: > On 9/25/20 4:21 AM, Manivannan Sadhasivam wrote: > > > > > > On 24 September 2020 4:11:11 PM IST, Marek Vasut <marex@denx.de> wrote: > >> On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: > >>> > >>> > >>> On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> > >> wrote: > >>>> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor > >> SoMs. > >>>> > >>>> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree > >> support > >>>> based on STM32MP157A") > >>> > >>> The change looks good but what does this patch fixes? > >> > >> The missing temp sensor, which helps you detect overheat of the SoC. > >> That is esp. important on the 800 MHz AV96. > > > > This doesn't quality as a "fix". Essentially you're just adding a missing feature and not fixing any issues. So please remove the fixes tag and resubmit. > > I would argue that if the system overheats and crashes, we want to know > about that, possibly in advance so thermal throttling can be applied. > Currently this is not possible and I think that is a bug. No, this is not a _bug_. This is a missing feature that the current kernel doesn't support and you know about that! The fact that you can trigger a crash due to hw limitation doesn't qualify as a bug IMO. And you can do that by other means also (CPU throttling without CPUFreq support etc...) Anyway, I'll stop here and let Alex to make a call. Either case, feel free to add: Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani
On 9/25/20 6:20 PM, Manivannan Sadhasivam wrote: > On Fri, Sep 25, 2020 at 01:12:12PM +0200, Marek Vasut wrote: >> On 9/25/20 4:21 AM, Manivannan Sadhasivam wrote: >>> >>> >>> On 24 September 2020 4:11:11 PM IST, Marek Vasut <marex@denx.de> wrote: >>>> On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: >>>>> >>>>> >>>>> On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> >>>> wrote: >>>>>> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor >>>> SoMs. >>>>>> >>>>>> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree >>>> support >>>>>> based on STM32MP157A") >>>>> >>>>> The change looks good but what does this patch fixes? >>>> >>>> The missing temp sensor, which helps you detect overheat of the SoC. >>>> That is esp. important on the 800 MHz AV96. >>> >>> This doesn't quality as a "fix". Essentially you're just adding a missing feature and not fixing any issues. So please remove the fixes tag and resubmit. >> >> I would argue that if the system overheats and crashes, we want to know >> about that, possibly in advance so thermal throttling can be applied. >> Currently this is not possible and I think that is a bug. > > No, this is not a _bug_. This is a missing feature that the current kernel > doesn't support and you know about that! The fact that you can trigger a crash > due to hw limitation doesn't qualify as a bug IMO. And you can do that by other > means also (CPU throttling without CPUFreq support etc...) I would say that makes the hardware pretty useless, which is not the expectation I would have of a system I would like to use in production. Just assume your laptop would randomly overheat and crash, would you consider that a missing feature or a bug that should be fixed ? > Anyway, I'll stop here and let Alex to make a call. I agree.
Hi Marek, Mani On 9/25/20 6:20 PM, Manivannan Sadhasivam wrote: > On Fri, Sep 25, 2020 at 01:12:12PM +0200, Marek Vasut wrote: >> On 9/25/20 4:21 AM, Manivannan Sadhasivam wrote: >>> >>> >>> On 24 September 2020 4:11:11 PM IST, Marek Vasut <marex@denx.de> wrote: >>>> On 9/24/20 7:16 AM, Manivannan Sadhasivam wrote: >>>>> >>>>> >>>>> On 24 September 2020 4:55:35 AM IST, Marek Vasut <marex@denx.de> >>>> wrote: >>>>>> Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor >>>> SoMs. >>>>>> >>>>>> Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree >>>> support >>>>>> based on STM32MP157A") >>>>> >>>>> The change looks good but what does this patch fixes? >>>> >>>> The missing temp sensor, which helps you detect overheat of the SoC. >>>> That is esp. important on the 800 MHz AV96. >>> >>> This doesn't quality as a "fix". Essentially you're just adding a missing feature and not fixing any issues. So please remove the fixes tag and resubmit. >> >> I would argue that if the system overheats and crashes, we want to know >> about that, possibly in advance so thermal throttling can be applied. >> Currently this is not possible and I think that is a bug. > > No, this is not a _bug_. This is a missing feature that the current kernel > doesn't support and you know about that! The fact that you can trigger a crash > due to hw limitation doesn't qualify as a bug IMO. And you can do that by other > means also (CPU throttling without CPUFreq support etc...) > > Anyway, I'll stop here and let Alex to make a call. I take this patch as fix this time. I understand your argument Mani, but to avoid unexplained crashes at boot it's better to get also on older version. So applied on stm32-dt-for-v5.10-fixes. regards alex > Either case, feel free to add: > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Thanks, > Mani >
diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi index 04fbb324a541..803eb8bc9c85 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi @@ -21,6 +21,10 @@ memory@c0000000 { }; }; +&dts { + status = "okay"; +}; + &i2c4 { pinctrl-names = "default"; pinctrl-0 = <&i2c4_pins_a>;
Enable STM32 Digital Thermal Sensor driver for stm32mp15xx-dhcor SoMs. Fixes: 94cafe1b6482 ("ARM: dts: stm32: Add Avenger96 devicetree support based on STM32MP157A") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@st.com> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Cc: linux-stm32@st-md-mailman.stormreply.com To: linux-arm-kernel@lists.infradead.org --- arch/arm/boot/dts/stm32mp15xx-dhcor-som.dtsi | 4 ++++ 1 file changed, 4 insertions(+)