[PATCH v3 0/4] Add device links to clocks

[PATCH v3 0/4] Add device links to clocks

[Miquel Raynal]

Hello,

While working on suspend to RAM feature, I ran into troubles multiple
times when clocks where not suspending/resuming at the desired time. I
had a look at the core and I think the same logic as in the
regulator's core may be applied here to (very easily) fix this issue:
using device links.

The only additional change I had to do was to always (when available)
populate the device entry of the core clock structure so that it could
be used later. This is the purpose of patch 1. Patch 2 actually adds
support for device links.

Here is a step-by-step explanation of how links are managed, following
Maxime Ripard's suggestion.


The order of probe has no importance because the framework already
handles orphaned clocks so let's be simple and say there are two root
clocks, not depending on anything, that are probed first: xtal0 and
xtal1. None of these clocks have a parent, there is no device link in
the game, yet.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +----------------+            +----------------+

Then, a peripheral clock periph0 is probed. His parent is xtal1. The
clock_register_*() call will run __clk_init_parent() and a link between
periph0's core and xtal1's core will be created and stored in
periph0's core->parent_clk_link entry.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +----------------+            +-------^--------+
                                         |
                                         |
                          +--------------+
                          |   ->parent_clk_link
                          |
                  +----------------+
                  |                |
                  |                |
                  |  periph0 core  |
                  |                |
                  |                |
                  +-------^^-------+
                          ||
                          ||
                  +----------------+
                  |                |
                  |  periph0 clk 0 |
                  |                |
                  +----------------+

Then, device0 is probed and "get" the periph0 clock. clk_get() will be
called and a struct clk will be instantiated for device0 (called in
the figure clk 1). A link between device0 and the new clk 1 instance of
periph0 will be created and stored in the clk->consumer_link entry.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +----------------+            +-------^--------+
                                         |
                                         |
                          +--------------+
                          |   ->parent_clk_link
                          |
                  +----------------+
                  |                |
                  |                |
                  |  periph0 core  |
                  |                <-------------+
                  |                <-------------|
                  +-------^^-------+            ||
                          ||                    ||
                          ||                    ||
                  +----------------+    +----------------+
                  |                |    |                |
                  |  periph0 clk 0 |    |  periph0 clk 1 |
                  |                |    |                |
                  +----------------+    +----------------+
                                                |
                                                | ->consumer_link
                                                |
                                                |
                                                |
                                        +-------v--------+
                                        |    device0     |
                                        +----------------+

Right now, device0 is linked to periph0, itself linked to xtal1 so
everything is fine.

Now let's get some fun: the new parent of periph0 is xtal1. The process
will call clk_reparent(), periph0's core->parent_clk_link will be
destroyed and a new link to xtal1 will be setup and stored. The
situation is now that device0 is linked to periph0 and periph0 is
linked to xtal1, so the dependency between device0 and xtal1 is still
clear.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +-------^--------+            +----------------+
           |
           |                           \ /
           +----------------------------x
      ->parent_clk_link   |            / \
                          |
                  +----------------+
                  |                |
                  |                |
                  |  periph0 core  |
                  |                <-------------+
                  |                <-------------|
                  +-------^^-------+            ||
                          ||                    ||
                          ||                    ||
                  +----------------+    +----------------+
                  |                |    |                |
                  |  periph0 clk 0 |    |  periph0 clk 1 |
                  |                |    |                |
                  +----------------+    +----------------+
                                                |
                                                | ->consumer_link
                                                |
                                                |
                                                |
                                        +-------v--------+
                                        |    device0     |
                                        +----------------+

I assume periph0 cannot be removed while there are devices using it,
same for xtal0.

What can happen is that device0 'put' the clock periph0. The relevant
link is deleted and the clk instance dropped.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +-------^--------+            +----------------+
           |
           |                           \ /
           +----------------------------x
      ->parent_clk_link   |            / \
                          |
                  +----------------+
                  |                |
                  |                |
                  |  periph0 core  |
                  |                |
                  |                |
                  +-------^^-------+
                          ||
                          ||
                  +----------------+
                  |                |
                  |  periph0 clk 0 |
                  |                |
                  +----------------+

Now we can unregister periph0: link with the parent will be destroyed
and the clock may be safely removed.

   +----------------+            +----------------+
   |                |            |                |
   |                |            |                |
   |   xtal0 core   |            |   xtal1 core   |
   |                |            |                |
   |                |            |                |
   +-------^^-------+            +-------^^-------+
           ||                            ||
           ||                            ||
   +----------------+            +----------------+
   |                |            |                |
   |   xtal0 clk    |            |   xtal1 clk    |
   |                |            |                |
   +----------------+            +----------------+


This is my understanding of the common clock framework and how links
can be added to it.

As a result, here are the links created during the boot of an
ESPRESSObin:

----->8-----
marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-tbg-clock d0013200.tbg: Dropping the link to d0013800.pinctrl:xtal-clk
marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013200.tbg
marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-periph-clock d0018000.sb-periph-clk: Linked as a consumer to d0013200.tbg
mvneta d0030000.ethernet: Linked as a consumer to d0018000.sb-periph-clk
xhci-hcd d0058000.usb: Linked as a consumer to d0018000.sb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d0000.sdhci: Dropping the link to d0013000.nb-periph-clk
mvebu-uart d0012000.serial: Linked as a consumer to d0013800.pinctrl:xtal-clk
advk-pcie d0070000.pcie: Linked as a consumer to d0018000.sb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to regulator.1
cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk
cpu cpu0: Dropping the link to d0013000.nb-periph-clk
cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk
-----8<-----

Thanks,
Miquèl

Changes since v2:
=================
* Fixed compilation issue when not using the common clock framework:
  removed the static keyword in front of clk_link/unlink_consumer()
  dummy definitions in clkdev.c.

Changes since v1:
=================
* Add clock->clock links, not only device->clock links.
* Helpers renamed:
  > clk_{link,unlink}_hierarchy()
  > clk_{link,unlink}_consumer()
* Add two patches to pass a "struct device" to the clock registration
  helper. This way device links may work between clocks themselves
  (otherwise the link is not created).


Miquel Raynal (4):
  clk: core: clarify the check for runtime PM
  clk: core: link consumer with clock driver
  clk: mvebu: armada-37xx-tbg: fill the device entry when registering
    the clocks
  clk: mvebu: armada-37xx-xtal: fill the device entry when registering
    the clock

 drivers/clk/clk.c                    | 64 ++++++++++++++++++++++++----
 drivers/clk/clkdev.c                 | 16 +++++--
 drivers/clk/mvebu/armada-37xx-tbg.c  |  6 ++-
 drivers/clk/mvebu/armada-37xx-xtal.c |  3 +-
 include/linux/clk-provider.h         |  2 +
 5 files changed, 77 insertions(+), 14 deletions(-)

-- 
2.19.1

[PATCH v3 1/4] clk: core: clarify the check for runtime PM

[Miquel Raynal]

Currently, the core->dev entry is populated only if runtime PM is
enabled. Doing so prevents accessing the device structure in any
case.

Keep the same logic but instead of using the presence of core->dev as
the only condition, also check the status of
pm_runtime_enabled(). Then, we can set the core->dev pointer at any
time as long as a device structure is available.

