| Message ID | 20211103054316.25272-1-jammy_huang@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: aspeed: use reset to replace clk off/on | expand |
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 <jammy_huang@aspeedtech.com> > --- > 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 <jae.hyun.yoo@linux.intel.com> Sent: Thursday, November 4, 2021 11:54 PM To: Jammy Huang <jammy_huang@aspeedtech.com>; eajames@linux.ibm.com; mchehab@kernel.org; joel@jms.id.au; andrew@aj.id.au; linux-media@vger.kernel.org; openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org 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 <jammy_huang@aspeedtech.com> > --- > 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 <jae.hyun.yoo@linux.intel.com> > Sent: Thursday, November 4, 2021 11:54 PM > To: Jammy Huang <jammy_huang@aspeedtech.com>; eajames@linux.ibm.com; mchehab@kernel.org; joel@jms.id.au; andrew@aj.id.au; linux-media@vger.kernel.org; openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > 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 <jammy_huang@aspeedtech.com> >> --- >> 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 <jae.hyun.yoo@linux.intel.com> >> Sent: Thursday, November 4, 2021 11:54 PM >> To: Jammy Huang <jammy_huang@aspeedtech.com>; eajames@linux.ibm.com; mchehab@kernel.org; joel@jms.id.au; andrew@aj.id.au; linux-media@vger.kernel.org; openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> 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 <jammy_huang@aspeedtech.com> >>> --- >>> 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"); >>>
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");
reset should be more proper than clk off/on to bring HW back to good state. Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> --- drivers/media/platform/aspeed-video.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)