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

Commit Message

Ciprian Marian Costea Oct. 31, 2024, 8:35 a.m. UTC

From: Ciprian Marian Costea <[e-mail]>

This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.

Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
Co-developed-by: Ghennadi Procopciuc <[e-mail]>
Signed-off-by: Ghennadi Procopciuc <[e-mail]>
Signed-off-by: Ciprian Marian Costea <[e-mail]>
---
 .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml

Comments

Frank Li Oct. 31, 2024, 4:39 p.m. UTC | #1

On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
> From: Ciprian Marian Costea <[e-mail]>
>
> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>
> Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
> Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
> Co-developed-by: Ghennadi Procopciuc <[e-mail]>
> Signed-off-by: Ghennadi Procopciuc <[e-mail]>
> Signed-off-by: Ciprian Marian Costea <[e-mail]>
> ---

next time you can cc [e-mail]

>  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
>  1 file changed, 99 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..3694af883dc7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> @@ -0,0 +1,99 @@
> +# 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 <[e-mail]>
> +  - Ciprian Marian Costea <[e-mail]>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - nxp,s32g2-rtc
> +      - items:
> +          - const: nxp,s32g3-rtc
> +          - const: nxp,s32g2-rtc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 1

Does your RTC is clock provider? why need #clock-cells

> +
> +  clocks:
> +    items:
> +      - description: ipg clock drives the access to the
> +          RTC iomapped registers
> +
> +  clock-names:
> +    items:
> +      - const: ipg
> +
> +  assigned-clocks:
> +    minItems: 1
> +    items:
> +      - description: Runtime clock source. It must be a clock
> +            source for the RTC module. It will be disabled by hardware
> +            during Standby/Suspend.
> +      - description: Standby/Suspend clock source. It is optional
> +            and can be used in case the RTC will continue ticking during
> +            platform/system suspend. RTC hardware module contains a
> +            hardware mux for clock source selection.
> +
> +  assigned-clock-parents:
> +    description: List of phandles to each parent clock.
> +
> +  assigned-clock-rates:
> +    description: List of frequencies for RTC clock sources.
> +            RTC module contains 2 hardware divisors which can be
> +            enabled or not. Hence, available frequencies are the following
> +            parent_freq, parent_freq / 512, parent_freq / 32 or
> +            parent_freq / (512 * 32)

Needn't assigned-*

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    rtc0: rtc@40060000 {

needn't label

> +        compatible = "nxp,s32g3-rtc",
> +                   "nxp,s32g2-rtc";
> +        reg = <0x40060000 0x1000>;
> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> +        #clock-cells = <1>;
> +        clocks = <&clks 54>;
> +        clock-names = "ipg";
> +        /*
> +         * Configuration of default parent clocks.
> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> +         * 4-7 IDs are Suspend/Standby clock sources.
> +         */
> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> +        /*
> +         * Clock frequency can be divided by value
> +         * 512 or 32 (or both) via hardware divisors.
> +         * Below configuration:
> +         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
> +         * Suspend/Standby clock source: SIRC (32 KHz)
> +         */
> +        assigned-clock-rates = <99609>, <32000>;
> +    };
> --
> 2.45.2
>

Ciprian Marian Costea Nov. 1, 2024, 9:29 a.m. UTC | #2

On 10/31/2024 6:39 PM, Frank Li wrote:

Hello Frank,

Thank you for your review!

> On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <[e-mail]>
>>
>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>
>> Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
>> Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
>> Co-developed-by: Ghennadi Procopciuc <[e-mail]>
>> Signed-off-by: Ghennadi Procopciuc <[e-mail]>
>> Signed-off-by: Ciprian Marian Costea <[e-mail]>
>> ---
> 
> next time you can cc [e-mail]

Thanks for your suggestion, I will start adding this list.

> 
>>   .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
>>   1 file changed, 99 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..3694af883dc7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>> @@ -0,0 +1,99 @@
>> +# 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 <[e-mail]>
>> +  - Ciprian Marian Costea <[e-mail]>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - enum:
>> +          - nxp,s32g2-rtc
>> +      - items:
>> +          - const: nxp,s32g3-rtc
>> +          - const: nxp,s32g2-rtc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  "#clock-cells":
>> +    const: 1
> 
> Does your RTC is clock provider? why need #clock-cells
> 

RTC clocking on S32G2/S32G3 has a clock mux for selecting between up to 
4 different clock sources (parents).
Now, as suggested in previous reviews, I've switched to using the CCF in 
order to implement this hardware particularity. In the end I've resorted 
to implementing 'assigned-*' approach which while not mandatory as per 
proposed bindings I find it quite scalable in selecting clock sources 
for the RTC module compared to the first iteration (V1) of this patchset.

>> +
>> +  clocks:
>> +    items:
>> +      - description: ipg clock drives the access to the
>> +          RTC iomapped registers
>> +
>> +  clock-names:
>> +    items:
>> +      - const: ipg
>> +
>> +  assigned-clocks:
>> +    minItems: 1
>> +    items:
>> +      - description: Runtime clock source. It must be a clock
>> +            source for the RTC module. It will be disabled by hardware
>> +            during Standby/Suspend.
>> +      - description: Standby/Suspend clock source. It is optional
>> +            and can be used in case the RTC will continue ticking during
>> +            platform/system suspend. RTC hardware module contains a
>> +            hardware mux for clock source selection.
>> +
>> +  assigned-clock-parents:
>> +    description: List of phandles to each parent clock.
>> +
>> +  assigned-clock-rates:
>> +    description: List of frequencies for RTC clock sources.
>> +            RTC module contains 2 hardware divisors which can be
>> +            enabled or not. Hence, available frequencies are the following
>> +            parent_freq, parent_freq / 512, parent_freq / 32 or
>> +            parent_freq / (512 * 32)
> 
> Needn't assigned-*
> 

'assigned-*' entries are not required, but based on my previous answer I 
would prefer to document them in order to instruct further S32 SoC based 
boards which may use this driver.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - "#clock-cells"
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    rtc0: rtc@40060000 {
> 
> needn't label

Thanks. I will remove the label in V4.

> 
>> +        compatible = "nxp,s32g3-rtc",
>> +                   "nxp,s32g2-rtc";
>> +        reg = <0x40060000 0x1000>;
>> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
>> +        #clock-cells = <1>;
>> +        clocks = <&clks 54>;
>> +        clock-names = "ipg";
>> +        /*
>> +         * Configuration of default parent clocks.
>> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
>> +         * 4-7 IDs are Suspend/Standby clock sources.
>> +         */
>> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
>> +        assigned-clock-parents = <&clks 56>, <&clks 55>;
>> +        /*
>> +         * Clock frequency can be divided by value
>> +         * 512 or 32 (or both) via hardware divisors.
>> +         * Below configuration:
>> +         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
>> +         * Suspend/Standby clock source: SIRC (32 KHz)
>> +         */
>> +        assigned-clock-rates = <99609>, <32000>;
>> +    };
>> --
>> 2.45.2
>>

Frank Li Nov. 1, 2024, 5:57 p.m. UTC | #3

On Fri, Nov 01, 2024 at 11:29:24AM +0200, Ciprian Marian Costea wrote:
> On 10/31/2024 6:39 PM, Frank Li wrote:
>
> Hello Frank,
>
> Thank you for your review!
>
> > On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
> > > From: Ciprian Marian Costea <[e-mail]>
> > >
> > > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > >
> > > Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
> > > Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
> > > Co-developed-by: Ghennadi Procopciuc <[e-mail]>
> > > Signed-off-by: Ghennadi Procopciuc <[e-mail]>
> > > Signed-off-by: Ciprian Marian Costea <[e-mail]>
> > > ---
> >
> > next time you can cc [e-mail]
>
> Thanks for your suggestion, I will start adding this list.
>
> >
> > >   .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
> > >   1 file changed, 99 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..3694af883dc7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > @@ -0,0 +1,99 @@
> > > +# 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 <[e-mail]>
> > > +  - Ciprian Marian Costea <[e-mail]>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - enum:
> > > +          - nxp,s32g2-rtc
> > > +      - items:
> > > +          - const: nxp,s32g3-rtc
> > > +          - const: nxp,s32g2-rtc
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  "#clock-cells":
> > > +    const: 1
> >
> > Does your RTC is clock provider? why need #clock-cells
> >
>
> RTC clocking on S32G2/S32G3 has a clock mux for selecting between up to 4
> different clock sources (parents).
> Now, as suggested in previous reviews, I've switched to using the CCF in
> order to implement this hardware particularity. In the end I've resorted to
> implementing 'assigned-*' approach which while not mandatory as per proposed
> bindings I find it quite scalable in selecting clock sources for the RTC
> module compared to the first iteration (V1) of this patchset.

