From: Miquel Raynal <miquel.raynal@bootlin.com> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Russell King <linux@armlinux.org.uk> Cc: linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Antoine Tenart <antoine.tenart@bootlin.com>, Maxime Chevallier <maxime.chevallier@bootlin.com>, Gregory Clement <gregory.clement@bootlin.com>, Nadav Haklai <nadavh@marvell.com>, Miquel Raynal <miquel.raynal@bootlin.com> Subject: [PATCH v3 0/4] Add device links to clocks Date: Tue, 4 Dec 2018 20:24:36 +0100 [thread overview] Message-ID: <20181204192440.12125-1-miquel.raynal@bootlin.com> (raw) 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
WARNING: multiple messages have this Message-ID (diff)
From: Miquel Raynal <miquel.raynal@bootlin.com> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Russell King <linux@armlinux.org.uk> Cc: Antoine Tenart <antoine.tenart@bootlin.com>, Gregory Clement <gregory.clement@bootlin.com>, linux-kernel@vger.kernel.org, Maxime Chevallier <maxime.chevallier@bootlin.com>, Nadav Haklai <nadavh@marvell.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Miquel Raynal <miquel.raynal@bootlin.com>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 0/4] Add device links to clocks Date: Tue, 4 Dec 2018 20:24:36 +0100 [thread overview] Message-ID: <20181204192440.12125-1-miquel.raynal@bootlin.com> (raw) 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next reply other threads:[~2018-12-04 19:24 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-04 19:24 Miquel Raynal [this message] 2018-12-04 19:24 ` [PATCH v3 0/4] Add device links to clocks Miquel Raynal 2018-12-04 19:24 ` [PATCH v3 1/4] clk: core: clarify the check for runtime PM Miquel Raynal 2018-12-04 19:24 ` Miquel Raynal 2018-12-19 0:03 ` Stephen Boyd 2018-12-19 0:03 ` Stephen Boyd 2018-12-19 8:03 ` Miquel Raynal 2018-12-19 8:03 ` Miquel Raynal 2018-12-20 21:09 ` Stephen Boyd 2018-12-20 21:09 ` Stephen Boyd 2019-01-02 10:55 ` Miquel Raynal 2019-01-02 10:55 ` Miquel Raynal 2018-12-04 19:24 ` [PATCH v3 2/4] clk: core: link consumer with clock driver Miquel Raynal 2018-12-04 19:24 ` Miquel Raynal 2018-12-11 17:12 ` Stephen Boyd 2018-12-11 17:12 ` Stephen Boyd 2018-12-17 14:24 ` Miquel Raynal 2018-12-17 14:24 ` Miquel Raynal 2019-01-04 15:54 ` Miquel Raynal 2019-01-04 15:54 ` Miquel Raynal 2019-01-25 21:28 ` Stephen Boyd 2019-01-25 21:28 ` Stephen Boyd 2019-01-28 10:06 ` Miquel Raynal 2019-01-28 10:06 ` Miquel Raynal 2018-12-04 19:24 ` [PATCH v3 3/4] clk: mvebu: armada-37xx-tbg: fill the device entry when registering the clocks Miquel Raynal 2018-12-04 19:24 ` Miquel Raynal 2018-12-04 19:24 ` [PATCH v3 4/4] clk: mvebu: armada-37xx-xtal: fill the device entry when registering the clock Miquel Raynal 2018-12-04 19:24 ` Miquel Raynal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181204192440.12125-1-miquel.raynal@bootlin.com \ --to=miquel.raynal@bootlin.com \ --cc=antoine.tenart@bootlin.com \ --cc=gregory.clement@bootlin.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=maxime.chevallier@bootlin.com \ --cc=mturquette@baylibre.com \ --cc=nadavh@marvell.com \ --cc=sboyd@kernel.org \ --cc=thomas.petazzoni@bootlin.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.