[RFC] clk: add boot clock support

[RFC] clk: add boot clock support

[Sebastian Reichel]

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@collabora.com/

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../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.
+
 For example:
 
     oscillator {
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..029088ed5f1a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4499,6 +4499,146 @@ static const struct of_device_id __clk_of_table_sentinel
 static LIST_HEAD(of_clk_providers);
 static DEFINE_MUTEX(of_clk_mutex);
 
+struct of_boot_clk {
+	struct of_clk_provider cp;
+	struct clk_hw clk;
+	unsigned long rate;
+};
+
+/**
+ * of_clk_get_boot_rate() - Get DT configured boot rate for a DT clkspec
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function provides the boot clock rate configured in DT
+ * for a clkspec without requiring a device being registered in
+ * the kernel.
+ *
+ * This is required for clock setups with circular dependencies,
+ * which only work because of some clocks being enabled
+ * automatically.
+ *
+ * The return value is either the rate,
+ * -EINVAL for malformed DT,
+ * -ENODATA if no boot frequency is specified.
+ */
+static int of_clk_get_boot_rate(struct of_phandle_args *clkspec)
+{
+	const struct device_node *np;
+	u32 cells;
+	u32 val;
+
+	if (!clkspec || !clkspec->np)
+		return -EINVAL;
+	np = clkspec->np;
+
+	if (!of_property_read_u32(np, "#clock-cells", &cells))
+		return -EINVAL;
+
+	/* complex clock providers are currently not supported */
+	if (cells > 0)
+		return -EINVAL;
+
+	if (!of_property_read_u32(np, "boot-clock-frequencies", &val))
+		return -ENODATA;
+
+	return val;
+}
+
+static struct clk_hw *of_boot_clk_get(struct of_phandle_args *clkspec, void *data)
+{
+	return data;
+}
+
+static unsigned long
+of_boot_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct of_boot_clk *bootclk = container_of(hw, struct of_boot_clk, clk);
+
+	return bootclk->rate;
+}
+
+static const struct clk_ops of_boot_clk_ops = {
+	.recalc_rate = of_boot_clk_recalc_rate,
+};
+
+/**
+ * of_clk_register_boot_clk() - Register a boot clock provider for a node
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * Register a fixed rate dummy clock for solving circular dependencies
+ * during boot. This is expected to be replaced by a real clock device
+ * once the correct driver is probed.
+ *
+ * The function expects of_clk_mutex to be locked.
+ *
+ * Returns:
+ *  -EPROBE_DEFER, if DT does not specify a boot clock
+ *  -ENOMEM, if there is not sufficient memory available
+ *  -EINVAL, if DT contains invalid data
+ *  dummy clk_hw device on success
+ */
+static struct clk_hw *
+of_clk_register_boot_clk(struct of_phandle_args *clkspec)
+{
+	struct of_boot_clk *bootclk;
+	struct clk_init_data init;
+	int rate = of_clk_get_boot_rate(clkspec);
+
+	WARN_ON(!mutex_is_locked(&of_clk_mutex));
+
+	if (rate < 0) {
+		if (rate == -ENODATA)
+			return ERR_PTR(-EPROBE_DEFER);
+		return ERR_PTR(rate);
+	}
+
+	bootclk = kzalloc(sizeof(*bootclk), GFP_KERNEL);
+	if (!bootclk)
+		return ERR_PTR(-ENOMEM);
+
+	bootclk->rate = rate;
+
+	/* TODO: name should be unique, use idr_alloc */
+	init.name = "dummy-boot-clk";
+	init.ops = &of_boot_clk_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.num_parents = 0;
+	bootclk->clk.init = &init;
+
+	bootclk->cp.node = of_node_get(clkspec->np);
+	bootclk->cp.data = bootclk;
+	bootclk->cp.get_hw = of_boot_clk_get;
+
+	/* TODO: use same name as in clk.init.name */
+	clk_hw_register_clkdev(&bootclk->clk, NULL, "dummy-boot-clk");
+
+	list_add(&bootclk->cp.link, &of_clk_providers);
+
+	pr_debug("Added clk_hw boot provider from %pOF\n", clkspec->np);
+
+	return &bootclk->clk;
+}
+
+static void of_clk_unregister_boot_clk(struct device_node *np)
+{
+	struct of_boot_clk *bootclk;
+	struct of_clk_provider *cp;
+
+	WARN_ON(!mutex_is_locked(&of_clk_mutex));
+
+	list_for_each_entry(cp, &of_clk_providers, link) {
+		if (cp->node == np && cp->get_hw == of_boot_clk_get) {
+			bootclk = container_of(cp, struct of_boot_clk, cp);
+			list_del(&cp->link);
+			// TODO: undo clk_hw_register_clkdev
+			of_node_put(cp->node);
+			kfree(bootclk);
+			break;
+		}
+	}
+}
+
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				     void *data)
 {
@@ -4566,6 +4706,7 @@ int of_clk_add_provider(struct device_node *np,
 	cp->get = clk_src_get;
 
 	mutex_lock(&of_clk_mutex);
+	of_clk_unregister_boot_clk(np);
 	list_add(&cp->link, &of_clk_providers);
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clock from %pOF\n", np);
@@ -4605,6 +4746,7 @@ int of_clk_add_hw_provider(struct device_node *np,
 	cp->get_hw = get;
 
 	mutex_lock(&of_clk_mutex);
+	of_clk_unregister_boot_clk(np);
 	list_add(&cp->link, &of_clk_providers);
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clk_hw provider from %pOF\n", np);
@@ -4837,6 +4979,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
 				break;
 		}
 	}
+
+	if (hw == ERR_PTR(-EPROBE_DEFER))
+		hw = of_clk_register_boot_clk(clkspec);
+
 	mutex_unlock(&of_clk_mutex);
 
 	return hw;
-- 
2.30.2

Re: [RFC] clk: add boot clock support

[Rob Herring]

+Saravana

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@collabora.com/
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../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.

Rob

Re: [RFC] clk: add boot clock support

[Saravana Kannan]

On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
>
> +Saravana
>
> 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@collabora.com/
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../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?

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.

Thanks,
Saravana

Re: [RFC] clk: add boot clock support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6080 bytes --]

