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

[Eddie James]

On 4/1/19 1:23 AM, Joel Stanley wrote:
> On Fri, 29 Mar 2019 at 21:15, Eddie James <eajames at linux.ibm.com> wrote:
>> The reset line is toggled by enabling the clocks, so it's not necessary
>> to manually toggle the reset as well.
> Hi Eddie,
>
> You didn't include a changelog here. You said last week that without
> the extra reset line toggle you would get interrupt storms. Can you
> fill us in on what changed?
I disabled interrupts before turning the clocks off. That wasn't the 
only issue, so I had to add the delay after turning clocks on in order 
to reliably get the resolution.
>
>>   #define INVALID_RESOLUTION_DELAY       msecs_to_jiffies(250)
>> -#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(500)
>> -#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(500)
>> -#define STOP_TIMEOUT                   msecs_to_jiffies(1000)
>> +#define RESOLUTION_CHANGE_DELAY                msecs_to_jiffies(750)
>> +#define MODE_DETECT_TIMEOUT            msecs_to_jiffies(1000)
>> +#define STOP_TIMEOUT                   msecs_to_jiffies(1500)
> These numbers changed but you didn't mention why.
Increasing these helped improve stability when changing resolution.
>
>>   #define DIRECT_FETCH_THRESHOLD         0x0c0000 /* 1024 * 768 */
>>
>>   #define VE_MAX_SRC_BUFFER_SIZE         0x8ca000 /* 1920 * 1200, 32bpp */
>> @@ -208,7 +207,6 @@ struct aspeed_video {
>>          void __iomem *base;
>>          struct clk *eclk;
>>          struct clk *vclk;
>> -       struct reset_control *rst;
>>
>>          struct device *dev;
>>          struct v4l2_ctrl_handler ctrl_handler;
>> @@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
>>          aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
>>   }
>>
>> -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);
>> -}
>> -
>>   static void aspeed_video_off(struct aspeed_video *video)
>>   {
>> -       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);
> How did you get to 100us?

I used the delay from between the reset assert/deassert used in the old 
aspeed_video_reset function.

>
>> @@ -1464,7 +1459,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
>> -               aspeed_video_reset(video);
>> +               aspeed_video_off(video);
>> +               aspeed_video_on(video);
>> +
> This includes 5.1ms of sleeping in the clock driver's enable path; are
> you sure we need to turn clocks off and on again? Can we avoid the
> full reset by instead resetting the registers to a specific state?

In this case the engine has failed to stop streaming, so it's not 
obeying commands via the registers. Only option is to reset the engine. 
It's an error condition.

>