[PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Rajendra Nayak]

Do not skip the sysc programming in the hmwod framework based
on the cached value alone, since at times the module might have lost
context (due to the Powerdomain in which the module belongs
transitions to either Open Switch RET or OFF).

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index cb911d7..e57fe21 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -115,7 +115,10 @@ static void _write_sysconfig(u32 v, struct omap_hwmod *oh)
 
 	/* XXX ensure module interface clock is up */
 
-	if (oh->_sysc_cache != v) {
+	if ((oh->_sysc_cache != v) ||
+		/* Did the module go to off and loose context? */
+		(oh->_sysc_cache !=
+			 omap_hwmod_readl(oh, oh->class->sysc->sysc_offs))) {
 		oh->_sysc_cache = v;
 		omap_hwmod_writel(v, oh, oh->class->sysc->sysc_offs);
 	}
-- 
1.7.0.4

Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Kevin Hilman]

Rajendra Nayak <rnayak@ti.com> writes:

> Do not skip the sysc programming in the hmwod framework based
> on the cached value alone, since at times the module might have lost
> context (due to the Powerdomain in which the module belongs
> transitions to either Open Switch RET or OFF).

Shouldn't the driver for each IP be responsible for restoring it's
register contents after context loss, including it's SYSC?

Seems to me that if SYSC is lost, it means the driver's save/restore 
is buggy.

Kevin

> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index cb911d7..e57fe21 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -115,7 +115,10 @@ static void _write_sysconfig(u32 v, struct omap_hwmod *oh)
>  
>  	/* XXX ensure module interface clock is up */
>  
> -	if (oh->_sysc_cache != v) {
> +	if ((oh->_sysc_cache != v) ||
> +		/* Did the module go to off and loose context? */
> +		(oh->_sysc_cache !=
> +			 omap_hwmod_readl(oh, oh->class->sysc->sysc_offs))) {
>  		oh->_sysc_cache = v;
>  		omap_hwmod_writel(v, oh, oh->class->sysc->sysc_offs);
>  	}

RE: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Shilimkar, Santosh]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Friday, October 15, 2010 3:44 AM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit; Shilimkar,
> Santosh
> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module
> context is lost
> 
> Rajendra Nayak <rnayak@ti.com> writes:
> 
> > Do not skip the sysc programming in the hmwod framework based
> > on the cached value alone, since at times the module might have lost
> > context (due to the Powerdomain in which the module belongs
> > transitions to either Open Switch RET or OFF).
> 
> Shouldn't the driver for each IP be responsible for restoring it's
> register contents after context loss, including it's SYSC?
> 
> Seems to me that if SYSC is lost, it means the driver's save/restore
> is buggy.
> 
I am glad you asked this question. I had a same argument with Benoit
that driver anyway does context save restore for other registers and
it can do SYSC as well.

But Benoit's point was that "sysconfig is a part of the PRCM located
in the IP, but this is purely TI implementation specific. The same
IP in another platform will not have this sysconfig entry. That's why
its important to hide them from the driver "

This make sense too.

> 
> > Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> > Cc: Benoit Cousson <b-cousson@ti.com>
> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Cc: Kevin Hilman <khilman@deeprootsystems.com>
> > ---
> >  arch/arm/mach-omap2/omap_hwmod.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
> omap2/omap_hwmod.c
> > index cb911d7..e57fe21 100644
> > --- a/arch/arm/mach-omap2/omap_hwmod.c
> > +++ b/arch/arm/mach-omap2/omap_hwmod.c
> > @@ -115,7 +115,10 @@ static void _write_sysconfig(u32 v, struct
> omap_hwmod *oh)
> >
> >  	/* XXX ensure module interface clock is up */
> >
> > -	if (oh->_sysc_cache != v) {
> > +	if ((oh->_sysc_cache != v) ||
> > +		/* Did the module go to off and loose context? */
> > +		(oh->_sysc_cache !=
> > +			 omap_hwmod_readl(oh, oh->class->sysc->sysc_offs))) {
> >  		oh->_sysc_cache = v;
> >  		omap_hwmod_writel(v, oh, oh->class->sysc->sysc_offs);
> >  	}

Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Cousson, Benoit]

Hi Santosh,

On 10/15/2010 7:48 AM, Shilimkar, Santosh wrote:
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Friday, October 15, 2010 3:44 AM
>>
>> Rajendra Nayak<rnayak@ti.com>  writes:
>>
>>> Do not skip the sysc programming in the hmwod framework based
>>> on the cached value alone, since at times the module might have lost
>>> context (due to the Powerdomain in which the module belongs
>>> transitions to either Open Switch RET or OFF).
>>
>> Shouldn't the driver for each IP be responsible for restoring it's
>> register contents after context loss, including it's SYSC?
>>
>> Seems to me that if SYSC is lost, it means the driver's save/restore
>> is buggy.
>>
> I am glad you asked this question. I had a same argument with Benoit
> that driver anyway does context save restore for other registers and
> it can do SYSC as well.
>
> But Benoit's point was that "sysconfig is a part of the PRCM located
> in the IP, but this is purely TI implementation specific. The same
> IP in another platform will not have this sysconfig entry. That's why
> its important to hide them from the driver "

