| Message ID | 20240830130218.3377060-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add RTC support for the Renesas RZ/G3S SoC | expand |
Quoting Claudiu (2024-08-30 06:02:13) > diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > index 067a26a66c24..247fa80a4f53 100644 > --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > @@ -160,6 +160,18 @@ i2c3: i2c@10090c00 { > status = "disabled"; > }; > > + vbattb: vbattb@1005c000 { > + compatible = "renesas,r9a08g045-vbattb"; > + reg = <0 0x1005c000 0 0x1000>; > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; > + clock-names = "bclk", "rtx"; > + #clock-cells = <1>; > + power-domains = <&cpg>; > + resets = <&cpg R9A08G045_VBAT_BRESETN>; > + status = "disabled"; > + }; > + > cpg: clock-controller@11010000 { > compatible = "renesas,r9a08g045-cpg"; > reg = <0 0x11010000 0 0x10000>; > @@ -425,4 +437,11 @@ timer { > interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", > "hyp-virt"; > }; > + > + vbattb_xtal: vbattb-xtal { The node name should be something like clock-<frequency> but if the frequency is different per-board then I don't know what should happen here. Can you leave the vbattb_xtal phandle up above and then require the node to be defined in the board with the proper frequency after the dash? > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + /* This value must be overridden by the board. */ > + clock-frequency = <0>;
Hi, Stephen, On 03.09.2024 22:48, Stephen Boyd wrote: > Quoting Claudiu (2024-08-30 06:02:13) >> diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi >> index 067a26a66c24..247fa80a4f53 100644 >> --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi >> +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi >> @@ -160,6 +160,18 @@ i2c3: i2c@10090c00 { >> status = "disabled"; >> }; >> >> + vbattb: vbattb@1005c000 { >> + compatible = "renesas,r9a08g045-vbattb"; >> + reg = <0 0x1005c000 0 0x1000>; >> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; >> + clock-names = "bclk", "rtx"; >> + #clock-cells = <1>; >> + power-domains = <&cpg>; >> + resets = <&cpg R9A08G045_VBAT_BRESETN>; >> + status = "disabled"; >> + }; >> + >> cpg: clock-controller@11010000 { >> compatible = "renesas,r9a08g045-cpg"; >> reg = <0 0x11010000 0 0x10000>; >> @@ -425,4 +437,11 @@ timer { >> interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", >> "hyp-virt"; >> }; >> + >> + vbattb_xtal: vbattb-xtal { > > The node name should be something like clock-<frequency> but if the > frequency is different per-board then I don't know what should happen > here. The frequency should be always around 32768 Hz but not necessarily exactly 32768 Hz. It depends on what is installed on the board, indeed. RTC can do time error adjustments based on the variations around 32768 Hz. > Can you leave the vbattb_xtal phandle up above and then require > the node to be defined in the board with the proper frequency after the > dash? Is it OK for you something like this (applied on top of this series)? diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi index f31ec08a1e1d..60679211dc48 100644 --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi @@ -181,7 +181,8 @@ vbattb: clock-controller@1005c000 { compatible = "renesas,r9a08g045-vbattb"; reg = <0 0x1005c000 0 0x1000>; interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; - clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; + /* rtx clock must be overridden by the board. */ + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <0>; clock-names = "bclk", "rtx"; #clock-cells = <1>; power-domains = <&cpg>; @@ -454,11 +455,4 @@ timer { interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt"; }; - - vbattb_xtal: vbattb-xtal { - compatible = "fixed-clock"; - #clock-cells = <0>; - /* This value must be overridden by the board. */ - clock-frequency = <0>; - }; }; diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi index 95b79a03d3d5..46cce0d48ddc 100644 --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi @@ -47,6 +47,12 @@ chosen { stdout-path = "serial0:115200n8"; }; + vbattb_xtal: clock-32768 { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + }; + memory@48000000 { device_type = "memory"; /* First 128MB is reserved for secure area. */ @@ -351,14 +357,11 @@ &rtc { }; &vbattb { + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; renesas,vbattb-load-nanofarads = <12500>; status = "okay"; }; -&vbattb_xtal { - clock-frequency = <32768>; -}; - &wdt0 { timeout-sec = <60>; status = "okay"; Thank you for your review, Claudiu Beznea > >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + /* This value must be overridden by the board. */ >> + clock-frequency = <0>;
Quoting claudiu beznea (2024-09-04 05:17:30) > On 03.09.2024 22:48, Stephen Boyd wrote: > > > > The node name should be something like clock-<frequency> but if the > > frequency is different per-board then I don't know what should happen > > here. > > The frequency should be always around 32768 Hz but not necessarily exactly > 32768 Hz. It depends on what is installed on the board, indeed. RTC can do > time error adjustments based on the variations around 32768 Hz. > > > Can you leave the vbattb_xtal phandle up above and then require > > the node to be defined in the board with the proper frequency after the > > dash? > > Is it OK for you something like this (applied on top of this series)? Yes, it's too bad we can't append to a property in DT, or somehow leave alone certain cells and only modify one of them.
Hi Stephen, On Thu, Sep 5, 2024 at 8:09 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting claudiu beznea (2024-09-04 05:17:30) > > On 03.09.2024 22:48, Stephen Boyd wrote: > > > The node name should be something like clock-<frequency> but if the > > > frequency is different per-board then I don't know what should happen > > > here. > > > > The frequency should be always around 32768 Hz but not necessarily exactly > > 32768 Hz. It depends on what is installed on the board, indeed. RTC can do > > time error adjustments based on the variations around 32768 Hz. > > > > > Can you leave the vbattb_xtal phandle up above and then require > > > the node to be defined in the board with the proper frequency after the > > > dash? > > > > Is it OK for you something like this (applied on top of this series)? > > Yes, it's too bad we can't append to a property in DT, or somehow leave > alone certain cells and only modify one of them. My main objections are that (1) this approach is different than the one used for all other external clock inputs on Renesas SoCs, and (2) this requires duplicating part of the clocks property in all board DTS files. Gr{oetje,eeting}s, Geert
Quoting Geert Uytterhoeven (2024-09-06 00:28:38) > Hi Stephen, > > On Thu, Sep 5, 2024 at 8:09 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting claudiu beznea (2024-09-04 05:17:30) > > > On 03.09.2024 22:48, Stephen Boyd wrote: > > > > The node name should be something like clock-<frequency> but if the > > > > frequency is different per-board then I don't know what should happen > > > > here. > > > > > > The frequency should be always around 32768 Hz but not necessarily exactly > > > 32768 Hz. It depends on what is installed on the board, indeed. RTC can do > > > time error adjustments based on the variations around 32768 Hz. > > > > > > > Can you leave the vbattb_xtal phandle up above and then require > > > > the node to be defined in the board with the proper frequency after the > > > > dash? > > > > > > Is it OK for you something like this (applied on top of this series)? > > > > Yes, it's too bad we can't append to a property in DT, or somehow leave > > alone certain cells and only modify one of them. > > My main objections are that (1) this approach is different than the one used > for all other external clock inputs on Renesas SoCs, and (2) this requires > duplicating part of the clocks property in all board DTS files. > Can 'clock-ranges' be used here? Leave the cell as null in the SoC dtsi file and then fill it in with clocks property at the parent node. I think you'd have to use clock-names for this though.
Hi Stephen, On Sat, Sep 7, 2024 at 1:01 AM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Geert Uytterhoeven (2024-09-06 00:28:38) > > On Thu, Sep 5, 2024 at 8:09 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting claudiu beznea (2024-09-04 05:17:30) > > > > On 03.09.2024 22:48, Stephen Boyd wrote: > > > > > The node name should be something like clock-<frequency> but if the > > > > > frequency is different per-board then I don't know what should happen > > > > > here. > > > > > > > > The frequency should be always around 32768 Hz but not necessarily exactly > > > > 32768 Hz. It depends on what is installed on the board, indeed. RTC can do > > > > time error adjustments based on the variations around 32768 Hz. > > > > > > > > > Can you leave the vbattb_xtal phandle up above and then require > > > > > the node to be defined in the board with the proper frequency after the > > > > > dash? > > > > > > > > Is it OK for you something like this (applied on top of this series)? > > > > > > Yes, it's too bad we can't append to a property in DT, or somehow leave > > > alone certain cells and only modify one of them. > > > > My main objections are that (1) this approach is different than the one used > > for all other external clock inputs on Renesas SoCs, and (2) this requires > > duplicating part of the clocks property in all board DTS files. > > Can 'clock-ranges' be used here? Leave the cell as null in the SoC dtsi > file and then fill it in with clocks property at the parent node. I > think you'd have to use clock-names for this though. "clock-ranges" does not seem to be well-documented... IUIC, your suggestion is to: 1. Add "clock-ranges" to the /soc subnode, 2. Completely leave out the "rtx" clock from the clocks property of the vbattb@1005c000 node, 3. Add the following to the board DTS: &soc { clocks = <&vbattb_xtal>; clock-names = "rtx"; }; Then, when resolving "rtx" for the vbattb@1005c000 node, of_parse_clkspec() would iterate up and find the proper vbattb_xtal. Is that correct? And probably that should be done for other external clock inputs as well? Still, it looks a bit complicated and un-intuitive. And what about e.g. carrier boards with a SoM, where some clocks are provided by the SoM, and some by the carrier? In that case you still have to override the clock and clock-names properties in the carrier .dts, thus duplicating all clocks provided by the SoM. So I prefer the original approach, like is done for all other external SoC clock inputs on Renesas SoCs. Gr{oetje,eeting}s, Geert
Quoting Geert Uytterhoeven (2024-09-09 05:11:03) > Hi Stephen, > > On Sat, Sep 7, 2024 at 1:01 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Geert Uytterhoeven (2024-09-06 00:28:38) > > > > > > My main objections are that (1) this approach is different than the one used > > > for all other external clock inputs on Renesas SoCs, and (2) this requires > > > duplicating part of the clocks property in all board DTS files. > > > > Can 'clock-ranges' be used here? Leave the cell as null in the SoC dtsi > > file and then fill it in with clocks property at the parent node. I > > think you'd have to use clock-names for this though. > > "clock-ranges" does not seem to be well-documented... Yeah, I wasn't aware of it for years! > > IUIC, your suggestion is to: > 1. Add "clock-ranges" to the /soc subnode, > 2. Completely leave out the "rtx" clock from the clocks property > of the vbattb@1005c000 node, > 3. Add the following to the board DTS: > > &soc { > clocks = <&vbattb_xtal>; > clock-names = "rtx"; > }; > > Then, when resolving "rtx" for the vbattb@1005c000 node, > of_parse_clkspec() would iterate up and find the proper vbattb_xtal. > Is that correct? And probably that should be done for other external > clock inputs as well? Sounds about right. > > Still, it looks a bit complicated and un-intuitive. And what about > e.g. carrier boards with a SoM, where some clocks are provided by > the SoM, and some by the carrier? In that case you still have to > override the clock and clock-names properties in the carrier .dts, > thus duplicating all clocks provided by the SoM. This is the same case as the board wanting to override the soc node? When it's a SoM is there a node for the SoM? Is the clock on the SoM? Does this case exist? Hopefully this isn't a straw man. > > So I prefer the original approach, like is done for all other external > SoC clock inputs on Renesas SoCs. > Sure. I'm just suggesting to follow the preferred approach by DT maintainers. I don't feel strongly either way and I'm not the SoC maintainer so feel free to do what you want.
Hi Stephen, On Mon, Sep 9, 2024 at 11:18 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Geert Uytterhoeven (2024-09-09 05:11:03) > > On Sat, Sep 7, 2024 at 1:01 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting Geert Uytterhoeven (2024-09-06 00:28:38) > > > > > > > > My main objections are that (1) this approach is different than the one used > > > > for all other external clock inputs on Renesas SoCs, and (2) this requires > > > > duplicating part of the clocks property in all board DTS files. > > > > > > Can 'clock-ranges' be used here? Leave the cell as null in the SoC dtsi > > > file and then fill it in with clocks property at the parent node. I > > > think you'd have to use clock-names for this though. > > > > "clock-ranges" does not seem to be well-documented... > > Yeah, I wasn't aware of it for years! > > > IUIC, your suggestion is to: > > 1. Add "clock-ranges" to the /soc subnode, > > 2. Completely leave out the "rtx" clock from the clocks property > > of the vbattb@1005c000 node, > > 3. Add the following to the board DTS: > > > > &soc { > > clocks = <&vbattb_xtal>; > > clock-names = "rtx"; > > }; > > > > Then, when resolving "rtx" for the vbattb@1005c000 node, > > of_parse_clkspec() would iterate up and find the proper vbattb_xtal. > > Is that correct? And probably that should be done for other external > > clock inputs as well? > > Sounds about right. > > > Still, it looks a bit complicated and un-intuitive. And what about > > e.g. carrier boards with a SoM, where some clocks are provided by > > the SoM, and some by the carrier? In that case you still have to > > override the clock and clock-names properties in the carrier .dts, > > thus duplicating all clocks provided by the SoM. > > This is the same case as the board wanting to override the soc node? Yes, but more complicated, > When it's a SoM is there a node for the SoM? Is the clock on the SoM? > Does this case exist? Hopefully this isn't a straw man. E.g. the White Hawk CPU board[1] contains extal_clk, extalr_clk, and scif_clk, so it would need something like: &soc { clocks = <&extal_clk>, <&extalr_clk>, <&scif_clk>; clocks-names = "extal", "extalr", "scif"; }; The White Hawk Break-Out board[2] contains can_clk, so it would need to append that, by overriding (duplicate + append): &soc { clocks = <&extal_clk>, <&extalr_clk>, <&scif_clk>, <&can_clk>; clocks-names = "extal", "extalr", "scif", "can"; }; Currently, [1] does: &extal_clk { clock-frequency = <16666666>; }; &extalr_clk { clock-frequency = <32768>; }; &scif_clk { clock-frequency = <24000000>; }; And [2] does: &can_clk { clock-frequency = <40000000>; }; [1] arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi: [2] arch/arm64/boot/dts/renesas/white-hawk-common.dtsi Gr{oetje,eeting}s, Geert
Hi Claudiu, On Fri, Aug 30, 2024 at 3:02 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add the DT node for the VBATTB IP along with DT bindings for the clock > it provides. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v3: > - dropped the child nodes of vbattb; along with this dropped ranges, > interrupt-names, #address-cells, #size-cells > - added vbattb_xtal as input clock for vbattb Thanks for the update! > --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > @@ -160,6 +160,18 @@ i2c3: i2c@10090c00 { > status = "disabled"; > }; > > + vbattb: vbattb@1005c000 { Please insert this after serial@1004b800, to preserve sort order. > + compatible = "renesas,r9a08g045-vbattb"; > + reg = <0 0x1005c000 0 0x1000>; > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; > + clock-names = "bclk", "rtx"; > + #clock-cells = <1>; > + power-domains = <&cpg>; > + resets = <&cpg R9A08G045_VBAT_BRESETN>; > + status = "disabled"; > + }; > + > cpg: clock-controller@11010000 { > compatible = "renesas,r9a08g045-cpg"; > reg = <0 0x11010000 0 0x10000>; The rest LGTM, so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi index 067a26a66c24..247fa80a4f53 100644 --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi @@ -160,6 +160,18 @@ i2c3: i2c@10090c00 { status = "disabled"; }; + vbattb: vbattb@1005c000 { + compatible = "renesas,r9a08g045-vbattb"; + reg = <0 0x1005c000 0 0x1000>; + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb_xtal>; + clock-names = "bclk", "rtx"; + #clock-cells = <1>; + power-domains = <&cpg>; + resets = <&cpg R9A08G045_VBAT_BRESETN>; + status = "disabled"; + }; + cpg: clock-controller@11010000 { compatible = "renesas,r9a08g045-cpg"; reg = <0 0x11010000 0 0x10000>; @@ -425,4 +437,11 @@ timer { interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt"; }; + + vbattb_xtal: vbattb-xtal { + compatible = "fixed-clock"; + #clock-cells = <0>; + /* This value must be overridden by the board. */ + clock-frequency = <0>; + }; };