| Message ID | 20240213181233.242316-6-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | RZ/G2L CSI/CRU improvements | expand |
Hi Biju, Thank you for the patch. On Tue, Feb 13, 2024 at 06:12:33PM +0000, Biju Das wrote: > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the > latest hardware manual (R01UH0914EJ0145 Rev.1.45) we need to supply all > the clocks and then release the CRU resets. Currently we are releasing > the resets and then supplying the clocks. So, fix the start reception > procedure. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v3: > * New patch. > --- > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 59 +++++++++---------- > 1 file changed, 28 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index d15a9bfcc98b..b16b8af6e8f8 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -489,39 +489,24 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > > video_device_pipeline_stop(&cru->vdev); > > - pm_runtime_put_sync(cru->dev); > - clk_disable_unprepare(cru->vclk); > - > return stream_off_ret; > } > > - ret = pm_runtime_resume_and_get(cru->dev); > - if (ret) > - return ret; > - > - ret = clk_prepare_enable(cru->vclk); > - if (ret) > - goto err_pm_put; > - > ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > if (ret) > - goto err_vclk_disable; > + return ret; > > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > ret = video_device_pipeline_start(&cru->vdev, pipe); > if (ret) > - goto err_vclk_disable; > + return ret; > > ret = v4l2_subdev_call(sd, video, pre_streamon, 0); > - if (ret == -ENOIOCTLCMD) > - ret = 0; > - if (ret) > + if (ret && ret != -ENOIOCTLCMD) > goto pipe_line_stop; > > ret = v4l2_subdev_call(sd, video, s_stream, 1); > - if (ret == -ENOIOCTLCMD) > - ret = 0; > - if (ret) > + if (ret && ret != -ENOIOCTLCMD) > goto err_s_stream; > > return 0; > @@ -532,12 +517,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > pipe_line_stop: > video_device_pipeline_stop(&cru->vdev); > > -err_vclk_disable: > - clk_disable_unprepare(cru->vclk); > - > -err_pm_put: > - pm_runtime_put_sync(cru->dev); > - > return ret; > } > > @@ -636,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count > struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq); > int ret; > > + ret = pm_runtime_resume_and_get(cru->dev); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(cru->vclk); > + if (ret) > + goto err_pm_put; > + > /* Release reset state */ > ret = reset_control_deassert(cru->aresetn); > if (ret) { > dev_err(cru->dev, "failed to deassert aresetn\n"); > - return ret; > + goto err_vclk_disable; > } > > ret = reset_control_deassert(cru->presetn); > if (ret) { > reset_control_assert(cru->aresetn); > dev_err(cru->dev, "failed to deassert presetn\n"); > - return ret; > + goto assert_aresetn; > } > > ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq, > IRQF_SHARED, KBUILD_MODNAME, cru); Requesting the IRQ every time the device is started seems strange. That's not related to this patch, but you may want to address it in a separate series. Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > if (ret) { > dev_err(cru->dev, "failed to request irq\n"); > - goto assert_resets; > + goto assert_presetn; > } > > /* Allocate scratch buffer. */ > @@ -686,10 +673,18 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count > free_image_conv_irq: > free_irq(cru->image_conv_irq, cru); > > -assert_resets: > +assert_presetn: > reset_control_assert(cru->presetn); > + > +assert_aresetn: > reset_control_assert(cru->aresetn); > > +err_vclk_disable: > + clk_disable_unprepare(cru->vclk); > + > +err_pm_put: > + pm_runtime_put_sync(cru->dev); > + > return ret; > } > > @@ -704,9 +699,11 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq) > cru->scratch, cru->scratch_phys); > > free_irq(cru->image_conv_irq, cru); > - reset_control_assert(cru->presetn); > - > return_unused_buffers(cru, VB2_BUF_STATE_ERROR); > + > + reset_control_assert(cru->presetn); > + clk_disable_unprepare(cru->vclk); > + pm_runtime_put_sync(cru->dev); > } > > static const struct vb2_ops rzg2l_cru_qops = {
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Wednesday, February 14, 2024 2:56 PM > Subject: Re: [PATCH v3 5/5] media: platform: rzg2l-cru: rzg2l-video: Fix > start reception procedure > > Hi Biju, > > Thank you for the patch. > > On Tue, Feb 13, 2024 at 06:12:33PM +0000, Biju Das wrote: > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on > > the latest hardware manual (R01UH0914EJ0145 Rev.1.45) we need to > > supply all the clocks and then release the CRU resets. Currently we > > are releasing the resets and then supplying the clocks. So, fix the > > start reception procedure. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v3: > > * New patch. > > --- > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 59 > > +++++++++---------- > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index d15a9bfcc98b..b16b8af6e8f8 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -489,39 +489,24 @@ static int rzg2l_cru_set_stream(struct > > rzg2l_cru_dev *cru, int on) > > > > video_device_pipeline_stop(&cru->vdev); > > > > - pm_runtime_put_sync(cru->dev); > > - clk_disable_unprepare(cru->vclk); > > - > > return stream_off_ret; > > } > > > > - ret = pm_runtime_resume_and_get(cru->dev); > > - if (ret) > > - return ret; > > - > > - ret = clk_prepare_enable(cru->vclk); > > - if (ret) > > - goto err_pm_put; > > - > > ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > > if (ret) > > - goto err_vclk_disable; > > + return ret; > > > > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > > ret = video_device_pipeline_start(&cru->vdev, pipe); > > if (ret) > > - goto err_vclk_disable; > > + return ret; > > > > ret = v4l2_subdev_call(sd, video, pre_streamon, 0); > > - if (ret == -ENOIOCTLCMD) > > - ret = 0; > > - if (ret) > > + if (ret && ret != -ENOIOCTLCMD) > > goto pipe_line_stop; > > > > ret = v4l2_subdev_call(sd, video, s_stream, 1); > > - if (ret == -ENOIOCTLCMD) > > - ret = 0; > > - if (ret) > > + if (ret && ret != -ENOIOCTLCMD) > > goto err_s_stream; > > > > return 0; > > @@ -532,12 +517,6 @@ static int rzg2l_cru_set_stream(struct > > rzg2l_cru_dev *cru, int on) > > pipe_line_stop: > > video_device_pipeline_stop(&cru->vdev); > > > > -err_vclk_disable: > > - clk_disable_unprepare(cru->vclk); > > - > > -err_pm_put: > > - pm_runtime_put_sync(cru->dev); > > - > > return ret; > > } > > > > @@ -636,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct > vb2_queue *vq, unsigned int count > > struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq); > > int ret; > > > > + ret = pm_runtime_resume_and_get(cru->dev); > > + if (ret) > > + return ret; > > + > > + ret = clk_prepare_enable(cru->vclk); > > + if (ret) > > + goto err_pm_put; > > + > > /* Release reset state */ > > ret = reset_control_deassert(cru->aresetn); > > if (ret) { > > dev_err(cru->dev, "failed to deassert aresetn\n"); > > - return ret; > > + goto err_vclk_disable; > > } > > > > ret = reset_control_deassert(cru->presetn); > > if (ret) { > > reset_control_assert(cru->aresetn); > > dev_err(cru->dev, "failed to deassert presetn\n"); > > - return ret; > > + goto assert_aresetn; > > } > > > > ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq, > > IRQF_SHARED, KBUILD_MODNAME, cru); > > Requesting the IRQ every time the device is started seems strange. > That's not related to this patch, but you may want to address it in a > separate series. Agreed. Will send a separate patch. Cheers, Biju
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c index d15a9bfcc98b..b16b8af6e8f8 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c @@ -489,39 +489,24 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) video_device_pipeline_stop(&cru->vdev); - pm_runtime_put_sync(cru->dev); - clk_disable_unprepare(cru->vclk); - return stream_off_ret; } - ret = pm_runtime_resume_and_get(cru->dev); - if (ret) - return ret; - - ret = clk_prepare_enable(cru->vclk); - if (ret) - goto err_pm_put; - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); if (ret) - goto err_vclk_disable; + return ret; pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; ret = video_device_pipeline_start(&cru->vdev, pipe); if (ret) - goto err_vclk_disable; + return ret; ret = v4l2_subdev_call(sd, video, pre_streamon, 0); - if (ret == -ENOIOCTLCMD) - ret = 0; - if (ret) + if (ret && ret != -ENOIOCTLCMD) goto pipe_line_stop; ret = v4l2_subdev_call(sd, video, s_stream, 1); - if (ret == -ENOIOCTLCMD) - ret = 0; - if (ret) + if (ret && ret != -ENOIOCTLCMD) goto err_s_stream; return 0; @@ -532,12 +517,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) pipe_line_stop: video_device_pipeline_stop(&cru->vdev); -err_vclk_disable: - clk_disable_unprepare(cru->vclk); - -err_pm_put: - pm_runtime_put_sync(cru->dev); - return ret; } @@ -636,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq); int ret; + ret = pm_runtime_resume_and_get(cru->dev); + if (ret) + return ret; + + ret = clk_prepare_enable(cru->vclk); + if (ret) + goto err_pm_put; + /* Release reset state */ ret = reset_control_deassert(cru->aresetn); if (ret) { dev_err(cru->dev, "failed to deassert aresetn\n"); - return ret; + goto err_vclk_disable; } ret = reset_control_deassert(cru->presetn); if (ret) { reset_control_assert(cru->aresetn); dev_err(cru->dev, "failed to deassert presetn\n"); - return ret; + goto assert_aresetn; } ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq, IRQF_SHARED, KBUILD_MODNAME, cru); if (ret) { dev_err(cru->dev, "failed to request irq\n"); - goto assert_resets; + goto assert_presetn; } /* Allocate scratch buffer. */ @@ -686,10 +673,18 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count free_image_conv_irq: free_irq(cru->image_conv_irq, cru); -assert_resets: +assert_presetn: reset_control_assert(cru->presetn); + +assert_aresetn: reset_control_assert(cru->aresetn); +err_vclk_disable: + clk_disable_unprepare(cru->vclk); + +err_pm_put: + pm_runtime_put_sync(cru->dev); + return ret; } @@ -704,9 +699,11 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq) cru->scratch, cru->scratch_phys); free_irq(cru->image_conv_irq, cru); - reset_control_assert(cru->presetn); - return_unused_buffers(cru, VB2_BUF_STATE_ERROR); + + reset_control_assert(cru->presetn); + clk_disable_unprepare(cru->vclk); + pm_runtime_put_sync(cru->dev); } static const struct vb2_ops rzg2l_cru_qops = {
As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the latest hardware manual (R01UH0914EJ0145 Rev.1.45) we need to supply all the clocks and then release the CRU resets. Currently we are releasing the resets and then supplying the clocks. So, fix the start reception procedure. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v3: * New patch. --- .../platform/renesas/rzg2l-cru/rzg2l-video.c | 59 +++++++++---------- 1 file changed, 28 insertions(+), 31 deletions(-)