Hi Saravana,

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@kernel.org> wrote:
> >
> > +Saravana
> >
> > 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@collabora.com/
> > >
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  .../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@collabora.com/

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.

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). 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 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)

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] clk: add boot clock support

[Saravana Kannan]

On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi Saravana,
>
> 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@kernel.org> wrote:
> > >
> > > +Saravana
> > >
> > > 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@collabora.com/
> > > >
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > ---
> > > >  .../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@collabora.com/
>
> 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.

> 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.

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. 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.

-Saravana

Re: [RFC] clk: add boot clock support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 10840 bytes --]

Hi,

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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > >
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > ---
> > > > >  .../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@collabora.com/
> >
> > 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.

> 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.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] clk: add boot clock support

[Saravana Kannan]

On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> 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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > > >
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > ---
> > > > > >  .../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@collabora.com/
> > >
> > > 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 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.
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.

-Saravana

Re: [RFC] clk: add boot clock support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 13395 bytes --]

Hi,

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@collabora.com> 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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > > > >
> > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > ---
> > > > > > >  .../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@collabora.com/
> > > >
> > > > 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, what would be the content of hws[IMX6QDL_CLK_CKIL]?
What ensures, that the consumers get correct clock rates?

> > > 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 :)

[0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@collabora.com/
[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.

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] clk: add boot clock support

[Saravana Kannan]

On Tue, Mar 30, 2021 at 2:09 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> 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@collabora.com> 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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > > > > >
> > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > ---
> > > > > > > >  .../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@collabora.com/
> > > > >
> > > > > 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,

> 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. 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.

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.

> > > > 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.

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.

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@collabora.com/
> [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. But it's not a new
DT property binding (if you go with option 2).

-Saravana

Re: [RFC] clk: add boot clock support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 17123 bytes --]

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@collabora.com> 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@collabora.com> 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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > > ---
> > > > > > > > >  .../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@collabora.com/
> > > > > >
> > > > > > 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@collabora.com/
> > [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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] clk: add boot clock support

[Saravana Kannan]

On Mon, Apr 5, 2021 at 3:43 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> 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@collabora.com> 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@collabora.com> 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@collabora.com> 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@kernel.org> 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@collabora.com/
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > > > ---
> > > > > > > > > >  .../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@collabora.com/
> > > > > > >
> > > > > > > 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.

Ok I had some time so I looked up some examples for you. Look at
drivers/clk/qcom/dispcc-sm8250.c and see how it used .fw_name =
"bi_tcxo" for the parents. You'll also see that it never does a
clk_get() on "bi_tcxo". It does the same thing for a bunch of other
clock inputs too. See arch/arm64/boot/dts/qcom/sm8250.dtsi for the
node that lists the input clocks. Basically parent's fw_name should
match what you have in DT. Hope that helps.

> > 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.

Right, to avoid turning off the "unused" fixed clock, the RTC driver
will have to NOT register the same clock if you have a fixed-clock
child node.

>
> > > > > > 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@collabora.com/
> > > [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:"

The "sure, you can do it" is referring to you saying you'll need to
update the RTC's binding doc to list the child node.

> > 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.

-Saravana