[PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line
Eddie James
eajames at linux.ibm.com
Tue Apr 2 13:01:07 AEDT 2019
On 4/1/19 8:39 PM, Joel Stanley wrote:
> On Mon, 1 Apr 2019 at 14:32, Eddie James <eajames at linux.ibm.com> wrote:
>>
>> 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.
> You need to mention why in your commit message.
I've dropped those changes in the latest patch set.
>
>>>> #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.
> Is this what aspeed said we needed? What happens when it's less?
I didn't do any testing between 1 and 99 us. The original value was
derived from some of the comments in the clock change area of the
ast2500 specification. In my testing of this patch set, 0 delay resulted
in frequent failure to grab the resolution, but today I was unable to
reproduce that behavior and so I have dropped the delay from the latest
patch set.
>
>>>> @@ -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.
> This would be a good comment to add to the source code.
There is a comment and dev_err message already, just not in the diff.
>
More information about the openbmc
mailing list