[PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Ulf Hansson]

Since we are always runtime resumed when leaving probe
the clock must be enabled. To accomplish that we are able
to be runtime suspended after probe in the case when no
request is going to be recieved, a runtime_idle function
has been implemented.

Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447
---
 drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index f103e47..ad48fba 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2184,6 +2184,12 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		goto  err_clk_prep;
 	}
 
+	status = clk_enable(pl022->clk);
+	if (status) {
+		dev_err(&adev->dev, "could not enable SSP/SPI bus clock\n");
+		goto err_no_clk_en;
+	}
+
 	/* Disable SSP */
 	writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)),
 	       SSP_CR1(pl022->virtbase));
@@ -2237,6 +2243,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 
 	free_irq(adev->irq[0], pl022);
  err_no_irq:
+	clk_disable(pl022->clk);
+ err_no_clk_en:
 	clk_unprepare(pl022->clk);
  err_clk_prep:
 	clk_put(pl022->clk);
@@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
 
 	return 0;
 }
+
+static int pl022_runtime_idle(struct device *dev)
+{
+	pm_runtime_suspend(dev);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops pl022_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
-	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
+			   pl022_runtime_resume,
+			   pl022_runtime_idle)
 };
 
 static struct vendor_data vendor_arm = {
-- 
1.7.5.4


------------------------------------------------------------------------------
The demand for IT networking professionals continues to grow, and the
demand for specialized networking skills is growing even more rapidly.
Take a complimentary Learning@Cisco Self-Assessment and learn 
about Cisco certifications, training, and career opportunities. 
http://p.sf.net/sfu/cisco-dev2dev

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Grant Likely]

On Fri, Oct 21, 2011 at 04:08:44PM +0200, Ulf Hansson wrote:
> Since we are always runtime resumed when leaving probe
> the clock must be enabled. To accomplish that we are able
> to be runtime suspended after probe in the case when no
> request is going to be recieved, a runtime_idle function
> has been implemented.
> 
> Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Acked-by: Grant Likely <grant.likely@secretlab.ca>

Since it depends on Russell's tree, I'm okay with it going in that way.

g.

> ---
>  drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index f103e47..ad48fba 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -2184,6 +2184,12 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto  err_clk_prep;
>  	}
>  
> +	status = clk_enable(pl022->clk);
> +	if (status) {
> +		dev_err(&adev->dev, "could not enable SSP/SPI bus clock\n");
> +		goto err_no_clk_en;
> +	}
> +
>  	/* Disable SSP */
>  	writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)),
>  	       SSP_CR1(pl022->virtbase));
> @@ -2237,6 +2243,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	free_irq(adev->irq[0], pl022);
>   err_no_irq:
> +	clk_disable(pl022->clk);
> + err_no_clk_en:
>  	clk_unprepare(pl022->clk);
>   err_clk_prep:
>  	clk_put(pl022->clk);
> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> +
> +static int pl022_runtime_idle(struct device *dev)
> +{
> +	pm_runtime_suspend(dev);
> +	return 0;
> +}
>  #endif
>  
>  static const struct dev_pm_ops pl022_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
> +			   pl022_runtime_resume,
> +			   pl022_runtime_idle)
>  };
>  
>  static struct vendor_data vendor_arm = {
> -- 
> 1.7.5.4
> 

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 02:03:42PM +0200, Grant Likely wrote:
> On Fri, Oct 21, 2011 at 04:08:44PM +0200, Ulf Hansson wrote:
> > Since we are always runtime resumed when leaving probe
> > the clock must be enabled. To accomplish that we are able
> > to be runtime suspended after probe in the case when no
> > request is going to be recieved, a runtime_idle function
> > has been implemented.
> > 
> > Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
> > Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> > Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447
> 
> Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> Since it depends on Russell's tree, I'm okay with it going in that way.

I've received this patch in the patch system, but I'm not planning to
apply it yet because it depends on two branches in my tree (amba and clk).

------------------------------------------------------------------------------
The demand for IT networking professionals continues to grow, and the
demand for specialized networking skills is growing even more rapidly.
Take a complimentary Learning@Cisco Self-Assessment and learn 
about Cisco certifications, training, and career opportunities. 
http://p.sf.net/sfu/cisco-dev2dev

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Linus Walleij]

2011/10/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Oct 24, 2011 at 02:03:42PM +0200, Grant Likely wrote:
>> On Fri, Oct 21, 2011 at 04:08:44PM +0200, Ulf Hansson wrote:
>> > Since we are always runtime resumed when leaving probe
>> > the clock must be enabled. To accomplish that we are able
>> > to be runtime suspended after probe in the case when no
>> > request is going to be recieved, a runtime_idle function
>> > has been implemented.
>> >
>> > Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
>> > Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
>> > Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447
>>
>> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>>
>> Since it depends on Russell's tree, I'm okay with it going in that way.
>
> I've received this patch in the patch system, but I'm not planning to
> apply it yet because it depends on two branches in my tree (amba and clk).

