From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Subject: Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework Date: Thu, 30 Jul 2015 12:17:47 +0100 Message-ID: <20150730111747.GF14642@x1> References: <1437570255-21049-1-git-send-email-lee.jones@linaro.org> <1437570255-21049-4-git-send-email-lee.jones@linaro.org> <20150730010213.642.10831@quantum> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <20150730010213.642.10831@quantum> Sender: linux-kernel-owner@vger.kernel.org To: Michael Turquette Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@stlinux.com, sboyd@codeaurora.org, devicetree@vger.kernel.org, geert@linux-m68k.org, maxime.ripard@free-electrons.com, s.hauer@pengutronix.de, linux-clk@vger.kernel.org List-Id: devicetree@vger.kernel.org On Wed, 29 Jul 2015, Michael Turquette wrote: > Hi Lee, >=20 > + linux-clk ml >=20 > Quoting Lee Jones (2015-07-22 06:04:13) > > These new API calls will firstly provide a mechanisms to tag a cloc= k as > > critical and secondly allow any knowledgeable driver to (un)gate cl= ocks, > > even if they are marked as critical. > >=20 > > Suggested-by: Maxime Ripard > > Signed-off-by: Lee Jones > > --- > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++= ++++++++++++ > > include/linux/clk-provider.h | 2 ++ > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++ > > 3 files changed, 77 insertions(+) > >=20 > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 61c3fc5..486b1da 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const ch= ar *name); > > =20 > > /*** private data structures ***/ > > =20 > > +/** > > + * struct critical - Provides 'play' over critical clocks. A cl= ock can be > > + * marked as critical, meaning that it should = not be > > + * disabled. However, if a driver which is aw= are of the > > + * critical behaviour wants to control it, it = can do so > > + * using clk_enable_critical() and clk_disable= _critical(). > > + * > > + * @enabled Is clock critical? Once set, doesn't change > > + * @leave_on Self explanatory. Can be disabled by knowledgeable= drivers >=20 > Not self explanatory. I need this explained to me. What does leave_on > do? Better yet, what would happen if leave_on did not exist? >=20 > > + */ > > +struct critical { > > + bool enabled; > > + bool leave_on; > > +}; > > + > > struct clk_core { > > const char *name; > > const struct clk_ops *ops; > > @@ -75,6 +90,7 @@ struct clk_core { > > struct dentry *dentry; > > #endif > > struct kref ref; > > + struct critical critical; > > }; > > =20 > > struct clk { > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core = *clk) > > if (WARN_ON(clk->enable_count =3D=3D 0)) > > return; > > =20 > > + /* Refuse to turn off a critical clock */ > > + if (clk->enable_count =3D=3D 1 && clk->critical.leave_on) > > + return; >=20 > How do we get to this point? clk_enable_critical actually calls > clk_enable, thus incrementing the enable_count. The only time that we > could hit the above case is if, >=20 > a) there is an imbalance in clk_enable and clk_disable calls. If this= is > the case then the drivers need to be fixed. Or better yet some > infrastructure to catch that, now that we have per-user struct clk > cookies. >=20 > b) a driver knowingly calls clk_enable_critical(foo) and then regular= , > old clk_disable(foo). But why would a driver do that? >=20 > It might be that I am missing the point here, so please feel free to > clue me in. This check behaves in a very similar to the WARN() above. It's more of a fail-safe. If all drivers are behaving properly, then it shouldn't ever be true. If they're not, it prevents an incorrectly written driver from irrecoverably crippling the system. As I said in the other mail. We can do without these 3 new wrappers. We _could_ just write a driver which only calls clk_enable() _after_ it calls clk_disable(), a kind of intentional unbalance and it would do that same thing. However, what we're trying to do here is provide a proper API, so we can see at first glance what the 'knowledgeable' driver is trying to do and not have someone attempt to submit a 'fix' which calls clk_enable() or something. > > + > > if (--clk->enable_count > 0) > > return; > > =20 > > @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_disable); > > =20 > > +void clk_disable_critical(struct clk *clk) > > +{ > > + clk->core->critical.leave_on =3D false; > > + clk_disable(clk); > > +} > > +EXPORT_SYMBOL_GPL(clk_disable_critical); > > + > > static int clk_core_enable(struct clk_core *clk) > > { > > int ret =3D 0; > > @@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_enable); > > =20 > > +int clk_enable_critical(struct clk *clk) > > +{ > > + if (clk->core->critical.enabled) > > + clk->core->critical.leave_on =3D true; > > + > > + return clk_enable(clk); > > +} > > +EXPORT_SYMBOL_GPL(clk_enable_critical); > > + > > static unsigned long clk_core_round_rate_nolock(struct clk_core *c= lk, > > unsigned long rate, > > unsigned long min_r= ate, > > @@ -2482,6 +2518,15 @@ fail_out: > > } > > EXPORT_SYMBOL_GPL(clk_register); > > =20 > > +void clk_init_critical(struct clk *clk) > > +{ > > + struct critical *critical =3D &clk->core->critical; > > + > > + critical->enabled =3D true; > > + critical->leave_on =3D true; > > +} > > +EXPORT_SYMBOL_GPL(clk_init_critical); > > + > > /* > > * Free memory allocated for a clock. > > * Caller must hold prepare_lock. > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provi= der.h > > index 5591ea7..15ef8c9 100644 > > --- a/include/linux/clk-provider.h > > +++ b/include/linux/clk-provider.h > > @@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *de= v, struct clk_hw *hw); > > void clk_unregister(struct clk *clk); > > void devm_clk_unregister(struct device *dev, struct clk *clk); > > =20 > > +void clk_init_critical(struct clk *clk); > > + > > /* helper functions */ > > const char *__clk_get_name(struct clk *clk); > > struct clk_hw *__clk_get_hw(struct clk *clk); > > diff --git a/include/linux/clk.h b/include/linux/clk.h > > index 8381bbf..9807f3b 100644 > > --- a/include/linux/clk.h > > +++ b/include/linux/clk.h > > @@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, c= onst char *id); > > int clk_enable(struct clk *clk); > > =20 > > /** > > + * clk_enable_critical - inform the system when the clock source s= hould be > > + * running, even if clock is critical. > > + * @clk: clock source > > + * > > + * If the clock can not be enabled/disabled, this should return su= ccess. > > + * > > + * May be called from atomic contexts. > > + * > > + * Returns success (0) or negative errno. > > + */ > > +int clk_enable_critical(struct clk *clk); > > + > > +/** > > * clk_disable - inform the system when the clock source is no lon= ger required. > > * @clk: clock source > > * > > @@ -247,6 +260,23 @@ int clk_enable(struct clk *clk); > > void clk_disable(struct clk *clk); > > =20 > > /** > > + * clk_disable_critical - inform the system when the clock source = is no > > + * longer required, even if clock is critica= l. > > + * @clk: clock source > > + * > > + * Inform the system that a clock source is no longer required by > > + * a driver and may be shut down. > > + * > > + * May be called from atomic contexts. > > + * > > + * Implementation detail: if the clock source is shared between > > + * multiple drivers, clk_enable_critical() calls must be balanced > > + * by the same number of clk_disable_critical() calls for the cloc= k > > + * source to be disabled. > > + */ > > +void clk_disable_critical(struct clk *clk); > > + > > +/** > > * clk_get_rate - obtain the current clock rate (in Hz) for a cloc= k source. > > * This is only valid once the clock source has been= enabled. > > * @clk: clock source --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 Open source software for ARM SoCs =46ollow Linaro: Facebook | Twitter | Blog