| Message ID | CAKnu2MppEKxZD2Z77tfShMfG80d-Xx26yBGhZtJkJ2C6SjE=1w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote: > 2011/7/31 Linus Walleij <linus.walleij@linaro.org>: > > 2011/7/30 Russell King - ARM Linux <linux@arm.linux.org.uk>: > >> On Sat, Jul 30, 2011 at 01:07:40PM +0100, Russell King - ARM Linux wrote: > >>> It may make better sense to convert this to runtime PM. I suspect > >>> that there's core support which the amba/bus.c can do to help in that > >>> respect (eg, managing the apb pclk itself) so that we don't have to > >>> add the same code to every primecell driver. > >> > >> Something like this for the bus driver (untested): > >> > >> drivers/amba/bus.c | 38 ++++++++++++++++++++++++++++++++++++-- > >> 1 files changed, 36 insertions(+), 2 deletions(-) > > > > I think the pm_runtime_* code Rabin put in place inside > > drivers/spi/spi-pl022.c would play really well with this approach, and > > just work, so: > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > ..and while it will just cause some double refcounts on the clock, > it makes sense to delete the pclk manipulation from the PL022 > driver code as part of the patch, like this: Yes, this looks fine. Shall I wrap it up as part of my patch? Two other things I've spotted in this driver are: 1. The remove function doesn't undo what the probe function did to the pclk and vcore. It needs to keep things balanced. For a driver which doesn't manage its pclk, this is what happens: - core gets pclk - core enables pclk - core calls driver's probe - driver sets stuff up ... - core calls driver's remove - driver tidies up - core disables pclk - core puts pclk And PL022 does this: - core gets pclk - core enables pclk - core calls driver's probe - driver sets stuff up - driver disables pclk ... - core calls driver's remove - driver tidies up - core disables pclk - core puts pclk Notice the double-disable of pclk in that sequence. If ->probe disables pclk, ->remove needs to return with that disable balanced with an enable. 2. It thinks it can refuse 'remove' by returning an error code. This is false. removes can't be aborted - here's the code from drivers/base/dd.c: static void __device_release_driver(struct device *dev) { ... if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) drv->remove(dev); ... } Notice how return codes go nowhere. remove should _really_ be a void function to stop people thinking that it can be aborted. It can't.
On Sun, Jul 31, 2011 at 7:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote: >> >> ..and while it will just cause some double refcounts on the clock, >> it makes sense to delete the pclk manipulation from the PL022 >> driver code as part of the patch, like this: > > Yes, this looks fine. Shall I wrap it up as part of my patch? Yes please. Acked-by. > Two other things I've spotted in this driver are: > > 1. The remove function doesn't undo what the probe function did to > the pclk and vcore. It needs to keep things balanced. For a driver > which doesn't manage its pclk, this is what happens: > - core gets pclk > - core enables pclk > - core calls driver's probe > - driver sets stuff up > ... > - core calls driver's remove > - driver tidies up > - core disables pclk > - core puts pclk > > And PL022 does this: > - core gets pclk > - core enables pclk > - core calls driver's probe > - driver sets stuff up > - driver disables pclk > ... > - core calls driver's remove > - driver tidies up > - core disables pclk > - core puts pclk > > Notice the double-disable of pclk in that sequence. If ->probe disables > pclk, ->remove needs to return with that disable balanced with an enable. Ah yes. Can you fix this as part of the patch since it's pretty related? Much appreciated if you do. > 2. It thinks it can refuse 'remove' by returning an error code. This > is false. removes can't be aborted - here's the code from drivers/base/dd.c: > > static void __device_release_driver(struct device *dev) > { > ... > if (dev->bus && dev->bus->remove) > dev->bus->remove(dev); > else if (drv->remove) > drv->remove(dev); > ... > } > > Notice how return codes go nowhere. remove should _really_ be a void > function to stop people thinking that it can be aborted. It can't. Correct, thanks for spotting this. I'll send a separate patch to Grant fixing this up. Yours, Linus Walleij
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index eba88c7..4c4c02a 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -516,7 +516,6 @@ static void giveback(struct pl022 *pl022) msg->complete(msg->context); /* This message is completed, so let's turn off the clocks & power */ clk_disable(pl022->clk); - amba_pclk_disable(pl022->adev); amba_vcore_disable(pl022->adev); pm_runtime_put(&pl022->adev->dev); } @@ -1546,7 +1545,6 @@ static void pump_messages(struct work_struct *work) */ pm_runtime_get_sync(&pl022->adev->dev); amba_vcore_enable(pl022->adev); - amba_pclk_enable(pl022->adev); clk_enable(pl022->clk); restore_state(pl022); flush(pl022); @@ -2236,10 +2234,10 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id) } dev_dbg(dev, "probe succeeded\n"); /* - * Disable the silicon block pclk and any voltage domain and just - * power it up and clock it when it's needed + * Disable the silicon block voltage domain and just + * power it up when it's needed. The AMBA core code will + * enable the pclk. */ - amba_pclk_disable(adev); amba_vcore_disable(adev); return 0;