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

[Kevin Hilman <khilman@deeprootsystems.com>]

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.

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