| Message ID | 20131019121218.GH25034@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 19, 2013 at 01:12:18PM +0100, Russell King - ARM Linux wrote: > Okay, more on this. > > If I switch to using the DI clock, sourced from PLL5, and then "fix" > the clock tree such that I can change PLL5's rate, then I get most of > the display modes working. > > However, it will occasionally not work, and when that happens I need > to reboot to get any kind of output working again. I haven't delved > into it with the scope to find out what the HDMI clock is doing yet. > > Having solved it, what I think is going on is as follows: > > When one of the PLLs is prepared and then subsequently enabled, the > following sequence is used: > > - Clear bypass > - Power up the PLL > - Wait for the PLL to indicate lock > - Enable output > > This causes problems for me. If I change this to: > > - Power up the PLL > - Wait for PLL to indicate lock > - Clear bypass > - Enable output > > then I don't see any problems. My theory is that the bypass mux and/or > enable gate settings are synchronised with the mux output, and if the > mux output glitches while the PLL is gaining lock, it knocks that out > preventing the output from ever being enabled until the chip is hit with > a reset. You're right. I just checked FSL 3.0.35 kernel and found that BYPASS bit is written after POWER bit. > The other issue is that the set_rate() functions don't wait for the PLL > to lock before returning, and don't set bypass mode. It looks like this > code relies on drivers unpreparing the clocks before calling clk_set_rate(). > I don't see the clk API enforcing this in any way, so I'd suggest that > the set_rate() function also does the bypass -> set rate -> wait lock > -> clear bypass transition too. Maybe this should be applied to the > other PLLs too. Yes. We experienced the problem when upgrading FSL kernel tree to 3.10. The .set_rate() should wait for LOCK bit, but does not need to touch BYPASS. > Here's a diff which updates the av PLL to do this: Some other PLLs needs update too. I will cook up patches to fix these problems. Shawn > arch/arm/mach-imx/clk-pllv3.c | 77 ++++++++++++++++++++++++++++------------ > 1 files changed, 54 insertions(+), 23 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c > index f6640b6..11a7048 100644 > --- a/arch/arm/mach-imx/clk-pllv3.c > +++ b/arch/arm/mach-imx/clk-pllv3.c > @@ -45,21 +45,10 @@ struct clk_pllv3 { > > #define to_clk_pllv3(_hw) container_of(_hw, struct clk_pllv3, hw) > > -static int clk_pllv3_prepare(struct clk_hw *hw) > +static int clk_pllv3_wait_lock(struct clk_pllv3 *pll) > { > - struct clk_pllv3 *pll = to_clk_pllv3(hw); > - unsigned long timeout; > - u32 val; > - > - val = readl_relaxed(pll->base); > - val &= ~BM_PLL_BYPASS; > - if (pll->powerup_set) > - val |= BM_PLL_POWER; > - else > - val &= ~BM_PLL_POWER; > - writel_relaxed(val, pll->base); > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > > - timeout = jiffies + msecs_to_jiffies(10); > /* Wait for PLL to lock */ > do { > if (readl_relaxed(pll->base) & BM_PLL_LOCK) > @@ -68,10 +57,31 @@ static int clk_pllv3_prepare(struct clk_hw *hw) > break; > } while (1); > > - if (readl_relaxed(pll->base) & BM_PLL_LOCK) > - return 0; > + return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT; > +} > + > +static int clk_pllv3_prepare(struct clk_hw *hw) > +{ > + struct clk_pllv3 *pll = to_clk_pllv3(hw); > + u32 val, newval; > + int ret; > + > + val = readl_relaxed(pll->base); > + if (pll->powerup_set) > + newval = val | BM_PLL_POWER; > else > - return -ETIMEDOUT; > + newval = val & ~BM_PLL_POWER; > + writel_relaxed(newval, pll->base); > + > + ret = clk_pllv3_wait_lock(pll); > + if (ret == 0) { > + newval &= ~BM_PLL_BYPASS; > + writel_relaxed(newval, pll->base); > + } else { > + writel_relaxed(val, pll->base); > + } > + > + return ret; > } > > static void clk_pllv3_unprepare(struct clk_hw *hw) > @@ -80,6 +90,7 @@ static void clk_pllv3_unprepare(struct clk_hw *hw) > u32 val; > > val = readl_relaxed(pll->base); > + pr_info("%s: 0x%08x\n", __func__, val); > val |= BM_PLL_BYPASS; > if (pll->powerup_set) > val &= ~BM_PLL_POWER; > @@ -256,9 +267,10 @@ static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate, > struct clk_pllv3 *pll = to_clk_pllv3(hw); > unsigned long min_rate = parent_rate * 27; > unsigned long max_rate = parent_rate * 54; > - u32 val, div; > + u32 val, newval, div; > u32 mfn, mfd = 1000000; > s64 temp64; > + int ret; > > if (rate < min_rate || rate > max_rate) > return -EINVAL; > @@ -270,13 +282,32 @@ static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate, > mfn = temp64; > > val = readl_relaxed(pll->base); > - val &= ~pll->div_mask; > - val |= div; > - writel_relaxed(val, pll->base); > + > + /* set the PLL into bypass mode */ > + newval = val | BM_PLL_BYPASS; > + writel_relaxed(newval, pll->base); > + > + /* configure the new frequency */ > + newval &= ~pll->div_mask; > + newval |= div; > + writel_relaxed(newval, pll->base); > writel_relaxed(mfn, pll->base + PLL_NUM_OFFSET); > - writel_relaxed(mfd, pll->base + PLL_DENOM_OFFSET); > + writel(mfd, pll->base + PLL_DENOM_OFFSET); > + > + if (val & BM_PLL_POWER) { > + /* PLL is powered down */ > + ret = 0; > + } else { > + ret = clk_pllv3_wait_lock(pll); > + if (ret == 0) { > + /* only if it locked can we switch back to the PLL */ > + newval &= ~BM_PLL_BYPASS; > + newval |= val & BM_PLL_BYPASS; > + writel(newval, pll->base); > + } > + } > > - return 0; > + return ret; > } > > static const struct clk_ops clk_pllv3_av_ops = { >
On Mon, Oct 21, 2013 at 05:13:03PM +0800, Shawn Guo wrote: > On Sat, Oct 19, 2013 at 01:12:18PM +0100, Russell King - ARM Linux wrote: > > The other issue is that the set_rate() functions don't wait for the PLL > > to lock before returning, and don't set bypass mode. It looks like this > > code relies on drivers unpreparing the clocks before calling clk_set_rate(). > > I don't see the clk API enforcing this in any way, so I'd suggest that > > the set_rate() function also does the bypass -> set rate -> wait lock > > -> clear bypass transition too. Maybe this should be applied to the > > other PLLs too. > > Yes. We experienced the problem when upgrading FSL kernel tree to 3.10. > The .set_rate() should wait for LOCK bit, but does not need to touch > BYPASS. However, the IPU is more stable with switching to bypass while it relocks than not. > > Here's a diff which updates the av PLL to do this: > > Some other PLLs needs update too. I will cook up patches to fix these > problems. Yes, I only changed the set_rate on the AV codecs as I haven't tested that on the others.
On Mon, Oct 21, 2013 at 10:34:48AM +0100, Russell King - ARM Linux wrote: > However, the IPU is more stable with switching to bypass while it relocks > than not. As far as I know, FSL kernel has always been doing relock without switching to bypass, and I haven't heard unstable IPU issue from their testing. Do you have a case that I can set up here to see the issue? Shawn > > > Here's a diff which updates the av PLL to do this: > > > > Some other PLLs needs update too. I will cook up patches to fix these > > problems. > > Yes, I only changed the set_rate on the AV codecs as I haven't tested > that on the others.
On Mon, Oct 21, 2013 at 08:36:55PM +0800, Shawn Guo wrote: > On Mon, Oct 21, 2013 at 10:34:48AM +0100, Russell King - ARM Linux wrote: > > However, the IPU is more stable with switching to bypass while it relocks > > than not. > > As far as I know, FSL kernel has always been doing relock without > switching to bypass, and I haven't heard unstable IPU issue from their > testing. Do you have a case that I can set up here to see the issue? export DISPLAY=:0 while :; do xrandr -s 1280x720 -r 50 sleep 5 xrandr -s 1280x720 -r 60 sleep 5 done is one way. Another way is to switch between all possible resolutions supported by the connected device, keeping each resolution for 30 seconds a time. Normally after one or two run-throughs, the IPU will be deader than the parrot in Monty Python's parrot sketch, requiring a reboot to get it working again. As I've said previously, when it gets into this state, all the status registers I can find report that all FIFOs in the IPU are sitting in their empty state. The TMDS clock is correct, and the frame composer in the HDMI block is working, but it's not producing any sync signals or pixel data.
On Mon, Oct 21, 2013 at 02:17:58PM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 21, 2013 at 08:36:55PM +0800, Shawn Guo wrote: > > On Mon, Oct 21, 2013 at 10:34:48AM +0100, Russell King - ARM Linux wrote: > > > However, the IPU is more stable with switching to bypass while it relocks > > > than not. > > > > As far as I know, FSL kernel has always been doing relock without > > switching to bypass, and I haven't heard unstable IPU issue from their > > testing. Do you have a case that I can set up here to see the issue? > > export DISPLAY=:0 > while :; do > xrandr -s 1280x720 -r 50 > sleep 5 > xrandr -s 1280x720 -r 60 > sleep 5 > done Sorry, it took me a long time to set up this test on xubuntu 13.10. Building an image using your cubox-i branch, I can still see the above test plays my HDMI device to death quite easily. That said, I'm not sure if the BYPASS handling in .set_rate() does help the unstable issue we're seeing. BTW, the xubuntu session can crash quite easily here if I play the desktop a little bit, dragging a window and moving it around, popping up a dialog, or something. Shawn > > is one way. Another way is to switch between all possible resolutions > supported by the connected device, keeping each resolution for 30 seconds > a time. Normally after one or two run-throughs, the IPU will be deader > than the parrot in Monty Python's parrot sketch, requiring a reboot to > get it working again. > > As I've said previously, when it gets into this state, all the status > registers I can find report that all FIFOs in the IPU are sitting in > their empty state. The TMDS clock is correct, and the frame composer > in the HDMI block is working, but it's not producing any sync signals or > pixel data.
diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c index f6640b6..11a7048 100644 --- a/arch/arm/mach-imx/clk-pllv3.c +++ b/arch/arm/mach-imx/clk-pllv3.c @@ -45,21 +45,10 @@ struct clk_pllv3 { #define to_clk_pllv3(_hw) container_of(_hw, struct clk_pllv3, hw) -static int clk_pllv3_prepare(struct clk_hw *hw) +static int clk_pllv3_wait_lock(struct clk_pllv3 *pll) { - struct clk_pllv3 *pll = to_clk_pllv3(hw); - unsigned long timeout; - u32 val; - - val = readl_relaxed(pll->base); - val &= ~BM_PLL_BYPASS; - if (pll->powerup_set) - val |= BM_PLL_POWER; - else - val &= ~BM_PLL_POWER; - writel_relaxed(val, pll->base); + unsigned long timeout = jiffies + msecs_to_jiffies(100); - timeout = jiffies + msecs_to_jiffies(10); /* Wait for PLL to lock */ do { if (readl_relaxed(pll->base) & BM_PLL_LOCK) @@ -68,10 +57,31 @@ static int clk_pllv3_prepare(struct clk_hw *hw) break; } while (1); - if (readl_relaxed(pll->base) & BM_PLL_LOCK) - return 0; + return readl_relaxed(pll->base) & BM_PLL_LOCK ? 0 : -ETIMEDOUT; +} + +static int clk_pllv3_prepare(struct clk_hw *hw) +{ + struct clk_pllv3 *pll = to_clk_pllv3(hw); + u32 val, newval; + int ret; + + val = readl_relaxed(pll->base); + if (pll->powerup_set) + newval = val | BM_PLL_POWER; else - return -ETIMEDOUT; + newval = val & ~BM_PLL_POWER; + writel_relaxed(newval, pll->base); + + ret = clk_pllv3_wait_lock(pll); + if (ret == 0) { + newval &= ~BM_PLL_BYPASS; + writel_relaxed(newval, pll->base); + } else { + writel_relaxed(val, pll->base); + } + + return ret; } static void clk_pllv3_unprepare(struct clk_hw *hw) @@ -80,6 +90,7 @@ static void clk_pllv3_unprepare(struct clk_hw *hw) u32 val; val = readl_relaxed(pll->base); + pr_info("%s: 0x%08x\n", __func__, val); val |= BM_PLL_BYPASS; if (pll->powerup_set) val &= ~BM_PLL_POWER; @@ -256,9 +267,10 @@ static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate, struct clk_pllv3 *pll = to_clk_pllv3(hw); unsigned long min_rate = parent_rate * 27; unsigned long max_rate = parent_rate * 54; - u32 val, div; + u32 val, newval, div; u32 mfn, mfd = 1000000; s64 temp64; + int ret; if (rate < min_rate || rate > max_rate) return -EINVAL; @@ -270,13 +282,32 @@ static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate, mfn = temp64; val = readl_relaxed(pll->base); - val &= ~pll->div_mask; - val |= div; - writel_relaxed(val, pll->base); + + /* set the PLL into bypass mode */ + newval = val | BM_PLL_BYPASS; + writel_relaxed(newval, pll->base); + + /* configure the new frequency */ + newval &= ~pll->div_mask; + newval |= div; + writel_relaxed(newval, pll->base); writel_relaxed(mfn, pll->base + PLL_NUM_OFFSET); - writel_relaxed(mfd, pll->base + PLL_DENOM_OFFSET); + writel(mfd, pll->base + PLL_DENOM_OFFSET); + + if (val & BM_PLL_POWER) { + /* PLL is powered down */ + ret = 0; + } else { + ret = clk_pllv3_wait_lock(pll); + if (ret == 0) { + /* only if it locked can we switch back to the PLL */ + newval &= ~BM_PLL_BYPASS; + newval |= val & BM_PLL_BYPASS; + writel(newval, pll->base); + } + } - return 0; + return ret; } static const struct clk_ops clk_pllv3_av_ops = {