[PATCH 0/4] add NXP RTC driver support for S32G2/S32G3 SoCs

[PATCH 0/4] add NXP RTC driver support for S32G2/S32G3 SoCs

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

This patch series add support for the NXP
RTC hardware module present on S32G2/S32G3 SoCs.

The RTC module is used to enable Suspend to RAM (STR) support.
RTC tracks clock time during system suspend.

Following is an example of Suspend to RAM trigger on
S32G2/S32G3 SoCs, using userspace tools such as rtcwake:
# rtcwake -s 2 -m mem
# rtcwake: assuming RTC uses UTC ...
# rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb  6 06:28:36 2036
#

Ciprian Marian Costea (4):
  dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs
  rtc: s32g: add NXP S32G2/S32G3 SoC support
  arm64: defconfig: add S32G RTC module support
  MAINTAINERS: add MAINTAINER for S32G2/S32G3 RTC driver

 .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml |  79 ++
 MAINTAINERS                                   |   2 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/rtc/Kconfig                           |  10 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-s32g.c                        | 689 ++++++++++++++++++
 6 files changed, 782 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
 create mode 100644 drivers/rtc/rtc-s32g.c

-- 
2.45.2

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

[Ciprian Costea]

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
+
+  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.
+    items:
+      - description: div512
+      - description: div32
+
+  clocks:
+    maxItems: 3
+
+  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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

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?

> +
> +  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.

> +    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.

> +
> +  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ciprian Marian Costea]

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.

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
>>

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]

On Thu, Sep 12, 2024 at 01:50:25PM +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>

> > > +  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?
> > 
> 
> 
> 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.

Given both clocks must be provided, what is the benefit of using the
slow clock outside of standby mode? Why would someone not just always
use the fast clock outside of standby and slow in standby?

> 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

What do you mean by "custom" hardware? I assume that you mean on a per
board basis?

> 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.

So a different user of the same board might want a different rollover
time? If so, this doesn't really seem like something that should be
controlled from devicetree.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ciprian Marian Costea]

On 9/12/2024 2:27 PM, Conor Dooley wrote:
> On Thu, Sep 12, 2024 at 01:50:25PM +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>
> 
>>>> +  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?
>>>
>>
>>
>> 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.
> 
> Given both clocks must be provided, what is the benefit of using the
> slow clock outside of standby mode? Why would someone not just always
> use the fast clock outside of standby and slow in standby?
> 

Hello Conor,

I cannot find any benefit of using the slow clock outside of standby 
mode. Hence, I see the reasons for removing CLKSEL support and 
defaulting to a static configuration.

On the other hand, having the CLKSEL mux support implemented and 
available would help if RTC external clock would want to be added, as 
the RTC hardware module supports it.

>> 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
> 
> What do you mean by "custom" hardware? I assume that you mean on a per
> board basis?

Indeed, I was considering the same S32G2 or S32G3 SoC but on different 
boards/different scenarios. I just wanted to expose the ability to reach 
lower frequencies by using those available hardware divisors.

> 
>> 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.
> 
> So a different user of the same board might want a different rollover
> time? If so, this doesn't really seem like something that should be
> controlled from devicetree.
> 
> Cheers,
> Conor.

I understand your point, indeed it does not seem to fit the devicetree, 
but maybe exposing them via sysfs would be a better approach. I will 
remove these bindings for now and consider an alternative, in V2 of this 
patchset.


Regards,
Ciprian

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

[Alexandre Belloni]

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?

> 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
> > > 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Ciprian Marian Costea]

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
>>>>
>>
> 

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

[Alexandre Belloni]

On 12/09/2024 15:36:46+0300, Ciprian Marian Costea wrote:
> > 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.

This is irrelevant, if this is a clock mux, it must be in the CCF, just
as when the RTC has a clock output.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Ciprian Marian Costea]

On 9/12/2024 5:03 PM, Alexandre Belloni wrote:
> On 12/09/2024 15:36:46+0300, Ciprian Marian Costea wrote:
>>> 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.
> 
> This is irrelevant, if this is a clock mux, it must be in the CCF, just
> as when the RTC has a clock output.
> 
> 

I understand your point, but taking into account the fact that FIRC 
clock should be used in most scenarios, would it be acceptable to not 
export this 'clksel' property in the devicetree bindings and simply use 
the FIRC clock by default in the RTC driver ?

At least for this patchset, in order to ease the review process. If 
configurable clock source support would want to be enabled and exported 
via bindings for this S32G2/S32G3 RTC driver, then CCF registration for 
this clk mux could be added in future patches.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Tue, Sep 17, 2024 at 10:21:32AM +0300, Ciprian Marian Costea wrote:
> On 9/12/2024 5:03 PM, Alexandre Belloni wrote:
> > On 12/09/2024 15:36:46+0300, Ciprian Marian Costea wrote:
> > > > 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.
> > 
> > This is irrelevant, if this is a clock mux, it must be in the CCF, just
> > as when the RTC has a clock output.
> > 
> > 
> 
> I understand your point, but taking into account the fact that FIRC clock
> should be used in most scenarios, would it be acceptable to not export this
> 'clksel' property in the devicetree bindings and simply use the FIRC clock
> by default in the RTC driver ?

Devices should be described in full in the bindings, regardless of
whether or not the driver for the device makes use of that information.

Cheers,
Conor,

> 
> At least for this patchset, in order to ease the review process. If
> configurable clock source support would want to be enabled and exported via
> bindings for this S32G2/S32G3 RTC driver, then CCF registration for this clk
> mux could be added in future patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Alexandre Belloni]

On 17/09/2024 10:21:32+0300, Ciprian Marian Costea wrote:
> On 9/12/2024 5:03 PM, Alexandre Belloni wrote:
> > On 12/09/2024 15:36:46+0300, Ciprian Marian Costea wrote:
> > > > 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.
> > 
> > This is irrelevant, if this is a clock mux, it must be in the CCF, just
> > as when the RTC has a clock output.
> > 
> > 
> 
> I understand your point, but taking into account the fact that FIRC clock
> should be used in most scenarios, would it be acceptable to not export this
> 'clksel' property in the devicetree bindings and simply use the FIRC clock
> by default in the RTC driver ?
> 

No, this doesn't work for RTCs because their lifecycle is longer than the
system's and f you change a configuration from the default value without
providing a way to control it, we won't have any upgrade path without
breaking users.

> At least for this patchset, in order to ease the review process. If
> configurable clock source support would want to be enabled and exported via
> bindings for this S32G2/S32G3 RTC driver, then CCF registration for this clk
> mux could be added in future patches.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

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.

> +properties:
> +  compatible:
> +    const: nxp,s32g-rtc

Also, how come there are not specific compatibles for the two SoCs
supported here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ciprian Marian Costea]

On 9/11/2024 9:22 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.
> 
>> +properties:
>> +  compatible:
>> +    const: nxp,s32g-rtc
> 
> Also, how come there are not specific compatibles for the two SoCs
> supported here?

Hello Conor,

The RTC module is the same for S32G2 and S32G3 SoCs.
Therefore, I did not wanted to add two compatible strings 
('nxp,s32g2-rtc' and 'nxp,s32g3-rtc') when there is no actual difference 
which they could target.

Furthermore, in the future I plan to refactor the common part from [1] 
and [2] files into 's32g.dtsi'. Maybe then such common compatible 
strings would make more sense.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g2.dtsi

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g3.dtsi

Regards,
Ciprian

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On Thu, Sep 12, 2024 at 01:55:34PM +0300, Ciprian Marian Costea wrote:
> On 9/11/2024 9:22 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.
> > 
> > > +properties:
> > > +  compatible:
> > > +    const: nxp,s32g-rtc
> > 
> > Also, how come there are not specific compatibles for the two SoCs
> > supported here?
> 
> Hello Conor,
> 
> The RTC module is the same for S32G2 and S32G3 SoCs.
> Therefore, I did not wanted to add two compatible strings ('nxp,s32g2-rtc'
> and 'nxp,s32g3-rtc') when there is no actual difference which they could
> target.

Are these different fusings of the same silicon, or are they distinctly
different SoCs that happen to share an IP block?

> Furthermore, in the future I plan to refactor the common part from [1] and
> [2] files into 's32g.dtsi'. Maybe then such common compatible strings would
> make more sense.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g2.dtsi
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g3.dtsi
> 
> Regards,
> Ciprian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ciprian Marian Costea]

On 9/12/2024 2:13 PM, Conor Dooley wrote:
> On Thu, Sep 12, 2024 at 01:55:34PM +0300, Ciprian Marian Costea wrote:
>> On 9/11/2024 9:22 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.
>>>
>>>> +properties:
>>>> +  compatible:
>>>> +    const: nxp,s32g-rtc
>>>
>>> Also, how come there are not specific compatibles for the two SoCs
>>> supported here?
>>
>> Hello Conor,
>>
>> The RTC module is the same for S32G2 and S32G3 SoCs.
>> Therefore, I did not wanted to add two compatible strings ('nxp,s32g2-rtc'
>> and 'nxp,s32g3-rtc') when there is no actual difference which they could
>> target.
> 
> Are these different fusings of the same silicon, or are they distinctly
> different SoCs that happen to share an IP block?
> 

S32G2 and S32G3 are different SoCs that share the RTC IP block.

>> Furthermore, in the future I plan to refactor the common part from [1] and
>> [2] files into 's32g.dtsi'. Maybe then such common compatible strings would
>> make more sense.
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g2.dtsi
>>
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/s32g3.dtsi
>>
>> Regards,
>> Ciprian

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Thu, Sep 12, 2024 at 03:00:20PM +0300, Ciprian Marian Costea wrote:
> On 9/12/2024 2:13 PM, Conor Dooley wrote:
> > On Thu, Sep 12, 2024 at 01:55:34PM +0300, Ciprian Marian Costea wrote:
> > > On 9/11/2024 9:22 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.
> > > > 
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: nxp,s32g-rtc
> > > > 
> > > > Also, how come there are not specific compatibles for the two SoCs
> > > > supported here?
> > > 
> > > The RTC module is the same for S32G2 and S32G3 SoCs.
> > > Therefore, I did not wanted to add two compatible strings ('nxp,s32g2-rtc'
> > > and 'nxp,s32g3-rtc') when there is no actual difference which they could
> > > target.
> > 
> > Are these different fusings of the same silicon, or are they distinctly
> > different SoCs that happen to share an IP block?
> > 
> 
> S32G2 and S32G3 are different SoCs that share the RTC IP block.

