| Message ID | 20240911070028.127659-2-ciprianmarian.costea@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add NXP RTC driver support for S32G2/S32G3 SoCs | expand |
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 >
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?
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 >>
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
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
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.
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
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.
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.
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 > > > >
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 >>>> >> >
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
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.
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.
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.
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.
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>; + };