| Message ID | 1389658764-39199-7-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 13, 2014 at 06:19:23PM -0600, Suman Anna wrote: > The number of hwspinlocks are determined based on the value read > from the IP block's SYSSTATUS register. However, the module may > not be enabled and clocked, and the read may result in a bus error. > > This particular issue is seen rather easily on AM33XX, since the > module wakeup is software controlled, and it is disabled out of > reset. Make sure the module is enabled and clocked before reading > the SYSSTATUS register. > > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > drivers/hwspinlock/omap_hwspinlock.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c > index 9f56fb2..194886e 100644 > --- a/drivers/hwspinlock/omap_hwspinlock.c > +++ b/drivers/hwspinlock/omap_hwspinlock.c > @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) > if (!io_base) > return -ENOMEM; > > + /* > + * make sure the module is enabled and clocked before reading > + * the module SYSSTATUS register > + */ > + pm_runtime_enable(&pdev->dev); > + pm_runtime_get_sync(&pdev->dev); > + > /* Determine number of locks */ > i = readl(io_base + SYSSTATUS_OFFSET); > i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET; > > + /* > + * runtime PM will make sure the clock of this module is > + * enabled again iff at least one lock is requested > + */ > + pm_runtime_put(&pdev->dev); there is a small race here (which was already present previously) where you could return from probe() in a failure case before your PM runtime put request then you would disable pm_runtime while the device was still alive.
Hi again, On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote: > > diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c > > index 9f56fb2..194886e 100644 > > --- a/drivers/hwspinlock/omap_hwspinlock.c > > +++ b/drivers/hwspinlock/omap_hwspinlock.c > > @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) > > if (!io_base) > > return -ENOMEM; > > > > + /* > > + * make sure the module is enabled and clocked before reading > > + * the module SYSSTATUS register > > + */ > > + pm_runtime_enable(&pdev->dev); > > + pm_runtime_get_sync(&pdev->dev); another thing, you need to check return of pm_runtime_get_sync()
Felipe, On 01/14/2014 08:04 AM, Felipe Balbi wrote: > Hi again, > > On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote: >>> diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c >>> index 9f56fb2..194886e 100644 >>> --- a/drivers/hwspinlock/omap_hwspinlock.c >>> +++ b/drivers/hwspinlock/omap_hwspinlock.c >>> @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) >>> if (!io_base) >>> return -ENOMEM; >>> >>> + /* >>> + * make sure the module is enabled and clocked before reading >>> + * the module SYSSTATUS register >>> + */ >>> + pm_runtime_enable(&pdev->dev); >>> + pm_runtime_get_sync(&pdev->dev); > > another thing, you need to check return of pm_runtime_get_sync() OK, let me check this and your other comment, and the fix is probably a separate patch. regards Suman
Felipe, > > On 01/14/2014 08:04 AM, Felipe Balbi wrote: >> Hi again, >> >> On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote: >>>> diff --git a/drivers/hwspinlock/omap_hwspinlock.c >>>> b/drivers/hwspinlock/omap_hwspinlock.c >>>> index 9f56fb2..194886e 100644 >>>> --- a/drivers/hwspinlock/omap_hwspinlock.c >>>> +++ b/drivers/hwspinlock/omap_hwspinlock.c >>>> @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct >>>> platform_device *pdev) >>>> if (!io_base) >>>> return -ENOMEM; >>>> >>>> + /* >>>> + * make sure the module is enabled and clocked before reading >>>> + * the module SYSSTATUS register >>>> + */ >>>> + pm_runtime_enable(&pdev->dev); >>>> + pm_runtime_get_sync(&pdev->dev); >> >> another thing, you need to check return of pm_runtime_get_sync() > > OK, let me check this and your other comment, and the fix is probably a > separate patch. > I realized the changes relevant to your comments were introduced in this patch, so just refreshed the patch with fixes instead of doing a separate patch. I didn't do a v5 just for these change, and will do so if there are more comments on the DT adaptation. regards Suman
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c index 9f56fb2..194886e 100644 --- a/drivers/hwspinlock/omap_hwspinlock.c +++ b/drivers/hwspinlock/omap_hwspinlock.c @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) if (!io_base) return -ENOMEM; + /* + * make sure the module is enabled and clocked before reading + * the module SYSSTATUS register + */ + pm_runtime_enable(&pdev->dev); + pm_runtime_get_sync(&pdev->dev); + /* Determine number of locks */ i = readl(io_base + SYSSTATUS_OFFSET); i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET; + /* + * runtime PM will make sure the clock of this module is + * enabled again iff at least one lock is requested + */ + pm_runtime_put(&pdev->dev); + /* one of the four lsb's must be set, and nothing else */ if (hweight_long(i & 0xf) != 1 || i > 8) { ret = -EINVAL; @@ -124,12 +137,6 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++) hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i; - /* - * runtime PM will make sure the clock of this module is - * enabled iff at least one lock is requested - */ - pm_runtime_enable(&pdev->dev); - ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops, base_id, num_locks); if (ret) @@ -138,9 +145,9 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) return 0; reg_fail: - pm_runtime_disable(&pdev->dev); kfree(bank); iounmap_base: + pm_runtime_disable(&pdev->dev); iounmap(io_base); return ret; }
The number of hwspinlocks are determined based on the value read from the IP block's SYSSTATUS register. However, the module may not be enabled and clocked, and the read may result in a bus error. This particular issue is seen rather easily on AM33XX, since the module wakeup is software controlled, and it is disabled out of reset. Make sure the module is enabled and clocked before reading the SYSSTATUS register. Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/hwspinlock/omap_hwspinlock.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)