| Message ID | 1404194129-25543-4-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 01, 2014 at 02:55:28AM -0300, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > SSI clocks use the share_count mechanism since SSI and SPDIF share the same > clock gate bits. > > When using the share_count for the SSI clock we notice that it gets disabled > due to the usage of pre-decrement operation in the clk_gate2_disable() function. > > Use the post-decrement operation so that the correct share_count is used and the > SSI clock does not get disable when an audio file needs to be played. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-gate2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c > index 4ba587d..463083c 100644 > --- a/arch/arm/mach-imx/clk-gate2.c > +++ b/arch/arm/mach-imx/clk-gate2.c > @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw) > > spin_lock_irqsave(gate->lock, flags); > > - if (gate->share_count && --(*gate->share_count) > 0) > + if (gate->share_count && (*gate->share_count)-- > 0) The change makes no sense. Let's say that clk_gate2_disable() gets called with share_count being 1. In this case, we should access register to gate the clock, right? Shawn > goto out; > > reg = readl(gate->reg); > -- > 1.8.3.2 >
On Tue, Jul 1, 2014 at 8:52 AM, Shawn Guo <shawn.guo@freescale.com> wrote: >> --- a/arch/arm/mach-imx/clk-gate2.c >> +++ b/arch/arm/mach-imx/clk-gate2.c >> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw) >> >> spin_lock_irqsave(gate->lock, flags); >> >> - if (gate->share_count && --(*gate->share_count) > 0) >> + if (gate->share_count && (*gate->share_count)-- > 0) > > The change makes no sense. Let's say that clk_gate2_disable() gets > called with share_count being 1. In this case, we should access > register to gate the clock, right? If share_count is 1 it means that someone else is using the clock and we can't disable it. Please try running the series without this patch. When the extern audio clock is enabled, share_count is 1. Later the the spdif clock (the one that is shared with extern audio clock) is disabled by the CCF as it is not used, which makes the extern audio clock to be also disabled, which is not what we want. What would you suggest?
Quoting Fabio Estevam (2014-07-01 10:44:40) > On Tue, Jul 1, 2014 at 8:52 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > > >> --- a/arch/arm/mach-imx/clk-gate2.c > >> +++ b/arch/arm/mach-imx/clk-gate2.c > >> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw) > >> > >> spin_lock_irqsave(gate->lock, flags); > >> > >> - if (gate->share_count && --(*gate->share_count) > 0) > >> + if (gate->share_count && (*gate->share_count)-- > 0) > > > > The change makes no sense. Let's say that clk_gate2_disable() gets > > called with share_count being 1. In this case, we should access > > register to gate the clock, right? > > If share_count is 1 it means that someone else is using the clock and > we can't disable it. Why do you keep track of share_count at all? Is the enable_count bookkeeping within the clock framework insufficient for your needs? Regards, Mike > > Please try running the series without this patch. When the extern > audio clock is enabled, share_count is 1. Later the the spdif clock > (the one that is shared with extern audio clock) is disabled by the > CCF as it is not used, which makes the extern audio clock to be also > disabled, which is not what we want. > > What would you suggest? > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Mike, On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote: >> If share_count is 1 it means that someone else is using the clock and >> we can't disable it. > > Why do you keep track of share_count at all? Is the enable_count > bookkeeping within the clock framework insufficient for your needs? What we are trying to handle here is the case when two clocks share the same clock gating bit. Let's say clocks clk1 and clk2 share the same clock gating bit. What we want to achieve are: Scenario 1: clk2 is disabled clk1 is enabled --> clock gate bit is set to 1 clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled, it is OK to gate off the clock here Scenario 2: clk1 is enabled -> clock gate bit is set to 1 clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to fix it because the scenario 2 is not working. I am wondering if we should just extend drivers/clk/clk-gate.c to handle shared clock instead of doing this in arch/arm/mach-imx? What do you think? Thanks, Fabio Estevam
Quoting Fabio Estevam (2014-07-02 09:52:56) > Hi Mike, > > On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote: > > >> If share_count is 1 it means that someone else is using the clock and > >> we can't disable it. > > > > Why do you keep track of share_count at all? Is the enable_count > > bookkeeping within the clock framework insufficient for your needs? > > What we are trying to handle here is the case when two clocks share > the same clock gating bit. > > Let's say clocks clk1 and clk2 share the same clock gating bit. > > What we want to achieve are: > > Scenario 1: > > clk2 is disabled > clk1 is enabled --> clock gate bit is set to 1 > clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled, > it is OK to gate off the clock here > > Scenario 2: > > clk1 is enabled -> clock gate bit is set to 1 > clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled > > We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to > fix it because the scenario 2 is not working. > > I am wondering if we should just extend drivers/clk/clk-gate.c to > handle shared clock instead of doing this in arch/arm/mach-imx? > > What do you think? I am actually looking into this right now, so it is good timing for me to come across this thread. While hacking on the coordinated clock rates feature I started to think about coordinating clock enables for exactly your case: a single clock control enables or disables multiple clock outputs. I think some core framework changes are needed to support this sensibly, and only putting the changes into clk-gate.c might not be sufficient or elegant. I've added this task to my stack and will keep you Cc'd when something hits the list. Regards, Mike > > Thanks, > > Fabio Estevam
On Wed, Jul 02, 2014 at 01:52:56PM -0300, Fabio Estevam wrote: > Hi Mike, > > On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote: > > >> If share_count is 1 it means that someone else is using the clock and > >> we can't disable it. > > > > Why do you keep track of share_count at all? Is the enable_count > > bookkeeping within the clock framework insufficient for your needs? > > What we are trying to handle here is the case when two clocks share > the same clock gating bit. > > Let's say clocks clk1 and clk2 share the same clock gating bit. > > What we want to achieve are: > > Scenario 1: > > clk2 is disabled > clk1 is enabled --> clock gate bit is set to 1 > clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled, > it is OK to gate off the clock here > > Scenario 2: > > clk1 is enabled -> clock gate bit is set to 1 > clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled This never happens. If clk2 is disabled by clk_disable() without being enabled by clk_enable() beforehand, it returns from __clk_disable() immediately due to hitting of the WARN below. if (WARN_ON(clk->enable_count == 0)) return; Shawn > > We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to > fix it because the scenario 2 is not working. > > I am wondering if we should just extend drivers/clk/clk-gate.c to > handle shared clock instead of doing this in arch/arm/mach-imx? > > What do you think? > > Thanks, > > Fabio Estevam
On Thu, Jul 03, 2014 at 03:46:00PM +0800, Shawn Guo wrote: > > Scenario 2: > > > > clk1 is enabled -> clock gate bit is set to 1 > > clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled > > This never happens. If clk2 is disabled by clk_disable() without being > enabled by clk_enable() beforehand, it returns from __clk_disable() > immediately due to hitting of the WARN below. > > if (WARN_ON(clk->enable_count == 0)) > return; Well, it only happens in case that clock core calls clk->ops->disable() directly from clk_disable_unused_subtree() in order to disable unused clocks. Shawn
diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c index 4ba587d..463083c 100644 --- a/arch/arm/mach-imx/clk-gate2.c +++ b/arch/arm/mach-imx/clk-gate2.c @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw) spin_lock_irqsave(gate->lock, flags); - if (gate->share_count && --(*gate->share_count) > 0) + if (gate->share_count && (*gate->share_count)-- > 0) goto out; reg = readl(gate->reg);