diff mbox series

[v3,07/12] arm64: dts: renesas: r9a08g045: Add VBATTB node

Message ID 20240830130218.3377060-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Add RTC support for the Renesas RZ/G3S SoC | expand

Commit Message

Claudiu Beznea Aug. 30, 2024, 1:02 p.m. UTC
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

Changes in v2:
- update compatibles
- updated clocks and clock-names for clock-controller node
- removed the power domain from the clock-controller as this is
  controlled by parent node in v2

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Stephen Boyd Sept. 3, 2024, 7:48 p.m. UTC | #1
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>;
Claudiu Beznea Sept. 4, 2024, 12:17 p.m. UTC | #2
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>;
Stephen Boyd Sept. 5, 2024, 6:09 p.m. UTC | #3
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.
Geert Uytterhoeven Sept. 6, 2024, 7:28 a.m. UTC | #4
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
Stephen Boyd Sept. 6, 2024, 11:01 p.m. UTC | #5
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.
Geert Uytterhoeven Sept. 9, 2024, 12:11 p.m. UTC | #6
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
Stephen Boyd Sept. 9, 2024, 9:18 p.m. UTC | #7
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.
Geert Uytterhoeven Sept. 10, 2024, 8:02 a.m. UTC | #8
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
Geert Uytterhoeven Oct. 10, 2024, 3:17 p.m. UTC | #9
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 mbox series

Patch

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>;
+	};
 };