| 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
| ||
|
Message-ID: <Zr8J4V/OL++W+lp1@zankapfel.net>
Date: Fri, 16 Aug 2024 10:12:17 +0200
From: Phil Eichinger <phil@...kapfel.net>
To: Jammy Huang <jammy_huang@...eedtech.com>
Cc: Hans Verkuil <hverkuil-cisco@...all.nl>,
"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 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@...00000 {
> <&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@...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";
> };
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@...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);
> +
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
Powered by blists - more mailing lists