lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <TYZPR06MB65685ED0C73E35A848004820F1852@TYZPR06MB6568.apcprd06.prod.outlook.com>
Date: Mon, 12 Aug 2024 08:05:52 +0000
From: Jammy Huang <jammy_huang@...eedtech.com>
To: Hans Verkuil <hverkuil-cisco@...all.nl>, Phil Eichinger
	<phil@...kapfel.net>, "eajames@...ux.ibm.com" <eajames@...ux.ibm.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>, "joel@....id.au" <joel@....id.au>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"sboyd@...nel.org" <sboyd@...nel.org>, "jae.hyun.yoo@...ux.intel.com"
	<jae.hyun.yoo@...ux.intel.com>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] media: aspeed: fix clock stopping logic

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@...00000 {
                                         <&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@...00000 {
                                         <&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@...00000 {
                                         <&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@...all.nl>
> Sent: Thursday, August 8, 2024 3:17 PM
> To: Phil Eichinger <phil@...kapfel.net>; eajames@...ux.ibm.com;
> mchehab@...nel.org; joel@....id.au; andrew@...econstruct.com.au;
> sboyd@...nel.org; jae.hyun.yoo@...ux.intel.com; linux-media@...r.kernel.org;
> linux-kernel@...r.kernel.org; Jammy Huang
> <jammy_huang@...eedtech.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@...kapfel.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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