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

2018-12-03 Thread Stephen Boyd
Quoting Miquel Raynal (2018-12-03 14:16:55)
> Stephen Boyd  wrote on Mon, 03 Dec 2018 11:20:31
> -0800:
> > Quoting Miquel Raynal (2018-11-30 02:20:52)
> > > Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
> > > -0800:
> > > > Quoting Miquel Raynal (2018-11-23 01:11:32)  
> > > 
> > > Because we need the caller's 'struct device' to make the link and
> > > this is not available in __clk_get(). I tried to ad it as parameter but
> > > I don't think it is possible to retrieve a 'struct device' from the
> > > device name. The functions where this is problematic are:
> > > * clk.c:__of_clk_get_from_provider()
> > > * clkdev.c:clk_get_sys()
> > > 
> > > By the way in my new version I called the helpers:
> > > * clk_{link,unlink}_hierarchy()
> > > * clk_{link,unlink}_consumer()
> > > 
> > > I will send a new version with these helpers, but if you have anything
> > > in mind to help me achieve the above request, I will welcome the idea.
> > >   
> > 
> > We can do the linking in __clk_get() and __clk_put() if we poke into the
> > struct clk -> struct clk_core and bury the struct device into each
> > clk_core structure.
> > 
> 
> I meant the consumer device's structure. Yes, from a struct clk, the
> first change in patch 1/2 let's us do clk->core->dev to get the clock
> device. But for linking I need both the clock device and the consumer
> device; and the latter will be missing in __clk_get().
> 

Ah ok, sounds like this is how it has to be then.



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

2018-12-03 Thread Miquel Raynal
Hi Stephen,

Stephen Boyd  wrote on Mon, 03 Dec 2018 11:20:31
-0800:

> Quoting Miquel Raynal (2018-11-30 02:20:52)
> > Hi Stephen,
> > 
> > Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
> > -0800:
> >   
> > > Quoting Miquel Raynal (2018-11-23 01:11:32)  
> > > > Would you agree with me adding dummy functions in the #else section
> > > > like:
> > > > 
> > > > static inline void __clk_device_link(struct device *consumer, struct 
> > > > clk *clk)
> > > > {
> > > >return;
> > > > }
> > > > 
> > > > static inline void __clk_device_unlink(struct clk *clk)
> > > > {
> > > >return;
> > > > }
> > > > 
> > > > Do you want me to also declare these functions in the #if section
> > > > (with the external keyword) to balance the above declarations?
> > > 
> > > Why can't we do the linking in __clk_get() and __clk_put()?
> > >   
> > 
> > Because we need the caller's 'struct device' to make the link and
> > this is not available in __clk_get(). I tried to ad it as parameter but
> > I don't think it is possible to retrieve a 'struct device' from the
> > device name. The functions where this is problematic are:
> > * clk.c:__of_clk_get_from_provider()
> > * clkdev.c:clk_get_sys()
> > 
> > By the way in my new version I called the helpers:
> > * clk_{link,unlink}_hierarchy()
> > * clk_{link,unlink}_consumer()
> > 
> > I will send a new version with these helpers, but if you have anything
> > in mind to help me achieve the above request, I will welcome the idea.
> >   
> 
> We can do the linking in __clk_get() and __clk_put() if we poke into the
> struct clk -> struct clk_core and bury the struct device into each
> clk_core structure.
> 

I meant the consumer device's structure. Yes, from a struct clk, the
first change in patch 1/2 let's us do clk->core->dev to get the clock
device. But for linking I need both the clock device and the consumer
device; and the latter will be missing in __clk_get().


Thanks,
Miquèl


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

2018-12-03 Thread Stephen Boyd
Quoting Miquel Raynal (2018-11-30 02:20:52)
> Hi Stephen,
> 
> Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
> -0800:
> 
> > Quoting Miquel Raynal (2018-11-23 01:11:32)
> > > Would you agree with me adding dummy functions in the #else section
> > > like:
> > > 
> > > static inline void __clk_device_link(struct device *consumer, struct clk 
> > > *clk)
> > > {
> > >return;
> > > }
> > > 
> > > static inline void __clk_device_unlink(struct clk *clk)
> > > {
> > >return;
> > > }
> > > 
> > > Do you want me to also declare these functions in the #if section
> > > (with the external keyword) to balance the above declarations?  
> > 
> > Why can't we do the linking in __clk_get() and __clk_put()?
> > 
> 
> Because we need the caller's 'struct device' to make the link and
> this is not available in __clk_get(). I tried to ad it as parameter but
> I don't think it is possible to retrieve a 'struct device' from the
> device name. The functions where this is problematic are:
> * clk.c:__of_clk_get_from_provider()
> * clkdev.c:clk_get_sys()
> 
> By the way in my new version I called the helpers:
> * clk_{link,unlink}_hierarchy()
> * clk_{link,unlink}_consumer()
> 
> I will send a new version with these helpers, but if you have anything
> in mind to help me achieve the above request, I will welcome the idea.
> 