We can fixup that in the aftermath of the merge window, no
hurries.

Yours,
Linus Walleij

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Linus Walleij]

On Fri, Oct 21, 2011 at 4:08 PM, Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org> wrote:

> Since we are always runtime resumed when leaving probe
> the clock must be enabled. To accomplish that we are able
> to be runtime suspended after probe in the case when no
> request is going to be recieved, a runtime_idle function
> has been implemented.
>
> Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
> Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Skip the gerrit weirdo stuff.

Notice that this patch cannot be merged until Russell's
AMBA PM patches are merged, so put this in Russell's
patch tracker so he can put it on his AMBA PM branch.

But looks correct.
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

------------------------------------------------------------------------------
The demand for IT networking professionals continues to grow, and the
demand for specialized networking skills is growing even more rapidly.
Take a complimentary Learning@Cisco Self-Assessment and learn 
about Cisco certifications, training, and career opportunities. 
http://p.sf.net/sfu/cisco-dev2dev

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Ulf Hansson]

Linus Walleij wrote:
> On Fri, Oct 21, 2011 at 4:08 PM, Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org> wrote:
> 
>> Since we are always runtime resumed when leaving probe
>> the clock must be enabled. To accomplish that we are able
>> to be runtime suspended after probe in the case when no
>> request is going to be recieved, a runtime_idle function
>> has been implemented.
>>
>> Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
>> Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
>> Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447
> 
> Skip the gerrit weirdo stuff.

Sorry, for not cleaning up the commit msg, my bad. I fix.

> 
> Notice that this patch cannot be merged until Russell's
> AMBA PM patches are merged, so put this in Russell's
> patch tracker so he can put it on his AMBA PM branch.
>

I will do!

> But looks correct.
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Yours,
> Linus Walleij
> 


------------------------------------------------------------------------------
The demand for IT networking professionals continues to grow, and the
demand for specialized networking skills is growing even more rapidly.
Take a complimentary Learning@Cisco Self-Assessment and learn 
about Cisco certifications, training, and career opportunities. 
http://p.sf.net/sfu/cisco-dev2dev

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Russell King - ARM Linux]

On Fri, Oct 21, 2011 at 04:08:44PM +0200, Ulf Hansson wrote:
> Since we are always runtime resumed when leaving probe
> the clock must be enabled. To accomplish that we are able
> to be runtime suspended after probe in the case when no
> request is going to be recieved, a runtime_idle function
> has been implemented.
> 
> Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
> Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Sorry Ulf, this patch has issues.

>  drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index f103e47..ad48fba 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -2184,6 +2184,12 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto  err_clk_prep;
>  	}
>  
> +	status = clk_enable(pl022->clk);
> +	if (status) {
> +		dev_err(&adev->dev, "could not enable SSP/SPI bus clock\n");
> +		goto err_no_clk_en;
> +	}
> +
>  	/* Disable SSP */
>  	writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)),
>  	       SSP_CR1(pl022->virtbase));

Ok - this is a bug fix.

> @@ -2237,6 +2243,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	free_irq(adev->irq[0], pl022);
>   err_no_irq:
> +	clk_disable(pl022->clk);
> + err_no_clk_en:
>  	clk_unprepare(pl022->clk);
>   err_clk_prep:
>  	clk_put(pl022->clk);

As is this.

> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> +
> +static int pl022_runtime_idle(struct device *dev)
> +{
> +	pm_runtime_suspend(dev);
> +	return 0;
> +}
>  #endif
>  
>  static const struct dev_pm_ops pl022_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
> +			   pl022_runtime_resume,
> +			   pl022_runtime_idle)

This is an unnecessary change.

The bus-level ops runtime PM ops call pm_generic_runtime_idle() when
its 'runtime_idle' operation is invoked.  Let's look at the code
there:

int pm_generic_runtime_idle(struct device *dev)
{
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

        if (pm && pm->runtime_idle) {
                int ret = pm->runtime_idle(dev);
                if (ret)
                        return ret;
        }

        pm_runtime_suspend(dev);
        return 0;
}

If the driver has a NULL runtime idle, then generic code will call
pm_runtime_suspend() for the device.  So, adding a runtime_idle callback
to a driver to explicitly call pm_runtime_suspend() is not required.

------------------------------------------------------------------------------
RSA&#174; Conference 2012
Save $700 by Nov 18
Register now&#33;
http://p.sf.net/sfu/rsa-sfdev2dev1

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Ulf Hansson]

>> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>>  
>>  	return 0;
>>  }
>> +
>> +static int pl022_runtime_idle(struct device *dev)
>> +{
>> +	pm_runtime_suspend(dev);
>> +	return 0;
>> +}
>>  #endif
>>  
>>  static const struct dev_pm_ops pl022_dev_pm_ops = {
>>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
>> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
>> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
>> +			   pl022_runtime_resume,
>> +			   pl022_runtime_idle)
> 
> This is an unnecessary change.
> 
> The bus-level ops runtime PM ops call pm_generic_runtime_idle() when
> its 'runtime_idle' operation is invoked.  Let's look at the code
> there:
> 
> int pm_generic_runtime_idle(struct device *dev)
> {
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> 
>         if (pm && pm->runtime_idle) {
>                 int ret = pm->runtime_idle(dev);
>                 if (ret)
>                         return ret;
>         }
> 
>         pm_runtime_suspend(dev);
>         return 0;
> }
> 
> If the driver has a NULL runtime idle, then generic code will call
> pm_runtime_suspend() for the device.  So, adding a runtime_idle callback
> to a driver to explicitly call pm_runtime_suspend() is not required.
> 

You are somewhat correct. But the patch is still needed as is!

Reason is simply that after a probe, driver core is calling 
pm_runtime_put_sync. This will not go through the 
pm_generic_runtime_idle function, but directly to __pm_runtime_idle.





------------------------------------------------------------------------------
RSA(R) Conference 2012
Save $700 by Nov 18
Register now
http://p.sf.net/sfu/rsa-sfdev2dev1

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Russell King - ARM Linux]

On Thu, Nov 03, 2011 at 02:59:53PM +0100, Ulf Hansson wrote:
>>> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>>>   	return 0;
>>>  }
>>> +
>>> +static int pl022_runtime_idle(struct device *dev)
>>> +{
>>> +	pm_runtime_suspend(dev);
>>> +	return 0;
>>> +}
>>>  #endif
>>>   static const struct dev_pm_ops pl022_dev_pm_ops = {
>>>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
>>> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
>>> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
>>> +			   pl022_runtime_resume,
>>> +			   pl022_runtime_idle)
>>
>> This is an unnecessary change.
>>
>> The bus-level ops runtime PM ops call pm_generic_runtime_idle() when
>> its 'runtime_idle' operation is invoked.  Let's look at the code
>> there:
>>
>> int pm_generic_runtime_idle(struct device *dev)
>> {
>>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>
>>         if (pm && pm->runtime_idle) {
>>                 int ret = pm->runtime_idle(dev);
>>                 if (ret)
>>                         return ret;
>>         }
>>
>>         pm_runtime_suspend(dev);
>>         return 0;
>> }
>>
>> If the driver has a NULL runtime idle, then generic code will call
>> pm_runtime_suspend() for the device.  So, adding a runtime_idle callback
>> to a driver to explicitly call pm_runtime_suspend() is not required.
>>
>
> You are somewhat correct. But the patch is still needed as is!

No it is not required, by any means shape or form.

> Reason is simply that after a probe, driver core is calling  
> pm_runtime_put_sync. This will not go through the  
> pm_generic_runtime_idle function, but directly to __pm_runtime_idle.

Let's look at the code:

static inline int pm_runtime_put_sync(struct device *dev)
{
        return __pm_runtime_idle(dev, RPM_GET_PUT);
}

int __pm_runtime_idle(struct device *dev, int rpmflags)
{
...
        spin_lock_irqsave(&dev->power.lock, flags);
        retval = rpm_idle(dev, rpmflags);
        spin_unlock_irqrestore(&dev->power.lock, flags);
...
}

static int rpm_idle(struct device *dev, int rpmflags)
{
        int (*callback)(struct device *);
...
        if (dev->pm_domain)
                callback = dev->pm_domain->ops.runtime_idle;
        else if (dev->type && dev->type->pm)
                callback = dev->type->pm->runtime_idle;
        else if (dev->class && dev->class->pm)
                callback = dev->class->pm->runtime_idle;
        else if (dev->bus && dev->bus->pm)
                callback = dev->bus->pm->runtime_idle;
        else
                callback = NULL;

        if (callback)
                __rpm_callback(callback, dev);
...
}

static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
        __releases(&dev->power.lock) __acquires(&dev->power.lock)
{
...
        retval = cb(dev);
...
}

Nothing in there calls down to the _driver_ level PM ops from the core
runtime PM code.  What will happen is that this statement will assign
the callback pointer:

	callback = dev->bus->pm->runtime_idle;

and dev->bus->pm will be &amba_pm.  Its runtime idle function will be
pm_generic_runtime_idle.  As I quoted above:

>> int pm_generic_runtime_idle(struct device *dev)
>> {
>>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>
>>         if (pm && pm->runtime_idle) {
>>                 int ret = pm->runtime_idle(dev);
>>                 if (ret)
>>                         return ret;
>>         }
>>
>>         pm_runtime_suspend(dev);
>>         return 0;
>> }

