From: emilio@elopez.com.ar (Emilio López)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/5] clk: sun6i: Protect CPU clock
Date: Mon, 24 Feb 2014 20:38:44 -0300 [thread overview]
Message-ID: <530BD804.5090806@elopez.com.ar> (raw)
In-Reply-To: <20140224163034.GN21483@n2100.arm.linux.org.uk>
Hello Russell,
El 24/02/14 13:30, Russell King - ARM Linux escribi?:
> On Mon, Feb 24, 2014 at 05:22:43PM +0100, Maxime Ripard wrote:
>> Right now, AHB is an indirect child clock of the CPU clock. If that happens to
>> change, since the CPU clock has no other consumers declared in Linux, it would
>> be shut down, which is not really a good idea.
>>
>> Prevent this by forcing it enabled.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>> drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 23baad9..cedaf4b 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void)
>> clk_prepare_enable(clk);
>> clk_put(clk);
>> }
>> +
>> + /* CPU clocks - sun6i */
>> + clk = clk_get(NULL, "cpu");
>> + if (!IS_ERR(clk)) {
>> + clk_prepare_enable(clk);
>> + clk_put(clk);
>> + }
>
> This is broken. I'm not sure what's difficult to grasp about the concept
> of "while a clock is in use, you should keep a reference to that clock".
>
> That implies that if you get a clock, and then enable it, you don't
> put the clock until you've disabled it.
Why is this so? Can't a clock be left enabled while nobody has a
reference to it? I have looked around in Documentation/ (rather quickly
I must say) and have not found any explicit mention that it is required
to keep a reference to the clock while it's enabled. I'd appreciate it
if you could explain this a bit more verbosely or point me to the
relevant documents.
For what it's worth, I've seen this same pattern on
enable/disable_clock() on drivers/base/power/clock_ops.c as well.
Cheers,
Emilio
WARNING: multiple messages have this Message-ID (diff)
From: "Emilio López" <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org>
To: Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>,
Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Dan Williams
<dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
Vinod Koul <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Mike Turquette
<mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Subject: Re: [PATCH 1/5] clk: sun6i: Protect CPU clock
Date: Mon, 24 Feb 2014 20:38:44 -0300 [thread overview]
Message-ID: <530BD804.5090806@elopez.com.ar> (raw)
In-Reply-To: <20140224163034.GN21483-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
Hello Russell,
El 24/02/14 13:30, Russell King - ARM Linux escribió:
> On Mon, Feb 24, 2014 at 05:22:43PM +0100, Maxime Ripard wrote:
>> Right now, AHB is an indirect child clock of the CPU clock. If that happens to
>> change, since the CPU clock has no other consumers declared in Linux, it would
>> be shut down, which is not really a good idea.
>>
>> Prevent this by forcing it enabled.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>> drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 23baad9..cedaf4b 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void)
>> clk_prepare_enable(clk);
>> clk_put(clk);
>> }
>> +
>> + /* CPU clocks - sun6i */
>> + clk = clk_get(NULL, "cpu");
>> + if (!IS_ERR(clk)) {
>> + clk_prepare_enable(clk);
>> + clk_put(clk);
>> + }
>
> This is broken. I'm not sure what's difficult to grasp about the concept
> of "while a clock is in use, you should keep a reference to that clock".
>
> That implies that if you get a clock, and then enable it, you don't
> put the clock until you've disabled it.
Why is this so? Can't a clock be left enabled while nobody has a
reference to it? I have looked around in Documentation/ (rather quickly
I must say) and have not found any explicit mention that it is required
to keep a reference to the clock while it's enabled. I'd appreciate it
if you could explain this a bit more verbosely or point me to the
relevant documents.
For what it's worth, I've seen this same pattern on
enable/disable_clock() on drivers/base/power/clock_ops.c as well.
Cheers,
Emilio
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.
WARNING: multiple messages have this Message-ID (diff)
From: "Emilio López" <emilio@elopez.com.ar>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>,
Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
Vinod Koul <vinod.koul@intel.com>,
devicetree@vger.kernel.org,
Mike Turquette <mturquette@linaro.org>,
linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com,
dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/5] clk: sun6i: Protect CPU clock
Date: Mon, 24 Feb 2014 20:38:44 -0300 [thread overview]
Message-ID: <530BD804.5090806@elopez.com.ar> (raw)
In-Reply-To: <20140224163034.GN21483@n2100.arm.linux.org.uk>
Hello Russell,
El 24/02/14 13:30, Russell King - ARM Linux escribió:
> On Mon, Feb 24, 2014 at 05:22:43PM +0100, Maxime Ripard wrote:
>> Right now, AHB is an indirect child clock of the CPU clock. If that happens to
>> change, since the CPU clock has no other consumers declared in Linux, it would
>> be shut down, which is not really a good idea.
>>
>> Prevent this by forcing it enabled.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>> drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 23baad9..cedaf4b 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void)
>> clk_prepare_enable(clk);
>> clk_put(clk);
>> }
>> +
>> + /* CPU clocks - sun6i */
>> + clk = clk_get(NULL, "cpu");
>> + if (!IS_ERR(clk)) {
>> + clk_prepare_enable(clk);
>> + clk_put(clk);
>> + }
>
> This is broken. I'm not sure what's difficult to grasp about the concept
> of "while a clock is in use, you should keep a reference to that clock".
>
> That implies that if you get a clock, and then enable it, you don't
> put the clock until you've disabled it.
Why is this so? Can't a clock be left enabled while nobody has a
reference to it? I have looked around in Documentation/ (rather quickly
I must say) and have not found any explicit mention that it is required
to keep a reference to the clock while it's enabled. I'd appreciate it
if you could explain this a bit more verbosely or point me to the
relevant documents.
For what it's worth, I've seen this same pattern on
enable/disable_clock() on drivers/base/power/clock_ops.c as well.
Cheers,
Emilio
next prev parent reply other threads:[~2014-02-24 23:38 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-24 16:22 [PATCH 0/5] Add support for the Allwinner A31 DMA Controller Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` [PATCH 1/5] clk: sun6i: Protect CPU clock Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:30 ` Russell King - ARM Linux
2014-02-24 16:30 ` Russell King - ARM Linux
2014-02-24 16:30 ` Russell King - ARM Linux
2014-02-24 23:38 ` Emilio López [this message]
2014-02-24 23:38 ` Emilio López
2014-02-24 23:38 ` Emilio López
2014-02-25 0:01 ` Russell King - ARM Linux
2014-02-25 0:01 ` Russell King - ARM Linux
2014-02-25 0:01 ` Russell King - ARM Linux
2014-02-25 0:30 ` Emilio López
2014-02-25 0:30 ` Emilio López
2014-02-25 0:30 ` Emilio López
2014-02-24 16:22 ` [PATCH 2/5] clk: sun6i: Reparent AHB clock on PLL6 Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` [PATCH 3/5] clk: sun6i: Protect SDRAM gating bit Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` [PATCH 4/5] DMA: sun6i: Add driver for the Allwinner A31 DMA controller Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-25 11:28 ` Andy Shevchenko
2014-02-25 11:28 ` Andy Shevchenko
2014-02-25 11:28 ` Andy Shevchenko
2014-02-28 10:36 ` Maxime Ripard
2014-02-28 10:36 ` Maxime Ripard
2014-02-28 10:36 ` Maxime Ripard
2014-02-28 13:33 ` Andy Shevchenko
2014-02-28 13:33 ` Andy Shevchenko
2014-02-28 13:33 ` Andy Shevchenko
2014-02-28 14:27 ` Maxime Ripard
2014-02-28 14:27 ` Maxime Ripard
2014-02-28 14:27 ` Maxime Ripard
2014-02-24 16:22 ` [PATCH 5/5] ARM: sun6i: dt: Add A31 DMA controller to DTSI Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
2014-02-24 16:22 ` Maxime Ripard
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=530BD804.5090806@elopez.com.ar \
--to=emilio@elopez.com.ar \
--cc=linux-arm-kernel@lists.infradead.org \
/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: link
Be 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.