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
