Re: Bug in linux omap clock framework?

[Tomi Valkeinen <tomi.valkeinen@nokia.com>]

On Wed, 2008-12-10 at 10:44 +0200, Högander Jouni wrote:
> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
> 
> > On Wed, 2008-12-10 at 09:37 +0200, Högander Jouni wrote:
> >> "ext Tomi Valkeinen" <tomi.valkeinen@nokia.com> writes:
> >> 
> >> > Hi,
> >> >
> >> > On Mon, 2008-12-08 at 02:24 -0700, ext Paul Walmsley wrote:
> >> >> Hi Tomi,
> >> >> 
> >> >> On Mon, 8 Dec 2008, Tomi Valkeinen wrote:
> >> >> 
> >> >> > On Sat, 2008-12-06 at 16:51 -0700, ext Paul Walmsley wrote:
> >> >> > > Hi Tomi,
> >> >> > > 
> >> >> > > nice test case.
> >> >> > > 
> >> >> > > On Fri, 5 Dec 2008, Tomi.Valkeinen@nokia.com wrote:
> >> >> > > 
> >> >> > > > I have had strange clk_enable() crashes with DSS2, and now I managed to
> >> >> > > > isolate it. With the included patch, on OMAP3 SDP board, with default
> >> >> > > > kernel config, I always get the crash below. In this example case it
> >> >> > > > happens at specific time on boot, but with DSS2 it happens randomly at
> >> >> > > > runtime, when I enable the DSS clocks.
> >> >> > > 
> >> >> 
> >> >> At this point my guess would be that it is a DSS driver problem, but I 
> >> >> don't think it is clear yet.
> >> >> 
> >> >
> >> > I don't know much about power and clock domains, but I believe I found
> >> > the reason and fix for this. At least this should point to the right
> >> > direction.
> >> >
> >> > It looks to me that when I do clk_enable() to a clock which is inside a
> >> > turned off powerdomain, clk_enable() function will return before the
> >> > powerdomain is fully turned on. Reading PM_PWSTST_DSS shows that the
> >> > powerdomain is still in transition state.
> >> >
> >> > The following change waits until the powerdomain has finished the
> >> > transition. At least it fixed the problem for me, and other things seem
> >> > to be still working =).
> >> >
> >> >
> >> > diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c
> >> > index fa62f14..f713d0b 100644
> >> > --- a/arch/arm/mach-omap2/clockdomain.c
> >> > +++ b/arch/arm/mach-omap2/clockdomain.c
> >> > @@ -567,6 +567,8 @@ int omap2_clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
> >> >         else
> >> >                 omap2_clkdm_wakeup(clkdm);
> >> >  
> >> > +       pwrdm_wait_transition(clkdm->pwrdm.ptr);
> >> > +
> >> 
> >> Altough this patch is fixing the problem, I'll guess it's because of
> >> delay it causes rather than waiting for
> >> PM_PWSTST_DSS. omap2_clkdm_clk_enable alone doesn't switch pwrdm to
> >> on. This is because clockdomain/powerdomains are controlled by HW (hw
> >> supervised).
> >> 
> >> I think the right answer is to use ST_*_IDLE & ST_*_STDBY bits in
> >> omap2_clk_wait_ready.
> >
> > But ST_DSS_IDLE says active only after both interface and functional
> > clock are on, so it doesn't help here.
> 
> Yes and in ST_DSS_IDLE description it is saying that if it's "0x1
> Display sub-system is in idle mode and cannot be accessed". So it only
> way to get it out of idle is to enable both clocks it needs to be done.
> 
> If you look at arch/arm/mach-omap2/clock.c:omap2_clk_wait_ready it is
> actually checking that both clocks are enabled.

But the ST_DSS_IDLE doesn't help here. The kernel crashes after enabling
dss_ick, before your code can even enable dss_fck. The same happens if
you do it other way around, first dss_fck and then dss_ick.

> > Why is it wrong to wait for PM_PWSTST_DSS? Do you mean that this crash
> > is not caused by the DSS being not powered on, but by something else,
> > and so the small delay just accidentally fixes the problem?
> 
> Something like that.
> 
> If you look at your code:
> 
> +static int __init omapfb_init(void)
> +{
> +	struct clk *c1, *c2, *c3;
> +
> +	c1 = clk_get(NULL, "dss_ick");
> +	c2 = clk_get(NULL, "dss1_alwon_fck");
> +	c3 = clk_get(NULL, "dss_tv_fck");
> +
> +	base = ioremap(DISPC_BASE, SZ_1K);
> +	printk("mapped to %p\n", base);
> +
> +	clk_enable(c1);
> 
> At this point ST_DSS_IDLE should not be yet checked (because fck is
> not enabled yet.
> 
> +	clk_enable(c2);
> 
> But at this point (actually inside clk_enable and
> omap2_clk_wait_ready) ST_DSS_IDLE should be checked before those
> register accesses in next two lines. You can verify my comment by
> adding "while(ST_DSS_IDLE);" here.

Doesn't omap2_clk_wait_ready alread check this?

And again, the problem is not accessing DSS registers. The problem is
that clk_enable(dss_ick) crashes. so the following code crashes also:

static int __init test_init(void)
{
	struct clk *c1 = clk_get(NULL, "dss_ick");

	while(1) {
		clk_enable(c1);
		clk_disable(c1);
	}

	return 0;
}


> 
> +
> +	dispc_write_reg(DISPC_IRQENABLE, 0);
> +	dispc_write_reg(DISPC_IRQSTATUS, 0);
> +
> +	clk_disable(c1);
> +	clk_disable(c2);
> +
> +	printk("usecounts %d, %d, %d\n", clk_get_usecount(c1),
> +			clk_get_usecount(c2),
> +			clk_get_usecount(c3));
> +
> +	/*clk_enable(c2);*/
> +	/*clk_enable(c3);*/
> +
> +	while(1) {
> +		clk_enable(c1);
> +		clk_disable(c1);
> +	}
> +
> +	return 0;
> +}
> +
> +module_init(omapfb_init);
> +MODULE_LICENSE("GPL");
> >  Tomi
> >
> >
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html