[PATCH] media: aspeed: fix clock stopping logic

[PATCH] media: aspeed: fix clock stopping logic

[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

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

[Hans Verkuil]

+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);
>  }

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

[Jammy Huang]

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.

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

[Phil Eichinger]

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