This is the only way you get down to the driver-level pm->runtime_idle
callback.

Please describe what benefit having *THIS* pm->runtime_idle(dev) pointing
at your new function:

>>> +static int pl022_runtime_idle(struct device *dev)
>>> +{
>>> +	pm_runtime_suspend(dev);
>>> +	return 0;
>>> +}

gains us over the case where pm->runtime_idle is NULL inside
pm_generic_runtime_idle().

Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Thu, Nov 03, 2011 at 02:59:53PM +0100, Ulf Hansson wrote:
>>>> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>>>>   	return 0;
>>>>  }
>>>> +
>>>> +static int pl022_runtime_idle(struct device *dev)
>>>> +{
>>>> +	pm_runtime_suspend(dev);
>>>> +	return 0;
>>>> +}
>>>>  #endif
>>>>   static const struct dev_pm_ops pl022_dev_pm_ops = {
>>>>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
>>>> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
>>>> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
>>>> +			   pl022_runtime_resume,
>>>> +			   pl022_runtime_idle)
>>> This is an unnecessary change.
>>>
>>> The bus-level ops runtime PM ops call pm_generic_runtime_idle() when
>>> its 'runtime_idle' operation is invoked.  Let's look at the code
>>> there:
>>>
>>> int pm_generic_runtime_idle(struct device *dev)
>>> {
>>>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>>
>>>         if (pm && pm->runtime_idle) {
>>>                 int ret = pm->runtime_idle(dev);
>>>                 if (ret)
>>>                         return ret;
>>>         }
>>>
>>>         pm_runtime_suspend(dev);
>>>         return 0;
>>> }
>>>
>>> If the driver has a NULL runtime idle, then generic code will call
>>> pm_runtime_suspend() for the device.  So, adding a runtime_idle callback
>>> to a driver to explicitly call pm_runtime_suspend() is not required.
>>>
>> You are somewhat correct. But the patch is still needed as is!
> 
> No it is not required, by any means shape or form.
> 
>> Reason is simply that after a probe, driver core is calling  
>> pm_runtime_put_sync. This will not go through the  
>> pm_generic_runtime_idle function, but directly to __pm_runtime_idle.
> 
> Let's look at the code:
> 
> static inline int pm_runtime_put_sync(struct device *dev)
> {
>         return __pm_runtime_idle(dev, RPM_GET_PUT);
> }
> 
> int __pm_runtime_idle(struct device *dev, int rpmflags)
> {
> ...
>         spin_lock_irqsave(&dev->power.lock, flags);
>         retval = rpm_idle(dev, rpmflags);
>         spin_unlock_irqrestore(&dev->power.lock, flags);
> ...
> }
> 
> static int rpm_idle(struct device *dev, int rpmflags)
> {
>         int (*callback)(struct device *);
> ...
>         if (dev->pm_domain)
>                 callback = dev->pm_domain->ops.runtime_idle;
>         else if (dev->type && dev->type->pm)
>                 callback = dev->type->pm->runtime_idle;
>         else if (dev->class && dev->class->pm)
>                 callback = dev->class->pm->runtime_idle;
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->runtime_idle;
>         else
>                 callback = NULL;
> 
>         if (callback)
>                 __rpm_callback(callback, dev);
> ...
> }
> 
> static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>         __releases(&dev->power.lock) __acquires(&dev->power.lock)
> {
> ...
>         retval = cb(dev);
> ...
> }
> 
> Nothing in there calls down to the _driver_ level PM ops from the core
> runtime PM code.  What will happen is that this statement will assign
> the callback pointer:
> 
> 	callback = dev->bus->pm->runtime_idle;
> 
> and dev->bus->pm will be &amba_pm.  Its runtime idle function will be
> pm_generic_runtime_idle.  As I quoted above:

This I totally missed! You are absolutely right!
Of course the amba bus calls the generic runtime idle function.

I will re-work the patch and remove the runtime_idle function completely!

> 
>>> int pm_generic_runtime_idle(struct device *dev)
>>> {
>>>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>>
>>>         if (pm && pm->runtime_idle) {
>>>                 int ret = pm->runtime_idle(dev);
>>>                 if (ret)
>>>                         return ret;
>>>         }
>>>
>>>         pm_runtime_suspend(dev);
>>>         return 0;
>>> }
> 
> This is the only way you get down to the driver-level pm->runtime_idle
> callback.
> 
> Please describe what benefit having *THIS* pm->runtime_idle(dev) pointing
> at your new function:
> 
>>>> +static int pl022_runtime_idle(struct device *dev)
>>>> +{
>>>> +	pm_runtime_suspend(dev);
>>>> +	return 0;
>>>> +}
> 
> gains us over the case where pm->runtime_idle is NULL inside
> pm_generic_runtime_idle().
>