Re: [PATCH v1 2/2] arm64: dts: qcom: Enable TSENS support for QCS615 SoC

[Gaurav Kohli <quic_gkohli@quicinc.com>]

On 4/14/2025 3:23 PM, Konrad Dybcio wrote:
> On 4/14/25 10:28 AM, Gaurav Kohli wrote:
>> thanks for review!
>>
>> On 4/12/2025 5:13 AM, Konrad Dybcio wrote:
>>> On 4/10/25 4:00 PM, Gaurav Kohli wrote:
>>>> Add TSENS and thermal devicetree node for QCS615 SoC.
>>>>
>>>> Signed-off-by: Gaurav Kohli <quic_gkohli@quicinc.com>
>>>> ---
>>>
>>> subject: "arm64: dts: qcom: qcs615: ..">  arch/arm64/boot/dts/qcom/qcs615.dtsi | 281 +++++++++++++++++++++++++++
>>>>    1 file changed, 281 insertions(+)
>>>>
>> will fix this.
>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> index edfb796d8dd3..f0d8aed7da29 100644
>>>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> @@ -3668,6 +3668,17 @@ usb_2_dwc3: usb@a800000 {
>>>>                    maximum-speed = "high-speed";
>>>>                };
>>>>            };
>>>> +
>>>> +        tsens0: tsens@c222000 {
>>>> +            compatible = "qcom,qcs615-tsens", "qcom,tsens-v2";
>>>> +            reg = <0x0 0xc263000 0x0 0x1ff>,
>>>> +                <0x0 0xc222000 0x0 0x8>;
>>> Pad the address part to 8 hex digits with leading zeroes> +            interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
>>>
>>> &pdc 26
>>>
>>>> +                    <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> &pdc 28
>> we don't want to mark this as wake up capable, so using this approach.
> 
> Why not?
> 
Intention was to avoid wake up, as system is already in lowest state, 
please let me know if you see any concern here.
>>>> +
>>>> +        cpuss-0-thermal {
>>>> +            thermal-sensors = <&tsens0 1>;
>>>> +
>>>> +            trips {
>>>> +
>>>> +                trip-point0 {
>>>> +                    temperature = <115000>;
>>>> +                    hysteresis = <5000>;
>>>> +                    type = "passive";
>>>> +                };
>>>> +
>>>> +                trip-point1 {
>>>> +                    temperature = <118000>;
>>>> +                    hysteresis = <5000>;
>>>> +                    type = "passive";
>>>> +                };
>>>
>>> Please drop the passive trip point for the *CPU* tzones, see
>>>
>>
>> we are using trip-point 0 for cpu idle injection mitigation which i will add in subsequent patches, if you are fine i will add cpu idle injection cooling map in this series only ?
> 
> The folks working on qcs9xxx have made this point too, but I'm lukewarm
> on duplicating meaningless dt description everywhere. I've asked them to
> conduct some measurements on whether random default settings (that would
> be preset in the driver and require no additional dt fluff) show any
> significant difference - if not, we can save up on boilerplate.
> 
> So let's wait to hear back from them on this.
> 
Sure will wait for latest update.
>>> commit 06eadce936971dd11279e53b6dfb151804137836
>>> ("arm64: dts: qcom: x1e80100: Drop unused passive thermal trip points for CPU")
>>>
>>> and add a single critical point instead, see
>>>
>> As critical shutdown is already supported by hardware, so we are not defining here.
> 
> The hardware critical shutdown will literally pull the plug out with the OS
> having no chance to sync the filesystem etc.
> 
> Please define one that's like 5 degC below the hardware limit, so that the
> operating system can try to take some steps to avoid data loss
> 

Sure will post critical in next patch.
> Konrad