Any link of previous review?

Frank

>
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: ipg clock drives the access to the
> > > +          RTC iomapped registers
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: ipg
> > > +
> > > +  assigned-clocks:
> > > +    minItems: 1
> > > +    items:
> > > +      - description: Runtime clock source. It must be a clock
> > > +            source for the RTC module. It will be disabled by hardware
> > > +            during Standby/Suspend.
> > > +      - description: Standby/Suspend clock source. It is optional
> > > +            and can be used in case the RTC will continue ticking during
> > > +            platform/system suspend. RTC hardware module contains a
> > > +            hardware mux for clock source selection.
> > > +
> > > +  assigned-clock-parents:
> > > +    description: List of phandles to each parent clock.
> > > +
> > > +  assigned-clock-rates:
> > > +    description: List of frequencies for RTC clock sources.
> > > +            RTC module contains 2 hardware divisors which can be
> > > +            enabled or not. Hence, available frequencies are the following
> > > +            parent_freq, parent_freq / 512, parent_freq / 32 or
> > > +            parent_freq / (512 * 32)
> >
> > Needn't assigned-*
> >
>
> 'assigned-*' entries are not required, but based on my previous answer I
> would prefer to document them in order to instruct further S32 SoC based
> boards which may use this driver.
>
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - "#clock-cells"
> > > +  - clocks
> > > +  - clock-names
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    rtc0: rtc@40060000 {
> >
> > needn't label
>
> Thanks. I will remove the label in V4.
>
> >
> > > +        compatible = "nxp,s32g3-rtc",
> > > +                   "nxp,s32g2-rtc";
> > > +        reg = <0x40060000 0x1000>;
> > > +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > > +        #clock-cells = <1>;
> > > +        clocks = <&clks 54>;
> > > +        clock-names = "ipg";
> > > +        /*
> > > +         * Configuration of default parent clocks.
> > > +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> > > +         * 4-7 IDs are Suspend/Standby clock sources.
> > > +         */
> > > +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> > > +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> > > +        /*
> > > +         * Clock frequency can be divided by value
> > > +         * 512 or 32 (or both) via hardware divisors.
> > > +         * Below configuration:
> > > +         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
> > > +         * Suspend/Standby clock source: SIRC (32 KHz)
> > > +         */
> > > +        assigned-clock-rates = <99609>, <32000>;
> > > +    };
> > > --
> > > 2.45.2
> > >
>

Ciprian Marian Costea Nov. 4, 2024, 8:22 a.m. UTC | #4

On 11/1/2024 7:57 PM, Frank Li wrote:
> On Fri, Nov 01, 2024 at 11:29:24AM +0200, Ciprian Marian Costea wrote:
>> On 10/31/2024 6:39 PM, Frank Li wrote:
>>
>> Hello Frank,
>>
>> Thank you for your review!
>>
>>> On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
>>>> From: Ciprian Marian Costea <[e-mail]>
>>>>
>>>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>>>
>>>> Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
>>>> Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
>>>> Co-developed-by: Ghennadi Procopciuc <[e-mail]>
>>>> Signed-off-by: Ghennadi Procopciuc <[e-mail]>
>>>> Signed-off-by: Ciprian Marian Costea <[e-mail]>
>>>> ---
>>>
>>> next time you can cc [e-mail]
>>
>> Thanks for your suggestion, I will start adding this list.
>>
>>>
>>>>    .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
>>>>    1 file changed, 99 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..3694af883dc7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>> @@ -0,0 +1,99 @@
>>>> +# 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 <[e-mail]>
>>>> +  - Ciprian Marian Costea <[e-mail]>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - enum:
>>>> +          - nxp,s32g2-rtc
>>>> +      - items:
>>>> +          - const: nxp,s32g3-rtc
>>>> +          - const: nxp,s32g2-rtc
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#clock-cells":
>>>> +    const: 1
>>>
>>> Does your RTC is clock provider? why need #clock-cells
>>>
>>
>> RTC clocking on S32G2/S32G3 has a clock mux for selecting between up to 4
>> different clock sources (parents).
>> Now, as suggested in previous reviews, I've switched to using the CCF in
>> order to implement this hardware particularity. In the end I've resorted to
>> implementing 'assigned-*' approach which while not mandatory as per proposed
>> bindings I find it quite scalable in selecting clock sources for the RTC
>> module compared to the first iteration (V1) of this patchset.
> 
> Any link of previous review?
> 
> Frank
>