We can do the linking in __clk_get() and __clk_put() if we poke into the
struct clk -> struct clk_core and bury the struct device into each
clk_core structure.



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

2018-11-30 Thread Miquel Raynal
Hi Stephen,

Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
-0800:

> Quoting Miquel Raynal (2018-11-23 01:11:32)
> > Would you agree with me adding dummy functions in the #else section
> > like:
> > 
> > static inline void __clk_device_link(struct device *consumer, struct clk 
> > *clk)
> > {
> >return;
> > }
> > 
> > static inline void __clk_device_unlink(struct clk *clk)
> > {
> >return;
> > }
> > 
> > Do you want me to also declare these functions in the #if section
> > (with the external keyword) to balance the above declarations?  
> 
> Why can't we do the linking in __clk_get() and __clk_put()?
> 

Because we need the caller's 'struct device' to make the link and
this is not available in __clk_get(). I tried to ad it as parameter but
I don't think it is possible to retrieve a 'struct device' from the
device name. The functions where this is problematic are:
* clk.c:__of_clk_get_from_provider()
* clkdev.c:clk_get_sys()

By the way in my new version I called the helpers:
* clk_{link,unlink}_hierarchy()
* clk_{link,unlink}_consumer()

I will send a new version with these helpers, but if you have anything
in mind to help me achieve the above request, I will welcome the idea.


Thanks,
Miquèl


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

2018-11-30 Thread Stephen Boyd
Quoting Miquel Raynal (2018-11-23 01:11:32)
> Would you agree with me adding dummy functions in the #else section
> like:
> 
> static inline void __clk_device_link(struct device *consumer, struct clk *clk)
> {
>return;
> }
> 
> static inline void __clk_device_unlink(struct clk *clk)
> {
>return;
> }
> 
> Do you want me to also declare these functions in the #if section
> (with the external keyword) to balance the above declarations?

Why can't we do the linking in __clk_get() and __clk_put()?



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

2018-11-29 Thread Miquel Raynal
Hi Maxime,

Maxime Ripard  wrote on Tue, 27 Nov 2018
13:38:58 +0100:

