linux-tegra.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 00/10] Support for Tegra video capture from external sensor
@ 2020-08-03 15:42 Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 01/10] media: tegra-video: Fix channel format alignment Sowjanya Komatineni
                   ` (9 more replies)
  0 siblings, 10 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

This series adds support for video capture from external camera sensor to
Tegra video driver.

Jetson TX1 has camera expansion connector and supports custom camera module
designed as per TX1 design specification.

This series also enables camera capture support for Jetson Nano which has
Raspberry PI camera header.

This series is tested with IMX274 and IMX219 camera sensors.

This series include,

Tegra video driver updates
- TPG Vs Non-TPG based on Kconfig
- Support for external sensor video capture based on device graph from DT
- Support for selection ioctl operations
- Tegra MIPI CSI pads calibration
- CSI T-CLK and T-HS settle time computation based on clock rates
- dt-binding doc update

Host1x driver updates
- Keep MIPI clock enabled till calibration is done

Delta between patch versions:
[v8]:	Includes below minor changes
	- Fixed missing of_node_put and updated error message to be
	  more informative in tegra_csi_channels_alloc() and
	  tegra_vi_channels_alloc()

	Patch-0006 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/

[v7]:	Includes minor feedback from v6
	- Patch-0009 has minor update

	Note:
	Patch-0006 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/

[v6]:	Includes below changes based on v5 feedback
	- Patches are based on latest linux-next.
	- separated stream enable and disable implementations into
	  separate functions for tegra_channel_set_stream() and
	  tegra_csi_s_stream().
	- changed dev_err to dev_warn on MIPI calibration failure after
	  sensor streaming as its not critical error.

	Note:
	Patch-0006 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/

[v5]:	Includes below minor change based on v4 feedback
	Patch-0012: renames APIs to use calibration instead of calibrate.

	Note:
	Patch-0010 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/

[v4]:	Includes below fix based on v3 feedback
	- Patches are based on latest linux-next.
	- With split of tegra_mipi_calibrate() and tegra_mipi_wait(), mipi
	  clock is not left enabled till calibration done. This series adds
	  a patch to fix this by keeping clock enabled till calibration is
	  done.

	Note:
	Patch-0010 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/

[v3]:	Includes v2 feedback
	- Uses separate helper function for retrieving remote csi subdevice
	  and source subdevice.
	- Added check for presence of subdevice ops set/get_selection
	- dropped vb2_queue_release from driver and using
	  vb2_video_unregister_device instead of video_unregister_device.
	- video device register should happen in the last after all video
	  device related setup is done in the driver. This is being addressed
	  in below RFC patch. Once proper implementation of this is available
	  will update Tegra video driver to use split APIs and do all setup
	  prior to device register. Added this as TODO in the driver.
	  https://www.spinics.net/lists/linux-media/msg172761.html

	Note:
	Patch-0012 has compilation dependency on
	https://patchwork.kernel.org/patch/11659521/


[v2]:	Includes below changes based on v1 feedback
	- dt-binding document and the driver update for device graph to use
	  separate ports for sink endpoint and source endpoint for csi.
	- Use data-lanes endpoint property for csi.
	- Update tegra_mipi_request() to take device node pointer argument
	  rather than adding extra API.
	- Remove checking for clk pointer before clk_disable.


Sowjanya Komatineni (10):
  media: tegra-video: Fix channel format alignment
  media: tegra-video: Enable TPG based on kernel config
  media: tegra-video: Update format lookup to offset based
  dt-bindings: tegra: Update VI and CSI bindings with port info
  media: tegra-video: Separate CSI stream enable and disable
    implementations
  media: tegra-video: Add support for external sensor capture
  media: tegra-video: Add support for selection ioctl ops
  gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  media: tegra-video: Add CSI MIPI pads calibration
  media: tegra-video: Compute settle times based on the clock rate

 .../display/tegra/nvidia,tegra20-host1x.txt        |  92 ++-
 drivers/gpu/drm/tegra/dsi.c                        |   4 +-
 drivers/gpu/host1x/mipi.c                          |  19 +-
 drivers/staging/media/tegra-video/Kconfig          |   7 +
 drivers/staging/media/tegra-video/TODO             |   6 -
 drivers/staging/media/tegra-video/csi.c            | 314 +++++++-
 drivers/staging/media/tegra-video/csi.h            |   8 +
 drivers/staging/media/tegra-video/tegra210.c       |  25 +-
 drivers/staging/media/tegra-video/vi.c             | 841 +++++++++++++++++++--
 drivers/staging/media/tegra-video/vi.h             |  25 +-
 drivers/staging/media/tegra-video/video.c          |  23 +-
 include/linux/host1x.h                             |   5 +-
 12 files changed, 1252 insertions(+), 117 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH v8 01/10] media: tegra-video: Fix channel format alignment
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 02/10] media: tegra-video: Enable TPG based on kernel config Sowjanya Komatineni
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

Pixel format width is mistakenly aligned to surface align bytes
and altering width to aligned value may force sensor mode change
other than the requested one and also cause mismatch in width
programmed between sensor and vi which can lead to capture errors.

This patch removes width alignment and clamps width as per Tegra
minimum and maximum limits.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/vi.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 1b5e660..d621ebc 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -359,25 +359,15 @@ static void tegra_channel_fmt_align(struct tegra_vi_channel *chan,
 				    struct v4l2_pix_format *pix,
 				    unsigned int bpp)
 {
-	unsigned int align;
-	unsigned int min_width;
-	unsigned int max_width;
-	unsigned int width;
 	unsigned int min_bpl;
 	unsigned int max_bpl;
 	unsigned int bpl;
 
 	/*
-	 * The transfer alignment requirements are expressed in bytes. Compute
-	 * minimum and maximum values, clamp the requested width and convert
-	 * it back to pixels. Use bytesperline to adjust the width.
+	 * The transfer alignment requirements are expressed in bytes.
+	 * Clamp the requested width and height to the limits.
 	 */
-	align = lcm(SURFACE_ALIGN_BYTES, bpp);
-	min_width = roundup(TEGRA_MIN_WIDTH, align);
-	max_width = rounddown(TEGRA_MAX_WIDTH, align);
-	width = roundup(pix->width * bpp, align);
-
-	pix->width = clamp(width, min_width, max_width) / bpp;
+	pix->width = clamp(pix->width, TEGRA_MIN_WIDTH, TEGRA_MAX_WIDTH);
 	pix->height = clamp(pix->height, TEGRA_MIN_HEIGHT, TEGRA_MAX_HEIGHT);
 
 	/* Clamp the requested bytes per line value. If the maximum bytes per
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 02/10] media: tegra-video: Enable TPG based on kernel config
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 01/10] media: tegra-video: Fix channel format alignment Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 03/10] media: tegra-video: Update format lookup to offset based Sowjanya Komatineni
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

Tegra internal TPG mode is only for Tegra vi and csi testing
without a real sensor and driver should default support real
sensor.

So, This patch adds CONFIG_VIDEO_TEGRA_TPG and enables Tegra
internal TPG mode only when this config is selected.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/Kconfig    |  6 +++++
 drivers/staging/media/tegra-video/csi.c      | 38 +++++++++++++++++++++++-----
 drivers/staging/media/tegra-video/tegra210.c |  6 +++++
 drivers/staging/media/tegra-video/vi.c       | 13 +++++++---
 drivers/staging/media/tegra-video/video.c    | 23 +++++++++--------
 5 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/tegra-video/Kconfig b/drivers/staging/media/tegra-video/Kconfig
index f6c61ec..566da62 100644
--- a/drivers/staging/media/tegra-video/Kconfig
+++ b/drivers/staging/media/tegra-video/Kconfig
@@ -10,3 +10,9 @@ config VIDEO_TEGRA
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called tegra-video.
+
+config VIDEO_TEGRA_TPG
+	bool "NVIDIA Tegra VI driver TPG mode"
+	depends on VIDEO_TEGRA
+	help
+	  Say yes here to enable Tegra internal TPG mode
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 40ea195..fb667df 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -62,6 +62,9 @@ static int csi_enum_bus_code(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_mbus_code_enum *code)
 {
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	if (code->index >= ARRAY_SIZE(tegra_csi_tpg_fmts))
 		return -EINVAL;
 
@@ -76,6 +79,9 @@ static int csi_get_format(struct v4l2_subdev *subdev,
 {
 	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	fmt->format = csi_chan->format;
 
 	return 0;
@@ -121,6 +127,9 @@ static int csi_enum_framesizes(struct v4l2_subdev *subdev,
 {
 	unsigned int i;
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	if (fse->index >= ARRAY_SIZE(tegra_csi_tpg_sizes))
 		return -EINVAL;
 
@@ -148,6 +157,9 @@ static int csi_enum_frameintervals(struct v4l2_subdev *subdev,
 	const struct tpg_framerate *frmrate = csi->soc->tpg_frmrate_table;
 	int index;
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	/* one framerate per format and resolution */
 	if (fie->index > 0)
 		return -EINVAL;
@@ -172,6 +184,9 @@ static int csi_set_format(struct v4l2_subdev *subdev,
 	const struct v4l2_frmsize_discrete *sizes;
 	unsigned int i;
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	sizes = v4l2_find_nearest_size(tegra_csi_tpg_sizes,
 				       ARRAY_SIZE(tegra_csi_tpg_sizes),
 				       width, height,
@@ -208,6 +223,9 @@ static int tegra_csi_g_frame_interval(struct v4l2_subdev *subdev,
 {
 	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return -ENOIOCTLCMD;
+
 	vfi->interval.numerator = 1;
 	vfi->interval.denominator = csi_chan->framerate;
 
@@ -311,8 +329,12 @@ static int tegra_csi_channel_init(struct tegra_csi_channel *chan)
 	subdev = &chan->subdev;
 	v4l2_subdev_init(subdev, &tegra_csi_ops);
 	subdev->dev = csi->dev;
-	snprintf(subdev->name, V4L2_SUBDEV_NAME_SIZE, "%s-%d", "tpg",
-		 chan->csi_port_num);
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		snprintf(subdev->name, V4L2_SUBDEV_NAME_SIZE, "%s-%d", "tpg",
+			 chan->csi_port_num);
+	else
+		snprintf(subdev->name, V4L2_SUBDEV_NAME_SIZE, "%s",
+			 kbasename(chan->of_node->full_name));
 
 	v4l2_set_subdevdata(subdev, chan);
 	subdev->fwnode = of_fwnode_handle(chan->of_node);
@@ -405,11 +427,13 @@ static int tegra_csi_init(struct host1x_client *client)
 
 	INIT_LIST_HEAD(&csi->csi_chans);
 
-	ret = tegra_csi_tpg_channels_alloc(csi);
-	if (ret < 0) {
-		dev_err(csi->dev,
-			"failed to allocate tpg channels: %d\n", ret);
-		goto cleanup;
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+		ret = tegra_csi_tpg_channels_alloc(csi);
+		if (ret < 0) {
+			dev_err(csi->dev,
+				"failed to allocate tpg channels: %d\n", ret);
+			goto cleanup;
+		}
 	}
 
 	ret = tegra_csi_channels_init(csi);
diff --git a/drivers/staging/media/tegra-video/tegra210.c b/drivers/staging/media/tegra-video/tegra210.c
index 3baa4e3..3492a8a 100644
--- a/drivers/staging/media/tegra-video/tegra210.c
+++ b/drivers/staging/media/tegra-video/tegra210.c
@@ -631,7 +631,11 @@ const struct tegra_vi_soc tegra210_vi_soc = {
 	.ops = &tegra210_vi_ops,
 	.hw_revision = 3,
 	.vi_max_channels = 6,
+#if IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)
 	.vi_max_clk_hz = 499200000,
+#else
+	.vi_max_clk_hz = 998400000,
+#endif
 };
 
 /* Tegra210 CSI PHY registers accessors */
@@ -957,7 +961,9 @@ static const char * const tegra210_csi_cil_clks[] = {
 	"cilab",
 	"cilcd",
 	"cile",
+#if IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)
 	"csi_tpg",
+#endif
 };
 
 /* Tegra210 CSI operations */
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index d621ebc..0197f4e 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -565,6 +565,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
 {
 	int ret;
 
+#if IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)
 	/* add test pattern control handler to v4l2 device */
 	v4l2_ctrl_new_std_menu_items(&chan->ctrl_handler, &vi_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
@@ -576,6 +577,7 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
 		v4l2_ctrl_handler_free(&chan->ctrl_handler);
 		return chan->ctrl_handler.error;
 	}
+#endif
 
 	/* setup the controls */
 	ret = v4l2_ctrl_handler_setup(&chan->ctrl_handler);
@@ -918,10 +920,13 @@ static int tegra_vi_init(struct host1x_client *client)
 
 	INIT_LIST_HEAD(&vi->vi_chans);
 
-	ret = tegra_vi_tpg_channels_alloc(vi);
-	if (ret < 0) {
-		dev_err(vi->dev, "failed to allocate tpg channels: %d\n", ret);
-		goto free_chans;
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+		ret = tegra_vi_tpg_channels_alloc(vi);
+		if (ret < 0) {
+			dev_err(vi->dev,
+				"failed to allocate tpg channels: %d\n", ret);
+			goto free_chans;
+		}
 	}
 
 	ret = tegra_vi_channels_init(vi);
diff --git a/drivers/staging/media/tegra-video/video.c b/drivers/staging/media/tegra-video/video.c
index 30816aa..e50bd70 100644
--- a/drivers/staging/media/tegra-video/video.c
+++ b/drivers/staging/media/tegra-video/video.c
@@ -60,15 +60,17 @@ static int host1x_video_probe(struct host1x_device *dev)
 	if (ret < 0)
 		goto unregister_v4l2;
 
-	/*
-	 * Both vi and csi channels are available now.
-	 * Register v4l2 nodes and create media links for TPG.
-	 */
-	ret = tegra_v4l2_nodes_setup_tpg(vid);
-	if (ret < 0) {
-		dev_err(&dev->dev,
-			"failed to setup tpg graph: %d\n", ret);
-		goto device_exit;
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+		/*
+		 * Both vi and csi channels are available now.
+		 * Register v4l2 nodes and create media links for TPG.
+		 */
+		ret = tegra_v4l2_nodes_setup_tpg(vid);
+		if (ret < 0) {
+			dev_err(&dev->dev,
+				"failed to setup tpg graph: %d\n", ret);
+			goto device_exit;
+		}
 	}
 
 	return 0;
@@ -91,7 +93,8 @@ static int host1x_video_remove(struct host1x_device *dev)
 {
 	struct tegra_video_device *vid = dev_get_drvdata(&dev->dev);
 
-	tegra_v4l2_nodes_cleanup_tpg(vid);
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		tegra_v4l2_nodes_cleanup_tpg(vid);
 
 	host1x_device_exit(dev);
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 03/10] media: tegra-video: Update format lookup to offset based
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 01/10] media: tegra-video: Fix channel format alignment Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 02/10] media: tegra-video: Enable TPG based on kernel config Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 04/10] dt-bindings: tegra: Update VI and CSI bindings with port info Sowjanya Komatineni
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

Tegra VI supported video formats are more for non TPG and there
can be multiple pixel formats for the same media bus format.

This patch updates the helper function for format lookup based on
mbus code from pre-defined Tegra supported format list to look from
the specified list index offset.

Offset based look up is used with sensor device graph (non TPG)
where format enumeration can list all supported formats for the
specific sensor mbus codes.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/vi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 0197f4e..52d751f 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -53,11 +53,12 @@ to_tegra_channel_buffer(struct vb2_v4l2_buffer *vb)
 }
 
 static int tegra_get_format_idx_by_code(struct tegra_vi *vi,
-					unsigned int code)
+					unsigned int code,
+					unsigned int offset)
 {
 	unsigned int i;
 
-	for (i = 0; i < vi->soc->nformats; ++i) {
+	for (i = offset; i < vi->soc->nformats; ++i) {
 		if (vi->soc->video_formats[i].code == code)
 			return i;
 	}
@@ -598,11 +599,12 @@ static void vi_tpg_fmts_bitmap_init(struct tegra_vi_channel *chan)
 	bitmap_zero(chan->tpg_fmts_bitmap, MAX_FORMAT_NUM);
 
 	index = tegra_get_format_idx_by_code(chan->vi,
-					     MEDIA_BUS_FMT_SRGGB10_1X10);
+					     MEDIA_BUS_FMT_SRGGB10_1X10, 0);
 	bitmap_set(chan->tpg_fmts_bitmap, index, 1);
 
 	index = tegra_get_format_idx_by_code(chan->vi,
-					     MEDIA_BUS_FMT_RGB888_1X32_PADHI);
+					     MEDIA_BUS_FMT_RGB888_1X32_PADHI,
+					     0);
 	bitmap_set(chan->tpg_fmts_bitmap, index, 1);
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 04/10] dt-bindings: tegra: Update VI and CSI bindings with port info
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (2 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 03/10] media: tegra-video: Update format lookup to offset based Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 05/10] media: tegra-video: Separate CSI stream enable and disable implementations Sowjanya Komatineni
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

Update VI and CSI bindings to add port and endpoint nodes as per
media video-interfaces DT binding document.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../display/tegra/nvidia,tegra20-host1x.txt        | 92 +++++++++++++++++++++-
 1 file changed, 90 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
index 4731921..ac63ae4a 100644
--- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
@@ -51,8 +51,16 @@ of the following host1x client modules:
       - vi
   - Tegra210:
     - power-domains: Must include venc powergate node as vi is in VE partition.
-  - Tegra210 has CSI part of VI sharing same host interface and register space.
-    So, VI device node should have CSI child node.
+
+  ports (optional node)
+  vi can have optional ports node and max 6 ports are supported. Each port
+  should have single 'endpoint' child node. All port nodes are grouped under
+  ports node. Please refer to the bindings defined in
+  Documentation/devicetree/bindings/media/video-interfaces.txt
+
+  csi (required node)
+  Tegra210 has CSI part of VI sharing same host interface and register space.
+  So, VI device node should have CSI child node.
 
     - csi: mipi csi interface to vi
 
@@ -65,6 +73,46 @@ of the following host1x client modules:
       - power-domains: Must include sor powergate node as csicil is in
         SOR partition.
 
+      channel (optional nodes)
+      Maximum 6 channels are supported with each csi brick as either x4 or x2
+      based on hw connectivity to sensor.
+
+      Required properties:
+      - reg: csi port number. Valid port numbers are 0 through 5.
+      - nvidia,mipi-calibrate: Should contain a phandle and a specifier
+        specifying which pads are used by this CSI port and need to be
+	calibrated. See also ../display/tegra/nvidia,tegra114-mipi.txt.
+
+      Each channel node must contain 2 port nodes which can be grouped
+      under 'ports' node and each port should have a single child 'endpoint'
+      node.
+
+        ports node
+        Please refer to the bindings defined in
+        Documentation/devicetree/bindings/media/video-interfaces.txt
+
+        ports node must contain below 2 port nodes.
+        port@0 with single child 'endpoint' node always a sink.
+        port@1 with single child 'endpoint' node always a source.
+
+        port@0 (required node)
+        Required properties:
+        - reg: 0
+
+	  endpoint (required node)
+	  Required properties:
+	  - data-lanes: an array of data lane from 1 to 4. Valid array
+	    lengths are 1/2/4.
+	  - remote-endpoint: phandle to sensor 'endpoint' node.
+
+        port@1 (required node)
+        Required properties:
+        - reg: 1
+
+	  endpoint (required node)
+	  Required properties:
+	  - remote-endpoint: phandle to vi port 'endpoint' node.
+
 - epp: encoder pre-processor
 
   Required properties:
@@ -340,6 +388,18 @@ Example:
 
 			ranges = <0x0 0x0 0x54080000 0x2000>;
 
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					imx219_vi_in0: endpoint {
+						remote-endpoint = <&imx219_csi_out0>;
+					};
+				};
+			};
+
 			csi@838 {
 				compatible = "nvidia,tegra210-csi";
 				reg = <0x838 0x1300>;
@@ -362,6 +422,34 @@ Example:
 					 <&tegra_car TEGRA210_CLK_CSI_TPG>;
 				clock-names = "csi", "cilab", "cilcd", "cile", "csi_tpg";
 				power-domains = <&pd_sor>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				channel@0 {
+					reg = <0>;
+					nvidia,mipi-calibrate = <&mipi 0x001>;
+
+					ports {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						port@0 {
+							reg = <0>;
+							imx219_csi_in0: endpoint {
+								data-lanes = <1 2>;
+								remote-endpoint = <&imx219_out0>;
+							};
+						};
+
+						port@1 {
+							reg = <1>;
+							imx219_csi_out0: endpoint {
+								remote-endpoint = <&imx219_vi_in0>;
+							};
+						};
+					};
+				};
 			};
 		};
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 05/10] media: tegra-video: Separate CSI stream enable and disable implementations
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (3 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 04/10] dt-bindings: tegra: Update VI and CSI bindings with port info Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture Sowjanya Komatineni
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

This patch separates implementation of CSI stream enable and disable
into separate functions for readability.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/csi.c | 51 ++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index fb667df..cfe6187 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -232,34 +232,53 @@ static int tegra_csi_g_frame_interval(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
+static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
 {
 	struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
 	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
 	struct tegra_csi *csi = csi_chan->csi;
-	int ret = 0;
+	int ret;
+
+	ret = pm_runtime_get_sync(csi->dev);
+	if (ret < 0) {
+		dev_err(csi->dev, "failed to get runtime PM: %d\n", ret);
+		pm_runtime_put_noidle(csi->dev);
+		return ret;
+	}
 
 	csi_chan->pg_mode = chan->pg_mode;
-	if (enable) {
-		ret = pm_runtime_get_sync(csi->dev);
-		if (ret < 0) {
-			dev_err(csi->dev,
-				"failed to get runtime PM: %d\n", ret);
-			pm_runtime_put_noidle(csi->dev);
-			return ret;
-		}
+	ret = csi->ops->csi_start_streaming(csi_chan);
+	if (ret < 0)
+		goto rpm_put;
 
-		ret = csi->ops->csi_start_streaming(csi_chan);
-		if (ret < 0)
-			goto rpm_put;
+	return 0;
 
-		return 0;
-	}
+rpm_put:
+	pm_runtime_put(csi->dev);
+	return ret;
+}
+
+static int tegra_csi_disable_stream(struct v4l2_subdev *subdev)
+{
+	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
+	struct tegra_csi *csi = csi_chan->csi;
 
 	csi->ops->csi_stop_streaming(csi_chan);
 
-rpm_put:
 	pm_runtime_put(csi->dev);
+
+	return 0;
+}
+
+static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+	int ret;
+
+	if (enable)
+		ret = tegra_csi_enable_stream(subdev);
+	else
+		ret = tegra_csi_disable_stream(subdev);
+
 	return ret;
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (4 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 05/10] media: tegra-video: Separate CSI stream enable and disable implementations Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-04  5:57   ` Dmitry Osipenko
  2020-08-03 15:42 ` [PATCH v8 07/10] media: tegra-video: Add support for selection ioctl ops Sowjanya Komatineni
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

This patch adds support to capture from the external sensor
based on device graph in the device tree.

Driver walks through the device graph to create media links
between the entities and registers and unregisters video devices
when the corresponding sub-devices are bound and unbound.

Channel formats are enumerated based on available formats from
the sensor and the corresponding matched formats from the Tegra
supported video formats list.

Each Tegra CSI instance can be configured as 4-lane or 2-lane
based on supported lane configuration from the sensor through
the device tree.

Currently this driver supports V4L2 video node centric only.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/Kconfig    |   1 +
 drivers/staging/media/tegra-video/TODO       |   4 -
 drivers/staging/media/tegra-video/csi.c      | 139 +++++-
 drivers/staging/media/tegra-video/csi.h      |   1 +
 drivers/staging/media/tegra-video/tegra210.c |   2 +-
 drivers/staging/media/tegra-video/vi.c       | 692 +++++++++++++++++++++++++--
 drivers/staging/media/tegra-video/vi.h       |  25 +-
 7 files changed, 800 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/media/tegra-video/Kconfig b/drivers/staging/media/tegra-video/Kconfig
index 566da62..1f35da4 100644
--- a/drivers/staging/media/tegra-video/Kconfig
+++ b/drivers/staging/media/tegra-video/Kconfig
@@ -5,6 +5,7 @@ config VIDEO_TEGRA
 	depends on VIDEO_V4L2
 	select MEDIA_CONTROLLER
 	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
 	help
 	  Choose this option if you have an NVIDIA Tegra SoC.
 
diff --git a/drivers/staging/media/tegra-video/TODO b/drivers/staging/media/tegra-video/TODO
index 6ceb7549..97a19b4 100644
--- a/drivers/staging/media/tegra-video/TODO
+++ b/drivers/staging/media/tegra-video/TODO
@@ -1,8 +1,4 @@
 TODO list
-* Currently driver supports Tegra build-in TPG only with direct media links
-  from CSI to VI. Add kernel config CONFIG_VIDEO_TEGRA_TPG and update the
-  driver to do TPG Vs Sensor media links based on CONFIG_VIDEO_TEGRA_TPG.
-* Add real camera sensor capture support.
 * Add Tegra CSI MIPI pads calibration.
 * Add MIPI clock Settle time computation based on the data rate.
 * Add support for Ganged mode.
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index cfe6187..4176933 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -9,10 +9,13 @@
 #include <linux/host1x.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
+#include <media/v4l2-fwnode.h>
+
 #include "csi.h"
 #include "video.h"
 
@@ -304,31 +307,115 @@ static const struct v4l2_subdev_ops tegra_csi_ops = {
 	.pad    = &tegra_csi_pad_ops,
 };
 
+static int tegra_csi_channel_alloc(struct tegra_csi *csi,
+				   struct device_node *node,
+				   unsigned int port_num, unsigned int lanes,
+				   unsigned int num_pads)
+{
+	struct tegra_csi_channel *chan;
+
+	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	list_add_tail(&chan->list, &csi->csi_chans);
+	chan->csi = csi;
+	chan->csi_port_num = port_num;
+	chan->numlanes = lanes;
+	chan->of_node = node;
+	chan->numpads = num_pads;
+	if (num_pads & 0x2) {
+		chan->pads[0].flags = MEDIA_PAD_FL_SINK;
+		chan->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+	} else {
+		chan->pads[0].flags = MEDIA_PAD_FL_SOURCE;
+	}
+
+	return 0;
+}
+
 static int tegra_csi_tpg_channels_alloc(struct tegra_csi *csi)
 {
 	struct device_node *node = csi->dev->of_node;
 	unsigned int port_num;
-	struct tegra_csi_channel *chan;
 	unsigned int tpg_channels = csi->soc->csi_max_channels;
+	int ret;
 
 	/* allocate CSI channel for each CSI x2 ports */
 	for (port_num = 0; port_num < tpg_channels; port_num++) {
-		chan = kzalloc(sizeof(*chan), GFP_KERNEL);
-		if (!chan)
-			return -ENOMEM;
-
-		list_add_tail(&chan->list, &csi->csi_chans);
-		chan->csi = csi;
-		chan->csi_port_num = port_num;
-		chan->numlanes = 2;
-		chan->of_node = node;
-		chan->numpads = 1;
-		chan->pads[0].flags = MEDIA_PAD_FL_SOURCE;
+		ret = tegra_csi_channel_alloc(csi, node, port_num, 2, 1);
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
 }
 
+static int tegra_csi_channels_alloc(struct tegra_csi *csi)
+{
+	struct device_node *node = csi->dev->of_node;
+	struct v4l2_fwnode_endpoint v4l2_ep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	struct fwnode_handle *fwh;
+	struct device_node *channel;
+	struct device_node *ep;
+	unsigned int lanes, portno, num_pads;
+	int ret;
+
+	for_each_child_of_node(node, channel) {
+		if (!of_node_name_eq(channel, "channel"))
+			continue;
+
+		ret = of_property_read_u32(channel, "reg", &portno);
+		if (ret < 0)
+			continue;
+
+		if (portno >= csi->soc->csi_max_channels) {
+			dev_err(csi->dev, "invalid port num %d for %pOF\n",
+				portno, channel);
+			ret = -EINVAL;
+			goto err_node_put;
+		}
+
+		ep = of_graph_get_endpoint_by_regs(channel, 0, 0);
+		if (!ep)
+			continue;
+
+		fwh = of_fwnode_handle(ep);
+		ret = v4l2_fwnode_endpoint_parse(fwh, &v4l2_ep);
+		of_node_put(ep);
+		if (ret) {
+			dev_err(csi->dev,
+				"failed to parse v4l2 endpoint for %pOF: %d\n",
+				channel, ret);
+			goto err_node_put;
+		}
+
+		lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
+		if (!lanes || ((lanes & (lanes - 1)) != 0)) {
+			dev_err(csi->dev, "invalid data-lanes %d for %pOF\n",
+				lanes, channel);
+			ret = -EINVAL;
+			goto err_node_put;
+		}
+
+		num_pads = of_graph_get_endpoint_count(channel);
+		if (num_pads == TEGRA_CSI_PADS_NUM) {
+			ret = tegra_csi_channel_alloc(csi, channel, portno,
+						      lanes, num_pads);
+			if (ret < 0)
+				goto err_node_put;
+		}
+	}
+
+	return 0;
+
+err_node_put:
+	of_node_put(channel);
+	return ret;
+}
+
 static int tegra_csi_channel_init(struct tegra_csi_channel *chan)
 {
 	struct tegra_csi *csi = chan->csi;
@@ -369,6 +456,15 @@ static int tegra_csi_channel_init(struct tegra_csi_channel *chan)
 		return ret;
 	}
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+		ret = v4l2_async_register_subdev(subdev);
+		if (ret < 0) {
+			dev_err(csi->dev,
+				"failed to register subdev: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -408,8 +504,12 @@ static void tegra_csi_channels_cleanup(struct tegra_csi *csi)
 
 	list_for_each_entry_safe(chan, tmp, &csi->csi_chans, list) {
 		subdev = &chan->subdev;
-		if (subdev->dev)
+		if (subdev->dev) {
+			if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+				v4l2_async_unregister_subdev(subdev);
 			media_entity_cleanup(&subdev->entity);
+		}
+
 		list_del(&chan->list);
 		kfree(chan);
 	}
@@ -446,13 +546,14 @@ static int tegra_csi_init(struct host1x_client *client)
 
 	INIT_LIST_HEAD(&csi->csi_chans);
 
-	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
 		ret = tegra_csi_tpg_channels_alloc(csi);
-		if (ret < 0) {
-			dev_err(csi->dev,
-				"failed to allocate tpg channels: %d\n", ret);
-			goto cleanup;
-		}
+	else
+		ret = tegra_csi_channels_alloc(csi);
+	if (ret < 0) {
+		dev_err(csi->dev,
+			"failed to allocate channels: %d\n", ret);
+		goto cleanup;
 	}
 
 	ret = tegra_csi_channels_init(csi);
diff --git a/drivers/staging/media/tegra-video/csi.h b/drivers/staging/media/tegra-video/csi.h
index 93bd2a0..78a5110 100644
--- a/drivers/staging/media/tegra-video/csi.h
+++ b/drivers/staging/media/tegra-video/csi.h
@@ -7,6 +7,7 @@
 #define __TEGRA_CSI_H__
 
 #include <media/media-entity.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-subdev.h>
 
 /*
diff --git a/drivers/staging/media/tegra-video/tegra210.c b/drivers/staging/media/tegra-video/tegra210.c
index 3492a8a..253bf33 100644
--- a/drivers/staging/media/tegra-video/tegra210.c
+++ b/drivers/staging/media/tegra-video/tegra210.c
@@ -230,7 +230,7 @@ static void tegra_channel_capture_error_recover(struct tegra_vi_channel *chan)
 	tegra_channel_capture_setup(chan);
 
 	/* recover CSI block */
-	subdev = tegra_channel_get_remote_subdev(chan);
+	subdev = tegra_channel_get_remote_csi_subdev(chan);
 	tegra_csi_error_recover(subdev);
 }
 
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 52d751f..deb1252 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
@@ -52,6 +53,12 @@ to_tegra_channel_buffer(struct vb2_v4l2_buffer *vb)
 	return container_of(vb, struct tegra_channel_buffer, buf);
 }
 
+static inline struct tegra_vi_graph_entity *
+to_tegra_vi_graph_entity(struct v4l2_async_subdev *asd)
+{
+	return container_of(asd, struct tegra_vi_graph_entity, asd);
+}
+
 static int tegra_get_format_idx_by_code(struct tegra_vi *vi,
 					unsigned int code,
 					unsigned int offset)
@@ -146,33 +153,106 @@ static void tegra_channel_buffer_queue(struct vb2_buffer *vb)
 }
 
 struct v4l2_subdev *
-tegra_channel_get_remote_subdev(struct tegra_vi_channel *chan)
+tegra_channel_get_remote_csi_subdev(struct tegra_vi_channel *chan)
+{
+	struct media_pad *pad;
+
+	pad = media_entity_remote_pad(&chan->pad);
+	if (!pad)
+		return NULL;
+
+	return media_entity_to_v4l2_subdev(pad->entity);
+}
+
+struct v4l2_subdev *
+tegra_channel_get_remote_source_subdev(struct tegra_vi_channel *chan)
 {
 	struct media_pad *pad;
 	struct v4l2_subdev *subdev;
 	struct media_entity *entity;
 
-	pad = media_entity_remote_pad(&chan->pad);
-	entity = pad->entity;
-	subdev = media_entity_to_v4l2_subdev(entity);
+	subdev = tegra_channel_get_remote_csi_subdev(chan);
+	if (!subdev)
+		return NULL;
+
+	pad = &subdev->entity.pads[0];
+	while (!(pad->flags & MEDIA_PAD_FL_SOURCE)) {
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+		entity = pad->entity;
+		pad = &entity->pads[0];
+		subdev = media_entity_to_v4l2_subdev(entity);
+	}
 
 	return subdev;
 }
 
-int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
+static int tegra_channel_enable_stream(struct tegra_vi_channel *chan)
+{
+	struct v4l2_subdev *csi_subdev, *src_subdev;
+	int ret;
+
+	/*
+	 * Tegra CSI receiver can detect the first LP to HS transition.
+	 * So, start the CSI stream-on prior to sensor stream-on and
+	 * vice-versa for stream-off.
+	 */
+	csi_subdev = tegra_channel_get_remote_csi_subdev(chan);
+	ret = v4l2_subdev_call(csi_subdev, video, s_stream, true);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return 0;
+
+	src_subdev = tegra_channel_get_remote_source_subdev(chan);
+	ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
+	if (ret < 0 && ret != -ENOIOCTLCMD) {
+		v4l2_subdev_call(csi_subdev, video, s_stream, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tegra_channel_disable_stream(struct tegra_vi_channel *chan)
 {
 	struct v4l2_subdev *subdev;
 	int ret;
 
-	/* stream CSI */
-	subdev = tegra_channel_get_remote_subdev(chan);
-	ret = v4l2_subdev_call(subdev, video, s_stream, on);
-	if (on && ret < 0 && ret != -ENOIOCTLCMD)
+	/*
+	 * Stream-off subdevices in reverse order to stream-on.
+	 * Remote source subdev in TPG mode is same as CSI subdev.
+	 */
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	ret = v4l2_subdev_call(subdev, video, s_stream, false);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return 0;
+
+	subdev = tegra_channel_get_remote_csi_subdev(chan);
+	ret = v4l2_subdev_call(subdev, video, s_stream, false);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
 		return ret;
 
 	return 0;
 }
 
+int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
+{
+	int ret;
+
+	if (on)
+		ret = tegra_channel_enable_stream(chan);
+	else
+		ret = tegra_channel_disable_stream(chan);
+
+	return ret;
+}
+
 void tegra_channel_release_buffers(struct tegra_vi_channel *chan,
 				   enum vb2_buffer_state state)
 {
@@ -252,7 +332,7 @@ static int tegra_channel_g_parm(struct file *file, void *fh,
 	struct tegra_vi_channel *chan = video_drvdata(file);
 	struct v4l2_subdev *subdev;
 
-	subdev = tegra_channel_get_remote_subdev(chan);
+	subdev = tegra_channel_get_remote_source_subdev(chan);
 	return v4l2_g_parm_cap(&chan->video, subdev, a);
 }
 
@@ -262,7 +342,7 @@ static int tegra_channel_s_parm(struct file *file, void *fh,
 	struct tegra_vi_channel *chan = video_drvdata(file);
 	struct v4l2_subdev *subdev;
 
-	subdev = tegra_channel_get_remote_subdev(chan);
+	subdev = tegra_channel_get_remote_source_subdev(chan);
 	return v4l2_s_parm_cap(&chan->video, subdev, a);
 }
 
@@ -284,7 +364,7 @@ static int tegra_channel_enum_framesizes(struct file *file, void *fh,
 
 	fse.code = fmtinfo->code;
 
-	subdev = tegra_channel_get_remote_subdev(chan);
+	subdev = tegra_channel_get_remote_source_subdev(chan);
 	ret = v4l2_subdev_call(subdev, pad, enum_frame_size, NULL, &fse);
 	if (ret)
 		return ret;
@@ -316,7 +396,7 @@ static int tegra_channel_enum_frameintervals(struct file *file, void *fh,
 
 	fie.code = fmtinfo->code;
 
-	subdev = tegra_channel_get_remote_subdev(chan);
+	subdev = tegra_channel_get_remote_source_subdev(chan);
 	ret = v4l2_subdev_call(subdev, pad, enum_frame_interval, NULL, &fie);
 	if (ret)
 		return ret;
@@ -335,6 +415,9 @@ static int tegra_channel_enum_format(struct file *file, void *fh,
 	unsigned int index = 0, i;
 	unsigned long *fmts_bitmap = chan->tpg_fmts_bitmap;
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		fmts_bitmap = chan->fmts_bitmap;
+
 	if (f->index >= bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM))
 		return -EINVAL;
 
@@ -391,8 +474,12 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	struct v4l2_subdev *subdev;
 	struct v4l2_subdev_format fmt;
 	struct v4l2_subdev_pad_config *pad_cfg;
+	int ret;
+
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	if (!subdev)
+		return -ENODEV;
 
-	subdev = tegra_channel_get_remote_subdev(chan);
 	pad_cfg = v4l2_subdev_alloc_pad_config(subdev);
 	if (!pad_cfg)
 		return -ENOMEM;
@@ -412,7 +499,10 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	fmt.which = V4L2_SUBDEV_FORMAT_TRY;
 	fmt.pad = 0;
 	v4l2_fill_mbus_format(&fmt.format, pix, fmtinfo->code);
-	v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
+	ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
+	if (ret < 0)
+		return ret;
+
 	v4l2_fill_pix_format(pix, &fmt.format);
 	tegra_channel_fmt_align(chan, pix, fmtinfo->bpp);
 
@@ -452,8 +542,11 @@ static int tegra_channel_set_format(struct file *file, void *fh,
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	fmt.pad = 0;
 	v4l2_fill_mbus_format(&fmt.format, pix, fmtinfo->code);
-	subdev = tegra_channel_get_remote_subdev(chan);
-	v4l2_subdev_call(subdev, pad, set_fmt, NULL, &fmt);
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &fmt);
+	if (ret < 0)
+		return ret;
+
 	v4l2_fill_pix_format(pix, &fmt.format);
 	tegra_channel_fmt_align(chan, pix, fmtinfo->bpp);
 
@@ -463,15 +556,50 @@ static int tegra_channel_set_format(struct file *file, void *fh,
 	return 0;
 }
 
+static int tegra_channel_set_subdev_active_fmt(struct tegra_vi_channel *chan)
+{
+	int ret, index;
+	struct v4l2_subdev *subdev;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+
+	/*
+	 * Initialize channel format to the sub-device active format if there
+	 * is corresponding match in the Tegra supported video formats.
+	 */
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret;
+
+	index = tegra_get_format_idx_by_code(chan->vi, fmt.format.code, 0);
+	if (index < 0)
+		return -EINVAL;
+
+	chan->fmtinfo = &chan->vi->soc->video_formats[index];
+	v4l2_fill_pix_format(&chan->format, &fmt.format);
+	chan->format.pixelformat = chan->fmtinfo->fourcc;
+	chan->format.bytesperline = chan->format.width * chan->fmtinfo->bpp;
+	chan->format.sizeimage = chan->format.bytesperline *
+				 chan->format.height;
+	tegra_channel_fmt_align(chan, &chan->format, chan->fmtinfo->bpp);
+
+	return 0;
+}
+
 static int tegra_channel_enum_input(struct file *file, void *fh,
 				    struct v4l2_input *inp)
 {
-	/* currently driver supports internal TPG only */
+	struct tegra_vi_channel *chan = video_drvdata(file);
+	struct v4l2_subdev *subdev;
+
 	if (inp->index)
 		return -EINVAL;
 
 	inp->type = V4L2_INPUT_TYPE_CAMERA;
-	strscpy(inp->name, "Tegra TPG", sizeof(inp->name));
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	strscpy(inp->name, subdev->name, sizeof(inp->name));
 
 	return 0;
 }
@@ -578,6 +706,22 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan)
 		v4l2_ctrl_handler_free(&chan->ctrl_handler);
 		return chan->ctrl_handler.error;
 	}
+#else
+	struct v4l2_subdev *subdev;
+
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	if (!subdev)
+		return -ENODEV;
+
+	ret = v4l2_ctrl_add_handler(&chan->ctrl_handler, subdev->ctrl_handler,
+				    NULL, true);
+	if (ret < 0) {
+		dev_err(chan->vi->dev,
+			"failed to add subdev %s ctrl handler: %d\n",
+			subdev->name, ret);
+		v4l2_ctrl_handler_free(&chan->ctrl_handler);
+		return ret;
+	}
 #endif
 
 	/* setup the controls */
@@ -608,6 +752,61 @@ static void vi_tpg_fmts_bitmap_init(struct tegra_vi_channel *chan)
 	bitmap_set(chan->tpg_fmts_bitmap, index, 1);
 }
 
+static int vi_fmts_bitmap_init(struct tegra_vi_channel *chan)
+{
+	int index, ret, match_code = 0;
+	struct v4l2_subdev *subdev;
+	struct v4l2_subdev_mbus_code_enum code = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+
+	bitmap_zero(chan->fmts_bitmap, MAX_FORMAT_NUM);
+
+	/*
+	 * Set the bitmap bits based on all the matched formats between the
+	 * available media bus formats of sub-device and the pre-defined Tegra
+	 * supported video formats.
+	 */
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	while (1) {
+		ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
+				       NULL, &code);
+		if (ret < 0)
+			break;
+
+		index = tegra_get_format_idx_by_code(chan->vi, code.code, 0);
+		while (index >= 0) {
+			bitmap_set(chan->fmts_bitmap, index, 1);
+			if (!match_code)
+				match_code = code.code;
+			/* look for other formats with same mbus code */
+			index = tegra_get_format_idx_by_code(chan->vi,
+							     code.code,
+							     index + 1);
+		}
+
+		code.index++;
+	}
+
+	/*
+	 * Set the bitmap bit corresponding to default tegra video format if
+	 * there are no matched formats.
+	 */
+	if (!match_code) {
+		match_code = tegra_default_format.code;
+		index = tegra_get_format_idx_by_code(chan->vi, match_code, 0);
+		if (WARN_ON(index < 0))
+			return -EINVAL;
+
+		bitmap_set(chan->fmts_bitmap, index, 1);
+	}
+
+	/* initialize channel format to the sub-device active format */
+	tegra_channel_set_subdev_active_fmt(chan);
+
+	return 0;
+}
+
 static void tegra_channel_cleanup(struct tegra_vi_channel *chan)
 {
 	v4l2_ctrl_handler_free(&chan->ctrl_handler);
@@ -720,6 +919,9 @@ static int tegra_channel_init(struct tegra_vi_channel *chan)
 		goto free_v4l2_ctrl_hdl;
 	}
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		v4l2_async_notifier_init(&chan->notifier);
+
 	return 0;
 
 free_v4l2_ctrl_hdl:
@@ -733,31 +935,90 @@ static int tegra_channel_init(struct tegra_vi_channel *chan)
 	return ret;
 }
 
-static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
+static int tegra_vi_channel_alloc(struct tegra_vi *vi, unsigned int port_num,
+				  struct device_node *node)
 {
 	struct tegra_vi_channel *chan;
+
+	/*
+	 * Do not use devm_kzalloc as memory is freed immediately
+	 * when device instance is unbound but application might still
+	 * be holding the device node open. Channel memory allocated
+	 * with kzalloc is freed during video device release callback.
+	 */
+	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	chan->vi = vi;
+	chan->portno = port_num;
+	chan->of_node = node;
+	list_add_tail(&chan->list, &vi->vi_chans);
+
+	return 0;
+}
+
+static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
+{
 	unsigned int port_num;
 	unsigned int nchannels = vi->soc->vi_max_channels;
+	int ret;
 
 	for (port_num = 0; port_num < nchannels; port_num++) {
-		/*
-		 * Do not use devm_kzalloc as memory is freed immediately
-		 * when device instance is unbound but application might still
-		 * be holding the device node open. Channel memory allocated
-		 * with kzalloc is freed during video device release callback.
-		 */
-		chan = kzalloc(sizeof(*chan), GFP_KERNEL);
-		if (!chan)
-			return -ENOMEM;
-
-		chan->vi = vi;
-		chan->portno = port_num;
-		list_add_tail(&chan->list, &vi->vi_chans);
+		ret = tegra_vi_channel_alloc(vi, port_num, vi->dev->of_node);
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
 }
 
+static int tegra_vi_channels_alloc(struct tegra_vi *vi)
+{
+	struct device_node *node = vi->dev->of_node;
+	struct device_node *ep = NULL;
+	struct device_node *ports;
+	struct device_node *port;
+	unsigned int port_num;
+	int ret = 0;
+
+	ports = of_get_child_by_name(node, "ports");
+	if (!ports)
+		return -ENODEV;
+
+	for_each_child_of_node(ports, port) {
+		if (!of_node_name_eq(port, "port"))
+			continue;
+
+		ret = of_property_read_u32(port, "reg", &port_num);
+		if (ret < 0)
+			continue;
+
+		if (port_num > vi->soc->vi_max_channels) {
+			dev_err(vi->dev, "invalid port num %d for %pOF\n",
+				port_num, port);
+			ret = -EINVAL;
+			of_node_put(port);
+			goto cleanup;
+		}
+
+		ep = of_get_child_by_name(port, "endpoint");
+		if (!ep)
+			continue;
+
+		of_node_put(ep);
+		ret = tegra_vi_channel_alloc(vi, port_num, port);
+		if (ret < 0) {
+			of_node_put(port);
+			goto cleanup;
+		}
+	}
+
+cleanup:
+	of_node_put(ports);
+	return ret;
+}
+
 static int tegra_vi_channels_init(struct tegra_vi *vi)
 {
 	struct tegra_vi_channel *chan;
@@ -909,6 +1170,347 @@ static int __maybe_unused vi_runtime_suspend(struct device *dev)
 	return 0;
 }
 
+/*
+ * Graph Management
+ */
+static struct tegra_vi_graph_entity *
+tegra_vi_graph_find_entity(struct tegra_vi_channel *chan,
+			   const struct fwnode_handle *fwnode)
+{
+	struct tegra_vi_graph_entity *entity;
+	struct v4l2_async_subdev *asd;
+
+	list_for_each_entry(asd, &chan->notifier.asd_list, asd_list) {
+		entity = to_tegra_vi_graph_entity(asd);
+		if (entity->asd.match.fwnode == fwnode)
+			return entity;
+	}
+
+	return NULL;
+}
+
+static int tegra_vi_graph_build(struct tegra_vi_channel *chan,
+				struct tegra_vi_graph_entity *entity)
+{
+	struct tegra_vi *vi = chan->vi;
+	struct tegra_vi_graph_entity *ent;
+	struct fwnode_handle *ep = NULL;
+	struct v4l2_fwnode_link link;
+	struct media_entity *local = entity->entity;
+	struct media_entity *remote;
+	struct media_pad *local_pad;
+	struct media_pad *remote_pad;
+	u32 link_flags = MEDIA_LNK_FL_ENABLED;
+	int ret = 0;
+
+	dev_dbg(vi->dev, "creating links for entity %s\n", local->name);
+
+	while (1) {
+		ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
+						    ep);
+		if (!ep)
+			break;
+
+		ret = v4l2_fwnode_parse_link(ep, &link);
+		if (ret < 0) {
+			dev_err(vi->dev, "failed to parse link for %pOF: %d\n",
+				to_of_node(ep), ret);
+			continue;
+		}
+
+		if (link.local_port >= local->num_pads) {
+			dev_err(vi->dev, "invalid port number %u on %pOF\n",
+				link.local_port, to_of_node(link.local_node));
+			v4l2_fwnode_put_link(&link);
+			ret = -EINVAL;
+			break;
+		}
+
+		local_pad = &local->pads[link.local_port];
+		/* Remote node is vi node. So use channel video entity and pad
+		 * as remote/sink.
+		 */
+		if (link.remote_node == of_fwnode_handle(vi->dev->of_node)) {
+			remote = &chan->video.entity;
+			remote_pad = &chan->pad;
+			goto create_link;
+		}
+
+		/*
+		 * Skip sink ports, they will be processed from the other end
+		 * of the link.
+		 */
+		if (local_pad->flags & MEDIA_PAD_FL_SINK) {
+			dev_dbg(vi->dev, "skipping sink port %pOF:%u\n",
+				to_of_node(link.local_node), link.local_port);
+			v4l2_fwnode_put_link(&link);
+			continue;
+		}
+
+		/* find the remote entity from notifier list */
+		ent = tegra_vi_graph_find_entity(chan, link.remote_node);
+		if (!ent) {
+			dev_err(vi->dev, "no entity found for %pOF\n",
+				to_of_node(link.remote_node));
+			v4l2_fwnode_put_link(&link);
+			ret = -ENODEV;
+			break;
+		}
+
+		remote = ent->entity;
+		if (link.remote_port >= remote->num_pads) {
+			dev_err(vi->dev, "invalid port number %u on %pOF\n",
+				link.remote_port,
+				to_of_node(link.remote_node));
+			v4l2_fwnode_put_link(&link);
+			ret = -EINVAL;
+			break;
+		}
+
+		remote_pad = &remote->pads[link.remote_port];
+
+create_link:
+		dev_dbg(vi->dev, "creating %s:%u -> %s:%u link\n",
+			local->name, local_pad->index,
+			remote->name, remote_pad->index);
+
+		ret = media_create_pad_link(local, local_pad->index,
+					    remote, remote_pad->index,
+					    link_flags);
+		v4l2_fwnode_put_link(&link);
+		if (ret < 0) {
+			dev_err(vi->dev,
+				"failed to create %s:%u -> %s:%u link: %d\n",
+				local->name, local_pad->index,
+				remote->name, remote_pad->index, ret);
+			break;
+		}
+	}
+
+	fwnode_handle_put(ep);
+	return ret;
+}
+
+static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier)
+{
+	struct tegra_vi_graph_entity *entity;
+	struct v4l2_async_subdev *asd;
+	struct v4l2_subdev *subdev;
+	struct tegra_vi_channel *chan;
+	struct tegra_vi *vi;
+	int ret;
+
+	chan = container_of(notifier, struct tegra_vi_channel, notifier);
+	vi = chan->vi;
+
+	dev_dbg(vi->dev, "notify complete, all subdevs registered\n");
+
+	/*
+	 * Video device node should be created at the end of all the device
+	 * related initialization/setup.
+	 * Current video_register_device() does both initialize and register
+	 * video device in same API.
+	 *
+	 * TODO: Update v4l2-dev driver to split initialize and register into
+	 * separate APIs and then update Tegra video driver to do video device
+	 * initialize followed by all video device related setup and then
+	 * register the video device.
+	 */
+	ret = video_register_device(&chan->video, VFL_TYPE_VIDEO, -1);
+	if (ret < 0) {
+		dev_err(vi->dev,
+			"failed to register video device: %d\n", ret);
+		goto unregister_video;
+	}
+
+	/* create links between the entities */
+	list_for_each_entry(asd, &chan->notifier.asd_list, asd_list) {
+		entity = to_tegra_vi_graph_entity(asd);
+		ret = tegra_vi_graph_build(chan, entity);
+		if (ret < 0)
+			goto unregister_video;
+	}
+
+	ret = tegra_channel_setup_ctrl_handler(chan);
+	if (ret < 0) {
+		dev_err(vi->dev,
+			"failed to setup channel controls: %d\n", ret);
+		goto unregister_video;
+	}
+
+	ret = vi_fmts_bitmap_init(chan);
+	if (ret < 0) {
+		dev_err(vi->dev,
+			"failed to initialize formats bitmap: %d\n", ret);
+		goto unregister_video;
+	}
+
+	subdev = tegra_channel_get_remote_csi_subdev(chan);
+	if (!subdev) {
+		ret = -ENODEV;
+		dev_err(vi->dev,
+			"failed to get remote csi subdev: %d\n", ret);
+		goto unregister_video;
+	}
+
+	v4l2_set_subdev_hostdata(subdev, chan);
+
+	return 0;
+
+unregister_video:
+	video_unregister_device(&chan->video);
+	return ret;
+}
+
+static int tegra_vi_graph_notify_bound(struct v4l2_async_notifier *notifier,
+				       struct v4l2_subdev *subdev,
+				       struct v4l2_async_subdev *asd)
+{
+	struct tegra_vi_graph_entity *entity;
+	struct tegra_vi *vi;
+	struct tegra_vi_channel *chan;
+
+	chan = container_of(notifier, struct tegra_vi_channel, notifier);
+	vi = chan->vi;
+
+	/*
+	 * Locate the entity corresponding to the bound subdev and store the
+	 * subdev pointer.
+	 */
+	entity = tegra_vi_graph_find_entity(chan, subdev->fwnode);
+	if (!entity) {
+		dev_err(vi->dev, "no entity for subdev %s\n", subdev->name);
+		return -EINVAL;
+	}
+
+	if (entity->subdev) {
+		dev_err(vi->dev, "duplicate subdev for node %pOF\n",
+			to_of_node(entity->asd.match.fwnode));
+		return -EINVAL;
+	}
+
+	dev_dbg(vi->dev, "subdev %s bound\n", subdev->name);
+	entity->entity = &subdev->entity;
+	entity->subdev = subdev;
+
+	return 0;
+}
+
+static const struct v4l2_async_notifier_operations tegra_vi_async_ops = {
+	.bound = tegra_vi_graph_notify_bound,
+	.complete = tegra_vi_graph_notify_complete,
+};
+
+static int tegra_vi_graph_parse_one(struct tegra_vi_channel *chan,
+				    struct fwnode_handle *fwnode)
+{
+	struct tegra_vi *vi = chan->vi;
+	struct fwnode_handle *ep = NULL;
+	struct fwnode_handle *remote = NULL;
+	struct v4l2_async_subdev *asd;
+	struct device_node *node = NULL;
+	int ret;
+
+	dev_dbg(vi->dev, "parsing node %pOF\n", to_of_node(fwnode));
+
+	/* parse all the remote entities and put them into the list */
+	for_each_endpoint_of_node(to_of_node(fwnode), node) {
+		ep = of_fwnode_handle(node);
+		remote = fwnode_graph_get_remote_port_parent(ep);
+		if (!remote) {
+			dev_err(vi->dev,
+				"remote device at %pOF not found\n", node);
+			ret = -EINVAL;
+			goto cleanup;
+		}
+
+		/* skip entities that are already processed */
+		if (remote == dev_fwnode(vi->dev) ||
+		    tegra_vi_graph_find_entity(chan, remote)) {
+			fwnode_handle_put(remote);
+			continue;
+		}
+
+		asd = v4l2_async_notifier_add_fwnode_subdev(&chan->notifier,
+				remote, sizeof(struct tegra_vi_graph_entity));
+		if (IS_ERR(asd)) {
+			ret = PTR_ERR(asd);
+			dev_err(vi->dev,
+				"failed to add subdev to notifier: %d\n", ret);
+			fwnode_handle_put(remote);
+			goto cleanup;
+		}
+
+		ret = tegra_vi_graph_parse_one(chan, remote);
+		if (ret < 0) {
+			fwnode_handle_put(remote);
+			goto cleanup;
+		}
+
+		fwnode_handle_put(remote);
+	}
+
+	return 0;
+
+cleanup:
+	dev_err(vi->dev, "failed parsing the graph: %d\n", ret);
+	v4l2_async_notifier_cleanup(&chan->notifier);
+	of_node_put(node);
+	return ret;
+}
+
+static int tegra_vi_graph_init(struct tegra_vi *vi)
+{
+	struct tegra_video_device *vid = dev_get_drvdata(vi->client.host);
+	struct tegra_vi_channel *chan;
+	struct fwnode_handle *fwnode = dev_fwnode(vi->dev);
+	int ret;
+	struct fwnode_handle *remote = NULL;
+
+	/*
+	 * Walk the links to parse the full graph. Each channel will have
+	 * one endpoint of the composite node. Start by parsing the
+	 * composite node and parse the remote entities in turn.
+	 * Each channel will register v4l2 async notifier to make the graph
+	 * independent between the channels so we can the current channel
+	 * in case of something wrong during graph parsing and continue with
+	 * next channels.
+	 */
+	list_for_each_entry(chan, &vi->vi_chans, list) {
+		remote = fwnode_graph_get_remote_node(fwnode, chan->portno, 0);
+		if (!remote)
+			continue;
+
+		ret = tegra_vi_graph_parse_one(chan, remote);
+		fwnode_handle_put(remote);
+		if (ret < 0 || list_empty(&chan->notifier.asd_list))
+			continue;
+
+		chan->notifier.ops = &tegra_vi_async_ops;
+		ret = v4l2_async_notifier_register(&vid->v4l2_dev,
+						   &chan->notifier);
+		if (ret < 0) {
+			dev_err(vi->dev,
+				"failed to register channel %d notifier: %d\n",
+				chan->portno, ret);
+			v4l2_async_notifier_cleanup(&chan->notifier);
+		}
+	}
+
+	return 0;
+}
+
+static void tegra_vi_graph_cleanup(struct tegra_vi *vi)
+{
+	struct tegra_vi_channel *chan;
+
+	list_for_each_entry(chan, &vi->vi_chans, list) {
+		vb2_video_unregister_device(&chan->video);
+		v4l2_async_notifier_unregister(&chan->notifier);
+		v4l2_async_notifier_cleanup(&chan->notifier);
+	}
+}
+
 static int tegra_vi_init(struct host1x_client *client)
 {
 	struct tegra_video_device *vid = dev_get_drvdata(client->host);
@@ -922,13 +1524,14 @@ static int tegra_vi_init(struct host1x_client *client)
 
 	INIT_LIST_HEAD(&vi->vi_chans);
 
-	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
 		ret = tegra_vi_tpg_channels_alloc(vi);
-		if (ret < 0) {
-			dev_err(vi->dev,
-				"failed to allocate tpg channels: %d\n", ret);
-			goto free_chans;
-		}
+	else
+		ret = tegra_vi_channels_alloc(vi);
+	if (ret < 0) {
+		dev_err(vi->dev,
+			"failed to allocate vi channels: %d\n", ret);
+		goto free_chans;
 	}
 
 	ret = tegra_vi_channels_init(vi);
@@ -937,6 +1540,12 @@ static int tegra_vi_init(struct host1x_client *client)
 
 	vid->vi = vi;
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) {
+		ret = tegra_vi_graph_init(vi);
+		if (ret < 0)
+			goto free_chans;
+	}
+
 	return 0;
 
 free_chans:
@@ -950,6 +1559,8 @@ static int tegra_vi_init(struct host1x_client *client)
 
 static int tegra_vi_exit(struct host1x_client *client)
 {
+	struct tegra_vi *vi = host1x_client_to_vi(client);
+
 	/*
 	 * Do not cleanup the channels here as application might still be
 	 * holding video device nodes. Channels cleanup will happen during
@@ -957,6 +1568,9 @@ static int tegra_vi_exit(struct host1x_client *client)
 	 * device nodes are released.
 	 */
 
+	if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		tegra_vi_graph_cleanup(vi);
+
 	return 0;
 }
 
diff --git a/drivers/staging/media/tegra-video/vi.h b/drivers/staging/media/tegra-video/vi.h
index 6272c9a..7d6b7a6 100644
--- a/drivers/staging/media/tegra-video/vi.h
+++ b/drivers/staging/media/tegra-video/vi.h
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 
 #include <media/media-entity.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-dev.h>
@@ -93,6 +94,19 @@ struct tegra_vi {
 };
 
 /**
+ * struct tegra_vi_graph_entity - Entity in the video graph
+ *
+ * @asd: subdev asynchronous registration information
+ * @entity: media entity from the corresponding V4L2 subdev
+ * @subdev: V4L2 subdev
+ */
+struct tegra_vi_graph_entity {
+	struct v4l2_async_subdev asd;
+	struct media_entity *entity;
+	struct v4l2_subdev *subdev;
+};
+
+/**
  * struct tegra_vi_channel - Tegra video channel
  *
  * @list: list head for this entry
@@ -138,10 +152,13 @@ struct tegra_vi {
  * @done_lock: protects the capture done queue list
  *
  * @portno: VI channel port number
+ * @of_node: device node of VI channel
  *
  * @ctrl_handler: V4L2 control handler of this video channel
+ * @fmts_bitmap: a bitmap for supported formats matching v4l2 subdev formats
  * @tpg_fmts_bitmap: a bitmap for supported TPG formats
  * @pg_mode: test pattern generator mode (disabled/direct/patch)
+ * @notifier: V4L2 asynchronous subdevs notifier
  */
 struct tegra_vi_channel {
 	struct list_head list;
@@ -174,10 +191,14 @@ struct tegra_vi_channel {
 	spinlock_t done_lock;
 
 	unsigned char portno;
+	struct device_node *of_node;
 
 	struct v4l2_ctrl_handler ctrl_handler;
+	DECLARE_BITMAP(fmts_bitmap, MAX_FORMAT_NUM);
 	DECLARE_BITMAP(tpg_fmts_bitmap, MAX_FORMAT_NUM);
 	enum tegra_vi_pg_mode pg_mode;
+
+	struct v4l2_async_notifier notifier;
 };
 
 /**
@@ -249,7 +270,9 @@ extern const struct tegra_vi_soc tegra210_vi_soc;
 #endif
 
 struct v4l2_subdev *
-tegra_channel_get_remote_subdev(struct tegra_vi_channel *chan);
+tegra_channel_get_remote_csi_subdev(struct tegra_vi_channel *chan);
+struct v4l2_subdev *
+tegra_channel_get_remote_source_subdev(struct tegra_vi_channel *chan);
 int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on);
 void tegra_channel_release_buffers(struct tegra_vi_channel *chan,
 				   enum vb2_buffer_state state);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 07/10] media: tegra-video: Add support for selection ioctl ops
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (5 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done Sowjanya Komatineni
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

This patch adds selection v4l2 ioctl operations to allow configuring
a selection rectangle in the sensor through the Tegra video device
node.

Some sensor drivers supporting crop uses try_crop rectangle from
v4l2_subdev_pad_config during try format for computing binning.

So with selection ops support, this patch also updates try format
to use try crop rectangle either from subdev frame size enumeration
or from subdev crop boundary.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/vi.c | 106 +++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index deb1252..29a172f 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -474,6 +474,13 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	struct v4l2_subdev *subdev;
 	struct v4l2_subdev_format fmt;
 	struct v4l2_subdev_pad_config *pad_cfg;
+	struct v4l2_subdev_frame_size_enum fse = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+	};
+	struct v4l2_subdev_selection sdsel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.target = V4L2_SEL_TGT_CROP_BOUNDS,
+	};
 	int ret;
 
 	subdev = tegra_channel_get_remote_source_subdev(chan);
@@ -499,6 +506,24 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	fmt.which = V4L2_SUBDEV_FORMAT_TRY;
 	fmt.pad = 0;
 	v4l2_fill_mbus_format(&fmt.format, pix, fmtinfo->code);
+
+	/*
+	 * Attempt to obtain the format size from subdev.
+	 * If not available, try to get crop boundary from subdev.
+	 */
+	fse.code = fmtinfo->code;
+	ret = v4l2_subdev_call(subdev, pad, enum_frame_size, pad_cfg, &fse);
+	if (ret) {
+		ret = v4l2_subdev_call(subdev, pad, get_selection, NULL, &sdsel);
+		if (ret)
+			return -EINVAL;
+		pad_cfg->try_crop.width = sdsel.r.width;
+		pad_cfg->try_crop.height = sdsel.r.height;
+	} else {
+		pad_cfg->try_crop.width = fse.max_width;
+		pad_cfg->try_crop.height = fse.max_height;
+	}
+
 	ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
 	if (ret < 0)
 		return ret;
@@ -588,6 +613,85 @@ static int tegra_channel_set_subdev_active_fmt(struct tegra_vi_channel *chan)
 	return 0;
 }
 
+static int tegra_channel_g_selection(struct file *file, void *priv,
+				     struct v4l2_selection *sel)
+{
+	struct tegra_vi_channel *chan = video_drvdata(file);
+	struct v4l2_subdev *subdev;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev_selection sdsel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.target = sel->target,
+	};
+	int ret;
+
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	if (!v4l2_subdev_has_op(subdev, pad, get_selection))
+		return -ENOTTY;
+
+	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+	/*
+	 * Try the get selection operation and fallback to get format if not
+	 * implemented.
+	 */
+	ret = v4l2_subdev_call(subdev, pad, get_selection, NULL, &sdsel);
+	if (!ret)
+		sel->r = sdsel.r;
+	if (ret != -ENOIOCTLCMD)
+		return ret;
+
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret < 0)
+		return ret;
+
+	sel->r.left = 0;
+	sel->r.top = 0;
+	sel->r.width = fmt.format.width;
+	sel->r.height = fmt.format.height;
+
+	return 0;
+}
+
+static int tegra_channel_s_selection(struct file *file, void *fh,
+				     struct v4l2_selection *sel)
+{
+	struct tegra_vi_channel *chan = video_drvdata(file);
+	struct v4l2_subdev *subdev;
+	int ret;
+	struct v4l2_subdev_selection sdsel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.target = sel->target,
+		.flags = sel->flags,
+		.r = sel->r,
+	};
+
+	subdev = tegra_channel_get_remote_source_subdev(chan);
+	if (!v4l2_subdev_has_op(subdev, pad, set_selection))
+		return -ENOTTY;
+
+	if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	if (vb2_is_busy(&chan->queue))
+		return -EBUSY;
+
+	ret = v4l2_subdev_call(subdev, pad, set_selection, NULL, &sdsel);
+	if (!ret) {
+		sel->r = sdsel.r;
+		/*
+		 * Subdev active format resolution may have changed during
+		 * set selection operation. So, update channel format to
+		 * the sub-device active format.
+		 */
+		return tegra_channel_set_subdev_active_fmt(chan);
+	}
+
+	return ret;
+}
+
 static int tegra_channel_enum_input(struct file *file, void *fh,
 				    struct v4l2_input *inp)
 {
@@ -645,6 +749,8 @@ static const struct v4l2_ioctl_ops tegra_channel_ioctl_ops = {
 	.vidioc_streamoff		= vb2_ioctl_streamoff,
 	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
 	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
+	.vidioc_g_selection		= tegra_channel_g_selection,
+	.vidioc_s_selection		= tegra_channel_s_selection,
 };
 
 /*
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (6 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 07/10] media: tegra-video: Add support for selection ioctl ops Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-05 13:46   ` Thierry Reding
  2020-08-03 15:42 ` [PATCH v8 09/10] media: tegra-video: Add CSI MIPI pads calibration Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 10/10] media: tegra-video: Compute settle times based on the clock rate Sowjanya Komatineni
  9 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

With the split of MIPI calibration into tegra_mipi_calibrate() and
tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
is done.

So, this patch skips disabling MIPI clock after triggering start of
calibration and disables it only after waiting for done status from
the calibration logic.

This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
with their usage.

As MIPI clock is left enabled and in case of any failures with CSI input
streaming tegra_mipi_finish_calibration() will not get invoked.
So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
and consumer drivers can call this in such cases.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/gpu/drm/tegra/dsi.c |  4 ++--
 drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
 include/linux/host1x.h      |  5 +++--
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 3820e8d..a7864e9 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
 		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
 	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
 
-	err = tegra_mipi_calibrate(dsi->mipi);
+	err = tegra_mipi_start_calibration(dsi->mipi);
 	if (err < 0)
 		return err;
 
-	return tegra_mipi_wait(dsi->mipi);
+	return tegra_mipi_finish_calibration(dsi->mipi);
 }
 
 static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index e606464..b15ab6e 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
 }
 EXPORT_SYMBOL(tegra_mipi_disable);
 
-int tegra_mipi_wait(struct tegra_mipi_device *device)
+void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
+{
+	clk_disable(device->mipi->clk);
+}
+EXPORT_SYMBOL(tegra_mipi_cancel_calibration);
+
+int tegra_mipi_finish_calibration(struct tegra_mipi_device *device)
 {
 	struct tegra_mipi *mipi = device->mipi;
 	void __iomem *status_reg = mipi->regs + (MIPI_CAL_STATUS << 2);
 	u32 value;
 	int err;
 
-	err = clk_enable(device->mipi->clk);
-	if (err < 0)
-		return err;
-
 	mutex_lock(&device->mipi->lock);
 
 	err = readl_relaxed_poll_timeout(status_reg, value,
@@ -315,9 +317,9 @@ int tegra_mipi_wait(struct tegra_mipi_device *device)
 
 	return err;
 }
-EXPORT_SYMBOL(tegra_mipi_wait);
+EXPORT_SYMBOL(tegra_mipi_finish_calibration);
 
-int tegra_mipi_calibrate(struct tegra_mipi_device *device)
+int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
 {
 	const struct tegra_mipi_soc *soc = device->mipi->soc;
 	unsigned int i;
@@ -382,11 +384,10 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
 
 	mutex_unlock(&device->mipi->lock);
-	clk_disable(device->mipi->clk);
 
 	return 0;
 }
-EXPORT_SYMBOL(tegra_mipi_calibrate);
+EXPORT_SYMBOL(tegra_mipi_start_calibration);
 
 static const struct tegra_mipi_pad tegra114_mipi_pads[] = {
 	{ .data = MIPI_CAL_CONFIG_CSIA },
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 20c885d..b490dda 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -333,7 +333,8 @@ struct tegra_mipi_device *tegra_mipi_request(struct device *device,
 void tegra_mipi_free(struct tegra_mipi_device *device);
 int tegra_mipi_enable(struct tegra_mipi_device *device);
 int tegra_mipi_disable(struct tegra_mipi_device *device);
-int tegra_mipi_calibrate(struct tegra_mipi_device *device);
-int tegra_mipi_wait(struct tegra_mipi_device *device);
+int tegra_mipi_start_calibration(struct tegra_mipi_device *device);
+int tegra_mipi_finish_calibration(struct tegra_mipi_device *device);
+void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device);
 
 #endif
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 09/10] media: tegra-video: Add CSI MIPI pads calibration
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (7 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  2020-08-03 15:42 ` [PATCH v8 10/10] media: tegra-video: Compute settle times based on the clock rate Sowjanya Komatineni
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

CSI MIPI pads need to be enabled and calibrated for capturing from
the external sensor or transmitter.

MIPI CAL unit calibrates MIPI pads pull-up, pull-down and termination
impedances. Calibration is done by co-work of MIPI BIAS pad and MIPI
CAL control unit.

Triggering calibration start can happen any time after MIPI pads are
enabled but calibration results will be latched and applied to MIPI
pads by MIPI CAL unit only when the link is in LP11 state and then
calibration status register gets updated.

This patch enables CSI MIPI pads and calibrates them during streaming.

Tegra CSI receiver is able to catch the very first clock transition.
So, CSI receiver is always enabled prior to sensor streaming and
trigger of calibration start is done during CSI subdev streaming and
status of calibration is verified after sensor stream on.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/TODO  |  1 -
 drivers/staging/media/tegra-video/csi.c | 61 +++++++++++++++++++++++++++++++--
 drivers/staging/media/tegra-video/csi.h |  2 ++
 drivers/staging/media/tegra-video/vi.c  | 28 ++++++++++++---
 4 files changed, 84 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/tegra-video/TODO b/drivers/staging/media/tegra-video/TODO
index 97a19b4..98d3c7d 100644
--- a/drivers/staging/media/tegra-video/TODO
+++ b/drivers/staging/media/tegra-video/TODO
@@ -1,5 +1,4 @@
 TODO list
-* Add Tegra CSI MIPI pads calibration.
 * Add MIPI clock Settle time computation based on the data rate.
 * Add support for Ganged mode.
 * Add RAW10 packed video format support to Tegra210 video formats.
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 4176933..76b4311 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -240,7 +240,7 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
 	struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
 	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
 	struct tegra_csi *csi = csi_chan->csi;
-	int ret;
+	int ret, err;
 
 	ret = pm_runtime_get_sync(csi->dev);
 	if (ret < 0) {
@@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
 		return ret;
 	}
 
+	if (csi_chan->mipi) {
+		ret = tegra_mipi_enable(csi_chan->mipi);
+		if (ret < 0) {
+			dev_err(csi->dev,
+				"failed to enable MIPI pads: %d\n", ret);
+			goto rpm_put;
+		}
+
+		/*
+		 * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to
+		 * be calibrated after power on.
+		 * So, trigger the calibration start here and results will
+		 * be latched and applied to the pads when link is in LP11
+		 * state during start of sensor streaming.
+		 */
+		ret = tegra_mipi_start_calibration(csi_chan->mipi);
+		if (ret < 0) {
+			dev_err(csi->dev,
+				"failed to start MIPI calibration: %d\n", ret);
+			goto disable_mipi;
+		}
+	}
+
 	csi_chan->pg_mode = chan->pg_mode;
 	ret = csi->ops->csi_start_streaming(csi_chan);
 	if (ret < 0)
-		goto rpm_put;
+		goto cancel_calibration;
 
 	return 0;
 
+cancel_calibration:
+	if (csi_chan->mipi)
+		tegra_mipi_cancel_calibration(csi_chan->mipi);
+disable_mipi:
+	if (csi_chan->mipi) {
+		err = tegra_mipi_disable(csi_chan->mipi);
+		if (err < 0)
+			dev_err(csi->dev,
+				"failed to disable MIPI pads: %d\n", err);
+	}
+
 rpm_put:
 	pm_runtime_put(csi->dev);
 	return ret;
@@ -265,9 +299,17 @@ static int tegra_csi_disable_stream(struct v4l2_subdev *subdev)
 {
 	struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
 	struct tegra_csi *csi = csi_chan->csi;
+	int err;
 
 	csi->ops->csi_stop_streaming(csi_chan);
 
+	if (csi_chan->mipi) {
+		err = tegra_mipi_disable(csi_chan->mipi);
+		if (err < 0)
+			dev_err(csi->dev,
+				"failed to disable MIPI pads: %d\n", err);
+	}
+
 	pm_runtime_put(csi->dev);
 
 	return 0;
@@ -313,6 +355,7 @@ static int tegra_csi_channel_alloc(struct tegra_csi *csi,
 				   unsigned int num_pads)
 {
 	struct tegra_csi_channel *chan;
+	int ret = 0;
 
 	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
 	if (!chan)
@@ -331,7 +374,16 @@ static int tegra_csi_channel_alloc(struct tegra_csi *csi,
 		chan->pads[0].flags = MEDIA_PAD_FL_SOURCE;
 	}
 
-	return 0;
+	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+		return 0;
+
+	chan->mipi = tegra_mipi_request(csi->dev, node);
+	if (IS_ERR(chan->mipi)) {
+		ret = PTR_ERR(chan->mipi);
+		dev_err(csi->dev, "failed to get mipi device: %d\n", ret);
+	}
+
+	return ret;
 }
 
 static int tegra_csi_tpg_channels_alloc(struct tegra_csi *csi)
@@ -503,6 +555,9 @@ static void tegra_csi_channels_cleanup(struct tegra_csi *csi)
 	struct tegra_csi_channel *chan, *tmp;
 
 	list_for_each_entry_safe(chan, tmp, &csi->csi_chans, list) {
+		if (chan->mipi)
+			tegra_mipi_free(chan->mipi);
+
 		subdev = &chan->subdev;
 		if (subdev->dev) {
 			if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
diff --git a/drivers/staging/media/tegra-video/csi.h b/drivers/staging/media/tegra-video/csi.h
index 78a5110..0d50fc3 100644
--- a/drivers/staging/media/tegra-video/csi.h
+++ b/drivers/staging/media/tegra-video/csi.h
@@ -50,6 +50,7 @@ struct tegra_csi;
  * @framerate: active framerate for TPG
  * @h_blank: horizontal blanking for TPG active format
  * @v_blank: vertical blanking for TPG active format
+ * @mipi: mipi device for corresponding csi channel pads
  */
 struct tegra_csi_channel {
 	struct list_head list;
@@ -65,6 +66,7 @@ struct tegra_csi_channel {
 	unsigned int framerate;
 	unsigned int h_blank;
 	unsigned int v_blank;
+	struct tegra_mipi_device *mipi;
 };
 
 /**
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 29a172f..7547295 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -191,6 +191,7 @@ tegra_channel_get_remote_source_subdev(struct tegra_vi_channel *chan)
 static int tegra_channel_enable_stream(struct tegra_vi_channel *chan)
 {
 	struct v4l2_subdev *csi_subdev, *src_subdev;
+	struct tegra_csi_channel *csi_chan;
 	int ret;
 
 	/*
@@ -206,14 +207,33 @@ static int tegra_channel_enable_stream(struct tegra_vi_channel *chan)
 	if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
 		return 0;
 
+	csi_chan = v4l2_get_subdevdata(csi_subdev);
+	/*
+	 * TRM has incorrectly documented to wait for done status from
+	 * calibration logic after CSI interface power on.
+	 * As per the design, calibration results are latched and applied
+	 * to the pads only when the link is in LP11 state which will happen
+	 * during the sensor stream-on.
+	 * CSI subdev stream-on triggers start of MIPI pads calibration.
+	 * Wait for calibration to finish here after sensor subdev stream-on
+	 * and in case of sensor stream-on failure, cancel the calibration.
+	 */
 	src_subdev = tegra_channel_get_remote_source_subdev(chan);
 	ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
-	if (ret < 0 && ret != -ENOIOCTLCMD) {
-		v4l2_subdev_call(csi_subdev, video, s_stream, false);
-		return ret;
-	}
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		goto err_disable_csi_stream;
+
+	ret = tegra_mipi_finish_calibration(csi_chan->mipi);
+	if (ret < 0)
+		dev_warn(csi_chan->csi->dev,
+			 "MIPI calibration failed: %d\n", ret);
 
 	return 0;
+
+err_disable_csi_stream:
+	tegra_mipi_cancel_calibration(csi_chan->mipi);
+	v4l2_subdev_call(csi_subdev, video, s_stream, false);
+	return ret;
 }
 
 static int tegra_channel_disable_stream(struct tegra_vi_channel *chan)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH v8 10/10] media: tegra-video: Compute settle times based on the clock rate
  2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
                   ` (8 preceding siblings ...)
  2020-08-03 15:42 ` [PATCH v8 09/10] media: tegra-video: Add CSI MIPI pads calibration Sowjanya Komatineni
@ 2020-08-03 15:42 ` Sowjanya Komatineni
  9 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-03 15:42 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: digetx, gregkh, linux-media, devicetree, linux-tegra, linux-kernel

Settle time determines the number of cil clock cyles to wait after
LP00 when moving from LP to HS.

This patch computes T-CLK-SETTLE and T-HS-SETTLE times based on cil
clock rate and pixel rate from the sensor and programs them during
streaming.

T-CLK-SETTLE time is the interval during which receiver will ignore
any HS transitions on clock lane starting from the beginning of
T-CLK-PREPARE.

T-HS-SETTLE time is the interval during which recevier will ignore
any HS transitions on data lane starting from the beginning of
T-HS-PREPARE.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/staging/media/tegra-video/TODO       |  1 -
 drivers/staging/media/tegra-video/csi.c      | 55 ++++++++++++++++++++++++++++
 drivers/staging/media/tegra-video/csi.h      |  5 +++
 drivers/staging/media/tegra-video/tegra210.c | 17 ++++++++-
 4 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/tegra-video/TODO b/drivers/staging/media/tegra-video/TODO
index 98d3c7d..c821081 100644
--- a/drivers/staging/media/tegra-video/TODO
+++ b/drivers/staging/media/tegra-video/TODO
@@ -1,5 +1,4 @@
 TODO list
-* Add MIPI clock Settle time computation based on the data rate.
 * Add support for Ganged mode.
 * Add RAW10 packed video format support to Tegra210 video formats.
 * Add support for suspend and resume.
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 76b4311..93875cb 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -19,6 +19,8 @@
 #include "csi.h"
 #include "video.h"
 
+#define MHZ			1000000
+
 static inline struct tegra_csi *
 host1x_client_to_csi(struct host1x_client *client)
 {
@@ -235,6 +237,59 @@ static int tegra_csi_g_frame_interval(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static unsigned int csi_get_pixel_rate(struct tegra_csi_channel *csi_chan)
+{
+	struct tegra_vi_channel *chan;
+	struct v4l2_subdev *src_subdev;
+	struct v4l2_ctrl *ctrl;
+
+	chan = v4l2_get_subdev_hostdata(&csi_chan->subdev);
+	src_subdev = tegra_channel_get_remote_source_subdev(chan);
+	ctrl = v4l2_ctrl_find(src_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
+	if (ctrl)
+		return v4l2_ctrl_g_ctrl_int64(ctrl);
+
+	return 0;
+}
+
+void tegra_csi_calc_settle_time(struct tegra_csi_channel *csi_chan,
+				u8 *clk_settle_time,
+				u8 *ths_settle_time)
+{
+	struct tegra_csi *csi = csi_chan->csi;
+	unsigned int cil_clk_mhz;
+	unsigned int pix_clk_mhz;
+	int clk_idx = (csi_chan->csi_port_num >> 1) + 1;
+
+	cil_clk_mhz = clk_get_rate(csi->clks[clk_idx].clk) / MHZ;
+	pix_clk_mhz = csi_get_pixel_rate(csi_chan) / MHZ;
+
+	/*
+	 * CLK Settle time is the interval during which HS receiver should
+	 * ignore any clock lane HS transitions, starting from the beginning
+	 * of T-CLK-PREPARE.
+	 * Per DPHY specification, T-CLK-SETTLE should be between 95ns ~ 300ns
+	 *
+	 * 95ns < (clk-settle-programmed + 7) * lp clk period < 300ns
+	 * midpoint = 197.5 ns
+	 */
+	*clk_settle_time = ((95 + 300) * cil_clk_mhz - 14000) / 2000;
+
+	/*
+	 * THS Settle time is the interval during which HS receiver should
+	 * ignore any data lane HS transitions, starting from the beginning
+	 * of THS-PREPARE.
+	 *
+	 * Per DPHY specification, T-HS-SETTLE should be between 85ns + 6UI
+	 * and 145ns+10UI.
+	 * 85ns + 6UI < (Ths-settle-prog + 5) * lp_clk_period < 145ns + 10UI
+	 * midpoint = 115ns + 8UI
+	 */
+	if (pix_clk_mhz)
+		*ths_settle_time = (115 * cil_clk_mhz + 8000 * cil_clk_mhz
+				   / (2 * pix_clk_mhz) - 5000) / 1000;
+}
+
 static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
 {
 	struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
diff --git a/drivers/staging/media/tegra-video/csi.h b/drivers/staging/media/tegra-video/csi.h
index 0d50fc3..c65ff73 100644
--- a/drivers/staging/media/tegra-video/csi.h
+++ b/drivers/staging/media/tegra-video/csi.h
@@ -51,6 +51,7 @@ struct tegra_csi;
  * @h_blank: horizontal blanking for TPG active format
  * @v_blank: vertical blanking for TPG active format
  * @mipi: mipi device for corresponding csi channel pads
+ * @pixel_rate: active pixel rate from the sensor on this channel
  */
 struct tegra_csi_channel {
 	struct list_head list;
@@ -67,6 +68,7 @@ struct tegra_csi_channel {
 	unsigned int h_blank;
 	unsigned int v_blank;
 	struct tegra_mipi_device *mipi;
+	unsigned int pixel_rate;
 };
 
 /**
@@ -147,4 +149,7 @@ extern const struct tegra_csi_soc tegra210_csi_soc;
 #endif
 
 void tegra_csi_error_recover(struct v4l2_subdev *subdev);
+void tegra_csi_calc_settle_time(struct tegra_csi_channel *csi_chan,
+				u8 *clk_settle_time,
+				u8 *ths_settle_time);
 #endif
diff --git a/drivers/staging/media/tegra-video/tegra210.c b/drivers/staging/media/tegra-video/tegra210.c
index 253bf33..ac066c0 100644
--- a/drivers/staging/media/tegra-video/tegra210.c
+++ b/drivers/staging/media/tegra-video/tegra210.c
@@ -7,6 +7,7 @@
  * This source file contains Tegra210 supported video formats,
  * VI and CSI SoC specific data, operations and registers accessors.
  */
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
 #include <linux/delay.h>
@@ -98,6 +99,8 @@
 #define   BRICK_CLOCK_B_4X				(0x2 << 16)
 #define TEGRA_CSI_CIL_PAD_CONFIG1                       0x004
 #define TEGRA_CSI_CIL_PHY_CONTROL                       0x008
+#define   CLK_SETTLE_MASK				GENMASK(13, 8)
+#define   THS_SETTLE_MASK				GENMASK(5, 0)
 #define TEGRA_CSI_CIL_INTERRUPT_MASK                    0x00c
 #define TEGRA_CSI_CIL_STATUS                            0x010
 #define TEGRA_CSI_CILX_STATUS                           0x014
@@ -770,8 +773,14 @@ static int tegra210_csi_start_streaming(struct tegra_csi_channel *csi_chan)
 {
 	struct tegra_csi *csi = csi_chan->csi;
 	unsigned int portno = csi_chan->csi_port_num;
+	u8 clk_settle_time = 0;
+	u8 ths_settle_time = 10;
 	u32 val;
 
+	if (!csi_chan->pg_mode)
+		tegra_csi_calc_settle_time(csi_chan, &clk_settle_time,
+					   &ths_settle_time);
+
 	csi_write(csi, portno, TEGRA_CSI_CLKEN_OVERRIDE, 0);
 
 	/* clean up status */
@@ -782,7 +791,9 @@ static int tegra210_csi_start_streaming(struct tegra_csi_channel *csi_chan)
 
 	/* CIL PHY registers setup */
 	cil_write(csi, portno, TEGRA_CSI_CIL_PAD_CONFIG0, 0x0);
-	cil_write(csi, portno, TEGRA_CSI_CIL_PHY_CONTROL, 0xa);
+	cil_write(csi, portno, TEGRA_CSI_CIL_PHY_CONTROL,
+		  FIELD_PREP(CLK_SETTLE_MASK, clk_settle_time) |
+		  FIELD_PREP(THS_SETTLE_MASK, ths_settle_time));
 
 	/*
 	 * The CSI unit provides for connection of up to six cameras in
@@ -801,7 +812,9 @@ static int tegra210_csi_start_streaming(struct tegra_csi_channel *csi_chan)
 			  BRICK_CLOCK_A_4X);
 		cil_write(csi, portno + 1, TEGRA_CSI_CIL_PAD_CONFIG0, 0x0);
 		cil_write(csi, portno + 1, TEGRA_CSI_CIL_INTERRUPT_MASK, 0x0);
-		cil_write(csi, portno + 1, TEGRA_CSI_CIL_PHY_CONTROL, 0xa);
+		cil_write(csi, portno + 1, TEGRA_CSI_CIL_PHY_CONTROL,
+			  FIELD_PREP(CLK_SETTLE_MASK, clk_settle_time) |
+			  FIELD_PREP(THS_SETTLE_MASK, ths_settle_time));
 		csi_write(csi, portno, TEGRA_CSI_PHY_CIL_COMMAND,
 			  CSI_A_PHY_CIL_ENABLE | CSI_B_PHY_CIL_ENABLE);
 	} else {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture
  2020-08-03 15:42 ` [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture Sowjanya Komatineni
@ 2020-08-04  5:57   ` Dmitry Osipenko
  0 siblings, 0 replies; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-04  5:57 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
	sakari.ailus, robh+dt, helen.koike
  Cc: gregkh, linux-media, devicetree, linux-tegra, linux-kernel

03.08.2020 18:42, Sowjanya Komatineni пишет:
> This patch adds support to capture from the external sensor
> based on device graph in the device tree.
> 
> Driver walks through the device graph to create media links
> between the entities and registers and unregisters video devices
> when the corresponding sub-devices are bound and unbound.
> 
> Channel formats are enumerated based on available formats from
> the sensor and the corresponding matched formats from the Tegra
> supported video formats list.
> 
> Each Tegra CSI instance can be configured as 4-lane or 2-lane
> based on supported lane configuration from the sensor through
> the device tree.
> 
> Currently this driver supports V4L2 video node centric only.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/staging/media/tegra-video/Kconfig    |   1 +
>  drivers/staging/media/tegra-video/TODO       |   4 -
>  drivers/staging/media/tegra-video/csi.c      | 139 +++++-
>  drivers/staging/media/tegra-video/csi.h      |   1 +
>  drivers/staging/media/tegra-video/tegra210.c |   2 +-
>  drivers/staging/media/tegra-video/vi.c       | 692 +++++++++++++++++++++++++--
>  drivers/staging/media/tegra-video/vi.h       |  25 +-
>  7 files changed, 800 insertions(+), 64 deletions(-)

This and the rest of the patches look sane to me, thanks.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-03 15:42 ` [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done Sowjanya Komatineni
@ 2020-08-05 13:46   ` Thierry Reding
  2020-08-05 14:05     ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Thierry Reding @ 2020-08-05 13:46 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	digetx, gregkh, linux-media, devicetree, linux-tegra,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
> With the split of MIPI calibration into tegra_mipi_calibrate() and
> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
> is done.
> 
> So, this patch skips disabling MIPI clock after triggering start of
> calibration and disables it only after waiting for done status from
> the calibration logic.
> 
> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
> with their usage.
> 
> As MIPI clock is left enabled and in case of any failures with CSI input
> streaming tegra_mipi_finish_calibration() will not get invoked.
> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
> and consumer drivers can call this in such cases.
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>  include/linux/host1x.h      |  5 +++--
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 3820e8d..a7864e9 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>  
> -	err = tegra_mipi_calibrate(dsi->mipi);
> +	err = tegra_mipi_start_calibration(dsi->mipi);
>  	if (err < 0)
>  		return err;
>  
> -	return tegra_mipi_wait(dsi->mipi);
> +	return tegra_mipi_finish_calibration(dsi->mipi);
>  }
>  
>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index e606464..b15ab6e 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>  }
>  EXPORT_SYMBOL(tegra_mipi_disable);
>  
> -int tegra_mipi_wait(struct tegra_mipi_device *device)
> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
> +{
> +	clk_disable(device->mipi->clk);

Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
registers here? We don't clear the START bit in the former when the
calibration has successfully finished, but I suspect that's because
the bit is self-clearing. But I wonder if we still need to clear it
upon cancellation to make sure the calibration does indeed stop.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 13:46   ` Thierry Reding
@ 2020-08-05 14:05     ` Dmitry Osipenko
  2020-08-05 14:19       ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 14:05 UTC (permalink / raw)
  To: Thierry Reding, Sowjanya Komatineni
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 16:46, Thierry Reding пишет:
> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>> is done.
>>
>> So, this patch skips disabling MIPI clock after triggering start of
>> calibration and disables it only after waiting for done status from
>> the calibration logic.
>>
>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>> with their usage.
>>
>> As MIPI clock is left enabled and in case of any failures with CSI input
>> streaming tegra_mipi_finish_calibration() will not get invoked.
>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>> and consumer drivers can call this in such cases.
>>
>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>  include/linux/host1x.h      |  5 +++--
>>  3 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>> index 3820e8d..a7864e9 100644
>> --- a/drivers/gpu/drm/tegra/dsi.c
>> +++ b/drivers/gpu/drm/tegra/dsi.c
>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>  
>> -	err = tegra_mipi_calibrate(dsi->mipi);
>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>  	if (err < 0)
>>  		return err;
>>  
>> -	return tegra_mipi_wait(dsi->mipi);
>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>  }
>>  
>>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>> index e606464..b15ab6e 100644
>> --- a/drivers/gpu/host1x/mipi.c
>> +++ b/drivers/gpu/host1x/mipi.c
>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>  }
>>  EXPORT_SYMBOL(tegra_mipi_disable);
>>  
>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>> +{
>> +	clk_disable(device->mipi->clk);
> 
> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
> registers here? We don't clear the START bit in the former when the
> calibration has successfully finished, but I suspect that's because
> the bit is self-clearing. But I wonder if we still need to clear it
> upon cancellation to make sure the calibration does indeed stop.

Apparently there is no way to explicitly stop calibration other than to
reset MIPI calibration block, but Sowjanya says this is unnecessary.

Perhaps having a fixed delay before disabling clock could be enough to
ensure that calibration is stopped before the clock is disabled?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 14:05     ` Dmitry Osipenko
@ 2020-08-05 14:19       ` Dmitry Osipenko
  2020-08-05 16:33         ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 14:19 UTC (permalink / raw)
  To: Thierry Reding, Sowjanya Komatineni
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 17:05, Dmitry Osipenko пишет:
> 05.08.2020 16:46, Thierry Reding пишет:
>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>> is done.
>>>
>>> So, this patch skips disabling MIPI clock after triggering start of
>>> calibration and disables it only after waiting for done status from
>>> the calibration logic.
>>>
>>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>> with their usage.
>>>
>>> As MIPI clock is left enabled and in case of any failures with CSI input
>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>>> and consumer drivers can call this in such cases.
>>>
>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>  include/linux/host1x.h      |  5 +++--
>>>  3 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>> index 3820e8d..a7864e9 100644
>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>  
>>> -	err = tegra_mipi_calibrate(dsi->mipi);
>>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>>  	if (err < 0)
>>>  		return err;
>>>  
>>> -	return tegra_mipi_wait(dsi->mipi);
>>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>>  }
>>>  
>>>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>> index e606464..b15ab6e 100644
>>> --- a/drivers/gpu/host1x/mipi.c
>>> +++ b/drivers/gpu/host1x/mipi.c
>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>>  }
>>>  EXPORT_SYMBOL(tegra_mipi_disable);
>>>  
>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>> +{
>>> +	clk_disable(device->mipi->clk);
>>
>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>> registers here? We don't clear the START bit in the former when the
>> calibration has successfully finished, but I suspect that's because
>> the bit is self-clearing. But I wonder if we still need to clear it
>> upon cancellation to make sure the calibration does indeed stop.
> 
> Apparently there is no way to explicitly stop calibration other than to
> reset MIPI calibration block, but Sowjanya says this is unnecessary.
> 
> Perhaps having a fixed delay before disabling clock could be enough to
> ensure that calibration is stopped before the clock is disabled?
> 

Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
it needs to be polled until it's unset?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 14:19       ` Dmitry Osipenko
@ 2020-08-05 16:33         ` Sowjanya Komatineni
  2020-08-05 16:47           ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 16:33 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
> 05.08.2020 17:05, Dmitry Osipenko пишет:
>> 05.08.2020 16:46, Thierry Reding пишет:
>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>> is done.
>>>>
>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>> calibration and disables it only after waiting for done status from
>>>> the calibration logic.
>>>>
>>>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>> with their usage.
>>>>
>>>> As MIPI clock is left enabled and in case of any failures with CSI input
>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>>>> and consumer drivers can call this in such cases.
>>>>
>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>   drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>   drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>   include/linux/host1x.h      |  5 +++--
>>>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>> index 3820e8d..a7864e9 100644
>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>>>   		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>   	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>   
>>>> -	err = tegra_mipi_calibrate(dsi->mipi);
>>>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>>>   	if (err < 0)
>>>>   		return err;
>>>>   
>>>> -	return tegra_mipi_wait(dsi->mipi);
>>>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>>>   }
>>>>   
>>>>   static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>> index e606464..b15ab6e 100644
>>>> --- a/drivers/gpu/host1x/mipi.c
>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>>>   }
>>>>   EXPORT_SYMBOL(tegra_mipi_disable);
>>>>   
>>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>> +{
>>>> +	clk_disable(device->mipi->clk);
>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>> registers here? We don't clear the START bit in the former when the
>>> calibration has successfully finished, but I suspect that's because
>>> the bit is self-clearing. But I wonder if we still need to clear it
>>> upon cancellation to make sure the calibration does indeed stop.
>> Apparently there is no way to explicitly stop calibration other than to
>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>
>> Perhaps having a fixed delay before disabling clock could be enough to
>> ensure that calibration is stopped before the clock is disabled?
>>
> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
> it needs to be polled until it's unset?

Confirmed with HW design team during this patch update.

SW does not need to clear START bit and only write 1 takes effect to 
that bit.

Also, no need to have delay or do any other register settings unclear as 
its FSM and there's nothing to get stuck.

Also it goes thru small finite set of codes and by the time sensor 
programming happens for enabling streaming FSM will finish its 
calibration sequence much early and it will only wait for pads LP-11.

So, during cancel we only need disable MIPI clock.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 16:33         ` Sowjanya Komatineni
@ 2020-08-05 16:47           ` Dmitry Osipenko
  2020-08-05 16:50             ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 16:47 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 19:33, Sowjanya Komatineni пишет:
> 
> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>>> is done.
>>>>>
>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>> calibration and disables it only after waiting for done status from
>>>>> the calibration logic.
>>>>>
>>>>> This patch renames tegra_mipi_calibrate() as
>>>>> tegra_mipi_start_calibration()
>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>>> with their usage.
>>>>>
>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>> input
>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>> MIPI clock
>>>>> and consumer drivers can call this in such cases.
>>>>>
>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>>   drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>   drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>   include/linux/host1x.h      |  5 +++--
>>>>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>>> index 3820e8d..a7864e9 100644
>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>> tegra_dsi *dsi)
>>>>>           DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>       tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>   -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>       if (err < 0)
>>>>>           return err;
>>>>>   -    return tegra_mipi_wait(dsi->mipi);
>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>   }
>>>>>     static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>> unsigned long bclk,
>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>> index e606464..b15ab6e 100644
>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>> tegra_mipi_device *dev)
>>>>>   }
>>>>>   EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>   -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>> +{
>>>>> +    clk_disable(device->mipi->clk);
>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>> registers here? We don't clear the START bit in the former when the
>>>> calibration has successfully finished, but I suspect that's because
>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>> upon cancellation to make sure the calibration does indeed stop.
>>> Apparently there is no way to explicitly stop calibration other than to
>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>
>>> Perhaps having a fixed delay before disabling clock could be enough to
>>> ensure that calibration is stopped before the clock is disabled?
>>>
>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>> it needs to be polled until it's unset?
> 
> Confirmed with HW design team during this patch update.
> 
> SW does not need to clear START bit and only write 1 takes effect to
> that bit.
> 
> Also, no need to have delay or do any other register settings unclear as
> its FSM and there's nothing to get stuck.
> 
> Also it goes thru small finite set of codes and by the time sensor
> programming happens for enabling streaming FSM will finish its
> calibration sequence much early and it will only wait for pads LP-11.
> 
> So, during cancel we only need disable MIPI clock.
> 

But there is no guarantee that cancel_calibration() couldn't be invoked
in the middle of the calibration process, hence FSM could freeze in an
intermediate state if it's running on the disabled MIPI clock, this
doesn't sound good.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 16:47           ` Dmitry Osipenko
@ 2020-08-05 16:50             ` Sowjanya Komatineni
  2020-08-05 16:57               ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 16:50 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>>>> is done.
>>>>>>
>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>> calibration and disables it only after waiting for done status from
>>>>>> the calibration logic.
>>>>>>
>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>> tegra_mipi_start_calibration()
>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>>>> with their usage.
>>>>>>
>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>> input
>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>> MIPI clock
>>>>>> and consumer drivers can call this in such cases.
>>>>>>
>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>    drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>    include/linux/host1x.h      |  5 +++--
>>>>>>    3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>>>> index 3820e8d..a7864e9 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>> tegra_dsi *dsi)
>>>>>>            DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>        tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>    -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>        if (err < 0)
>>>>>>            return err;
>>>>>>    -    return tegra_mipi_wait(dsi->mipi);
>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>    }
>>>>>>      static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>> unsigned long bclk,
>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>> index e606464..b15ab6e 100644
>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>> tegra_mipi_device *dev)
>>>>>>    }
>>>>>>    EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>    -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>>> +{
>>>>>> +    clk_disable(device->mipi->clk);
>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>> registers here? We don't clear the START bit in the former when the
>>>>> calibration has successfully finished, but I suspect that's because
>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>> Apparently there is no way to explicitly stop calibration other than to
>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>
>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>> ensure that calibration is stopped before the clock is disabled?
>>>>
>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>> it needs to be polled until it's unset?
>> Confirmed with HW design team during this patch update.
>>
>> SW does not need to clear START bit and only write 1 takes effect to
>> that bit.
>>
>> Also, no need to have delay or do any other register settings unclear as
>> its FSM and there's nothing to get stuck.
>>
>> Also it goes thru small finite set of codes and by the time sensor
>> programming happens for enabling streaming FSM will finish its
>> calibration sequence much early and it will only wait for pads LP-11.
>>
>> So, during cancel we only need disable MIPI clock.
>>
> But there is no guarantee that cancel_calibration() couldn't be invoked
> in the middle of the calibration process, hence FSM could freeze in an
> intermediate state if it's running on the disabled MIPI clock, this
> doesn't sound good.
Actual calibration logic uses UART_FST_CAL clock which is always enabled

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 16:50             ` Sowjanya Komatineni
@ 2020-08-05 16:57               ` Dmitry Osipenko
  2020-08-05 17:04                 ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 16:57 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 19:50, Sowjanya Komatineni пишет:
> 
> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>> calibration
>>>>>>> is done.
>>>>>>>
>>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>>> calibration and disables it only after waiting for done status from
>>>>>>> the calibration logic.
>>>>>>>
>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>> tegra_mipi_start_calibration()
>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>> inline
>>>>>>> with their usage.
>>>>>>>
>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>> input
>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>> MIPI clock
>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>    drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>    include/linux/host1x.h      |  5 +++--
>>>>>>>    3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>> tegra_dsi *dsi)
>>>>>>>            DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>        tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>    -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>        if (err < 0)
>>>>>>>            return err;
>>>>>>>    -    return tegra_mipi_wait(dsi->mipi);
>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>    }
>>>>>>>      static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>> unsigned long bclk,
>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>> index e606464..b15ab6e 100644
>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>> tegra_mipi_device *dev)
>>>>>>>    }
>>>>>>>    EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>    -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>> *device)
>>>>>>> +{
>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>> Apparently there is no way to explicitly stop calibration other
>>>>> than to
>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>
>>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>
>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>>> it needs to be polled until it's unset?
>>> Confirmed with HW design team during this patch update.
>>>
>>> SW does not need to clear START bit and only write 1 takes effect to
>>> that bit.
>>>
>>> Also, no need to have delay or do any other register settings unclear as
>>> its FSM and there's nothing to get stuck.
>>>
>>> Also it goes thru small finite set of codes and by the time sensor
>>> programming happens for enabling streaming FSM will finish its
>>> calibration sequence much early and it will only wait for pads LP-11.
>>>
>>> So, during cancel we only need disable MIPI clock.
>>>
>> But there is no guarantee that cancel_calibration() couldn't be invoked
>> in the middle of the calibration process, hence FSM could freeze in an
>> intermediate state if it's running on the disabled MIPI clock, this
>> doesn't sound good.
> Actual calibration logic uses UART_FST_CAL clock which is always enabled

What enables the UART_FST_CAL clock? I don't see this clock used anywhere.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 16:57               ` Dmitry Osipenko
@ 2020-08-05 17:04                 ` Sowjanya Komatineni
  2020-08-05 17:23                   ` Dmitry Osipenko
  2020-08-05 17:29                   ` Sowjanya Komatineni
  0 siblings, 2 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 17:04 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>> calibration
>>>>>>>> is done.
>>>>>>>>
>>>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>>>> calibration and disables it only after waiting for done status from
>>>>>>>> the calibration logic.
>>>>>>>>
>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>> inline
>>>>>>>> with their usage.
>>>>>>>>
>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>> input
>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>> MIPI clock
>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>
>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>> tegra_dsi *dsi)
>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>         if (err < 0)
>>>>>>>>             return err;
>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>     }
>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>> unsigned long bclk,
>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>     }
>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>> *device)
>>>>>>>> +{
>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>> than to
>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>
>>>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>
>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>>>> it needs to be polled until it's unset?
>>>> Confirmed with HW design team during this patch update.
>>>>
>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>> that bit.
>>>>
>>>> Also, no need to have delay or do any other register settings unclear as
>>>> its FSM and there's nothing to get stuck.
>>>>
>>>> Also it goes thru small finite set of codes and by the time sensor
>>>> programming happens for enabling streaming FSM will finish its
>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>
>>>> So, during cancel we only need disable MIPI clock.
>>>>
>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>> in the middle of the calibration process, hence FSM could freeze in an
>>> intermediate state if it's running on the disabled MIPI clock, this
>>> doesn't sound good.
>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
> What enables the UART_FST_CAL clock? I don't see this clock used anywhere.

UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.

I don't see mipi driver handling this and I think that's because this 
clock is enabled all the time as its used for UART as well. Probably 
thierry can comment on this clock.

Also regarding cancel calibration, as FSM goes thru only finite sequence 
codes by the time csi stream and sensor stream happens which is where we 
check for calibration to complete for sure calibration will be finished 
and calibration logic will only wait for pads to be in LP-11 to apply 
results. So nothing special needed during cancel except to turn clock 
off to balance its usage count.



^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:04                 ` Sowjanya Komatineni
@ 2020-08-05 17:23                   ` Dmitry Osipenko
  2020-08-05 17:32                     ` Sowjanya Komatineni
  2020-08-05 17:29                   ` Sowjanya Komatineni
  1 sibling, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 17:23 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 20:04, Sowjanya Komatineni пишет:
> 
> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni
>>>>>>>> wrote:
>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>> calibration
>>>>>>>>> is done.
>>>>>>>>>
>>>>>>>>> So, this patch skips disabling MIPI clock after triggering
>>>>>>>>> start of
>>>>>>>>> calibration and disables it only after waiting for done status
>>>>>>>>> from
>>>>>>>>> the calibration logic.
>>>>>>>>>
>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>> inline
>>>>>>>>> with their usage.
>>>>>>>>>
>>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>>> input
>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>> MIPI clock
>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>         if (err < 0)
>>>>>>>>>             return err;
>>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>     }
>>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>> unsigned long bclk,
>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>     }
>>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>> *device)
>>>>>>>>> +{
>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and
>>>>>>>> MIPI_CAL_STATUS
>>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>> than to
>>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>>
>>>>>>> Perhaps having a fixed delay before disabling clock could be
>>>>>>> enough to
>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>
>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register.
>>>>>> Maybe
>>>>>> it needs to be polled until it's unset?
>>>>> Confirmed with HW design team during this patch update.
>>>>>
>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>> that bit.
>>>>>
>>>>> Also, no need to have delay or do any other register settings
>>>>> unclear as
>>>>> its FSM and there's nothing to get stuck.
>>>>>
>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>> programming happens for enabling streaming FSM will finish its
>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>
>>>>> So, during cancel we only need disable MIPI clock.
>>>>>
>>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>> doesn't sound good.
>>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
>> What enables the UART_FST_CAL clock? I don't see this clock used
>> anywhere.
> 
> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
> 
> I don't see mipi driver handling this and I think that's because this
> clock is enabled all the time as its used for UART as well. Probably
> thierry can comment on this clock.

It's not only the MIPI driver, the clock isn't defined at all neither in
the clk driver, nor in clk DT bindings.

It could be fragile to assume that it's always enabled.

> Also regarding cancel calibration, as FSM goes thru only finite sequence
> codes by the time csi stream and sensor stream happens which is where we
> check for calibration to complete for sure calibration will be finished
> and calibration logic will only wait for pads to be in LP-11 to apply
> results. So nothing special needed during cancel except to turn clock
> off to balance its usage count.

I guess it should be okay for the case of the VI driver, but
in general please don't assume that code can't change in the future. The
common API always should be made reliable for all possible situations.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:04                 ` Sowjanya Komatineni
  2020-08-05 17:23                   ` Dmitry Osipenko
@ 2020-08-05 17:29                   ` Sowjanya Komatineni
  2020-08-05 17:34                     ` Dmitry Osipenko
  1 sibling, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 17:29 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 10:04 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni 
>>>>>>>> wrote:
>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() 
>>>>>>>>> and
>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>> calibration
>>>>>>>>> is done.
>>>>>>>>>
>>>>>>>>> So, this patch skips disabling MIPI clock after triggering 
>>>>>>>>> start of
>>>>>>>>> calibration and disables it only after waiting for done status 
>>>>>>>>> from
>>>>>>>>> the calibration logic.
>>>>>>>>>
>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>> inline
>>>>>>>>> with their usage.
>>>>>>>>>
>>>>>>>>> As MIPI clock is left enabled and in case of any failures with 
>>>>>>>>> CSI
>>>>>>>>> input
>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>> MIPI clock
>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>         if (err < 0)
>>>>>>>>>             return err;
>>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>     }
>>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>> unsigned long bclk,
>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c 
>>>>>>>>> b/drivers/gpu/host1x/mipi.c
>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>     }
>>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>> *device)
>>>>>>>>> +{
>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and 
>>>>>>>> MIPI_CAL_STATUS
>>>>>>>> registers here? We don't clear the START bit in the former when 
>>>>>>>> the
>>>>>>>> calibration has successfully finished, but I suspect that's 
>>>>>>>> because
>>>>>>>> the bit is self-clearing. But I wonder if we still need to 
>>>>>>>> clear it
>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>> than to
>>>>>>> reset MIPI calibration block, but Sowjanya says this is 
>>>>>>> unnecessary.
>>>>>>>
>>>>>>> Perhaps having a fixed delay before disabling clock could be 
>>>>>>> enough to
>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>
>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. 
>>>>>> Maybe
>>>>>> it needs to be polled until it's unset?
>>>>> Confirmed with HW design team during this patch update.
>>>>>
>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>> that bit.
>>>>>
>>>>> Also, no need to have delay or do any other register settings 
>>>>> unclear as
>>>>> its FSM and there's nothing to get stuck.
>>>>>
>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>> programming happens for enabling streaming FSM will finish its
>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>
>>>>> So, during cancel we only need disable MIPI clock.
>>>>>
>>>> But there is no guarantee that cancel_calibration() couldn't be 
>>>> invoked
>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>> doesn't sound good.
>>> Actual calibration logic uses UART_FST_CAL clock which is always 
>>> enabled
>> What enables the UART_FST_CAL clock? I don't see this clock used 
>> anywhere.
>
> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
>
> I don't see mipi driver handling this and I think that's because this 
> clock is enabled all the time as its used for UART as well. Probably 
> thierry can comment on this clock.
>
> Also regarding cancel calibration, as FSM goes thru only finite 
> sequence codes by the time csi stream and sensor stream happens which 
> is where we check for calibration to complete for sure calibration 
> will be finished and calibration logic will only wait for pads to be 
> in LP-11 to apply results. So nothing special needed during cancel 
> except to turn clock off to balance its usage count.
>
>
UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM 
that goes thru sequence codes and when done waits for pads to be in 
LP-11 to apply results.

MIPI_CLK is controller gate clock which is also need to be kept enabled 
as incase if it sees LP-11 it updates registers so its recommended to 
have this clock enabled.

We can cancel_calibration() in CSI only when csi/sensor stream on fails 
and in which case there will be no LP-11 so we can unconditionally 
disable MIPI_CLK.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:23                   ` Dmitry Osipenko
@ 2020-08-05 17:32                     ` Sowjanya Komatineni
  0 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 17:32 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 10:23 AM, Dmitry Osipenko wrote:
> 05.08.2020 20:04, Sowjanya Komatineni пишет:
>> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni
>>>>>>>>> wrote:
>>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>>> calibration
>>>>>>>>>> is done.
>>>>>>>>>>
>>>>>>>>>> So, this patch skips disabling MIPI clock after triggering
>>>>>>>>>> start of
>>>>>>>>>> calibration and disables it only after waiting for done status
>>>>>>>>>> from
>>>>>>>>>> the calibration logic.
>>>>>>>>>>
>>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>>> inline
>>>>>>>>>> with their usage.
>>>>>>>>>>
>>>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>>>> input
>>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>>> MIPI clock
>>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>>      drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>>      include/linux/host1x.h      |  5 +++--
>>>>>>>>>>      3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>>              DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>>          tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>>      -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>>          if (err < 0)
>>>>>>>>>>              return err;
>>>>>>>>>>      -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>>      }
>>>>>>>>>>        static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>>> unsigned long bclk,
>>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>>      }
>>>>>>>>>>      EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>>      -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>>> *device)
>>>>>>>>>> +{
>>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and
>>>>>>>>> MIPI_CAL_STATUS
>>>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>>> than to
>>>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>>>
>>>>>>>> Perhaps having a fixed delay before disabling clock could be
>>>>>>>> enough to
>>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>>
>>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register.
>>>>>>> Maybe
>>>>>>> it needs to be polled until it's unset?
>>>>>> Confirmed with HW design team during this patch update.
>>>>>>
>>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>>> that bit.
>>>>>>
>>>>>> Also, no need to have delay or do any other register settings
>>>>>> unclear as
>>>>>> its FSM and there's nothing to get stuck.
>>>>>>
>>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>>> programming happens for enabling streaming FSM will finish its
>>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>>
>>>>>> So, during cancel we only need disable MIPI clock.
>>>>>>
>>>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>>> doesn't sound good.
>>>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
>>> What enables the UART_FST_CAL clock? I don't see this clock used
>>> anywhere.
>> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
>>
>> I don't see mipi driver handling this and I think that's because this
>> clock is enabled all the time as its used for UART as well. Probably
>> thierry can comment on this clock.
> It's not only the MIPI driver, the clock isn't defined at all neither in
> the clk driver, nor in clk DT bindings.
>
> It could be fragile to assume that it's always enabled.

 From SW clock architecture this clock is always kept programmed to 
68Mhz and enabled.

Its shared clock for UART and I see it enabled.

Thierry can comment more on where this clock is being handled? prior to 
kernel?

>
>> Also regarding cancel calibration, as FSM goes thru only finite sequence
>> codes by the time csi stream and sensor stream happens which is where we
>> check for calibration to complete for sure calibration will be finished
>> and calibration logic will only wait for pads to be in LP-11 to apply
>> results. So nothing special needed during cancel except to turn clock
>> off to balance its usage count.
> I guess it should be okay for the case of the VI driver, but
> in general please don't assume that code can't change in the future. The
> common API always should be made reliable for all possible situations.

Yes, There is no assumption here.

Just to be clear, UART_FST_MIPI_CAL is the clock used for calibration 
logic which is FSM that goes thru sequence codes and when done waits for 
pads to be in LP-11 to apply results.

MIPI_CLK is controller gate clock which is also need to be kept enabled 
as in case if it sees LP-11 it updates registers so its recommended to 
have this clock enabled during calibration process.

In case of CSI pads calibration, we call cancel_calibration() only when 
csi/sensor stream on fails and in which case there will be no LP-11 so 
we can unconditionally disable MIPI_CLK.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:29                   ` Sowjanya Komatineni
@ 2020-08-05 17:34                     ` Dmitry Osipenko
  2020-08-05 17:46                       ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-05 17:34 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

05.08.2020 20:29, Sowjanya Komatineni пишет:
...
> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
> that goes thru sequence codes and when done waits for pads to be in
> LP-11 to apply results.
> 
> MIPI_CLK is controller gate clock which is also need to be kept enabled
> as incase if it sees LP-11 it updates registers so its recommended to
> have this clock enabled.
> 
> We can cancel_calibration() in CSI only when csi/sensor stream on fails
> and in which case there will be no LP-11 so we can unconditionally
> disable MIPI_CLK.
> 

There is no guarantee that the fail comes before the LP-11. For example,
some odd camera driver may have a complicated enable sequence which may
fail after enabling the hardware streaming.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:34                     ` Dmitry Osipenko
@ 2020-08-05 17:46                       ` Sowjanya Komatineni
  2020-08-05 18:06                         ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 17:46 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
> 05.08.2020 20:29, Sowjanya Komatineni пишет:
> ...
>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>> that goes thru sequence codes and when done waits for pads to be in
>> LP-11 to apply results.
>>
>> MIPI_CLK is controller gate clock which is also need to be kept enabled
>> as incase if it sees LP-11 it updates registers so its recommended to
>> have this clock enabled.
>>
>> We can cancel_calibration() in CSI only when csi/sensor stream on fails
>> and in which case there will be no LP-11 so we can unconditionally
>> disable MIPI_CLK.
>>
> There is no guarantee that the fail comes before the LP-11. For example,
> some odd camera driver may have a complicated enable sequence which may
> fail after enabling the hardware streaming.

MIPI_CLK to keep enable is for calibration logic to update results, but 
like I said calibration logic uses UART_FST_MIPI_CAL clock. So even in 
case if fail happens from sensor after having pads in LP-11 then, 
calibration logic will still be running but result update will not 
happen with clock disabled. But HW will not stuck as this is confirmed 
from HW designer.




^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 17:46                       ` Sowjanya Komatineni
@ 2020-08-05 18:06                         ` Sowjanya Komatineni
  2020-08-06  0:47                           ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-05 18:06 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>> ...
>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>> that goes thru sequence codes and when done waits for pads to be in
>>> LP-11 to apply results.
>>>
>>> MIPI_CLK is controller gate clock which is also need to be kept enabled
>>> as incase if it sees LP-11 it updates registers so its recommended to
>>> have this clock enabled.
>>>
>>> We can cancel_calibration() in CSI only when csi/sensor stream on fails
>>> and in which case there will be no LP-11 so we can unconditionally
>>> disable MIPI_CLK.
>>>
>> There is no guarantee that the fail comes before the LP-11. For example,
>> some odd camera driver may have a complicated enable sequence which may
>> fail after enabling the hardware streaming.
>
> MIPI_CLK to keep enable is for calibration logic to update results, 
> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So 
> even in case if fail happens from sensor after having pads in LP-11 
> then, calibration logic will still be running but result update will 
> not happen with clock disabled. But HW will not stuck as this is 
> confirmed from HW designer.

If LP-11 happens from sensor stream (followed by fail) and by that time 
if calibration FSM is done and if calibration logic sees LP-11 then 
results will be applied to pads.

We did start of calibration before CSI stream so by the time we do 
sensor stream enable, calibration logic might have done with FSM and 
waiting for LP-11

Also if we see any special case, we always can use finish_calibration() 
instead of cancel_calibration() as well.

finish_calibration() has extra 250ms wait time polling done bit and we 
can ignore its return code during fail pathway.

>
>
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-05 18:06                         ` Sowjanya Komatineni
@ 2020-08-06  0:47                           ` Sowjanya Komatineni
  2020-08-06 13:32                             ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06  0:47 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>
>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>> ...
>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>> that goes thru sequence codes and when done waits for pads to be in
>>>> LP-11 to apply results.
>>>>
>>>> MIPI_CLK is controller gate clock which is also need to be kept 
>>>> enabled
>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>> have this clock enabled.
>>>>
>>>> We can cancel_calibration() in CSI only when csi/sensor stream on 
>>>> fails
>>>> and in which case there will be no LP-11 so we can unconditionally
>>>> disable MIPI_CLK.
>>>>
>>> There is no guarantee that the fail comes before the LP-11. For 
>>> example,
>>> some odd camera driver may have a complicated enable sequence which may
>>> fail after enabling the hardware streaming.
>>
>> MIPI_CLK to keep enable is for calibration logic to update results, 
>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So 
>> even in case if fail happens from sensor after having pads in LP-11 
>> then, calibration logic will still be running but result update will 
>> not happen with clock disabled. But HW will not stuck as this is 
>> confirmed from HW designer.
>
> If LP-11 happens from sensor stream (followed by fail) and by that 
> time if calibration FSM is done and if calibration logic sees LP-11 
> then results will be applied to pads.
>
> We did start of calibration before CSI stream so by the time we do 
> sensor stream enable, calibration logic might have done with FSM and 
> waiting for LP-11
>
> Also if we see any special case, we always can use 
> finish_calibration() instead of cancel_calibration() as well.
>
> finish_calibration() has extra 250ms wait time polling done bit and we 
> can ignore its return code during fail pathway.
>
Confirmed from HW designer, calibration FSM to finish takes worst case 
72uS so by the time it gets to sensor stream it will be done its 
sequence and will be waiting for DONE bit.

So disabling MIPI CAL clock on sensor stream fails is safe.

>>
>>
>>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06  0:47                           ` Sowjanya Komatineni
@ 2020-08-06 13:32                             ` Dmitry Osipenko
  2020-08-06 15:59                               ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 13:32 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 03:47, Sowjanya Komatineni пишет:
> 
> On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>>
>> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>>
>>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>>> ...
>>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>>> that goes thru sequence codes and when done waits for pads to be in
>>>>> LP-11 to apply results.
>>>>>
>>>>> MIPI_CLK is controller gate clock which is also need to be kept
>>>>> enabled
>>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>>> have this clock enabled.
>>>>>
>>>>> We can cancel_calibration() in CSI only when csi/sensor stream on
>>>>> fails
>>>>> and in which case there will be no LP-11 so we can unconditionally
>>>>> disable MIPI_CLK.
>>>>>
>>>> There is no guarantee that the fail comes before the LP-11. For
>>>> example,
>>>> some odd camera driver may have a complicated enable sequence which may
>>>> fail after enabling the hardware streaming.
>>>
>>> MIPI_CLK to keep enable is for calibration logic to update results,
>>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So
>>> even in case if fail happens from sensor after having pads in LP-11
>>> then, calibration logic will still be running but result update will
>>> not happen with clock disabled. But HW will not stuck as this is
>>> confirmed from HW designer.
>>
>> If LP-11 happens from sensor stream (followed by fail) and by that
>> time if calibration FSM is done and if calibration logic sees LP-11
>> then results will be applied to pads.
>>
>> We did start of calibration before CSI stream so by the time we do
>> sensor stream enable, calibration logic might have done with FSM and
>> waiting for LP-11
>>
>> Also if we see any special case, we always can use
>> finish_calibration() instead of cancel_calibration() as well.

Why not to do it right now?

Then the code could look like this:

src_subdev = tegra_channel_get_remote_source_subdev(chan);
ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
err = tegra_mipi_finish_calibration(csi_chan->mipi);

if (ret < 0 && ret != -ENOIOCTLCMD)
	goto err_disable_csi_stream;

if (err < 0)
	dev_warn(csi_chan->csi->dev,
		 "MIPI calibration failed: %d\n", err);

>> finish_calibration() has extra 250ms wait time polling done bit and we
>> can ignore its return code during fail pathway.
>>
> Confirmed from HW designer, calibration FSM to finish takes worst case
> 72uS so by the time it gets to sensor stream it will be done its
> sequence and will be waiting for DONE bit.
> 
> So disabling MIPI CAL clock on sensor stream fails is safe.


72us is quite a lot of time, what will happen if LP-11 happens before
FSM finished calibration?

Maybe the finish_calibration() needs to split into two parts:

 1. wait for CAL_STATUS_ACTIVE before enabling sensor
 2. wait for CAL_STATUS_DONE after enabling sensor

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 13:32                             ` Dmitry Osipenko
@ 2020-08-06 15:59                               ` Sowjanya Komatineni
  2020-08-06 16:10                                 ` Dmitry Osipenko
  2020-08-06 16:13                                 ` Dmitry Osipenko
  0 siblings, 2 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 15:59 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 6:32 AM, Dmitry Osipenko wrote:
> 06.08.2020 03:47, Sowjanya Komatineni пишет:
>> On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>>> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>>>> that goes thru sequence codes and when done waits for pads to be in
>>>>>> LP-11 to apply results.
>>>>>>
>>>>>> MIPI_CLK is controller gate clock which is also need to be kept
>>>>>> enabled
>>>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>>>> have this clock enabled.
>>>>>>
>>>>>> We can cancel_calibration() in CSI only when csi/sensor stream on
>>>>>> fails
>>>>>> and in which case there will be no LP-11 so we can unconditionally
>>>>>> disable MIPI_CLK.
>>>>>>
>>>>> There is no guarantee that the fail comes before the LP-11. For
>>>>> example,
>>>>> some odd camera driver may have a complicated enable sequence which may
>>>>> fail after enabling the hardware streaming.
>>>> MIPI_CLK to keep enable is for calibration logic to update results,
>>>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So
>>>> even in case if fail happens from sensor after having pads in LP-11
>>>> then, calibration logic will still be running but result update will
>>>> not happen with clock disabled. But HW will not stuck as this is
>>>> confirmed from HW designer.
>>> If LP-11 happens from sensor stream (followed by fail) and by that
>>> time if calibration FSM is done and if calibration logic sees LP-11
>>> then results will be applied to pads.
>>>
>>> We did start of calibration before CSI stream so by the time we do
>>> sensor stream enable, calibration logic might have done with FSM and
>>> waiting for LP-11
>>>
>>> Also if we see any special case, we always can use
>>> finish_calibration() instead of cancel_calibration() as well.
> Why not to do it right now?

> Then the code could look like this:
>
> src_subdev = tegra_channel_get_remote_source_subdev(chan);
> ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> err = tegra_mipi_finish_calibration(csi_chan->mipi);
>
> if (ret < 0 && ret != -ENOIOCTLCMD)
> 	goto err_disable_csi_stream;
>
> if (err < 0)
> 	dev_warn(csi_chan->csi->dev,
> 		 "MIPI calibration failed: %d\n", err);
>
>>> finish_calibration() has extra 250ms wait time polling done bit and we
>>> can ignore its return code during fail pathway.
>>>
>> Confirmed from HW designer, calibration FSM to finish takes worst case
>> 72uS so by the time it gets to sensor stream it will be done its
>> sequence and will be waiting for DONE bit.
>>
>> So disabling MIPI CAL clock on sensor stream fails is safe.
>
> 72us is quite a lot of time, what will happen if LP-11 happens before
> FSM finished calibration?
>
> Maybe the finish_calibration() needs to split into two parts:
>
>   1. wait for CAL_STATUS_ACTIVE before enabling sensor
>   2. wait for CAL_STATUS_DONE after enabling sensor

I don't think we need to split for active and done. Active will be 1 as 
long as other pads are in calibration as well.

We cant use active status check for specific pads under calibration. 
This is common bit for all pads.

Unfortunately HW don't have separate status indicating when sequence is 
done to indicate its waiting for LP11.


To avoid all this, will remove cancel_calibration() totally and use same 
finish calibration even in case of stream failure then.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 15:59                               ` Sowjanya Komatineni
@ 2020-08-06 16:10                                 ` Dmitry Osipenko
  2020-08-06 16:41                                   ` Sowjanya Komatineni
  2020-08-06 16:13                                 ` Dmitry Osipenko
  1 sibling, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 16:10 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 18:59, Sowjanya Komatineni пишет:
...
>>> Confirmed from HW designer, calibration FSM to finish takes worst case
>>> 72uS so by the time it gets to sensor stream it will be done its
>>> sequence and will be waiting for DONE bit.
>>>
>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>
>> 72us is quite a lot of time, what will happen if LP-11 happens before
>> FSM finished calibration?
>>
>> Maybe the finish_calibration() needs to split into two parts:
>>
>>   1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>   2. wait for CAL_STATUS_DONE after enabling sensor
> 
> I don't think we need to split for active and done. Active will be 1 as
> long as other pads are in calibration as well.
> 
> We cant use active status check for specific pads under calibration.
> This is common bit for all pads.

Does hardware have a single FSM block shared by all pads or there is FSM
per group of pads?

> Unfortunately HW don't have separate status indicating when sequence is
> done to indicate its waiting for LP11.
> 
> 
> To avoid all this, will remove cancel_calibration() totally and use same
> finish calibration even in case of stream failure then.
> 

What about to add 72us delay to the end of start_calibration() in order
to ensure that FSM is finished before LP-11?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 15:59                               ` Sowjanya Komatineni
  2020-08-06 16:10                                 ` Dmitry Osipenko
@ 2020-08-06 16:13                                 ` Dmitry Osipenko
  2020-08-06 16:37                                   ` Dmitry Osipenko
  1 sibling, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 16:13 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 18:59, Sowjanya Komatineni пишет:
..
> We cant use active status check for specific pads under calibration.
> This is common bit for all pads.

I'm not sure why this is a problem.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:13                                 ` Dmitry Osipenko
@ 2020-08-06 16:37                                   ` Dmitry Osipenko
  2020-08-06 16:42                                     ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 16:37 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 19:13, Dmitry Osipenko пишет:
> 06.08.2020 18:59, Sowjanya Komatineni пишет:
> ..
>> We cant use active status check for specific pads under calibration.
>> This is common bit for all pads.
> 
> I'm not sure why this is a problem.
> 

IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:10                                 ` Dmitry Osipenko
@ 2020-08-06 16:41                                   ` Sowjanya Komatineni
  2020-08-06 16:45                                     ` Dmitry Osipenko
  2020-08-06 17:12                                     ` Sowjanya Komatineni
  0 siblings, 2 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 16:41 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
> 06.08.2020 18:59, Sowjanya Komatineni пишет:
> ...
>>>> Confirmed from HW designer, calibration FSM to finish takes worst case
>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>> sequence and will be waiting for DONE bit.
>>>>
>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>> 72us is quite a lot of time, what will happen if LP-11 happens before
>>> FSM finished calibration?
>>>
>>> Maybe the finish_calibration() needs to split into two parts:
>>>
>>>    1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>    2. wait for CAL_STATUS_DONE after enabling sensor
>> I don't think we need to split for active and done. Active will be 1 as
>> long as other pads are in calibration as well.
>>
>> We cant use active status check for specific pads under calibration.
>> This is common bit for all pads.
> Does hardware have a single FSM block shared by all pads or there is FSM
> per group of pads?

MIPI CAL status register has DONE bits for individual pads status and 
single ACTIVE bit.

ACTIVE bit set to 1 indicates auto calibration is active which is the 
case even when other pads (other CSI pads from other ports streaming in 
case of parallel stream) are under calibration. Also DSI is shared as well.

We do calibration for individual pads. So, we should not rely on ACTIVE bit.


MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the 
beginning.

But I think this also should be fixed as in case of parallel streams 
calibration can happen in parallel waiting for ACTIVE to be cleared 
makes all calibration callers to wait for longer than needed as ACTIVE 
is common for all pads.

>
>> Unfortunately HW don't have separate status indicating when sequence is
>> done to indicate its waiting for LP11.
>>
>>
>> To avoid all this, will remove cancel_calibration() totally and use same
>> finish calibration even in case of stream failure then.
>>
> What about to add 72us delay to the end of start_calibration() in order
> to ensure that FSM is finished before LP-11?

Why we should add 72uS in start_calibration() when can use same 
finish_calibration() for both pass/fail cases?

Only timing loose we see is in case of failure we still wait for 250ms 
and as this is failing case I hope should be ok.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:37                                   ` Dmitry Osipenko
@ 2020-08-06 16:42                                     ` Sowjanya Komatineni
  2020-08-06 16:43                                       ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 16:42 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 9:37 AM, Dmitry Osipenko wrote:
> 06.08.2020 19:13, Dmitry Osipenko пишет:
>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>> ..
>>> We cant use active status check for specific pads under calibration.
>>> This is common bit for all pads.
>> I'm not sure why this is a problem.
>>
> IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
> and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.

As soon as START bit it set, FSM will set ACTIVE = 1

There is no added advantage of waiting for ACTIVE to be in 
start_calibration()


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:42                                     ` Sowjanya Komatineni
@ 2020-08-06 16:43                                       ` Sowjanya Komatineni
  0 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 16:43 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 9:42 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 9:37 AM, Dmitry Osipenko wrote:
>> 06.08.2020 19:13, Dmitry Osipenko пишет:
>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>> ..
>>>> We cant use active status check for specific pads under calibration.
>>>> This is common bit for all pads.
>>> I'm not sure why this is a problem.
>>>
>> IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
>> and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.
>
> As soon as START bit it set, FSM will set ACTIVE = 1
>
> There is no added advantage of waiting for ACTIVE to be in 
> start_calibration()
Also like I explained in other post of same discussion, ACTIVE we will 
be 1 even when other parallel pads are under calibration.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:41                                   ` Sowjanya Komatineni
@ 2020-08-06 16:45                                     ` Dmitry Osipenko
  2020-08-06 16:51                                       ` Sowjanya Komatineni
  2020-08-06 17:12                                     ` Sowjanya Komatineni
  1 sibling, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 16:45 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 19:41, Sowjanya Komatineni пишет:
...
>> What about to add 72us delay to the end of start_calibration() in order
>> to ensure that FSM is finished before LP-11?
> 
> Why we should add 72uS in start_calibration() when can use same
> finish_calibration() for both pass/fail cases?
> 
> Only timing loose we see is in case of failure we still wait for 250ms
> and as this is failing case I hope should be ok.
> 

You said that calibration settings are applied to pads on LP-11, but if
LP-11 happens before FSM is finished, then what values will be applied
if any?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:45                                     ` Dmitry Osipenko
@ 2020-08-06 16:51                                       ` Sowjanya Komatineni
  2020-08-06 17:15                                         ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 16:51 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 9:45 AM, Dmitry Osipenko wrote:
> 06.08.2020 19:41, Sowjanya Komatineni пишет:
> ...
>>> What about to add 72us delay to the end of start_calibration() in order
>>> to ensure that FSM is finished before LP-11?
>> Why we should add 72uS in start_calibration() when can use same
>> finish_calibration() for both pass/fail cases?
>>
>> Only timing loose we see is in case of failure we still wait for 250ms
>> and as this is failing case I hope should be ok.
>>
> You said that calibration settings are applied to pads on LP-11, but if
> LP-11 happens before FSM is finished, then what values will be applied
> if any?

No calibration logic will check for LP-11 only after finishing 
calibration sequence codes.

After that if it sees LP-11, it will apply results to pads and DONE bit 
will then be set to 1 indication pad results update.

Unfortunately like I said we don't have status indication for 
calibrating finished before waiting for LP-11.

ACTIVE bit is common for all PADS. If multiple 6 streams are happening 
in parallel, ACTIVE will be 1 as long as its calibrating any of the pads 
and its not for individual pads.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:41                                   ` Sowjanya Komatineni
  2020-08-06 16:45                                     ` Dmitry Osipenko
@ 2020-08-06 17:12                                     ` Sowjanya Komatineni
  2020-08-06 17:27                                       ` Dmitry Osipenko
  1 sibling, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 17:12 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 9:41 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>> ...
>>>>> Confirmed from HW designer, calibration FSM to finish takes worst 
>>>>> case
>>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>>> sequence and will be waiting for DONE bit.
>>>>>
>>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>>> 72us is quite a lot of time, what will happen if LP-11 happens before
>>>> FSM finished calibration?
>>>>
>>>> Maybe the finish_calibration() needs to split into two parts:
>>>>
>>>>    1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>>    2. wait for CAL_STATUS_DONE after enabling sensor
>>> I don't think we need to split for active and done. Active will be 1 as
>>> long as other pads are in calibration as well.
>>>
>>> We cant use active status check for specific pads under calibration.
>>> This is common bit for all pads.
>> Does hardware have a single FSM block shared by all pads or there is FSM
>> per group of pads?
>
> MIPI CAL status register has DONE bits for individual pads status and 
> single ACTIVE bit.
>
> ACTIVE bit set to 1 indicates auto calibration is active which is the 
> case even when other pads (other CSI pads from other ports streaming 
> in case of parallel stream) are under calibration. Also DSI is shared 
> as well.
>
> We do calibration for individual pads. So, we should not rely on 
> ACTIVE bit.
>
>
> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the 
> beginning.
>
> But I think this also should be fixed as in case of parallel streams 
> calibration can happen in parallel waiting for ACTIVE to be cleared 
> makes all calibration callers to wait for longer than needed as ACTIVE 
> is common for all pads.
>
>>
>>> Unfortunately HW don't have separate status indicating when sequence is
>>> done to indicate its waiting for LP11.
>>>
>>>
>>> To avoid all this, will remove cancel_calibration() totally and use 
>>> same
>>> finish calibration even in case of stream failure then.
>>>
>> What about to add 72us delay to the end of start_calibration() in order
>> to ensure that FSM is finished before LP-11?
>
> Why we should add 72uS in start_calibration() when can use same 
> finish_calibration() for both pass/fail cases?
>
> Only timing loose we see is in case of failure we still wait for 250ms 
> and as this is failing case I hope should be ok.
>
Also as we don't need cancel_calibration(), keeping tegra_mipi_wait() 
like earlier makes sense I believe as we are letting it finish going 
thru sequence.

So I think below are fixes,

1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit 
to be 0 and use only DONE bit to be 1 for wait condition  as we are 
calibrating separately for individual pads and this ACTIVE bit is common 
for all pads where it will not be 0 in case of other parallel streams 
which may also be under calibration.

2. No need for separate cancel_calibration. So, probably earlier names 
tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are waiting 
for calibration sequence to finish irrespective of fail/pass.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 16:51                                       ` Sowjanya Komatineni
@ 2020-08-06 17:15                                         ` Dmitry Osipenko
  0 siblings, 0 replies; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 17:15 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 19:51, Sowjanya Komatineni пишет:
>>>> What about to add 72us delay to the end of start_calibration() in order
>>>> to ensure that FSM is finished before LP-11?
>>> Why we should add 72uS in start_calibration() when can use same
>>> finish_calibration() for both pass/fail cases?
>>>
>>> Only timing loose we see is in case of failure we still wait for 250ms
>>> and as this is failing case I hope should be ok.
>>>
>> You said that calibration settings are applied to pads on LP-11, but if
>> LP-11 happens before FSM is finished, then what values will be applied
>> if any?
> 
> No calibration logic will check for LP-11 only after finishing
> calibration sequence codes.
> 
> After that if it sees LP-11, it will apply results to pads and DONE bit
> will then be set to 1 indication pad results update.

Are you sure that HW doesn't use level-triggered logic for LP-11 signal?

> Unfortunately like I said we don't have status indication for
> calibrating finished before waiting for LP-11.

This is not a problem if hardware can cope with LP-11 happened at the
time of calibration. If hardware can't cope with that, then somethings
needs to be done about it.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 17:12                                     ` Sowjanya Komatineni
@ 2020-08-06 17:27                                       ` Dmitry Osipenko
  2020-08-06 17:44                                         ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 17:27 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 20:12, Sowjanya Komatineni пишет:
> 
> On 8/6/20 9:41 AM, Sowjanya Komatineni wrote:
>>
>> On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>> ...
>>>>>> Confirmed from HW designer, calibration FSM to finish takes worst
>>>>>> case
>>>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>>>> sequence and will be waiting for DONE bit.
>>>>>>
>>>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>>>> 72us is quite a lot of time, what will happen if LP-11 happens before
>>>>> FSM finished calibration?
>>>>>
>>>>> Maybe the finish_calibration() needs to split into two parts:
>>>>>
>>>>>    1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>>>    2. wait for CAL_STATUS_DONE after enabling sensor
>>>> I don't think we need to split for active and done. Active will be 1 as
>>>> long as other pads are in calibration as well.
>>>>
>>>> We cant use active status check for specific pads under calibration.
>>>> This is common bit for all pads.
>>> Does hardware have a single FSM block shared by all pads or there is FSM
>>> per group of pads?
>>
>> MIPI CAL status register has DONE bits for individual pads status and
>> single ACTIVE bit.
>>
>> ACTIVE bit set to 1 indicates auto calibration is active which is the
>> case even when other pads (other CSI pads from other ports streaming
>> in case of parallel stream) are under calibration. Also DSI is shared
>> as well.
>>
>> We do calibration for individual pads. So, we should not rely on
>> ACTIVE bit.
>>
>>
>> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the
>> beginning.
>>
>> But I think this also should be fixed as in case of parallel streams
>> calibration can happen in parallel waiting for ACTIVE to be cleared
>> makes all calibration callers to wait for longer than needed as ACTIVE
>> is common for all pads.
>>
>>>
>>>> Unfortunately HW don't have separate status indicating when sequence is
>>>> done to indicate its waiting for LP11.
>>>>
>>>>
>>>> To avoid all this, will remove cancel_calibration() totally and use
>>>> same
>>>> finish calibration even in case of stream failure then.
>>>>
>>> What about to add 72us delay to the end of start_calibration() in order
>>> to ensure that FSM is finished before LP-11?
>>
>> Why we should add 72uS in start_calibration() when can use same
>> finish_calibration() for both pass/fail cases?
>>
>> Only timing loose we see is in case of failure we still wait for 250ms
>> and as this is failing case I hope should be ok.
>>
> Also as we don't need cancel_calibration(), keeping tegra_mipi_wait()
> like earlier makes sense I believe as we are letting it finish going
> thru sequence.
> 
> So I think below are fixes,
> 
> 1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit
> to be 0 and use only DONE bit to be 1 for wait condition  as we are
> calibrating separately for individual pads and this ACTIVE bit is common
> for all pads where it will not be 0 in case of other parallel streams
> which may also be under calibration.

Yes, looks like it's a mistake of the current MIPI driver that it polls
the ACTIVE bit.

> 2. No need for separate cancel_calibration. So, probably earlier names
> tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are waiting
> for calibration sequence to finish irrespective of fail/pass.

The new names reflect better what those functions actually do, IMO.

What about to make finish_calibration() to take an additional argument
which corresponds to the awaited HW bits? For example if it's CSIA, then
it could be:

  tegra_mipi_finish_calibration(csi_chan->mipi, MIPI_CAL_CSIA);


Also, is it okay that DSI and CSI could change MIPI_CAL_CTRL after DSI
or CSI already started calibration?

Looking at the current start_calibration(), I think the mutex should be
kept locked and then finish_calibration() should unlock it.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 17:27                                       ` Dmitry Osipenko
@ 2020-08-06 17:44                                         ` Sowjanya Komatineni
  2020-08-06 17:52                                           ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 17:44 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 10:27 AM, Dmitry Osipenko wrote:
> 06.08.2020 20:12, Sowjanya Komatineni пишет:
>> On 8/6/20 9:41 AM, Sowjanya Komatineni wrote:
>>> On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
>>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> Confirmed from HW designer, calibration FSM to finish takes worst
>>>>>>> case
>>>>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>>>>> sequence and will be waiting for DONE bit.
>>>>>>>
>>>>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>>>>> 72us is quite a lot of time, what will happen if LP-11 happens before
>>>>>> FSM finished calibration?
>>>>>>
>>>>>> Maybe the finish_calibration() needs to split into two parts:
>>>>>>
>>>>>>     1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>>>>     2. wait for CAL_STATUS_DONE after enabling sensor
>>>>> I don't think we need to split for active and done. Active will be 1 as
>>>>> long as other pads are in calibration as well.
>>>>>
>>>>> We cant use active status check for specific pads under calibration.
>>>>> This is common bit for all pads.
>>>> Does hardware have a single FSM block shared by all pads or there is FSM
>>>> per group of pads?
>>> MIPI CAL status register has DONE bits for individual pads status and
>>> single ACTIVE bit.
>>>
>>> ACTIVE bit set to 1 indicates auto calibration is active which is the
>>> case even when other pads (other CSI pads from other ports streaming
>>> in case of parallel stream) are under calibration. Also DSI is shared
>>> as well.
>>>
>>> We do calibration for individual pads. So, we should not rely on
>>> ACTIVE bit.
>>>
>>>
>>> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the
>>> beginning.
>>>
>>> But I think this also should be fixed as in case of parallel streams
>>> calibration can happen in parallel waiting for ACTIVE to be cleared
>>> makes all calibration callers to wait for longer than needed as ACTIVE
>>> is common for all pads.
>>>
>>>>> Unfortunately HW don't have separate status indicating when sequence is
>>>>> done to indicate its waiting for LP11.
>>>>>
>>>>>
>>>>> To avoid all this, will remove cancel_calibration() totally and use
>>>>> same
>>>>> finish calibration even in case of stream failure then.
>>>>>
>>>> What about to add 72us delay to the end of start_calibration() in order
>>>> to ensure that FSM is finished before LP-11?
>>> Why we should add 72uS in start_calibration() when can use same
>>> finish_calibration() for both pass/fail cases?
>>>
>>> Only timing loose we see is in case of failure we still wait for 250ms
>>> and as this is failing case I hope should be ok.
>>>
>> Also as we don't need cancel_calibration(), keeping tegra_mipi_wait()
>> like earlier makes sense I believe as we are letting it finish going
>> thru sequence.
>>
>> So I think below are fixes,
>>
>> 1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit
>> to be 0 and use only DONE bit to be 1 for wait condition  as we are
>> calibrating separately for individual pads and this ACTIVE bit is common
>> for all pads where it will not be 0 in case of other parallel streams
>> which may also be under calibration.
> Yes, looks like it's a mistake of the current MIPI driver that it polls
> the ACTIVE bit.
>
>> 2. No need for separate cancel_calibration. So, probably earlier names
>> tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are waiting
>> for calibration sequence to finish irrespective of fail/pass.
> The new names reflect better what those functions actually do, IMO.
ok Will keep same names.
>
> What about to make finish_calibration() to take an additional argument
> which corresponds to the awaited HW bits? For example if it's CSIA, then
> it could be:
>
>    tegra_mipi_finish_calibration(csi_chan->mipi, MIPI_CAL_CSIA);
MIPI device is separate for each stream so waiting for only those 
corresponding DONE bits happen currently and no need to pass argument.
>
>
> Also, is it okay that DSI and CSI could change MIPI_CAL_CTRL after DSI
> or CSI already started calibration?
>
> Looking at the current start_calibration(), I think the mutex should be
> kept locked and then finish_calibration() should unlock it.

Confirmed with HW designer.

ACTIVE is common bit for all pads where we see it 1 as long as all pads 
(DSI + all CSI Pads) are under calibration.

While MIPI CAL is doing calibration for certain pads, before issuing 
other start it has to wait for ACTIVE to be 0.


Earlier driver (before split) checks for ACTIVE to be 0 along with DONE 
bit to be 1 as it does both calibrate and wait in same API.

With the split, looks like we need below sequence to be safe.

1. tegra_mipi_start_calibration(): wait for ACTIVE to be 0 before 
issuing START and after issuing start wait for 72uS to let calibration 
code sequence finish so it will be ready to see LP-11 after that.

In case of parallel streams, call to start_calibration can happen when 
pads of other stream are under calibration.

2. tegra_mipi_finish_calibration(): check for DONE bit to be 1




^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 17:44                                         ` Sowjanya Komatineni
@ 2020-08-06 17:52                                           ` Sowjanya Komatineni
  2020-08-06 18:01                                             ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 17:52 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 10:44 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 10:27 AM, Dmitry Osipenko wrote:
>> 06.08.2020 20:12, Sowjanya Komatineni пишет:
>>> On 8/6/20 9:41 AM, Sowjanya Komatineni wrote:
>>>> On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
>>>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> Confirmed from HW designer, calibration FSM to finish takes worst
>>>>>>>> case
>>>>>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>>>>>> sequence and will be waiting for DONE bit.
>>>>>>>>
>>>>>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>>>>>> 72us is quite a lot of time, what will happen if LP-11 happens 
>>>>>>> before
>>>>>>> FSM finished calibration?
>>>>>>>
>>>>>>> Maybe the finish_calibration() needs to split into two parts:
>>>>>>>
>>>>>>>     1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>>>>>     2. wait for CAL_STATUS_DONE after enabling sensor
>>>>>> I don't think we need to split for active and done. Active will 
>>>>>> be 1 as
>>>>>> long as other pads are in calibration as well.
>>>>>>
>>>>>> We cant use active status check for specific pads under calibration.
>>>>>> This is common bit for all pads.
>>>>> Does hardware have a single FSM block shared by all pads or there 
>>>>> is FSM
>>>>> per group of pads?
>>>> MIPI CAL status register has DONE bits for individual pads status and
>>>> single ACTIVE bit.
>>>>
>>>> ACTIVE bit set to 1 indicates auto calibration is active which is the
>>>> case even when other pads (other CSI pads from other ports streaming
>>>> in case of parallel stream) are under calibration. Also DSI is shared
>>>> as well.
>>>>
>>>> We do calibration for individual pads. So, we should not rely on
>>>> ACTIVE bit.
>>>>
>>>>
>>>> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the
>>>> beginning.
>>>>
>>>> But I think this also should be fixed as in case of parallel streams
>>>> calibration can happen in parallel waiting for ACTIVE to be cleared
>>>> makes all calibration callers to wait for longer than needed as ACTIVE
>>>> is common for all pads.
>>>>
>>>>>> Unfortunately HW don't have separate status indicating when 
>>>>>> sequence is
>>>>>> done to indicate its waiting for LP11.
>>>>>>
>>>>>>
>>>>>> To avoid all this, will remove cancel_calibration() totally and use
>>>>>> same
>>>>>> finish calibration even in case of stream failure then.
>>>>>>
>>>>> What about to add 72us delay to the end of start_calibration() in 
>>>>> order
>>>>> to ensure that FSM is finished before LP-11?
>>>> Why we should add 72uS in start_calibration() when can use same
>>>> finish_calibration() for both pass/fail cases?
>>>>
>>>> Only timing loose we see is in case of failure we still wait for 250ms
>>>> and as this is failing case I hope should be ok.
>>>>
>>> Also as we don't need cancel_calibration(), keeping tegra_mipi_wait()
>>> like earlier makes sense I believe as we are letting it finish going
>>> thru sequence.
>>>
>>> So I think below are fixes,
>>>
>>> 1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit
>>> to be 0 and use only DONE bit to be 1 for wait condition  as we are
>>> calibrating separately for individual pads and this ACTIVE bit is 
>>> common
>>> for all pads where it will not be 0 in case of other parallel streams
>>> which may also be under calibration.
>> Yes, looks like it's a mistake of the current MIPI driver that it polls
>> the ACTIVE bit.
>>
>>> 2. No need for separate cancel_calibration. So, probably earlier names
>>> tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are 
>>> waiting
>>> for calibration sequence to finish irrespective of fail/pass.
>> The new names reflect better what those functions actually do, IMO.
> ok Will keep same names.
>>
>> What about to make finish_calibration() to take an additional argument
>> which corresponds to the awaited HW bits? For example if it's CSIA, then
>> it could be:
>>
>>    tegra_mipi_finish_calibration(csi_chan->mipi, MIPI_CAL_CSIA);
> MIPI device is separate for each stream so waiting for only those 
> corresponding DONE bits happen currently and no need to pass argument.
>>
>>
>> Also, is it okay that DSI and CSI could change MIPI_CAL_CTRL after DSI
>> or CSI already started calibration?
>>
>> Looking at the current start_calibration(), I think the mutex should be
>> kept locked and then finish_calibration() should unlock it.

Right mutex_unlock should happen at end of finish_calibration.

With keeping mutex locked in start, we dont have to check for active to 
be 0 to issue start as mutex will keep it locked and other pads 
calibration can only go thru when current one is done.

So instead of below sequence, its simpler to do this way?

start_calibration()

- mutex_lock

- wait for 72uS after start

finish_calibration()

- keep check for ACTIVE = 0 and DONE = 1

- mutex_unlock()

>
> Confirmed with HW designer.
>
> ACTIVE is common bit for all pads where we see it 1 as long as all 
> pads (DSI + all CSI Pads) are under calibration.
>
> While MIPI CAL is doing calibration for certain pads, before issuing 
> other start it has to wait for ACTIVE to be 0.
>
>
> Earlier driver (before split) checks for ACTIVE to be 0 along with 
> DONE bit to be 1 as it does both calibrate and wait in same API.
>
> With the split, looks like we need below sequence to be safe.
>
> 1. tegra_mipi_start_calibration(): wait for ACTIVE to be 0 before 
> issuing START and after issuing start wait for 72uS to let calibration 
> code sequence finish so it will be ready to see LP-11 after that.
>
> In case of parallel streams, call to start_calibration can happen when 
> pads of other stream are under calibration.
>
> 2. tegra_mipi_finish_calibration(): check for DONE bit to be 1
>
>
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 17:52                                           ` Sowjanya Komatineni
@ 2020-08-06 18:01                                             ` Dmitry Osipenko
  2020-08-06 18:07                                               ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 18:01 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 20:52, Sowjanya Komatineni пишет:
...
> Right mutex_unlock should happen at end of finish_calibration.
> 
> With keeping mutex locked in start, we dont have to check for active to
> be 0 to issue start as mutex will keep it locked and other pads
> calibration can only go thru when current one is done.
> 
> So instead of below sequence, its simpler to do this way?
> 
> start_calibration()
> 
> - mutex_lock
> 
> - wait for 72uS after start
> 
> finish_calibration()
> 
> - keep check for ACTIVE = 0 and DONE = 1

I think only the DONE bits which correspond to the mipi_device->pads
bitmask should be awaited.

> - mutex_unlock()

Perhaps the start_calibration() also needs to be changed to not touch
the MIPI_CAL_CONFIG bits of the unrelated pads?

Otherwise sounds good to me.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 18:01                                             ` Dmitry Osipenko
@ 2020-08-06 18:07                                               ` Sowjanya Komatineni
  2020-08-06 18:18                                                 ` Dmitry Osipenko
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 18:07 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
> 06.08.2020 20:52, Sowjanya Komatineni пишет:
> ...
>> Right mutex_unlock should happen at end of finish_calibration.
>>
>> With keeping mutex locked in start, we dont have to check for active to
>> be 0 to issue start as mutex will keep it locked and other pads
>> calibration can only go thru when current one is done.
>>
>> So instead of below sequence, its simpler to do this way?
>>
>> start_calibration()
>>
>> - mutex_lock
>>
>> - wait for 72uS after start
>>
>> finish_calibration()
>>
>> - keep check for ACTIVE = 0 and DONE = 1
> I think only the DONE bits which correspond to the mipi_device->pads
> bitmask should be awaited.

As next START can't be triggered when auto cal is ACTIVE, we should keep 
this in finish.

As we do mutex_unlock only at end of finish, other pads calibrations 
dont go thru till the one in process is finished.

So in this case ACTIVE applies to current selected pads that are under 
calibration.

>
>> - mutex_unlock()
> Perhaps the start_calibration() also needs to be changed to not touch
> the MIPI_CAL_CONFIG bits of the unrelated pads?
Driver already takes care of programming corresponding pads config only.
>
> Otherwise sounds good to me.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 18:07                                               ` Sowjanya Komatineni
@ 2020-08-06 18:18                                                 ` Dmitry Osipenko
  2020-08-06 18:44                                                   ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Dmitry Osipenko @ 2020-08-06 18:18 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel

06.08.2020 21:07, Sowjanya Komatineni пишет:
> 
> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>> ...
>>> Right mutex_unlock should happen at end of finish_calibration.
>>>
>>> With keeping mutex locked in start, we dont have to check for active to
>>> be 0 to issue start as mutex will keep it locked and other pads
>>> calibration can only go thru when current one is done.
>>>
>>> So instead of below sequence, its simpler to do this way?
>>>
>>> start_calibration()
>>>
>>> - mutex_lock
>>>
>>> - wait for 72uS after start
>>>
>>> finish_calibration()
>>>
>>> - keep check for ACTIVE = 0 and DONE = 1
>> I think only the DONE bits which correspond to the mipi_device->pads
>> bitmask should be awaited.
> 
> As next START can't be triggered when auto cal is ACTIVE, we should keep
> this in finish.
> 
> As we do mutex_unlock only at end of finish, other pads calibrations
> dont go thru till the one in process is finished.
> 
> So in this case ACTIVE applies to current selected pads that are under
> calibration.

Should be better to check only the relevant bits in order to catch bugs,
otherwise you may get a DONE status from the irrelevant pads.

>>> - mutex_unlock()
>> Perhaps the start_calibration() also needs to be changed to not touch
>> the MIPI_CAL_CONFIG bits of the unrelated pads?
> Driver already takes care of programming corresponding pads config only.

It writes 0 to the config of the unrelated pads, which probably isn't
nice if some pads use periodic auto-calibration.

https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350

Although looks like auto-calibration isn't supported by the current driver.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 18:18                                                 ` Dmitry Osipenko
@ 2020-08-06 18:44                                                   ` Sowjanya Komatineni
  2020-08-06 18:51                                                     ` Sowjanya Komatineni
  0 siblings, 1 reply; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 18:44 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 11:18 AM, Dmitry Osipenko wrote:
> 06.08.2020 21:07, Sowjanya Komatineni пишет:
>> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>>> ...
>>>> Right mutex_unlock should happen at end of finish_calibration.
>>>>
>>>> With keeping mutex locked in start, we dont have to check for active to
>>>> be 0 to issue start as mutex will keep it locked and other pads
>>>> calibration can only go thru when current one is done.
>>>>
>>>> So instead of below sequence, its simpler to do this way?
>>>>
>>>> start_calibration()
>>>>
>>>> - mutex_lock
>>>>
>>>> - wait for 72uS after start
>>>>
>>>> finish_calibration()
>>>>
>>>> - keep check for ACTIVE = 0 and DONE = 1
>>> I think only the DONE bits which correspond to the mipi_device->pads
>>> bitmask should be awaited.
>> As next START can't be triggered when auto cal is ACTIVE, we should keep
>> this in finish.
>>
>> As we do mutex_unlock only at end of finish, other pads calibrations
>> dont go thru till the one in process is finished.
>>
>> So in this case ACTIVE applies to current selected pads that are under
>> calibration.
> Should be better to check only the relevant bits in order to catch bugs,
> otherwise you may get a DONE status from the irrelevant pads.
tegra_mipi_device is separate for DSI and CSI channels. mutex lock used 
during calibrate is device specific lock.
So, it will not prevent other devices to hold till on going calibration 
is done unless we add wait for active bit before triggering start.

Currently we wait for active bit at end during calibration done check 
after start trigger. But when other devices go thru calibration in 
parallel as lock is device specific and not common lock for all devices 
it will trigger start but MIPI calibration logic ignore if previous 
calibration is still in progress.

Need to serialize calibration start requests from different devices 
based on ACTIVE bit.

>>> Perhaps the start_calibration() also needs to be changed to not touch
>>> the MIPI_CAL_CONFIG bits of the unrelated pads?
>> Driver already takes care of programming corresponding pads config only.
> It writes 0 to the config of the unrelated pads, which probably isn't
> nice if some pads use periodic auto-calibration.
>
> https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350
>
> Although looks like auto-calibration isn't supported by the current driver.

Yes we don't use auto-calibration.

Only common bit here is MIPI_CAL_CTRL start. All others are pad specific 
currently.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
  2020-08-06 18:44                                                   ` Sowjanya Komatineni
@ 2020-08-06 18:51                                                     ` Sowjanya Komatineni
  0 siblings, 0 replies; 47+ messages in thread
From: Sowjanya Komatineni @ 2020-08-06 18:51 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: jonathanh, frankc, hverkuil, sakari.ailus, robh+dt, helen.koike,
	gregkh, linux-media, devicetree, linux-tegra, linux-kernel


On 8/6/20 11:44 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 11:18 AM, Dmitry Osipenko wrote:
>> 06.08.2020 21:07, Sowjanya Komatineni пишет:
>>> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>>>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>>>> ...
>>>>> Right mutex_unlock should happen at end of finish_calibration.
>>>>>
>>>>> With keeping mutex locked in start, we dont have to check for 
>>>>> active to
>>>>> be 0 to issue start as mutex will keep it locked and other pads
>>>>> calibration can only go thru when current one is done.
>>>>>
>>>>> So instead of below sequence, its simpler to do this way?
>>>>>
>>>>> start_calibration()
>>>>>
>>>>> - mutex_lock
>>>>>
>>>>> - wait for 72uS after start
>>>>>
>>>>> finish_calibration()
>>>>>
>>>>> - keep check for ACTIVE = 0 and DONE = 1
>>>> I think only the DONE bits which correspond to the mipi_device->pads
>>>> bitmask should be awaited.
>>> As next START can't be triggered when auto cal is ACTIVE, we should 
>>> keep
>>> this in finish.
>>>
>>> As we do mutex_unlock only at end of finish, other pads calibrations
>>> dont go thru till the one in process is finished.
>>>
>>> So in this case ACTIVE applies to current selected pads that are under
>>> calibration.
>> Should be better to check only the relevant bits in order to catch bugs,
>> otherwise you may get a DONE status from the irrelevant pads.
> tegra_mipi_device is separate for DSI and CSI channels. mutex lock 
> used during calibrate is device specific lock.
> So, it will not prevent other devices to hold till on going 
> calibration is done unless we add wait for active bit before 
> triggering start.
>
> Currently we wait for active bit at end during calibration done check 
> after start trigger. But when other devices go thru calibration in 
> parallel as lock is device specific and not common lock for all 
> devices it will trigger start but MIPI calibration logic ignore if 
> previous calibration is still in progress.
>
> Need to serialize calibration start requests from different devices 
> based on ACTIVE bit.

Sorry confused. MIPI driver is using common lock from tegra_mipi for all 
tegra_mipi_device

So should be ok as only one device start_calibration can happen at a time.

But its still good to check ACTIVE is cleared and to report as error if 
not as ACTIVE will be cleared once set of pads enabled for this calibration.

Eg: CSI port can be 4 lane and all 4 pads gets enabled at same time and 
with this ACTIVE should still be verified to be 0 to make sure all pads 
calibration is done

>
>>>> Perhaps the start_calibration() also needs to be changed to not touch
>>>> the MIPI_CAL_CONFIG bits of the unrelated pads?
>>> Driver already takes care of programming corresponding pads config 
>>> only.
>> It writes 0 to the config of the unrelated pads, which probably isn't
>> nice if some pads use periodic auto-calibration.
>>
>> https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350 
>>
>>
>> Although looks like auto-calibration isn't supported by the current 
>> driver.
>
> Yes we don't use auto-calibration.
>
> Only common bit here is MIPI_CAL_CTRL start. All others are pad 
> specific currently.
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2020-08-06 18:52 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-03 15:42 [PATCH v8 00/10] Support for Tegra video capture from external sensor Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 01/10] media: tegra-video: Fix channel format alignment Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 02/10] media: tegra-video: Enable TPG based on kernel config Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 03/10] media: tegra-video: Update format lookup to offset based Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 04/10] dt-bindings: tegra: Update VI and CSI bindings with port info Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 05/10] media: tegra-video: Separate CSI stream enable and disable implementations Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 06/10] media: tegra-video: Add support for external sensor capture Sowjanya Komatineni
2020-08-04  5:57   ` Dmitry Osipenko
2020-08-03 15:42 ` [PATCH v8 07/10] media: tegra-video: Add support for selection ioctl ops Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done Sowjanya Komatineni
2020-08-05 13:46   ` Thierry Reding
2020-08-05 14:05     ` Dmitry Osipenko
2020-08-05 14:19       ` Dmitry Osipenko
2020-08-05 16:33         ` Sowjanya Komatineni
2020-08-05 16:47           ` Dmitry Osipenko
2020-08-05 16:50             ` Sowjanya Komatineni
2020-08-05 16:57               ` Dmitry Osipenko
2020-08-05 17:04                 ` Sowjanya Komatineni
2020-08-05 17:23                   ` Dmitry Osipenko
2020-08-05 17:32                     ` Sowjanya Komatineni
2020-08-05 17:29                   ` Sowjanya Komatineni
2020-08-05 17:34                     ` Dmitry Osipenko
2020-08-05 17:46                       ` Sowjanya Komatineni
2020-08-05 18:06                         ` Sowjanya Komatineni
2020-08-06  0:47                           ` Sowjanya Komatineni
2020-08-06 13:32                             ` Dmitry Osipenko
2020-08-06 15:59                               ` Sowjanya Komatineni
2020-08-06 16:10                                 ` Dmitry Osipenko
2020-08-06 16:41                                   ` Sowjanya Komatineni
2020-08-06 16:45                                     ` Dmitry Osipenko
2020-08-06 16:51                                       ` Sowjanya Komatineni
2020-08-06 17:15                                         ` Dmitry Osipenko
2020-08-06 17:12                                     ` Sowjanya Komatineni
2020-08-06 17:27                                       ` Dmitry Osipenko
2020-08-06 17:44                                         ` Sowjanya Komatineni
2020-08-06 17:52                                           ` Sowjanya Komatineni
2020-08-06 18:01                                             ` Dmitry Osipenko
2020-08-06 18:07                                               ` Sowjanya Komatineni
2020-08-06 18:18                                                 ` Dmitry Osipenko
2020-08-06 18:44                                                   ` Sowjanya Komatineni
2020-08-06 18:51                                                     ` Sowjanya Komatineni
2020-08-06 16:13                                 ` Dmitry Osipenko
2020-08-06 16:37                                   ` Dmitry Osipenko
2020-08-06 16:42                                     ` Sowjanya Komatineni
2020-08-06 16:43                                       ` Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 09/10] media: tegra-video: Add CSI MIPI pads calibration Sowjanya Komatineni
2020-08-03 15:42 ` [PATCH v8 10/10] media: tegra-video: Compute settle times based on the clock rate Sowjanya Komatineni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).