On 18.07.2024 03:39, Stephen Boyd wrote: > Quoting claudiu beznea (2024-07-17 01:31:20) >> Hi, Stephen, >> >> On 17.07.2024 01:28, Stephen Boyd wrote: >>> Quoting Claudiu (2024-07-16 03:30:17) >>>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c >>>> new file mode 100644 >>>> index 000000000000..8effe141fc0b >>>> --- /dev/null >>>> +++ b/drivers/clk/renesas/clk-vbattb.c >>>> @@ -0,0 +1,212 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * VBATTB clock driver >>>> + * >>>> + * Copyright (C) 2024 Renesas Electronics Corp. >>>> + */ >>>> + >>>> +#include <linux/cleanup.h> >>>> +#include <linux/clk.h> >>> >>> Prefer clk providers to not be clk consumers. >> >> I added it here to be able to use devm_clk_get_optional() as it was >> proposed to me in v1 to avoid adding a new binding for bypass and detect if >> it's needed by checking the input clock name. >> > > Understood. > >> >>> >>> I also wonder if this is really a mux, >> >> It's a way to determine what type of clock (crystal oscillator or device >> clock) is connected to RTXIN/RTXOUT pins of the module >> (the module block diagram at [1]) based on the clock name. Depending on the >> type of the clock connected to RTXIN/RTXOUT we need to select the XC or >> XBYP as input for the mux at [1]. >> >> [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png > > That diagram shows a mux block, so at least something in there is a mux. > From what I can tell the binding uses clock-names to describe the mux. > What I'd like to avoid is using clk_get() to determine how to configure > the mux. That's because clk_get() is a clk consumer API, and because we > want clk providers to be able to register clks without making sure that > the entire parent chain has been registered first. Eventually, we'd like > clk_get() to probe defer if the clk is an orphan. Having clk providers > use clk_get() breaks that pretty quickly. > >> >> >>> and either assigned-clock-parents should be used, >>> or the clk_ops should have an init routine that looks at >>> which parent is present by determining the index and then use that to >>> set the mux. The framework can take care of failing to set the other >>> parent when it isn't present. >> >> >> On the board, at any moment, it will be only one clock as input to the >> VBATTB clock (either crystal oscillator or a clock device). If I'm getting >> you correctly, this will involve describing both clocks in some scenarios. >> >> E.g. if want to use crystal osc, I can use this DT description: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>; >> clock-names = "xin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> If external clock device is to be used, I should describe a fake clock too: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>, <&ext_clk>; > > Is vbattb_xtal the fake clk? If so, I'd expect this to be > > clocks = <0>, <&ext_clk>; > > so that we don't have a useless clk node. > >> clock-names = "xin", "clkin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <0>; >> }; >> >> ext_clk: ext-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> Is this what you are suggesting? >> > > 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." [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?
