reset should be more proper than clk off/on to bring HW back to good
state.
Signed-off-by: Jammy Huang <[email protected]>
---
drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index fea5e4d0927e..10d182139809 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -23,6 +23,7 @@
#include <linux/workqueue.h>
#include <linux/debugfs.h>
#include <linux/ktime.h>
+#include <linux/reset.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-dev.h>
#include <media/v4l2-device.h>
@@ -220,6 +221,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;
@@ -554,6 +556,13 @@ static void aspeed_video_on(struct aspeed_video *video)
set_bit(VIDEO_CLOCKS_ON, &video->flags);
}
+static void aspeed_video_reset(struct aspeed_video *v)
+{
+ reset_control_assert(v->reset);
+ udelay(100);
+ reset_control_deassert(v->reset);
+}
+
static void aspeed_video_bufs_done(struct aspeed_video *video,
enum vb2_buffer_state state)
{
@@ -574,7 +583,9 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
set_bit(VIDEO_RES_CHANGE, &video->flags);
clear_bit(VIDEO_FRAME_INPRG, &video->flags);
- aspeed_video_off(video);
+ aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
+ aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
+ aspeed_video_reset(video);
aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
schedule_delayed_work(&video->res_work, delay);
@@ -1507,8 +1518,7 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
* Need to force stop any DMA and try and get HW into a good
* state for future calls to start streaming again.
*/
- aspeed_video_off(video);
- aspeed_video_on(video);
+ aspeed_video_reset(video);
aspeed_video_init_regs(video);
@@ -1715,6 +1725,12 @@ static int aspeed_video_init(struct aspeed_video *video)
return rc;
}
+ video->reset = devm_reset_control_get(dev, NULL);
+ if (IS_ERR(video->reset)) {
+ dev_err(dev, "Unable to get reset\n");
+ return PTR_ERR(video->reset);
+ }
+
video->eclk = devm_clk_get(dev, "eclk");
if (IS_ERR(video->eclk)) {
dev_err(dev, "Unable to get ECLK\n");
--
2.25.1
Hi Jammy,
On 11/2/2021 10:43 PM, Jammy Huang wrote:
> reset should be more proper than clk off/on to bring HW back to good
> state.
>
> Signed-off-by: Jammy Huang <[email protected]>
> ---
> drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index fea5e4d0927e..10d182139809 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -23,6 +23,7 @@
> #include <linux/workqueue.h>
> #include <linux/debugfs.h>
> #include <linux/ktime.h>
> +#include <linux/reset.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-dev.h>
> #include <media/v4l2-device.h>
> @@ -220,6 +221,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;
> @@ -554,6 +556,13 @@ static void aspeed_video_on(struct aspeed_video *video)
> set_bit(VIDEO_CLOCKS_ON, &video->flags);
> }
>
> +static void aspeed_video_reset(struct aspeed_video *v)
> +{
> + reset_control_assert(v->reset);
> + udelay(100);
> + reset_control_deassert(v->reset);
> +}
> +
> static void aspeed_video_bufs_done(struct aspeed_video *video,
> enum vb2_buffer_state state)
> {
> @@ -574,7 +583,9 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
> set_bit(VIDEO_RES_CHANGE, &video->flags);
> clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>
> - aspeed_video_off(video);
> + aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
> + aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
> + aspeed_video_reset(video);
> aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>
> schedule_delayed_work(&video->res_work, delay);
> @@ -1507,8 +1518,7 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
> * Need to force stop any DMA and try and get HW into a good
> * state for future calls to start streaming again.
> */
> - aspeed_video_off(video);
> - aspeed_video_on(video);
> + aspeed_video_reset(video);
You can find the ECLK configuration in 'clk-aspeed.c' or in
'clk-ast2600.c' that it's coupled with the video engine reset (SCU04[6]
for AST2500 / SCU040[6] for AST2600). It means that if we call
clk_disable() and clk_enable() through aspeed_video_off() and
aspeed_video_on(), the video engine reset will be implicitly asserted
and de-asserted by the clock driver so the reset mechanism is already in
the existing code.
Thanks,
Jae
> aspeed_video_init_regs(video);
>
> @@ -1715,6 +1725,12 @@ static int aspeed_video_init(struct aspeed_video *video)
> return rc;
> }
>
> + video->reset = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(video->reset)) {
> + dev_err(dev, "Unable to get reset\n");
> + return PTR_ERR(video->reset);
> + }
> +
> video->eclk = devm_clk_get(dev, "eclk");
> if (IS_ERR(video->eclk)) {
> dev_err(dev, "Unable to get ECLK\n");
>
Hi Jae,
OK, I found it.
Thanks for your help.
Regards,
Jammy Huang
Tel: 886-3-575-1185 ext.8630
************* 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.
-----Original Message-----
From: Jae Hyun Yoo <[email protected]>
Sent: Thursday, November 4, 2021 11:54 PM
To: Jammy Huang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] media: aspeed: use reset to replace clk off/on
Hi Jammy,
On 11/2/2021 10:43 PM, Jammy Huang wrote:
> reset should be more proper than clk off/on to bring HW back to good
> state.
>
> Signed-off-by: Jammy Huang <[email protected]>
> ---
> drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c
> b/drivers/media/platform/aspeed-video.c
> index fea5e4d0927e..10d182139809 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -23,6 +23,7 @@
> #include <linux/workqueue.h>
> #include <linux/debugfs.h>
> #include <linux/ktime.h>
> +#include <linux/reset.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-dev.h>
> #include <media/v4l2-device.h>
> @@ -220,6 +221,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; @@ -554,6 +556,13 @@ static
> void aspeed_video_on(struct aspeed_video *video)
> set_bit(VIDEO_CLOCKS_ON, &video->flags);
> }
>
> +static void aspeed_video_reset(struct aspeed_video *v) {
> + reset_control_assert(v->reset);
> + udelay(100);
> + reset_control_deassert(v->reset);
> +}
> +
> static void aspeed_video_bufs_done(struct aspeed_video *video,
> enum vb2_buffer_state state)
> {
> @@ -574,7 +583,9 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
> set_bit(VIDEO_RES_CHANGE, &video->flags);
> clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>
> - aspeed_video_off(video);
> + aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
> + aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
> + aspeed_video_reset(video);
> aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>
> schedule_delayed_work(&video->res_work, delay); @@ -1507,8 +1518,7
> @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
> * Need to force stop any DMA and try and get HW into a good
> * state for future calls to start streaming again.
> */
> - aspeed_video_off(video);
> - aspeed_video_on(video);
> + aspeed_video_reset(video);
You can find the ECLK configuration in 'clk-aspeed.c' or in 'clk-ast2600.c' that it's coupled with the video engine reset (SCU04[6] for AST2500 / SCU040[6] for AST2600). It means that if we call
clk_disable() and clk_enable() through aspeed_video_off() and aspeed_video_on(), the video engine reset will be implicitly asserted and de-asserted by the clock driver so the reset mechanism is already in the existing code.
Thanks,
Jae
> aspeed_video_init_regs(video);
>
> @@ -1715,6 +1725,12 @@ static int aspeed_video_init(struct aspeed_video *video)
> return rc;
> }
>
> + video->reset = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(video->reset)) {
> + dev_err(dev, "Unable to get reset\n");
> + return PTR_ERR(video->reset);
> + }
> +
> video->eclk = devm_clk_get(dev, "eclk");
> if (IS_ERR(video->eclk)) {
> dev_err(dev, "Unable to get ECLK\n");
>
Hi Jammy,
On 05/11/2021 02:27, Jammy Huang wrote:
> Hi Jae,
>
> OK, I found it.
> Thanks for your help.
So just so I understand this correctly: this patch can be dropped, right?
Regards,
Hans
>
> Regards,
> Jammy Huang
>
> Tel: 886-3-575-1185 ext.8630
>
> ************* 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.
>
> -----Original Message-----
> From: Jae Hyun Yoo <[email protected]>
> Sent: Thursday, November 4, 2021 11:54 PM
> To: Jammy Huang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] media: aspeed: use reset to replace clk off/on
>
> Hi Jammy,
>
> On 11/2/2021 10:43 PM, Jammy Huang wrote:
>> reset should be more proper than clk off/on to bring HW back to good
>> state.
>>
>> Signed-off-by: Jammy Huang <[email protected]>
>> ---
>> drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c
>> b/drivers/media/platform/aspeed-video.c
>> index fea5e4d0927e..10d182139809 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -23,6 +23,7 @@
>> #include <linux/workqueue.h>
>> #include <linux/debugfs.h>
>> #include <linux/ktime.h>
>> +#include <linux/reset.h>
>> #include <media/v4l2-ctrls.h>
>> #include <media/v4l2-dev.h>
>> #include <media/v4l2-device.h>
>> @@ -220,6 +221,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; @@ -554,6 +556,13 @@ static
>> void aspeed_video_on(struct aspeed_video *video)
>> set_bit(VIDEO_CLOCKS_ON, &video->flags);
>> }
>>
>> +static void aspeed_video_reset(struct aspeed_video *v) {
>> + reset_control_assert(v->reset);
>> + udelay(100);
>> + reset_control_deassert(v->reset);
>> +}
>> +
>> static void aspeed_video_bufs_done(struct aspeed_video *video,
>> enum vb2_buffer_state state)
>> {
>> @@ -574,7 +583,9 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
>> set_bit(VIDEO_RES_CHANGE, &video->flags);
>> clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>>
>> - aspeed_video_off(video);
>> + aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>> + aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
>> + aspeed_video_reset(video);
>> aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>>
>> schedule_delayed_work(&video->res_work, delay); @@ -1507,8 +1518,7
>> @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
>> * Need to force stop any DMA and try and get HW into a good
>> * state for future calls to start streaming again.
>> */
>> - aspeed_video_off(video);
>> - aspeed_video_on(video);
>> + aspeed_video_reset(video);
>
> You can find the ECLK configuration in 'clk-aspeed.c' or in 'clk-ast2600.c' that it's coupled with the video engine reset (SCU04[6] for AST2500 / SCU040[6] for AST2600). It means that if we call
> clk_disable() and clk_enable() through aspeed_video_off() and aspeed_video_on(), the video engine reset will be implicitly asserted and de-asserted by the clock driver so the reset mechanism is already in the existing code.
>
> Thanks,
> Jae
>
>> aspeed_video_init_regs(video);
>>
>> @@ -1715,6 +1725,12 @@ static int aspeed_video_init(struct aspeed_video *video)
>> return rc;
>> }
>>
>> + video->reset = devm_reset_control_get(dev, NULL);
>> + if (IS_ERR(video->reset)) {
>> + dev_err(dev, "Unable to get reset\n");
>> + return PTR_ERR(video->reset);
>> + }
>> +
>> video->eclk = devm_clk_get(dev, "eclk");
>> if (IS_ERR(video->eclk)) {
>> dev_err(dev, "Unable to get ECLK\n");
>>
Dear Hans,
On 2021/11/8 下午 04:53, Hans Verkuil wrote:
> Hi Jammy,
>
> On 05/11/2021 02:27, Jammy Huang wrote:
>> Hi Jae,
>>
>> OK, I found it.
>> Thanks for your help.
> So just so I understand this correctly: this patch can be dropped, right?
>
> Regards,
>
> Hans
Yes, aspeed clk driver will reset the related engine at clk-enabled.
Thus, this modification isn't necessary.
>> Regards,
>> Jammy Huang
>>
>> Tel: 886-3-575-1185 ext.8630
>>
>> ************* 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.
>>
>> -----Original Message-----
>> From: Jae Hyun Yoo <[email protected]>
>> Sent: Thursday, November 4, 2021 11:54 PM
>> To: Jammy Huang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH] media: aspeed: use reset to replace clk off/on
>>
>> Hi Jammy,
>>
>> On 11/2/2021 10:43 PM, Jammy Huang wrote:
>>> reset should be more proper than clk off/on to bring HW back to good
>>> state.
>>>
>>> Signed-off-by: Jammy Huang <[email protected]>
>>> ---
>>> drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++---
>>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/aspeed-video.c
>>> b/drivers/media/platform/aspeed-video.c
>>> index fea5e4d0927e..10d182139809 100644
>>> --- a/drivers/media/platform/aspeed-video.c
>>> +++ b/drivers/media/platform/aspeed-video.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/workqueue.h>
>>> #include <linux/debugfs.h>
>>> #include <linux/ktime.h>
>>> +#include <linux/reset.h>
>>> #include <media/v4l2-ctrls.h>
>>> #include <media/v4l2-dev.h>
>>> #include <media/v4l2-device.h>
>>> @@ -220,6 +221,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; @@ -554,6 +556,13 @@ static
>>> void aspeed_video_on(struct aspeed_video *video)
>>> set_bit(VIDEO_CLOCKS_ON, &video->flags);
>>> }
>>>
>>> +static void aspeed_video_reset(struct aspeed_video *v) {
>>> + reset_control_assert(v->reset);
>>> + udelay(100);
>>> + reset_control_deassert(v->reset);
>>> +}
>>> +
>>> static void aspeed_video_bufs_done(struct aspeed_video *video,
>>> enum vb2_buffer_state state)
>>> {
>>> @@ -574,7 +583,9 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
>>> set_bit(VIDEO_RES_CHANGE, &video->flags);
>>> clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>>>
>>> - aspeed_video_off(video);
>>> + aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
>>> + aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
>>> + aspeed_video_reset(video);
>>> aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>>>
>>> schedule_delayed_work(&video->res_work, delay); @@ -1507,8 +1518,7
>>> @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
>>> * Need to force stop any DMA and try and get HW into a good
>>> * state for future calls to start streaming again.
>>> */
>>> - aspeed_video_off(video);
>>> - aspeed_video_on(video);
>>> + aspeed_video_reset(video);
>> You can find the ECLK configuration in 'clk-aspeed.c' or in 'clk-ast2600.c' that it's coupled with the video engine reset (SCU04[6] for AST2500 / SCU040[6] for AST2600). It means that if we call
>> clk_disable() and clk_enable() through aspeed_video_off() and aspeed_video_on(), the video engine reset will be implicitly asserted and de-asserted by the clock driver so the reset mechanism is already in the existing code.
>>
>> Thanks,
>> Jae
>>
>>> aspeed_video_init_regs(video);
>>>
>>> @@ -1715,6 +1725,12 @@ static int aspeed_video_init(struct aspeed_video *video)
>>> return rc;
>>> }
>>>
>>> + video->reset = devm_reset_control_get(dev, NULL);
>>> + if (IS_ERR(video->reset)) {
>>> + dev_err(dev, "Unable to get reset\n");
>>> + return PTR_ERR(video->reset);
>>> + }
>>> +
>>> video->eclk = devm_clk_get(dev, "eclk");
>>> if (IS_ERR(video->eclk)) {
>>> dev_err(dev, "Unable to get ECLK\n");
>>>
--
Best Regards
Jammy