Re: [PATCH 6/9] media: rzg2l-cru: Add suspend/resume support

Hi Tommaso

On Tue, Jun 16, 2026 at 07:05:36PM +0200, Tommaso Merciai wrote:
> The CRU has no system sleep hooks, leaving the device in an undefined
> state across suspend/resume.
>
> On suspend, stop the pipeline, requeue any buffers held by the hardware
> back to the software queue, and assert the resets. On resume, deassert
> the resets and restart streaming.
>
> Add a bool running field to track pipeline state. stop_streaming uses it
> to skip rzg2l_cru_set_stream() when the pipeline was already stopped by
> a failed resume, avoiding a double-stop. Export rzg2l_cru_set_stream()
> and add rzg2l_cru_requeue_active_buffers() for use by the PM callbacks.
>
> Signed-off-by: Tommaso Merciai <[email protected]>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-core.c   | 67 +++++++++++++++++++
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  5 ++
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 25 ++++++-
>  3 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> index 1b12d91eaec9..2840f40e4c01 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> @@ -246,6 +246,72 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru)
>  	return 0;
>  }
>
> +static int rzg2l_cru_pm_suspend(struct device *dev)
> +{
> +	struct rzg2l_cru_dev *cru = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = cru->aresetn },
> +		{ .rstc = cru->presetn },
> +	};

You're re-creating this struct in a few places. Isn't it worth to move
struct reset_control_bulk_data resets[] to struct rzg2l_cru_dev and
populate it at probe time ?

> +	int ret;
> +
> +	if (!cru->running)
> +		return 0;
> +
> +	ret = rzg2l_cru_set_stream(cru, 0);
> +	if (ret)
> +		return ret;
> +
> +	rzg2l_cru_requeue_active_buffers(cru);
> +
> +	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
> +	if (ret) {
> +		if (rzg2l_cru_set_stream(cru, 1))
> +			vb2_queue_error(&cru->queue);
> +
> +		return ret;
> +	}

Do you need to prepare_enble/disable_unprepare clocks as well or only
resets ?

If you need clocks as well, then this section will become very similar
to what follows pm_runtime_resume_and_get()/precedes
pm_runtime_suspend in rzg2l_cru_start_streaming_vq() and
rzg2l_cru_stop_streaming_vq() respectively.

At this point you could break out the clock/reset handling to
dedicated functions and also install runtime_pm handlers, and re-use
the same routines here and in the below resume ?

> +
> +	return 0;
> +}
> +
> +static int rzg2l_cru_pm_resume(struct device *dev)
> +{
> +	struct rzg2l_cru_dev *cru = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = cru->aresetn },
> +		{ .rstc = cru->presetn },
> +	};
> +	int ret;
> +
> +	if (!cru->running)
> +		return 0;
> +
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
> +	if (ret)
> +		goto err_running;
> +
> +	ret = rzg2l_cru_set_stream(cru, 1);
> +	if (ret) {
> +		dev_err(cru->dev, "Failed to restart streaming: %d\n", ret);
> +		goto err_reset_assert;
> +	}
> +
> +	return 0;
> +
> +err_reset_assert:
> +	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
> +err_running:
> +	cru->running = false;
> +	vb2_queue_error(&cru->queue);
> +
> +	return ret;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(rzg2l_cru_pm_ops,
> +				rzg2l_cru_pm_suspend,
> +				rzg2l_cru_pm_resume);
> +
>  static int rzg2l_cru_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -437,6 +503,7 @@ static struct platform_driver rzg2l_cru_driver = {
>  	.driver = {
>  		.name = "rzg2l-cru",
>  		.of_match_table = rzg2l_cru_of_id_table,
> +		.pm = pm_sleep_ptr(&rzg2l_cru_pm_ops),
>  	},
>  	.probe = rzg2l_cru_probe,
>  	.remove = rzg2l_cru_remove,
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 5bf334e173d2..c079cad41266 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -161,12 +161,17 @@ struct rzg2l_cru_dev {
>  	struct list_head buf_list;
>  	unsigned int sequence;
>
> +	bool running;
> +
>  	struct v4l2_pix_format format;
>  };
>
>  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
>
> +int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on);
> +void rzg2l_cru_requeue_active_buffers(struct rzg2l_cru_dev *cru);
> +
>  int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru);
>  void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev *cru);
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 71d9c671f739..46a0823e1300 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -155,6 +155,23 @@ static void rzg2l_cru_return_buffers(struct rzg2l_cru_dev *cru,
>  	}
>  }
>
> +void rzg2l_cru_requeue_active_buffers(struct rzg2l_cru_dev *cru)
> +{
> +	unsigned int i;
> +
> +	scoped_guard(spinlock_irqsave, &cru->hw_lock) {

As the functions is fully covered by the lock this could be

        guard(spinlock_irqsave)(&cru->hw_lock);

Can this be called from irq context ? Do you need irqsave ?

> +		for (i = 0; i < cru->num_buf; i++) {

You can declare i inside the for loop

> +			if (!cru->queue_buf[i])
> +				continue;
> +			scoped_guard(spinlock_irqsave, &cru->qlock) {
> +				list_add_tail(to_buf_list(cru->queue_buf[i]),
> +					      &cru->buf_list);
> +			}
> +			cru->queue_buf[i] = NULL;
> +		}
> +	}
> +}
> +
>  static int rzg2l_cru_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  				 unsigned int *nplanes, unsigned int sizes[],
>  				 struct device *alloc_devs[])
> @@ -528,7 +545,7 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  	return 0;
>  }
>
> -static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
> +int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  {
>  	struct media_pipeline *pipe;
>  	struct v4l2_subdev *sd;
> @@ -707,6 +724,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
>  		goto out;
>  	}
>
> +	cru->running = true;

Is it necessary to protect access to cru->running for concurrent
suspend/start|stop_streaming sequences ?

Thanks
   j

>  	dev_dbg(cru->dev, "Starting to capture\n");
>  	return 0;
>
> @@ -731,7 +749,10 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
>  {
>  	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq);
>
> -	rzg2l_cru_set_stream(cru, 0);
> +	if (cru->running) {
> +		rzg2l_cru_set_stream(cru, 0);
> +		cru->running = false;
> +	}
>
>  	/* Free scratch buffer */
>  	dma_free_coherent(cru->dev, cru->format.sizeimage,
> --
> 2.54.0
>