Re: [PATCH v4 3/5] dt-bindings: watchdog: qcom-wdt: Document qcom,imem property

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 5/23/25 4:35 PM, Kathiravan Thirumoorthy wrote:
> 
> On 5/22/2025 9:15 PM, Konrad Dybcio wrote:
>> On 5/21/25 8:53 AM, Krzysztof Kozlowski wrote:
>>> On 20/05/2025 18:00, Konrad Dybcio wrote:
>>>> On 5/20/25 9:25 AM, Krzysztof Kozlowski wrote:
>>>>> On Mon, May 19, 2025 at 02:04:03PM GMT, Kathiravan Thirumoorthy wrote:
>>>>>> Document the "qcom,imem" property for the watchdog device on Qualcomm
>>>>>> IPQ platforms. Use this property to extract the restart reason from
>>>>>> IMEM, which is updated by XBL. Populate the watchdog's bootstatus sysFS
>>>>>> entry with this information, when the system reboots due to a watchdog
>>>>>> timeout.
>>>>>>
>>>>>> Describe this property for the IPQ5424 watchdog device and extend support
>>>>>> to other targets subsequently.
>>>>>>
>>>>>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>>>>>> ---
>>>>>> Changes in v4:
>>>>>>     - New patch
>>>>>> ---
>>>>>>   .../devicetree/bindings/watchdog/qcom-wdt.yaml       | 20 ++++++++++++++++++++
>>>>>>   1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>>>>>> index 49e2b807db0bc9d3edfc93ec41ad0df0b74ed032..bbe9b68ff4c8b813744ffd86bb52303943366fa2 100644
>>>>>> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
>>>>>> @@ -81,6 +81,16 @@ properties:
>>>>>>       minItems: 1
>>>>>>       maxItems: 5
>>>>>>   +  qcom,imem:
>>>>> Shoouldn't this be existing 'sram' property? If IMEM is something
>>>>> similar to OCMEM, then we already use sram for that.
>>>> We specifically want a handle to a predefined byte in IMEM, something akin
>>>> to qcom,4ln-config-sel in
>>>>
>>>> Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
>>> Nothing stops that with sram. Above example is poor, because it mentions
>>> syscon. There is no hardware as syscon. Does not exist. What is IMEM
>>> here, what is this relationship?
>> IMEM is indeed a small block of on-die SRAM. In this context, another subsystem
>> may write a magic value at a known offset that would correspond to the platform
>> having been rebooted by the watchdog. Now why the wdt register is cleared in the
>> first place, I have no clue.
> 
> 
> Thanks, Konrad for chiming in and providing the background information. With respect to the WDT register, when the interrupt is triggered, I see the expire bit is set in the watchdog register. The bite interrupt is handled by TZ and TZ does the system reboot. After the system reboots, bit is cleared. I have cross checked with the design team and they confirmed that the behavior is expected one.
> 
> Krzysztof, Based on the discussions from the previous versions, I have made the changes. Can you help to guide me on how to handle this? Should I just name the property as "sram" and point to the sub block in the IMEM region like how it is done at [1][2], which is more or like similar to what I have submitted in V1 of this series[3] Or is the current approach acceptable? Or some other way to handle this?
> 
> [1] https://lore.kernel.org/linux-arm-msm/20250523-topic-ipa_imem-v1-1-b5d536291c7f@oss.qualcomm.com/T/#u
> 
> [2] https://lore.kernel.org/linux-arm-msm/20250523-topic-ipa_imem-v1-2-b5d536291c7f@oss.qualcomm.com/T/#u
> 
> [3] https://lore.kernel.org/linux-arm-msm/20250408-wdt_reset_reason-v1-0-e6ec30c2c926@oss.qualcomm.com/

Let's go with desired-value-in-dt here.. I don't trust the firmware
to never change. `sram` is prooobably fine, let's hear from Krzysztof

Konrad