| Message ID | 1236582316453-git-send-email-ext-eero.nurkkala@nokia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Tony Lindgren |
| Headers | show |
On Mon, 9 Mar 2009 08:05:12 +0100 "Nurkkala Eero.An (EXT-Offcode/Oulu)" <ext-Eero.Nurkkala@nokia.com> wrote: > From: Eero Nurkkala <ext-eero.nurkkala@nokia.com> > > McBSP clocks are being double enabled in the event the > McBSP is already active. Also, they are unnecessarily > disabled when there's no active McBSP in use. Fix this > phenomenom by enabling and disabling the clocks at a > proper location. > > @@ -226,9 +226,6 @@ int omap_mcbsp_request(unsigned int id) > if (mcbsp->pdata && mcbsp->pdata->ops && > mcbsp->pdata->ops->request) mcbsp->pdata->ops->request(id); > > - for (i = 0; i < mcbsp->num_clks; i++) > - clk_enable(mcbsp->clks[i]); > - > spin_lock(&mcbsp->lock); > if (!mcbsp->free) { > dev_err(mcbsp->dev, "McBSP%d is currently in use\n", > @@ -240,6 +237,9 @@ int omap_mcbsp_request(unsigned int id) > mcbsp->free = 0; > spin_unlock(&mcbsp->lock); > > + for (i = 0; i < mcbsp->num_clks; i++) > + clk_enable(mcbsp->clks[i]); > + Valid fix. > @@ -319,9 +319,6 @@ void omap_mcbsp_free(unsigned int id) > if (mcbsp->pdata && mcbsp->pdata->ops && > mcbsp->pdata->ops->free) mcbsp->pdata->ops->free(id); > > - for (i = mcbsp->num_clks - 1; i >= 0; i--) > - clk_disable(mcbsp->clks[i]); > - > spin_lock(&mcbsp->lock); > if (mcbsp->free) { > dev_err(mcbsp->dev, "McBSP%d was not reserved\n", > @@ -333,6 +330,9 @@ void omap_mcbsp_free(unsigned int id) > mcbsp->free = 1; > spin_unlock(&mcbsp->lock); > > + for (i = mcbsp->num_clks - 1; i >= 0; i--) > + clk_disable(mcbsp->clks[i]); > + Race here? See, McBSP is marked free before disabling the clocks and thus omap_mcbsp_request can start enabling them. So should you keep clock disabling after test for mcbsp->free as here now but mark McBSP free only after clocks are disabled? I think you should send this as an separate fix since this should go to the upstream also. Jarkko -- 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
> Race here? See, McBSP is marked free before disabling the clocks and > thus omap_mcbsp_request can start enabling them. So should you keep > clock disabling after test for mcbsp->free as here now but mark McBSP > free only after clocks are disabled? > > I think you should send this as an separate fix since this should go to > the upstream also. Good comment... why do I always do everything in such a hurry, without thinking any =) Either that or leave clock disabling within the spin_lock covered code? (probably risky business) - Eero -- 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
On Tue, 10 Mar 2009 19:43:40 +0100 "Nurkkala Eero.An (EXT-Offcode/Oulu)" <ext-Eero.Nurkkala@nokia.com> wrote: > Good comment... why do I always do everything in such a hurry, without > thinking any =) > Either that or leave clock disabling within the spin_lock covered > code? (probably risky business) > Didn't check any deeply can clk_disable sleep in any case but anyway there is no need to cover it with spinlock so better to not hold the lock while disabling. Jarkko -- 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
> Didn't check any deeply can clk_disable sleep in any case but anyway > there is no need to cover it with spinlock so better to not hold the > lock while disabling. > > > Jarkko Actually, is there any room for the race? I remember someone saying the clk:s have counters, so: 1. If mcbsp is "taken", clks on; - and at the same time, the previous mcbsp has been released, leaving clks off , clk counter decrements, nothing bad happens.. - Eero-- 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
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index 59850c2..e7755b9 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -226,9 +226,6 @@ int omap_mcbsp_request(unsigned int id) if (mcbsp->pdata && mcbsp->pdata->ops && mcbsp->pdata->ops->request) mcbsp->pdata->ops->request(id); - for (i = 0; i < mcbsp->num_clks; i++) - clk_enable(mcbsp->clks[i]); - spin_lock(&mcbsp->lock); if (!mcbsp->free) { dev_err(mcbsp->dev, "McBSP%d is currently in use\n", @@ -240,6 +237,9 @@ int omap_mcbsp_request(unsigned int id) mcbsp->free = 0; spin_unlock(&mcbsp->lock); + for (i = 0; i < mcbsp->num_clks; i++) + clk_enable(mcbsp->clks[i]); + /* * Enable wakup behavior, smart idle and all wakeups * REVISIT: some wakeups may be unnecessary @@ -319,9 +319,6 @@ void omap_mcbsp_free(unsigned int id) if (mcbsp->pdata && mcbsp->pdata->ops && mcbsp->pdata->ops->free) mcbsp->pdata->ops->free(id); - for (i = mcbsp->num_clks - 1; i >= 0; i--) - clk_disable(mcbsp->clks[i]); - spin_lock(&mcbsp->lock); if (mcbsp->free) { dev_err(mcbsp->dev, "McBSP%d was not reserved\n", @@ -333,6 +330,9 @@ void omap_mcbsp_free(unsigned int id) mcbsp->free = 1; spin_unlock(&mcbsp->lock); + for (i = mcbsp->num_clks - 1; i >= 0; i--) + clk_disable(mcbsp->clks[i]); + if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { /* Free IRQs */ free_irq(mcbsp->rx_irq, (void *)mcbsp);