On the gta04 with a dm3730 omap_hdq does not work properly when the
device enters lower power states. Idling uart1 and 2 is enough
to show up that problem, if there are no other things enabled.
Further research reveals that hdq iclk must not be turned off during
transfers, also according to the TRM. That fact is also correctly described
in the flags but the code to handle that is incomplete.
To handle multiple users of a single ick, autoidle is disabled
when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))
Changes since the RFC version:
- mutex lock for autoidle changes
- deny_idle/allow_idle calls moved to clock enable/disable of the
individual modules
Andreas Kemnade (2):
clk: ti: add a usecount for autoidle
arm: omap_hwmod disable ick autoidling when a hwmod requires that
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++--------
include/linux/clk/ti.h | 1 +
3 files changed, 37 insertions(+), 12 deletions(-)
--
2.11.0
Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
that makes hwmods working properly which cannot handle
autoidle properly in lower power states.
Affected is e. g. the omap_hdq.
Since an ick might have mulitple users, autoidle is disabled
when an individual user requires that rather than in
_setup_iclk_autoidle. dss_ick is an example for that.
Signed-off-by: Andreas Kemnade <[email protected]>
---
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index bb641e6c93d0..0078b0e1d242 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -986,8 +986,10 @@ static int _enable_clocks(struct omap_hwmod *oh)
clk_enable(oh->_clk);
list_for_each_entry(os, &oh->slave_ports, node) {
- if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
+ if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
+ omap2_clk_deny_idle(os->_clk);
clk_enable(os->_clk);
+ }
}
/* The opt clocks are controlled by the device driver. */
@@ -1039,8 +1041,10 @@ static int _disable_clocks(struct omap_hwmod *oh)
clk_disable(oh->_clk);
list_for_each_entry(os, &oh->slave_ports, node) {
- if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
+ if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
clk_disable(os->_clk);
+ omap2_clk_allow_idle(os->_clk);
+ }
}
if (oh->flags & HWMOD_OPT_CLKS_NEEDED)
@@ -2410,9 +2414,13 @@ static void __init _setup_iclk_autoidle(struct omap_hwmod *oh)
continue;
if (os->flags & OCPIF_SWSUP_IDLE) {
- /* XXX omap_iclk_deny_idle(c); */
+ /*
+ * we might have multiple users of one iclk with
+ * different requirements, disable autoidle when
+ * the module is enabled, e.g. dss iclk
+ */
} else {
- /* XXX omap_iclk_allow_idle(c); */
+ /* we are enabling autoidle afterwards anyways */
clk_enable(os->_clk);
}
}
--
2.11.0
We have the scenario that first autoidle is disabled for all clocks,
then it is disabled for selected ones and then enabled for all. So
we should have some counting here, also according to the
comment in _setup_iclk_autoidle()
Signed-off-by: Andreas Kemnade <[email protected]>
---
drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++--------
include/linux/clk/ti.h | 1 +
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 7bb9afbe4058..bb6cff168e73 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -37,6 +37,14 @@ struct clk_ti_autoidle {
static LIST_HEAD(autoidle_clks);
static LIST_HEAD(clk_hw_omap_clocks);
+/*
+ * we have some non-atomic read/write
+ * operations behind it, so lets
+ * take one mutex for handling autoidle
+ * of all clocks
+ */
+static DEFINE_MUTEX(autoidle_mutex);
+
/**
* omap2_clk_deny_idle - disable autoidle on an OMAP clock
* @clk: struct clk * to disable autoidle for
@@ -48,8 +56,13 @@ int omap2_clk_deny_idle(struct clk *clk)
struct clk_hw_omap *c;
c = to_clk_hw_omap(__clk_get_hw(clk));
- if (c->ops && c->ops->deny_idle)
- c->ops->deny_idle(c);
+ if (c->ops && c->ops->deny_idle) {
+ mutex_lock(&autoidle_mutex);
+ c->autoidle_count--;
+ if (c->autoidle_count == -1)
+ c->ops->deny_idle(c);
+ mutex_unlock(&autoidle_mutex);
+ }
return 0;
}
@@ -64,8 +77,13 @@ int omap2_clk_allow_idle(struct clk *clk)
struct clk_hw_omap *c;
c = to_clk_hw_omap(__clk_get_hw(clk));
- if (c->ops && c->ops->allow_idle)
- c->ops->allow_idle(c);
+ if (c->ops && c->ops->allow_idle) {
+ mutex_lock(&autoidle_mutex);
+ c->autoidle_count++;
+ if (c->autoidle_count == 0)
+ c->ops->allow_idle(c);
+ mutex_unlock(&autoidle_mutex);
+ }
return 0;
}
@@ -201,8 +219,7 @@ int omap2_clk_enable_autoidle_all(void)
struct clk_hw_omap *c;
list_for_each_entry(c, &clk_hw_omap_clocks, node)
- if (c->ops && c->ops->allow_idle)
- c->ops->allow_idle(c);
+ omap2_clk_allow_idle(c->hw.clk);
_clk_generic_allow_autoidle_all();
@@ -223,8 +240,7 @@ int omap2_clk_disable_autoidle_all(void)
struct clk_hw_omap *c;
list_for_each_entry(c, &clk_hw_omap_clocks, node)
- if (c->ops && c->ops->deny_idle)
- c->ops->deny_idle(c);
+ omap2_clk_deny_idle(c->hw.clk);
_clk_generic_deny_autoidle_all();
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index a8faa38b1ed6..c460355419c0 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -159,6 +159,7 @@ struct clk_hw_omap {
const char *clkdm_name;
struct clockdomain *clkdm;
const struct clk_hw_omap_ops *ops;
+ int autoidle_count;
};
/*
--
2.11.0
ping.
..after stumbling again about that problem during testing with 4.20-rc1.
will retest it there.
On Thu, 4 Oct 2018 22:38:15 +0200
Andreas Kemnade <[email protected]> wrote:
> On the gta04 with a dm3730 omap_hdq does not work properly when the
> device enters lower power states. Idling uart1 and 2 is enough
> to show up that problem, if there are no other things enabled.
> Further research reveals that hdq iclk must not be turned off during
> transfers, also according to the TRM. That fact is also correctly described
> in the flags but the code to handle that is incomplete.
>
> To handle multiple users of a single ick, autoidle is disabled
> when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))
>
> Changes since the RFC version:
> - mutex lock for autoidle changes
> - deny_idle/allow_idle calls moved to clock enable/disable of the
> individual modules
>
> Andreas Kemnade (2):
> clk: ti: add a usecount for autoidle
> arm: omap_hwmod disable ick autoidling when a hwmod requires that
>
> arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
> drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++--------
> include/linux/clk/ti.h | 1 +
> 3 files changed, 37 insertions(+), 12 deletions(-)
>
> --
> 2.11.0
>
>
On 04/10/2018 23:38, Andreas Kemnade wrote:
> Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
> that makes hwmods working properly which cannot handle
> autoidle properly in lower power states.
> Affected is e. g. the omap_hdq.
> Since an ick might have mulitple users, autoidle is disabled
> when an individual user requires that rather than in
> _setup_iclk_autoidle. dss_ick is an example for that.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index bb641e6c93d0..0078b0e1d242 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -986,8 +986,10 @@ static int _enable_clocks(struct omap_hwmod *oh)
> clk_enable(oh->_clk);
>
> list_for_each_entry(os, &oh->slave_ports, node) {
> - if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
> + if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
> + omap2_clk_deny_idle(os->_clk);
I think calling this unconditionally across all platforms / clock types
might cause problems. Checking kernel, am33xx seems to have one clock
with this flag that is not of omap2 type. Do we have any testing data
that this doesn't break things?
-Tero
> clk_enable(os->_clk);
> + }
> }
>
> /* The opt clocks are controlled by the device driver. */
> @@ -1039,8 +1041,10 @@ static int _disable_clocks(struct omap_hwmod *oh)
> clk_disable(oh->_clk);
>
> list_for_each_entry(os, &oh->slave_ports, node) {
> - if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
> + if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
> clk_disable(os->_clk);
> + omap2_clk_allow_idle(os->_clk);
> + }
> }
>
> if (oh->flags & HWMOD_OPT_CLKS_NEEDED)
> @@ -2410,9 +2414,13 @@ static void __init _setup_iclk_autoidle(struct omap_hwmod *oh)
> continue;
>
> if (os->flags & OCPIF_SWSUP_IDLE) {
> - /* XXX omap_iclk_deny_idle(c); */
> + /*
> + * we might have multiple users of one iclk with
> + * different requirements, disable autoidle when
> + * the module is enabled, e.g. dss iclk
> + */
> } else {
> - /* XXX omap_iclk_allow_idle(c); */
> + /* we are enabling autoidle afterwards anyways */
> clk_enable(os->_clk);
> > }
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 04/10/2018 23:38, Andreas Kemnade wrote:
> We have the scenario that first autoidle is disabled for all clocks,
> then it is disabled for selected ones and then enabled for all. So
> we should have some counting here, also according to the
> comment in _setup_iclk_autoidle()
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++--------
> include/linux/clk/ti.h | 1 +
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> index 7bb9afbe4058..bb6cff168e73 100644
> --- a/drivers/clk/ti/autoidle.c
> +++ b/drivers/clk/ti/autoidle.c
> @@ -37,6 +37,14 @@ struct clk_ti_autoidle {
> static LIST_HEAD(autoidle_clks);
> static LIST_HEAD(clk_hw_omap_clocks);
>
> +/*
> + * we have some non-atomic read/write
> + * operations behind it, so lets
> + * take one mutex for handling autoidle
> + * of all clocks
> + */
> +static DEFINE_MUTEX(autoidle_mutex);
Why mutex? This prevents calling the autoidle APIs from atomic context.
Did you check the mutex debug kernel configs with this?
This may cause problems with the runtime PM entries to the code at least.
> +
> /**
> * omap2_clk_deny_idle - disable autoidle on an OMAP clock
> * @clk: struct clk * to disable autoidle for
> @@ -48,8 +56,13 @@ int omap2_clk_deny_idle(struct clk *clk)
> struct clk_hw_omap *c;
>
> c = to_clk_hw_omap(__clk_get_hw(clk));
> - if (c->ops && c->ops->deny_idle)
> - c->ops->deny_idle(c);
> + if (c->ops && c->ops->deny_idle) {
> + mutex_lock(&autoidle_mutex);
> + c->autoidle_count--;
> + if (c->autoidle_count == -1)
I think you should swap the arithmetics here, all the other usecounters
use positive values, here you enter deep to the negative side when
autoidle is denied by multiple users, which might be confusing.
-Tero
> + c->ops->deny_idle(c);
> + mutex_unlock(&autoidle_mutex);
> + }
> return 0;
> }
>
> @@ -64,8 +77,13 @@ int omap2_clk_allow_idle(struct clk *clk)
> struct clk_hw_omap *c;
>
> c = to_clk_hw_omap(__clk_get_hw(clk));
> - if (c->ops && c->ops->allow_idle)
> - c->ops->allow_idle(c);
> + if (c->ops && c->ops->allow_idle) {
> + mutex_lock(&autoidle_mutex);
> + c->autoidle_count++;
> + if (c->autoidle_count == 0)
> + c->ops->allow_idle(c);
> + mutex_unlock(&autoidle_mutex);
> + }
> return 0;
> }
>
> @@ -201,8 +219,7 @@ int omap2_clk_enable_autoidle_all(void)
> struct clk_hw_omap *c;
>
> list_for_each_entry(c, &clk_hw_omap_clocks, node)
> - if (c->ops && c->ops->allow_idle)
> - c->ops->allow_idle(c);
> + omap2_clk_allow_idle(c->hw.clk);
>
> _clk_generic_allow_autoidle_all();
>
> @@ -223,8 +240,7 @@ int omap2_clk_disable_autoidle_all(void)
> struct clk_hw_omap *c;
>
> list_for_each_entry(c, &clk_hw_omap_clocks, node)
> - if (c->ops && c->ops->deny_idle)
> - c->ops->deny_idle(c);
> + omap2_clk_deny_idle(c->hw.clk);
>
> _clk_generic_deny_autoidle_all();
>
> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
> index a8faa38b1ed6..c460355419c0 100644
> --- a/include/linux/clk/ti.h
> +++ b/include/linux/clk/ti.h
> @@ -159,6 +159,7 @@ struct clk_hw_omap {
> const char *clkdm_name;
> struct clockdomain *clkdm;
> const struct clk_hw_omap_ops *ops;
> + int autoidle_count;
> };
>
> /*
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi,
On Thu, 8 Nov 2018 12:26:08 +0200
Tero Kristo <[email protected]> wrote:
> On 04/10/2018 23:38, Andreas Kemnade wrote:
> > Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
> > that makes hwmods working properly which cannot handle
> > autoidle properly in lower power states.
> > Affected is e. g. the omap_hdq.
> > Since an ick might have mulitple users, autoidle is disabled
> > when an individual user requires that rather than in
> > _setup_iclk_autoidle. dss_ick is an example for that.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> > index bb641e6c93d0..0078b0e1d242 100644
> > --- a/arch/arm/mach-omap2/omap_hwmod.c
> > +++ b/arch/arm/mach-omap2/omap_hwmod.c
> > @@ -986,8 +986,10 @@ static int _enable_clocks(struct omap_hwmod *oh)
> > clk_enable(oh->_clk);
> >
> > list_for_each_entry(os, &oh->slave_ports, node) {
> > - if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
> > + if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
> > + omap2_clk_deny_idle(os->_clk);
>
> I think calling this unconditionally across all platforms / clock types
> might cause problems. Checking kernel, am33xx seems to have one clock
> with this flag that is not of omap2 type. Do we have any testing data
> that this doesn't break things?
>
Somehow I have missed that am33xx clock. I have not tested it on that
platform. Concerning am3xxx I have only a beaglebone block but it not
am33xx I guess. So I have to recheck I guess.
But I think the intention of this flag was to control autoidle
vs. sw-controlled idle according to...
[...]
> > if (os->flags & OCPIF_SWSUP_IDLE) {
> > - /* XXX omap_iclk_deny_idle(c); */
> > + /*
this comment. So we need a if (clock_is_omap2()) or something like that
or remove that flag from any other clocks?
Regards,
Andreas
On 08/11/2018 13:08, Andreas Kemnade wrote:
> Hi,
>
> On Thu, 8 Nov 2018 12:26:08 +0200
> Tero Kristo <[email protected]> wrote:
>
>> On 04/10/2018 23:38, Andreas Kemnade wrote:
>>> Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
>>> that makes hwmods working properly which cannot handle
>>> autoidle properly in lower power states.
>>> Affected is e. g. the omap_hdq.
>>> Since an ick might have mulitple users, autoidle is disabled
>>> when an individual user requires that rather than in
>>> _setup_iclk_autoidle. dss_ick is an example for that.
>>>
>>> Signed-off-by: Andreas Kemnade <[email protected]>
>>> ---
>>> arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>>> index bb641e6c93d0..0078b0e1d242 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>> @@ -986,8 +986,10 @@ static int _enable_clocks(struct omap_hwmod *oh)
>>> clk_enable(oh->_clk);
>>>
>>> list_for_each_entry(os, &oh->slave_ports, node) {
>>> - if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
>>> + if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
>>> + omap2_clk_deny_idle(os->_clk);
>>
>> I think calling this unconditionally across all platforms / clock types
>> might cause problems. Checking kernel, am33xx seems to have one clock
>> with this flag that is not of omap2 type. Do we have any testing data
>> that this doesn't break things?
>>
> Somehow I have missed that am33xx clock. I have not tested it on that
> platform. Concerning am3xxx I have only a beaglebone block but it not
> am33xx I guess. So I have to recheck I guess.
Beaglebone black is am33xx based device, so you can use that for testing.
> But I think the intention of this flag was to control autoidle
> vs. sw-controlled idle according to...
> [...]
>>> if (os->flags & OCPIF_SWSUP_IDLE) {
>>> - /* XXX omap_iclk_deny_idle(c); */
>>> + /*
> this comment. So we need a if (clock_is_omap2()) or something like that
> or remove that flag from any other clocks?
You could add check within the omap2_clk_deny_idle / allow_idle calls
themselves. You can check for presence of flag CLK_IS_BASIC, this is not
set for omap clocks.
-Tero
>
> Regards,
> Andreas
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Thu, 8 Nov 2018 12:36:35 +0200
Tero Kristo <[email protected]> wrote:
> On 04/10/2018 23:38, Andreas Kemnade wrote:
> > We have the scenario that first autoidle is disabled for all clocks,
> > then it is disabled for selected ones and then enabled for all. So
> > we should have some counting here, also according to the
> > comment in _setup_iclk_autoidle()
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++--------
> > include/linux/clk/ti.h | 1 +
> > 2 files changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> > index 7bb9afbe4058..bb6cff168e73 100644
> > --- a/drivers/clk/ti/autoidle.c
> > +++ b/drivers/clk/ti/autoidle.c
> > @@ -37,6 +37,14 @@ struct clk_ti_autoidle {
> > static LIST_HEAD(autoidle_clks);
> > static LIST_HEAD(clk_hw_omap_clocks);
> >
> > +/*
> > + * we have some non-atomic read/write
> > + * operations behind it, so lets
> > + * take one mutex for handling autoidle
> > + * of all clocks
> > + */
> > +static DEFINE_MUTEX(autoidle_mutex);
>
> Why mutex? This prevents calling the autoidle APIs from atomic context.
> Did you check the mutex debug kernel configs with this?
>
Oops, I thought they were on, but they were not. OK,
I am preparing a v2 of this thing.
> This may cause problems with the runtime PM entries to the code at least.
>
>
> > +
> > /**
> > * omap2_clk_deny_idle - disable autoidle on an OMAP clock
> > * @clk: struct clk * to disable autoidle for
> > @@ -48,8 +56,13 @@ int omap2_clk_deny_idle(struct clk *clk)
> > struct clk_hw_omap *c;
> >
> > c = to_clk_hw_omap(__clk_get_hw(clk));
> > - if (c->ops && c->ops->deny_idle)
> > - c->ops->deny_idle(c);
> > + if (c->ops && c->ops->deny_idle) {
> > + mutex_lock(&autoidle_mutex);
> > + c->autoidle_count--;
> > + if (c->autoidle_count == -1)
>
> I think you should swap the arithmetics here, all the other usecounters
> use positive values, here you enter deep to the negative side when
> autoidle is denied by multiple users, which might be confusing.
>
agreed.
Regards,
Andreas