Hi, On Tue, Mar 30, 2021 at 10:05:45AM -0700, Saravana Kannan wrote: > On Tue, Mar 30, 2021 at 2:09 AM Sebastian Reichel > <sebastian.reichel@xxxxxxxxxxxxx> wrote: > > On Mon, Mar 29, 2021 at 05:36:11PM -0700, Saravana Kannan wrote: > > > On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel > > > <sebastian.reichel@xxxxxxxxxxxxx> wrote: > > > > On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote: > > > > > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel > > > > > <sebastian.reichel@xxxxxxxxxxxxx> wrote: > > > > > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote: > > > > > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > > > > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote: > > > > > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks > > > > > > > > > is provided by an I2C RTC. Specifying this properly results in a > > > > > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot > > > > > > > > > be initialized without the i.MX6 clock controller being initialized. > > > > > > > > > > > > > > > > > > With current code the following path is executed when i.MX6 clock > > > > > > > > > controller is probed (and ckil clock is specified to be the I2C RTC > > > > > > > > > via DT): > > > > > > > > > > > > > > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0); > > > > > > > > > 2. of_clk_get_by_name(ccm_node, "ckil"); > > > > > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil"); > > > > > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil") > > > > > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle > > > > > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER > > > > > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred > > > > > > > > > 8. I2C controller is never initialized without clock controller > > > > > > > > > I2C RTC is never initialized without I2C controller > > > > > > > > > CKIL clock is never initialized without I2C RTC > > > > > > > > > clock controller is never initialized without CKIL > > > > > > > > > > > > > > > > > > To fix the circular dependency this registers a dummy clock when > > > > > > > > > the RTC clock is tried to be acquired. The dummy clock will later > > > > > > > > > be unregistered when the proper clock is registered for the RTC > > > > > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of > > > > > > > > > fixing up the clock tree. > > > > > > > > > > > > > > > > > > NOTE: For now the patch is compile tested only. If this approach > > > > > > > > > is the correct one I will do some testing and properly submit this. > > > > > > > > > You can find all the details about the hardware in the following > > > > > > > > > patchset: > > > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@xxxxxxxxxxxxx/ > > > > > > > > > > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx> > > > > > > > > > --- > > > > > > > > > .../bindings/clock/clock-bindings.txt | 7 + > > > > > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++ > > > > > > > > > 2 files changed, 153 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > > > > > > > > > index f2ea53832ac6..66d67ff4aa0f 100644 > > > > > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > > > > > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > > > > > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal > > > > > > > > > Clock consumer nodes must never directly reference > > > > > > > > > the provider's clock-output-names property. > > > > > > > > > > > > > > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled > > > > > > > > > + by default with the provided frequency at boot time. This > > > > > > > > > + is required to break circular clock dependencies. For clock > > > > > > > > > + providers with #clock-cells = 0 this is a single u32 > > > > > > > > > + with the frequency in Hz. Otherwise it's a list of > > > > > > > > > + clock cell specifier + frequency in Hz. > > > > > > > > > > > > > > > > Seems alright to me. I hadn't thought about the aspect of needing to > > > > > > > > know the frequency. Other cases probably don't as you only need the > > > > > > > > clocks once both components have registered. > > > > > > > > > > > > > > > > Note this could be lost being threaded in the other series. > > > > > > > > > > > > > > I read this thread and tried to understand it. But my head isn't right > > > > > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look > > > > > > > at it again after the weekend. In the meantime, Sebastian can you > > > > > > > please point me to the DT file and the specific device nodes (names or > > > > > > > line number) where this cycle is present? > > > > > > > > > > > > I have not yet sent an updated DT file, but if you look at this > > > > > > submission: > > > > > > > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@xxxxxxxxxxxxx/ > > > > > > > > > > > > There is a node > > > > > > > > > > > > rtc: m41t62@68 { compatible = "st,m41t62"; }; > > > > > > > > > > > > That is an I2C RTC, which provides a 32.768 kHz clock by default > > > > > > (i.e. after power loss). This clock signal is used to provide the > > > > > > i.MX6 CKIL: > > > > > > > > > > > > ------------------------------------ > > > > > > &clks { > > > > > > clocks = <&rtc>; > > > > > > clock-names = "ckil"; > > > > > > }; > > > > > > ------------------------------------ > > > > > > > > > > > > > Keeping a clock on until all its consumers probe is part of my TODO > > > > > > > list (next item after fw_devlink=on lands). I already have it working > > > > > > > in AOSP, but need to clean it up for upstream. fw_devlink can also > > > > > > > break *some* cycles (not all). So I'm wondering if the kernel will > > > > > > > solve this automatically soon(ish). If it can solve it automatically, > > > > > > > I'd rather not add new DT bindings because it'll make it more work for > > > > > > > fw_devlink. > > > > > > > > > > > > As written above on Congatec QMX6 an I2C RTC provides one of the > > > > > > SoC's input frequencies. The SoC basically expects that frequency > > > > > > to be always enabled and this is what it works like before clock > > > > > > support had been added to the RTC driver. > > > > > > > > > > Thanks. I skimmed through the RTC driver code and > > > > > imx6q_obtain_fixed_clk_hw() and the DT files. > > > > > > > > > > > > > > > > > With the link properly being described the Kernel tries to probe > > > > > > the SoC's clock controller during early boot. Then it tries to get a > > > > > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw() > > > > > > and that returns -EPROBE_DEFER (because the RTC driver has not > > > > > > yet been probed). > > > > > > > > > > But the RTC (which is a proper I2C device) will never probe before > > > > > CLK_OF_DECLARE() initializes the core clock controller. So, it's not > > > > > clear how "protected-clocks" helps here since it doesn't change > > > > > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which > > > > > is called from the CLK_OF_DECLARE() callback). Oof... I see what you > > > > > are doing with of_clk_register_boot_clk(). You are having the consumer > > > > > register its own clock and then use it. Kinda beats the whole point of > > > > > describing the link in the first place. > > > > > > > > I agree, that it does not make sense from a code point of view for > > > > this platform. All of this is just to make the DT look correct. > > > > From a platform point of view the most logical way is to handle the > > > > RTC clock as do-not-touch always enabled fixed-clock. > > > > > > > > > > Without the clock controller basically none of > > > > > > the i.MX6 SoC drivers can probe including the I2C driver. Without > > > > > > the I2C bus being registered, the RTC driver never probes and the > > > > > > boot process is stuck. > > > > > > > > > > > > I'm not sure how fw_devlink can help here. > > > > > > > > > > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was > > > > > implemented as an actual platform device driver and not using > > > > > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this > > > > > assumption later. > > > > > > > > > > In that case, fw_devlink will notice this cycle: > > > > > syntax: consumer -(reason)-> supplier > > > > > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks. > > > > > > > > > > It'll then reason that it doesn't make sense for a device (clks) to > > > > > have a supplier (rtc) whose parent (i2c3) in turn depends on the > > > > > device (clks). It'll then drop the clks -> rtc dependency because > > > > > that's the most illogical one in terms of probing. > > > > > > > > > > So all you'd need to do is delete any -EPROBE defer you might do in > > > > > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle, > > > > > fw_devlink will make sure the supplier of ckil has probed first. For > > > > > cases where there's a cycle like this, it'll be smart enough to drop > > > > > this dependency during probe ordering. > > > > > > > > What do you mean drop? Anything using ckil will not be registered? > > > > That will basically kill the system within a few seconds, since the > > > > watchdog hardware uses ckil. > > > > > > No, it means that it won't block CCM on ckil. It's not a generic > > > "ignore dependency for all consumers of ckil". fw_devlink does this > > > specifically to the link that causes a probe dependency cycle. > > > > I still don't follow. If CCM proceeds booting without blocking on > > missing CCM, > > I think you meant to say missing CKIL, Yes. > > what would be the content of hws[IMX6QDL_CLK_CKIL]? > > What ensures, that the consumers get correct clock rates? > > I haven't dug into the IMX CCM driver, but my current understanding is > that the 32 KHz clock is the CKIL input coming into CCM and it's a > parent/ancestor to some/all of the CCM clocks. Right. > The clock framework allows you to register clocks before their > parents are registered (because clocks are messy and clock > providers can have cycles between them). So if the IMX CCM driver > is written correctly, it'd register the clock with the clock > framework saying "hey, my parent is clock CKIL from this other DT > node, connect us up when it's registered". I'll let you figure > out the details of the implementation. I've been told this is supposed to work in theory before, but nobody could point at an example. All drivers, that I checked end up with -EPROBE_DEFER on missing parent clocks, which is good enough for most dependency issues, but not for cyclic dependencies. > Also, as I said before, the fixed-clock(s) will be available and > working before the RTC probes. So, either the CCM registers first or > the CKIL fixed-clock registers first and the same caller would > register the other. And then the clock framework will connect them up > and everything will continue working nicely. Yes, since fixed-clock is completley unrelated to RTC. Without further driver changes the RTC would still register a clock device and disable the clock because of it being unused. > > > > > I don't know enough about the clocks in imx6q to comment if you can > > > > > get away without using CLK_OF_DECLARE() at all. The only clock that > > > > > really needs to use CLK_OF_DECLARE() is any clock that's needed for > > > > > the scheduler timer. Other than that, everything else can be > > > > > initialized by a normal driver. Including UART clocks. I can get into > > > > > more specifics if you go down this path. > > > > > > > > > > So, that's how fw_devlink could help here if you massage > > > > > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll > > > > > have to set fw_devlink=on in the kernel commandline though (it's work > > > > > in progress to set this by default). There are some additional details > > > > > here about keeping clocks on, but we can discuss the solution for that > > > > > if it becomes an issue. > > > > > > > > > > > I see exactly two > > > > > > options to solve this: > > > > > > > > > > > > a) do not describe the link and keep RTC clock enabled somehow. > > > > > > (my initial patchset) > > > > > > b) describe the link, but ignore it during boot. > > > > > > (what I'm trying to do here) > > > > > > > > > > > > > > > > Even if you completely ignore fw_devlink, why not just model this > > > > > clock as a fixed-clock in DT for this specific machine? > > > > > > > > > > It's clearly expecting the clock to be an always on fixed clock. > > > > > > > > Yes. SoC runs unreliably with this. Downstream vendor kernel does > > > > not contain a clock driver for the squarewave pin of the RTC (i.e. > > > > their driver does not yet contain 1373e77b4f10) and just works. > > > > Upstream kernel disables the RTC's squarewave and then goes into > > > > reboot loop because of watchdog going crazy. > > > > > > > > > This will also remove the need for adding "boot-clock-frequencies" > > > > > binding. "fixed-clocks" devices are initialized very early on > > > > > (they use CLK_OF_DECLARE too) even without their parents probing > > > > > (not sure I agree with this, but this is how it works now). > > > > > > > > > > Something like: > > > > > > > > > > rtc: m41t62@68 { > > > > > compatible = "st,m41t62"; > > > > > reg = <0x68>; > > > > > > > > > > clock-ckil { > > > > > compatible = "fixed-clock"; > > > > > #clock-cells = <0>; > > > > > clock-frequency = <32768>; > > > > > }; > > > > > }; > > > > > > > > > > I hope this helps. > > > > > > > > This looks like a complex way of my initial patchset with > > > > 'protected-clocks' property replaced by a fixed-clock > > > > node. RTC driver needs to check if that exists and avoid > > > > registering its own clock. > > > > > > If anything, I'd argue this is a lot more simpler because it avoids > > > adding a new DT binding, it avoids changes to drivers/clk/clk.c. > > > > My original patch [0] is a two liner, which does not change > > drivers/clk/clk.c and protected-clocks is a standard property > > from [1]. I think you confused this with the boot-clock-frequencies > > approach :) > > I think my confusion was that you wanted to do both [0] and [1] > because of them being threaded and not having v1/v2. Yes, I send PATCHv1 and an incomplete RFCv2 labled RFC, sorry. > Just to clarify, I'm not NAKing any patch here. I'm just explaining > how things work and giving options because I was CCed. I'll leave it > to Stephen/Rob to decide what they want to accept. and I try to figure out how to get this thing supported upstream, which blocks the whole series adding a new system on module and five similar boards using it :) > But I can see the point in Rob's request for wanting the DT to capture > the real hardware connections correctly. > > > [0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@xxxxxxxxxxxxx/ > > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt. > > > > > Instead of checking for "protected-clocks" you just check for this > > > child node (or just any child node). Also, technically if you set the > > > CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any > > > explicit checking in the RTC driver as long as some other driver > > > doesn't try to get this clock and turn it on/off. > > > > Child nodes are part of DT binding, so the information about the > > potential clock subnode also needs to be added to the RTC binding. > > It also changes the reference point from referencing the RTC node > > to referencing a subnode, which seems a bit inconsistent to me. > > Sure, you can add a child node to the RTC binding. /me is confused. This is what you suggested, see "Something like:" > But it's not a new DT property binding (if you go with option 2). well of course binding for RTC and for fixed-clock already exist, but RTC binding needs to be changed to support a fixed-clock sub-node (binding, not driver!). If Rob is fine with this I can take that route. -- Sebastian
Attachment:
signature.asc
Description: PGP signature