In that case, I'd expect there to be two compatibles, one for each SoC.
One can then fall back to the other, so the driver only has to be aware
of one compatible. Had they been different fusings of the same silicon,
thus sharing the same integration etc, a generic compatible would have
been fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ciprian Marian Costea]

On 9/12/2024 3:12 PM, Conor Dooley wrote:
> On Thu, Sep 12, 2024 at 03:00:20PM +0300, Ciprian Marian Costea wrote:
>> On 9/12/2024 2:13 PM, Conor Dooley wrote:
>>> On Thu, Sep 12, 2024 at 01:55:34PM +0300, Ciprian Marian Costea wrote:
>>>> On 9/11/2024 9:22 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.
>>>>>
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: nxp,s32g-rtc
>>>>>
>>>>> Also, how come there are not specific compatibles for the two SoCs
>>>>> supported here?
>>>>
>>>> The RTC module is the same for S32G2 and S32G3 SoCs.
>>>> Therefore, I did not wanted to add two compatible strings ('nxp,s32g2-rtc'
>>>> and 'nxp,s32g3-rtc') when there is no actual difference which they could
>>>> target.
>>>
>>> Are these different fusings of the same silicon, or are they distinctly
>>> different SoCs that happen to share an IP block?
>>>
>>
>> S32G2 and S32G3 are different SoCs that share the RTC IP block.
> 
> In that case, I'd expect there to be two compatibles, one for each SoC.
> One can then fall back to the other, so the driver only has to be aware
> of one compatible. Had they been different fusings of the same silicon,
> thus sharing the same integration etc, a generic compatible would have
> been fine.

I understand your point. I will update accordingly in V2 of this patchset.

[PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Add a RTC driver for NXP S32G2/S32G3 SoCs.

The RTC module is used to enable Suspend to RAM (STR) support
on NXP S32G2/S32G3 SoC based boards.
RTC tracks clock time during system suspend.

Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
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>
---
 drivers/rtc/Kconfig    |  10 +
 drivers/rtc/Makefile   |   1 +
 drivers/rtc/rtc-s32g.c | 689 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 700 insertions(+)
 create mode 100644 drivers/rtc/rtc-s32g.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2a95b05982ad..552eecd78f88 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -2043,4 +2043,14 @@ config RTC_DRV_SSD202D
 	  This driver can also be built as a module, if so, the module
 	  will be called "rtc-ssd20xd".
 
+config RTC_DRV_S32G
+	tristate "RTC driver for S32G2/S32G3 SoCs"
+	depends on ARCH_S32 || COMPILE_TEST
+	help
+	  Say yes to enable RTC driver for platforms based on the
+	  S32G2/S32G3 SoC family.
+
+	  This RTC module can be used as a wakeup source.
+	  Please note that it is not battery-powered.
+
 endif # RTC_CLASS
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 3004e372f25f..36d2ed5d0ae2 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -157,6 +157,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
 obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
 obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
 obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
+obj-$(CONFIG_RTC_DRV_S32G)	+= rtc-s32g.o
 obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
 obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
 obj-$(CONFIG_RTC_DRV_S5M)	+= rtc-s5m.o
diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
new file mode 100644
index 000000000000..e77ff0c065ba
--- /dev/null
+++ b/drivers/rtc/rtc-s32g.c
@@ -0,0 +1,689 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/of_irq.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/iopoll.h>
+
+#define RTCSUPV_OFFSET	0x0ul
+#define RTCC_OFFSET	0x4ul
+#define RTCS_OFFSET	0x8ul
+#define RTCCNT_OFFSET	0xCul
+#define APIVAL_OFFSET	0x10ul
+#define RTCVAL_OFFSET	0x14ul
+
+/* RTCSUPV fields */
+#define RTCSUPV_SUPV		BIT(31)
+
+/* RTCC fields */
+#define RTCC_CNTEN		BIT(31)
+#define RTCC_RTCIE_SHIFT	30
+#define RTCC_RTCIE		BIT(RTCC_RTCIE_SHIFT)
+#define RTCC_ROVREN		BIT(28)
+#define RTCC_APIEN		BIT(15)
+#define RTCC_APIIE		BIT(14)
+#define RTCC_CLKSEL_MASK	GENMASK(13, 12)
+#define RTCC_CLKSEL(n)		(((n) << 12) & RTCC_CLKSEL_MASK)
+#define RTCC_DIV512EN		BIT(11)
+#define RTCC_DIV32EN		BIT(10)
+
+/* RTCS fields */
+#define RTCS_RTCF		BIT(29)
+#define RTCS_INV_RTC		BIT(18)
+#define RTCS_APIF		BIT(13)
+#define RTCS_ROVRF		BIT(10)
+
+#define ROLLOVER_VAL		0xFFFFFFFFULL
+#define RTC_SYNCH_TIMEOUT	(100 * USEC_PER_MSEC)
+
+/* Clock sources - usable with RTCC_CLKSEL */
+#define S32G_RTC_SOURCE_SIRC	0x0
+#define S32G_RTC_SOURCE_FIRC	0x2
+
+struct rtc_time_base {
+	s64 sec;
+	u64 cycles;
+	u64 rollovers;
+#ifdef CONFIG_PM_SLEEP
+	struct rtc_time tm;
+#endif
+};
+
+struct rtc_priv {
+	struct rtc_device *rdev;
+	struct device *dev;
+	u8 __iomem *rtc_base;
+	struct clk *firc;
+	struct clk *sirc;
+	struct clk *ipg;
+	struct rtc_time_base base;
+	u64 rtc_hz;
+	u64 rollovers;
+	int dt_irq_id;
+	u8 clk_source;
+	bool div512;
+	bool div32;
+};
+
+static u64 cycles_to_sec(u64 hz, u64 cycles)
+{
+	return cycles / hz;
+}
+
+/*
+ * Convert a number of seconds to a value suitable for RTCVAL in our clock's
+ * current configuration.
+ * @rtcval: The value to go into RTCVAL[RTCVAL]
+ * Returns: 0 for success, -EINVAL if @seconds push the counter at least
+ *          twice the rollover interval
+ */
+static int sec_to_rtcval(const struct rtc_priv *priv,
+			 unsigned long seconds, u32 *rtcval)
+{
+	u32 rtccnt, delta_cnt;
+	u32 target_cnt = 0;
+
+	/* For now, support at most one rollover of the counter */
+	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, ULONG_MAX))
+		return -EINVAL;
+
+	/*
+	 * RTCCNT is read-only; we must return a value relative to the
+	 * current value of the counter (and hope we don't linger around
+	 * too much before we get to enable the interrupt)
+	 */
+	delta_cnt = seconds * priv->rtc_hz;
+	rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+
+	if (~rtccnt < delta_cnt)
+		target_cnt = (delta_cnt - ~rtccnt);
+	else
+		target_cnt = rtccnt + delta_cnt;
+
+	/*
+	 * According to RTCVAL register description,
+	 * its minimum value should be 4.
+	 */
+	if (unlikely(target_cnt < 4))
+		target_cnt = 4;
+
+	*rtcval = target_cnt;
+
+	return 0;
+}
+
+static irqreturn_t rtc_handler(int irq, void *dev)
+{
+	struct rtc_priv *priv = platform_get_drvdata(dev);
+	u32 status;
+
+	status = ioread32(priv->rtc_base + RTCS_OFFSET);
+	if (status & RTCS_ROVRF) {
+		if (priv->rollovers == ULONG_MAX)
+			priv->rollovers = 0;
+		else
+			priv->rollovers++;
+	}
+
+	if (status & RTCS_RTCF) {
+		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
+		rtc_update_irq(priv->rdev, 1, RTC_AF);
+	}
+
+	if (status & RTCS_APIF)
+		rtc_update_irq(priv->rdev, 1, RTC_PF);
+
+	iowrite32(status, priv->rtc_base + RTCS_OFFSET);
+
+	return IRQ_HANDLED;
+}
+
+static int get_time_left(struct device *dev, struct rtc_priv *priv,
+			 u32 *sec)
+{
+	u32 rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+	u32 rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
+
+	if (rtcval < rtccnt) {
+		dev_err(dev, "RTC timer expired before entering suspend\n");
+		return -EIO;
+	}
+
+	*sec = cycles_to_sec(priv->rtc_hz, rtcval - rtccnt);
+
+	return 0;
+}
+
+static int s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
+				     u32 offset)
+{
+	u64 cycles, sec, base_cycles;
+	u32 counter;
+
+	counter = ioread32(priv->rtc_base + offset);
+	cycles = priv->rollovers * ROLLOVER_VAL + counter;
+	base_cycles = priv->base.cycles + priv->base.rollovers * ROLLOVER_VAL;
+
+	if (cycles < base_cycles)
+		return -EINVAL;
+
+	cycles -= base_cycles;
+	sec = priv->base.sec + cycles_to_sec(priv->rtc_hz, cycles);
+
+	return sec;
+}
+
+static int s32g_rtc_read_time(struct device *dev,
+			      struct rtc_time *tm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u64 sec;
+
+	if (!tm)
+		return -EINVAL;
+
+	sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
+	if (sec < 0)
+		return -EINVAL;
+
+	rtc_time64_to_tm(sec, tm);
+
+	return 0;
+}
+
+static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 rtcc, sec_left;
+	u64 sec;
+
+	if (!alrm)
+		return -EINVAL;
+
+	sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
+	if (sec < 0)
+		return -EINVAL;
+
+	rtc_time64_to_tm(sec, &alrm->time);
+
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	alrm->enabled = sec && (rtcc & RTCC_RTCIE);
+
+	alrm->pending = 0;
+	if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
+		alrm->pending = !!sec_left;
+
+	return 0;
+}
+
+static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 rtcc;
+
+	if (!priv->dt_irq_id)
+		return -EIO;
+
+	/*
+	 * RTCIE cannot be deasserted because it will also disable the
+	 * rollover interrupt.
+	 */
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	if (enabled)
+		rtcc |= RTCC_RTCIE;
+
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+
+	return 0;
+}
+
+static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	struct rtc_time time_crt;
+	long long t_crt, t_alrm;
+	int ret = 0;
+	u32 rtcval, rtcs;
+
+	iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
+
+	t_alrm = rtc_tm_to_time64(&alrm->time);
+
+	/*
+	 * Assuming the alarm is being set relative to the same time
+	 * returned by our s32g_rtc_read_time callback
+	 */
+	ret = s32g_rtc_read_time(dev, &time_crt);
+	if (ret)
+		return ret;
+
+	t_crt = rtc_tm_to_time64(&time_crt);
+	if (t_alrm <= t_crt) {
+		dev_warn(dev, "Alarm is set in the past\n");
+		return -EINVAL;
+	}
+
+	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
+	if (ret) {
+		dev_warn(dev, "Alarm is set too far in the future\n");
+		return ret;
+	}
+
+	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
+				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
+	if (ret) {
+		dev_err(dev, "Synchronization failed\n");
+		return ret;
+	}
+
+	iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET);
+
+	return 0;
+}
+
+static int s32g_rtc_set_time(struct device *dev,
+			     struct rtc_time *time)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (!time)
+		return -EINVAL;
+
+	priv->base.rollovers = priv->rollovers;
+	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+	priv->base.sec = rtc_tm_to_time64(time);
+
+	return 0;
+}
+
+static const struct rtc_class_ops rtc_ops = {
+	.read_time = s32g_rtc_read_time,
+	.set_time = s32g_rtc_set_time,
+	.read_alarm = s32g_rtc_read_alarm,
+	.set_alarm = s32g_rtc_set_alarm,
+	.alarm_irq_enable = s32g_rtc_alarm_irq_enable,
+};
+
+static void rtc_disable(struct rtc_priv *priv)
+{
+	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+
+	rtcc &= ~RTCC_CNTEN;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static void rtc_enable(struct rtc_priv *priv)
+{
+	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+
+	rtcc |= RTCC_CNTEN;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static int rtc_init(struct rtc_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct clk *sclk;
+	u32 rtcc = 0;
+	u32 clksel;
+	int ret;
+
+	ret = clk_prepare_enable(priv->ipg);
+	if (ret) {
+		dev_err(dev, "Cannot enable 'ipg' clock, error: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(priv->sirc);
+	if (ret) {
+		dev_err(dev, "Cannot enable 'sirc' clock, error: %d\n", ret);
+		goto disable_ipg_clk;
+	}
+
+	ret = clk_prepare_enable(priv->firc);
+	if (ret) {
+		dev_err(dev, "Cannot enable 'firc' clock, error: %d\n", ret);
+		goto disable_sirc_clk;
+	}
+
+	/* Make sure the RTC ticking is disabled before we configure dividers */
+	rtc_disable(priv);
+
+	clksel = RTCC_CLKSEL(priv->clk_source);
+	rtcc |= clksel;
+
+	/* Precompute the base frequency of the clock */
+	switch (clksel) {
+	case RTCC_CLKSEL(S32G_RTC_SOURCE_SIRC):
+		sclk = priv->sirc;
+		break;
+	case RTCC_CLKSEL(S32G_RTC_SOURCE_FIRC):
+		sclk = priv->firc;
+		break;
+	default:
+		dev_err(dev, "Invalid clksel value: %u\n", clksel);
+		ret = -EINVAL;
+		goto disable_firc_clk;
+	}
+
+	priv->rtc_hz = clk_get_rate(sclk);
+	if (!priv->rtc_hz) {
+		dev_err(dev, "Invalid RTC frequency\n");
+		ret = -EINVAL;
+		goto disable_firc_clk;
+	}
+
+	/* Adjust frequency if dividers are enabled */
+	if (priv->div512) {
+		rtcc |= RTCC_DIV512EN;
+		priv->rtc_hz /= 512;
+	}
+
+	if (priv->div32) {
+		rtcc |= RTCC_DIV32EN;
+		priv->rtc_hz /= 32;
+	}
+
+	rtcc |= RTCC_RTCIE | RTCC_ROVREN;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+
+	return 0;
+
+disable_firc_clk:
+	clk_disable_unprepare(priv->firc);
+disable_sirc_clk:
+	clk_disable_unprepare(priv->sirc);
+disable_ipg_clk:
+	clk_disable_unprepare(priv->ipg);
+	return ret;
+}
+
+static int priv_dts_init(struct rtc_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	u32 clksel = S32G_RTC_SOURCE_SIRC;
+	/* div512 and div32 */
+	u32 div[2] = { 0 };
+	int ret;
+
+	priv->sirc = devm_clk_get(dev, "sirc");
+	if (IS_ERR(priv->sirc)) {
+		dev_err(dev, "Failed to get 'sirc' clock\n");
+		return PTR_ERR(priv->sirc);
+	}
+
+	priv->firc = devm_clk_get(dev, "firc");
+	if (IS_ERR(priv->firc)) {
+		dev_err(dev, "Failed to get 'firc' clock\n");
+		return PTR_ERR(priv->firc);
+	}
+
+	priv->ipg = devm_clk_get(dev, "ipg");
+	if (IS_ERR(priv->ipg)) {
+		dev_err(dev, "Failed to get 'ipg' clock\n");
+		return PTR_ERR(priv->ipg);
+	}
+
+	priv->dt_irq_id = platform_get_irq(pdev, 0);
+	if (priv->dt_irq_id < 0) {
+		dev_err(dev, "Error reading interrupt # from dts\n");
+		return priv->dt_irq_id;
+	}
+
+	ret = device_property_read_u32_array(dev, "nxp,dividers", div,
+					     ARRAY_SIZE(div));
+	if (ret) {
+		dev_err(dev, "Error reading dividers configuration, err: %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "nxp,clksel", &clksel);
+	if (ret) {
+		dev_err(dev, "Error reading clksel configuration, err: %d\n", ret);
+		return ret;
+	}
+
+	priv->div512 = !!div[0];
+	priv->div32 = !!div[1];
+
+	switch (clksel) {
+	case S32G_RTC_SOURCE_SIRC:
+	case S32G_RTC_SOURCE_FIRC:
+		priv->clk_source = clksel;
+		break;
+	default:
+		dev_err(dev, "Unsupported clksel: %d\n", clksel);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtc_priv *priv;
+	int ret = 0;
+
+	priv = devm_kzalloc(dev, sizeof(struct rtc_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->rtc_base)) {
+		dev_err(dev, "Failed to map registers\n");
+		return PTR_ERR(priv->rtc_base);
+	}
+
+	device_init_wakeup(dev, true);
+	priv->dev = dev;
+
+	ret = priv_dts_init(priv);
+	if (ret)
+		return ret;
+
+	ret = rtc_init(priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+	rtc_enable(priv);
+
+	priv->rdev = devm_rtc_device_register(dev, "s32g_rtc",
+					      &rtc_ops, THIS_MODULE);
+	if (IS_ERR_OR_NULL(priv->rdev)) {
+		dev_err(dev, "Error registering RTC device, err: %ld\n",
+			PTR_ERR(priv->rdev));
+		ret = PTR_ERR(priv->rdev);
+		goto disable_rtc;
+	}
+
+	ret = devm_request_irq(dev, priv->dt_irq_id,
+			       rtc_handler, 0, dev_name(dev), pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
+			priv->dt_irq_id, ret);
+		goto disable_rtc;
+	}
+
+	return 0;
+
+disable_rtc:
+	rtc_disable(priv);
+	return ret;
+}
+
+static void rtc_remove(struct platform_device *pdev)
+{
+	struct rtc_priv *priv = platform_get_drvdata(pdev);
+
+	rtc_disable(priv);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void enable_api_irq(struct device *dev, unsigned int enabled)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
+	u32 rtcc;
+
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	if (enabled)
+		rtcc |= api_irq;
+	else
+		rtcc &= ~api_irq;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static int adjust_dividers(u32 sec, struct rtc_priv *priv)
+{
+	u64 rtcval_max = U32_MAX;
+	u64 rtcval;
+
+	priv->div32 = 0;
+	priv->div512 = 0;
+
+	rtcval = sec * priv->rtc_hz;
+	if (rtcval < rtcval_max)
+		return 0;
+
+	if (rtcval / 32 < rtcval_max) {
+		priv->div32 = 1;
+		return 0;
+	}
+
+	if (rtcval / 512 < rtcval_max) {
+		priv->div512 = 1;
+		return 0;
+	}
+
+	if (rtcval / (512 * 32) < rtcval_max) {
+		priv->div32 = 1;
+		priv->div512 = 1;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int rtc_suspend(struct device *dev)
+{
+	struct rtc_priv *init_priv = dev_get_drvdata(dev);
+	struct rtc_priv priv;
+	long long base_sec;
+	int ret = 0;
+	u32 rtcval;
+	u32 sec;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	/* Save last known timestamp before we switch clocks and reinit RTC */
+	ret = s32g_rtc_read_time(dev, &priv.base.tm);
+	if (ret)
+		return ret;
+
+	if (init_priv->clk_source == S32G_RTC_SOURCE_SIRC)
+		return 0;
+
+	/*
+	 * Use a local copy of the RTC control block to
+	 * avoid restoring it on resume path.
+	 */
+	memcpy(&priv, init_priv, sizeof(priv));
+
+	/* Switch to SIRC */
+	priv.clk_source = S32G_RTC_SOURCE_SIRC;
+
+	ret = get_time_left(dev, init_priv, &sec);
+	if (ret)
+		return ret;
+
+	/* Adjust for the number of seconds we'll be asleep */
+	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
+	base_sec += sec;
+	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
+
+	rtc_disable(&priv);
+
+	ret = adjust_dividers(sec, &priv);
+	if (ret) {
+		dev_err(dev, "Failed to adjust RTC dividers to match a %u seconds delay\n", sec);
+		return ret;
+	}
+
+	ret = rtc_init(&priv);
+	if (ret)
+		return ret;
+
+	ret = sec_to_rtcval(&priv, sec, &rtcval);
+	if (ret) {
+		dev_warn(dev, "Alarm is too far in the future\n");
+		return ret;
+	}
+
+	s32g_rtc_alarm_irq_enable(dev, 0);
+	enable_api_irq(dev, 1);
+	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
+	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
+
+	rtc_enable(&priv);
+
+	return ret;
+}
+
+static int rtc_resume(struct device *dev)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	/* Disable wake-up interrupts */
+	enable_api_irq(dev, 0);
+
+	/* Reinitialize the driver using the initial settings */
+	ret = rtc_init(priv);
+	if (ret)
+		return ret;
+
+	rtc_enable(priv);
+
+	/*
+	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
+	 * reapply the saved time settings
+	 */
+	return s32g_rtc_set_time(dev, &priv->base.tm);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct of_device_id rtc_dt_ids[] = {
+	{.compatible = "nxp,s32g-rtc" },
+	{ /* sentinel */ },
+};
+
+static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
+			 rtc_suspend, rtc_resume);
+
+static struct platform_driver rtc_driver = {
+	.driver		= {
+		.name			= "s32g-rtc",
+		.pm				= &rtc_pm_ops,
+		.of_match_table = of_match_ptr(rtc_dt_ids),
+	},
+	.probe		= rtc_probe,
+	.remove_new	= rtc_remove,
+};
+module_platform_driver(rtc_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[kernel test robot]

Hi Ciprian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on robh/for-next arm64/for-next/core linus/master v6.11-rc7 next-20240911]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20240911-150205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20240911070028.127659-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: sh-randconfig-r073-20240912 (https://download.01.org/0day-ci/archive/20240912/202409121103.EX9BTDFT-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240912/202409121103.EX9BTDFT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409121103.EX9BTDFT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/rtc/rtc-s32g.c:668:34: warning: 'rtc_dt_ids' defined but not used [-Wunused-const-variable=]
     668 | static const struct of_device_id rtc_dt_ids[] = {
         |                                  ^~~~~~~~~~


vim +/rtc_dt_ids +668 drivers/rtc/rtc-s32g.c

   667	
 > 668	static const struct of_device_id rtc_dt_ids[] = {
   669		{.compatible = "nxp,s32g-rtc" },
   670		{ /* sentinel */ },
   671	};
   672	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[kernel test robot]

Hi Ciprian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on robh/for-next arm64/for-next/core linus/master v6.11-rc7 next-20240912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20240911-150205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20240911070028.127659-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: hexagon-randconfig-r112-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131950.ozDVVv5X-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project bf684034844c660b778f0eba103582f582b710c9)
reproduce: (https://download.01.org/0day-ci/archive/20240913/202409131950.ozDVVv5X-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131950.ozDVVv5X-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/rtc/rtc-s32g.c:7:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/rtc/rtc-s32g.c:7:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/rtc/rtc-s32g.c:7:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/rtc/rtc-s32g.c:668:34: warning: unused variable 'rtc_dt_ids' [-Wunused-const-variable]
     668 | static const struct of_device_id rtc_dt_ids[] = {
         |                                  ^~~~~~~~~~
   7 warnings generated.


vim +/rtc_dt_ids +668 drivers/rtc/rtc-s32g.c

   667	
 > 668	static const struct of_device_id rtc_dt_ids[] = {
   669		{.compatible = "nxp,s32g-rtc" },
   670		{ /* sentinel */ },
   671	};
   672	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Krzysztof Kozlowski]

On 11/09/2024 09:00, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Add a RTC driver for NXP S32G2/S32G3 SoCs.
> 
> The RTC module is used to enable Suspend to RAM (STR) support
> on NXP S32G2/S32G3 SoC based boards.
> RTC tracks clock time during system suspend.
> 

...

> +	priv->div512 = !!div[0];
> +	priv->div32 = !!div[1];
> +
> +	switch (clksel) {
> +	case S32G_RTC_SOURCE_SIRC:
> +	case S32G_RTC_SOURCE_FIRC:
> +		priv->clk_source = clksel;
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported clksel: %d\n", clksel);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rtc_priv *priv;
> +	int ret = 0;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rtc_priv),

sizeof(*)

> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->rtc_base)) {
> +		dev_err(dev, "Failed to map registers\n");
> +		return PTR_ERR(priv->rtc_base);

return dev_err_probe

> +	}
> +
> +	device_init_wakeup(dev, true);
> +	priv->dev = dev;
> +
> +	ret = priv_dts_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +	rtc_enable(priv);
> +
> +	priv->rdev = devm_rtc_device_register(dev, "s32g_rtc",
> +					      &rtc_ops, THIS_MODULE);
> +	if (IS_ERR_OR_NULL(priv->rdev)) {
> +		dev_err(dev, "Error registering RTC device, err: %ld\n",
> +			PTR_ERR(priv->rdev));
> +		ret = PTR_ERR(priv->rdev);
> +		goto disable_rtc;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->dt_irq_id,
> +			       rtc_handler, 0, dev_name(dev), pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
> +			priv->dt_irq_id, ret);
> +		goto disable_rtc;
> +	}
> +
> +	return 0;
> +
> +disable_rtc:
> +	rtc_disable(priv);
> +	return ret;
> +}
> +
> +static void rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_priv *priv = platform_get_drvdata(pdev);
> +
> +	rtc_disable(priv);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void enable_api_irq(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
> +	u32 rtcc;
> +
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	if (enabled)
> +		rtcc |= api_irq;
> +	else
> +		rtcc &= ~api_irq;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static int adjust_dividers(u32 sec, struct rtc_priv *priv)
> +{
> +	u64 rtcval_max = U32_MAX;
> +	u64 rtcval;
> +
> +	priv->div32 = 0;
> +	priv->div512 = 0;
> +
> +	rtcval = sec * priv->rtc_hz;
> +	if (rtcval < rtcval_max)
> +		return 0;
> +
> +	if (rtcval / 32 < rtcval_max) {
> +		priv->div32 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / 512 < rtcval_max) {
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / (512 * 32) < rtcval_max) {
> +		priv->div32 = 1;
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int rtc_suspend(struct device *dev)
> +{
> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
> +	struct rtc_priv priv;
> +	long long base_sec;
> +	int ret = 0;
> +	u32 rtcval;
> +	u32 sec;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Save last known timestamp before we switch clocks and reinit RTC */
> +	ret = s32g_rtc_read_time(dev, &priv.base.tm);
> +	if (ret)
> +		return ret;
> +
> +	if (init_priv->clk_source == S32G_RTC_SOURCE_SIRC)
> +		return 0;
> +
> +	/*
> +	 * Use a local copy of the RTC control block to
> +	 * avoid restoring it on resume path.
> +	 */
> +	memcpy(&priv, init_priv, sizeof(priv));
> +
> +	/* Switch to SIRC */
> +	priv.clk_source = S32G_RTC_SOURCE_SIRC;
> +
> +	ret = get_time_left(dev, init_priv, &sec);
> +	if (ret)
> +		return ret;
> +
> +	/* Adjust for the number of seconds we'll be asleep */
> +	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
> +	base_sec += sec;
> +	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
> +
> +	rtc_disable(&priv);
> +
> +	ret = adjust_dividers(sec, &priv);
> +	if (ret) {
> +		dev_err(dev, "Failed to adjust RTC dividers to match a %u seconds delay\n", sec);
> +		return ret;
> +	}
> +
> +	ret = rtc_init(&priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = sec_to_rtcval(&priv, sec, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is too far in the future\n");
> +		return ret;
> +	}
> +
> +	s32g_rtc_alarm_irq_enable(dev, 0);
> +	enable_api_irq(dev, 1);
> +	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
> +	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
> +
> +	rtc_enable(&priv);
> +
> +	return ret;
> +}
> +
> +static int rtc_resume(struct device *dev)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Disable wake-up interrupts */
> +	enable_api_irq(dev, 0);
> +
> +	/* Reinitialize the driver using the initial settings */
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	rtc_enable(priv);
> +
> +	/*
> +	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
> +	 * reapply the saved time settings
> +	 */
> +	return s32g_rtc_set_time(dev, &priv->base.tm);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct of_device_id rtc_dt_ids[] = {
> +	{.compatible = "nxp,s32g-rtc" },
> +	{ /* sentinel */ },
> +};
> +
> +static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
> +			 rtc_suspend, rtc_resume);
> +
> +static struct platform_driver rtc_driver = {
> +	.driver		= {
> +		.name			= "s32g-rtc",
> +		.pm				= &rtc_pm_ops,
> +		.of_match_table = of_match_ptr(rtc_dt_ids),

Drop of_match_ptr, you have here warning.

> +	},
> +	.probe		= rtc_probe,
> +	.remove_new	= rtc_remove,
> +};
> +module_platform_driver(rtc_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 9/17/2024 8:40 PM, Krzysztof Kozlowski wrote:
> On 11/09/2024 09:00, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>>
>> The RTC module is used to enable Suspend to RAM (STR) support
>> on NXP S32G2/S32G3 SoC based boards.
>> RTC tracks clock time during system suspend.
>>
> 
> ...
> 
>> +static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
>> +			 rtc_suspend, rtc_resume);
>> +
>> +static struct platform_driver rtc_driver = {
>> +	.driver		= {
>> +		.name			= "s32g-rtc",
>> +		.pm				= &rtc_pm_ops,
>> +		.of_match_table = of_match_ptr(rtc_dt_ids),
> 
> Drop of_match_ptr, you have here warning.
> 
>> +	},
>> +	.probe		= rtc_probe,
>> +	.remove_new	= rtc_remove,
>> +};
>> +module_platform_driver(rtc_driver);
>> +
>> +MODULE_AUTHOR("NXP");
>> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
>> +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof
> 

Hello Krzysztof,

Thank you for your review.
I will address your findings in V2.

Best Regards,
Ciprian

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Alexandre Belloni]

On 11/09/2024 10:00:26+0300, Ciprian Costea wrote:
> +struct rtc_time_base {
> +	s64 sec;
> +	u64 cycles;
> +	u64 rollovers;

The whole rollovers implementation seems useless as the rollover may
happen when Linux is off.

> +#ifdef CONFIG_PM_SLEEP
> +	struct rtc_time tm;
> +#endif
> +};
> +
> +struct rtc_priv {
> +	struct rtc_device *rdev;
> +	struct device *dev;
> +	u8 __iomem *rtc_base;
> +	struct clk *firc;
> +	struct clk *sirc;
> +	struct clk *ipg;
> +	struct rtc_time_base base;
> +	u64 rtc_hz;
> +	u64 rollovers;
> +	int dt_irq_id;
> +	u8 clk_source;
> +	bool div512;
> +	bool div32;
> +};
> +
> +static u64 cycles_to_sec(u64 hz, u64 cycles)
> +{
> +	return cycles / hz;
> +}
> +
> +/*
> + * Convert a number of seconds to a value suitable for RTCVAL in our clock's
> + * current configuration.
> + * @rtcval: The value to go into RTCVAL[RTCVAL]
> + * Returns: 0 for success, -EINVAL if @seconds push the counter at least
> + *          twice the rollover interval
> + */
> +static int sec_to_rtcval(const struct rtc_priv *priv,
> +			 unsigned long seconds, u32 *rtcval)
> +{
> +	u32 rtccnt, delta_cnt;
> +	u32 target_cnt = 0;
> +
> +	/* For now, support at most one rollover of the counter */
> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, ULONG_MAX))
> +		return -EINVAL;
> +
> +	/*
> +	 * RTCCNT is read-only; we must return a value relative to the
> +	 * current value of the counter (and hope we don't linger around
> +	 * too much before we get to enable the interrupt)
> +	 */
> +	delta_cnt = seconds * priv->rtc_hz;
> +	rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +
> +	if (~rtccnt < delta_cnt)
> +		target_cnt = (delta_cnt - ~rtccnt);
> +	else
> +		target_cnt = rtccnt + delta_cnt;
> +
> +	/*
> +	 * According to RTCVAL register description,
> +	 * its minimum value should be 4.
> +	 */
> +	if (unlikely(target_cnt < 4))
> +		target_cnt = 4;
> +
> +	*rtcval = target_cnt;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rtc_handler(int irq, void *dev)
> +{
> +	struct rtc_priv *priv = platform_get_drvdata(dev);
> +	u32 status;
> +
> +	status = ioread32(priv->rtc_base + RTCS_OFFSET);
> +	if (status & RTCS_ROVRF) {
> +		if (priv->rollovers == ULONG_MAX)
> +			priv->rollovers = 0;
> +		else
> +			priv->rollovers++;
> +	}
> +
> +	if (status & RTCS_RTCF) {
> +		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
> +		rtc_update_irq(priv->rdev, 1, RTC_AF);
> +	}
> +
> +	if (status & RTCS_APIF)
> +		rtc_update_irq(priv->rdev, 1, RTC_PF);
> +
> +	iowrite32(status, priv->rtc_base + RTCS_OFFSET);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int get_time_left(struct device *dev, struct rtc_priv *priv,
> +			 u32 *sec)
> +{
> +	u32 rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +	u32 rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
> +
> +	if (rtcval < rtccnt) {
> +		dev_err(dev, "RTC timer expired before entering suspend\n");
> +		return -EIO;
> +	}
> +
> +	*sec = cycles_to_sec(priv->rtc_hz, rtcval - rtccnt);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
> +				     u32 offset)
> +{
> +	u64 cycles, sec, base_cycles;
> +	u32 counter;
> +
> +	counter = ioread32(priv->rtc_base + offset);
> +	cycles = priv->rollovers * ROLLOVER_VAL + counter;
> +	base_cycles = priv->base.cycles + priv->base.rollovers * ROLLOVER_VAL;
> +
> +	if (cycles < base_cycles)
> +		return -EINVAL;
> +
> +	cycles -= base_cycles;
> +	sec = priv->base.sec + cycles_to_sec(priv->rtc_hz, cycles);

What happens after you reboot?

This doesn't seem to be a proper RTC, unless you have some NVMEM to
store the offset between the current time and the value of the counter.
As-is, the driver is not working.

> +
> +	return sec;
> +}
> +
> +static int s32g_rtc_read_time(struct device *dev,
> +			      struct rtc_time *tm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u64 sec;
> +
> +	if (!tm)
> +		return -EINVAL;
> +
> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
> +	if (sec < 0)
> +		return -EINVAL;
> +
> +	rtc_time64_to_tm(sec, tm);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 rtcc, sec_left;
> +	u64 sec;
> +
> +	if (!alrm)
> +		return -EINVAL;
> +
> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
> +	if (sec < 0)
> +		return -EINVAL;
> +
> +	rtc_time64_to_tm(sec, &alrm->time);
> +
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	alrm->enabled = sec && (rtcc & RTCC_RTCIE);
> +
> +	alrm->pending = 0;
> +	if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
> +		alrm->pending = !!sec_left;
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 rtcc;
> +
> +	if (!priv->dt_irq_id)
> +		return -EIO;
> +
> +	/*
> +	 * RTCIE cannot be deasserted because it will also disable the
> +	 * rollover interrupt.
> +	 */
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	if (enabled)
> +		rtcc |= RTCC_RTCIE;
> +
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	struct rtc_time time_crt;
> +	long long t_crt, t_alrm;
> +	int ret = 0;
> +	u32 rtcval, rtcs;
> +
> +	iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
> +
> +	t_alrm = rtc_tm_to_time64(&alrm->time);
> +
> +	/*
> +	 * Assuming the alarm is being set relative to the same time
> +	 * returned by our s32g_rtc_read_time callback
> +	 */
> +	ret = s32g_rtc_read_time(dev, &time_crt);
> +	if (ret)
> +		return ret;
> +
> +	t_crt = rtc_tm_to_time64(&time_crt);
> +	if (t_alrm <= t_crt) {
> +		dev_warn(dev, "Alarm is set in the past\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is set too far in the future\n");
> +		return ret;
> +	}
> +
> +	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
> +				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
> +	if (ret) {
> +		dev_err(dev, "Synchronization failed\n");
> +		return ret;
> +	}
> +
> +	iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_set_time(struct device *dev,
> +			     struct rtc_time *time)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +
> +	if (!time)
> +		return -EINVAL;
> +
> +	priv->base.rollovers = priv->rollovers;
> +	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +	priv->base.sec = rtc_tm_to_time64(time);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops rtc_ops = {
> +	.read_time = s32g_rtc_read_time,
> +	.set_time = s32g_rtc_set_time,
> +	.read_alarm = s32g_rtc_read_alarm,
> +	.set_alarm = s32g_rtc_set_alarm,
> +	.alarm_irq_enable = s32g_rtc_alarm_irq_enable,
> +};
> +
> +static void rtc_disable(struct rtc_priv *priv)
> +{
> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +
> +	rtcc &= ~RTCC_CNTEN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static void rtc_enable(struct rtc_priv *priv)
> +{
> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +
> +	rtcc |= RTCC_CNTEN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static int rtc_init(struct rtc_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct clk *sclk;
> +	u32 rtcc = 0;
> +	u32 clksel;
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->ipg);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'ipg' clock, error: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(priv->sirc);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'sirc' clock, error: %d\n", ret);
> +		goto disable_ipg_clk;
> +	}
> +
> +	ret = clk_prepare_enable(priv->firc);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'firc' clock, error: %d\n", ret);
> +		goto disable_sirc_clk;
> +	}
> +
> +	/* Make sure the RTC ticking is disabled before we configure dividers */
> +	rtc_disable(priv);

At this point, you lost the RTC accuracy, this must not be done on probe
> +
> +	clksel = RTCC_CLKSEL(priv->clk_source);
> +	rtcc |= clksel;
> +
> +	/* Precompute the base frequency of the clock */
> +	switch (clksel) {
> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_SIRC):
> +		sclk = priv->sirc;
> +		break;
> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_FIRC):
> +		sclk = priv->firc;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid clksel value: %u\n", clksel);
> +		ret = -EINVAL;
> +		goto disable_firc_clk;
> +	}
> +
> +	priv->rtc_hz = clk_get_rate(sclk);
> +	if (!priv->rtc_hz) {
> +		dev_err(dev, "Invalid RTC frequency\n");
> +		ret = -EINVAL;
> +		goto disable_firc_clk;
> +	}
> +
> +	/* Adjust frequency if dividers are enabled */
> +	if (priv->div512) {
> +		rtcc |= RTCC_DIV512EN;
> +		priv->rtc_hz /= 512;
> +	}
> +
> +	if (priv->div32) {
> +		rtcc |= RTCC_DIV32EN;
> +		priv->rtc_hz /= 32;
> +	}
> +
> +	rtcc |= RTCC_RTCIE | RTCC_ROVREN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +
> +	return 0;
> +
> +disable_firc_clk:
> +	clk_disable_unprepare(priv->firc);
> +disable_sirc_clk:
> +	clk_disable_unprepare(priv->sirc);
> +disable_ipg_clk:
> +	clk_disable_unprepare(priv->ipg);
> +	return ret;
> +}
> +
> +static int priv_dts_init(struct rtc_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	u32 clksel = S32G_RTC_SOURCE_SIRC;
> +	/* div512 and div32 */
> +	u32 div[2] = { 0 };
> +	int ret;
> +
> +	priv->sirc = devm_clk_get(dev, "sirc");
> +	if (IS_ERR(priv->sirc)) {
> +		dev_err(dev, "Failed to get 'sirc' clock\n");
> +		return PTR_ERR(priv->sirc);
> +	}
> +
> +	priv->firc = devm_clk_get(dev, "firc");
> +	if (IS_ERR(priv->firc)) {
> +		dev_err(dev, "Failed to get 'firc' clock\n");
> +		return PTR_ERR(priv->firc);
> +	}
> +
> +	priv->ipg = devm_clk_get(dev, "ipg");
> +	if (IS_ERR(priv->ipg)) {
> +		dev_err(dev, "Failed to get 'ipg' clock\n");
> +		return PTR_ERR(priv->ipg);
> +	}
> +
> +	priv->dt_irq_id = platform_get_irq(pdev, 0);
> +	if (priv->dt_irq_id < 0) {
> +		dev_err(dev, "Error reading interrupt # from dts\n");
> +		return priv->dt_irq_id;
> +	}
> +
> +	ret = device_property_read_u32_array(dev, "nxp,dividers", div,
> +					     ARRAY_SIZE(div));
> +	if (ret) {
> +		dev_err(dev, "Error reading dividers configuration, err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "nxp,clksel", &clksel);
> +	if (ret) {
> +		dev_err(dev, "Error reading clksel configuration, err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->div512 = !!div[0];
> +	priv->div32 = !!div[1];
> +
> +	switch (clksel) {
> +	case S32G_RTC_SOURCE_SIRC:
> +	case S32G_RTC_SOURCE_FIRC:
> +		priv->clk_source = clksel;
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported clksel: %d\n", clksel);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rtc_priv *priv;
> +	int ret = 0;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rtc_priv),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->rtc_base)) {
> +		dev_err(dev, "Failed to map registers\n");
> +		return PTR_ERR(priv->rtc_base);
> +	}
> +
> +	device_init_wakeup(dev, true);
> +	priv->dev = dev;
> +
> +	ret = priv_dts_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +	rtc_enable(priv);
> +
> +	priv->rdev = devm_rtc_device_register(dev, "s32g_rtc",
> +					      &rtc_ops, THIS_MODULE);

This is deprecated, please use devm_rtc_allocate_device and devm_rtc_register_device

> +	if (IS_ERR_OR_NULL(priv->rdev)) {
> +		dev_err(dev, "Error registering RTC device, err: %ld\n",
> +			PTR_ERR(priv->rdev));
> +		ret = PTR_ERR(priv->rdev);
> +		goto disable_rtc;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->dt_irq_id,
> +			       rtc_handler, 0, dev_name(dev), pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
> +			priv->dt_irq_id, ret);
> +		goto disable_rtc;

You must not fail after registering the RTC, else your driver will be
opened to a race condition;

> +	}
> +
> +	return 0;
> +
> +disable_rtc:
> +	rtc_disable(priv);
> +	return ret;
> +}
> +
> +static void rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_priv *priv = platform_get_drvdata(pdev);
> +
> +	rtc_disable(priv);

Disabling the RTC when shutting down renders the RTC useless.

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void enable_api_irq(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
> +	u32 rtcc;
> +
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	if (enabled)
> +		rtcc |= api_irq;
> +	else
> +		rtcc &= ~api_irq;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static int adjust_dividers(u32 sec, struct rtc_priv *priv)
> +{
> +	u64 rtcval_max = U32_MAX;
> +	u64 rtcval;
> +
> +	priv->div32 = 0;
> +	priv->div512 = 0;
> +
> +	rtcval = sec * priv->rtc_hz;
> +	if (rtcval < rtcval_max)
> +		return 0;
> +
> +	if (rtcval / 32 < rtcval_max) {
> +		priv->div32 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / 512 < rtcval_max) {
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / (512 * 32) < rtcval_max) {
> +		priv->div32 = 1;
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int rtc_suspend(struct device *dev)
> +{
> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
> +	struct rtc_priv priv;
> +	long long base_sec;
> +	int ret = 0;
> +	u32 rtcval;
> +	u32 sec;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Save last known timestamp before we switch clocks and reinit RTC */
> +	ret = s32g_rtc_read_time(dev, &priv.base.tm);
> +	if (ret)
> +		return ret;
> +
> +	if (init_priv->clk_source == S32G_RTC_SOURCE_SIRC)
> +		return 0;
> +
> +	/*
> +	 * Use a local copy of the RTC control block to
> +	 * avoid restoring it on resume path.
> +	 */
> +	memcpy(&priv, init_priv, sizeof(priv));
> +
> +	/* Switch to SIRC */
> +	priv.clk_source = S32G_RTC_SOURCE_SIRC;
> +
> +	ret = get_time_left(dev, init_priv, &sec);
> +	if (ret)
> +		return ret;
> +
> +	/* Adjust for the number of seconds we'll be asleep */
> +	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
> +	base_sec += sec;
> +	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
> +
> +	rtc_disable(&priv);
> +
> +	ret = adjust_dividers(sec, &priv);
> +	if (ret) {
> +		dev_err(dev, "Failed to adjust RTC dividers to match a %u seconds delay\n", sec);
> +		return ret;
> +	}
> +
> +	ret = rtc_init(&priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = sec_to_rtcval(&priv, sec, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is too far in the future\n");
> +		return ret;
> +	}

All of this seems super fishy.

> +
> +	s32g_rtc_alarm_irq_enable(dev, 0);
> +	enable_api_irq(dev, 1);
> +	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
> +	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
> +
> +	rtc_enable(&priv);
> +
> +	return ret;
> +}
> +
> +static int rtc_resume(struct device *dev)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Disable wake-up interrupts */
> +	enable_api_irq(dev, 0);
> +
> +	/* Reinitialize the driver using the initial settings */
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	rtc_enable(priv);
> +
> +	/*
> +	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
> +	 * reapply the saved time settings
> +	 */
> +	return s32g_rtc_set_time(dev, &priv->base.tm);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct of_device_id rtc_dt_ids[] = {
> +	{.compatible = "nxp,s32g-rtc" },
> +	{ /* sentinel */ },
> +};
> +
> +static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
> +			 rtc_suspend, rtc_resume);
> +
> +static struct platform_driver rtc_driver = {
> +	.driver		= {
> +		.name			= "s32g-rtc",
> +		.pm				= &rtc_pm_ops,
> +		.of_match_table = of_match_ptr(rtc_dt_ids),
> +	},
> +	.probe		= rtc_probe,
> +	.remove_new	= rtc_remove,
> +};
> +module_platform_driver(rtc_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
> +MODULE_LICENSE("GPL");
> -- 
> 2.45.2
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 9/18/2024 1:26 PM, Alexandre Belloni wrote:
> On 11/09/2024 10:00:26+0300, Ciprian Costea wrote:
>> +struct rtc_time_base {
>> +	s64 sec;
>> +	u64 cycles;
>> +	u64 rollovers;
> 
> The whole rollovers implementation seems useless as the rollover may
> happen when Linux is off.
>

Hello Alexandre,

Thank you for your valuable tehnical feedback on this RTC driver proposal.
I will try, to the best of my knowledge to address your concers.

Regarding the rollover implementation, the Hardware RTC module registers 
present of NXP S32G2/S32G3 SoCs are being reset during system reboot.
On the other hand, during suspend, the RTC module will keep counting if 
a clock source is available. In this case, an internal low-rate 
oscillator (32K -- SIRC) is used, or an external clock can be provided.

The meaning of 'rollover' in this driver refers to a field which will 
count the number of times the RTC counter transitions from value 
0xFFFFFFFF to 0x0. At a 52MHz counting rate (FIRC clock), this would 
mean once in around 18 seconds. Its value is used to report the time 
after multiple rollovers. This can be observed in function 
's32g_rtc_get_time_or_alrm'.

>> +#ifdef CONFIG_PM_SLEEP
>> +	struct rtc_time tm;
>> +#endif
>> +};
>> +
>> +struct rtc_priv {
>> +	struct rtc_device *rdev;
>> +	struct device *dev;
>> +	u8 __iomem *rtc_base;
>> +	struct clk *firc;
>> +	struct clk *sirc;
>> +	struct clk *ipg;
>> +	struct rtc_time_base base;
>> +	u64 rtc_hz;
>> +	u64 rollovers;
>> +	int dt_irq_id;
>> +	u8 clk_source;
>> +	bool div512;
>> +	bool div32;
>> +};
>> +
>> +static u64 cycles_to_sec(u64 hz, u64 cycles)
>> +{
>> +	return cycles / hz;
>> +}
>> +
>> +/*
>> + * Convert a number of seconds to a value suitable for RTCVAL in our clock's
>> + * current configuration.
>> + * @rtcval: The value to go into RTCVAL[RTCVAL]
>> + * Returns: 0 for success, -EINVAL if @seconds push the counter at least
>> + *          twice the rollover interval
>> + */
>> +static int sec_to_rtcval(const struct rtc_priv *priv,
>> +			 unsigned long seconds, u32 *rtcval)
>> +{
>> +	u32 rtccnt, delta_cnt;
>> +	u32 target_cnt = 0;
>> +
>> +	/* For now, support at most one rollover of the counter */
>> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, ULONG_MAX))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * RTCCNT is read-only; we must return a value relative to the
>> +	 * current value of the counter (and hope we don't linger around
>> +	 * too much before we get to enable the interrupt)
>> +	 */
>> +	delta_cnt = seconds * priv->rtc_hz;
>> +	rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +
>> +	if (~rtccnt < delta_cnt)
>> +		target_cnt = (delta_cnt - ~rtccnt);
>> +	else
>> +		target_cnt = rtccnt + delta_cnt;
>> +
>> +	/*
>> +	 * According to RTCVAL register description,
>> +	 * its minimum value should be 4.
>> +	 */
>> +	if (unlikely(target_cnt < 4))
>> +		target_cnt = 4;
>> +
>> +	*rtcval = target_cnt;
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t rtc_handler(int irq, void *dev)
>> +{
>> +	struct rtc_priv *priv = platform_get_drvdata(dev);
>> +	u32 status;
>> +
>> +	status = ioread32(priv->rtc_base + RTCS_OFFSET);
>> +	if (status & RTCS_ROVRF) {
>> +		if (priv->rollovers == ULONG_MAX)
>> +			priv->rollovers = 0;
>> +		else
>> +			priv->rollovers++;
>> +	}
>> +
>> +	if (status & RTCS_RTCF) {
>> +		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
>> +		rtc_update_irq(priv->rdev, 1, RTC_AF);
>> +	}
>> +
>> +	if (status & RTCS_APIF)
>> +		rtc_update_irq(priv->rdev, 1, RTC_PF);
>> +
>> +	iowrite32(status, priv->rtc_base + RTCS_OFFSET);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int get_time_left(struct device *dev, struct rtc_priv *priv,
>> +			 u32 *sec)
>> +{
>> +	u32 rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +	u32 rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
>> +
>> +	if (rtcval < rtccnt) {
>> +		dev_err(dev, "RTC timer expired before entering suspend\n");
>> +		return -EIO;
>> +	}
>> +
>> +	*sec = cycles_to_sec(priv->rtc_hz, rtcval - rtccnt);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
>> +				     u32 offset)
>> +{
>> +	u64 cycles, sec, base_cycles;
>> +	u32 counter;
>> +
>> +	counter = ioread32(priv->rtc_base + offset);
>> +	cycles = priv->rollovers * ROLLOVER_VAL + counter;
>> +	base_cycles = priv->base.cycles + priv->base.rollovers * ROLLOVER_VAL;
>> +
>> +	if (cycles < base_cycles)
>> +		return -EINVAL;
>> +
>> +	cycles -= base_cycles;
>> +	sec = priv->base.sec + cycles_to_sec(priv->rtc_hz, cycles);
> 
> What happens after you reboot?
> 
> This doesn't seem to be a proper RTC, unless you have some NVMEM to
> store the offset between the current time and the value of the counter.
> As-is, the driver is not working.
>

With the existing hardware on S32G2/S32G3, only parts of what Linux 
expects from an RTC can be implemented in software.

Please note that the RTC module is not battery-backed.
Would it be acceptable to return errors for the use cases that cannot be 
fullfilled by the driver and list its limitations in the Kconfig menu ?

In the end, the main goal of this driver is to represent a time-based 
wakeup source for the SoC.

>> +
>> +	return sec;
>> +}
>> +
>> +static int s32g_rtc_read_time(struct device *dev,
>> +			      struct rtc_time *tm)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	u64 sec;
>> +
>> +	if (!tm)
>> +		return -EINVAL;
>> +
>> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
>> +	if (sec < 0)
>> +		return -EINVAL;
>> +
>> +	rtc_time64_to_tm(sec, tm);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	u32 rtcc, sec_left;
>> +	u64 sec;
>> +
>> +	if (!alrm)
>> +		return -EINVAL;
>> +
>> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
>> +	if (sec < 0)
>> +		return -EINVAL;
>> +
>> +	rtc_time64_to_tm(sec, &alrm->time);
>> +
>> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
>> +	alrm->enabled = sec && (rtcc & RTCC_RTCIE);
>> +
>> +	alrm->pending = 0;
>> +	if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
>> +		alrm->pending = !!sec_left;
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	u32 rtcc;
>> +
>> +	if (!priv->dt_irq_id)
>> +		return -EIO;
>> +
>> +	/*
>> +	 * RTCIE cannot be deasserted because it will also disable the
>> +	 * rollover interrupt.
>> +	 */
>> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
>> +	if (enabled)
>> +		rtcc |= RTCC_RTCIE;
>> +
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	struct rtc_time time_crt;
>> +	long long t_crt, t_alrm;
>> +	int ret = 0;
>> +	u32 rtcval, rtcs;
>> +
>> +	iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
>> +
>> +	t_alrm = rtc_tm_to_time64(&alrm->time);
>> +
>> +	/*
>> +	 * Assuming the alarm is being set relative to the same time
>> +	 * returned by our s32g_rtc_read_time callback
>> +	 */
>> +	ret = s32g_rtc_read_time(dev, &time_crt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	t_crt = rtc_tm_to_time64(&time_crt);
>> +	if (t_alrm <= t_crt) {
>> +		dev_warn(dev, "Alarm is set in the past\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
>> +	if (ret) {
>> +		dev_warn(dev, "Alarm is set too far in the future\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
>> +				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
>> +	if (ret) {
>> +		dev_err(dev, "Synchronization failed\n");
>> +		return ret;
>> +	}
>> +
>> +	iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_set_time(struct device *dev,
>> +			     struct rtc_time *time)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +
>> +	if (!time)
>> +		return -EINVAL;
>> +
>> +	priv->base.rollovers = priv->rollovers;
>> +	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +	priv->base.sec = rtc_tm_to_time64(time);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct rtc_class_ops rtc_ops = {
>> +	.read_time = s32g_rtc_read_time,
>> +	.set_time = s32g_rtc_set_time,
>> +	.read_alarm = s32g_rtc_read_alarm,
>> +	.set_alarm = s32g_rtc_set_alarm,
>> +	.alarm_irq_enable = s32g_rtc_alarm_irq_enable,
>> +};
>> +
>> +static void rtc_disable(struct rtc_priv *priv)
>> +{
>> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
>> +
>> +	rtcc &= ~RTCC_CNTEN;
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +}
>> +
>> +static void rtc_enable(struct rtc_priv *priv)
>> +{
>> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
>> +
>> +	rtcc |= RTCC_CNTEN;
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +}
>> +
>> +static int rtc_init(struct rtc_priv *priv)
>> +{
>> +	struct device *dev = priv->dev;
>> +	struct clk *sclk;
>> +	u32 rtcc = 0;
>> +	u32 clksel;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(priv->ipg);
>> +	if (ret) {
>> +		dev_err(dev, "Cannot enable 'ipg' clock, error: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->sirc);
>> +	if (ret) {
>> +		dev_err(dev, "Cannot enable 'sirc' clock, error: %d\n", ret);
>> +		goto disable_ipg_clk;
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->firc);
>> +	if (ret) {
>> +		dev_err(dev, "Cannot enable 'firc' clock, error: %d\n", ret);
>> +		goto disable_sirc_clk;
>> +	}
>> +
>> +	/* Make sure the RTC ticking is disabled before we configure dividers */
>> +	rtc_disable(priv);
> 
> At this point, you lost the RTC accuracy, this must not be done on probe

The 'rtc_init' procedure is called during the probe and suspend
callbacks. The first one is expected, while the latter may be a bit of a
surprise, but it follows what the hardware expects to see when switching
from one clock source to another.
In this case (suspend), we are forced to switch from a clock source that
can be used only during running (FIRC) to SIRC, which is the only option 
for the period when Linux is suspended.

>> +
>> +	clksel = RTCC_CLKSEL(priv->clk_source);
>> +	rtcc |= clksel;
>> +
>> +	/* Precompute the base frequency of the clock */
>> +	switch (clksel) {
>> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_SIRC):
>> +		sclk = priv->sirc;
>> +		break;
>> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_FIRC):
>> +		sclk = priv->firc;
>> +		break;
>> +	default:
>> +		dev_err(dev, "Invalid clksel value: %u\n", clksel);
>> +		ret = -EINVAL;
>> +		goto disable_firc_clk;
>> +	}
>> +
>> +	priv->rtc_hz = clk_get_rate(sclk);
>> +	if (!priv->rtc_hz) {
>> +		dev_err(dev, "Invalid RTC frequency\n");
>> +		ret = -EINVAL;
>> +		goto disable_firc_clk;
>> +	}
>> +
>> +	/* Adjust frequency if dividers are enabled */
>> +	if (priv->div512) {
>> +		rtcc |= RTCC_DIV512EN;
>> +		priv->rtc_hz /= 512;
>> +	}
>> +
>> +	if (priv->div32) {
>> +		rtcc |= RTCC_DIV32EN;
>> +		priv->rtc_hz /= 32;
>> +	}
>> +
>> +	rtcc |= RTCC_RTCIE | RTCC_ROVREN;
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +
>> +	return 0;
>> +
>> +disable_firc_clk:
>> +	clk_disable_unprepare(priv->firc);
>> +disable_sirc_clk:
>> +	clk_disable_unprepare(priv->sirc);
>> +disable_ipg_clk:
>> +	clk_disable_unprepare(priv->ipg);
>> +	return ret;
>> +}
>> +
>> +static int priv_dts_init(struct rtc_priv *priv)
>> +{
>> +	struct device *dev = priv->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	u32 clksel = S32G_RTC_SOURCE_SIRC;
>> +	/* div512 and div32 */
>> +	u32 div[2] = { 0 };
>> +	int ret;
>> +
>> +	priv->sirc = devm_clk_get(dev, "sirc");
>> +	if (IS_ERR(priv->sirc)) {
>> +		dev_err(dev, "Failed to get 'sirc' clock\n");
>> +		return PTR_ERR(priv->sirc);
>> +	}
>> +
>> +	priv->firc = devm_clk_get(dev, "firc");
>> +	if (IS_ERR(priv->firc)) {
>> +		dev_err(dev, "Failed to get 'firc' clock\n");
>> +		return PTR_ERR(priv->firc);
>> +	}
>> +
>> +	priv->ipg = devm_clk_get(dev, "ipg");
>> +	if (IS_ERR(priv->ipg)) {
>> +		dev_err(dev, "Failed to get 'ipg' clock\n");
>> +		return PTR_ERR(priv->ipg);
>> +	}
>> +
>> +	priv->dt_irq_id = platform_get_irq(pdev, 0);
>> +	if (priv->dt_irq_id < 0) {
>> +		dev_err(dev, "Error reading interrupt # from dts\n");
>> +		return priv->dt_irq_id;
>> +	}
>> +
>> +	ret = device_property_read_u32_array(dev, "nxp,dividers", div,
>> +					     ARRAY_SIZE(div));
>> +	if (ret) {
>> +		dev_err(dev, "Error reading dividers configuration, err: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = device_property_read_u32(dev, "nxp,clksel", &clksel);
>> +	if (ret) {
>> +		dev_err(dev, "Error reading clksel configuration, err: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	priv->div512 = !!div[0];
>> +	priv->div32 = !!div[1];
>> +
>> +	switch (clksel) {
>> +	case S32G_RTC_SOURCE_SIRC:
>> +	case S32G_RTC_SOURCE_FIRC:
>> +		priv->clk_source = clksel;
>> +		break;
>> +	default:
>> +		dev_err(dev, "Unsupported clksel: %d\n", clksel);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rtc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rtc_priv *priv;
>> +	int ret = 0;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(struct rtc_priv),
>> +			    GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(priv->rtc_base)) {
>> +		dev_err(dev, "Failed to map registers\n");
>> +		return PTR_ERR(priv->rtc_base);
>> +	}
>> +
>> +	device_init_wakeup(dev, true);
>> +	priv->dev = dev;
>> +
>> +	ret = priv_dts_init(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = rtc_init(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +	rtc_enable(priv);
>> +
>> +	priv->rdev = devm_rtc_device_register(dev, "s32g_rtc",
>> +					      &rtc_ops, THIS_MODULE);
> 
> This is deprecated, please use devm_rtc_allocate_device and devm_rtc_register_device
>

Thanks, I will update in V2.

>> +	if (IS_ERR_OR_NULL(priv->rdev)) {
>> +		dev_err(dev, "Error registering RTC device, err: %ld\n",
>> +			PTR_ERR(priv->rdev));
>> +		ret = PTR_ERR(priv->rdev);
>> +		goto disable_rtc;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, priv->dt_irq_id,
>> +			       rtc_handler, 0, dev_name(dev), pdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
>> +			priv->dt_irq_id, ret);
>> +		goto disable_rtc;
> 
> You must not fail after registering the RTC, else your driver will be
> opened to a race condition;
> 
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_rtc:
>> +	rtc_disable(priv);
>> +	return ret;
>> +}
>> +
>> +static void rtc_remove(struct platform_device *pdev)
>> +{
>> +	struct rtc_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	rtc_disable(priv);
> 
> Disabling the RTC when shutting down renders the RTC useless.
>

The RTC hardware module state is not preserved during system poweroff.
Hence why I've disabled it on remove callback.

>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static void enable_api_irq(struct device *dev, unsigned int enabled)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
>> +	u32 rtcc;
>> +
>> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
>> +	if (enabled)
>> +		rtcc |= api_irq;
>> +	else
>> +		rtcc &= ~api_irq;
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +}
>> +
>> +static int adjust_dividers(u32 sec, struct rtc_priv *priv)
>> +{
>> +	u64 rtcval_max = U32_MAX;
>> +	u64 rtcval;
>> +
>> +	priv->div32 = 0;
>> +	priv->div512 = 0;
>> +
>> +	rtcval = sec * priv->rtc_hz;
>> +	if (rtcval < rtcval_max)
>> +		return 0;
>> +
>> +	if (rtcval / 32 < rtcval_max) {
>> +		priv->div32 = 1;
>> +		return 0;
>> +	}
>> +
>> +	if (rtcval / 512 < rtcval_max) {
>> +		priv->div512 = 1;
>> +		return 0;
>> +	}
>> +
>> +	if (rtcval / (512 * 32) < rtcval_max) {
>> +		priv->div32 = 1;
>> +		priv->div512 = 1;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int rtc_suspend(struct device *dev)
>> +{
>> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
>> +	struct rtc_priv priv;
>> +	long long base_sec;
>> +	int ret = 0;
>> +	u32 rtcval;
>> +	u32 sec;
>> +
>> +	if (!device_may_wakeup(dev))
>> +		return 0;
>> +
>> +	/* Save last known timestamp before we switch clocks and reinit RTC */
>> +	ret = s32g_rtc_read_time(dev, &priv.base.tm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (init_priv->clk_source == S32G_RTC_SOURCE_SIRC)
>> +		return 0;
>> +
>> +	/*
>> +	 * Use a local copy of the RTC control block to
>> +	 * avoid restoring it on resume path.
>> +	 */
>> +	memcpy(&priv, init_priv, sizeof(priv));
>> +
>> +	/* Switch to SIRC */
>> +	priv.clk_source = S32G_RTC_SOURCE_SIRC;
>> +
>> +	ret = get_time_left(dev, init_priv, &sec);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Adjust for the number of seconds we'll be asleep */
>> +	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
>> +	base_sec += sec;
>> +	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
>> +
>> +	rtc_disable(&priv);
>> +
>> +	ret = adjust_dividers(sec, &priv);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to adjust RTC dividers to match a %u seconds delay\n", sec);
>> +		return ret;
>> +	}
>> +
>> +	ret = rtc_init(&priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sec_to_rtcval(&priv, sec, &rtcval);
>> +	if (ret) {
>> +		dev_warn(dev, "Alarm is too far in the future\n");
>> +		return ret;
>> +	}
> 
> All of this seems super fishy.
>

I agree it is... above rollover support enables RTC alarm during Sleep
for a maximum timespan of ~3 months.
In this case, the rollover does not matter as it's not involved in sleep 
period settings. The only way we could avoid all of this is to always 
keep the counter on SIRC, starting with driver's probing.
This would avoid the transitions, but the precision would not be as 
accurate as running at 51MHz.

>> +
>> +	s32g_rtc_alarm_irq_enable(dev, 0);
>> +	enable_api_irq(dev, 1);
>> +	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
>> +	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
>> +
>> +	rtc_enable(&priv);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rtc_resume(struct device *dev)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (!device_may_wakeup(dev))
>> +		return 0;
>> +
>> +	/* Disable wake-up interrupts */
>> +	enable_api_irq(dev, 0);
>> +
>> +	/* Reinitialize the driver using the initial settings */
>> +	ret = rtc_init(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	rtc_enable(priv);
>> +
>> +	/*
>> +	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
>> +	 * reapply the saved time settings
>> +	 */
>> +	return s32g_rtc_set_time(dev, &priv->base.tm);
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +static const struct of_device_id rtc_dt_ids[] = {
>> +	{.compatible = "nxp,s32g-rtc" },
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
>> +			 rtc_suspend, rtc_resume);
>> +
>> +static struct platform_driver rtc_driver = {
>> +	.driver		= {
>> +		.name			= "s32g-rtc",
>> +		.pm				= &rtc_pm_ops,
>> +		.of_match_table = of_match_ptr(rtc_dt_ids),
>> +	},
>> +	.probe		= rtc_probe,
>> +	.remove_new	= rtc_remove,
>> +};
>> +module_platform_driver(rtc_driver);
>> +
>> +MODULE_AUTHOR("NXP");
>> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.45.2
>>
>>
> 

[PATCH 3/4] arm64: defconfig: add S32G RTC module support

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

This patch enables CONFIG_RTC_DRV_S32G as module by default.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 7d32fca64996..5de9ae08294c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1198,6 +1198,7 @@ CONFIG_RTC_DRV_DA9063=m
 CONFIG_RTC_DRV_EFI=y
 CONFIG_RTC_DRV_CROS_EC=y
 CONFIG_RTC_DRV_FSL_FTM_ALARM=m
+CONFIG_RTC_DRV_S32G=m
 CONFIG_RTC_DRV_S3C=y
 CONFIG_RTC_DRV_PL031=y
 CONFIG_RTC_DRV_SUN6I=y
-- 
2.45.2

Re: [PATCH 3/4] arm64: defconfig: add S32G RTC module support

[Krzysztof Kozlowski]

On 11/09/2024 09:00, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> This patch enables CONFIG_RTC_DRV_S32G as module by default.

We see this from the diff. We do not see why we would like this.

Best regards,
Krzysztof

Re: [PATCH 3/4] arm64: defconfig: add S32G RTC module support

[Ciprian Marian Costea]

On 9/17/2024 8:36 PM, Krzysztof Kozlowski wrote:
> On 11/09/2024 09:00, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> This patch enables CONFIG_RTC_DRV_S32G as module by default.
> 
> We see this from the diff. We do not see why we would like this.
> 
> Best regards,
> Krzysztof
> 

Hello Krzysztof,

Thanks for your feedback.

I can see how not providing any information in the commit message 
regarding why this config is important for S32G2/S32G3 SoCs is not ideal.

Considering this, if I would add such information, would it be 
acceptable to enable this config as module ? Or should I drop this 
commit instead ?

My decision to enable it as module by default was also influenced by 
other similar RTC related configs, which are present in 'defconfig'.

Best Regards,
Ciprian

Re: [PATCH 3/4] arm64: defconfig: add S32G RTC module support

[Krzysztof Kozlowski]

On 18/09/2024 10:02, Ciprian Marian Costea wrote:
> On 9/17/2024 8:36 PM, Krzysztof Kozlowski wrote:
>> On 11/09/2024 09:00, Ciprian Costea wrote:
>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>
>>> This patch enables CONFIG_RTC_DRV_S32G as module by default.
>>
>> We see this from the diff. We do not see why we would like this.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Hello Krzysztof,
> 
> Thanks for your feedback.
> 
> I can see how not providing any information in the commit message 
> regarding why this config is important for S32G2/S32G3 SoCs is not ideal.
> 
> Considering this, if I would add such information, would it be 
> acceptable to enable this config as module ? Or should I drop this 
> commit instead ?
> 
> My decision to enable it as module by default was also influenced by 
> other similar RTC related configs, which are present in 'defconfig'.
> 

I don't object the patch contents itself. I want some sort of
explanation in the commit msg. Each commit msg is supposed to answer to
"why?" (unless it is obvious) and the answer is for example that
platform foo bar uses it. See some examples in git history.

Best regards,
Krzysztof

[PATCH 4/4] MAINTAINERS: add MAINTAINER for S32G2/S32G3 RTC driver

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Now that a RTC driver was added for S32G2/S32G3 SoC, update
the mainainters list for it.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..a6d91101ec43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2686,11 +2686,13 @@ ARM/NXP S32G ARCHITECTURE
 R:	Chester Lin <chester62515@gmail.com>
 R:	Matthias Brugger <mbrugger@suse.com>
 R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
+R:	Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
 L:	NXP S32 Linux Team <s32@nxp.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	arch/arm64/boot/dts/freescale/s32g*.dts*
 F:	drivers/pinctrl/nxp/
+F:	drivers/rtc/rtc-s32g.c
 
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
 M:	Alexander Clouter <alex@digriz.org.uk>
-- 
2.45.2

Re: [PATCH 4/4] MAINTAINERS: add MAINTAINER for S32G2/S32G3 RTC driver

[Krzysztof Kozlowski]

On 11/09/2024 09:00, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Now that a RTC driver was added for S32G2/S32G3 SoC, update
> the mainainters list for it.

Why? You don't do that alone. You add yourself for entire platform!

> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f328373463b0..a6d91101ec43 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2686,11 +2686,13 @@ ARM/NXP S32G ARCHITECTURE
>  R:	Chester Lin <chester62515@gmail.com>
>  R:	Matthias Brugger <mbrugger@suse.com>
>  R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>

If you are touching someone's maintainer entry, at least you could do is
to CC them.

And how many reviewers do you want to have in that platform? Are all
entries real or some are stale?

Best regards,
Krzysztof

Re: [PATCH 4/4] MAINTAINERS: add MAINTAINER for S32G2/S32G3 RTC driver

[Ciprian Marian Costea]

On 9/17/2024 8:37 PM, Krzysztof Kozlowski wrote:
> On 11/09/2024 09:00, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Now that a RTC driver was added for S32G2/S32G3 SoC, update
>> the mainainters list for it.
> 
> Why? You don't do that alone. You add yourself for entire platform!
> 
>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> ---
>>   MAINTAINERS | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f328373463b0..a6d91101ec43 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2686,11 +2686,13 @@ ARM/NXP S32G ARCHITECTURE
>>   R:	Chester Lin <chester62515@gmail.com>
>>   R:	Matthias Brugger <mbrugger@suse.com>
>>   R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
> 
> If you are touching someone's maintainer entry, at least you could do is
> to CC them.
> 
> And how many reviewers do you want to have in that platform? Are all
> entries real or some are stale?
> 
> Best regards,
> Krzysztof
> 

Hello Krzysztof,

My intention was to add myself as a reviewer for the S32G Architecture 
and not as a maintainer.

I plan to send more patches targeting this architecture and I would like 
to review any other changes to them in the future.

On the other hand I understand your point, already having a list of 
reviewers. If its unacceptable, I can only add myself as a maintainer 
for the S32G RTC driver, in V2 of this patchset.

Best Regards,
Ciprian

Re: [PATCH 4/4] MAINTAINERS: add MAINTAINER for S32G2/S32G3 RTC driver

[Krzysztof Kozlowski]

On 18/09/2024 10:13, Ciprian Marian Costea wrote:
> On 9/17/2024 8:37 PM, Krzysztof Kozlowski wrote:
>> On 11/09/2024 09:00, Ciprian Costea wrote:
>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>
>>> Now that a RTC driver was added for S32G2/S32G3 SoC, update
>>> the mainainters list for it.
>>
>> Why? You don't do that alone. You add yourself for entire platform!
>>
>>>
>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>> ---
>>>   MAINTAINERS | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index f328373463b0..a6d91101ec43 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2686,11 +2686,13 @@ ARM/NXP S32G ARCHITECTURE
>>>   R:	Chester Lin <chester62515@gmail.com>
>>>   R:	Matthias Brugger <mbrugger@suse.com>
>>>   R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
>>
>> If you are touching someone's maintainer entry, at least you could do is
>> to CC them.
>>
>> And how many reviewers do you want to have in that platform? Are all
>> entries real or some are stale?
>>
>> Best regards,
>> Krzysztof
>>
> 
> Hello Krzysztof,
> 
> My intention was to add myself as a reviewer for the S32G Architecture 
> and not as a maintainer.
> 
> I plan to send more patches targeting this architecture and I would like 
> to review any other changes to them in the future.
> 
> On the other hand I understand your point, already having a list of 
> reviewers. If its unacceptable, I can only add myself as a maintainer 
> for the S32G RTC driver, in V2 of this patchset.
> 

Folks:
1. It is okay to have more maintainers or reviewers
2. Document the express, be explicit in commit msg, provide some reasoning
3. Be sure BEFORE you talked with existing maintainers and they are
onboard, unless they are not responsive
4. Explain why the list of three has to grow to list of 5 (or you did
not align it with your colleagues, either). Yes, I see your other emails
and it is very confusing that two of such events happen independently.
It is not community task to coordinate your team activities...

Best regards,
Krzysztof