| Message ID | 20240830130218.3377060-9-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add RTC support for the Renesas RZ/G3S SoC | expand |
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 RTC IP available on the Renesas RZ/G3S SoC. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v3: > - added CPG clock, power domain, reset > - and assigned-clocks, assigned-clock-parents to configure the > VBATTCLK > - included dt-bindings/clock/r9a08g045-vbattb.h Thanks for your patch! > --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi > @@ -160,6 +161,22 @@ i2c3: i2c@10090c00 { > status = "disabled"; > }; > > + rtc: rtc@1004ec00 { Please insert this after serial@1004b800, to preserve sort order. > + compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3"; > + reg = <0 0x1004ec00 0 0x400>; > + interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "alarm", "period", "carry"; > + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb VBATTB_VBATTCLK>; > + clock-names = "bus", "counter"; > + assigned-clocks = <&vbattb VBATTB_MUX>; > + assigned-clock-parents = <&vbattb VBATTB_XC>; Don't the assigned-clock* properties belong in the board DTS? In addition, I think they should be documented in the DT bindings, and be made required, so board developers don't forget about them. > + power-domains = <&cpg>; > + resets = <&cpg R9A08G045_VBAT_BRESETN>; > + status = "disabled"; > + }; > + > vbattb: vbattb@1005c000 { > compatible = "renesas,r9a08g045-vbattb"; > reg = <0 0x1005c000 0 0x1000>; The rest LGTM. Gr{oetje,eeting}s, Geert
Hi, Geert, On 10.10.2024 18:22, Geert Uytterhoeven wrote: > 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 RTC IP available on the Renesas RZ/G3S SoC. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v3: >> - added CPG clock, power domain, reset >> - and assigned-clocks, assigned-clock-parents to configure the >> VBATTCLK >> - included dt-bindings/clock/r9a08g045-vbattb.h > > Thanks for your patch! > >> --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi >> +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi >> @@ -160,6 +161,22 @@ i2c3: i2c@10090c00 { >> status = "disabled"; >> }; >> >> + rtc: rtc@1004ec00 { > > Please insert this after serial@1004b800, to preserve sort order. You're right. I though I have already checked this. > >> + compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3"; >> + reg = <0 0x1004ec00 0 0x400>; >> + interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "alarm", "period", "carry"; >> + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb VBATTB_VBATTCLK>; >> + clock-names = "bus", "counter"; >> + assigned-clocks = <&vbattb VBATTB_MUX>; >> + assigned-clock-parents = <&vbattb VBATTB_XC>; > > Don't the assigned-clock* properties belong in the board DTS? It makes sense to be in the board DTS, indeed. > In addition, I think they should be documented in the DT bindings, > and be made required, so board developers don't forget about them. It would be better, indeed. Thank you, Claudiu Beznea > >> + power-domains = <&cpg>; >> + resets = <&cpg R9A08G045_VBAT_BRESETN>; >> + status = "disabled"; >> + }; >> + >> vbattb: vbattb@1005c000 { >> compatible = "renesas,r9a08g045-vbattb"; >> reg = <0 0x1005c000 0 0x1000>; > > The rest LGTM. > > Gr{oetje,eeting}s, > > Geert >
On 11/10/2024 13:28:55+0300, claudiu beznea wrote: > >> + compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3"; > >> + reg = <0 0x1004ec00 0 0x400>; > >> + interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, > >> + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, > >> + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>; > >> + interrupt-names = "alarm", "period", "carry"; > >> + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb VBATTB_VBATTCLK>; > >> + clock-names = "bus", "counter"; > >> + assigned-clocks = <&vbattb VBATTB_MUX>; > >> + assigned-clock-parents = <&vbattb VBATTB_XC>; > > > > Don't the assigned-clock* properties belong in the board DTS? > > It makes sense to be in the board DTS, indeed. > > > In addition, I think they should be documented in the DT bindings, > > and be made required, so board developers don't forget about them. > > It would be better, indeed. There were multiple recent emails from Rob saying that assigned-clocks should not be part of the bindings.
Hi Alexandre, On Thu, Oct 17, 2024 at 12:03 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > On 11/10/2024 13:28:55+0300, claudiu beznea wrote: > > >> + compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3"; > > >> + reg = <0 0x1004ec00 0 0x400>; > > >> + interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, > > >> + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, > > >> + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>; > > >> + interrupt-names = "alarm", "period", "carry"; > > >> + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb VBATTB_VBATTCLK>; > > >> + clock-names = "bus", "counter"; > > >> + assigned-clocks = <&vbattb VBATTB_MUX>; > > >> + assigned-clock-parents = <&vbattb VBATTB_XC>; > > > > > > Don't the assigned-clock* properties belong in the board DTS? > > > > It makes sense to be in the board DTS, indeed. > > > > > In addition, I think they should be documented in the DT bindings, > > > and be made required, so board developers don't forget about them. > > > > It would be better, indeed. > > There were multiple recent emails from Rob saying that assigned-clocks > should not be part of the bindings. You mean "In general, assigned-clocks* do not need to be documented and should never be required."[1] and "Whatever clock setup needed is outside the scope of a binding"[2]? In this case, it's not generic, and not specific to "renesas,rz-rtca3", but related to the integration of RTC-A3 on the RZ/G3S SoC. Which is indeed not relevant for the RTC-A3 bindings, so I agree the assigned-clock* properties should not be part of this binding. At least they're present in the example in the bindings ;-) [1] https://lore.kernel.org/all/20241015211540.GA1968867-robh@kernel.org [2] https://lore.kernel.org/all/20241015164609.GA1235530-robh@kernel.org Gr{oetje,eeting}s, Geert
diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi index 247fa80a4f53..ac9b6733d289 100644 --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi @@ -7,6 +7,7 @@ #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/clock/r9a08g045-cpg.h> +#include <dt-bindings/clock/r9a08g045-vbattb.h> / { compatible = "renesas,r9a08g045"; @@ -160,6 +161,22 @@ i2c3: i2c@10090c00 { status = "disabled"; }; + rtc: rtc@1004ec00 { + compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3"; + reg = <0 0x1004ec00 0 0x400>; + interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "alarm", "period", "carry"; + clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&vbattb VBATTB_VBATTCLK>; + clock-names = "bus", "counter"; + assigned-clocks = <&vbattb VBATTB_MUX>; + assigned-clock-parents = <&vbattb VBATTB_XC>; + power-domains = <&cpg>; + resets = <&cpg R9A08G045_VBAT_BRESETN>; + status = "disabled"; + }; + vbattb: vbattb@1005c000 { compatible = "renesas,r9a08g045-vbattb"; reg = <0 0x1005c000 0 0x1000>;