RE: [PATCH] media: aspeed: fix clock stopping logic
From: Jammy Huang
Date: Mon Aug 12 2024 - 04:06:21 EST
Hi Phil,
After some investigation, I think the problem is 'reset is not assert at aspeed_video_off() with
clk off'. When clk is enabled in aspeed_video_on(), reset will be assert and de-assert by clk_enable.
But there is nothing done for clk_disable. Thus, it will look like below:
// aspeed_video_on
enable vclk
reset assert
delay 100us
enable eclk
delay 10ms
reset de-assert
// aspeed_video_off
disable eclk
disable vclk
I think if we add reset before disable eclk, your problem will be fixed. Could you try the patch below
which I add reset in aspeed_video_off().
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
index e6f3cf3c721e..b9655d5259a7 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
@@ -308,6 +308,7 @@ video: video@1e700000 {
<&syscon ASPEED_CLK_GATE_ECLK>;
clock-names = "vclk", "eclk";
interrupts = <7>;
+ resets = <&syscon ASPEED_RESET_VIDEO>;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 7fb421153596..62c65b13dc7b 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -451,6 +451,7 @@ video: video@1e700000 {
<&syscon ASPEED_CLK_GATE_ECLK>;
clock-names = "vclk", "eclk";
interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ resets = <&syscon ASPEED_RESET_VIDEO>;
status = "disabled";
};
diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
index 9c53c9c2285b..fc633f574566 100644
--- a/drivers/media/platform/aspeed/aspeed-video.c
+++ b/drivers/media/platform/aspeed/aspeed-video.c
@@ -25,6 +25,7 @@
#include <linux/workqueue.h>
#include <linux/debugfs.h>
#include <linux/ktime.h>
+#include <linux/reset.h>
#include <linux/regmap.h>
#include <linux/mfd/syscon.h>
#include <media/v4l2-ctrls.h>
@@ -310,6 +311,7 @@ struct aspeed_video {
void __iomem *base;
struct clk *eclk;
struct clk *vclk;
clock-names = "vclk", "eclk";
interrupts = <7>;
+ resets = <&syscon ASPEED_RESET_VIDEO>;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 7fb421153596..62c65b13dc7b 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -451,6 +451,7 @@ video: video@1e700000 {
<&syscon ASPEED_CLK_GATE_ECLK>;
clock-names = "vclk", "eclk";
interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ resets = <&syscon ASPEED_RESET_VIDEO>;
status = "disabled";
};
diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
index 9c53c9c2285b..fc633f574566 100644
--- a/drivers/media/platform/aspeed/aspeed-video.c
+++ b/drivers/media/platform/aspeed/aspeed-video.c
@@ -25,6 +25,7 @@
#include <linux/workqueue.h>
#include <linux/debugfs.h>
#include <linux/ktime.h>
+#include <linux/reset.h>
#include <linux/regmap.h>
#include <linux/mfd/syscon.h>
#include <media/v4l2-ctrls.h>
@@ -310,6 +311,7 @@ struct aspeed_video {
void __iomem *base;
struct clk *eclk;
struct clk *vclk;
+ struct reset_control *reset;
struct device *dev;
struct v4l2_ctrl_handler ctrl_handler;
@@ -704,6 +706,9 @@ static void aspeed_video_off(struct aspeed_video *video)
aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
+ eset_control_assert(video->reset);
+ usleep_range(100, 200);
+
Regards,
Jammy Huang
> -----Original Message-----
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Sent: Thursday, August 8, 2024 3:17 PM
> To: Phil Eichinger <phil@xxxxxxxxxxxxx>; eajames@xxxxxxxxxxxxx;
> mchehab@xxxxxxxxxx; joel@xxxxxxxxx; andrew@xxxxxxxxxxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; jae.hyun.yoo@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Jammy Huang
> <jammy_huang@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH] media: aspeed: fix clock stopping logic
>
> +Jammy Huang
>
> Jammy,
>
> Can you review this patch? It looks OK to me, but I wonder if in
> aspeed_video_on the order of the clocks should be reversed as well to match
> the new video_off sequence.
>
> Regards,
>
> Hans
>
> On 19/07/2024 11:40, Phil Eichinger wrote:
> > When stopping clocks for Video Capture and Video Engine in
> > aspeed_video_off() the order is reversed.
> >
> > Occasionally during screen blanking hard lock-ups occur on AST2500,
> > accompanied by the heart beat LED stopping.
> >
> > Stopping Video Capture clock before Video Engine seems logical and
> > fixes the random lock-ups.
> >
> > Fixes: 3536169f8531 ("media: aspeed: fix clock handling logic")
> > Signed-off-by: Phil Eichinger <phil@xxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/aspeed/aspeed-video.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/aspeed/aspeed-video.c
> > b/drivers/media/platform/aspeed/aspeed-video.c
> > index fc6050e3be0d..8f1f3c847162 100644
> > --- a/drivers/media/platform/aspeed/aspeed-video.c
> > +++ b/drivers/media/platform/aspeed/aspeed-video.c
> > @@ -661,8 +661,8 @@ static void aspeed_video_off(struct aspeed_video
> *video)
> > aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
> >
> > /* Turn off the relevant clocks */
> > - clk_disable(video->eclk);
> > clk_disable(video->vclk);
> > + clk_disable(video->eclk);
> >
> > clear_bit(VIDEO_CLOCKS_ON, &video->flags); }
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.