Re: [RFC v.4] omap: hwspinlock: Added hwspinlock driver

["Cousson, Benoit" <b-cousson@ti.com>]

On 9/20/2010 7:30 PM, Kevin Hilman wrote:
> Ohad Ben-Cohen<ohad@wizery.com>  writes:
>
>> Hi Hari,
>>
>> On Thu, Aug 12, 2010 at 12:44 AM, Kanigeri, Hari<h-kanigeri2@ti.com>  wrote:
>>>>> +/* Attempt to acquire a spinlock once */
>>>>> +int hwspinlock_trylock(struct hwspinlock *handle)
>>>>> +{
>>>>> +       int retval = 0;
>>>>> +
>>>>> +       if (WARN_ON(handle == NULL))
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       if (WARN_ON(in_irq()))
>>>>> +               return -EPERM;
>>>>> +
>>>>> +       if (pm_runtime_get(&handle->pdev->dev)<  0)
>>>>> +               return -ENODEV;
>>>>> +
>>>>> +       /* Attempt to acquire the lock by reading from it */
>>>>> +       retval = readl(handle->lock_reg);
>>>>> +
>>>>> +       if (retval == HWSPINLOCK_BUSY)
>>>>> +               pm_runtime_put(&handle->pdev->dev);
>>>> Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
>>>>
>>>> Using pm_runtime_gett_sync&  pm_runtime_put_sync have been recommended by
>>>> Kevin Hilman.
>>>
>>> Actually is there a need to call pm_runtime_put_sync for hwspinlock ? Spinlocks are used by the co-processors and we have to ensure that the device doesn't enter some low power mode without the knowledge of Co-processor. I don't think run time PM is needed for hwspinlock.
>>>
>>> Just doing pm_runtime_get_sync at probe time for all the spinlock instances should be good.
>>
>>
>> It would probably make more sense to call pm_runtime_get_sync during
>> hwspinlock_request{_specific}, and then call pm_runtime_put during
>> hwspinlock_free.
>>
>> This way the runtime PM's usage_count will reflect the number of locks
>> that are actually used, and if that number drops to (or never go
>> beyond) zero, it is desirable to have the hwspinlock's clock disabled.
>>
>> This is also safe since no other core will use the hwspinlock if it
>> wasn't requested by the MPU beforehand (and if it does, we better know
>> about it and fix it).
>
> FWIW, I agree with Ohad.
>
> An additional benefit of using runtime PM is that the runtime PM core is
> growing some useful debug and statistics features so that userspace
> tools (including newer versions of powertop) can present useful stats
> about which devices are active and how often etc.

And I agree with both of you.

Just to explain the context to Hari. IP like hwspinlock or mailbox does 
not require functional clock to work. Because of that you can use it 
without explicit clock enable thanks to the PRCM automatic modes.

So in theory you do not have to enable / idle hwmod for each spinlock 
request or even during the probe, and this is what the original patches 
from Simon were doing. I asked Simon to add explicit pm_runtime call, 
even if useless, because of the reason mentioned by Kevin, but also 
because the driver should not assume any automatic mode in the HW. That 
IP can be used in other SoC that will not have PRCM at all.
In our case these calls will be mostly NOP, but that's still needed.

Regards,
Benoit

>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html