From: Russell King - ARM Linux <linux@arm.linux.org.uk> To: "Turquette, Mike" <mturquette@ti.com> Cc: Paul Walmsley <paul@pwsan.com>, linux-omap@vger.kernel.org, Rajendra Nayak <rnayak@ti.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/3] ARM: omap: clk: add clk_prepare and clk_unprepare Date: Tue, 31 Jul 2012 10:23:15 +0100 [thread overview] Message-ID: <20120731092315.GK7405@n2100.arm.linux.org.uk> (raw) In-Reply-To: <CAJOA=zNnLahwhzjE0jkmiax8=kU8O4puJQJ1VbA-gDScawg6jw@mail.gmail.com> On Mon, Jul 30, 2012 at 05:31:23PM -0700, Turquette, Mike wrote: > On Mon, Jul 30, 2012 at 4:02 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Jul 30, 2012 at 03:42:13PM -0700, Turquette, Mike wrote: > >> On Mon, Jul 30, 2012 at 3:31 PM, Paul Walmsley <paul@pwsan.com> wrote: > >> > So if we make a change like this one, it seems like we would basically > >> > expect it to break once we start doing anything meaningful with > >> > clk_prepare(), like using clk_prepare() for voltage scaling or something > >> > that requires I2C? We'd also probably want to mark this with some kind of > >> > "HACK" comment. > >> > >> Hi Paul, > >> > >> Did you have anything in mind to start using clk_prepare? I still > >> hope to get rid of it some day and replace that call with a > >> combination of clk_enable and a check like clk_enable_can_sleep. > > > > Don't you dare. > > > > We arrived at the clk_prepare()/clk_enable() split after lots of > > discussion between different platform maintainers and their > > requirements. > > > > Shoving crap like "clk_enable_can_sleep()" down into drivers is > > totally and utterly idiotic. We've had the situation *already* > > where some drivers can be used on some platforms but not on others > > because of differences in clk_enable() expectations. > > > > How does having a dynamic run-time check cause a generic driver to run > on "some platforms but not on others"? What you're asking is for drivers which use clk_prepare() and clk_enable() correctly to be littered with: - clk_prepare(clk); + if (clk_enable_can_sleep(clk)) + clk_enable(clk); and, in atomic contexts: - clk_enable(clk); + if (!clk_enable_can_sleep(clk)) + clk_enable(clk); which is total bollocks - drivers should not need to know about this clk_enable_can_sleep() crap. > Two calls exist because of context differences. But in practice they > do the same thing: cause a line to toggle at a rate. If a run-time > check exists and the framework could handle this gracefully, why would > you still choose the split api? How can it handle it gracefully? Take a look at something like amba-pl011.c for a clue. Remove the clk_prepare() and clk_unprepare() calls. Then consider a clk implementation which does not support clk_enable()/clk_disable() from atomic contexts. Now think about how the pl011 driver enable the clock for console output from atomic contexts? Answer, it can't. We've been here before with this stuff, and this is exactly why we split clk_enable() into the two separate calls we have today.
WARNING: multiple messages have this Message-ID (diff)
From: linux@arm.linux.org.uk (Russell King - ARM Linux) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/3] ARM: omap: clk: add clk_prepare and clk_unprepare Date: Tue, 31 Jul 2012 10:23:15 +0100 [thread overview] Message-ID: <20120731092315.GK7405@n2100.arm.linux.org.uk> (raw) In-Reply-To: <CAJOA=zNnLahwhzjE0jkmiax8=kU8O4puJQJ1VbA-gDScawg6jw@mail.gmail.com> On Mon, Jul 30, 2012 at 05:31:23PM -0700, Turquette, Mike wrote: > On Mon, Jul 30, 2012 at 4:02 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Jul 30, 2012 at 03:42:13PM -0700, Turquette, Mike wrote: > >> On Mon, Jul 30, 2012 at 3:31 PM, Paul Walmsley <paul@pwsan.com> wrote: > >> > So if we make a change like this one, it seems like we would basically > >> > expect it to break once we start doing anything meaningful with > >> > clk_prepare(), like using clk_prepare() for voltage scaling or something > >> > that requires I2C? We'd also probably want to mark this with some kind of > >> > "HACK" comment. > >> > >> Hi Paul, > >> > >> Did you have anything in mind to start using clk_prepare? I still > >> hope to get rid of it some day and replace that call with a > >> combination of clk_enable and a check like clk_enable_can_sleep. > > > > Don't you dare. > > > > We arrived at the clk_prepare()/clk_enable() split after lots of > > discussion between different platform maintainers and their > > requirements. > > > > Shoving crap like "clk_enable_can_sleep()" down into drivers is > > totally and utterly idiotic. We've had the situation *already* > > where some drivers can be used on some platforms but not on others > > because of differences in clk_enable() expectations. > > > > How does having a dynamic run-time check cause a generic driver to run > on "some platforms but not on others"? What you're asking is for drivers which use clk_prepare() and clk_enable() correctly to be littered with: - clk_prepare(clk); + if (clk_enable_can_sleep(clk)) + clk_enable(clk); and, in atomic contexts: - clk_enable(clk); + if (!clk_enable_can_sleep(clk)) + clk_enable(clk); which is total bollocks - drivers should not need to know about this clk_enable_can_sleep() crap. > Two calls exist because of context differences. But in practice they > do the same thing: cause a line to toggle at a rate. If a run-time > check exists and the framework could handle this gracefully, why would > you still choose the split api? How can it handle it gracefully? Take a look at something like amba-pl011.c for a clue. Remove the clk_prepare() and clk_unprepare() calls. Then consider a clk implementation which does not support clk_enable()/clk_disable() from atomic contexts. Now think about how the pl011 driver enable the clock for console output from atomic contexts? Answer, it can't. We've been here before with this stuff, and this is exactly why we split clk_enable() into the two separate calls we have today.
next prev parent reply other threads:[~2012-07-31 9:23 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-07-02 10:24 [PATCH v3 0/3] Prepare for OMAP2+ movement to Common Clk Rajendra Nayak 2012-07-02 10:24 ` Rajendra Nayak 2012-07-02 10:24 ` [PATCH v3 1/3] ARM: omap: clk: add clk_prepare and clk_unprepare Rajendra Nayak 2012-07-02 10:24 ` Rajendra Nayak 2012-07-30 22:31 ` Paul Walmsley 2012-07-30 22:31 ` Paul Walmsley 2012-07-30 22:42 ` Turquette, Mike 2012-07-30 22:42 ` Turquette, Mike 2012-07-30 23:02 ` Russell King - ARM Linux 2012-07-30 23:02 ` Russell King - ARM Linux 2012-07-31 0:31 ` Turquette, Mike 2012-07-31 0:31 ` Turquette, Mike 2012-07-31 8:21 ` Saravana Kannan 2012-07-31 8:21 ` Saravana Kannan 2012-07-31 9:27 ` Russell King - ARM Linux 2012-07-31 9:27 ` Russell King - ARM Linux 2012-07-31 9:23 ` Russell King - ARM Linux [this message] 2012-07-31 9:23 ` Russell King - ARM Linux 2012-07-31 18:00 ` Paul Walmsley 2012-07-31 18:00 ` Paul Walmsley 2012-07-02 10:24 ` [PATCH v3 2/3] ARM: omap: hwmod: get rid of all omap_clk_get_by_name usage Rajendra Nayak 2012-07-02 10:24 ` Rajendra Nayak 2012-07-02 10:24 ` [PATCH v3 3/3] ARM: omap: clk: Remove all direct dereferencing of struct clk Rajendra Nayak 2012-07-02 10:24 ` Rajendra Nayak 2012-07-30 7:12 ` Paul Walmsley 2012-07-30 7:12 ` Paul Walmsley 2012-07-30 7:36 ` Rajendra Nayak 2012-07-30 7:36 ` Rajendra Nayak 2012-07-30 18:56 ` Paul Walmsley 2012-07-30 18:56 ` Paul Walmsley 2012-07-31 6:05 ` Rajendra Nayak 2012-07-31 6:05 ` Rajendra Nayak
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=20120731092315.GK7405@n2100.arm.linux.org.uk \ --to=linux@arm.linux.org.uk \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=mturquette@ti.com \ --cc=paul@pwsan.com \ --cc=rnayak@ti.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.