Re: [PATCH 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

[Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>]

On 9/12/2024 3:26 PM, Alexandre Belloni wrote:
> On 12/09/2024 13:50:25+0300, Ciprian Marian Costea wrote:
>> On 9/11/2024 9:21 PM, Conor Dooley wrote:
>>> On Wed, Sep 11, 2024 at 10:00:25AM +0300, Ciprian Costea wrote:
>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>
>>>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>>>
>>>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
>>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>> ---
>>>>    .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 79 +++++++++++++++++++
>>>>    1 file changed, 79 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>> new file mode 100644
>>>> index 000000000000..8f78bce6470a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>> @@ -0,0 +1,79 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: NXP S32G2/S32G3 Real Time Clock (RTC)
>>>> +
>>>> +maintainers:
>>>> +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
>>>> +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: nxp,s32g-rtc
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  nxp,clksel:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      Input clock selector. Choose between 0-SIRC and 2-FIRC.
>>>> +      The reason for these IDs not being consecutive is because
>>>> +      they are hardware coupled.
>>>> +    enum:
>>>> +      - 0  # SIRC
>>>> +      - 2  # FIRC
>>>
>>> Could you please explain why, given both clocks must be provided by
>>> the hardware for there to be a choice, why choosing between them is a
>>> property of the hardware?
>>>
>>
>> Hello Conor,
>>
>> Thanks for your review.
>>
>> According to RTC module's clocking scheme for S32G2/S32G3 SoCs, it has three
>> potential clock sources to select between:
>>    1. FIRC:
>>      - fast clock - ~48 MHz output
>>      - chosen by default because it is proven to be more reliable (e.g:
>> temperature drift).
>>    2. SIRC:
>>      - slow clock - ~32 kHz output
>>      - When in Standby mode, SIRC_CLK is the only available clock for RTC.
>> This is important because RTC module is used as a wakeup source from Suspend
>> to RAM on S32G2/S32G3 SoC. Therefore, a temporary switch to SIRC clock is
>> performed when entering Suspend to RAM.
>>
>>    3. EXT_CLK:
>>      - has not been tested/validated for those SoCs within NXP's downstream
>> Linux. Therefore, I did not treat it, nor mention it, for the moment.
>>
>> Now to answer your question, all above clocks are entering a RTCC[CLKSEL]
>> (RTCC - RTC Control Register) mux. Therefore, a selection can be made,
>> according to one's needs.
>>
> 
> Then should this mux be registered in the CCF so you can use the usual
> clock node properties?

Hello Alexandre,

In hardware, these clock muxes and divisors are part of the RTC module 
itself and not external. Therefore, I would say no.

> 
>> I will add a shorter version of above information in the bindings
>> documentation in the V2 of this patchset.
>>
>>>> +
>>>> +  nxp,dividers:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    description:
>>>> +      An array of two u32 elements, the former encoding DIV512,
>>>> +      the latter encoding DIV32. These are dividers that can be enabled
>>>> +      individually, or cascaded. Use 0 to disable the respective divider,
>>>> +      and 1 to enable it.
>>>
>>> Please explain to me what makes this a property of the hardware and how
>>> someone would go about choosing the divider settings for their hardware.
>>>
>>
>> As per hardware RTC module clocking scheme, the output of the clock mux can
>> be optionally divided by a combination of 512 and 32 (via other two input
>> cascaded muxes) to give various count periods for different clock sources.
>>
>> With respect to choosing the divider settings for custom hardware, it
>> depends on the clock source being selected and the desired rollover time.
>> For example, on S32G2 or S32G3 SoC based boards, using FIRC (~48-51 MHz)
>> with DIV512 enabled results in a rollover time of aprox. 13 hours.
>>
>>>> +    items:
>>>> +      - description: div512
>>>> +      - description: div32
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 3
>>>
>>> I'd rather you provided an explicit items list here, explaining what
>>> each of the tree clocks do.
>>>
>>> Cheers,
>>> Conor.
>>>
>>
>> I will add such information in the V2 of this patchset.
>>
>> Regards,
>> Ciprian
>>
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: ipg
>>>> +      - const: sirc
>>>> +      - const: firc
>>>> +
>>>> +required:
>>>> +  - clock-names
>>>> +  - clocks
>>>> +  - compatible
>>>> +  - interrupts
>>>> +  - nxp,clksel
>>>> +  - nxp,dividers
>>>> +  - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +
>>>> +    rtc0: rtc@40060000 {
>>>> +        compatible = "nxp,s32g-rtc";
>>>> +        reg = <0x40060000 0x1000>;
>>>> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
>>>> +        clocks = <&clks 54>,
>>>> +                 <&clks 55>,
>>>> +                 <&clks 56>;
>>>> +        clock-names = "ipg", "sirc", "firc";
>>>> +        nxp,clksel = <2>;
>>>> +        nxp,dividers = <1 0>;
>>>> +    };
>>>> -- 
>>>> 2.45.2
>>>>
>>
>