Re: [PATCH] media: aspeed: fix clock stopping logic

[Phil Eichinger <phil@zankapfel.net>]

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