Please consider the following related reviews:

[1] https://lore.kernel.org/all/[e-mail]/
[2] 
https://lore.kernel.org/all/[e-mail]/
[3] https://lore.kernel.org/all/[e-mail]/

>>
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: ipg clock drives the access to the
>>>> +          RTC iomapped registers
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: ipg
>>>> +
>>>> +  assigned-clocks:
>>>> +    minItems: 1
>>>> +    items:
>>>> +      - description: Runtime clock source. It must be a clock
>>>> +            source for the RTC module. It will be disabled by hardware
>>>> +            during Standby/Suspend.
>>>> +      - description: Standby/Suspend clock source. It is optional
>>>> +            and can be used in case the RTC will continue ticking during
>>>> +            platform/system suspend. RTC hardware module contains a
>>>> +            hardware mux for clock source selection.
>>>> +
>>>> +  assigned-clock-parents:
>>>> +    description: List of phandles to each parent clock.
>>>> +
>>>> +  assigned-clock-rates:
>>>> +    description: List of frequencies for RTC clock sources.
>>>> +            RTC module contains 2 hardware divisors which can be
>>>> +            enabled or not. Hence, available frequencies are the following
>>>> +            parent_freq, parent_freq / 512, parent_freq / 32 or
>>>> +            parent_freq / (512 * 32)
>>>
>>> Needn't assigned-*
>>>
>>
>> 'assigned-*' entries are not required, but based on my previous answer I
>> would prefer to document them in order to instruct further S32 SoC based
>> boards which may use this driver.
>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - "#clock-cells"
>>>> +  - clocks
>>>> +  - clock-names
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +
>>>> +    rtc0: rtc@40060000 {
>>>
>>> needn't label
>>
>> Thanks. I will remove the label in V4.
>>
>>>
>>>> +        compatible = "nxp,s32g3-rtc",
>>>> +                   "nxp,s32g2-rtc";
>>>> +        reg = <0x40060000 0x1000>;
>>>> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
>>>> +        #clock-cells = <1>;
>>>> +        clocks = <&clks 54>;
>>>> +        clock-names = "ipg";
>>>> +        /*
>>>> +         * Configuration of default parent clocks.
>>>> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
>>>> +         * 4-7 IDs are Suspend/Standby clock sources.
>>>> +         */
>>>> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
>>>> +        assigned-clock-parents = <&clks 56>, <&clks 55>;
>>>> +        /*
>>>> +         * Clock frequency can be divided by value
>>>> +         * 512 or 32 (or both) via hardware divisors.
>>>> +         * Below configuration:
>>>> +         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
>>>> +         * Suspend/Standby clock source: SIRC (32 KHz)
>>>> +         */
>>>> +        assigned-clock-rates = <99609>, <32000>;
>>>> +    };
>>>> --
>>>> 2.45.2
>>>>
>>

Rob Herring (Arm) Nov. 4, 2024, 3:31 p.m. UTC | #5

