arm: mx28: check for gated clocks when setting saif divider
Subsystems (3)
PATCH 1/1 arm: mx28: check for gated clocks when setting saif divider
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.
---
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(-)
arch/arm/mach-mxs/clock-mx28.c +4 -0
@@ -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--) \
Signature · 1 line
1.7.5.4
PATCH 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 4 lines · 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.
Quoted 8 lines
Signed-off-by Wolfram Sang <[email protected]>Cc Sascha Hauer <[email protected]>Cc Shawn Guo <[email protected]>Cc Dong Aisheng-B29396 <[email protected]>---
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?
Quoted 22 lines
arch/arm/mach-mxs/clock-mx28.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)arch/arm/mach-mxs/clock-mx28.c +4
-0@@ -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--) \Signature · 1 line
1.7.5.4
Signature · 9 lines
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 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 19 lines
-----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 dividerQuoted 10 lines · 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 <[email protected]>Cc Sascha Hauer <[email protected]>Cc Shawn Guo <[email protected]>Cc Dong Aisheng-B29396 <[email protected]>---
Quoted 6 lines
Quoted 2 lines
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 befixed 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?
Quoted 29 lines
arch/arm/mach-mxs/clock-mx28.c static int name##_set_rate(struct clk *clk,
Quoted 9 lines
arch/arm/mach-mxs/clock-mx28.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)(quoted diff) +0
-0
b/arch/arm/mach-mxs/clock-mx28.cindex 63d6117..c9482b5 100644arch/arm/mach-mxs/clock-mx28.c +0
-0@@ -457,6 +457,10 @@ static int name##_set_rate(struct clk *clk,unsigned long rate) \
Quoted 11 lines
arch/arm/mach-mxs/clock-mx28.c +4
-1reg = __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--) \ --Quoted 2 lines
1.7.5.4
--
Pengutronix e.K. | Wolfram Sang
|
Industrial Linux Solutions | http://www.pengutronix.de/
|
PATCH 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 23 lines · On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote:
Quoted 23 lines
-----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 dividerQuoted 4 lines · 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.
Quoted 8 lines
Signed-off-by Wolfram Sang <[email protected]>Cc Sascha Hauer <[email protected]>Cc Shawn Guo <[email protected]>Cc Dong Aisheng-B29396 <[email protected]>---
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 befixed in the driver?
Quoted 8 lines
Quoted 2 lines
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_disableto set a proper rate, then when needs the clock on, doclk_enableagain?
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
Signature · 9 lines
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 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 37 lines
-----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 dividerQuoted 28 lines · On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote:
Quoted 19 lines
-----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 dividerQuoted 10 lines · 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 <[email protected]>Cc Sascha Hauer <[email protected]>Cc Shawn Guo <[email protected]>Cc Dong Aisheng-B29396 <[email protected]>---
Quoted 7 lines
Quoted 3 lines
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 befixed in the driver?
Ping. Trying to catch up, has this been resolved meanwhile?
Sorry, I missed this patch.
Quoted 8 lines
Quoted 4 lines
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_disableto set a proper rate, then when needs the clock on, doclk_enableagain?Confused, do you really mean enable ->
set_rate-> disable? Because you can't set clocks when they are enabled?
Yes.
Quoted 1 line
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).
Quoted 6 lines
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 1/1 arm: mx28: check for gated clocks when setting saif divider
In reply to Wolfram SangQuoted 33 lines · Wolfram Sang
Quoted 23 lines · On Wed, Nov 16, 2011 at 01:35:04PM +0000, Dong Aisheng-B29396 wrote:
Quoted 23 lines
-----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 dividerQuoted 4 lines · 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.
Quoted 8 lines
Signed-off-by Wolfram Sang <[email protected]>Cc Sascha Hauer <[email protected]>Cc Shawn Guo <[email protected]>Cc Dong Aisheng-B29396 <[email protected]>---
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 befixed in the driver?
Quoted 8 lines
Quoted 2 lines
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_disableto set a proper rate, then when needs the clock on, doclk_enableagain?
Quoted 5 lines
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.
Quoted 6 lines
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?
Signature · 2 lines
Regards, Shawn
PATCH 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 8 lines
Quoted 6 lines
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.
Signature · 9 lines
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 1/1 arm: mx28: check for gated clocks when setting saif divider
Quoted 15 lines
-----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 dividerQuoted 6 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
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 1/1 arm: mx28: check for gated clocks when setting saif divider
In reply to Shawn GuoQuoted 1 line · Shawn Guo
On Wed, Nov 16, 2011 at 02:51:26PM +0100, Wolfram Sang wrote:
...
Quoted 9 lines
Quoted 6 lines
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: ...".
Signature · 2 lines
Regards, Shawn