This change will help supporting device links in the clock subsystem.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/clk.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af011974d4ec..b799347c5fd6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
 {
 	int ret = 0;
 
-	if (!core->dev)
+	if (!core->dev || !pm_runtime_enabled(core->dev))
 		return 0;
 
 	ret = pm_runtime_get_sync(core->dev);
@@ -106,7 +106,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
 
 static void clk_pm_runtime_put(struct clk_core *core)
 {
-	if (!core->dev)
+	if (!core->dev || !pm_runtime_enabled(core->dev))
 		return;
 
 	pm_runtime_put_sync(core->dev);
@@ -226,7 +226,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
 	 * taking enable spinlock, but the below check is needed if one tries
 	 * to call it from other places.
 	 */
-	if (core->dev) {
+	if (core->dev && pm_runtime_enabled(core->dev)) {
 		pm_runtime_get_noresume(core->dev);
 		if (!pm_runtime_active(core->dev)) {
 			ret = false;
@@ -236,7 +236,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
 
 	ret = core->ops->is_enabled(core->hw);
 done:
-	if (core->dev)
+	if (core->dev && pm_runtime_enabled(core->dev))
 		pm_runtime_put(core->dev);
 
 	return ret;
@@ -3272,8 +3272,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	}
 	core->ops = hw->init->ops;
 
-	if (dev && pm_runtime_enabled(dev))
-		core->dev = dev;
+	core->dev = dev;
 	if (dev && dev->driver)
 		core->owner = dev->driver->owner;
 	core->hw = hw;
-- 
2.19.1

Re: [PATCH v3 1/4] clk: core: clarify the check for runtime PM

[Stephen Boyd]

Quoting Miquel Raynal (2018-12-04 11:24:37)
> Currently, the core->dev entry is populated only if runtime PM is
> enabled. Doing so prevents accessing the device structure in any
> case.
> 
> Keep the same logic but instead of using the presence of core->dev as
> the only condition, also check the status of
> pm_runtime_enabled(). Then, we can set the core->dev pointer at any
> time as long as a device structure is available.
> 
> This change will help supporting device links in the clock subsystem.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/clk/clk.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af011974d4ec..b799347c5fd6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
>  {
>         int ret = 0;
>  
> -       if (!core->dev)
> +       if (!core->dev || !pm_runtime_enabled(core->dev))

This looks potentially dangerous. What if runtime PM is disabled for the
clk when this function is called? Shouldn't we just stash a bool away in
the clk_core structure when it's registered? And then we can replace the
check for !core->dev with a check for 'core->rpm_enabled' instead. That
would be a more exact transformation.

Re: [PATCH v3 1/4] clk: core: clarify the check for runtime PM

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29
-0800:

> Quoting Miquel Raynal (2018-12-04 11:24:37)
> > Currently, the core->dev entry is populated only if runtime PM is
> > enabled. Doing so prevents accessing the device structure in any
> > case.
> > 
> > Keep the same logic but instead of using the presence of core->dev as
> > the only condition, also check the status of
> > pm_runtime_enabled(). Then, we can set the core->dev pointer at any
> > time as long as a device structure is available.
> > 
> > This change will help supporting device links in the clock subsystem.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/clk/clk.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af011974d4ec..b799347c5fd6 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
> >  {
> >         int ret = 0;
> >  
> > -       if (!core->dev)
> > +       if (!core->dev || !pm_runtime_enabled(core->dev))  
> 
> This looks potentially dangerous. What if runtime PM is disabled for the
> clk when this function is called? Shouldn't we just stash a bool away in
> the clk_core structure when it's registered? And then we can replace the
> check for !core->dev with a check for 'core->rpm_enabled' instead. That
> would be a more exact transformation.

Sure, I'll do that if you think there is a danger.


Thanks,
Miquèl

Re: [PATCH v3 1/4] clk: core: clarify the check for runtime PM

[Stephen Boyd]

Quoting Miquel Raynal (2018-12-19 00:03:31)
> Hi Stephen,
> 
> Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29
> -0800:
> 
> > Quoting Miquel Raynal (2018-12-04 11:24:37)
> > > Currently, the core->dev entry is populated only if runtime PM is
> > > enabled. Doing so prevents accessing the device structure in any
> > > case.
> > > 
> > > Keep the same logic but instead of using the presence of core->dev as
> > > the only condition, also check the status of
> > > pm_runtime_enabled(). Then, we can set the core->dev pointer at any
> > > time as long as a device structure is available.
> > > 
> > > This change will help supporting device links in the clock subsystem.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/clk/clk.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..b799347c5fd6 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
> > >  {
> > >         int ret = 0;
> > >  
> > > -       if (!core->dev)
> > > +       if (!core->dev || !pm_runtime_enabled(core->dev))  
> > 
> > This looks potentially dangerous. What if runtime PM is disabled for the
> > clk when this function is called? Shouldn't we just stash a bool away in
> > the clk_core structure when it's registered? And then we can replace the
> > check for !core->dev with a check for 'core->rpm_enabled' instead. That
> > would be a more exact transformation.
> 
> Sure, I'll do that if you think there is a danger.

I've made the change and pushed it out to the clk tree. I'm working on
some patches to change how we do parent lookups and reworking clk_get()
in the process. Take a look so we don't duplicate efforts. The code
isn't complete, but I hope to finish soon. Your device links patches can
build upon this.

 git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite

Re: [PATCH v3 1/4] clk: core: clarify the check for runtime PM

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Thu, 20 Dec 2018 13:09:19
-0800:

> Quoting Miquel Raynal (2018-12-19 00:03:31)
> > Hi Stephen,
> > 
> > Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29
> > -0800:
> >   
> > > Quoting Miquel Raynal (2018-12-04 11:24:37)  
> > > > Currently, the core->dev entry is populated only if runtime PM is
> > > > enabled. Doing so prevents accessing the device structure in any
> > > > case.
> > > > 
> > > > Keep the same logic but instead of using the presence of core->dev as
> > > > the only condition, also check the status of
> > > > pm_runtime_enabled(). Then, we can set the core->dev pointer at any
> > > > time as long as a device structure is available.
> > > > 
> > > > This change will help supporting device links in the clock subsystem.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/clk/clk.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index af011974d4ec..b799347c5fd6 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
> > > >  {
> > > >         int ret = 0;
> > > >  
> > > > -       if (!core->dev)
> > > > +       if (!core->dev || !pm_runtime_enabled(core->dev))    
> > > 
> > > This looks potentially dangerous. What if runtime PM is disabled for the
> > > clk when this function is called? Shouldn't we just stash a bool away in
> > > the clk_core structure when it's registered? And then we can replace the
> > > check for !core->dev with a check for 'core->rpm_enabled' instead. That
> > > would be a more exact transformation.  
> > 
> > Sure, I'll do that if you think there is a danger.  
> 
> I've made the change and pushed it out to the clk tree. I'm working on
> some patches to change how we do parent lookups and reworking clk_get()
> in the process. Take a look so we don't duplicate efforts. The code
> isn't complete, but I hope to finish soon. Your device links patches can
> build upon this.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite
> 

Sorry for the delay and thanks for the update. I have been preempted by
another issue, I will switch to clocks and propose something ASAP.


Thanks,
Miquèl

[PATCH v3 2/4] clk: core: link consumer with clock driver

[Miquel Raynal]

One major concern when, for instance, suspending/resuming a platform
is to never access registers before the underlying clock has been
resumed, otherwise most of the time the kernel will just crash. One
solution is to use syscore operations when registering clock drivers
suspend/resume callbacks. One problem of using syscore_ops is that the
suspend/resume scheduling will depend on the order of the
registrations, which brings (unacceptable) randomness in the process.

A feature called device links has been introduced to handle such
situation. It creates dependencies between consumers and providers,
enforcing e.g. the suspend/resume order when needed. Such feature is
already in use for regulators.

Add device links support in the clock subsystem by creating/deleting
the links at get/put time.

Example of a boot (ESPRESSObin, A3700 SoC) with devices linked to clocks:

marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-tbg-clock d0013200.tbg: Dropping the link to d0013800.pinctrl:xtal-clk
marvell-armada-3700-tbg-clock d0013200.tbg: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013200.tbg
marvell-armada-3700-periph-clock d0013000.nb-periph-clk: Linked as a consumer to d0013800.pinctrl:xtal-clk
marvell-armada-3700-periph-clock d0018000.sb-periph-clk: Linked as a consumer to d0013200.tbg
mvneta d0030000.ethernet: Linked as a consumer to d0018000.sb-periph-clk
xhci-hcd d0058000.usb: Linked as a consumer to d0018000.sb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d0000.sdhci: Dropping the link to d0013000.nb-periph-clk
mvebu-uart d0012000.serial: Linked as a consumer to d0013800.pinctrl:xtal-clk
advk-pcie d0070000.pcie: Linked as a consumer to d0018000.sb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d0000.sdhci: Linked as a consumer to regulator.1
cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk
cpu cpu0: Dropping the link to d0013000.nb-periph-clk
cpu cpu0: Linked as a consumer to d0013000.nb-periph-clk

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/clk.c            | 53 ++++++++++++++++++++++++++++++++++--
 drivers/clk/clkdev.c         | 16 +++++++++--
 include/linux/clk-provider.h |  2 ++
 3 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b799347c5fd6..0cc887b43f66 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -77,6 +77,7 @@ struct clk_core {
 	struct hlist_node	debug_node;
 #endif
 	struct kref		ref;
+	struct device_link	*parent_clk_link;
 };
 
 #define CREATE_TRACE_POINTS
@@ -90,6 +91,7 @@ struct clk {
 	unsigned long max_rate;
 	unsigned int exclusive_count;
 	struct hlist_node clks_node;
+	struct device_link *consumer_link;
 };
 
 /***           runtime pm          ***/
@@ -274,6 +276,37 @@ struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw)
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_parent);
 
+void clk_link_consumer(struct device *consumer, struct clk *clk)
+{
+	if (consumer && clk)
+		clk->consumer_link = device_link_add(consumer, clk->core->dev,
+						     DL_FLAG_STATELESS);
+}
+EXPORT_SYMBOL_GPL(clk_link_consumer);
+
+void clk_unlink_consumer(struct clk *clk)
+{
+	if (clk && clk->consumer_link)
+		device_link_del(clk->consumer_link);
+}
+EXPORT_SYMBOL_GPL(clk_unlink_consumer);
+
+static void clk_link_hierarchy(struct clk_core *consumer,
+			       struct clk_core *provider)
+{
+	if (consumer && provider)
+		consumer->parent_clk_link = device_link_add(consumer->dev,
+							    provider->dev,
+							    DL_FLAG_STATELESS);
+}
+
+static void clk_unlink_hierarchy(struct clk_core *consumer,
+				 struct clk_core *provider)
+{
+	if (consumer && provider && consumer->parent_clk_link)
+		device_link_del(consumer->parent_clk_link);
+}
+
 static struct clk_core *__clk_lookup_subtree(const char *name,
 					     struct clk_core *core)
 {
@@ -1542,6 +1575,9 @@ static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
 
 	hlist_del(&core->child_node);
 
+	if (core->parent)
+		clk_unlink_hierarchy(core, core->parent);
+
 	if (new_parent) {
 		bool becomes_orphan = new_parent->orphan;
 
@@ -1553,6 +1589,8 @@ static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
 
 		if (was_orphan != becomes_orphan)
 			clk_core_update_orphan_status(core, becomes_orphan);
+
+		clk_link_hierarchy(core, new_parent);
 	} else {
 		hlist_add_head(&core->child_node, &clk_orphan_list);
 		if (!was_orphan)
@@ -2244,12 +2282,16 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
 
 static struct clk_core *__clk_init_parent(struct clk_core *core)
 {
+	struct clk_core *parent;
 	u8 index = 0;
 
 	if (core->num_parents > 1 && core->ops->get_parent)
 		index = core->ops->get_parent(core->hw);
 
-	return clk_core_get_parent_by_index(core, index);
+	parent = clk_core_get_parent_by_index(core, index);
+	clk_link_hierarchy(core, parent);
+
+	return parent;
 }
 
 static void clk_core_reparent(struct clk_core *core,
@@ -3437,11 +3479,18 @@ void clk_unregister(struct clk *clk)
 	clk->core->ops = &clk_nodrv_ops;
 	clk_enable_unlock(flags);
 
+	clk_unlink_hierarchy(clk->core, clk->core->parent);
+
 	if (!hlist_empty(&clk->core->children)) {
 		struct clk_core *child;
 		struct hlist_node *t;
 
-		/* Reparent all children to the orphan list. */
+		/*
+		 * Reparent all children to the orphan list.
+		 *
+		 * No need to unlink the child clock manually, this will be
+		 * handled by clk_reparent().
+		 */
 		hlist_for_each_entry_safe(child, t, &clk->core->children,
 					  child_node)
 			clk_core_set_parent_nolock(child, NULL);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..8c6e087437bf 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -112,6 +112,9 @@ EXPORT_SYMBOL(of_clk_get_by_name);
 
 #else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
 
+void clk_link_consumer(struct device *consumer, struct clk *clk) {}
+void clk_unlink_consumer(struct clk *clk) {}
+
 static struct clk *__of_clk_get_by_name(struct device_node *np,
 					const char *dev_id,
 					const char *name)
@@ -194,20 +197,27 @@ EXPORT_SYMBOL(clk_get_sys);
 struct clk *clk_get(struct device *dev, const char *con_id)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
-	struct clk *clk;
+	struct clk *clk = NULL;
 
 	if (dev && dev->of_node) {
 		clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
-		if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
+		if (PTR_ERR(clk) == -EPROBE_DEFER)
 			return clk;
 	}
 
-	return clk_get_sys(dev_id, con_id);
+	if (IS_ERR_OR_NULL(clk))
+		clk = clk_get_sys(dev_id, con_id);
+
+	if (!IS_ERR(clk))
+		clk_link_consumer(dev, clk);
+
+	return clk;
 }
 EXPORT_SYMBOL(clk_get);
 
 void clk_put(struct clk *clk)
 {
+	clk_unlink_consumer(clk);
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 60c51871b04b..721d6b55b2fa 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
 const char *__clk_get_name(const struct clk *clk);
 const char *clk_hw_get_name(const struct clk_hw *hw);
 struct clk_hw *__clk_get_hw(struct clk *clk);
+void clk_link_consumer(struct device *consumer, struct clk *clk);
+void clk_unlink_consumer(struct clk *clk);
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
-- 
2.19.1

Re: [PATCH v3 2/4] clk: core: link consumer with clock driver

[Stephen Boyd]

Sorry, I'm not reviewing the whole patch right now, just this one little
bit because I'm also working in the same area.

Quoting Miquel Raynal (2018-12-04 11:24:38)
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 60c51871b04b..721d6b55b2fa 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
>  const char *__clk_get_name(const struct clk *clk);
>  const char *clk_hw_get_name(const struct clk_hw *hw);
>  struct clk_hw *__clk_get_hw(struct clk *clk);
> +void clk_link_consumer(struct device *consumer, struct clk *clk);
> +void clk_unlink_consumer(struct clk *clk);

We shouldn't need to add these functions as far as I can tell. That's
because __clk_get() has become an internal API between clkdev.c and
clk.c that does nothing now on implementations that aren't the CCF. We
can even change this API to take a clk_hw pointer instead of a clk
pointer.

I'd rather see us plumb a struct device and clk_hw structure down into
__clk_get() and fold it all into __clk_create_clk, possibly even
renaming __clk_create_clk to clk_hw_create_clk(). That way we can get
the calling device and clk_hw pointer in one call in the clk framework,
along with the device name and connection name, and then generate the
struct clk right there. This can simplify some code and make it easier
to extend this to associate calling devices with the clk consumer
somehow.

Here's the diff. With this, you should be able to add and remove device
links in clk_hw_create_clk() when dev != NULL.

----8<----

 drivers/clk/clk.c    | 57 +++++++++++++++++++++++-----------------------------
 drivers/clk/clk.h    | 12 +++++------
 drivers/clk/clkdev.c | 43 ++++++++++++++++++++-------------------
 3 files changed, 53 insertions(+), 59 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af011974d4ec..684f9a4b7d65 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3202,10 +3202,11 @@ static int __clk_core_init(struct clk_core *core)
 	return ret;
 }
 
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id)
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
+			      const char *dev_id, const char *con_id)
 {
 	struct clk *clk;
+	struct clk_core *core;
 
 	/* This is to allow this function to be chained to others */
 	if (IS_ERR_OR_NULL(hw))
@@ -3215,15 +3216,23 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	if (!clk)
 		return ERR_PTR(-ENOMEM);
 
-	clk->core = hw->core;
+	core = hw->core;
+	clk->core = core;
 	clk->dev_id = dev_id;
 	clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
 	clk->max_rate = ULONG_MAX;
 
 	clk_prepare_lock();
-	hlist_add_head(&clk->clks_node, &hw->core->clks);
+	hlist_add_head(&clk->clks_node, &core->clks);
 	clk_prepare_unlock();
 
+	if (!try_module_get(core->owner)) {
+		kfree(clk);
+		return ERR_PTR(-ENOENT);
+	}
+
+	kref_get(&core->ref);
+
 	return clk;
 }
 
@@ -3313,7 +3322,11 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 	INIT_HLIST_HEAD(&core->clks);
 
-	hw->clk = __clk_create_clk(hw, NULL, NULL);
+	/*
+	 * Pass NULL here for device because we eventually plan
+	 * to get rid of this clk created here
+	 */
+	hw->clk = clk_hw_create_clk(NULL, hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
 		goto fail_parents;
@@ -3594,18 +3607,6 @@ EXPORT_SYMBOL_GPL(devm_clk_hw_unregister);
 /*
  * clkdev helpers
  */
-int __clk_get(struct clk *clk)
-{
-	struct clk_core *core = !clk ? NULL : clk->core;
-
-	if (core) {
-		if (!try_module_get(core->owner))
-			return 0;
-
-		kref_get(&core->ref);
-	}
-	return 1;
-}
 
 /* keep in sync with __clk_free_clk */
 void __clk_put(struct clk *clk)
@@ -3976,12 +3977,12 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
 	return __clk_get_hw(clk);
 }
 
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
+struct clk *__of_clk_get_from_provider(struct device *dev,
+				       struct of_phandle_args *clkspec,
 				       const char *dev_id, const char *con_id)
 {
 	struct of_clk_provider *provider;
-	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
-	struct clk_hw *hw;
+	struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
 
 	if (!clkspec)
 		return ERR_PTR(-EINVAL);
@@ -3991,21 +3992,13 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 	list_for_each_entry(provider, &of_clk_providers, link) {
 		if (provider->node == clkspec->np) {
 			hw = __of_clk_get_hw_from_provider(provider, clkspec);
-			clk = __clk_create_clk(hw, dev_id, con_id);
-		}
-
-		if (!IS_ERR(clk)) {
-			if (!__clk_get(clk)) {
-				__clk_free_clk(clk);
-				clk = ERR_PTR(-ENOENT);
-			}
-
-			break;
+			if (!IS_ERR(hw))
+				break;
 		}
 	}
 	mutex_unlock(&of_clk_mutex);
 
-	return clk;
+	return clk_hw_create_clk(dev, hw, dev_id, con_id);
 }
 
 /**
@@ -4018,7 +4011,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
  */
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
-	return __of_clk_get_from_provider(clkspec, NULL, __func__);
+	return __of_clk_get_from_provider(NULL, clkspec, NULL, __func__);
 }
 EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
 
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 70c0ba6336c1..a0d4aa0fcfcd 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -12,20 +12,21 @@
 struct clk_hw;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
+struct clk *__of_clk_get_from_provider(struct device *dev,
+				       struct of_phandle_args *clkspec,
 				       const char *dev_id, const char *con_id);
 #endif
 
 #ifdef CONFIG_COMMON_CLK
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id);
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
+			      const char *dev_id, const char *con_id);
 void __clk_free_clk(struct clk *clk);
-int __clk_get(struct clk *clk);
 void __clk_put(struct clk *clk);
 #else
 /* All these casts to avoid ifdefs in clkdev... */
 static inline struct clk *
-__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
+		  const char *con_id)
 {
 	return (struct clk *)hw;
 }
@@ -34,7 +35,6 @@ static struct clk_hw *__clk_get_hw(struct clk *clk)
 {
 	return (struct clk_hw *)clk;
 }
-static inline int __clk_get(struct clk *clk) { return 1; }
 static inline void __clk_put(struct clk *clk) { }
 
 #endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..08cf01c168fb 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -28,8 +28,9 @@ static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-static struct clk *__of_clk_get(struct device_node *np, int index,
-			       const char *dev_id, const char *con_id)
+static struct clk *__of_clk_get(struct device *dev, struct device_node *np,
+				int index, const char *dev_id,
+				const char *con_id)
 {
 	struct of_phandle_args clkspec;
 	struct clk *clk;
@@ -40,7 +41,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index,
 	if (rc)
 		return ERR_PTR(rc);
 
-	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
+	clk = __of_clk_get_from_provider(dev, &clkspec, dev_id, con_id);
 	of_node_put(clkspec.np);
 
 	return clk;
@@ -48,13 +49,13 @@ static struct clk *__of_clk_get(struct device_node *np, int index,
 
 struct clk *of_clk_get(struct device_node *np, int index)
 {
-	return __of_clk_get(np, index, np->full_name, NULL);
+	return __of_clk_get(NULL, np, index, np->full_name, NULL);
 }
 EXPORT_SYMBOL(of_clk_get);
 
-static struct clk *__of_clk_get_by_name(struct device_node *np,
-					const char *dev_id,
-					const char *name)
+static struct clk *__of_clk_get_by_name(struct device *dev,
+					struct device_node *np,
+					const char *dev_id, const char *name)
 {
 	struct clk *clk = ERR_PTR(-ENOENT);
 
@@ -69,7 +70,7 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
 		 */
 		if (name)
 			index = of_property_match_string(np, "clock-names", name);
-		clk = __of_clk_get(np, index, dev_id, name);
+		clk = __of_clk_get(dev, np, index, dev_id, name);
 		if (!IS_ERR(clk)) {
 			break;
 		} else if (name && index >= 0) {
@@ -106,13 +107,14 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 	if (!np)
 		return ERR_PTR(-ENOENT);
 
-	return __of_clk_get_by_name(np, np->full_name, name);
+	return __of_clk_get_by_name(NULL, np, np->full_name, name);
 }
 EXPORT_SYMBOL(of_clk_get_by_name);
 
 #else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
 
-static struct clk *__of_clk_get_by_name(struct device_node *np,
+static struct clk *__of_clk_get_by_name(struct device *dev,
+					struct device_node *np,
 					const char *dev_id,
 					const char *name)
 {
@@ -163,7 +165,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 	return cl;
 }
 
-struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
+				 const char *con_id)
 {
 	struct clk_lookup *cl;
 	struct clk *clk = NULL;
@@ -174,21 +177,19 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 	if (!cl)
 		goto out;
 
-	clk = __clk_create_clk(cl->clk_hw, dev_id, con_id);
+	clk = clk_hw_create_clk(dev, cl->clk_hw, dev_id, con_id);
 	if (IS_ERR(clk))
-		goto out;
-
-	if (!__clk_get(clk)) {
-		__clk_free_clk(clk);
 		cl = NULL;
-		goto out;
-	}
-
 out:
 	mutex_unlock(&clocks_mutex);
 
 	return cl ? clk : ERR_PTR(-ENOENT);
 }
+
+struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+{
+	return __clk_get_sys(NULL, dev_id, con_id);
+}
 EXPORT_SYMBOL(clk_get_sys);
 
 struct clk *clk_get(struct device *dev, const char *con_id)
@@ -197,12 +198,12 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 	struct clk *clk;
 
 	if (dev && dev->of_node) {
-		clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
+		clk = __of_clk_get_by_name(dev, dev->of_node, dev_id, con_id);
 		if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
 			return clk;
 	}
 
-	return clk_get_sys(dev_id, con_id);
+	return __clk_get_sys(dev, dev_id, con_id);
 }
 EXPORT_SYMBOL(clk_get);
 

Re: [PATCH v3 2/4] clk: core: link consumer with clock driver

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Tue, 11 Dec 2018 09:12:55
-0800:

> Sorry, I'm not reviewing the whole patch right now, just this one little
> bit because I'm also working in the same area.
> 
> Quoting Miquel Raynal (2018-12-04 11:24:38)
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 60c51871b04b..721d6b55b2fa 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
> >  const char *__clk_get_name(const struct clk *clk);
> >  const char *clk_hw_get_name(const struct clk_hw *hw);
> >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > +void clk_link_consumer(struct device *consumer, struct clk *clk);
> > +void clk_unlink_consumer(struct clk *clk);  
> 
> We shouldn't need to add these functions as far as I can tell. That's
> because __clk_get() has become an internal API between clkdev.c and
> clk.c that does nothing now on implementations that aren't the CCF. We
> can even change this API to take a clk_hw pointer instead of a clk
> pointer.
> 
> I'd rather see us plumb a struct device and clk_hw structure down into
> __clk_get() and fold it all into __clk_create_clk, possibly even
> renaming __clk_create_clk to clk_hw_create_clk(). That way we can get
> the calling device and clk_hw pointer in one call in the clk framework,
> along with the device name and connection name, and then generate the
> struct clk right there. This can simplify some code and make it easier
> to extend this to associate calling devices with the clk consumer
> somehow.
> 
> Here's the diff. With this, you should be able to add and remove device
> links in clk_hw_create_clk() when dev != NULL.

Thanks for the pointers, if you think we should modify the API then
it's fine by me, I will integrate your diff and try to propose
something in sync with your comments this week.

Thanks,
Miquèl

Re: [PATCH v3 2/4] clk: core: link consumer with clock driver

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Tue, 11 Dec 2018 09:12:55
-0800:

> Sorry, I'm not reviewing the whole patch right now, just this one little
> bit because I'm also working in the same area.
> 
> Quoting Miquel Raynal (2018-12-04 11:24:38)
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 60c51871b04b..721d6b55b2fa 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
> >  const char *__clk_get_name(const struct clk *clk);
> >  const char *clk_hw_get_name(const struct clk_hw *hw);
> >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > +void clk_link_consumer(struct device *consumer, struct clk *clk);
> > +void clk_unlink_consumer(struct clk *clk);  
> 
> We shouldn't need to add these functions as far as I can tell. That's
> because __clk_get() has become an internal API between clkdev.c and
> clk.c that does nothing now on implementations that aren't the CCF. We
> can even change this API to take a clk_hw pointer instead of a clk
> pointer.
> 
> I'd rather see us plumb a struct device and clk_hw structure down into
> __clk_get() and fold it all into __clk_create_clk, possibly even
> renaming __clk_create_clk to clk_hw_create_clk(). That way we can get
> the calling device and clk_hw pointer in one call in the clk framework,
> along with the device name and connection name, and then generate the
> struct clk right there. This can simplify some code and make it easier
> to extend this to associate calling devices with the clk consumer
> somehow.
> 
> Here's the diff. With this, you should be able to add and remove device
> links in clk_hw_create_clk() when dev != NULL.

Thanks for the help; I updated my work on top of yours, it looks ok but
I need to run some more tests.

However I had to tweak a parameter in one of your recent changes, you
used '-1' as index in __of_clock_get() while it is not a valid value
(returning an error). As in the __of_clk_get_by_name() function you
removed index was just set to 0 at the top of the function, I think the
below fix is valid.


Thanks,
Miquèl


---

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index ba4d1e06732d..f2f4f2afd28c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -163,7 +163,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
        struct clk_hw *hw;
 
        if (dev && dev->of_node) {
-               hw = of_clk_get_hw(dev->of_node, -1, con_id);
+               hw = of_clk_get_hw(dev->of_node, 0, con_id);
                if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
                        return clk_hw_create_clk(dev, hw, dev_id, con_id);
        }

Re: [PATCH v3 2/4] clk: core: link consumer with clock driver

[Stephen Boyd]

Quoting Miquel Raynal (2019-01-04 07:54:06)
> Hi Stephen,
> 
> Stephen Boyd <sboyd@kernel.org> wrote on Tue, 11 Dec 2018 09:12:55
> -0800:
> 
> > Sorry, I'm not reviewing the whole patch right now, just this one little
> > bit because I'm also working in the same area.
> > 
> > Quoting Miquel Raynal (2018-12-04 11:24:38)
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index 60c51871b04b..721d6b55b2fa 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
> > >  const char *__clk_get_name(const struct clk *clk);
> > >  const char *clk_hw_get_name(const struct clk_hw *hw);
> > >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > > +void clk_link_consumer(struct device *consumer, struct clk *clk);
> > > +void clk_unlink_consumer(struct clk *clk);  
> > 
> > We shouldn't need to add these functions as far as I can tell. That's
> > because __clk_get() has become an internal API between clkdev.c and
> > clk.c that does nothing now on implementations that aren't the CCF. We
> > can even change this API to take a clk_hw pointer instead of a clk
> > pointer.
> > 
> > I'd rather see us plumb a struct device and clk_hw structure down into
> > __clk_get() and fold it all into __clk_create_clk, possibly even
> > renaming __clk_create_clk to clk_hw_create_clk(). That way we can get
> > the calling device and clk_hw pointer in one call in the clk framework,
> > along with the device name and connection name, and then generate the
> > struct clk right there. This can simplify some code and make it easier
> > to extend this to associate calling devices with the clk consumer
> > somehow.
> > 
> > Here's the diff. With this, you should be able to add and remove device
> > links in clk_hw_create_clk() when dev != NULL.
> 
> Thanks for the help; I updated my work on top of yours, it looks ok but
> I need to run some more tests.
> 
> However I had to tweak a parameter in one of your recent changes, you
> used '-1' as index in __of_clock_get() while it is not a valid value
> (returning an error). As in the __of_clk_get_by_name() function you
> removed index was just set to 0 at the top of the function, I think the
> below fix is valid.

Thanks. Makes sense so I folded it in.

Re: [PATCH v3 2/4] clk: core: link consumer with clock driver

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Fri, 25 Jan 2019 13:28:28
-0800:

> Quoting Miquel Raynal (2019-01-04 07:54:06)
> > Hi Stephen,
> > 
> > Stephen Boyd <sboyd@kernel.org> wrote on Tue, 11 Dec 2018 09:12:55
> > -0800:
> >   
> > > Sorry, I'm not reviewing the whole patch right now, just this one little
> > > bit because I'm also working in the same area.
> > > 
> > > Quoting Miquel Raynal (2018-12-04 11:24:38)  
> > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > index 60c51871b04b..721d6b55b2fa 100644
> > > > --- a/include/linux/clk-provider.h
> > > > +++ b/include/linux/clk-provider.h
> > > > @@ -781,6 +781,8 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw);
> > > >  const char *__clk_get_name(const struct clk *clk);
> > > >  const char *clk_hw_get_name(const struct clk_hw *hw);
> > > >  struct clk_hw *__clk_get_hw(struct clk *clk);
> > > > +void clk_link_consumer(struct device *consumer, struct clk *clk);
> > > > +void clk_unlink_consumer(struct clk *clk);    
> > > 
> > > We shouldn't need to add these functions as far as I can tell. That's
> > > because __clk_get() has become an internal API between clkdev.c and
> > > clk.c that does nothing now on implementations that aren't the CCF. We
> > > can even change this API to take a clk_hw pointer instead of a clk
> > > pointer.
> > > 
> > > I'd rather see us plumb a struct device and clk_hw structure down into
> > > __clk_get() and fold it all into __clk_create_clk, possibly even
> > > renaming __clk_create_clk to clk_hw_create_clk(). That way we can get
> > > the calling device and clk_hw pointer in one call in the clk framework,
> > > along with the device name and connection name, and then generate the
> > > struct clk right there. This can simplify some code and make it easier
> > > to extend this to associate calling devices with the clk consumer
> > > somehow.
> > > 
> > > Here's the diff. With this, you should be able to add and remove device
> > > links in clk_hw_create_clk() when dev != NULL.  
> > 
> > Thanks for the help; I updated my work on top of yours, it looks ok but
> > I need to run some more tests.
> > 
> > However I had to tweak a parameter in one of your recent changes, you
> > used '-1' as index in __of_clock_get() while it is not a valid value
> > (returning an error). As in the __of_clk_get_by_name() function you
> > removed index was just set to 0 at the top of the function, I think the
> > below fix is valid.  
> 
> Thanks. Makes sense so I folded it in.

Great!

Can you update me with the status of this series? Is it going in for 5.1?

Lorenzo, the PCI maintainer, is waiting for this series to enter before
accepting another series depending on it; so please do not hesitate to
keep me updated.


Thanks,
Miquèl

[PATCH v3 3/4] clk: mvebu: armada-37xx-tbg: fill the device entry when registering the clocks

[Miquel Raynal]

So far the clk_hw_register_fixed_factor() calls are not providing any
device structure. While doing so is harmless for regular use, the
missing device structure may be a problem for suspend to RAM support.

Since, device links have been added to clocks, links created during
probe will enforce the suspend/resume orders. When the device is
missing during the registration, no link can be established, hence the
order between parent and child clocks are not enforced.

Adding the device structure here will create a link between the 4 TBG
clocks (registered by this driver) and:
* their parent clock: XTAL,
* their child clocks: several 'periph' clock.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-tbg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-tbg.c b/drivers/clk/mvebu/armada-37xx-tbg.c
index ee272d4d8c24..78de057a2406 100644
--- a/drivers/clk/mvebu/armada-37xx-tbg.c
+++ b/drivers/clk/mvebu/armada-37xx-tbg.c
@@ -116,8 +116,10 @@ static int armada_3700_tbg_clock_probe(struct platform_device *pdev)
 		name = tbg[i].name;
 		mult = tbg_get_mult(reg, &tbg[i]);
 		div = tbg_get_div(reg, &tbg[i]);
-		hw_tbg_data->hws[i] = clk_hw_register_fixed_factor(NULL, name,
-						parent_name, 0, mult, div);
+		hw_tbg_data->hws[i] = clk_hw_register_fixed_factor(dev, name,
+								   parent_name,
+								   0, mult,
+								   div);
 		if (IS_ERR(hw_tbg_data->hws[i]))
 			dev_err(dev, "Can't register TBG clock %s\n", name);
 	}
-- 
2.19.1

[PATCH v3 4/4] clk: mvebu: armada-37xx-xtal: fill the device entry when registering the clock

[Miquel Raynal]

So far the clk_hw_register_fixed_factor() call was not providing any
device structure. While doing so is harmless for regular use, the
missing device structure may be a problem for suspend to RAM support.

Since, device links have been added to clocks, links created during
probe will enforce the suspend/resume orders. When the device is
missing during the registration, no link can be established, hence the
order between parent and child clocks are not enforced.

Adding the device structure here will create a link between the XTAL
clock (this one) and the four TBG clocks that are derived from it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-xtal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/armada-37xx-xtal.c b/drivers/clk/mvebu/armada-37xx-xtal.c
index e9e306d4e9af..0e74bcd83d1a 100644
--- a/drivers/clk/mvebu/armada-37xx-xtal.c
+++ b/drivers/clk/mvebu/armada-37xx-xtal.c
@@ -57,7 +57,8 @@ static int armada_3700_xtal_clock_probe(struct platform_device *pdev)
 		rate = 25000000;
 
 	of_property_read_string_index(np, "clock-output-names", 0, &xtal_name);
-	xtal_hw = clk_hw_register_fixed_rate(NULL, xtal_name, NULL, 0, rate);
+	xtal_hw = clk_hw_register_fixed_rate(&pdev->dev, xtal_name, NULL, 0,
+					     rate);
 	if (IS_ERR(xtal_hw))
 		return PTR_ERR(xtal_hw);
 	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, xtal_hw);
-- 
2.19.1