[PATCH] arm: mx28: check for gated clocks when setting saif divider

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Wolfram Sang]

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

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

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Wolfram Sang]

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
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/a7bcb41c/attachment.sig>

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Wednesday, November 16, 2011 9:22 PM
> To: linux-arm-kernel at 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/
> |

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Wolfram Sang]

On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> > Sent: Wednesday, November 16, 2011 9:22 PM
> > To: linux-arm-kernel at 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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/aa84fca4/attachment.sig>

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Wednesday, November 16, 2011 9:51 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-kernel at 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 at pengutronix.de]
> > > Sent: Wednesday, November 16, 2011 9:22 PM
> > > To: linux-arm-kernel at 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

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Shawn Guo]

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 at pengutronix.de]
> > > Sent: Wednesday, November 16, 2011 9:22 PM
> > > To: linux-arm-kernel at 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?

-- 
Regards,
Shawn

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Wolfram Sang]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111117/454a0c31/attachment.sig>

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Thursday, November 17, 2011 5:30 PM
> To: Guo Shawn-R65073
> Cc: Dong Aisheng-B29396; linux-arm-kernel at 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 

[PATCH] arm: mx28: check for gated clocks when setting saif divider

[Shawn Guo]

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: ...".

-- 
Regards,
Shawn