| Message ID | 20241015105133.656360-2-ciprianmarian.costea@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add NXP RTC driver support for S32G2/S32G3 SoCs | expand |
On Tue, Oct 15, 2024 at 01:51:30PM +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. > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ > 1 file changed, 102 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..3a77d4dd8f3d > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml > @@ -0,0 +1,102 @@ > +# 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: > + 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. If the RTC h/w contains a mux, then your mux inputs should be listed in 'clocks', not here. > + > + 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) In general, assigned-clocks* do not need to be documented and should never be required. > + > +required: > + - compatible > + - reg > + - interrupts > + - "#clock-cells" > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-parents > + - assigned-clock-rates > + > +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>; That's weird... > + assigned-clock-parents = <&clks 56>, <&clks 55>; I'd expect these should be in 'clocks'. I don't think this node should be a clock provider unless it provides a clock to something outside the RTC. Looks like you are just using assigned-clocks to configure the clock mux in the RTC. That's way over complicated. Just define a vendor specific property with the mux settings. > + /* > + * 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 >
On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote: > On Tue, Oct 15, 2024 at 01:51:30PM +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. > > > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ > > 1 file changed, 102 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..3a77d4dd8f3d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml > > @@ -0,0 +1,102 @@ > > +# 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: > > + 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. > > If the RTC h/w contains a mux, then your mux inputs should be listed in > 'clocks', not here. > > > + > > + 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) > > In general, assigned-clocks* do not need to be documented and should > never be required. > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - "#clock-cells" > > + - clocks > > + - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > + - assigned-clock-rates > > + > > +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>; > > That's weird... > > > + assigned-clock-parents = <&clks 56>, <&clks 55>; > > I'd expect these should be in 'clocks'. I don't think this node should > be a clock provider unless it provides a clock to something outside the > RTC. > > Looks like you are just using assigned-clocks to configure the clock mux > in the RTC. That's way over complicated. Just define a vendor specific > property with the mux settings. I just read v1 and got told use the clock framework... I disagree completely. Tons of h/w blocks have the ability to select (internal to the block) from multiple clock sources. Making the block a clock provider to itself is completely pointless and an overkill, and we *never* do that. Any display controller or audio interface has mutiple clock sources as just 2 examples. However, I don't see why you need the divider config in DT. Can't you figure out what divider you need based on input frequency? The output frequency should be fixed, right? Rob
On 15/10/2024 16:27:17-0500, Rob Herring wrote: > On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote: > > On Tue, Oct 15, 2024 at 01:51:30PM +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. > > > > > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ > > > 1 file changed, 102 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..3a77d4dd8f3d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml > > > @@ -0,0 +1,102 @@ > > > +# 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: > > > + 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. > > > > If the RTC h/w contains a mux, then your mux inputs should be listed in > > 'clocks', not here. > > > > > + > > > + 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) > > > > In general, assigned-clocks* do not need to be documented and should > > never be required. > > > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - interrupts > > > + - "#clock-cells" > > > + - clocks > > > + - clock-names > > > + - assigned-clocks > > > + - assigned-clock-parents > > > + - assigned-clock-rates > > > + > > > +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>; > > > > That's weird... > > > > > + assigned-clock-parents = <&clks 56>, <&clks 55>; > > > > I'd expect these should be in 'clocks'. I don't think this node should > > be a clock provider unless it provides a clock to something outside the > > RTC. > > > > Looks like you are just using assigned-clocks to configure the clock mux > > in the RTC. That's way over complicated. Just define a vendor specific > > property with the mux settings. > > I just read v1 and got told use the clock framework... > > I disagree completely. Tons of h/w blocks have the ability to select > (internal to the block) from multiple clock sources. Making the block a > clock provider to itself is completely pointless and an overkill, and > we *never* do that. Any display controller or audio interface has > mutiple clock sources as just 2 examples. And in 6 months, we are going to learn that the rtc is used to clock the wifi chip or whatever and we are going to need to add everything in the CCF and we will have an unused property that we are going to have to support forever to avoid breaking the ABI. This already happened... > > However, I don't see why you need the divider config in DT. Can't you > figure out what divider you need based on input frequency? The output > frequency should be fixed, right? > > Rob
On 10/16/2024 7:08 PM, Alexandre Belloni wrote: Hello Rob and Alexandre, Thank you for your careful review! > On 15/10/2024 16:27:17-0500, Rob Herring wrote: >> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote: >>> On Tue, Oct 15, 2024 at 01:51:30PM +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. >>>> >>>> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> >>>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> >>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ >>>> 1 file changed, 102 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..3a77d4dd8f3d >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml >>>> @@ -0,0 +1,102 @@ >>>> +# 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: >>>> + 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. >>> >>> If the RTC h/w contains a mux, then your mux inputs should be listed in >>> 'clocks', not here. >>> >>>> + >>>> + 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) >>> >>> In general, assigned-clocks* do not need to be documented and should >>> never be required. >>> >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - interrupts >>>> + - "#clock-cells" >>>> + - clocks >>>> + - clock-names >>>> + - assigned-clocks >>>> + - assigned-clock-parents >>>> + - assigned-clock-rates >>>> + >>>> +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>; >>> >>> That's weird... >>> >>>> + assigned-clock-parents = <&clks 56>, <&clks 55>; >>> >>> I'd expect these should be in 'clocks'. I don't think this node should >>> be a clock provider unless it provides a clock to something outside the >>> RTC. >>> >>> Looks like you are just using assigned-clocks to configure the clock mux >>> in the RTC. That's way over complicated. Just define a vendor specific >>> property with the mux settings. >> >> I just read v1 and got told use the clock framework... >> >> I disagree completely. Tons of h/w blocks have the ability to select >> (internal to the block) from multiple clock sources. Making the block a >> clock provider to itself is completely pointless and an overkill, and >> we *never* do that. Any display controller or audio interface has >> mutiple clock sources as just 2 examples. > > And in 6 months, we are going to learn that the rtc is used to clock the > wifi chip or whatever and we are going to need to add everything in the > CCF and we will have an unused property that we are going to have to > support forever to avoid breaking the ABI. This already happened... > >> >> However, I don't see why you need the divider config in DT. Can't you >> figure out what divider you need based on input frequency? The output >> frequency should be fixed, right? >> >> Rob > Would the following approach be acceptable ? 1. Keep 'assigned-clocks' based implementation as optional (not required), in order to take leverage of its scalability for allowing different clock source and divisor settings. 2. Implement a default clock muxing setting in the driver for both Runtime and Suspend. Best Regards, Ciprian
On Wed, Oct 16, 2024 at 06:08:23PM +0200, Alexandre Belloni wrote: > On 15/10/2024 16:27:17-0500, Rob Herring wrote: > > On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote: > > > On Tue, Oct 15, 2024 at 01:51:30PM +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. > > > > > > > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> > > > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ > > > > 1 file changed, 102 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..3a77d4dd8f3d > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml > > > > @@ -0,0 +1,102 @@ > > > > +# 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: > > > > + 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. > > > > > > If the RTC h/w contains a mux, then your mux inputs should be listed in > > > 'clocks', not here. > > > > > > > + > > > > + 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) > > > > > > In general, assigned-clocks* do not need to be documented and should > > > never be required. > > > > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - interrupts > > > > + - "#clock-cells" > > > > + - clocks > > > > + - clock-names > > > > + - assigned-clocks > > > > + - assigned-clock-parents > > > > + - assigned-clock-rates > > > > + > > > > +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>; > > > > > > That's weird... > > > > > > > + assigned-clock-parents = <&clks 56>, <&clks 55>; > > > > > > I'd expect these should be in 'clocks'. I don't think this node should > > > be a clock provider unless it provides a clock to something outside the > > > RTC. > > > > > > Looks like you are just using assigned-clocks to configure the clock mux > > > in the RTC. That's way over complicated. Just define a vendor specific > > > property with the mux settings. > > > > I just read v1 and got told use the clock framework... > > > > I disagree completely. Tons of h/w blocks have the ability to select > > (internal to the block) from multiple clock sources. Making the block a > > clock provider to itself is completely pointless and an overkill, and > > we *never* do that. Any display controller or audio interface has > > mutiple clock sources as just 2 examples. > > And in 6 months, we are going to learn that the rtc is used to clock the > wifi chip or whatever and we are going to need to add everything in the > CCF and we will have an unused property that we are going to have to > support forever to avoid breaking the ABI. This already happened... For that to happen, the RTC needs to have a clock output. AFAICT, from the series it doesn't have any clock output. If it does have an output clock, then yes, I would agree with you. But I only know as much as what is put here about this h/w. Rob
On 11/4/2024 5:29 PM, Rob Herring wrote: > On Wed, Oct 16, 2024 at 06:08:23PM +0200, Alexandre Belloni wrote: >> On 15/10/2024 16:27:17-0500, Rob Herring wrote: >>> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote: >>>> On Tue, Oct 15, 2024 at 01:51:30PM +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. >>>>> >>>>> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> >>>>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com> >>>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@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 | 102 ++++++++++++++++++ >>>>> 1 file changed, 102 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..3a77d4dd8f3d >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml >>>>> @@ -0,0 +1,102 @@ >>>>> +# 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: >>>>> + 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. >>>> >>>> If the RTC h/w contains a mux, then your mux inputs should be listed in >>>> 'clocks', not here. >>>> >>>>> + >>>>> + 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) >>>> >>>> In general, assigned-clocks* do not need to be documented and should >>>> never be required. >>>> >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - interrupts >>>>> + - "#clock-cells" >>>>> + - clocks >>>>> + - clock-names >>>>> + - assigned-clocks >>>>> + - assigned-clock-parents >>>>> + - assigned-clock-rates >>>>> + >>>>> +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>; >>>> >>>> That's weird... >>>> >>>>> + assigned-clock-parents = <&clks 56>, <&clks 55>; >>>> >>>> I'd expect these should be in 'clocks'. I don't think this node should >>>> be a clock provider unless it provides a clock to something outside the >>>> RTC. >>>> >>>> Looks like you are just using assigned-clocks to configure the clock mux >>>> in the RTC. That's way over complicated. Just define a vendor specific >>>> property with the mux settings. >>> >>> I just read v1 and got told use the clock framework... >>> >>> I disagree completely. Tons of h/w blocks have the ability to select >>> (internal to the block) from multiple clock sources. Making the block a >>> clock provider to itself is completely pointless and an overkill, and >>> we *never* do that. Any display controller or audio interface has >>> mutiple clock sources as just 2 examples. >> >> And in 6 months, we are going to learn that the rtc is used to clock the >> wifi chip or whatever and we are going to need to add everything in the >> CCF and we will have an unused property that we are going to have to >> support forever to avoid breaking the ABI. This already happened... > > For that to happen, the RTC needs to have a clock output. AFAICT, from > the series it doesn't have any clock output. If it does have an output > clock, then yes, I would agree with you. But I only know as much as what > is put here about this h/w. > > Rob Hello Rob, Thank you for resuming discussion on this patchset. Indeed, the RTC does not have a clock output. In the meantime I've sent a V3 where 'assigned-*' entries are no longer required. Please let me know if V3 is headed into the right direction since I prefer not to throw away current 'assigned-*' CCF approach since IMHO its scalable with the clock source selection for runtime and suspend. Ciprian
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..3a77d4dd8f3d --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml @@ -0,0 +1,102 @@ +# 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: + 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 + - assigned-clocks + - assigned-clock-parents + - assigned-clock-rates + +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>; + };