Skip to contentSkip to conversation

spi/pl022: Enable clock in probe an use runtime_idle

PATCH New Ulf Hansson 4 participants 10 messages Oct 21, 2011 – Nov 3, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

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

Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

---
 drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)
drivers/spi/spi-pl022.c +17 -1
index f103e47..ad48fba 100644
@@ -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 = {
Signature · 1 line
1.7.5.4
#1 lore raw

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

Quoted 9 lines · 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

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.

Yours,
Linus Walleij

#2 lore raw

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

Quoted 8 lines · Linus Walleij
Quoted 6 lines · 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.

Quoted 5 lines
Quoted 3 lines
Change-Id I6cb86f2cad30ecaab16f512daf4674b039b18213

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.

Quoted 5 lines

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!

Quoted 6 lines

But looks correct.

Yours,
Linus Walleij

#3 lore raw

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

In reply to Ulf Hansson
Quoted 9 lines · 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

Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

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

g.

Quoted 54 lines
---
 drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)
drivers/spi/spi-pl022.c +17 -1
index f103e47..ad48fba 100644
@@ -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 = {
Signature · 1 line
1.7.5.4
#4 lore raw

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

Quoted 7 lines · Grant Likely
Quoted 6 lines · 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.

Quoted 7 lines
Quoted 3 lines
Change-Id I6cb86f2cad30ecaab16f512daf4674b039b18213

Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

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).

#5 lore raw

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

2011/10/24 Russell King - ARM Linux <[email protected]>:

Quoted 12 lines · Grant Likely
Quoted 11 lines · Ulf Hansson
Quoted 9 lines

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

Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Quoted 6 lines
Quoted 3 lines

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

#6 lore raw

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

In reply to Ulf Hansson
Quoted 9 lines · 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

Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Sorry Ulf, this patch has issues.

Quoted 10 lines
 drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)
drivers/spi/spi-pl022.c +0 -0
index f103e47..ad48fba 100644
@@ -2184,6 +2184,12 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		goto  err_clk_prep;
 	}

drivers/spi/spi-pl022.c pl022_probe(struct amba_device *adev, const struct amba_id *id) · line 2195

Quoted 10 lines
drivers/spi/spi-pl022.c +6 -0
 
+	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.

drivers/spi/spi-pl022.c pl022_probe(struct amba_device *adev, const struct amba_id *id) · line 2250

Quoted 9 lines
drivers/spi/spi-pl022.c +2 -0
@@ -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.

Quoted 8 lines
drivers/spi/spi-pl022.c +4 -0
@@ -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);

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2365

Quoted 10 lines
drivers/spi/spi-pl022.c +5 -1
+	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.

#7 lore raw

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

Quoted 39 lines
Quoted 8 lines
drivers/spi/spi-pl022.c +4 -0
@@ -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);

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2365

Quoted 10 lines
drivers/spi/spi-pl022.c +5 -1
+	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;
}
Quoted 4 lines

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.

#8 lore raw

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

Quoted 37 lines · Ulf Hansson
Quoted 37 lines
Quoted 6 lines
drivers/spi/spi-pl022.c +3 -0
@@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
  	return 0;
 }
+
+static int pl022_runtime_idle(struct device *dev)
+{

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2363

Quoted 10 lines
drivers/spi/spi-pl022.c +6 -1
+	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;
}
Quoted 6 lines
Quoted 4 lines

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.

Quoted 3 lines

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:

Quoted 10 lines
Quoted 10 lines

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;
        }
Quoted 3 lines
Quoted 3 lines
        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:

Quoted 5 lines
Quoted 5 lines

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2358

Quoted 5 lines
drivers/spi/spi-pl022.c +5 -0
+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().

#9 lore raw

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

Quoted 93 lines · Russell King - ARM Linux
Quoted 36 lines · Ulf Hansson
Quoted 36 lines
Quoted 6 lines
drivers/spi/spi-pl022.c +3 -0
@@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
  	return 0;
 }
+
+static int pl022_runtime_idle(struct device *dev)
+{

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2363

Quoted 10 lines
drivers/spi/spi-pl022.c +6 -1
+	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;
}
Quoted 5 lines
Quoted 4 lines

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.

Quoted 3 lines

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);

...
}

Quoted 8 lines

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!

Quoted 30 lines
Quoted 11 lines
Quoted 11 lines

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;
        }
Quoted 3 lines
Quoted 3 lines
        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:

Quoted 5 lines
Quoted 5 lines

drivers/spi/spi-pl022.c static int pl022_runtime_resume(struct device *dev) · line 2358

Quoted 5 lines
drivers/spi/spi-pl022.c +5 -0
+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().