I don't have anything to add :-)

Thanks Santosh,
Benoit

>
> This make sense too.
>
>>
>>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>>> Cc: Paul Walmsley<paul@pwsan.com>
>>> Cc: Benoit Cousson<b-cousson@ti.com>
>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> ---
>>>   arch/arm/mach-omap2/omap_hwmod.c |    5 ++++-
>>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
>> omap2/omap_hwmod.c
>>> index cb911d7..e57fe21 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>> @@ -115,7 +115,10 @@ static void _write_sysconfig(u32 v, struct
>> omap_hwmod *oh)
>>>
>>>   	/* XXX ensure module interface clock is up */
>>>
>>> -	if (oh->_sysc_cache != v) {
>>> +	if ((oh->_sysc_cache != v) ||
>>> +		/* Did the module go to off and loose context? */
>>> +		(oh->_sysc_cache !=
>>> +			 omap_hwmod_readl(oh, oh->class->sysc->sysc_offs))) {
>>>   		oh->_sysc_cache = v;
>>>   		omap_hwmod_writel(v, oh, oh->class->sysc->sysc_offs);
>>>   	}

Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Kevin Hilman]

"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Friday, October 15, 2010 3:44 AM
>> To: Nayak, Rajendra
>> Cc: linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit; Shilimkar,
>> Santosh
>> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module
>> context is lost
>> 
>> Rajendra Nayak <rnayak@ti.com> writes:
>> 
>> > Do not skip the sysc programming in the hmwod framework based
>> > on the cached value alone, since at times the module might have lost
>> > context (due to the Powerdomain in which the module belongs
>> > transitions to either Open Switch RET or OFF).
>> 
>> Shouldn't the driver for each IP be responsible for restoring it's
>> register contents after context loss, including it's SYSC?
>> 
>> Seems to me that if SYSC is lost, it means the driver's save/restore
>> is buggy.
> 
> I am glad you asked this question. I had a same argument with Benoit
> that driver anyway does context save restore for other registers and
> it can do SYSC as well.
>
> But Benoit's point was that "sysconfig is a part of the PRCM located
> in the IP, but this is purely TI implementation specific. The same
> IP in another platform will not have this sysconfig entry. That's why
> its important to hide them from the driver "

OK, but this patch still doesn't address the real problem.  Namely, that
*somebody* needs to save/restore the SYSC reg for the IP.  

Otherwise, all this patch does is refresh the _sysc_cache with
completely unknown contents.  It also somewhat defeats the purpose of
having a cache.  If you're going to read SYSC in order to determine
whether or not you can avoid a write, you might as well just blindly
write.

One option to fix the save/restore would be that TI specific context
save/restore could be done in the device layer by adding some additional
save/restore do the functions calling omap_device_[idle|enable].

IOW, most devices just call omap_device directly by doing something like:

struct omap_device_pm_latency omap_wdt_latency[] = {
	[0] = {
		.deactivate_func = omap_device_idle_hwmods,
		.activate_func   = omap_device_enable_hwmods,
		.flags		 = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
	},
};


But these [de]activate functions can be anything, and could be made into
functions that do some additional save/restore and then call
omap_device_*

Kevin

RE: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Nayak, Rajendra]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Friday, October 15, 2010 9:10 PM
> To: Shilimkar, Santosh
> Cc: Nayak, Rajendra; linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit
> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost
> 
> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> Sent: Friday, October 15, 2010 3:44 AM
> >> To: Nayak, Rajendra
> >> Cc: linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit; Shilimkar,
> >> Santosh
> >> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module
> >> context is lost
> >>
> >> Rajendra Nayak <rnayak@ti.com> writes:
> >>
> >> > Do not skip the sysc programming in the hmwod framework based
> >> > on the cached value alone, since at times the module might have lost
> >> > context (due to the Powerdomain in which the module belongs
> >> > transitions to either Open Switch RET or OFF).
> >>
> >> Shouldn't the driver for each IP be responsible for restoring it's
> >> register contents after context loss, including it's SYSC?
> >>
> >> Seems to me that if SYSC is lost, it means the driver's save/restore
> >> is buggy.
> >
> > I am glad you asked this question. I had a same argument with Benoit
> > that driver anyway does context save restore for other registers and
> > it can do SYSC as well.
> >
> > But Benoit's point was that "sysconfig is a part of the PRCM located
> > in the IP, but this is purely TI implementation specific. The same
> > IP in another platform will not have this sysconfig entry. That's why
> > its important to hide them from the driver "
> 
> OK, but this patch still doesn't address the real problem.  Namely, that
> *somebody* needs to save/restore the SYSC reg for the IP.

