| Message ID | 20240719094056.1169057-1-phil@zankapfel.net |
|---|---|
| State | New |
| Headers | show |
| Series | media: aspeed: fix clock stopping logic | expand |
+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@zankapfel.net> > --- > 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); > }
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@xs4all.nl>
> Sent: Thursday, August 8, 2024 3:17 PM
> To: Phil Eichinger <phil@zankapfel.net>; eajames@linux.ibm.com;
> mchehab@kernel.org; joel@jms.id.au; andrew@codeconstruct.com.au;
> sboyd@kernel.org; jae.hyun.yoo@linux.intel.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; Jammy Huang
> <jammy_huang@aspeedtech.com>
> 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@zankapfel.net>
> > ---
> > 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.
Hi Jammy, please see my comments below: On Mon, Aug 12, 2024 at 08:05:52AM +0000, Jammy Huang wrote: > 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"; > }; ASPEED_RESET_VIDEO does not exist in mainline for AST2500. I have added it to drivers/clk/clk-aspeed.c and include/dt-bindings/clock/aspeed-clock.h like in the Aspeed fork. > 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"; > }; This is bogus. > 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); > + I assume you meant reset_control_assert()? Anyway I got your patch compilable by adding ASPEED_RESET_VIDEO like so: diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c index 411ff5fb2c07..9684fb086d38 100644 --- a/drivers/clk/clk-aspeed.c +++ b/drivers/clk/clk-aspeed.c @@ -278,6 +278,7 @@ static const u8 aspeed_resets[] = { [ASPEED_RESET_PECI] = 10, [ASPEED_RESET_I2C] = 2, [ASPEED_RESET_AHB] = 1, + [ASPEED_RESET_VIDEO] = 6, /* * SCUD4 resets start at an * offset to separate them From diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h index 06d568382c77..421ca577c1b2 100644 --- a/include/dt-bindings/clock/aspeed-clock.h +++ b/include/dt-bindings/clock/aspeed-clock.h @@ -53,5 +53,6 @@ #define ASPEED_RESET_AHB 8 #define ASPEED_RESET_CRT1 9 #define ASPEED_RESET_HACE 10 +#define ASPEED_RESET_VIDEO 21 Anyways during testing it almost immediately caused the crash again, when the clocks were disabled in the original order. Cheers, Phil
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); }
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@zankapfel.net> --- drivers/media/platform/aspeed/aspeed-video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)