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

[Eddie James]

On 4/1/19 12:25 PM, Jae Hyun Yoo wrote:
>
>
> 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.


That is the logical conclusion, but the results I got didn't agree. In 
my previous testing, I was only able to detect the resolution maybe 50% 
of the time without this delay.


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


Now I just ran my tests, and I can't recreate my previous problem with 
or without the delay... I don't understand that. Same code, same system, 
but different kernel build. Very odd. But that leaves me with no 
justification for this delay, so I'll remove it.


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


Looks good, but this should probably go in a different patch set.


>
> This threaded irq handling works well in my H/W.
>
> Cheers,
> Jae
>