Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/10/2024 16:09, Jonathan Marek wrote:
> On 10/14/24 9:38 AM, Krzysztof Kozlowski wrote:
>> On 14/10/2024 14:58, Jonathan Marek wrote:
>>> On 10/14/24 3:34 AM, Krzysztof Kozlowski wrote:
>>>> On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
>>>>> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
>>>>> Thus writing to RTC alarm registers and receiving alarm interrupts is not
>>>>> possible.
>>>>>
>>>>> Add a no-alarm flag to support RTC on this platform.
>>>>>
>>>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> index d274bb7a534b5..210f76a819e90 100644
>>>>> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> @@ -40,6 +40,11 @@ properties:
>>>>>        description:
>>>>>          Indicates that the setting of RTC time is allowed by the host CPU.
>>>>>    
>>>>> +  no-alarm:
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> +    description:
>>>>> +      Indicates that RTC alarm is not owned by HLOS (Linux).
>>>>
>>>> This is not even properly used/tested, because you disable the RTC
>>>> entirely in your DTS.
>>>>
>>>
>>> What? The next patch in this series is enabling RTC on x1e using this flag
>>
>> D'oh, right, I must have looked at wrong diff hunks. I had somehow
>> impression you add status=reserved, but you just dropped it.
>>
>>>
>>>> I expect here unified property for all Qualcomm devices for this case.
>>>> We already have "remotely-controlled" and other flavors. I don't want
>>>> each device to express the same with different name...
>>>>
>>>> Also: missing vendor prefix.
>>>>
>>>
>>> I don't care what the property is named (as long as its a bool
>>> property), if you have a name you prefer I will use it.
>>>
>>> The existing 'allow-set-time' property (also related to HLOS permissions
>>> to the RTC) is also specific to this driver doesn't have a vendor prefix.
>>
>> Yeah, that one sneaked in some years ago.
>>
>> So you can set time, but not alarm? Some previous platforms could not
>> set time, but could set alarm?
>>
>> I wonder whether we actually describe the real issue here. It looks like
>> group of band-aids.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Firmware can set different permissions for the RTC time (0x61xx) and RTC 
> alarm (0x62xx) regions. So it makes sense to have one flag for each region.
> 
> RTC time is almost always read-only (not owned by HLOS/Linux), so the 
> 'allow-set-time' property is almost never used (the driver supports 
> using nvmem to store an offset for setting time as a workaround).
> 
> The "can set time, but not alarm" combination will probably never be 
> used, but the 3 other combinations are possible (the common one is 
> "can't set time, but can set alarm").
> 
> (in the next patch I deleted the "alarm" region/interrupt from the dts 
> but that's wrong, the HW still exists, the patch should be only 
> replacing the reserved status with the new flag)

OK, let's just add vendor prefix and describe actual hardware property,
e.g. qcom,no-alarm or qcom,alarm-restricted

Best regards,
Krzysztof