[PATCH dev-5.0 v2 1/5] media: platform: Aspeed: Remove use of reset line
Joel Stanley
joel at jms.id.au
Mon Apr 1 17:23:55 AEDT 2019
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?
> #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.
> #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?
> @@ -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?
More information about the openbmc
mailing list