> Hi Miquel,
> 
> On Thu, Nov 22, 2018 at 10:22:12PM +0100, Miquel Raynal wrote:
> > 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:
> > ahci-mvebu d00e.sata: Linked as a consumer to d0013000.nb-periph-clk
> > mvneta d003.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 d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
> > xenon-sdhci d00d.sdhci: Dropping the link to d0013000.nb-periph-clk
> > advk-pcie d007.pcie: Linked as a consumer to d0018000.sb-periph-clk
> > xenon-sdhci d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
> > xenon-sdhci d00d.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 
> > ---
> >  drivers/clk/clk.c| 20 
> >  drivers/clk/clkdev.c | 13 ++---
> >  include/linux/clk-provider.h |  2 ++
> >  3 files changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index b799347c5fd6..33a0f2b0533a 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -90,6 +90,7 @@ struct clk {
> > unsigned long max_rate;
> > unsigned int exclusive_count;
> > struct hlist_node clks_node;
> > +   struct device_link *link;
> >  };
> >  
> >  /***   runtime pm  ***/
> > @@ -262,6 +263,25 @@ struct clk_hw *__clk_get_hw(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(__clk_get_hw);
> >  
> > +void __clk_device_link(struct device *consumer, struct clk *clk)
> > +{
> > +   if (!consumer || !clk || !clk->core)
> > +   return;
> > +
> > +   clk->link = device_link_add(consumer, clk->core->dev,
> > +   DL_FLAG_STATELESS);
> > +}
> > +EXPORT_SYMBOL_GPL(__clk_device_link);
> > +
> > +void __clk_device_unlink(struct clk *clk)
> > +{
> > +   if (!clk || !clk->link)
> > +   return;
> > +
> > +   device_link_del(clk->link);
> > +}
> > +EXPORT_SYMBOL_GPL(__clk_device_unlink);
> > +
> >  unsigned int clk_hw_get_num_parents(const struct clk_hw *hw)
> >  {
> > return hw->core->num_parents;
> > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> > index 9ab3db8b3988..fccfd4c01457 100644
> > --- a/drivers/clk/clkdev.c
> > +++ b/drivers/clk/clkdev.c
> > @@ -194,20 +194,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_device_link(dev, clk);
> > +
> > +   return clk;  
> 
> I think this doesn't address all the cases. In your case, where you
> have one consumer that is not a clock, and one provider that is a
> clock, it works just fine.
> 
> However, if you have clocks providers chained, for example with one
> oscillator, a clock controller, and a device, the link will be created
> between the device and the controller, but there will be no link
> between the controller and the oscillator.

Indeed, there is not clock_get() in this case so you are right, a link
is missing if we want to track dependencies between clocks.

> 
> Adding a link in __clk_init_parent looks like it would address that
> case.

I will add one, thanks for the pointer.

> 
> Maxime
> 

I think device links must be managed in the following situations:
* A de

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

2018-11-27 Thread Maxime Ripard
Hi Miquel,

On Thu, Nov 22, 2018 at 10:22:12PM +0100, Miquel Raynal wrote:
> 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:
> ahci-mvebu d00e.sata: Linked as a consumer to d0013000.nb-periph-clk
> mvneta d003.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 d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
> xenon-sdhci d00d.sdhci: Dropping the link to d0013000.nb-periph-clk
> advk-pcie d007.pcie: Linked as a consumer to d0018000.sb-periph-clk
> xenon-sdhci d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
> xenon-sdhci d00d.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 
> ---
>  drivers/clk/clk.c| 20 
>  drivers/clk/clkdev.c | 13 ++---
>  include/linux/clk-provider.h |  2 ++
>  3 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b799347c5fd6..33a0f2b0533a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -90,6 +90,7 @@ struct clk {
>   unsigned long max_rate;
>   unsigned int exclusive_count;
>   struct hlist_node clks_node;
> + struct device_link *link;
>  };
>  
>  /***   runtime pm  ***/
> @@ -262,6 +263,25 @@ struct clk_hw *__clk_get_hw(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(__clk_get_hw);
>  
> +void __clk_device_link(struct device *consumer, struct clk *clk)
> +{
> + if (!consumer || !clk || !clk->core)
> + return;
> +
> + clk->link = device_link_add(consumer, clk->core->dev,
> + DL_FLAG_STATELESS);
> +}
> +EXPORT_SYMBOL_GPL(__clk_device_link);
> +
> +void __clk_device_unlink(struct clk *clk)
> +{
> + if (!clk || !clk->link)
> + return;
> +
> + device_link_del(clk->link);
> +}
> +EXPORT_SYMBOL_GPL(__clk_device_unlink);
> +
>  unsigned int clk_hw_get_num_parents(const struct clk_hw *hw)
>  {
>   return hw->core->num_parents;
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 9ab3db8b3988..fccfd4c01457 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -194,20 +194,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_device_link(dev, clk);
> +
> + return clk;

I think this doesn't address all the cases. In your case, where you
have one consumer that is not a clock, and one provider that is a
clock, it works just fine.

However, if you have clocks providers chained, for example with one
oscillator, a clock controller, and a device, the link will be created
between the device and the controller, but there will be no link
between the controller and the oscillator.

Adding a link in __clk_init_parent looks like it would address that
case.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature


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

2018-11-23 Thread Miquel Raynal
Hello,

kbuild test robot  wrote on Fri, 23 Nov 2018 16:30:00
+0800:

> Hi Miquel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on clk/clk-next]
> [also build test ERROR on v4.20-rc3 next-20181122]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Miquel-Raynal/Link-consumer-with-clock-driver/20181123-113833
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
> config: sh-titan_defconfig (attached as .config)
> compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.2.0 make.cross ARCH=sh 
> 
> All errors (new ones prefixed by >>):
> 
>drivers//clk/clkdev.c: In function 'clk_get':
> >> drivers//clk/clkdev.c:209:3: error: implicit declaration of function 
> >> '__clk_device_link'; did you mean '__clk_free_clk'? 
> >> [-Werror=implicit-function-declaration]  
>   __clk_device_link(dev, clk);
>   ^
>   __clk_free_clk
>drivers//clk/clkdev.c: In function 'clk_put':
> >> drivers//clk/clkdev.c:217:2: error: implicit declaration of function 
> >> '__clk_device_unlink'; did you mean 'device_online'? 
> >> [-Werror=implicit-function-declaration]  
>  __clk_device_unlink(clk);
>  ^~~
>  device_online
>cc1: some warnings being treated as errors

I figured thanks to this report that this code won't compile with
architectures not compliant to the common clock framework. I see there
is the following block in clkdev.c.

#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
#else
#endif

Would you agree with me adding dummy functions in the #else section
like:

static inline void __clk_device_link(struct device *consumer, struct clk *clk)
{
   return;
}

static inline void __clk_device_unlink(struct clk *clk)
{
   return;
}

Do you want me to also declare these functions in the #if section
(with the external keyword) to balance the above declarations?

Thanks for your input.
Miquèl

> 
> vim +209 drivers//clk/clkdev.c
> 
>193
>194struct clk *clk_get(struct device *dev, const char *con_id)
>195{
>196const char *dev_id = dev ? dev_name(dev) : NULL;
>197struct clk *clk = NULL;
>198
>199if (dev && dev->of_node) {
>200clk = __of_clk_get_by_name(dev->of_node, 
> dev_id, con_id);
>201if (PTR_ERR(clk) == -EPROBE_DEFER)
>202return clk;
>203}
>204
>205if (IS_ERR_OR_NULL(clk))
>206clk = clk_get_sys(dev_id, con_id);
>207
>208if (!IS_ERR(clk))
>  > 209__clk_device_link(dev, clk);  
>210
>211return clk;
>212}
>213EXPORT_SYMBOL(clk_get);
>214
>215void clk_put(struct clk *clk)
>216{
>  > 217__clk_device_unlink(clk);  
>218__clk_put(clk);
>219}
>220EXPORT_SYMBOL(clk_put);
>221
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation


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

2018-11-23 Thread kbuild test robot
Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.20-rc3 next-20181122]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Miquel-Raynal/Link-consumer-with-clock-driver/20181123-113833
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   drivers//clk/clkdev.c: In function 'clk_get':
>> drivers//clk/clkdev.c:209:3: error: implicit declaration of function 
>> '__clk_device_link'; did you mean '__clk_free_clk'? 
>> [-Werror=implicit-function-declaration]
  __clk_device_link(dev, clk);
  ^
  __clk_free_clk
   drivers//clk/clkdev.c: In function 'clk_put':
>> drivers//clk/clkdev.c:217:2: error: implicit declaration of function 
>> '__clk_device_unlink'; did you mean 'device_online'? 
>> [-Werror=implicit-function-declaration]
 __clk_device_unlink(clk);
 ^~~
 device_online
   cc1: some warnings being treated as errors

vim +209 drivers//clk/clkdev.c

   193  
   194  struct clk *clk_get(struct device *dev, const char *con_id)
   195  {
   196  const char *dev_id = dev ? dev_name(dev) : NULL;
   197  struct clk *clk = NULL;
   198  
   199  if (dev && dev->of_node) {
   200  clk = __of_clk_get_by_name(dev->of_node, dev_id, 
con_id);
   201  if (PTR_ERR(clk) == -EPROBE_DEFER)
   202  return clk;
   203  }
   204  
   205  if (IS_ERR_OR_NULL(clk))
   206  clk = clk_get_sys(dev_id, con_id);
   207  
   208  if (!IS_ERR(clk))
 > 209  __clk_device_link(dev, clk);
   210  
   211  return clk;
   212  }
   213  EXPORT_SYMBOL(clk_get);
   214  
   215  void clk_put(struct clk *clk)
   216  {
 > 217  __clk_device_unlink(clk);
   218  __clk_put(clk);
   219  }
   220  EXPORT_SYMBOL(clk_put);
   221  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip


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

2018-11-22 Thread 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:
ahci-mvebu d00e.sata: Linked as a consumer to d0013000.nb-periph-clk
mvneta d003.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 d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d.sdhci: Dropping the link to d0013000.nb-periph-clk
advk-pcie d007.pcie: Linked as a consumer to d0018000.sb-periph-clk
xenon-sdhci d00d.sdhci: Linked as a consumer to d0013000.nb-periph-clk
xenon-sdhci d00d.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 
---
 drivers/clk/clk.c| 20 
 drivers/clk/clkdev.c | 13 ++---
 include/linux/clk-provider.h |  2 ++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b799347c5fd6..33a0f2b0533a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -90,6 +90,7 @@ struct clk {
unsigned long max_rate;
unsigned int exclusive_count;
struct hlist_node clks_node;
+   struct device_link *link;
 };
 
 /***   runtime pm  ***/
@@ -262,6 +263,25 @@ struct clk_hw *__clk_get_hw(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(__clk_get_hw);
 
+void __clk_device_link(struct device *consumer, struct clk *clk)
+{
+   if (!consumer || !clk || !clk->core)
+   return;
+
+   clk->link = device_link_add(consumer, clk->core->dev,
+   DL_FLAG_STATELESS);
+}
+EXPORT_SYMBOL_GPL(__clk_device_link);
+
+void __clk_device_unlink(struct clk *clk)
+{
+   if (!clk || !clk->link)
+   return;
+
+   device_link_del(clk->link);
+}
+EXPORT_SYMBOL_GPL(__clk_device_unlink);
+
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw)
 {
return hw->core->num_parents;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..fccfd4c01457 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -194,20 +194,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_device_link(dev, clk);
+
+   return clk;
 }
 EXPORT_SYMBOL(clk_get);
 
 void clk_put(struct clk *clk)
 {
+   __clk_device_unlink(clk);
__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 60c51871b04b..c7ba8098f854 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_device_link(struct device *consumer, struct clk *clk);
+void __clk_device_unlink(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