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 v1:
- uses spinlocks instead of mutexes
- invert counter logic
- check whether clock type is basic
Andreas Kemnade (3):
clk: ti: add a usecount for autoidle
clk: ti: check clock type before doing autoidle ops
arm: omap_hwmod disable ick autoidling when a hwmod requires that
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++----
drivers/clk/ti/autoidle.c | 48 +++++++++++++++++++++++++++++++---------
include/linux/clk/ti.h | 1 +
3 files changed, 51 insertions(+), 14 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]>
---
Comments to v1 to this patch were worked into a new 2/3
---
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 083dcd9942ce..3a86ba414973 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1002,8 +1002,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. */
@@ -1055,8 +1057,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)
@@ -2425,9 +2429,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
Multiple users might deny autoidle on a clock. So we should have some
counting here, also according to the comment in _setup_iclk_autoidle().
Also setting autoidle regs is not atomic, so there is another reason
for locking.
Signed-off-by: Andreas Kemnade <[email protected]>
---
Changes since v1:
- use spinlocks instead of mutexes
- invert logic
---
drivers/clk/ti/autoidle.c | 36 ++++++++++++++++++++++++++++--------
include/linux/clk/ti.h | 1 +
2 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 7bb9afbe4058..161f67850393 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 lock for handling autoidle
+ * of all clocks
+ */
+static DEFINE_SPINLOCK(autoidle_spinlock);
+
/**
* omap2_clk_deny_idle - disable autoidle on an OMAP clock
* @clk: struct clk * to disable autoidle for
@@ -48,8 +56,15 @@ 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) {
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&autoidle_spinlock, irqflags);
+ c->autoidle_count++;
+ if (c->autoidle_count == 1)
+ c->ops->deny_idle(c);
+ spin_unlock_irqrestore(&autoidle_spinlock, irqflags);
+ }
return 0;
}
@@ -64,8 +79,15 @@ 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) {
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&autoidle_spinlock, irqflags);
+ c->autoidle_count--;
+ if (c->autoidle_count == 0)
+ c->ops->allow_idle(c);
+ spin_unlock_irqrestore(&autoidle_spinlock, irqflags);
+ }
return 0;
}
@@ -201,8 +223,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 +244,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 eacc5df57b99..78872efc7be0 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -160,6 +160,7 @@ struct clk_hw_omap {
struct clockdomain *clkdm;
const struct clk_hw_omap_ops *ops;
u32 context;
+ int autoidle_count;
};
/*
--
2.11.0
Code might use autoidle api with clocks not being omap2 clocks,
so check if clock type is not basic
Signed-off-by: Andreas Kemnade <[email protected]>
---
New in v2
---
drivers/clk/ti/autoidle.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 161f67850393..5bdae5552d38 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
int omap2_clk_deny_idle(struct clk *clk)
{
struct clk_hw_omap *c;
+ struct clk_hw *hw = __clk_get_hw(clk);
- c = to_clk_hw_omap(__clk_get_hw(clk));
+ if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
+ return -EINVAL;
+
+ c = to_clk_hw_omap(hw);
if (c->ops && c->ops->deny_idle) {
unsigned long irqflags;
@@ -77,8 +81,12 @@ int omap2_clk_deny_idle(struct clk *clk)
int omap2_clk_allow_idle(struct clk *clk)
{
struct clk_hw_omap *c;
+ struct clk_hw *hw = __clk_get_hw(clk);
- c = to_clk_hw_omap(__clk_get_hw(clk));
+ if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
+ return -EINVAL;
+
+ c = to_clk_hw_omap(hw);
if (c->ops && c->ops->allow_idle) {
unsigned long irqflags;
--
2.11.0
Quoting Andreas Kemnade (2018-11-10 12:31:14)
> Code might use autoidle api with clocks not being omap2 clocks,
> so check if clock type is not basic
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> New in v2
> ---
> drivers/clk/ti/autoidle.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> index 161f67850393..5bdae5552d38 100644
> --- a/drivers/clk/ti/autoidle.c
> +++ b/drivers/clk/ti/autoidle.c
> @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
> int omap2_clk_deny_idle(struct clk *clk)
> {
> struct clk_hw_omap *c;
> + struct clk_hw *hw = __clk_get_hw(clk);
>
> - c = to_clk_hw_omap(__clk_get_hw(clk));
> + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
Maybe add some flag in clk_hw_omap() instead?
Quoting Andreas Kemnade (2018-11-10 12:31:12)
> 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 v1:
> - uses spinlocks instead of mutexes
> - invert counter logic
> - check whether clock type is basic
>
I'm expecting someone like Tero or Tony to review this.
Hi Stephen,
On Thu, 29 Nov 2018 16:25:05 -0800
Stephen Boyd <[email protected]> wrote:
> Quoting Andreas Kemnade (2018-11-10 12:31:14)
> > Code might use autoidle api with clocks not being omap2 clocks,
> > so check if clock type is not basic
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > New in v2
> > ---
> > drivers/clk/ti/autoidle.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> > index 161f67850393..5bdae5552d38 100644
> > --- a/drivers/clk/ti/autoidle.c
> > +++ b/drivers/clk/ti/autoidle.c
> > @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
> > int omap2_clk_deny_idle(struct clk *clk)
> > {
> > struct clk_hw_omap *c;
> > + struct clk_hw *hw = __clk_get_hw(clk);
> >
> > - c = to_clk_hw_omap(__clk_get_hw(clk));
> > + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
>
> Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
> Maybe add some flag in clk_hw_omap() instead?
>
hmm, Tero suggested that.
But to check flags in clk_hw_omap I first need to know that there is a
clk_hw_omap behind clk_hw. And for that I either need to check flags in
clk_hw or do more changes in the omap_hwmod code.
Regards,
Andreas
Quoting Andreas Kemnade (2018-11-29 22:15:34)
> Hi Stephen,
>
> On Thu, 29 Nov 2018 16:25:05 -0800
> Stephen Boyd <[email protected]> wrote:
>
> > Quoting Andreas Kemnade (2018-11-10 12:31:14)
> > > Code might use autoidle api with clocks not being omap2 clocks,
> > > so check if clock type is not basic
> > >
> > > Signed-off-by: Andreas Kemnade <[email protected]>
> > > ---
> > > New in v2
> > > ---
> > > drivers/clk/ti/autoidle.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> > > index 161f67850393..5bdae5552d38 100644
> > > --- a/drivers/clk/ti/autoidle.c
> > > +++ b/drivers/clk/ti/autoidle.c
> > > @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
> > > int omap2_clk_deny_idle(struct clk *clk)
> > > {
> > > struct clk_hw_omap *c;
> > > + struct clk_hw *hw = __clk_get_hw(clk);
> > >
> > > - c = to_clk_hw_omap(__clk_get_hw(clk));
> > > + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
> >
> > Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
> > Maybe add some flag in clk_hw_omap() instead?
> >
> hmm, Tero suggested that.
> But to check flags in clk_hw_omap I first need to know that there is a
> clk_hw_omap behind clk_hw. And for that I either need to check flags in
> clk_hw or do more changes in the omap_hwmod code.
Can you do it? The omap code is the only user of CLK_IS_BASIC. All the
other users are marking clks with this but there is no reason to do so.
I'll go make another pass over the tree and nuke those ones from orbit.
On 30/11/2018 09:20, Stephen Boyd wrote:
> Quoting Andreas Kemnade (2018-11-29 22:15:34)
>> Hi Stephen,
>>
>> On Thu, 29 Nov 2018 16:25:05 -0800
>> Stephen Boyd <[email protected]> wrote:
>>
>>> Quoting Andreas Kemnade (2018-11-10 12:31:14)
>>>> Code might use autoidle api with clocks not being omap2 clocks,
>>>> so check if clock type is not basic
>>>>
>>>> Signed-off-by: Andreas Kemnade <[email protected]>
>>>> ---
>>>> New in v2
>>>> ---
>>>> drivers/clk/ti/autoidle.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
>>>> index 161f67850393..5bdae5552d38 100644
>>>> --- a/drivers/clk/ti/autoidle.c
>>>> +++ b/drivers/clk/ti/autoidle.c
>>>> @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
>>>> int omap2_clk_deny_idle(struct clk *clk)
>>>> {
>>>> struct clk_hw_omap *c;
>>>> + struct clk_hw *hw = __clk_get_hw(clk);
>>>>
>>>> - c = to_clk_hw_omap(__clk_get_hw(clk));
>>>> + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
>>>
>>> Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
>>> Maybe add some flag in clk_hw_omap() instead?
>>>
>> hmm, Tero suggested that.
>> But to check flags in clk_hw_omap I first need to know that there is a
>> clk_hw_omap behind clk_hw. And for that I either need to check flags in
>> clk_hw or do more changes in the omap_hwmod code.
>
> Can you do it? The omap code is the only user of CLK_IS_BASIC. All the
> other users are marking clks with this but there is no reason to do so.
> I'll go make another pass over the tree and nuke those ones from orbit.
The reason for using this flag is because OMAP uses two clock types
around, the basic clocks like fixed-factor-clock/fixed-clock, and then
all the omap derivatives, which can be cast to clk_hw_omap. If we want
to avoid usage of CLK_IS_BASIC, we need to copy paste the remaining
basic code under drivers/clk/ti/ and convert them to use clk_hw_omap as
internal datatype. Is this preferred?
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 30/11/2018 02:26, Stephen Boyd wrote:
> Quoting Andreas Kemnade (2018-11-10 12:31:12)
>> 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 v1:
>> - uses spinlocks instead of mutexes
>> - invert counter logic
>> - check whether clock type is basic
>>
>
> I'm expecting someone like Tero or Tony to review this.
>
Rest of it looks fine to me, except for the discussion under the
CLK_IS_BASIC flag, which might trigger a bigger rework of the code.
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Quoting Tero Kristo (2018-11-29 23:35:35)
> On 30/11/2018 09:20, Stephen Boyd wrote:
> > Quoting Andreas Kemnade (2018-11-29 22:15:34)
> >> Hi Stephen,
> >>
> >> On Thu, 29 Nov 2018 16:25:05 -0800
> >> Stephen Boyd <[email protected]> wrote:
> >>
> >>> Quoting Andreas Kemnade (2018-11-10 12:31:14)
> >>>> Code might use autoidle api with clocks not being omap2 clocks,
> >>>> so check if clock type is not basic
> >>>>
> >>>> Signed-off-by: Andreas Kemnade <[email protected]>
> >>>> ---
> >>>> New in v2
> >>>> ---
> >>>> drivers/clk/ti/autoidle.c | 12 ++++++++++--
> >>>> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> >>>> index 161f67850393..5bdae5552d38 100644
> >>>> --- a/drivers/clk/ti/autoidle.c
> >>>> +++ b/drivers/clk/ti/autoidle.c
> >>>> @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
> >>>> int omap2_clk_deny_idle(struct clk *clk)
> >>>> {
> >>>> struct clk_hw_omap *c;
> >>>> + struct clk_hw *hw = __clk_get_hw(clk);
> >>>>
> >>>> - c = to_clk_hw_omap(__clk_get_hw(clk));
> >>>> + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
> >>>
> >>> Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
> >>> Maybe add some flag in clk_hw_omap() instead?
> >>>
> >> hmm, Tero suggested that.
> >> But to check flags in clk_hw_omap I first need to know that there is a
> >> clk_hw_omap behind clk_hw. And for that I either need to check flags in
> >> clk_hw or do more changes in the omap_hwmod code.
> >
> > Can you do it? The omap code is the only user of CLK_IS_BASIC. All the
> > other users are marking clks with this but there is no reason to do so.
> > I'll go make another pass over the tree and nuke those ones from orbit.
>
> The reason for using this flag is because OMAP uses two clock types
> around, the basic clocks like fixed-factor-clock/fixed-clock, and then
> all the omap derivatives, which can be cast to clk_hw_omap. If we want
> to avoid usage of CLK_IS_BASIC, we need to copy paste the remaining
> basic code under drivers/clk/ti/ and convert them to use clk_hw_omap as
> internal datatype. Is this preferred?
>
No that is not preferred. Can the omap2_clk_deny_idle() function be
integrated closer into the clk framework in some way that allows it to
be part of the clk_ops structure? And then have that take a clk_hw
structure instead of a struct clk? I haven't looked at this in any
detail whatsoever so I may be way off right now.
Quoting Tero Kristo (2018-11-29 23:37:35)
> On 30/11/2018 02:26, Stephen Boyd wrote:
> > Quoting Andreas Kemnade (2018-11-10 12:31:12)
> >> 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 v1:
> >> - uses spinlocks instead of mutexes
> >> - invert counter logic
> >> - check whether clock type is basic
> >>
> >
> > I'm expecting someone like Tero or Tony to review this.
> >
>
> Rest of it looks fine to me, except for the discussion under the
> CLK_IS_BASIC flag, which might trigger a bigger rework of the code.
>
Is that a Reviewed-by tag?
On 30/11/2018 09:57, Stephen Boyd wrote:
> Quoting Tero Kristo (2018-11-29 23:35:35)
>> On 30/11/2018 09:20, Stephen Boyd wrote:
>>> Quoting Andreas Kemnade (2018-11-29 22:15:34)
>>>> Hi Stephen,
>>>>
>>>> On Thu, 29 Nov 2018 16:25:05 -0800
>>>> Stephen Boyd <[email protected]> wrote:
>>>>
>>>>> Quoting Andreas Kemnade (2018-11-10 12:31:14)
>>>>>> Code might use autoidle api with clocks not being omap2 clocks,
>>>>>> so check if clock type is not basic
>>>>>>
>>>>>> Signed-off-by: Andreas Kemnade <[email protected]>
>>>>>> ---
>>>>>> New in v2
>>>>>> ---
>>>>>> drivers/clk/ti/autoidle.c | 12 ++++++++++--
>>>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
>>>>>> index 161f67850393..5bdae5552d38 100644
>>>>>> --- a/drivers/clk/ti/autoidle.c
>>>>>> +++ b/drivers/clk/ti/autoidle.c
>>>>>> @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
>>>>>> int omap2_clk_deny_idle(struct clk *clk)
>>>>>> {
>>>>>> struct clk_hw_omap *c;
>>>>>> + struct clk_hw *hw = __clk_get_hw(clk);
>>>>>>
>>>>>> - c = to_clk_hw_omap(__clk_get_hw(clk));
>>>>>> + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
>>>>>
>>>>> Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
>>>>> Maybe add some flag in clk_hw_omap() instead?
>>>>>
>>>> hmm, Tero suggested that.
>>>> But to check flags in clk_hw_omap I first need to know that there is a
>>>> clk_hw_omap behind clk_hw. And for that I either need to check flags in
>>>> clk_hw or do more changes in the omap_hwmod code.
>>>
>>> Can you do it? The omap code is the only user of CLK_IS_BASIC. All the
>>> other users are marking clks with this but there is no reason to do so.
>>> I'll go make another pass over the tree and nuke those ones from orbit.
>>
>> The reason for using this flag is because OMAP uses two clock types
>> around, the basic clocks like fixed-factor-clock/fixed-clock, and then
>> all the omap derivatives, which can be cast to clk_hw_omap. If we want
>> to avoid usage of CLK_IS_BASIC, we need to copy paste the remaining
>> basic code under drivers/clk/ti/ and convert them to use clk_hw_omap as
>> internal datatype. Is this preferred?
>>
>
> No that is not preferred. Can the omap2_clk_deny_idle() function be
> integrated closer into the clk framework in some way that allows it to
> be part of the clk_ops structure? And then have that take a clk_hw
> structure instead of a struct clk? I haven't looked at this in any
> detail whatsoever so I may be way off right now.
It could be added under the main clk_ops struct, however this would
introduce two new func pointers to it which are not used by anything
else but OMAP. Are you aware of any other platforms requiring similar
feature?
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 30/11/2018 09:57, Stephen Boyd wrote:
> Quoting Tero Kristo (2018-11-29 23:37:35)
>> On 30/11/2018 02:26, Stephen Boyd wrote:
>>> Quoting Andreas Kemnade (2018-11-10 12:31:12)
>>>> 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 v1:
>>>> - uses spinlocks instead of mutexes
>>>> - invert counter logic
>>>> - check whether clock type is basic
>>>>
>>>
>>> I'm expecting someone like Tero or Tony to review this.
>>>
>>
>> Rest of it looks fine to me, except for the discussion under the
>> CLK_IS_BASIC flag, which might trigger a bigger rework of the code.
>>
>
> Is that a Reviewed-by tag?
>
Not yet, lets see where discussion ends up with patch #2. :)
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero,
On Fri, 30 Nov 2018 11:20:49 +0200
Tero Kristo <[email protected]> wrote:
> On 30/11/2018 09:57, Stephen Boyd wrote:
> > Quoting Tero Kristo (2018-11-29 23:35:35)
> >> On 30/11/2018 09:20, Stephen Boyd wrote:
> >>> Quoting Andreas Kemnade (2018-11-29 22:15:34)
> >>>> Hi Stephen,
> >>>>
> >>>> On Thu, 29 Nov 2018 16:25:05 -0800
> >>>> Stephen Boyd <[email protected]> wrote:
> >>>>
> >>>>> Quoting Andreas Kemnade (2018-11-10 12:31:14)
> >>>>>> Code might use autoidle api with clocks not being omap2 clocks,
> >>>>>> so check if clock type is not basic
> >>>>>>
> >>>>>> Signed-off-by: Andreas Kemnade <[email protected]>
> >>>>>> ---
> >>>>>> New in v2
> >>>>>> ---
> >>>>>> drivers/clk/ti/autoidle.c | 12 ++++++++++--
> >>>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> >>>>>> index 161f67850393..5bdae5552d38 100644
> >>>>>> --- a/drivers/clk/ti/autoidle.c
> >>>>>> +++ b/drivers/clk/ti/autoidle.c
> >>>>>> @@ -54,8 +54,12 @@ static DEFINE_SPINLOCK(autoidle_spinlock);
> >>>>>> int omap2_clk_deny_idle(struct clk *clk)
> >>>>>> {
> >>>>>> struct clk_hw_omap *c;
> >>>>>> + struct clk_hw *hw = __clk_get_hw(clk);
> >>>>>>
> >>>>>> - c = to_clk_hw_omap(__clk_get_hw(clk));
> >>>>>> + if (clk_hw_get_flags(hw) & CLK_IS_BASIC)
> >>>>>
> >>>>> Please try to avoid using CLK_IS_BASIC if at all possible. Can you?
> >>>>> Maybe add some flag in clk_hw_omap() instead?
> >>>>>
> >>>> hmm, Tero suggested that.
> >>>> But to check flags in clk_hw_omap I first need to know that there is a
> >>>> clk_hw_omap behind clk_hw. And for that I either need to check flags in
> >>>> clk_hw or do more changes in the omap_hwmod code.
> >>>
> >>> Can you do it? The omap code is the only user of CLK_IS_BASIC. All the
> >>> other users are marking clks with this but there is no reason to do so.
> >>> I'll go make another pass over the tree and nuke those ones from orbit.
> >>
> >> The reason for using this flag is because OMAP uses two clock types
> >> around, the basic clocks like fixed-factor-clock/fixed-clock, and then
> >> all the omap derivatives, which can be cast to clk_hw_omap. If we want
> >> to avoid usage of CLK_IS_BASIC, we need to copy paste the remaining
> >> basic code under drivers/clk/ti/ and convert them to use clk_hw_omap as
> >> internal datatype. Is this preferred?
> >>
> >
> > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > integrated closer into the clk framework in some way that allows it to
> > be part of the clk_ops structure? And then have that take a clk_hw
> > structure instead of a struct clk? I haven't looked at this in any
> > detail whatsoever so I may be way off right now.
>
> It could be added under the main clk_ops struct, however this would
> introduce two new func pointers to it which are not used by anything
> else but OMAP. Are you aware of any other platforms requiring similar
> feature?
The question here is also how we organize the procedure here. One patchset
fixing nasty problems and another mainly reorganize things? Where do we draw
the line between these two? If we have the autoidle code in main clk_ops,
we could also think whether other autoidle code should go into main code.
Regards,
Andreas
Hi,
* Tero Kristo <[email protected]> [181130 09:21]:
> On 30/11/2018 09:57, Stephen Boyd wrote:
> > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > integrated closer into the clk framework in some way that allows it to
> > be part of the clk_ops structure? And then have that take a clk_hw
> > structure instead of a struct clk? I haven't looked at this in any
> > detail whatsoever so I may be way off right now.
>
> It could be added under the main clk_ops struct, however this would
> introduce two new func pointers to it which are not used by anything else
> but OMAP. Are you aware of any other platforms requiring similar feature?
From consumer usage point of view, I'm still wondering about
the relationship of clk_deny_idle() and clkdm_deny_idle().
It seems that we need to allow reset control drivers call
clk_deny_idle() for the duration of reset. And it seems the
clk_deny_idle() should propagate to also up to the related
clock domain driver to do clkdm_deny_idle().
So maybe clk_deny_idle() is could just be something like:
dev = clk_get_device(clk);
...
error = pm_runtime_get(dev);
...
pm_runtime_put(dev);
...
And that way it would just propagate to the parent clock
domain driver and the clock framework does not need to know
about clockdomains. A clockdomain could be just a genpd
domain.
Or do you guys have better ideas?
Regards,
Tony
Quoting Tony Lindgren (2018-11-30 07:37:29)
> Hi,
>
> * Tero Kristo <[email protected]> [181130 09:21]:
> > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > > integrated closer into the clk framework in some way that allows it to
> > > be part of the clk_ops structure? And then have that take a clk_hw
> > > structure instead of a struct clk? I haven't looked at this in any
> > > detail whatsoever so I may be way off right now.
> >
> > It could be added under the main clk_ops struct, however this would
> > introduce two new func pointers to it which are not used by anything else
> > but OMAP. Are you aware of any other platforms requiring similar feature?
>
> From consumer usage point of view, I'm still wondering about
> the relationship of clk_deny_idle() and clkdm_deny_idle().
>
> It seems that we need to allow reset control drivers call
> clk_deny_idle() for the duration of reset. And it seems the
> clk_deny_idle() should propagate to also up to the related
> clock domain driver to do clkdm_deny_idle().
>
> So maybe clk_deny_idle() is could just be something like:
>
> dev = clk_get_device(clk);
> ...
> error = pm_runtime_get(dev);
> ...
> pm_runtime_put(dev);
> ...
>
> And that way it would just propagate to the parent clock
> domain driver and the clock framework does not need to know
> about clockdomains. A clockdomain could be just a genpd
> domain.
>
> Or do you guys have better ideas?
>
Wouldn't the device link in clk framework patches do this for you if we
had the RUNTIME_PM flag passed in. If this is about keeping the clock
controller active when a consumer device is using it then I think it may
work.
* Stephen Boyd <[email protected]> [181130 23:52]:
> Quoting Tony Lindgren (2018-11-30 07:37:29)
> > Hi,
> >
> > * Tero Kristo <[email protected]> [181130 09:21]:
> > > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > > > integrated closer into the clk framework in some way that allows it to
> > > > be part of the clk_ops structure? And then have that take a clk_hw
> > > > structure instead of a struct clk? I haven't looked at this in any
> > > > detail whatsoever so I may be way off right now.
> > >
> > > It could be added under the main clk_ops struct, however this would
> > > introduce two new func pointers to it which are not used by anything else
> > > but OMAP. Are you aware of any other platforms requiring similar feature?
> >
> > From consumer usage point of view, I'm still wondering about
> > the relationship of clk_deny_idle() and clkdm_deny_idle().
> >
> > It seems that we need to allow reset control drivers call
> > clk_deny_idle() for the duration of reset. And it seems the
> > clk_deny_idle() should propagate to also up to the related
> > clock domain driver to do clkdm_deny_idle().
> >
> > So maybe clk_deny_idle() is could just be something like:
> >
> > dev = clk_get_device(clk);
> > ...
> > error = pm_runtime_get(dev);
> > ...
> > pm_runtime_put(dev);
> > ...
> >
> > And that way it would just propagate to the parent clock
> > domain driver and the clock framework does not need to know
> > about clockdomains. A clockdomain could be just a genpd
> > domain.
> >
> > Or do you guys have better ideas?
> >
>
> Wouldn't the device link in clk framework patches do this for you if we
> had the RUNTIME_PM flag passed in. If this is about keeping the clock
> controller active when a consumer device is using it then I think it may
> work.
The consumer device stays active just fine with PM runtime
calls. So yes, the problem is keeping a clock controller forced
active for the period of consumer device reset. Other than
that typically autoidle can be just kept enabled.
Below is a clarified suggested example usage if we wanted to
use PM runtime on a clock controller device from a consumer
device reset driver:
error = pm_runtime_get_dev()
...
cdev = clk_get_device(clk);
...
error = pm_runtime_get(cdev);
...
/* Do the consumer device reset here */
...
pm_runtime_put(cdev);
pm_runtime_put(dev);
Regards,
Tony
Quoting Tony Lindgren (2018-12-03 07:39:10)
> * Stephen Boyd <[email protected]> [181130 23:52]:
> > Quoting Tony Lindgren (2018-11-30 07:37:29)
> > > * Tero Kristo <[email protected]> [181130 09:21]:
> > > > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > ...
> > >
> > > And that way it would just propagate to the parent clock
> > > domain driver and the clock framework does not need to know
> > > about clockdomains. A clockdomain could be just a genpd
> > > domain.
> > >
> > > Or do you guys have better ideas?
> > >
> >
> > Wouldn't the device link in clk framework patches do this for you if we
> > had the RUNTIME_PM flag passed in. If this is about keeping the clock
> > controller active when a consumer device is using it then I think it may
> > work.
>
> The consumer device stays active just fine with PM runtime
> calls. So yes, the problem is keeping a clock controller forced
> active for the period of consumer device reset. Other than
> that typically autoidle can be just kept enabled.
>
> Below is a clarified suggested example usage if we wanted to
> use PM runtime on a clock controller device from a consumer
> device reset driver:
>
> error = pm_runtime_get_dev()
> ...
> cdev = clk_get_device(clk);
> ...
> error = pm_runtime_get(cdev);
> ...
> /* Do the consumer device reset here */
> ...
> pm_runtime_put(cdev);
> pm_runtime_put(dev);
>
Does the consumer reset use the reset framework or something else? If
the driver is using the reset framework, I would expect the reset
framework to _also_ have device links and keep the clock controller,
i.e. reset provider, active while the reset is being toggled. And this
assumes the reset controller and clock controller code is all rolled up
together in a single driver that can tell itself to deny idle for
certain clks that are associated with whatever resets they affect.
On Mon, 3 Dec 2018 07:39:10 -0800
Tony Lindgren <[email protected]> wrote:
> * Stephen Boyd <[email protected]> [181130 23:52]:
> > Quoting Tony Lindgren (2018-11-30 07:37:29)
> > > Hi,
> > >
> > > * Tero Kristo <[email protected]> [181130 09:21]:
> > > > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > > > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > > > > integrated closer into the clk framework in some way that allows it to
> > > > > be part of the clk_ops structure? And then have that take a clk_hw
> > > > > structure instead of a struct clk? I haven't looked at this in any
> > > > > detail whatsoever so I may be way off right now.
> > > >
> > > > It could be added under the main clk_ops struct, however this would
> > > > introduce two new func pointers to it which are not used by anything else
> > > > but OMAP. Are you aware of any other platforms requiring similar feature?
> > >
> > > From consumer usage point of view, I'm still wondering about
> > > the relationship of clk_deny_idle() and clkdm_deny_idle().
> > >
> > > It seems that we need to allow reset control drivers call
> > > clk_deny_idle() for the duration of reset. And it seems the
> > > clk_deny_idle() should propagate to also up to the related
> > > clock domain driver to do clkdm_deny_idle().
> > >
> > > So maybe clk_deny_idle() is could just be something like:
> > >
> > > dev = clk_get_device(clk);
> > > ...
> > > error = pm_runtime_get(dev);
> > > ...
> > > pm_runtime_put(dev);
> > > ...
> > >
> > > And that way it would just propagate to the parent clock
> > > domain driver and the clock framework does not need to know
> > > about clockdomains. A clockdomain could be just a genpd
> > > domain.
> > >
> > > Or do you guys have better ideas?
> > >
> >
> > Wouldn't the device link in clk framework patches do this for you if we
> > had the RUNTIME_PM flag passed in. If this is about keeping the clock
> > controller active when a consumer device is using it then I think it may
> > work.
>
> The consumer device stays active just fine with PM runtime
> calls. So yes, the problem is keeping a clock controller forced
> active for the period of consumer device reset. Other than
> that typically autoidle can be just kept enabled.
>
Are we still talking about the same problem? Maybe I am losing track
here. Just to make sure.
The patch series was about disabling autoidle for devices which cannot
work with it during normal operation. Not during reset or something
like that.
Or is the keep-clock-active-during-reset just a requirement for bigger
restructuring ideas?
Regards,
Andreas
* Andreas Kemnade <[email protected]> [181204 06:17]:
> On Mon, 3 Dec 2018 07:39:10 -0800
> Tony Lindgren <[email protected]> wrote:
> > The consumer device stays active just fine with PM runtime
> > calls. So yes, the problem is keeping a clock controller forced
> > active for the period of consumer device reset. Other than
> > that typically autoidle can be just kept enabled.
> >
> Are we still talking about the same problem? Maybe I am losing track
> here. Just to make sure.
> The patch series was about disabling autoidle for devices which cannot
> work with it during normal operation. Not during reset or something
> like that.
> Or is the keep-clock-active-during-reset just a requirement for bigger
> restructuring ideas?
Yeah there are two issues: The fix needed for the issue you brought up,
and also how to let a reset driver to block autoidle for reset.
Regards,
Tony
Hi,
On Tue, 4 Dec 2018 08:45:57 -0800
Tony Lindgren <[email protected]> wrote:
> * Andreas Kemnade <[email protected]> [181204 06:17]:
> > On Mon, 3 Dec 2018 07:39:10 -0800
> > Tony Lindgren <[email protected]> wrote:
> > > The consumer device stays active just fine with PM runtime
> > > calls. So yes, the problem is keeping a clock controller forced
> > > active for the period of consumer device reset. Other than
> > > that typically autoidle can be just kept enabled.
> > >
> > Are we still talking about the same problem? Maybe I am losing track
> > here. Just to make sure.
> > The patch series was about disabling autoidle for devices which cannot
> > work with it during normal operation. Not during reset or something
> > like that.
> > Or is the keep-clock-active-during-reset just a requirement for bigger
> > restructuring ideas?
>
> Yeah there are two issues: The fix needed for the issue you brought up,
> and also how to let a reset driver to block autoidle for reset.
>
Hmm, is this set now waiting for the famous "somebody" fixing all
the stuff?
What are currently visible symptoms for the driver not blocking
autoidle for reset? Maybe I can at least test something there. I have
also omap5 here.
Regards,
Andreas
* Andreas Kemnade <[email protected]> [181227 20:13]:
> Hi,
>
> On Tue, 4 Dec 2018 08:45:57 -0800
> Tony Lindgren <[email protected]> wrote:
>
> > * Andreas Kemnade <[email protected]> [181204 06:17]:
> > > On Mon, 3 Dec 2018 07:39:10 -0800
> > > Tony Lindgren <[email protected]> wrote:
> > > > The consumer device stays active just fine with PM runtime
> > > > calls. So yes, the problem is keeping a clock controller forced
> > > > active for the period of consumer device reset. Other than
> > > > that typically autoidle can be just kept enabled.
> > > >
> > > Are we still talking about the same problem? Maybe I am losing track
> > > here. Just to make sure.
> > > The patch series was about disabling autoidle for devices which cannot
> > > work with it during normal operation. Not during reset or something
> > > like that.
> > > Or is the keep-clock-active-during-reset just a requirement for bigger
> > > restructuring ideas?
> >
> > Yeah there are two issues: The fix needed for the issue you brought up,
> > and also how to let a reset driver to block autoidle for reset.
> >
> Hmm, is this set now waiting for the famous "somebody" fixing all
> the stuff?
Well I think we're still waiting on Tero to comment on this.
> What are currently visible symptoms for the driver not blocking
> autoidle for reset? Maybe I can at least test something there. I have
> also omap5 here.
Oh that's just for making drivers/reset drivers to work in
the long run. Let's keep that separate from these fixes..
Regards,
Tony
On 28/12/2018 22:02, Tony Lindgren wrote:
> * Andreas Kemnade <[email protected]> [181227 20:13]:
>> Hi,
>>
>> On Tue, 4 Dec 2018 08:45:57 -0800
>> Tony Lindgren <[email protected]> wrote:
>>
>>> * Andreas Kemnade <[email protected]> [181204 06:17]:
>>>> On Mon, 3 Dec 2018 07:39:10 -0800
>>>> Tony Lindgren <[email protected]> wrote:
>>>>> The consumer device stays active just fine with PM runtime
>>>>> calls. So yes, the problem is keeping a clock controller forced
>>>>> active for the period of consumer device reset. Other than
>>>>> that typically autoidle can be just kept enabled.
>>>>>
>>>> Are we still talking about the same problem? Maybe I am losing track
>>>> here. Just to make sure.
>>>> The patch series was about disabling autoidle for devices which cannot
>>>> work with it during normal operation. Not during reset or something
>>>> like that.
>>>> Or is the keep-clock-active-during-reset just a requirement for bigger
>>>> restructuring ideas?
>>>
>>> Yeah there are two issues: The fix needed for the issue you brought up,
>>> and also how to let a reset driver to block autoidle for reset.
>>>
>> Hmm, is this set now waiting for the famous "somebody" fixing all
>> the stuff?
>
> Well I think we're still waiting on Tero to comment on this.
The only item requiring immediate fixing is the point Stephen made out,
removing the usage of CLK_IS_BASIC from this patch.
Afaics, the reset related concerns Tony has can be handled later.
-Tero
>
>> What are currently visible symptoms for the driver not blocking
>> autoidle for reset? Maybe I can at least test something there. I have
>> also omap5 here.
>
> Oh that's just for making drivers/reset drivers to work in
> the long run. Let's keep that separate from these fixes..
>
> Regards,
>
> Tony
>
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, 31 Dec 2018 09:23:01 +0200
Tero Kristo <[email protected]> wrote:
> On 28/12/2018 22:02, Tony Lindgren wrote:
> > * Andreas Kemnade <[email protected]> [181227 20:13]:
> >> Hi,
> >>
> >> On Tue, 4 Dec 2018 08:45:57 -0800
> >> Tony Lindgren <[email protected]> wrote:
> >>
> >>> * Andreas Kemnade <[email protected]> [181204 06:17]:
> >>>> On Mon, 3 Dec 2018 07:39:10 -0800
> >>>> Tony Lindgren <[email protected]> wrote:
> >>>>> The consumer device stays active just fine with PM runtime
> >>>>> calls. So yes, the problem is keeping a clock controller forced
> >>>>> active for the period of consumer device reset. Other than
> >>>>> that typically autoidle can be just kept enabled.
> >>>>>
> >>>> Are we still talking about the same problem? Maybe I am losing track
> >>>> here. Just to make sure.
> >>>> The patch series was about disabling autoidle for devices which cannot
> >>>> work with it during normal operation. Not during reset or something
> >>>> like that.
> >>>> Or is the keep-clock-active-during-reset just a requirement for bigger
> >>>> restructuring ideas?
> >>>
> >>> Yeah there are two issues: The fix needed for the issue you brought up,
> >>> and also how to let a reset driver to block autoidle for reset.
> >>>
> >> Hmm, is this set now waiting for the famous "somebody" fixing all
> >> the stuff?
> >
> > Well I think we're still waiting on Tero to comment on this.
>
> The only item requiring immediate fixing is the point Stephen made out,
> removing the usage of CLK_IS_BASIC from this patch.
>
> Afaics, the reset related concerns Tony has can be handled later.
>
hmm, and there we need Stephen's opinion about having the allow/deny
autoidle functions in the main clk_ops struct.
Regards,
Andreas
Quoting Andreas Kemnade (2018-12-31 00:30:21)
> On Mon, 31 Dec 2018 09:23:01 +0200
> Tero Kristo <[email protected]> wrote:
>
> > On 28/12/2018 22:02, Tony Lindgren wrote:
> > > * Andreas Kemnade <[email protected]> [181227 20:13]:
> > >> Hi,
> > >>
> > >> On Tue, 4 Dec 2018 08:45:57 -0800
> > >> Tony Lindgren <[email protected]> wrote:
> > >>
> > >>> * Andreas Kemnade <[email protected]> [181204 06:17]:
> > >>>> On Mon, 3 Dec 2018 07:39:10 -0800
> > >>>> Tony Lindgren <[email protected]> wrote:
> > >>>>> The consumer device stays active just fine with PM runtime
> > >>>>> calls. So yes, the problem is keeping a clock controller forced
> > >>>>> active for the period of consumer device reset. Other than
> > >>>>> that typically autoidle can be just kept enabled.
> > >>>>>
> > >>>> Are we still talking about the same problem? Maybe I am losing track
> > >>>> here. Just to make sure.
> > >>>> The patch series was about disabling autoidle for devices which cannot
> > >>>> work with it during normal operation. Not during reset or something
> > >>>> like that.
> > >>>> Or is the keep-clock-active-during-reset just a requirement for bigger
> > >>>> restructuring ideas?
> > >>>
> > >>> Yeah there are two issues: The fix needed for the issue you brought up,
> > >>> and also how to let a reset driver to block autoidle for reset.
> > >>>
> > >> Hmm, is this set now waiting for the famous "somebody" fixing all
> > >> the stuff?
> > >
> > > Well I think we're still waiting on Tero to comment on this.
> >
> > The only item requiring immediate fixing is the point Stephen made out,
> > removing the usage of CLK_IS_BASIC from this patch.
> >
> > Afaics, the reset related concerns Tony has can be handled later.
> >
> hmm, and there we need Stephen's opinion about having the allow/deny
> autoidle functions in the main clk_ops struct.
>
I have unanswered questions on the list for this thread[1]. I'm not sure
what allow/deny autoidle functions mean to clk drivers. It looks like an
OMAP specific addition to the clk_ops struct, which sounds wrong to put
it plainly. Hopefully it can be done outside of the clk framework by
having the provider driver know more things about all the frameworks
it's hooking into.
[1] https://lkml.kernel.org/r/154385676593.88331.5239924154783168815@swboyd.mtv.corp.google.com
On 04/01/2019 01:39, Stephen Boyd wrote:
> Quoting Andreas Kemnade (2018-12-31 00:30:21)
>> On Mon, 31 Dec 2018 09:23:01 +0200
>> Tero Kristo <[email protected]> wrote:
>>
>>> On 28/12/2018 22:02, Tony Lindgren wrote:
>>>> * Andreas Kemnade <[email protected]> [181227 20:13]:
>>>>> Hi,
>>>>>
>>>>> On Tue, 4 Dec 2018 08:45:57 -0800
>>>>> Tony Lindgren <[email protected]> wrote:
>>>>>
>>>>>> * Andreas Kemnade <[email protected]> [181204 06:17]:
>>>>>>> On Mon, 3 Dec 2018 07:39:10 -0800
>>>>>>> Tony Lindgren <[email protected]> wrote:
>>>>>>>> The consumer device stays active just fine with PM runtime
>>>>>>>> calls. So yes, the problem is keeping a clock controller forced
>>>>>>>> active for the period of consumer device reset. Other than
>>>>>>>> that typically autoidle can be just kept enabled.
>>>>>>>>
>>>>>>> Are we still talking about the same problem? Maybe I am losing track
>>>>>>> here. Just to make sure.
>>>>>>> The patch series was about disabling autoidle for devices which cannot
>>>>>>> work with it during normal operation. Not during reset or something
>>>>>>> like that.
>>>>>>> Or is the keep-clock-active-during-reset just a requirement for bigger
>>>>>>> restructuring ideas?
>>>>>>
>>>>>> Yeah there are two issues: The fix needed for the issue you brought up,
>>>>>> and also how to let a reset driver to block autoidle for reset.
>>>>>>
>>>>> Hmm, is this set now waiting for the famous "somebody" fixing all
>>>>> the stuff?
>>>>
>>>> Well I think we're still waiting on Tero to comment on this.
>>>
>>> The only item requiring immediate fixing is the point Stephen made out,
>>> removing the usage of CLK_IS_BASIC from this patch.
>>>
>>> Afaics, the reset related concerns Tony has can be handled later.
>>>
>> hmm, and there we need Stephen's opinion about having the allow/deny
>> autoidle functions in the main clk_ops struct.
>>
>
> I have unanswered questions on the list for this thread[1].
The reset portion we can't answer with the current knowledge I fear, we
need to prototype this a bit first and see which way to go.
> I'm not sure
> what allow/deny autoidle functions mean to clk drivers. It looks like an
> OMAP specific addition to the clk_ops struct, which sounds wrong to put
> it plainly.
Yeah, I don't think other SoCs implement the same functionality, at
least not in the same way. The autoidle bits are available in
omap2/omap3 only, where they control the HW autoidle functionality of
these clocks. If the bit is enabled, the HW can autonomously disable the
clock once it is not needed anymore by HW.
> Hopefully it can be done outside of the clk framework by
> having the provider driver know more things about all the frameworks
> it's hooking into.
This is how it has been done so far, however Andreas wants to expand the
functionality a bit where it breaks... unless we can use the
CLK_IS_BASIC flag to detect if we accessing an OMAP specific clock or
not. If we are passing in a clk pointer from a consumer level API, I
don't know if there is any other way to go with this if we can't modify
the generic clk_ops struct.
The same flag check is used across TI clock driver already btw.
-Tero
>
> [1] https://lkml.kernel.org/r/154385676593.88331.5239924154783168815@swboyd.mtv.corp.google.com
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Quoting Tero Kristo (2019-01-03 23:28:58)
> On 04/01/2019 01:39, Stephen Boyd wrote:
> > Quoting Andreas Kemnade (2018-12-31 00:30:21)
> >> On Mon, 31 Dec 2018 09:23:01 +0200
> >> Tero Kristo <[email protected]> wrote:
> >>
> >>> On 28/12/2018 22:02, Tony Lindgren wrote:
> >>>> * Andreas Kemnade <[email protected]> [181227 20:13]:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, 4 Dec 2018 08:45:57 -0800
> >>>>> Tony Lindgren <[email protected]> wrote:
> >>>>>
> >>>>>> * Andreas Kemnade <[email protected]> [181204 06:17]:
> >>>>>>> On Mon, 3 Dec 2018 07:39:10 -0800
> >>>>>>> Tony Lindgren <[email protected]> wrote:
> >>>>>>>> The consumer device stays active just fine with PM runtime
> >>>>>>>> calls. So yes, the problem is keeping a clock controller forced
> >>>>>>>> active for the period of consumer device reset. Other than
> >>>>>>>> that typically autoidle can be just kept enabled.
> >>>>>>>>
> >>>>>>> Are we still talking about the same problem? Maybe I am losing track
> >>>>>>> here. Just to make sure.
> >>>>>>> The patch series was about disabling autoidle for devices which cannot
> >>>>>>> work with it during normal operation. Not during reset or something
> >>>>>>> like that.
> >>>>>>> Or is the keep-clock-active-during-reset just a requirement for bigger
> >>>>>>> restructuring ideas?
> >>>>>>
> >>>>>> Yeah there are two issues: The fix needed for the issue you brought up,
> >>>>>> and also how to let a reset driver to block autoidle for reset.
> >>>>>>
> >>>>> Hmm, is this set now waiting for the famous "somebody" fixing all
> >>>>> the stuff?
> >>>>
> >>>> Well I think we're still waiting on Tero to comment on this.
> >>>
> >>> The only item requiring immediate fixing is the point Stephen made out,
> >>> removing the usage of CLK_IS_BASIC from this patch.
> >>>
> >>> Afaics, the reset related concerns Tony has can be handled later.
> >>>
> >> hmm, and there we need Stephen's opinion about having the allow/deny
> >> autoidle functions in the main clk_ops struct.
> >>
> >
> > I have unanswered questions on the list for this thread[1].
>
> The reset portion we can't answer with the current knowledge I fear, we
> need to prototype this a bit first and see which way to go.
>
> > I'm not sure
> > what allow/deny autoidle functions mean to clk drivers. It looks like an
> > OMAP specific addition to the clk_ops struct, which sounds wrong to put
> > it plainly.
>
> Yeah, I don't think other SoCs implement the same functionality, at
> least not in the same way. The autoidle bits are available in
> omap2/omap3 only, where they control the HW autoidle functionality of
> these clocks. If the bit is enabled, the HW can autonomously disable the
> clock once it is not needed anymore by HW.
Some qcom chips have automatic clock gating (basically hw clk gating)
but they don't really need to involve that with the reset asserting or
deasserting anymore. It used to be that they had to turn off the
automatic mode, assert the reset, deassert the reset, and then reenable
the automatic mode. So there is some precedence for this. But again, I
think that the reset controller and the clk controller are the same
device in both vendor instances so in theory the driver can be one
driver for both clk and reset and do the proper things on the backend.
So just use reset controller framework and register that from the clk
controller driver?
>
> > Hopefully it can be done outside of the clk framework by
> > having the provider driver know more things about all the frameworks
> > it's hooking into.
>
> This is how it has been done so far, however Andreas wants to expand the
> functionality a bit where it breaks... unless we can use the
> CLK_IS_BASIC flag to detect if we accessing an OMAP specific clock or
> not. If we are passing in a clk pointer from a consumer level API, I
> don't know if there is any other way to go with this if we can't modify
> the generic clk_ops struct.
>
> The same flag check is used across TI clock driver already btw.
>
Sure, it's not like this is a new problem. I'd just like to see if we
can solve it now and get rid of the CLK_IS_BASIC flag now. It would be
great if some extra effort could be put into it vs. punting the problem
until 2020 or something.
On 12/01/2019 00:49, Stephen Boyd wrote:
> Quoting Tero Kristo (2019-01-03 23:28:58)
>> On 04/01/2019 01:39, Stephen Boyd wrote:
>>> Quoting Andreas Kemnade (2018-12-31 00:30:21)
>>>> On Mon, 31 Dec 2018 09:23:01 +0200
>>>> Tero Kristo <[email protected]> wrote:
>>>>
>>>>> On 28/12/2018 22:02, Tony Lindgren wrote:
>>>>>> * Andreas Kemnade <[email protected]> [181227 20:13]:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, 4 Dec 2018 08:45:57 -0800
>>>>>>> Tony Lindgren <[email protected]> wrote:
>>>>>>>
>>>>>>>> * Andreas Kemnade <[email protected]> [181204 06:17]:
>>>>>>>>> On Mon, 3 Dec 2018 07:39:10 -0800
>>>>>>>>> Tony Lindgren <[email protected]> wrote:
>>>>>>>>>> The consumer device stays active just fine with PM runtime
>>>>>>>>>> calls. So yes, the problem is keeping a clock controller forced
>>>>>>>>>> active for the period of consumer device reset. Other than
>>>>>>>>>> that typically autoidle can be just kept enabled.
>>>>>>>>>>
>>>>>>>>> Are we still talking about the same problem? Maybe I am losing track
>>>>>>>>> here. Just to make sure.
>>>>>>>>> The patch series was about disabling autoidle for devices which cannot
>>>>>>>>> work with it during normal operation. Not during reset or something
>>>>>>>>> like that.
>>>>>>>>> Or is the keep-clock-active-during-reset just a requirement for bigger
>>>>>>>>> restructuring ideas?
>>>>>>>>
>>>>>>>> Yeah there are two issues: The fix needed for the issue you brought up,
>>>>>>>> and also how to let a reset driver to block autoidle for reset.
>>>>>>>>
>>>>>>> Hmm, is this set now waiting for the famous "somebody" fixing all
>>>>>>> the stuff?
>>>>>>
>>>>>> Well I think we're still waiting on Tero to comment on this.
>>>>>
>>>>> The only item requiring immediate fixing is the point Stephen made out,
>>>>> removing the usage of CLK_IS_BASIC from this patch.
>>>>>
>>>>> Afaics, the reset related concerns Tony has can be handled later.
>>>>>
>>>> hmm, and there we need Stephen's opinion about having the allow/deny
>>>> autoidle functions in the main clk_ops struct.
>>>>
>>>
>>> I have unanswered questions on the list for this thread[1].
>>
>> The reset portion we can't answer with the current knowledge I fear, we
>> need to prototype this a bit first and see which way to go.
>>
>>> I'm not sure
>>> what allow/deny autoidle functions mean to clk drivers. It looks like an
>>> OMAP specific addition to the clk_ops struct, which sounds wrong to put
>>> it plainly.
>>
>> Yeah, I don't think other SoCs implement the same functionality, at
>> least not in the same way. The autoidle bits are available in
>> omap2/omap3 only, where they control the HW autoidle functionality of
>> these clocks. If the bit is enabled, the HW can autonomously disable the
>> clock once it is not needed anymore by HW.
>
> Some qcom chips have automatic clock gating (basically hw clk gating)
> but they don't really need to involve that with the reset asserting or
> deasserting anymore. It used to be that they had to turn off the
> automatic mode, assert the reset, deassert the reset, and then reenable
> the automatic mode. So there is some precedence for this. But again, I
> think that the reset controller and the clk controller are the same
> device in both vendor instances so in theory the driver can be one
> driver for both clk and reset and do the proper things on the backend.
> So just use reset controller framework and register that from the clk
> controller driver?
>
>>
>>> Hopefully it can be done outside of the clk framework by
>>> having the provider driver know more things about all the frameworks
>>> it's hooking into.
>>
>> This is how it has been done so far, however Andreas wants to expand the
>> functionality a bit where it breaks... unless we can use the
>> CLK_IS_BASIC flag to detect if we accessing an OMAP specific clock or
>> not. If we are passing in a clk pointer from a consumer level API, I
>> don't know if there is any other way to go with this if we can't modify
>> the generic clk_ops struct.
>>
>> The same flag check is used across TI clock driver already btw.
>>
>
> Sure, it's not like this is a new problem. I'd just like to see if we
> can solve it now and get rid of the CLK_IS_BASIC flag now. It would be
> great if some extra effort could be put into it vs. punting the problem
> until 2020 or something.
Ok, let me see if I can figure out something for this...
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki