lkml.org 
[lkml]   [2018]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] clk: core: link consumer with clock driver
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 d00e0000.sata: Linked as a consumer to d0013000.nb-periph-clk
> 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
> 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 | 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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-11-27 13:41    [W:0.180 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site