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

[Milton Miller II]

Around 04/01/2019 12:26PM in some timezone, 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?
>> 

More importantly, the prepare part of prepare_enable is sleepable 
and must be called from a thread context, so threaded irq is likely 
required if this needs to be called.

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

milton