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

[Kevin Hilman <khilman@deeprootsystems.com>]

"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