[PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line

[Jae Hyun Yoo]

On 4/1/2019 8:11 AM, Eddie James wrote:
> 
> On 3/29/19 6:54 PM, Jae Hyun Yoo wrote:
>>>   static void aspeed_video_off(struct aspeed_video *video)
>>>   {
>>> -    aspeed_video_reset(video);
>>> +    /* Disable interrupts */
>>> +    aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>>         /* Turn off the relevant clocks */
>>>       clk_disable_unprepare(video->vclk);
>>> @@ -508,7 +497,13 @@ static void aspeed_video_on(struct aspeed_video 
>>> *video)
>>>       clk_prepare_enable(video->eclk);
>>>       clk_prepare_enable(video->vclk);
>>>   -    aspeed_video_reset(video);
>>> +    /*
>>> +     * Delay (and don't sleep in case in interrupt context) to let the
>>> +     * clocks stabilize. Without this, mode detection sometimes fails.
>>> +     * Possibly the registers don't update too soon after clocks are
>>> +     * enabled.
>>> +     */
>>> +    udelay(100);
>>>   }
>>
>> aspeed_video_off() will be called from interrupt context too. Are you
>> sure that this 100us delay is needed at here? If it's actually needed,
>> would it be batter to use devm_request_threaded_irq() instead of
>> devm_request_irq() in aspeed_video_init() function?
> 
> Well this isn't new to this patchset, it delayed 100us before in 
> aspeed_video_reset.
> 
> I am quite sure it's needed.

This is previous code of aspeed_video_reset():
static void aspeed_video_reset(struct aspeed_video *video)
{
	/* Reset the engine */
	reset_control_assert(video->rst);

	/* Don't usleep here; function may be called in interrupt context */
	udelay(100);
	reset_control_deassert(video->rst);
}

The delay was added between assert and deassert to shape the reset
signal up, but the delay in this patch set is after the deasserting of
reset signal. Now in this patch set, you are going to use reset control
using the way that clk-aspeed module provides. The module already has
a 10msec delay in aspeed_clk_enable() for making the reset pulse so
you don't need to add an additional delay at here.

Practically, I tested this patch after removing the 100usec delay and
checked that this driver works well.

> Since the ast2500 has only one logical processor, I'm not sure a 
> threaded irq would function any differently.

Yes, good point. Differently from other Aspeed driver modules, it calls
clk_prepare_enable() in interrupt context which will cause at least
10msecs of busy waiting thru aspeed_clk_enable() in isr. In UP kernel,
this will disturb handling of most of all other interrupts so it's the
reason why I suggested threaded irq instead.

Try this change in aspeed_video_init():
rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
			       IRQF_ONESHOT | IRQF_SHARED, DEVICE_NAME,
			       video);

This threaded irq handling works well in my H/W.

Cheers,
Jae