On Mon, Nov 04, 2024 at 10:22:05AM +0200, Ciprian Marian Costea wrote:
> On 11/1/2024 7:57 PM, Frank Li wrote:
> > On Fri, Nov 01, 2024 at 11:29:24AM +0200, Ciprian Marian Costea wrote:
> > > On 10/31/2024 6:39 PM, Frank Li wrote:
> > > 
> > > Hello Frank,
> > > 
> > > Thank you for your review!
> > > 
> > > > On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
> > > > > From: Ciprian Marian Costea <[e-mail]>
> > > > > 
> > > > > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > > > > 
> > > > > Co-developed-by: Bogdan-Gabriel Roman <[e-mail]>
> > > > > Signed-off-by: Bogdan-Gabriel Roman <[e-mail]>
> > > > > Co-developed-by: Ghennadi Procopciuc <[e-mail]>
> > > > > Signed-off-by: Ghennadi Procopciuc <[e-mail]>
> > > > > Signed-off-by: Ciprian Marian Costea <[e-mail]>
> > > > > ---
> > > > 
> > > > next time you can cc [e-mail]
> > > 
> > > Thanks for your suggestion, I will start adding this list.
> > > 
> > > > 
> > > > >    .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
> > > > >    1 file changed, 99 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..3694af883dc7
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > > > @@ -0,0 +1,99 @@
> > > > > +# 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 <[e-mail]>
> > > > > +  - Ciprian Marian Costea <[e-mail]>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    oneOf:
> > > > > +      - enum:
> > > > > +          - nxp,s32g2-rtc
> > > > > +      - items:
> > > > > +          - const: nxp,s32g3-rtc
> > > > > +          - const: nxp,s32g2-rtc
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#clock-cells":
> > > > > +    const: 1
> > > > 
> > > > Does your RTC is clock provider? why need #clock-cells
> > > > 
> > > 
> > > RTC clocking on S32G2/S32G3 has a clock mux for selecting between up to 4
> > > different clock sources (parents).
> > > Now, as suggested in previous reviews, I've switched to using the CCF in
> > > order to implement this hardware particularity. In the end I've resorted to
> > > implementing 'assigned-*' approach which while not mandatory as per proposed
> > > bindings I find it quite scalable in selecting clock sources for the RTC
> > > module compared to the first iteration (V1) of this patchset.
> > 
> > Any link of previous review?
> > 
> > Frank
> > 
> 
> Please consider the following related reviews:
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> [2]
> https://lore.kernel.org/all/[e-mail]/
> [3] https://lore.kernel.org/all/[e-mail]/

I've replied on the prior version.

Rob

Patch

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..3694af883dc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
@@ -0,0 +1,99 @@ 
+# 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 <[e-mail]>
+  - Ciprian Marian Costea <[e-mail]>
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nxp,s32g2-rtc
+      - items:
+          - const: nxp,s32g3-rtc
+          - const: nxp,s32g2-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    items:
+      - description: ipg clock drives the access to the
+          RTC iomapped registers
+
+  clock-names:
+    items:
+      - const: ipg
+
+  assigned-clocks:
+    minItems: 1
+    items:
+      - description: Runtime clock source. It must be a clock
+            source for the RTC module. It will be disabled by hardware
+            during Standby/Suspend.
+      - description: Standby/Suspend clock source. It is optional
+            and can be used in case the RTC will continue ticking during
+            platform/system suspend. RTC hardware module contains a
+            hardware mux for clock source selection.
+
+  assigned-clock-parents:
+    description: List of phandles to each parent clock.
+
+  assigned-clock-rates:
+    description: List of frequencies for RTC clock sources.
+            RTC module contains 2 hardware divisors which can be
+            enabled or not. Hence, available frequencies are the following
+            parent_freq, parent_freq / 512, parent_freq / 32 or
+            parent_freq / (512 * 32)
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc0: rtc@40060000 {
+        compatible = "nxp,s32g3-rtc",
+                   "nxp,s32g2-rtc";
+        reg = <0x40060000 0x1000>;
+        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
+        #clock-cells = <1>;
+        clocks = <&clks 54>;
+        clock-names = "ipg";
+        /*
+         * Configuration of default parent clocks.
+         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
+         * 4-7 IDs are Suspend/Standby clock sources.
+         */
+        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
+        assigned-clock-parents = <&clks 56>, <&clks 55>;
+        /*
+         * Clock frequency can be divided by value
+         * 512 or 32 (or both) via hardware divisors.
+         * Below configuration:
+         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
+         * Suspend/Standby clock source: SIRC (32 KHz)
+         */
+        assigned-clock-rates = <99609>, <32000>;
+    };