Hi Kevin,

What the patch does is reprogram's the sysc value (from the cache)
whenever its lost. So its infact saved in the cache and restored when 
needed. 

> 
> Otherwise, all this patch does is refresh the _sysc_cache with
> completely unknown contents.  It also somewhat defeats the purpose of
> having a cache.  If you're going to read SYSC in order to determine
> whether or not you can avoid a write, you might as well just blindly
> write.

I thought of this and dismissed it thinking I would end up with a read/or/write
and instead a read always to avoid write is better.
But now looking back again, it does make sense to still keep the cache to avoid
a read (since a read has significantly more latency than write) and do a blind write
always. Does that make sense?

Regards,
Rajendra

> 
> One option to fix the save/restore would be that TI specific context
> save/restore could be done in the device layer by adding some additional
> save/restore do the functions calling omap_device_[idle|enable].
> 
> IOW, most devices just call omap_device directly by doing something like:
> 
> struct omap_device_pm_latency omap_wdt_latency[] = {
> 	[0] = {
> 		.deactivate_func = omap_device_idle_hwmods,
> 		.activate_func   = omap_device_enable_hwmods,
> 		.flags		 = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> 	},
> };
> 
> 
> But these [de]activate functions can be anything, and could be made into
> functions that do some additional save/restore and then call
> omap_device_*
> 
> Kevin

Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Cousson, Benoit]

On 10/21/2010 12:13 PM, Nayak, Rajendra wrote:

<snip>

> Hi Kevin,
>
> What the patch does is reprogram's the sysc value (from the cache)
> whenever its lost. So its infact saved in the cache and restored when
> needed.
>
>>
>> Otherwise, all this patch does is refresh the _sysc_cache with
>> completely unknown contents.  It also somewhat defeats the purpose of
>> having a cache.  If you're going to read SYSC in order to determine
>> whether or not you can avoid a write, you might as well just blindly
>> write.
>
> I thought of this and dismissed it thinking I would end up with a read/or/write
> and instead a read always to avoid write is better.
> But now looking back again, it does make sense to still keep the cache to avoid
> a read (since a read has significantly more latency than write) and do a blind write
> always. Does that make sense?

That seems indeed better. The point is that cache is already a location 
for the "save" part.
So writing blindly will do the restore with always the good value.
It is anyway faster than trying to check if we lost context or not 
through PRM registers.

Benoit

Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost

[Kevin Hilman]

"Nayak, Rajendra" <rnayak@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Friday, October 15, 2010 9:10 PM
>> To: Shilimkar, Santosh
>> Cc: Nayak, Rajendra; linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit
>> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module context is lost
>> 
>> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> >> Sent: Friday, October 15, 2010 3:44 AM
>> >> To: Nayak, Rajendra
>> >> Cc: linux-omap@vger.kernel.org; Paul Walmsley; Cousson, Benoit; Shilimkar,
>> >> Santosh
>> >> Subject: Re: [PATCH] OMAP: hmwod: Update the sysc_cache in case module
>> >> context is lost
>> >>
>> >> Rajendra Nayak <rnayak@ti.com> writes:
>> >>
>> >> > Do not skip the sysc programming in the hmwod framework based
>> >> > on the cached value alone, since at times the module might have lost
>> >> > context (due to the Powerdomain in which the module belongs
>> >> > transitions to either Open Switch RET or OFF).
>> >>
>> >> Shouldn't the driver for each IP be responsible for restoring it's
>> >> register contents after context loss, including it's SYSC?
>> >>
>> >> Seems to me that if SYSC is lost, it means the driver's save/restore
>> >> is buggy.
>> >
>> > I am glad you asked this question. I had a same argument with Benoit
>> > that driver anyway does context save restore for other registers and
>> > it can do SYSC as well.
>> >
>> > But Benoit's point was that "sysconfig is a part of the PRCM located
>> > in the IP, but this is purely TI implementation specific. The same
>> > IP in another platform will not have this sysconfig entry. That's why
>> > its important to hide them from the driver "
>> 
>> OK, but this patch still doesn't address the real problem.  Namely, that
>> *somebody* needs to save/restore the SYSC reg for the IP.
>
> Hi Kevin,
>
> What the patch does is reprogram's the sysc value (from the cache)
> whenever its lost. So its infact saved in the cache and restored when 
> needed. 
>
>> 
>> Otherwise, all this patch does is refresh the _sysc_cache with
>> completely unknown contents.  It also somewhat defeats the purpose of
>> having a cache.  If you're going to read SYSC in order to determine
>> whether or not you can avoid a write, you might as well just blindly
>> write.
>
> I thought of this and dismissed it thinking I would end up with a read/or/write
> and instead a read always to avoid write is better.
> But now looking back again, it does make sense to still keep the cache to avoid
> a read (since a read has significantly more latency than write) and do a blind write
> always. Does that make sense?

Makes sense to me.

Kevin