Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting claudiu beznea (2024-07-18 07:41:03)
> 
> 
> On 18.07.2024 03:39, Stephen Boyd wrote:
> > 
> > Sort of. Ignoring the problem with the subnode for the clk driver, I
> > don't really like having clock-names that don't match the hardware pin
> > names. From the diagram you provided, it looks like clock-names should
> > be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the
> > clock-cells should be "1" instead of "0", and the mux should be one of
> > those provided clks and "xc" and "xbyp" should be the other two. If that
> > was done, then assigned-clocks could be used to assign the parent of the
> > mux.
> > 
> > #define VBATTBCLK          0
> > #define VBATTB_XBYP        1
> > #define VBATTB_XC          2
> > 
> >     vbattb: vbattb@1005c000 {
> >         compatible = "renesas,r9a08g045-vbattb";
> >         reg = <0x1005c000 0x1000>;
> >         ranges = <0 0 0x1005c000 0 0x1000>;
> >         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> >         interrupt-names = "tampdi";
> >         clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
> >         clock-names = "bclk", "rtxin";
> >         power-domains = <&cpg>;
> >         resets = <&cpg R9A08G045_VBAT_BRESETN>;
> >         #clock-cells = <1>;
> >         assigned-clocks = <&vbattb VBATTBCLK>;
> >       assigned-clock-parents = <&vbattb VBATTB_XBYP>;
> >         renesas,vbattb-load-nanofarads = <12500>;
> >     };
> 
> I think I got it now. Thank you for the detailed explanation.
> 
> > 
> > One last thing that I don't really understand is why this needs to be a
> > clk provider. In the diagram, the RTC is also part of vbattb, so it
> > looks odd to have this node be a clk provider with #clock-cells at all.
> 
> I did it like this because the RTC is a different IP mapped at it's own
> address and considering the other VBATTB functionalities (tamper, SRAM)
> might be implemented at some point.
> 
> I also failed to notice that RTC might not work w/o bclk being enabled
> (thanks for pointing it).

> 
> I saw that diagram more like describing the always-on power domain
> (PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is
> backed by battery. From HW manual [1]: "PD_VBATT domain is the area where
> the RTC/backup register is located, works on battery power when the power
> of PD_VCC and PD_ISOVCC domain are turned off."

Ah ok, so PD_VBATTB is like a power domain/wrapper and not an IP block
mapped on the bus at the same address as the RTC that it wraps. That
changes things a bit.

> 
> [1]
> https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250
> 
> > Is it the case that if the rtxin pin is connected, you mux that over,
> > and if the pin is disconnected you mux over the internal oscillator? 
> 
> From the description here at [2] I'm getting that the "32-KHz clock
> oscillator" block is used when crystal oscillator is connected to RTXIN,
> RTXOUT pins and it is skipped if external clock device is connected.
> 
> [2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ

It looks like in both cases something is connected to the pins. XC is to
use an external crystal connected to both pins and XBYP is to use a
single clk. Given that, perhaps naming the clk "rtx" is simplest and
then using assigned-clock-parents is most direct because there's only
really one "logical" clk input for this device. That means the XC and
XBYP clks have the same parent, "rtx", and the XC clk is a gateable
fixed rate clk while the XBYP clk is a fixed factor clk that does
nothing besides pass on the rate of the "rtx" clk.

> 
> > I'm
> > really wondering why a clk provider is implemented at all. Why not just
> > hit the registers directly from the RTC driver depending on a
> > devm_clk_get_optional() call?
> 
> I did it like this because the RTC is a different IP mapped at it's own
> address with it's own interrupts, clock, power domain and considering that
> the other VBATTB functionalities (tamper, SRAM) might be used at some point
> in future. At the same time I failed to noticed the VBATTB clock might be
> needed for RTC.

The docs say VBATT in some places. Not sure if you want to rename it to
vbatt and drop the extra b which probably stands for "backup"?

> 
> Do you consider better to just take a regmap to VBATTB from RTC driver and
> set the VBATTB from RTC driver itself?

No, don't do that. The only change from the above DT node is that the
assigned-clocks and assigned-clock-parents property should be moved to
the RTC node.


     vbattb: vbattb@1005c000 {
         compatible = "renesas,r9a08g045-vbattb";
         reg = <0x1005c000 0x1000>;
         ranges = <0 0 0x1005c000 0 0x1000>;
         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
         interrupt-names = "tampdi";
         clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
         clock-names = "bclk", "rtx";
         power-domains = <&cpg>;
         resets = <&cpg R9A08G045_VBAT_BRESETN>;
         #clock-cells = <1>;
         renesas,vbattb-load-nanofarads = <12500>;
     };

     rtc@1004ec00 {
         compatible = "renesas,r9a08g045-rtc";
         reg = <0x1004ec00 0x400>;
         clocks = <&vbattb VBATTBCLK>;
         assigned-clocks = <&vbattb VBATTBCLK>;
         assigned-clock-parents = <&vbattb VBATTB_XBYP>; // Or VBATTB_XC if external crystal connected
     };