bug in PL011 console

[linuxzsc@gmail.com (Richard Zhao)]

2010/12/23 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> On Thu, Dec 23, 2010 at 03:08:41PM +0000, Russell King - ARM Linux wrote:
>> On Thu, Dec 23, 2010 at 04:02:34PM +0100, Uwe Kleine-K?nig wrote:
>> > Steven told me on irc that sleeping was not allowed in the console write
>> > callback. ?Maybe this didn't show up earlier because not all clk
>> > implementations sleep as mxs' does.
>> >
>> > I think the only possible fix is to do the clk_enable in the setup
>> > callback instead of per-write.
>> >
>> > Will send a patch as follow up.
>>
>> We really need to sort out what's expected from the CLK API. ?The drivers
>> I write assume that it's absolutely fine to call clk_enable/clk_disable
>> from IRQ context, and for the platforms I implemented the CLK API for,
>> that's absolutely true.
> The common struct clk patch[1] by Jeremy Kerr sleeps, too. ?And I think
> most people who commented to this series thought that this is the right
> behaviour.
I vote atomic clock. All pure clock operations are atomic. I suggest
voltage change is not included in clock API. If we need a finegrained
power management, when we open a device, we can first request some
power conditions and release the power conditions when we close it.
Mixing sleepy and tomic functions together is likely making mess.

Thanks
Richard
>
>> I'd lobby for it because it allows for proper power saving management of
>> clocks for devices - PL011 only enables the clock when either the port is
>> open or it's actually sending data out the port. ?So it's doing absolutely
>> the best power management that can be done with UARTs.
> Yeah, that makes fine-grained clk enabling harder/impossible. ?So
> ideally we'd have something that only makes clk_enable sleep iff
> that's sensible for that clk. ?And if you have a clock that can be
> enabled "fast" it would not sleep.
>
> Don't know if that works, maybe something like that:
>
> ? ? ? ?int clk_enable(struct clk *clk)
> ? ? ? ?{
> ? ? ? ? ? ? ? ?spin_lock(something);
> ? ? ? ? ? ? ? ?if (clk->flags & (SOME|FLAGS))
> ? ? ? ? ? ? ? ? ? ? ? ?goto out_busy;
> ? ? ? ? ? ? ? ?clk->flags |= ENABLING;
> ? ? ? ? ? ? ? ?spin_unlock(something);
>
> ? ? ? ? ? ? ? ?ret = clk->really_enable(...);
>
> ? ? ? ? ? ? ? ?spin_lock(something);
> ? ? ? ? ? ? ? ?clk->flags &= ~ENABLING;
> ? ? ? ? ? ? ? ?spin_unlock(something);
> ? ? ? ?}
>
> Some things that need careful consideration are:
>
> ? ? ? ?- clk->flags already has ENABLING when clk_enable is entered.
> ? ? ? ? ?(needs to sleep/poll then?)
> ? ? ? ?- clk->usecount already > 0
> ? ? ? ? ?(early return unless clk->flags & DISABLING)
> ? ? ? ?- do we need the irqsaving spinlock variants?
> ? ? ? ? ?(I assume yes)
>
> Probably there are more.
>
> Best regards
> Uwe
>
> [1] last submission: http://thread.gmane.org/gmane.linux.kernel/1073751
>
> --
> Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | Uwe Kleine-K?nig ? ? ? ? ? ?|
> Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?|
>