Re: Bug in linux omap clock framework?

[jouni.hogander@nokia.com (Högander Jouni)]

Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:

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

Yes, you are right this is already done.

>
> 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;
> }
>

Are you still using SDP and default config + latest linux-omap? I'm
not able to reproduce this using your example code.

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

-- 
Jouni Högander

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