* [PATCH] media: aspeed: fix clock stopping logic
@ 2024-07-19 9:40 Phil Eichinger
2024-08-08 7:16 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Phil Eichinger @ 2024-07-19 9:40 UTC (permalink / raw)
To: eajames, mchehab, joel, andrew, sboyd, hverkuil-cisco,
jae.hyun.yoo, linux-media, linux-kernel
Cc: Phil Eichinger
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);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] media: aspeed: fix clock stopping logic 2024-07-19 9:40 [PATCH] media: aspeed: fix clock stopping logic Phil Eichinger @ 2024-08-08 7:16 ` Hans Verkuil 2024-08-12 8:05 ` Jammy Huang 0 siblings, 1 reply; 4+ messages in thread From: Hans Verkuil @ 2024-08-08 7:16 UTC (permalink / raw) To: Phil Eichinger, eajames, mchehab, joel, andrew, sboyd, jae.hyun.yoo, linux-media, linux-kernel, Jammy Huang +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); > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] media: aspeed: fix clock stopping logic 2024-08-08 7:16 ` Hans Verkuil @ 2024-08-12 8:05 ` Jammy Huang 2024-08-16 8:12 ` Phil Eichinger 0 siblings, 1 reply; 4+ messages in thread From: Jammy Huang @ 2024-08-12 8:05 UTC (permalink / raw) To: Hans Verkuil, Phil Eichinger, 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 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. ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: aspeed: fix clock stopping logic 2024-08-12 8:05 ` Jammy Huang @ 2024-08-16 8:12 ` Phil Eichinger 0 siblings, 0 replies; 4+ messages in thread From: Phil Eichinger @ 2024-08-16 8:12 UTC (permalink / raw) To: Jammy Huang Cc: Hans Verkuil, 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 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-16 8:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-19 9:40 [PATCH] media: aspeed: fix clock stopping logic Phil Eichinger 2024-08-08 7:16 ` Hans Verkuil 2024-08-12 8:05 ` Jammy Huang 2024-08-16 8:12 ` Phil Eichinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox