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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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? 


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux