[PATCH 0/4] RZ/G2L CSI/CRU Improvements

[PATCH 0/4] RZ/G2L CSI/CRU Improvements

[Biju Das]

This patch series aims to sync the CSI/CRU driver code with the latest
hardware manual(R01UH0914EJ0140 Rev.1.40).

Biju Das (4):
  media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()
  media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset
  media: platform: rzg2l-cru: rzg2l-video: Fix image processing
    initialization
  media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 63 ++++++++++----
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 18 ++--
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 83 ++++++++-----------
 4 files changed, 88 insertions(+), 79 deletions(-)

-- 
2.25.1

[PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Biju Das]

SET_RUNTIME_PM_OPS() are deprecated and require __maybe_unused
protection against unused function warnings. The usage of pm_ptr() and
RUNTIME_PM_OPS() allows the compiler to see the functions, thus
suppressing the warning. Thus drop the __maybe_unused markings.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index d20f4eff93a4..c4609da9bf1a 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 }
 
-static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
+static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
 {
 	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
 
@@ -843,7 +843,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
+static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
 {
 	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
 
@@ -851,7 +851,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
-	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)
+	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)
 };
 
 static const struct of_device_id rzg2l_csi2_of_table[] = {
@@ -865,7 +865,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
 	.driver	= {
 		.name = "rzg2l-csi2",
 		.of_match_table = rzg2l_csi2_of_table,
-		.pm = &rzg2l_csi2_pm_ops,
+		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
 	},
 };
 
-- 
2.25.1

[PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
after DPHY reset, we need to wait for 1 msec or more before start
receiving data from the sensor. So add a delay after pre_streamon().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 9f351a05893e..5468dc179de7 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2022 Renesas Electronics Corp.
  */
 
+#include <linux/delay.h>
 #include "rzg2l-cru.h"
 
 struct rzg2l_cru_ip_format {
@@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
 		if (ret)
 			return ret;
 
+		usleep_range(1000, 2000);
+
 		ret = rzg2l_cru_start_image_processing(cru);
 		if (ret) {
 			v4l2_subdev_call(cru->ip.remote, video, post_streamoff);
-- 
2.25.1

[PATCH 3/4] media: platform: rzg2l-cru: rzg2l-video: Fix image processing initialization

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
initialize the AXI master first and then initialize the image processing.
Fix the start procedure as per the hardware manual.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index d0ffa90bc656..a7d6fe831d54 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -430,13 +430,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 
 	spin_lock_irqsave(&cru->qlock, flags);
 
-	/* Initialize image convert */
-	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
-	if (ret) {
-		spin_unlock_irqrestore(&cru->qlock, flags);
-		return ret;
-	}
-
 	/* Select a video input */
 	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
 
@@ -450,6 +443,13 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	/* Initialize the AXI master */
 	rzg2l_cru_initialize_axi(cru);
 
+	/* Initialize image convert */
+	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
+	if (ret) {
+		spin_unlock_irqrestore(&cru->qlock, flags);
+		return ret;
+	}
+
 	/* Enable interrupt */
 	rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
 
-- 
2.25.1

[PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
we need to supply all CRU clks and  we need to disable the vclk before
enabling the LINK reception and enable the vclk after enabling the link
Reception. So restructure clk handling as per the HW manual.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 ++++++++-----------
 4 files changed, 74 insertions(+), 68 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 811603f18af0..a5a99b004322 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
 	struct v4l2_pix_format format;
 };
 
-void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru);
-int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
-
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
 void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index c4609da9bf1a..f4c5cbb30bc9 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -108,6 +109,7 @@ struct rzg2l_csi2 {
 	struct reset_control *presetn;
 	struct reset_control *cmn_rstb;
 	struct clk *sysclk;
+	struct clk *vclk;
 	unsigned long vclk_rate;
 
 	struct v4l2_subdev subdev;
@@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct v4l2_subdev *sd, bool on)
 	return rzg2l_csi2_dphy_disable(csi2);
 }
 
-static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
+static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 {
 	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
 	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
+	int ret, count;
 
 	/* Select data lanes */
 	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes));
@@ -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
 	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
 
+	clk_disable_unprepare(csi2->vclk);
+	for (count = 0; count < 5; count++) {
+		if (!(__clk_is_enabled(csi2->vclk)))
+			break;
+		usleep_range(10, 20);
+	}
+
+	if (count == 5) {
+		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
+		return -ETIMEDOUT;
+	}
+
 	/* Enable LINK reception */
 	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
+
+	ret = clk_prepare_enable(csi2->vclk);
+	if (ret)
+		return ret;
+
+	for (count = 0; count < 5; count++) {
+		if (__clk_is_enabled(csi2->vclk))
+			break;
+		usleep_range(10, 20);
+	}
+
+	if (count == 5) {
+		dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
 }
 
-static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
+static int rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
 {
 	unsigned int timeout = VSRSTS_RETRIES;
 
@@ -409,18 +441,21 @@ static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
 
 	if (!timeout)
 		dev_err(csi2->dev, "Clearing CSI2nRTST.VSRSTS timed out\n");
+
+	return 0;
 }
 
 static int rzg2l_csi2_mipi_link_setting(struct v4l2_subdev *sd, bool on)
 {
 	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
+	int ret;
 
 	if (on)
-		rzg2l_csi2_mipi_link_enable(csi2);
+		ret = rzg2l_csi2_mipi_link_enable(csi2);
 	else
-		rzg2l_csi2_mipi_link_disable(csi2);
+		ret = rzg2l_csi2_mipi_link_disable(csi2);
 
-	return 0;
+	return ret;
 }
 
 static int rzg2l_csi2_s_stream(struct v4l2_subdev *sd, int enable)
@@ -731,7 +766,6 @@ static const struct media_entity_operations rzg2l_csi2_entity_ops = {
 static int rzg2l_csi2_probe(struct platform_device *pdev)
 {
 	struct rzg2l_csi2 *csi2;
-	struct clk *vclk;
 	int ret;
 
 	csi2 = devm_kzalloc(&pdev->dev, sizeof(*csi2), GFP_KERNEL);
@@ -757,12 +791,11 @@ static int rzg2l_csi2_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->sysclk),
 				     "Failed to get system clk\n");
 
-	vclk = clk_get(&pdev->dev, "video");
-	if (IS_ERR(vclk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(vclk),
+	csi2->vclk = devm_clk_get(&pdev->dev, "video");
+	if (IS_ERR(csi2->vclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->vclk),
 				     "Failed to get video clock\n");
-	csi2->vclk_rate = clk_get_rate(vclk);
-	clk_put(vclk);
+	csi2->vclk_rate = clk_get_rate(csi2->vclk);
 
 	csi2->dev = &pdev->dev;
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 5468dc179de7..36c6122c3fa6 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -80,20 +80,9 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
 			return ret;
 		}
 
-		rzg2l_cru_vclk_unprepare(cru);
-
 		ret = v4l2_subdev_call(cru->ip.remote, video, s_stream, enable);
-		if (ret == -ENOIOCTLCMD)
-			ret = 0;
-		if (!ret) {
-			ret = rzg2l_cru_vclk_prepare(cru);
-			if (!ret)
-				return 0;
-		} else {
-			/* enable back vclk so that s_stream in error path disables it */
-			if (rzg2l_cru_vclk_prepare(cru))
-				dev_err(cru->dev, "Failed to enable vclk\n");
-		}
+		if (!ret || (ret == -ENOIOCTLCMD))
+			return 0;
 
 		s_stream_ret = ret;
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index a7d6fe831d54..b16b8af6e8f8 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -461,16 +461,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	return 0;
 }
 
-void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru)
-{
-	clk_disable_unprepare(cru->vclk);
-}
-
-int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru)
-{
-	return clk_prepare_enable(cru->vclk);
-}
-
 static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 {
 	struct media_pipeline *pipe;
@@ -499,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;
@@ -542,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;
 }
 
@@ -646,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. */
@@ -696,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;
 }
 
@@ -714,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 = {
-- 
2.25.1

Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Sakari Ailus]

Hi Biju,

Thanks for the patch.

On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
> latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
> we need to supply all CRU clks and  we need to disable the vclk before
> enabling the LINK reception and enable the vclk after enabling the link
> Reception. So restructure clk handling as per the HW manual.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
>  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 ++++++++-----------
>  4 files changed, 74 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 811603f18af0..a5a99b004322 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
>  	struct v4l2_pix_format format;
>  };
>  
> -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru);
> -int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> -
>  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index c4609da9bf1a..f4c5cbb30bc9 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
>  	struct reset_control *presetn;
>  	struct reset_control *cmn_rstb;
>  	struct clk *sysclk;
> +	struct clk *vclk;
>  	unsigned long vclk_rate;
>  
>  	struct v4l2_subdev subdev;
> @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct v4l2_subdev *sd, bool on)
>  	return rzg2l_csi2_dphy_disable(csi2);
>  }
>  
> -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
>  {
>  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
>  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> +	int ret, count;
>  
>  	/* Select data lanes */
>  	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes));
> @@ -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
>  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
>  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
>  
> +	clk_disable_unprepare(csi2->vclk);
> +	for (count = 0; count < 5; count++) {
> +		if (!(__clk_is_enabled(csi2->vclk)))
> +			break;
> +		usleep_range(10, 20);
> +	}
> +
> +	if (count == 5) {
> +		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
> +		return -ETIMEDOUT;
> +	}

It'd be nice to have a CCF function to do this. Either way, you can use
read_poll_timeout().

> +
>  	/* Enable LINK reception */
>  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> +
> +	ret = clk_prepare_enable(csi2->vclk);
> +	if (ret)
> +		return ret;
> +
> +	for (count = 0; count < 5; count++) {
> +		if (__clk_is_enabled(csi2->vclk))
> +			break;
> +		usleep_range(10, 20);
> +	}
> +
> +	if (count == 5) {
> +		dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
>  }
>  

-- 
Regards,

Sakari Ailus

RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

Hi Sakari Ailus,

Thanks for the feedback.

> -----Original Message-----
> Subject: Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
> 
> Hi Biju,
> 
> Thanks for the patch.
> 
> On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > that we need to supply all CRU clks and  we need to disable the vclk
> > before enabling the LINK reception and enable the vclk after enabling
> > the link Reception. So restructure clk handling as per the HW manual.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69
> > ++++++++-----------
> >  4 files changed, 74 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index 811603f18af0..a5a99b004322 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> >  	struct v4l2_pix_format format;
> >  };
> >
> > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int
> > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > -
> >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
> > void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > index c4609da9bf1a..f4c5cbb30bc9 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
> >  	struct reset_control *presetn;
> >  	struct reset_control *cmn_rstb;
> >  	struct clk *sysclk;
> > +	struct clk *vclk;
> >  	unsigned long vclk_rate;
> >
> >  	struct v4l2_subdev subdev;
> > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct
> v4l2_subdev *sd, bool on)
> >  	return rzg2l_csi2_dphy_disable(csi2);  }
> >
> > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> >  {
> >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> > +	int ret, count;
> >
> >  	/* Select data lanes */
> >  	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes)); @@
> > -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct
> rzg2l_csi2 *csi2)
> >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> >
> > +	clk_disable_unprepare(csi2->vclk);
> > +	for (count = 0; count < 5; count++) {
> > +		if (!(__clk_is_enabled(csi2->vclk)))
> > +			break;
> > +		usleep_range(10, 20);
> > +	}
> > +
> > +	if (count == 5) {
> > +		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
> > +		return -ETIMEDOUT;
> > +	}
> 
> It'd be nice to have a CCF function to do this. Either way, you can use
> read_poll_timeout().

OK, Will update rzg2l_mod_clock_endisable() [1] to handle this.
Currently it does status check for clk on but missing the same status check for clock off.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/clk/renesas/rzg2l-cpg.c#L1201

Cheers,
Biju

Re: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Tue, Jan 23, 2024 at 11:58:18AM +0000, Biju Das wrote:
> SET_RUNTIME_PM_OPS() are deprecated

Not that I particularly care about SET_RUNTIME_PM_OPS, but I'm not aware
of it being deprecated. Has that been announced somewhere ?

> and require __maybe_unused
> protection against unused function warnings. The usage of pm_ptr() and
> RUNTIME_PM_OPS() allows the compiler to see the functions, thus
> suppressing the warning. Thus drop the __maybe_unused markings.

Have you tried to compile this with CONFIG_PM disabled, and confirmed
the compiler doesn't generate any warning ?

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index d20f4eff93a4..c4609da9bf1a 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }
>  
> -static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> +static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
>  {
>  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
>  
> @@ -843,7 +843,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
> +static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
>  {
>  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
>  
> @@ -851,7 +851,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
> -	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)
> +	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)

While at it, I would wrap the line to

	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
		       rzg2l_csi2_pm_runtime_resume, NULL)

Up to you.

>  };
>  
>  static const struct of_device_id rzg2l_csi2_of_table[] = {
> @@ -865,7 +865,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
>  	.driver	= {
>  		.name = "rzg2l-csi2",
>  		.of_match_table = rzg2l_csi2_of_table,
> -		.pm = &rzg2l_csi2_pm_ops,
> +		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
>  	},
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Tue, Jan 23, 2024 at 11:58:19AM +0000, Biju Das wrote:
> As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
> latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
> after DPHY reset, we need to wait for 1 msec or more before start
> receiving data from the sensor. So add a delay after pre_streamon().
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 9f351a05893e..5468dc179de7 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2022 Renesas Electronics Corp.
>   */
>  
> +#include <linux/delay.h>
>  #include "rzg2l-cru.h"
>  
>  struct rzg2l_cru_ip_format {
> @@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (ret)
>  			return ret;
>  
> +		usleep_range(1000, 2000);
> +

What would you think of using

		fsleep(1000);

instead ?

With or without that,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  		ret = rzg2l_cru_start_image_processing(cru);
>  		if (ret) {
>  			v4l2_subdev_call(cru->ip.remote, video, post_streamoff);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: platform: rzg2l-cru: rzg2l-video: Fix image processing initialization

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Tue, Jan 23, 2024 at 11:58:20AM +0000, Biju Das wrote:
> As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
> latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that

s/manual/manual /

> initialize the AXI master first and then initialize the image processing.
> Fix the start procedure as per the hardware manual.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index d0ffa90bc656..a7d6fe831d54 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -430,13 +430,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  
>  	spin_lock_irqsave(&cru->qlock, flags);
>  
> -	/* Initialize image convert */
> -	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
> -	if (ret) {
> -		spin_unlock_irqrestore(&cru->qlock, flags);
> -		return ret;
> -	}
> -
>  	/* Select a video input */
>  	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
>  
> @@ -450,6 +443,13 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  	/* Initialize the AXI master */
>  	rzg2l_cru_initialize_axi(cru);
>  
> +	/* Initialize image convert */
> +	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
> +	if (ret) {
> +		spin_unlock_irqrestore(&cru->qlock, flags);
> +		return ret;
> +	}
> +
>  	/* Enable interrupt */
>  	rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote:
> Hi Biju,
> 
> Thanks for the patch.
> 
> On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
> > latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned that
> > we need to supply all CRU clks and  we need to disable the vclk before
> > enabling the LINK reception and enable the vclk after enabling the link
> > Reception. So restructure clk handling as per the HW manual.
> > 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 ++++++++-----------
> >  4 files changed, 74 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index 811603f18af0..a5a99b004322 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> >  	struct v4l2_pix_format format;
> >  };
> >  
> > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru);
> > -int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > -
> >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
> >  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
> >  
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > index c4609da9bf1a..f4c5cbb30bc9 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
> >  	struct reset_control *presetn;
> >  	struct reset_control *cmn_rstb;
> >  	struct clk *sysclk;
> > +	struct clk *vclk;
> >  	unsigned long vclk_rate;
> >  
> >  	struct v4l2_subdev subdev;
> > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct v4l2_subdev *sd, bool on)
> >  	return rzg2l_csi2_dphy_disable(csi2);
> >  }
> >  
> > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> >  {
> >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> > +	int ret, count;
> >  
> >  	/* Select data lanes */
> >  	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes));
> > @@ -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> >  
> > +	clk_disable_unprepare(csi2->vclk);
> > +	for (count = 0; count < 5; count++) {
> > +		if (!(__clk_is_enabled(csi2->vclk)))
> > +			break;
> > +		usleep_range(10, 20);
> > +	}
> > +
> > +	if (count == 5) {
> > +		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
> > +		return -ETIMEDOUT;
> > +	}
> 
> It'd be nice to have a CCF function to do this. Either way, you can use
> read_poll_timeout().

I would have sworn that clk_disable_unprepare() is synchronous, and
would have sworn even stronger for clk_prepare_enable(). What's going on
here, is there really a delay ? It sounds like a bug in the clock
driver.

> > +
> >  	/* Enable LINK reception */
> >  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> > +
> > +	ret = clk_prepare_enable(csi2->vclk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (count = 0; count < 5; count++) {
> > +		if (__clk_is_enabled(csi2->vclk))
> > +			break;
> > +		usleep_range(10, 20);
> > +	}
> > +
> > +	if (count == 5) {
> > +		dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> >  }
> >  

-- 
Regards,

Laurent Pinchart

RE: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 3:29 PM
> Subject: Re: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to
> RUNTIME_PM_OPS()
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Tue, Jan 23, 2024 at 11:58:18AM +0000, Biju Das wrote:
> > SET_RUNTIME_PM_OPS() are deprecated
> 
> Not that I particularly care about SET_RUNTIME_PM_OPS, but I'm not aware
> of it being deprecated. Has that been announced somewhere ?

I was under the impression from [1], this is new style and old style going to be deprecated.

If it is not the case, I would like to drop this patch.

[1] https://www.spinics.net/lists/stable/msg691416.html

> 
> > and require __maybe_unused
> > protection against unused function warnings. The usage of pm_ptr() and
> > RUNTIME_PM_OPS() allows the compiler to see the functions, thus
> > suppressing the warning. Thus drop the __maybe_unused markings.
> 
> Have you tried to compile this with CONFIG_PM disabled, and confirmed the
> compiler doesn't generate any warning ?

I am not able to compile with CONFIG_PM disabled as it is throwing errors in power management code.

kernel/power/suspend.c: In function ‘suspend_prepare’:
kernel/power/suspend.c:360:10: error: implicit declaration of function ‘pm_notifier_call_chain_robust’; did you mean ‘raw_notifier_call_chain_robust’? [-Werror=implicit-function-declaration]
  360 |  error = pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |          raw_notifier_call_chain_robust
kernel/power/hibernate.c:325:2: error: implicit declaration of function ‘save_processor_state’ [-Werror=implicit-function-declaration]
  325 |  save_processor_state();
      |  ^~~~~~~~~~~~~~~~~~~~
kernel/power/suspend.c:372:2: error: implicit declaration of function ‘pm_notifier_call_chain’; did you mean ‘raw_notifier_call_chain’? [-Werror=implicit-function-declaration]
  372 |  pm_notifier_call_chain(PM_POST_SUSPEND);
      |  ^~~~~~~~~~~~~~~~~~~~~~
      |  raw_notifier_call_chain
  CC      io_uring/timeout.o
kernel/power/suspend.c: In function ‘suspend_enter’:
kernel/power/suspend.c:405:10: error: implicit declaration of function ‘dpm_suspend_late’; did you mean ‘dpm_suspend_start’? [-Werror=implicit-function-declaration]
  405 |  error = dpm_suspend_late(PMSG_SUSPEND);
      |          ^~~~~~~~~~~~~~~~
      |          dpm_suspend_start 

> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > index d20f4eff93a4..c4609da9bf1a 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device
> *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  }
> >
> > -static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device
> > *dev)
> > +static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> >  {
> >  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
> >
> > @@ -843,7 +843,7 @@ static int __maybe_unused
> rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> >  	return 0;
> >  }
> >
> > -static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device
> > *dev)
> > +static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
> >  {
> >  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
> >
> > @@ -851,7 +851,7 @@ static int __maybe_unused
> > rzg2l_csi2_pm_runtime_resume(struct device *dev)  }
> >
> >  static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
> > -	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> rzg2l_csi2_pm_runtime_resume, NULL)
> > +	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> > +rzg2l_csi2_pm_runtime_resume, NULL)
> 
> While at it, I would wrap the line to
> 
> 	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> 		       rzg2l_csi2_pm_runtime_resume, NULL)
> 
> Up to you.

If it is not a requirement to move to use the modern pm_ops/pm_sleep_ops
then as said in the above I would like to drop this patch.

Cheers,
Biju

> 
> >  };
> >
> >  static const struct of_device_id rzg2l_csi2_of_table[] = { @@ -865,7
> > +865,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
> >  	.driver	= {
> >  		.name = "rzg2l-csi2",
> >  		.of_match_table = rzg2l_csi2_of_table,
> > -		.pm = &rzg2l_csi2_pm_ops,
> > +		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
> >  	},
> >  };
> >
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 3:30 PM
> Subject: Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay
> after D-PHY reset
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Tue, Jan 23, 2024 at 11:58:19AM +0000, Biju Das wrote:
> > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > that after DPHY reset, we need to wait for 1 msec or more before start
> > receiving data from the sensor. So add a delay after pre_streamon().
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > index 9f351a05893e..5468dc179de7 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2022 Renesas Electronics Corp.
> >   */
> >
> > +#include <linux/delay.h>
> >  #include "rzg2l-cru.h"
> >
> >  struct rzg2l_cru_ip_format {
> > @@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev
> *sd, int enable)
> >  		if (ret)
> >  			return ret;
> >
> > +		usleep_range(1000, 2000);
> > +
> 
> What would you think of using
> 
> 		fsleep(1000);
> 
> instead ?

Essentially it is same by looking at the code[1].
OK will use fsleep()


[1]
static inline void fsleep(unsigned long usecs)
{
	if (usecs <= 10)
		udelay(usecs);
	else if (usecs <= 20000)
		usleep_range(usecs, 2 * usecs);
	else
		msleep(DIV_ROUND_UP(usecs, 1000));
}

Cheers,
Biju

> 
> With or without that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> >  		ret = rzg2l_cru_start_image_processing(cru);
> >  		if (ret) {
> >  			v4l2_subdev_call(cru->ip.remote, video, post_streamoff);
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH 3/4] media: platform: rzg2l-cru: rzg2l-video: Fix image processing initialization

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 3:33 PM
> Subject: Re: [PATCH 3/4] media: platform: rzg2l-cru: rzg2l-video: Fix
> image processing initialization
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Tue, Jan 23, 2024 at 11:58:20AM +0000, Biju Das wrote:
> > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > that
> 
> s/manual/manual /

OK, will fix this in next version.

Cheers,
Biju

> 
> > initialize the AXI master first and then initialize the image
> processing.
> > Fix the start procedure as per the hardware manual.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> > ---
> >  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 14
> > +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index d0ffa90bc656..a7d6fe831d54 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -430,13 +430,6 @@ int rzg2l_cru_start_image_processing(struct
> > rzg2l_cru_dev *cru)
> >
> >  	spin_lock_irqsave(&cru->qlock, flags);
> >
> > -	/* Initialize image convert */
> > -	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
> > -	if (ret) {
> > -		spin_unlock_irqrestore(&cru->qlock, flags);
> > -		return ret;
> > -	}
> > -
> >  	/* Select a video input */
> >  	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
> >
> > @@ -450,6 +443,13 @@ int rzg2l_cru_start_image_processing(struct
> rzg2l_cru_dev *cru)
> >  	/* Initialize the AXI master */
> >  	rzg2l_cru_initialize_axi(cru);
> >
> > +	/* Initialize image convert */
> > +	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
> > +	if (ret) {
> > +		spin_unlock_irqrestore(&cru->qlock, flags);
> > +		return ret;
> > +	}
> > +
> >  	/* Enable interrupt */
> >  	rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
> >
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

Hi Laurent Pinchart,

thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 3:36 PM
> Subject: Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
> 
> On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote:
> > Hi Biju,
> >
> > Thanks for the patch.
> >
> > On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > > that we need to supply all CRU clks and  we need to disable the vclk
> > > before enabling the LINK reception and enable the vclk after
> > > enabling the link Reception. So restructure clk handling as per the HW
> manual.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> > >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69
> > > ++++++++-----------
> > >  4 files changed, 74 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > index 811603f18af0..a5a99b004322 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> > >  	struct v4l2_pix_format format;
> > >  };
> > >
> > > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int
> > > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > > -
> > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
> > > void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > index c4609da9bf1a..f4c5cbb30bc9 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > @@ -6,6 +6,7 @@
> > >   */
> > >
> > >  #include <linux/clk.h>
> > > +#include <linux/clk-provider.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/io.h>
> > > @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
> > >  	struct reset_control *presetn;
> > >  	struct reset_control *cmn_rstb;
> > >  	struct clk *sysclk;
> > > +	struct clk *vclk;
> > >  	unsigned long vclk_rate;
> > >
> > >  	struct v4l2_subdev subdev;
> > > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct
> v4l2_subdev *sd, bool on)
> > >  	return rzg2l_csi2_dphy_disable(csi2);  }
> > >
> > > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > >  {
> > >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> > >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> > > +	int ret, count;
> > >
> > >  	/* Select data lanes */
> > >  	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct
> rzg2l_csi2 *csi2)
> > >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > >
> > > +	clk_disable_unprepare(csi2->vclk);
> > > +	for (count = 0; count < 5; count++) {
> > > +		if (!(__clk_is_enabled(csi2->vclk)))
> > > +			break;
> > > +		usleep_range(10, 20);
> > > +	}
> > > +
> > > +	if (count == 5) {
> > > +		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
> > > +		return -ETIMEDOUT;
> > > +	}
> >
> > It'd be nice to have a CCF function to do this. Either way, you can
> > use read_poll_timeout().
> 
> I would have sworn that clk_disable_unprepare() is synchronous, and would
> have sworn even stronger for clk_prepare_enable(). What's going on here,
> is there really a delay ? It sounds like a bug in the clock driver.

At the moment we are not checking clock status when we turn off a clock
However, for clock ON we are checking the status while turning it ON. 
We need to fix the driver for clk_disable_unprepare().

Cheers,
Biju

> 
> > > +
> > >  	/* Enable LINK reception */
> > >  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> > > +
> > > +	ret = clk_prepare_enable(csi2->vclk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (count = 0; count < 5; count++) {
> > > +		if (__clk_is_enabled(csi2->vclk))
> > > +			break;
> > > +		usleep_range(10, 20);
> > > +	}
> > > +
> > > +	if (count == 5) {
> > > +		dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n");
> > > +		return -ETIMEDOUT;
> > > +	}
> > > +
> > > +	return 0;
> > >  }
> > >
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 06:38:00PM +0000, Biju Das wrote:
> Hi Laurent Pinchart,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: Tuesday, January 23, 2024 3:30 PM
> > Subject: Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay
> > after D-PHY reset
> > 
> > Hi Biju,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Jan 23, 2024 at 11:58:19AM +0000, Biju Das wrote:
> > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > > that after DPHY reset, we need to wait for 1 msec or more before start
> > > receiving data from the sensor. So add a delay after pre_streamon().
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > index 9f351a05893e..5468dc179de7 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > @@ -5,6 +5,7 @@
> > >   * Copyright (C) 2022 Renesas Electronics Corp.
> > >   */
> > >
> > > +#include <linux/delay.h>
> > >  #include "rzg2l-cru.h"
> > >
> > >  struct rzg2l_cru_ip_format {
> > > @@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev
> > *sd, int enable)
> > >  		if (ret)
> > >  			return ret;
> > >
> > > +		usleep_range(1000, 2000);
> > > +
> > 
> > What would you think of using
> > 
> > 		fsleep(1000);
> > 
> > instead ?
> 
> Essentially it is same by looking at the code[1].
> OK will use fsleep()

Yes, it will result in the same delay. fsleep() is recommended as the
default sleep function unless there's a specific need to do something
different.

> [1]
> static inline void fsleep(unsigned long usecs)
> {
> 	if (usecs <= 10)
> 		udelay(usecs);
> 	else if (usecs <= 20000)
> 		usleep_range(usecs, 2 * usecs);
> 	else
> 		msleep(DIV_ROUND_UP(usecs, 1000));
> }
> 
> > With or without that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > >  		ret = rzg2l_cru_start_image_processing(cru);
> > >  		if (ret) {
> > >  			v4l2_subdev_call(cru->ip.remote, video, post_streamoff);

-- 
Regards,

Laurent Pinchart

RE: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Biju Das]

Hi Laurent Pinchart,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 6:46 PM
> Subject: Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay
> after D-PHY reset
> 
> On Tue, Jan 23, 2024 at 06:38:00PM +0000, Biju Das wrote:
> > Hi Laurent Pinchart,
> >
> > Thanks for the feedback.
> >
> > > -----Original Message-----
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: Tuesday, January 23, 2024 3:30 PM
> > > Subject: Re: [PATCH 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add
> > > delay after D-PHY reset
> > >
> > > Hi Biju,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Jan 23, 2024 at 11:58:19AM +0000, Biju Das wrote:
> > > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input
> > > > on the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is
> > > > mentioned that after DPHY reset, we need to wait for 1 msec or
> > > > more before start receiving data from the sensor. So add a delay
> after pre_streamon().
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > > index 9f351a05893e..5468dc179de7 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > > > @@ -5,6 +5,7 @@
> > > >   * Copyright (C) 2022 Renesas Electronics Corp.
> > > >   */
> > > >
> > > > +#include <linux/delay.h>
> > > >  #include "rzg2l-cru.h"
> > > >
> > > >  struct rzg2l_cru_ip_format {
> > > > @@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct
> > > > v4l2_subdev
> > > *sd, int enable)
> > > >  		if (ret)
> > > >  			return ret;
> > > >
> > > > +		usleep_range(1000, 2000);
> > > > +
> > >
> > > What would you think of using
> > >
> > > 		fsleep(1000);
> > >
> > > instead ?
> >
> > Essentially it is same by looking at the code[1].
> > OK will use fsleep()
> 
> Yes, it will result in the same delay. fsleep() is recommended as the
> default sleep function unless there's a specific need to do something
> different.

OK. Thanks for the explanation.

Cheers,
Biju

> 
> > [1]
> > static inline void fsleep(unsigned long usecs) {
> > 	if (usecs <= 10)
> > 		udelay(usecs);
> > 	else if (usecs <= 20000)
> > 		usleep_range(usecs, 2 * usecs);
> > 	else
> > 		msleep(DIV_ROUND_UP(usecs, 1000));
> > }
> >
> > > With or without that,
> > >
> > > Reviewed-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > >  		ret = rzg2l_cru_start_image_processing(cru);
> > > >  		if (ret) {
> > > >  			v4l2_subdev_call(cru->ip.remote, video,
> post_streamoff);
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 06:42:19PM +0000, Biju Das wrote:
> > On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote:
> > > On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> > > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > > > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned
> > > > that we need to supply all CRU clks and  we need to disable the vclk
> > > > before enabling the LINK reception and enable the vclk after
> > > > enabling the link Reception. So restructure clk handling as per the HW
> > manual.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69
> > > > ++++++++-----------
> > > >  4 files changed, 74 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > index 811603f18af0..a5a99b004322 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> > > >  	struct v4l2_pix_format format;
> > > >  };
> > > >
> > > > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int
> > > > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > > > -
> > > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
> > > > void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > index c4609da9bf1a..f4c5cbb30bc9 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > @@ -6,6 +6,7 @@
> > > >   */
> > > >
> > > >  #include <linux/clk.h>
> > > > +#include <linux/clk-provider.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/io.h>
> > > > @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
> > > >  	struct reset_control *presetn;
> > > >  	struct reset_control *cmn_rstb;
> > > >  	struct clk *sysclk;
> > > > +	struct clk *vclk;
> > > >  	unsigned long vclk_rate;
> > > >
> > > >  	struct v4l2_subdev subdev;
> > > > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct
> > v4l2_subdev *sd, bool on)
> > > >  	return rzg2l_csi2_dphy_disable(csi2);  }
> > > >
> > > > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > > > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > > >  {
> > > >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> > > >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> > > > +	int ret, count;
> > > >
> > > >  	/* Select data lanes */
> > > >  	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > -386,11 +389,40 @@ static void rzg2l_csi2_mipi_link_enable(struct
> > rzg2l_csi2 *csi2)
> > > >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > >
> > > > +	clk_disable_unprepare(csi2->vclk);
> > > > +	for (count = 0; count < 5; count++) {
> > > > +		if (!(__clk_is_enabled(csi2->vclk)))
> > > > +			break;
> > > > +		usleep_range(10, 20);
> > > > +	}
> > > > +
> > > > +	if (count == 5) {
> > > > +		dev_err(csi2->dev, "Timeout, not able to turn OFF vclk\n");
> > > > +		return -ETIMEDOUT;
> > > > +	}
> > >
> > > It'd be nice to have a CCF function to do this. Either way, you can
> > > use read_poll_timeout().
> > 
> > I would have sworn that clk_disable_unprepare() is synchronous, and would
> > have sworn even stronger for clk_prepare_enable(). What's going on here,
> > is there really a delay ? It sounds like a bug in the clock driver.
> 
> At the moment we are not checking clock status when we turn off a clock
> However, for clock ON we are checking the status while turning it ON. 
> We need to fix the driver for clk_disable_unprepare().

Does that mean that the check below clk_prepare_enable() could be
removed already ?

Regarding clock disable, it isn't clear if the API guarantees
synchronous calls. I'd recommend asking the clock maintainers. If it
doesn't, then the clock driver isn't wrong (and may lead to faster
operation in most cases), but I think synchronizing the clock disable by
waiting for the clock to be actually disabled should be implemented as a
helper in CCF.

> > > > +
> > > >  	/* Enable LINK reception */
> > > >  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> > > > +
> > > > +	ret = clk_prepare_enable(csi2->vclk);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	for (count = 0; count < 5; count++) {
> > > > +		if (__clk_is_enabled(csi2->vclk))
> > > > +			break;
> > > > +		usleep_range(10, 20);
> > > > +	}
> > > > +
> > > > +	if (count == 5) {
> > > > +		dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n");
> > > > +		return -ETIMEDOUT;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > >  }
> > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 06:33:57PM +0000, Biju Das wrote:
> > On Tue, Jan 23, 2024 at 11:58:18AM +0000, Biju Das wrote:
> > > SET_RUNTIME_PM_OPS() are deprecated
> > 
> > Not that I particularly care about SET_RUNTIME_PM_OPS, but I'm not aware
> > of it being deprecated. Has that been announced somewhere ?
> 
> I was under the impression from [1], this is new style and old style going to be deprecated.
> 
> If it is not the case, I would like to drop this patch.
> 
> [1] https://www.spinics.net/lists/stable/msg691416.html

I'm not against this change, I was just wondering about the status of
SET_RUNTIME_PM_OPS. If you think using RUNTIME_PM_OPS is better, I have
no problem with that. The commit message should probably state that the
latter is better, instead of saying that SET_RUNTIME_PM_OPS is
deprecated.

> > > and require __maybe_unused
> > > protection against unused function warnings. The usage of pm_ptr() and
> > > RUNTIME_PM_OPS() allows the compiler to see the functions, thus
> > > suppressing the warning. Thus drop the __maybe_unused markings.
> > 
> > Have you tried to compile this with CONFIG_PM disabled, and confirmed the
> > compiler doesn't generate any warning ?
> 
> I am not able to compile with CONFIG_PM disabled as it is throwing errors in power management code.
> 
> kernel/power/suspend.c: In function ‘suspend_prepare’:
> kernel/power/suspend.c:360:10: error: implicit declaration of function ‘pm_notifier_call_chain_robust’; did you mean ‘raw_notifier_call_chain_robust’? [-Werror=implicit-function-declaration]
>   360 |  error = pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND);
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |          raw_notifier_call_chain_robust
> kernel/power/hibernate.c:325:2: error: implicit declaration of function ‘save_processor_state’ [-Werror=implicit-function-declaration]
>   325 |  save_processor_state();
>       |  ^~~~~~~~~~~~~~~~~~~~
> kernel/power/suspend.c:372:2: error: implicit declaration of function ‘pm_notifier_call_chain’; did you mean ‘raw_notifier_call_chain’? [-Werror=implicit-function-declaration]
>   372 |  pm_notifier_call_chain(PM_POST_SUSPEND);
>       |  ^~~~~~~~~~~~~~~~~~~~~~
>       |  raw_notifier_call_chain
>   CC      io_uring/timeout.o
> kernel/power/suspend.c: In function ‘suspend_enter’:
> kernel/power/suspend.c:405:10: error: implicit declaration of function ‘dpm_suspend_late’; did you mean ‘dpm_suspend_start’? [-Werror=implicit-function-declaration]
>   405 |  error = dpm_suspend_late(PMSG_SUSPEND);
>       |          ^~~~~~~~~~~~~~~~
>       |          dpm_suspend_start 

I'm surprised. Sakari, isn't !CONFIG_PM supposed to be supported ?

> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > index d20f4eff93a4..c4609da9bf1a 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > @@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device
> > *pdev)
> > >  	pm_runtime_disable(&pdev->dev);
> > >  }
> > >
> > > -static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device
> > > *dev)
> > > +static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> > >  {
> > >  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
> > >
> > > @@ -843,7 +843,7 @@ static int __maybe_unused
> > rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> > >  	return 0;
> > >  }
> > >
> > > -static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device
> > > *dev)
> > > +static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
> > >  {
> > >  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
> > >
> > > @@ -851,7 +851,7 @@ static int __maybe_unused
> > > rzg2l_csi2_pm_runtime_resume(struct device *dev)  }
> > >
> > >  static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
> > > -	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> > rzg2l_csi2_pm_runtime_resume, NULL)
> > > +	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> > > +rzg2l_csi2_pm_runtime_resume, NULL)
> > 
> > While at it, I would wrap the line to
> > 
> > 	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> > 		       rzg2l_csi2_pm_runtime_resume, NULL)
> > 
> > Up to you.
> 
> If it is not a requirement to move to use the modern pm_ops/pm_sleep_ops
> then as said in the above I would like to drop this patch.
> 
> > >  };
> > >
> > >  static const struct of_device_id rzg2l_csi2_of_table[] = { @@ -865,7
> > > +865,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
> > >  	.driver	= {
> > >  		.name = "rzg2l-csi2",
> > >  		.of_match_table = rzg2l_csi2_of_table,
> > > -		.pm = &rzg2l_csi2_pm_ops,
> > > +		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
> > >  	},
> > >  };
> > >

-- 
Regards,

Laurent Pinchart

RE: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 10:17 PM
> Subject: Re: [PATCH 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to
> RUNTIME_PM_OPS()
> 
> On Tue, Jan 23, 2024 at 06:33:57PM +0000, Biju Das wrote:
> > > On Tue, Jan 23, 2024 at 11:58:18AM +0000, Biju Das wrote:
> > > > SET_RUNTIME_PM_OPS() are deprecated
> > >
> > > Not that I particularly care about SET_RUNTIME_PM_OPS, but I'm not
> > > aware of it being deprecated. Has that been announced somewhere ?
> >
> > I was under the impression from [1], this is new style and old style
> going to be deprecated.
> >
> > If it is not the case, I would like to drop this patch.
> >
> > [1]
> 
> I'm not against this change, I was just wondering about the status of
> SET_RUNTIME_PM_OPS. If you think using RUNTIME_PM_OPS is better, I have no
> problem with that. The commit message should probably state that the
> latter is better, instead of saying that SET_RUNTIME_PM_OPS is deprecated.


Agreed. Will update the commit message along with the changes you suggested in
Next version.

Cheers,
Biju

RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, January 23, 2024 10:10 PM
> Subject: Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
> 
> On Tue, Jan 23, 2024 at 06:42:19PM +0000, Biju Das wrote:
> > > On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote:
> > > > On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote:
> > > > > As per section 35.3.1 Starting Reception for the MIPI CSI-2
> > > > > Input on the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it
> > > > > is mentioned that we need to supply all CRU clks and  we need to
> > > > > disable the vclk before enabling the LINK reception and enable
> > > > > the vclk after enabling the link Reception. So restructure clk
> > > > > handling as per the HW
> > > manual.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> > > > >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 55 ++++++++++++--
> -
> > > > >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> > > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69
> > > > > ++++++++-----------
> > > > >  4 files changed, 74 insertions(+), 68 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > > index 811603f18af0..a5a99b004322 100644
> > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> > > > >  	struct v4l2_pix_format format;  };
> > > > >
> > > > > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int
> > > > > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > > > > -
> > > > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev
> > > > > *cru); void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev
> > > > > *cru);
> > > > >
> > > > > diff --git
> > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > > index c4609da9bf1a..f4c5cbb30bc9 100644
> > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > > @@ -6,6 +6,7 @@
> > > > >   */
> > > > >
> > > > >  #include <linux/clk.h>
> > > > > +#include <linux/clk-provider.h>
> > > > >  #include <linux/delay.h>
> > > > >  #include <linux/interrupt.h>
> > > > >  #include <linux/io.h>
> > > > > @@ -108,6 +109,7 @@ struct rzg2l_csi2 {
> > > > >  	struct reset_control *presetn;
> > > > >  	struct reset_control *cmn_rstb;
> > > > >  	struct clk *sysclk;
> > > > > +	struct clk *vclk;
> > > > >  	unsigned long vclk_rate;
> > > > >
> > > > >  	struct v4l2_subdev subdev;
> > > > > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct
> > > v4l2_subdev *sd, bool on)
> > > > >  	return rzg2l_csi2_dphy_disable(csi2);  }
> > > > >
> > > > > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2
> > > > > *csi2)
> > > > > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > > > >  {
> > > > >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> > > > >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> > > > > +	int ret, count;
> > > > >
> > > > >  	/* Select data lanes */
> > > > >  	rzg2l_csi2_write(csi2, CSI2nMCT0,
> > > > > CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > > -386,11 +389,40 @@ static void
> > > > > rzg2l_csi2_mipi_link_enable(struct
> > > rzg2l_csi2 *csi2)
> > > > >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > > >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > > >
> > > > > +	clk_disable_unprepare(csi2->vclk);
> > > > > +	for (count = 0; count < 5; count++) {
> > > > > +		if (!(__clk_is_enabled(csi2->vclk)))
> > > > > +			break;
> > > > > +		usleep_range(10, 20);
> > > > > +	}
> > > > > +
> > > > > +	if (count == 5) {
> > > > > +		dev_err(csi2->dev, "Timeout, not able to turn OFF
> vclk\n");
> > > > > +		return -ETIMEDOUT;
> > > > > +	}
> > > >
> > > > It'd be nice to have a CCF function to do this. Either way, you
> > > > can use read_poll_timeout().
> > >
> > > I would have sworn that clk_disable_unprepare() is synchronous, and
> > > would have sworn even stronger for clk_prepare_enable(). What's
> > > going on here, is there really a delay ? It sounds like a bug in the
> clock driver.
> >
> > At the moment we are not checking clock status when we turn off a
> > clock However, for clock ON we are checking the status while turning it
> ON.
> > We need to fix the driver for clk_disable_unprepare().
> 
> Does that mean that the check below clk_prepare_enable() could be removed
> already ?

Yes, that is correct I will remove in the next version as clk_prepare_enable() is
synchronous as it checks the status to make sure it is turned ON.

> 
> Regarding clock disable, it isn't clear if the API guarantees synchronous
> calls. I'd recommend asking the clock maintainers. If it doesn't, then the
> clock driver isn't wrong (and may lead to faster operation in most cases),
> but I think synchronizing the clock disable by waiting for the clock to be
> actually disabled should be implemented as a helper in CCF.

+clk.

Hi Stephen and all,

Can you please shed some light on this?

Cheers,
Biju

> 
> > > > > +
> > > > >  	/* Enable LINK reception */
> > > > >  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> > > > > +
> > > > > +	ret = clk_prepare_enable(csi2->vclk);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	for (count = 0; count < 5; count++) {
> > > > > +		if (__clk_is_enabled(csi2->vclk))
> > > > > +			break;
> > > > > +		usleep_range(10, 20);
> > > > > +	}
> > > > > +
> > > > > +	if (count == 5) {
> > > > > +		dev_err(csi2->dev, "Timeout, not able to turn ON
> vclk\n");
> > > > > +		return -ETIMEDOUT;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > >  }
> > > > >
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Stephen Boyd]

Quoting Biju Das (2024-01-24 06:01:30)
> > > > > > CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > > > -386,11 +389,40 @@ static void
> > > > > > rzg2l_csi2_mipi_link_enable(struct
> > > > rzg2l_csi2 *csi2)
> > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > > > >
> > > > > > +     clk_disable_unprepare(csi2->vclk);
> > > > > > +     for (count = 0; count < 5; count++) {
> > > > > > +             if (!(__clk_is_enabled(csi2->vclk)))

__clk_is_enabled() is a clk provider API. You shouldn't be using it in
consumer drivers.

> > > > > > +                     break;
> > > > > > +             usleep_range(10, 20);
> > > > > > +     }
> > > > > > +
> > > > > > +     if (count == 5) {
> > > > > > +             dev_err(csi2->dev, "Timeout, not able to turn OFF
> > vclk\n");
> > > > > > +             return -ETIMEDOUT;
> > > > > > +     }
> > > > >
> > > > > It'd be nice to have a CCF function to do this. Either way, you
> > > > > can use read_poll_timeout().
> > > >
> > > > I would have sworn that clk_disable_unprepare() is synchronous, and
> > > > would have sworn even stronger for clk_prepare_enable(). What's
> > > > going on here, is there really a delay ? It sounds like a bug in the
> > clock driver.
> > >
> > > At the moment we are not checking clock status when we turn off a
> > > clock However, for clock ON we are checking the status while turning it
> > ON.
> > > We need to fix the driver for clk_disable_unprepare().
> > 
> > Does that mean that the check below clk_prepare_enable() could be removed
> > already ?
> 
> Yes, that is correct I will remove in the next version as clk_prepare_enable() is
> synchronous as it checks the status to make sure it is turned ON.
> 
> > 
> > Regarding clock disable, it isn't clear if the API guarantees synchronous
> > calls. I'd recommend asking the clock maintainers. If it doesn't, then the
> > clock driver isn't wrong (and may lead to faster operation in most cases),
> > but I think synchronizing the clock disable by waiting for the clock to be
> > actually disabled should be implemented as a helper in CCF.
> 
> +clk.
> 
> Hi Stephen and all,
> 
> Can you please shed some light on this?
> 

clk_disable() is reference counted. The call to clk_disable() may do
nothing besides decrement a count and return. Per the documentation in
clk.h it means that the consumer is no longer using the clk. The clk
could be turned off later, or not at all.

It seems like the clk driver has a bug. Make the clk driver synchronize
the disable with enable please.

RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

Hi Laurent and Sakari,

> -----Original Message-----
> From: Biju Das
> Sent: Wednesday, January 24, 2024 9:50 PM
> Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
> 
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@kernel.org>
> > Sent: Wednesday, January 24, 2024 8:49 PM
> > Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> > Restructure clk handling
> >
> > Quoting Biju Das (2024-01-24 06:01:30)
> > > > > > > > CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > > > > > -386,11 +389,40 @@ static void
> > > > > > > > rzg2l_csi2_mipi_link_enable(struct
> > > > > > rzg2l_csi2 *csi2)
> > > > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > > > > > >       rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > > > > > >
> > > > > > > > +     clk_disable_unprepare(csi2->vclk);
> > > > > > > > +     for (count = 0; count < 5; count++) {
> > > > > > > > +             if (!(__clk_is_enabled(csi2->vclk)))
> >
> > __clk_is_enabled() is a clk provider API. You shouldn't be using it in
> > consumer drivers.
> 
> OK, Basically before enabling the link reception, vclk must be disabled.
> 
> Currently rzg2l clk driver is turning off the clock but it is not checking
> the status to make sure it is actually turned off.
> 
> I guess checking the status is not mandatory for clock OFF(disable) for
> performance reasons.
> 
> So shall I introduce clk_disable_unprepare_sync() and .disable_sync() for
> disabling and checking the status to make sure clock is turned off So that
> I can enable link reception reliably??
> 
> .disable(), just turn off the clock, but it doesn't guarantee that it is
> actually turned off.
> 
> whereas .disable_sync() guarantees that the clk is actually turned off.
> 
> >
> > > > > > > > +                     break;
> > > > > > > > +             usleep_range(10, 20);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     if (count == 5) {
> > > > > > > > +             dev_err(csi2->dev, "Timeout, not able to
> > > > > > > > + turn OFF
> > > > vclk\n");
> > > > > > > > +             return -ETIMEDOUT;
> > > > > > > > +     }
> > > > > > >
> > > > > > > It'd be nice to have a CCF function to do this. Either way,
> > > > > > > you can use read_poll_timeout().
> > > > > >
> > > > > > I would have sworn that clk_disable_unprepare() is
> > > > > > synchronous, and would have sworn even stronger for
> clk_prepare_enable().
> > > > > > What's going on here, is there really a delay ? It sounds like
> > > > > > a bug in the
> > > > clock driver.
> > > > >
> > > > > At the moment we are not checking clock status when we turn off
> > > > > a clock However, for clock ON we are checking the status while
> > > > > turning it
> > > > ON.
> > > > > We need to fix the driver for clk_disable_unprepare().
> > > >
> > > > Does that mean that the check below clk_prepare_enable() could be
> > > > removed already ?
> > >
> > > Yes, that is correct I will remove in the next version as
> > > clk_prepare_enable() is synchronous as it checks the status to make
> > > sure
> > it is turned ON.
> > >
> > > >
> > > > Regarding clock disable, it isn't clear if the API guarantees
> > > > synchronous calls. I'd recommend asking the clock maintainers. If
> > > > it doesn't, then the clock driver isn't wrong (and may lead to
> > > > faster operation in most cases), but I think synchronizing the
> > > > clock disable by waiting for the clock to be actually disabled
> > > > should be
> > implemented as a helper in CCF.
> > >
> > > +clk.
> > >
> > > Hi Stephen and all,
> > >
> > > Can you please shed some light on this?
> > >
> >
> > clk_disable() is reference counted. The call to clk_disable() may do
> > nothing besides decrement a count and return. Per the documentation in
> > clk.h it means that the consumer is no longer using the clk. The clk
> > could be turned off later, or not at all.
> >
> > It seems like the clk driver has a bug. Make the clk driver
> > synchronize the disable with enable please.
> 
> You mean like enable(), we should check the clock status for disable() to
> make sure it is turned off??
> 
> Or
> 
> Introduce a new API .disable_sync() for this special synchronization
> operation??


I have tested RZ/G2{L,LC,UL} and RZ/V2L SMARC EVK platforms with
just clk_disable_unprepare() and the clk_prepare_enable() the capture works fine
on these platforms.

So, I will send V2 with these changes. Please let me know if you think otherwise.

Also, I am planning to send a RFC for the API clk_disable_unprepare_sync()
in CCF for separate discussion which guarantees synchronous operation
for Clock OFF.

Cheers,
Biju