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

[Ciprian Marian Costea]

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 <ciprianmarian.costea at oss.nxp.com>
>>
>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>
>> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman at nxp.com>
>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman at nxp.com>
>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc at nxp.com>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc at nxp.com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea at oss.nxp.com>
>> ---
> 
> next time you can cc imx at lists.linux.dev

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 <bogdan.hamciuc at nxp.com>
>> +  - Ciprian Marian Costea <ciprianmarian.costea at nxp.com>
>> +
>> +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 at 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
>>