| Message ID | 1315650583-4793-1-git-send-email-w.sang@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Sat, Sep 10, 2011 at 12:29:43PM +0200, Wolfram Sang wrote: > Like with all other clocks, the divider for the SAIF devices should not > be altered when the clock is gated. Bail out when this is the case like > the other clocks do. > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawn.guo@freescale.com> > Cc: Dong Aisheng-B29396 <B29396@freescale.com> > --- > > Aisheng: I think this is the correct solution for clock-mx28.c. If setting the > rate of the saif clocks hit the error path, it should be fixed in the driver? Ping. Trying to catch up, has this been resolved meanwhile? > > arch/arm/mach-mxs/clock-mx28.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c > index 63d6117..c9482b5 100644 > --- a/arch/arm/mach-mxs/clock-mx28.c > +++ b/arch/arm/mach-mxs/clock-mx28.c > @@ -457,6 +457,10 @@ static int name##_set_rate(struct clk *clk, unsigned long rate) \ > reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ > reg &= ~BM_CLKCTRL_##rs##_DIV; \ > reg |= div << BP_CLKCTRL_##rs##_DIV; \ > + if (reg & (1 << clk->enable_shift)) { \ > + pr_err("%s: clock is gated\n", __func__); \ > + return -EINVAL; \ > + } \ > __raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ > \ > for (i = 10000; i; i--) \ > -- > 1.7.5.4 >
> -----Original Message----- > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > Sent: Wednesday, November 16, 2011 9:22 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396 > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting saif > divider > > On Sat, Sep 10, 2011 at 12:29:43PM +0200, Wolfram Sang wrote: > > Like with all other clocks, the divider for the SAIF devices should > > not be altered when the clock is gated. Bail out when this is the case > > like the other clocks do. > > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > Cc: Shawn Guo <shawn.guo@freescale.com> > > Cc: Dong Aisheng-B29396 <B29396@freescale.com> > > --- > > > > Aisheng: I think this is the correct solution for clock-mx28.c. If > > setting the rate of the saif clocks hit the error path, it should be > fixed in the driver? > > Ping. Trying to catch up, has this been resolved meanwhile? > Sorry, I missed this patch. If I understand right, the convention way is to clk_set_rate() then clk_enable().I f that, is it reasonable for driver to do something like: Clk_enable -> clk_set_rate->clk_disable to set a proper rate, then when needs the clock on, do clk_enable again? > > > > arch/arm/mach-mxs/clock-mx28.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mach-mxs/clock-mx28.c > > b/arch/arm/mach-mxs/clock-mx28.c index 63d6117..c9482b5 100644 > > --- a/arch/arm/mach-mxs/clock-mx28.c > > +++ b/arch/arm/mach-mxs/clock-mx28.c > > @@ -457,6 +457,10 @@ static int name##_set_rate(struct clk *clk, > unsigned long rate) \ > > reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ > > reg &= ~BM_CLKCTRL_##rs##_DIV; \ > > reg |= div << BP_CLKCTRL_##rs##_DIV; \ > > + if (reg & (1 << clk->enable_shift)) { \ > > + pr_err("%s: clock is gated\n", __func__); \ > > + return -EINVAL; \ > > + } \ > > __raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ > > \ > > for (i = 10000; i; i--) \ > > -- > > 1.7.5.4 > > > > -- > Pengutronix e.K. | Wolfram Sang > | > Industrial Linux Solutions | http://www.pengutronix.de/ > |
On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote: > > -----Original Message----- > > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > > Sent: Wednesday, November 16, 2011 9:22 PM > > To: linux-arm-kernel@lists.infradead.org > > Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396 > > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting saif > > divider > > > > On Sat, Sep 10, 2011 at 12:29:43PM +0200, Wolfram Sang wrote: > > > Like with all other clocks, the divider for the SAIF devices should > > > not be altered when the clock is gated. Bail out when this is the case > > > like the other clocks do. > > > > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > > Cc: Shawn Guo <shawn.guo@freescale.com> > > > Cc: Dong Aisheng-B29396 <B29396@freescale.com> > > > --- > > > > > > Aisheng: I think this is the correct solution for clock-mx28.c. If > > > setting the rate of the saif clocks hit the error path, it should be > > fixed in the driver? > > > > Ping. Trying to catch up, has this been resolved meanwhile? > > > Sorry, I missed this patch. > > If I understand right, the convention way is to clk_set_rate() then > clk_enable().I f that, is it reasonable for driver to do something like: > Clk_enable -> clk_set_rate->clk_disable to set a proper rate, > then when needs the clock on, do clk_enable again? Confused, do you really mean enable -> set_rate -> disable? Because you can't set clocks when they are enabled? Well, to be honest, this is all is not very nice due to mxs restrictions. I chose this approach because this is the common pattern in all other clocks. There might be a better way of handling this, but then we'd need to adapt all other clocks as well. What we definately should not have is one kind of handling the 'already enabled' case for a few clocks and another kind for others. Regards, Wolfram
> -----Original Message----- > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > Sent: Wednesday, November 16, 2011 9:51 PM > To: Dong Aisheng-B29396 > Cc: linux-arm-kernel@lists.infradead.org; Sascha Hauer; Guo Shawn-R65073 > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting saif > divider > > On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote: > > > -----Original Message----- > > > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > > > Sent: Wednesday, November 16, 2011 9:22 PM > > > To: linux-arm-kernel@lists.infradead.org > > > Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396 > > > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting > > > saif divider > > > > > > On Sat, Sep 10, 2011 at 12:29:43PM +0200, Wolfram Sang wrote: > > > > Like with all other clocks, the divider for the SAIF devices > > > > should not be altered when the clock is gated. Bail out when this > > > > is the case like the other clocks do. > > > > > > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > > > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > > > Cc: Shawn Guo <shawn.guo@freescale.com> > > > > Cc: Dong Aisheng-B29396 <B29396@freescale.com> > > > > --- > > > > > > > > Aisheng: I think this is the correct solution for clock-mx28.c. If > > > > setting the rate of the saif clocks hit the error path, it should > > > > be > > > fixed in the driver? > > > > > > Ping. Trying to catch up, has this been resolved meanwhile? > > > > > Sorry, I missed this patch. > > > > If I understand right, the convention way is to clk_set_rate() then > > clk_enable().I f that, is it reasonable for driver to do something like: > > Clk_enable -> clk_set_rate->clk_disable to set a proper rate, then > > when needs the clock on, do clk_enable again? > > Confused, do you really mean enable -> set_rate -> disable? Because you > can't set clocks when they are enabled? > Yes. > Well, to be honest, this is all is not very nice due to mxs restrictions. Yes, it's truly not comfortable for drivers know this restrictions (not handled by clock code). > I chose this approach because this is the common pattern in all other > clocks. There might be a better way of handling this, but then we'd need > to adapt all other clocks as well. What we definately should not have is > one kind of handling the 'already enabled' case for a few clocks and > another kind for others. > I agree with you point here. I was wondering formerly that maybe we can handle this restriction in clock code that not make clk_set_rate function blocked by clock is still not enable issue. I know that all other clocks are using the pattern as you said now. If we decide to follow that pattern, I will agree (although not comfortable) and update the driver side. Regards Dong Aisheng
On Wed, Nov 16, 2011 at 02:51:26PM +0100, Wolfram Sang wrote: > On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote: > > > -----Original Message----- > > > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > > > Sent: Wednesday, November 16, 2011 9:22 PM > > > To: linux-arm-kernel@lists.infradead.org > > > Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396 > > > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting saif > > > divider > > > > > > On Sat, Sep 10, 2011 at 12:29:43PM +0200, Wolfram Sang wrote: > > > > Like with all other clocks, the divider for the SAIF devices should > > > > not be altered when the clock is gated. Bail out when this is the case > > > > like the other clocks do. > > > > > > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > > > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > > > Cc: Shawn Guo <shawn.guo@freescale.com> > > > > Cc: Dong Aisheng-B29396 <B29396@freescale.com> > > > > --- > > > > > > > > Aisheng: I think this is the correct solution for clock-mx28.c. If > > > > setting the rate of the saif clocks hit the error path, it should be > > > fixed in the driver? > > > > > > Ping. Trying to catch up, has this been resolved meanwhile? > > > > > Sorry, I missed this patch. > > > > If I understand right, the convention way is to clk_set_rate() then > > clk_enable().I f that, is it reasonable for driver to do something like: > > Clk_enable -> clk_set_rate->clk_disable to set a proper rate, > > then when needs the clock on, do clk_enable again? > > Confused, do you really mean enable -> set_rate -> disable? Because you > can't set clocks when they are enabled? > > Well, to be honest, this is all is not very nice due to mxs > restrictions. When the code was written, it inherited the restriction from FSL internal code. I doubt the restriction is the hardware one, and I would try to remove the restriction when migrating to common clock framework. > I chose this approach because this is the common pattern > in all other clocks. There might be a better way of handling this, but > then we'd need to adapt all other clocks as well. What we definately > should not have is one kind of handling the 'already enabled' case for a > few clocks and another kind for others. > What about leaving it as it is for now, and try to consolidate when we rework the code for common clock framework migration?
> > I chose this approach because this is the common pattern > > in all other clocks. There might be a better way of handling this, but > > then we'd need to adapt all other clocks as well. What we definately > > should not have is one kind of handling the 'already enabled' case for a > > few clocks and another kind for others. > > > What about leaving it as it is for now, and try to consolidate when > we rework the code for common clock framework migration? In general, very fine with me. I seem to recall saif-drivers will need a hack then, but Aisheng surely knows better.
> -----Original Message----- > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > Sent: Thursday, November 17, 2011 5:30 PM > To: Guo Shawn-R65073 > Cc: Dong Aisheng-B29396; linux-arm-kernel@lists.infradead.org; Sascha > Hauer; Guo Shawn-R65073 > Subject: Re: [PATCH] arm: mx28: check for gated clocks when setting saif > divider > > > > I chose this approach because this is the common pattern in all > > > other clocks. There might be a better way of handling this, but then > > > we'd need to adapt all other clocks as well. What we definately > > > should not have is one kind of handling the 'already enabled' case > > > for a few clocks and another kind for others. > > > > > What about leaving it as it is for now, and try to consolidate when we > > rework the code for common clock framework migration? > > In general, very fine with me. I seem to recall saif-drivers will need a > hack then, but Aisheng surely knows better. > Yes, saif driver enables clock until trigger happens. Before that, it needs set the clock rate as preparation. Obviously, it cannot work since current clock code does not allow to set rate before the clock is enabled. My consider is that can my mxs-specific patchs adding record support go in first since it's only related to clock driver? Then this clock issue can be fixed later when move to common clock. Regards Dong Aisheng
On Thu, Nov 17, 2011 at 09:18:34AM +0800, Shawn Guo wrote: > On Wed, Nov 16, 2011 at 02:51:26PM +0100, Wolfram Sang wrote: ... > > I chose this approach because this is the common pattern > > in all other clocks. There might be a better way of handling this, but > > then we'd need to adapt all other clocks as well. What we definately > > should not have is one kind of handling the 'already enabled' case for a > > few clocks and another kind for others. > > > What about leaving it as it is for now, and try to consolidate when > we rework the code for common clock framework migration? > I confirmed this patch is needed and decide to apply it immediately without waiting for common clock consolidation effort. PS. I'm changing the subject prefix to "ARM: mx28: ...".
diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c index 63d6117..c9482b5 100644 --- a/arch/arm/mach-mxs/clock-mx28.c +++ b/arch/arm/mach-mxs/clock-mx28.c @@ -457,6 +457,10 @@ static int name##_set_rate(struct clk *clk, unsigned long rate) \ reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ reg &= ~BM_CLKCTRL_##rs##_DIV; \ reg |= div << BP_CLKCTRL_##rs##_DIV; \ + if (reg & (1 << clk->enable_shift)) { \ + pr_err("%s: clock is gated\n", __func__); \ + return -EINVAL; \ + } \ __raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs); \ \ for (i = 10000; i; i--) \
Like with all other clocks, the divider for the SAIF devices should not be altered when the clock is gated. Bail out when this is the case like the other clocks do. Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawn.guo@freescale.com> Cc: Dong Aisheng-B29396 <B29396@freescale.com> --- Aisheng: I think this is the correct solution for clock-mx28.c. If setting the rate of the saif clocks hit the error path, it should be fixed in the driver? arch/arm/mach-mxs/clock-mx28.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)