Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Hi Mike,
On Mon, Mar 30, 2015 at 6:36 PM, Michael Turquette
wrote:
> Quoting Andrew Bresticker (2015-03-30 17:15:43)
>> On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd wrote:
>> > On 02/24/15 19:56, Andrew Bresticker wrote:
>> >> +
>> >> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
>> >> + unsigned int *clk_ids, unsigned int num)
>> >> +{
>> >> + unsigned int i;
>> >> + int err;
>> >> +
>> >> + for (i = 0; i < num; i++) {
>> >> + struct clk *clk = p->clk_data.clks[clk_ids[i]];
>> >> +
>> >> + if (IS_ERR(clk))
>> >> + continue;
>> >> +
>> >> + err = clk_prepare_enable(clk);
>> >> + if (err)
>> >> + pr_err("Failed to enable clock %s: %d\n",
>> >> +__clk_get_name(clk), err);
>> >> + }
>> >> +}
>> >>
>> >
>> > Is this to workaround some problems in the framework where clocks are
>> > turned off? Or is it that these clocks are already on before we boot
>> > Linux and we need to make sure the framework knows that?
>>
>> It's the former. These clocks are enabled at POR and may only be
>> gated as the final step to entering suspend, so they must remain on at
>> runtime. The issue we were running into was that consumers of these
>> critical clocks or their descendants would enable/disable their clocks
>> during boot or runtime PM and cause these clocks to get disabled.
>> Bumping up the prepare/enable count of these critical clocks seemed
>> like the best way to handle this - is there a more preferred way?
>> FWIW, this is also how the Tegra and Rockchip drivers handled this
>> problem.
>
> Hi Andrew,
>
> Why are your drivers allowed to disable clocks which must not be
> disabled? (you mentioned boot and runtime pm)
The issue is that they do not directly consume a critical clock, but
rather a descendant of the critical clock and thus could cause the
critical clock to be disabled. For example, the periph_sys clock is
one of these critical clocks and it is the parent of various
peripheral clocks, like the watchdog, I2C, PWM, etc. If the I2C
driver enables and disables it's clock during probe(), and nothing
else has caused the periph_sys clock to be enabled, the disable() call
will cause periph_sys to become disabled since its enable count drops
to 0.
Now this could be solved if there was a driver to directly consume
these clocks and keep them enabled, but, like Stephen mentioned, there
really isn't a proper driver for that. So it looks like I want
something like the always-on feature Lee is trying to introduce.
Thanks,
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Quoting Andrew Bresticker (2015-03-30 17:15:43)
> On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd wrote:
> > On 02/24/15 19:56, Andrew Bresticker wrote:
> >> +
> >> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
> >> + unsigned int *clk_ids, unsigned int num)
> >> +{
> >> + unsigned int i;
> >> + int err;
> >> +
> >> + for (i = 0; i < num; i++) {
> >> + struct clk *clk = p->clk_data.clks[clk_ids[i]];
> >> +
> >> + if (IS_ERR(clk))
> >> + continue;
> >> +
> >> + err = clk_prepare_enable(clk);
> >> + if (err)
> >> + pr_err("Failed to enable clock %s: %d\n",
> >> +__clk_get_name(clk), err);
> >> + }
> >> +}
> >>
> >
> > Is this to workaround some problems in the framework where clocks are
> > turned off? Or is it that these clocks are already on before we boot
> > Linux and we need to make sure the framework knows that?
>
> It's the former. These clocks are enabled at POR and may only be
> gated as the final step to entering suspend, so they must remain on at
> runtime. The issue we were running into was that consumers of these
> critical clocks or their descendants would enable/disable their clocks
> during boot or runtime PM and cause these clocks to get disabled.
> Bumping up the prepare/enable count of these critical clocks seemed
> like the best way to handle this - is there a more preferred way?
> FWIW, this is also how the Tegra and Rockchip drivers handled this
> problem.
Hi Andrew,
Why are your drivers allowed to disable clocks which must not be
disabled? (you mentioned boot and runtime pm)
Is this the case that a critical clock is not directly disabled, but a
parent of that critical clock is and it is gated as a result?
Regards,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On Mon, Mar 30, 2015 at 6:21 PM, Stephen Boyd wrote:
> On 03/30/15 17:15, Andrew Bresticker wrote:
>> On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd wrote:
>>> On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i < num; i++) {
+ struct clk *clk = p->clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err("Failed to enable clock %s: %d\n",
+__clk_get_name(clk), err);
+ }
+}
>>> Is this to workaround some problems in the framework where clocks are
>>> turned off? Or is it that these clocks are already on before we boot
>>> Linux and we need to make sure the framework knows that?
>> It's the former. These clocks are enabled at POR and may only be
>> gated as the final step to entering suspend, so they must remain on at
>> runtime. The issue we were running into was that consumers of these
>> critical clocks or their descendants would enable/disable their clocks
>> during boot or runtime PM and cause these clocks to get disabled.
>> Bumping up the prepare/enable count of these critical clocks seemed
>> like the best way to handle this - is there a more preferred way?
>> FWIW, this is also how the Tegra and Rockchip drivers handled this
>> problem.
>
> Ideally clock providers just provide clocks and don't actually call
> clock consumer APIs. I don't see where these clocks are disabled in this
> series. Is it just because suspend isn't done right now so there isn't a
> place to hook the disable part? I hope that it's a 1:1 relation between
> the clocks that are turned on here and the clocks that are turned off
> during suspend.
Suspend hasn't been hooked up yet and it's still a long ways off.
> I have a slightly similar problem on my hardware. Consider the case
> where the bootloader has left on the display and audio clocks and they
> share a common parent PLL. When the kernel boots up, all it knows is
> that the display clock and audio clock share a common PLL and the rate
> they're running at. If the audio driver probes first, calls clk_enable()
> on the audio clock (almost a no-op except for the fact that we call the
> .enable op when it's already on) and then calls clk_disable() on the
> audio clock when it's done we'll also turn off the shared PLL.
> Unfortunately it's also being used by the display clock for the display
> driver that hasn't probed yet and so the display stops working and it
> may show an artifact or black screen.
>
> Other cases are where certain clocks should never be turned off because
> they're used by some non-linux entity (dram controller for example) and
> we don't have a place to put the clk_prepare_enable() besides in the
> clock driver itself. In these cases, it may be better to tell the
> framework that a clock should always be on. I think this case is what
> Lee Jones is working on here[1].
>
> Do you fall into one of these two cases? It isn't clear to me how
> suspend is special and needs to be dealt with differently. Why wouldn't
> these clocks be always on?
These clocks fall into the latter case in that there's really no good
place to put the clk_prepare_enable() calls, so it looks like I want
to use what Lee is proposing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On 03/30/15 17:15, Andrew Bresticker wrote:
> On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd wrote:
>> On 02/24/15 19:56, Andrew Bresticker wrote:
>>> +
>>> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
>>> + unsigned int *clk_ids, unsigned int num)
>>> +{
>>> + unsigned int i;
>>> + int err;
>>> +
>>> + for (i = 0; i < num; i++) {
>>> + struct clk *clk = p->clk_data.clks[clk_ids[i]];
>>> +
>>> + if (IS_ERR(clk))
>>> + continue;
>>> +
>>> + err = clk_prepare_enable(clk);
>>> + if (err)
>>> + pr_err("Failed to enable clock %s: %d\n",
>>> +__clk_get_name(clk), err);
>>> + }
>>> +}
>>>
>> Is this to workaround some problems in the framework where clocks are
>> turned off? Or is it that these clocks are already on before we boot
>> Linux and we need to make sure the framework knows that?
> It's the former. These clocks are enabled at POR and may only be
> gated as the final step to entering suspend, so they must remain on at
> runtime. The issue we were running into was that consumers of these
> critical clocks or their descendants would enable/disable their clocks
> during boot or runtime PM and cause these clocks to get disabled.
> Bumping up the prepare/enable count of these critical clocks seemed
> like the best way to handle this - is there a more preferred way?
> FWIW, this is also how the Tegra and Rockchip drivers handled this
> problem.
Ideally clock providers just provide clocks and don't actually call
clock consumer APIs. I don't see where these clocks are disabled in this
series. Is it just because suspend isn't done right now so there isn't a
place to hook the disable part? I hope that it's a 1:1 relation between
the clocks that are turned on here and the clocks that are turned off
during suspend.
I have a slightly similar problem on my hardware. Consider the case
where the bootloader has left on the display and audio clocks and they
share a common parent PLL. When the kernel boots up, all it knows is
that the display clock and audio clock share a common PLL and the rate
they're running at. If the audio driver probes first, calls clk_enable()
on the audio clock (almost a no-op except for the fact that we call the
.enable op when it's already on) and then calls clk_disable() on the
audio clock when it's done we'll also turn off the shared PLL.
Unfortunately it's also being used by the display clock for the display
driver that hasn't probed yet and so the display stops working and it
may show an artifact or black screen.
Other cases are where certain clocks should never be turned off because
they're used by some non-linux entity (dram controller for example) and
we don't have a place to put the clk_prepare_enable() besides in the
clock driver itself. In these cases, it may be better to tell the
framework that a clock should always be on. I think this case is what
Lee Jones is working on here[1].
Do you fall into one of these two cases? It isn't clear to me how
suspend is special and needs to be dealt with differently. Why wouldn't
these clocks be always on?
[1] https://lkml.org/lkml/2015/2/27/548
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd wrote:
> On 02/24/15 19:56, Andrew Bresticker wrote:
>> +
>> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
>> + unsigned int *clk_ids, unsigned int num)
>> +{
>> + unsigned int i;
>> + int err;
>> +
>> + for (i = 0; i < num; i++) {
>> + struct clk *clk = p->clk_data.clks[clk_ids[i]];
>> +
>> + if (IS_ERR(clk))
>> + continue;
>> +
>> + err = clk_prepare_enable(clk);
>> + if (err)
>> + pr_err("Failed to enable clock %s: %d\n",
>> +__clk_get_name(clk), err);
>> + }
>> +}
>>
>
> Is this to workaround some problems in the framework where clocks are
> turned off? Or is it that these clocks are already on before we boot
> Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On 02/24/15 19:56, Andrew Bresticker wrote:
> +
> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
> + unsigned int *clk_ids, unsigned int num)
> +{
> + unsigned int i;
> + int err;
> +
> + for (i = 0; i < num; i++) {
> + struct clk *clk = p->clk_data.clks[clk_ids[i]];
> +
> + if (IS_ERR(clk))
> + continue;
> +
> + err = clk_prepare_enable(clk);
> + if (err)
> + pr_err("Failed to enable clock %s: %d\n",
> +__clk_get_name(clk), err);
> + }
> +}
>
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On 03/30/15 17:15, Andrew Bresticker wrote:
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
Ideally clock providers just provide clocks and don't actually call
clock consumer APIs. I don't see where these clocks are disabled in this
series. Is it just because suspend isn't done right now so there isn't a
place to hook the disable part? I hope that it's a 1:1 relation between
the clocks that are turned on here and the clocks that are turned off
during suspend.
I have a slightly similar problem on my hardware. Consider the case
where the bootloader has left on the display and audio clocks and they
share a common parent PLL. When the kernel boots up, all it knows is
that the display clock and audio clock share a common PLL and the rate
they're running at. If the audio driver probes first, calls clk_enable()
on the audio clock (almost a no-op except for the fact that we call the
.enable op when it's already on) and then calls clk_disable() on the
audio clock when it's done we'll also turn off the shared PLL.
Unfortunately it's also being used by the display clock for the display
driver that hasn't probed yet and so the display stops working and it
may show an artifact or black screen.
Other cases are where certain clocks should never be turned off because
they're used by some non-linux entity (dram controller for example) and
we don't have a place to put the clk_prepare_enable() besides in the
clock driver itself. In these cases, it may be better to tell the
framework that a clock should always be on. I think this case is what
Lee Jones is working on here[1].
Do you fall into one of these two cases? It isn't clear to me how
suspend is special and needs to be dealt with differently. Why wouldn't
these clocks be always on?
[1] https://lkml.org/lkml/2015/2/27/548
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Hi Mike,
On Mon, Mar 30, 2015 at 6:36 PM, Michael Turquette
mturque...@linaro.org wrote:
Quoting Andrew Bresticker (2015-03-30 17:15:43)
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
Hi Andrew,
Why are your drivers allowed to disable clocks which must not be
disabled? (you mentioned boot and runtime pm)
The issue is that they do not directly consume a critical clock, but
rather a descendant of the critical clock and thus could cause the
critical clock to be disabled. For example, the periph_sys clock is
one of these critical clocks and it is the parent of various
peripheral clocks, like the watchdog, I2C, PWM, etc. If the I2C
driver enables and disables it's clock during probe(), and nothing
else has caused the periph_sys clock to be enabled, the disable() call
will cause periph_sys to become disabled since its enable count drops
to 0.
Now this could be solved if there was a driver to directly consume
these clocks and keep them enabled, but, like Stephen mentioned, there
really isn't a proper driver for that. So it looks like I want
something like the always-on feature Lee is trying to introduce.
Thanks,
Andrew
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
On Mon, Mar 30, 2015 at 6:21 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 03/30/15 17:15, Andrew Bresticker wrote:
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
Ideally clock providers just provide clocks and don't actually call
clock consumer APIs. I don't see where these clocks are disabled in this
series. Is it just because suspend isn't done right now so there isn't a
place to hook the disable part? I hope that it's a 1:1 relation between
the clocks that are turned on here and the clocks that are turned off
during suspend.
Suspend hasn't been hooked up yet and it's still a long ways off.
I have a slightly similar problem on my hardware. Consider the case
where the bootloader has left on the display and audio clocks and they
share a common parent PLL. When the kernel boots up, all it knows is
that the display clock and audio clock share a common PLL and the rate
they're running at. If the audio driver probes first, calls clk_enable()
on the audio clock (almost a no-op except for the fact that we call the
.enable op when it's already on) and then calls clk_disable() on the
audio clock when it's done we'll also turn off the shared PLL.
Unfortunately it's also being used by the display clock for the display
driver that hasn't probed yet and so the display stops working and it
may show an artifact or black screen.
Other cases are where certain clocks should never be turned off because
they're used by some non-linux entity (dram controller for example) and
we don't have a place to put the clk_prepare_enable() besides in the
clock driver itself. In these cases, it may be better to tell the
framework that a clock should always be on. I think this case is what
Lee Jones is working on here[1].
Do you fall into one of these two cases? It isn't clear to me how
suspend is special and needs to be dealt with differently. Why wouldn't
these clocks be always on?
These clocks fall into the latter case in that there's really no good
place to put the clk_prepare_enable() calls, so it looks like I want
to use what Lee is proposing.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Quoting Andrew Bresticker (2015-03-30 17:15:43)
On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd sb...@codeaurora.org wrote:
On 02/24/15 19:56, Andrew Bresticker wrote:
+
+void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
+ unsigned int *clk_ids, unsigned int num)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i num; i++) {
+ struct clk *clk = p-clk_data.clks[clk_ids[i]];
+
+ if (IS_ERR(clk))
+ continue;
+
+ err = clk_prepare_enable(clk);
+ if (err)
+ pr_err(Failed to enable clock %s: %d\n,
+__clk_get_name(clk), err);
+ }
+}
Is this to workaround some problems in the framework where clocks are
turned off? Or is it that these clocks are already on before we boot
Linux and we need to make sure the framework knows that?
It's the former. These clocks are enabled at POR and may only be
gated as the final step to entering suspend, so they must remain on at
runtime. The issue we were running into was that consumers of these
critical clocks or their descendants would enable/disable their clocks
during boot or runtime PM and cause these clocks to get disabled.
Bumping up the prepare/enable count of these critical clocks seemed
like the best way to handle this - is there a more preferred way?
FWIW, this is also how the Tegra and Rockchip drivers handled this
problem.
Hi Andrew,
Why are your drivers allowed to disable clocks which must not be
disabled? (you mentioned boot and runtime pm)
Is this the case that a critical clock is not directly disabled, but a
parent of that critical clock is and it is gated as a result?
Regards,
Mike
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Add helpers for registering clocks and clock providers on Pistachio.
Signed-off-by: Andrew Bresticker
---
drivers/clk/Makefile | 1 +
drivers/clk/pistachio/Makefile | 1 +
drivers/clk/pistachio/clk.c| 140 +
drivers/clk/pistachio/clk.h| 124
4 files changed, 266 insertions(+)
create mode 100644 drivers/clk/pistachio/Makefile
create mode 100644 drivers/clk/pistachio/clk.c
create mode 100644 drivers/clk/pistachio/clk.h
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d478ceb..e43ff53 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_ARCH_MMP)+= mmp/
endif
obj-$(CONFIG_PLAT_ORION) += mvebu/
obj-$(CONFIG_ARCH_MXS) += mxs/
+obj-$(CONFIG_MACH_PISTACHIO) += pistachio/
obj-$(CONFIG_COMMON_CLK_PXA) += pxa/
obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/
obj-$(CONFIG_ARCH_ROCKCHIP)+= rockchip/
diff --git a/drivers/clk/pistachio/Makefile b/drivers/clk/pistachio/Makefile
new file mode 100644
index 000..fc216ad
--- /dev/null
+++ b/drivers/clk/pistachio/Makefile
@@ -0,0 +1 @@
+obj-y += clk.o
diff --git a/drivers/clk/pistachio/clk.c b/drivers/clk/pistachio/clk.c
new file mode 100644
index 000..85faa83
--- /dev/null
+++ b/drivers/clk/pistachio/clk.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "clk.h"
+
+struct pistachio_clk_provider *
+pistachio_clk_alloc_provider(struct device_node *node, unsigned int num_clks)
+{
+ struct pistachio_clk_provider *p;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p)
+ return p;
+
+ p->clk_data.clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
+ if (!p->clk_data.clks)
+ goto free_provider;
+ p->clk_data.clk_num = num_clks;
+ p->node = node;
+ p->base = of_iomap(node, 0);
+ if (!p->base) {
+ pr_err("Failed to map clock provider registers\n");
+ goto free_clks;
+ }
+
+ return p;
+
+free_clks:
+ kfree(p->clk_data.clks);
+free_provider:
+ kfree(p);
+ return NULL;
+}
+
+void pistachio_clk_register_provider(struct pistachio_clk_provider *p)
+{
+ unsigned int i;
+
+ for (i = 0; i < p->clk_data.clk_num; i++) {
+ if (IS_ERR(p->clk_data.clks[i]))
+ pr_warn("Failed to register clock %d: %ld\n", i,
+ PTR_ERR(p->clk_data.clks[i]));
+ }
+
+ of_clk_add_provider(p->node, of_clk_src_onecell_get, >clk_data);
+}
+
+void pistachio_clk_register_gate(struct pistachio_clk_provider *p,
+struct pistachio_gate *gate,
+unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i < num; i++) {
+ clk = clk_register_gate(NULL, gate[i].name, gate[i].parent,
+ CLK_SET_RATE_PARENT,
+ p->base + gate[i].reg, gate[i].shift,
+ 0, NULL);
+ p->clk_data.clks[gate[i].id] = clk;
+ }
+}
+
+void pistachio_clk_register_mux(struct pistachio_clk_provider *p,
+ struct pistachio_mux *mux,
+ unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i < num; i++) {
+ clk = clk_register_mux(NULL, mux[i].name, mux[i].parents,
+ mux[i].num_parents,
+ CLK_SET_RATE_NO_REPARENT,
+ p->base + mux[i].reg, mux[i].shift,
+ get_count_order(mux[i].num_parents),
+ 0, NULL);
+ p->clk_data.clks[mux[i].id] = clk;
+ }
+}
+
+void pistachio_clk_register_div(struct pistachio_clk_provider *p,
+ struct pistachio_div *div,
+ unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i < num; i++) {
+ clk = clk_register_divider(NULL, div[i].name, div[i].parent,
+ 0, p->base + div[i].reg, 0,
+ div[i].width, div[i].div_flags,
+ NULL);
+ p->clk_data.clks[div[i].id] = clk;
+ }
+}
+
+void pistachio_clk_register_fixed_factor(struct pistachio_clk_provider *p,
+
[PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks
Add helpers for registering clocks and clock providers on Pistachio.
Signed-off-by: Andrew Bresticker abres...@chromium.org
---
drivers/clk/Makefile | 1 +
drivers/clk/pistachio/Makefile | 1 +
drivers/clk/pistachio/clk.c| 140 +
drivers/clk/pistachio/clk.h| 124
4 files changed, 266 insertions(+)
create mode 100644 drivers/clk/pistachio/Makefile
create mode 100644 drivers/clk/pistachio/clk.c
create mode 100644 drivers/clk/pistachio/clk.h
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d478ceb..e43ff53 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_ARCH_MMP)+= mmp/
endif
obj-$(CONFIG_PLAT_ORION) += mvebu/
obj-$(CONFIG_ARCH_MXS) += mxs/
+obj-$(CONFIG_MACH_PISTACHIO) += pistachio/
obj-$(CONFIG_COMMON_CLK_PXA) += pxa/
obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/
obj-$(CONFIG_ARCH_ROCKCHIP)+= rockchip/
diff --git a/drivers/clk/pistachio/Makefile b/drivers/clk/pistachio/Makefile
new file mode 100644
index 000..fc216ad
--- /dev/null
+++ b/drivers/clk/pistachio/Makefile
@@ -0,0 +1 @@
+obj-y += clk.o
diff --git a/drivers/clk/pistachio/clk.c b/drivers/clk/pistachio/clk.c
new file mode 100644
index 000..85faa83
--- /dev/null
+++ b/drivers/clk/pistachio/clk.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include linux/clk-provider.h
+#include linux/kernel.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/slab.h
+
+#include clk.h
+
+struct pistachio_clk_provider *
+pistachio_clk_alloc_provider(struct device_node *node, unsigned int num_clks)
+{
+ struct pistachio_clk_provider *p;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p)
+ return p;
+
+ p-clk_data.clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
+ if (!p-clk_data.clks)
+ goto free_provider;
+ p-clk_data.clk_num = num_clks;
+ p-node = node;
+ p-base = of_iomap(node, 0);
+ if (!p-base) {
+ pr_err(Failed to map clock provider registers\n);
+ goto free_clks;
+ }
+
+ return p;
+
+free_clks:
+ kfree(p-clk_data.clks);
+free_provider:
+ kfree(p);
+ return NULL;
+}
+
+void pistachio_clk_register_provider(struct pistachio_clk_provider *p)
+{
+ unsigned int i;
+
+ for (i = 0; i p-clk_data.clk_num; i++) {
+ if (IS_ERR(p-clk_data.clks[i]))
+ pr_warn(Failed to register clock %d: %ld\n, i,
+ PTR_ERR(p-clk_data.clks[i]));
+ }
+
+ of_clk_add_provider(p-node, of_clk_src_onecell_get, p-clk_data);
+}
+
+void pistachio_clk_register_gate(struct pistachio_clk_provider *p,
+struct pistachio_gate *gate,
+unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ clk = clk_register_gate(NULL, gate[i].name, gate[i].parent,
+ CLK_SET_RATE_PARENT,
+ p-base + gate[i].reg, gate[i].shift,
+ 0, NULL);
+ p-clk_data.clks[gate[i].id] = clk;
+ }
+}
+
+void pistachio_clk_register_mux(struct pistachio_clk_provider *p,
+ struct pistachio_mux *mux,
+ unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ clk = clk_register_mux(NULL, mux[i].name, mux[i].parents,
+ mux[i].num_parents,
+ CLK_SET_RATE_NO_REPARENT,
+ p-base + mux[i].reg, mux[i].shift,
+ get_count_order(mux[i].num_parents),
+ 0, NULL);
+ p-clk_data.clks[mux[i].id] = clk;
+ }
+}
+
+void pistachio_clk_register_div(struct pistachio_clk_provider *p,
+ struct pistachio_div *div,
+ unsigned int num)
+{
+ struct clk *clk;
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ clk = clk_register_divider(NULL, div[i].name, div[i].parent,
+ 0, p-base + div[i].reg, 0,
+ div[i].width, div[i].div_flags,
+ NULL);
+ p-clk_data.clks[div[i].id] = clk;
+ }
+}
+
+void
