Message-ID: <62ba70ca-429e-476c-bb7b-78f743574a68@oss.nxp.com>
Date: Thu, 12 Sep 2024 13:50:25 +0300
From: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
To: Conor Dooley <conor@...nel.org>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Catalin Marinas
 <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 NXP S32 Linux Team <s32@....com>,
 Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>,
 Ghennadi Procopciuc <ghennadi.procopciuc@....com>
Subject: Re: [PATCH 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

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@....nxp.com>
>>
>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>
>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@....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@....com>
>> +  - Ciprian Marian Costea <ciprianmarian.costea@....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.

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@...60000 {
>> +        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
>>

Powered by blists - more mailing lists