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 IMX219 camera sensor.
This series include,
VI I2C related fixes
- Camera sensor programming happens through VI I2C which is on host1x bus.
- These patches includes device tree and I2C driver fixes for VI I2C.
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.
Host1x driver updates
- Adds API to allow creating mipi device for specific device node.
- Splits MIPI pads calibrate start and waiting for calibration to be done.
Device tree updates
- Adds camera connector 2V8, 1V8, 1V2 regulator supplies to Jetson TX1 DT.
- Enabled VI and CSI support in Jetson Nano DT.
Delta between patch versions:
[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 (14):
i2c: tegra: Don't mark VI I2C as IRQ safe runtime PM
i2c: tegra: Remove NULL pointer check before
clk_enable/disable/prepare/unprepare
i2c: tegra: Fix the error path in tegra_i2c_runtime_resume
i2c: tegra: Fix runtime resume to re-init VI I2C
i2c: tegra: Avoid tegra_i2c_init_dma() for Tegra210 vi i2c
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: 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/i2c/busses/i2c-tegra.c | 101 +--
drivers/staging/media/tegra-video/Kconfig | 7 +
drivers/staging/media/tegra-video/TODO | 6 -
drivers/staging/media/tegra-video/csi.c | 258 ++++++-
drivers/staging/media/tegra-video/csi.h | 8 +
drivers/staging/media/tegra-video/tegra210.c | 25 +-
drivers/staging/media/tegra-video/vi.c | 800 +++++++++++++++++++--
drivers/staging/media/tegra-video/vi.h | 25 +-
drivers/staging/media/tegra-video/video.c | 23 +-
include/linux/host1x.h | 5 +-
13 files changed, 1224 insertions(+), 149 deletions(-)
--
2.7.4
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 <[email protected]>
---
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 752ebe8..5bcd58e 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_s_stream(struct v4l2_subdev *subdev, int enable)
{
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
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.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
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
Update VI and CSI bindings to add port and endpoint nodes as per
media video-interfaces DT binding document.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
.../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
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.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/staging/media/tegra-video/TODO | 1 -
drivers/staging/media/tegra-video/csi.c | 55 +++++++++++++++++++++++++++++++--
drivers/staging/media/tegra-video/csi.h | 2 ++
drivers/staging/media/tegra-video/vi.c | 25 ++++++++++++++-
4 files changed, 78 insertions(+), 5 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 c21dd09..752ebe8 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -252,15 +252,51 @@ static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
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;
+ }
+ }
+
ret = csi->ops->csi_start_streaming(csi_chan);
- if (ret < 0)
- goto rpm_put;
+ if (ret < 0) {
+ if (csi_chan->mipi)
+ tegra_mipi_cancel_calibration(csi_chan->mipi);
+ goto disable_mipi;
+ }
return 0;
}
csi->ops->csi_stop_streaming(csi_chan);
+disable_mipi:
+ if (csi_chan->mipi) {
+ ret = tegra_mipi_disable(csi_chan->mipi);
+ if (ret < 0)
+ dev_err(csi->dev,
+ "failed to disable MIPI pads: %d\n", ret);
+ }
+
rpm_put:
pm_runtime_put(csi->dev);
return ret;
@@ -294,6 +330,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)
@@ -312,7 +349,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)
@@ -475,6 +521,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 fc43629..9c01cc5 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -191,10 +191,12 @@ tegra_channel_get_remote_source_subdev(struct tegra_vi_channel *chan)
int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
{
struct v4l2_subdev *subdev, *csi_subdev, *src_subdev;
+ struct tegra_csi_channel *csi_chan;
int ret;
csi_subdev = tegra_channel_get_remote_csi_subdev(chan);
src_subdev = tegra_channel_get_remote_source_subdev(chan);
+ csi_chan = v4l2_get_subdevdata(csi_subdev);
/*
* Tegra CSI receiver can detect the first LP to HS transition.
* So, start the CSI stream-on prior to sensor stream-on and
@@ -208,10 +210,31 @@ int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
return 0;
+ /*
+ * 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.
+ */
subdev = on ? src_subdev : csi_subdev;
ret = v4l2_subdev_call(subdev, video, s_stream, on);
- if (ret < 0 && ret != -ENOIOCTLCMD)
+ if (ret < 0 && ret != -ENOIOCTLCMD) {
+ if (on && csi_chan->mipi)
+ tegra_mipi_cancel_calibration(csi_chan->mipi);
return ret;
+ }
+
+ if (on && csi_chan->mipi) {
+ ret = tegra_mipi_finish_calibration(csi_chan->mipi);
+ if (ret < 0)
+ dev_err(csi_chan->csi->dev,
+ "MIPI calibration failed: %d\n", ret);
+ return ret;
+ }
return 0;
}
--
2.7.4
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 <[email protected]>
---
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 f7dbea2..fc43629 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -437,6 +437,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);
@@ -462,6 +469,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;
@@ -551,6 +576,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)
{
@@ -608,6 +712,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
clk_enable, clk_disable, clk_prepare, and clk_unprepare APIs have
implementation for checking clk pointer not NULL and clock consumers
can safely call these APIs without NULL pointer check.
So, this patch cleans up Tegra i2c driver to remove explicit checks
before these APIs.
Reviewed-by: Dmitry Osipenko <[email protected]>
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 64 +++++++++++++++---------------------------
1 file changed, 23 insertions(+), 41 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 3be1018..c91307b9 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -655,21 +655,17 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
if (ret)
return ret;
- if (!i2c_dev->hw->has_single_clk_source) {
- ret = clk_enable(i2c_dev->fast_clk);
- if (ret < 0) {
- dev_err(i2c_dev->dev,
- "Enabling fast clk failed, err %d\n", ret);
- return ret;
- }
+ ret = clk_enable(i2c_dev->fast_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev,
+ "Enabling fast clk failed, err %d\n", ret);
+ return ret;
}
- if (i2c_dev->slow_clk) {
- ret = clk_enable(i2c_dev->slow_clk);
- if (ret < 0) {
- dev_err(dev, "failed to enable slow clock: %d\n", ret);
- return ret;
- }
+ ret = clk_enable(i2c_dev->slow_clk);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable slow clock: %d\n", ret);
+ return ret;
}
ret = clk_enable(i2c_dev->div_clk);
@@ -688,12 +684,8 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev)
struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
clk_disable(i2c_dev->div_clk);
-
- if (i2c_dev->slow_clk)
- clk_disable(i2c_dev->slow_clk);
-
- if (!i2c_dev->hw->has_single_clk_source)
- clk_disable(i2c_dev->fast_clk);
+ clk_disable(i2c_dev->slow_clk);
+ clk_disable(i2c_dev->fast_clk);
return pinctrl_pm_select_idle_state(i2c_dev->dev);
}
@@ -1716,20 +1708,16 @@ static int tegra_i2c_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, i2c_dev);
- if (!i2c_dev->hw->has_single_clk_source) {
- ret = clk_prepare(i2c_dev->fast_clk);
- if (ret < 0) {
- dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
- return ret;
- }
+ ret = clk_prepare(i2c_dev->fast_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+ return ret;
}
- if (i2c_dev->slow_clk) {
- ret = clk_prepare(i2c_dev->slow_clk);
- if (ret < 0) {
- dev_err(dev, "failed to prepare slow clock: %d\n", ret);
- goto unprepare_fast_clk;
- }
+ ret = clk_prepare(i2c_dev->slow_clk);
+ if (ret < 0) {
+ dev_err(dev, "failed to prepare slow clock: %d\n", ret);
+ goto unprepare_fast_clk;
}
if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ &&
@@ -1843,12 +1831,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
clk_unprepare(i2c_dev->div_clk);
unprepare_slow_clk:
- if (i2c_dev->is_vi)
- clk_unprepare(i2c_dev->slow_clk);
+ clk_unprepare(i2c_dev->slow_clk);
unprepare_fast_clk:
- if (!i2c_dev->hw->has_single_clk_source)
- clk_unprepare(i2c_dev->fast_clk);
+ clk_unprepare(i2c_dev->fast_clk);
return ret;
}
@@ -1867,12 +1853,8 @@ static int tegra_i2c_remove(struct platform_device *pdev)
tegra_i2c_runtime_suspend(&pdev->dev);
clk_unprepare(i2c_dev->div_clk);
-
- if (i2c_dev->slow_clk)
- clk_unprepare(i2c_dev->slow_clk);
-
- if (!i2c_dev->hw->has_single_clk_source)
- clk_unprepare(i2c_dev->fast_clk);
+ clk_unprepare(i2c_dev->slow_clk);
+ clk_unprepare(i2c_dev->fast_clk);
tegra_i2c_release_dma(i2c_dev);
return 0;
--
2.7.4
VI I2C is on host1x bus and is part of VE power domain.
During suspend/resume VE power domain goes through power off/on.
So, controller reset followed by i2c re-initialization is required
after the domain power up.
This patch fixes it.
Reviewed-by: Dmitry Osipenko <[email protected]>
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7b93c45..1bf3666 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -293,6 +293,8 @@ struct tegra_i2c_dev {
bool is_curr_atomic_xfer;
};
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit);
+
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
unsigned long reg)
{
@@ -675,8 +677,22 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
goto disable_slow_clk;
}
+ /*
+ * VI I2C device is attached to VE power domain which goes through
+ * power ON/OFF during PM runtime resume/suspend. So, controller
+ * should go through reset and need to re-initialize after power
+ * domain ON.
+ */
+ if (i2c_dev->is_vi) {
+ ret = tegra_i2c_init(i2c_dev, true);
+ if (ret)
+ goto disable_div_clk;
+ }
+
return 0;
+disable_div_clk:
+ clk_disable(i2c_dev->div_clk);
disable_slow_clk:
clk_disable(i2c_dev->slow_clk);
disable_fast_clk:
--
2.7.4
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 <[email protected]>
---
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
27.07.2020 23:57, Sowjanya Komatineni пишет:
> + /*
> + * 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.
> + */
> subdev = on ? src_subdev : csi_subdev;
> ret = v4l2_subdev_call(subdev, video, s_stream, on);
> - if (ret < 0 && ret != -ENOIOCTLCMD)
> + if (ret < 0 && ret != -ENOIOCTLCMD) {
I assume -ENOIOCTLCMD means that camera wasn't turned ON, so why
-ENOIOCTLCMD is special?
> + if (on && csi_chan->mipi)
> + tegra_mipi_cancel_calibration(csi_chan->mipi);
> return ret;
> + }
> +
> + if (on && csi_chan->mipi) {
Does finish_calibration() really need to be called for ret=-ENOIOCTLCMD?
Shouldn't it be cancel_calibration( for the -ENOIOCTLCMD?
> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
> + if (ret < 0)
> + dev_err(csi_chan->csi->dev,
> + "MIPI calibration failed: %d\n", ret);
Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
> + return ret;
> + }
>
> return 0;
> }
27.07.2020 23:57, Sowjanya Komatineni пишет:
> 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.
>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
> ---
> 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)
> +{
Doesn't MIPI_CAL need to be reset here?
> + 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);
This function sets MIPI_CAL_CLKEN_OVR bit, does it mean that MIPI clock
becomes always-ON?
I don't see where MIPI_CAL_CLKEN_OVR is unset.
> 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
>
On 7/28/20 3:30 AM, Dmitry Osipenko wrote:
> 27.07.2020 23:57, Sowjanya Komatineni пишет:
>> + /*
>> + * 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.
>> + */
>> subdev = on ? src_subdev : csi_subdev;
>> ret = v4l2_subdev_call(subdev, video, s_stream, on);
>> - if (ret < 0 && ret != -ENOIOCTLCMD)
>> + if (ret < 0 && ret != -ENOIOCTLCMD) {
> I assume -ENOIOCTLCMD means that camera wasn't turned ON, so why
> -ENOIOCTLCMD is special?
No -ENOIOCTLCMD mean subdev don't have s_stream ops
>
>> + if (on && csi_chan->mipi)
>> + tegra_mipi_cancel_calibration(csi_chan->mipi);
>> return ret;
>> + }
>> +
>> + if (on && csi_chan->mipi) {
> Does finish_calibration() really need to be called for ret=-ENOIOCTLCMD?
>
> Shouldn't it be cancel_calibration( for the -ENOIOCTLCMD?
start calibration happens during csi sensor streaming which happens
prior to this point.
In case if sensor subdev does not have s_stream ops, then either
finish/cancel calibration should happen to disable the clock.
>
>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>> + if (ret < 0)
>> + dev_err(csi_chan->csi->dev,
>> + "MIPI calibration failed: %d\n", ret);
> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
Not required as on error streaming fails and runtime PM will turn off
power anyway.
Also we only did csi subdev s_stream on and during sensor subdev
s_stream on fail, actual stream dont happen and on tegra side frame
capture by HW happens only when kthreads run.
>> + return ret;
>> + }
>>
>> return 0;
>> }
On 7/28/20 4:03 AM, Dmitry Osipenko wrote:
> 27.07.2020 23:57, Sowjanya Komatineni пишет:
>> 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.
>>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> 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)
>> +{
> Doesn't MIPI_CAL need to be reset here?
No need to reset MIPI CAL
>
>> + 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);
> This function sets MIPI_CAL_CLKEN_OVR bit, does it mean that MIPI clock
> becomes always-ON?
>
> I don't see where MIPI_CAL_CLKEN_OVR is unset.
CLKEN_OVR was always kept enabled in the driver prior to my patch and I
was not touching that.
But as we are turning ON/OFF MIPI clock itself no need to unset CLKEN_OVR
>
>> 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
>>
On Mon, Jul 27, 2020 at 01:57:20PM -0700, Sowjanya Komatineni wrote:
> clk_enable, clk_disable, clk_prepare, and clk_unprepare APIs have
> implementation for checking clk pointer not NULL and clock consumers
> can safely call these APIs without NULL pointer check.
>
> So, this patch cleans up Tegra i2c driver to remove explicit checks
> before these APIs.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
Applied to for-next, thanks!
On Mon, Jul 27, 2020 at 01:57:22PM -0700, Sowjanya Komatineni wrote:
> VI I2C is on host1x bus and is part of VE power domain.
>
> During suspend/resume VE power domain goes through power off/on.
>
> So, controller reset followed by i2c re-initialization is required
> after the domain power up.
>
> This patch fixes it.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
Applied to for-next, thanks!
On 7/28/20 8:59 AM, Sowjanya Komatineni wrote:
>
> On 7/28/20 3:30 AM, Dmitry Osipenko wrote:
>> 27.07.2020 23:57, Sowjanya Komatineni пишет:
>>> + /*
>>> + * 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.
>>> + */
>>> subdev = on ? src_subdev : csi_subdev;
>>> ret = v4l2_subdev_call(subdev, video, s_stream, on);
>>> - if (ret < 0 && ret != -ENOIOCTLCMD)
>>> + if (ret < 0 && ret != -ENOIOCTLCMD) {
>> I assume -ENOIOCTLCMD means that camera wasn't turned ON, so why
>> -ENOIOCTLCMD is special?
> No -ENOIOCTLCMD mean subdev don't have s_stream ops
>>
>>> + if (on && csi_chan->mipi)
>>> + tegra_mipi_cancel_calibration(csi_chan->mipi);
>>> return ret;
>>> + }
>>> +
>>> + if (on && csi_chan->mipi) {
>> Does finish_calibration() really need to be called for ret=-ENOIOCTLCMD?
>>
>> Shouldn't it be cancel_calibration( for the -ENOIOCTLCMD?
>
> start calibration happens during csi sensor streaming which happens
> prior to this point.
>
> In case if sensor subdev does not have s_stream ops, then either
> finish/cancel calibration should happen to disable the clock.
For -ENOIOCTLCMD, calling finish calibration as some sensors might keep
pads in LP-11 on power up and for such sensors calibration logic will
apply results to pads and done bit will be set.
Also avoiding additional check to specifically call cancel calibration
on ENOIOCTLCMD and making it fall into finish calibration as both does
disable clock except finish will wait for done bit to be set.
Also, most sensor subdev have s_stream ops implemented.
>
>>
>>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>>> + if (ret < 0)
>>> + dev_err(csi_chan->csi->dev,
>>> + "MIPI calibration failed: %d\n", ret);
>> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>
> Not required as on error streaming fails and runtime PM will turn off
> power anyway.
>
> Also we only did csi subdev s_stream on and during sensor subdev
> s_stream on fail, actual stream dont happen and on tegra side frame
> capture by HW happens only when kthreads run.
>>> + return ret;
>>> + }
>>> return 0;
>>> }
28.07.2020 19:04, Sowjanya Komatineni пишет:
...
>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>> +{
>> Doesn't MIPI_CAL need to be reset here?
> No need to reset MIPI CAL
Could you please explain why. There is a calibration state-machine that
apparently needs to be returned into initial state, does it return by
itself?
TRM says that MIPI block needs to be reset before of starting
calibration process. The reset is completely missing in the driver, I
assume it needs to be corrected with another patch.
On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
> 28.07.2020 19:04, Sowjanya Komatineni пишет:
> ...
>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>> +{
>>> Doesn't MIPI_CAL need to be reset here?
>> No need to reset MIPI CAL
> Could you please explain why. There is a calibration state-machine that
> apparently needs to be returned into initial state, does it return by
> itself?
>
> TRM says that MIPI block needs to be reset before of starting
> calibration process. The reset is completely missing in the driver, I
> assume it needs to be corrected with another patch.
TRM documented incorrectly. There is no need to reset MIPI_CAL.
MIPI CAL is FSM and it does not hang and done bit is to indicate if
results are applied to pads or not.
If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
results are not applied to pads.
Also while multiple streams can happen in parallel and we can't reset
MIPI CAL as other CSI channel streams (using other pads) may also be
going thru calibration process in parallel depending and also DSI pads
also are calibrated thru same MIPI CAL controller.
28.07.2020 18:59, Sowjanya Komatineni пишет:
...
>>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>>> + if (ret < 0)
>>> + dev_err(csi_chan->csi->dev,
>>> + "MIPI calibration failed: %d\n", ret);
>> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>
> Not required as on error streaming fails and runtime PM will turn off
> power anyway.
I see that camera drivers bump theirs RPM on s_stream=1, and thus,
s_stream=0 should be invoked in order to balance the RPM. What am I missing?
https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/i2c/ov2740.c#L634
> Also we only did csi subdev s_stream on and during sensor subdev
> s_stream on fail, actual stream dont happen and on tegra side frame
> capture by HW happens only when kthreads run.
Secondly, perhaps a failed calibration isn't a very critical error?
Hence just printing a warning message should be enough.
Could you please make a patch that factors all ON/OFF code paths into a
separate functions? It's a bit difficult to follow the combined code,
especially partial changes in the patches. Thanks in advance!
29.07.2020 20:55, Sowjanya Komatineni пишет:
>
> On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
>> 28.07.2020 19:04, Sowjanya Komatineni пишет:
>> ...
>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>> +{
>>>> Doesn't MIPI_CAL need to be reset here?
>>> No need to reset MIPI CAL
>> Could you please explain why. There is a calibration state-machine that
>> apparently needs to be returned into initial state, does it return by
>> itself?
>>
>> TRM says that MIPI block needs to be reset before of starting
>> calibration process. The reset is completely missing in the driver, I
>> assume it needs to be corrected with another patch.
>
> TRM documented incorrectly. There is no need to reset MIPI_CAL.
>
> MIPI CAL is FSM and it does not hang and done bit is to indicate if
> results are applied to pads or not.
>
> If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
> results are not applied to pads.
But how to stop calibration from triggering on LP-11 once it has been
enabled? The reset should be needed since there is no other way to reset
the calibration state.
> Also while multiple streams can happen in parallel and we can't reset
> MIPI CAL as other CSI channel streams (using other pads) may also be
> going thru calibration process in parallel depending and also DSI pads
> also are calibrated thru same MIPI CAL controller.
Perhaps this should be the case for a shared reset control API usage.
On 7/29/20 4:42 PM, Dmitry Osipenko wrote:
> 29.07.2020 20:55, Sowjanya Komatineni пишет:
>> On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
>>> 28.07.2020 19:04, Sowjanya Komatineni пишет:
>>> ...
>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>>> +{
>>>>> Doesn't MIPI_CAL need to be reset here?
>>>> No need to reset MIPI CAL
>>> Could you please explain why. There is a calibration state-machine that
>>> apparently needs to be returned into initial state, does it return by
>>> itself?
>>>
>>> TRM says that MIPI block needs to be reset before of starting
>>> calibration process. The reset is completely missing in the driver, I
>>> assume it needs to be corrected with another patch.
>> TRM documented incorrectly. There is no need to reset MIPI_CAL.
>>
>> MIPI CAL is FSM and it does not hang and done bit is to indicate if
>> results are applied to pads or not.
>>
>> If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
>> results are not applied to pads.
> But how to stop calibration from triggering on LP-11 once it has been
> enabled? The reset should be needed since there is no other way to reset
> the calibration state.
Its a finite state machine that goes thru fixed steps of sequence codes
internally and holds results in registers.
When it sees LP-11 results are applied to pads.
If it does not see LP-11, next start will again trigger calibrating with
finite sequence codes.
As per HW designers, we don't have to do any reverts when done bit is
not set.
>> Also while multiple streams can happen in parallel and we can't reset
>> MIPI CAL as other CSI channel streams (using other pads) may also be
>> going thru calibration process in parallel depending and also DSI pads
>> also are calibrated thru same MIPI CAL controller.
> Perhaps this should be the case for a shared reset control API usage.
On 7/29/20 4:25 PM, Dmitry Osipenko wrote:
> 28.07.2020 18:59, Sowjanya Komatineni пишет:
> ...
>>>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>>>> + if (ret < 0)
>>>> + dev_err(csi_chan->csi->dev,
>>>> + "MIPI calibration failed: %d\n", ret);
>>> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>> Not required as on error streaming fails and runtime PM will turn off
>> power anyway.
> I see that camera drivers bump theirs RPM on s_stream=1, and thus,
> s_stream=0 should be invoked in order to balance the RPM. What am I missing?
>
> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/i2c/ov2740.c#L634
Sensor drivers take care of RPM put when any failure happens during
s_stream.
So bridge driver don't have to call v4l2_subdev_call s_stream off incase
if sensor subdev stream on fails.
>> Also we only did csi subdev s_stream on and during sensor subdev
>> s_stream on fail, actual stream dont happen and on tegra side frame
>> capture by HW happens only when kthreads run.
> Secondly, perhaps a failed calibration isn't a very critical error?
> Hence just printing a warning message should be enough.
Using dev_err to report calibration failure. Are you suggesting to use
dev_warn instead of dev_err?
>
> Could you please make a patch that factors all ON/OFF code paths into a
> separate functions? It's a bit difficult to follow the combined code,
> especially partial changes in the patches. Thanks in advance!
what do you mean by partial changes in patches?
Can you please be more clear?
Thanks
Sowjanya
On 7/29/20 4:59 PM, Sowjanya Komatineni wrote:
>
> On 7/29/20 4:25 PM, Dmitry Osipenko wrote:
>> 28.07.2020 18:59, Sowjanya Komatineni пишет:
>> ...
>>>>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>>>>> + if (ret < 0)
>>>>> + dev_err(csi_chan->csi->dev,
>>>>> + "MIPI calibration failed: %d\n", ret);
>>>> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>>> Not required as on error streaming fails and runtime PM will turn off
>>> power anyway.
>> I see that camera drivers bump theirs RPM on s_stream=1, and thus,
>> s_stream=0 should be invoked in order to balance the RPM. What am I
>> missing?
>>
>> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/i2c/ov2740.c#L634
>>
>
> Sensor drivers take care of RPM put when any failure happens during
> s_stream.
>
> So bridge driver don't have to call v4l2_subdev_call s_stream off
> incase if sensor subdev stream on fails.
>
>>> Also we only did csi subdev s_stream on and during sensor subdev
>>> s_stream on fail, actual stream dont happen and on tegra side frame
>>> capture by HW happens only when kthreads run.
>> Secondly, perhaps a failed calibration isn't a very critical error?
>> Hence just printing a warning message should be enough.
>
> Using dev_err to report calibration failure. Are you suggesting to use
> dev_warn instead of dev_err?
>
>>
>> Could you please make a patch that factors all ON/OFF code paths into a
>> separate functions? It's a bit difficult to follow the combined code,
>> especially partial changes in the patches. Thanks in advance!
>
> what do you mean by partial changes in patches?
>
> Can you please be more clear?
Also please specify what ON/OFF code paths you are referring to when you
say to move into separate functions?
>
> Thanks
>
> Sowjanya
>
30.07.2020 02:54, Sowjanya Komatineni пишет:
>
> On 7/29/20 4:42 PM, Dmitry Osipenko wrote:
>> 29.07.2020 20:55, Sowjanya Komatineni пишет:
>>> On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
>>>> 28.07.2020 19:04, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>> *device)
>>>>>>> +{
>>>>>> Doesn't MIPI_CAL need to be reset here?
>>>>> No need to reset MIPI CAL
>>>> Could you please explain why. There is a calibration state-machine that
>>>> apparently needs to be returned into initial state, does it return by
>>>> itself?
>>>>
>>>> TRM says that MIPI block needs to be reset before of starting
>>>> calibration process. The reset is completely missing in the driver, I
>>>> assume it needs to be corrected with another patch.
>>> TRM documented incorrectly. There is no need to reset MIPI_CAL.
>>>
>>> MIPI CAL is FSM and it does not hang and done bit is to indicate if
>>> results are applied to pads or not.
>>>
>>> If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
>>> results are not applied to pads.
>> But how to stop calibration from triggering on LP-11 once it has been
>> enabled? The reset should be needed since there is no other way to reset
>> the calibration state.
>
> Its a finite state machine that goes thru fixed steps of sequence codes
> internally and holds results in registers.
>
> When it sees LP-11 results are applied to pads.
>
> If it does not see LP-11, next start will again trigger calibrating with
> finite sequence codes.
>
> As per HW designers, we don't have to do any reverts when done bit is
> not set.
Alright, then should be good if HW can't stuck.
Reviewed-by: Dmitry Osipenko <[email protected]>
On 7/29/20 5:27 PM, Sowjanya Komatineni wrote:
>
> On 7/29/20 4:59 PM, Sowjanya Komatineni wrote:
>>
>> On 7/29/20 4:25 PM, Dmitry Osipenko wrote:
>>> 28.07.2020 18:59, Sowjanya Komatineni пишет:
>>> ...
>>>>>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
>>>>>> + if (ret < 0)
>>>>>> + dev_err(csi_chan->csi->dev,
>>>>>> + "MIPI calibration failed: %d\n", ret);
>>>>> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error?
>>>> Not required as on error streaming fails and runtime PM will turn off
>>>> power anyway.
>>> I see that camera drivers bump theirs RPM on s_stream=1, and thus,
>>> s_stream=0 should be invoked in order to balance the RPM. What am I
>>> missing?
>>>
>>> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/i2c/ov2740.c#L634
>>>
>>
>> Sensor drivers take care of RPM put when any failure happens during
>> s_stream.
>>
>> So bridge driver don't have to call v4l2_subdev_call s_stream off
>> incase if sensor subdev stream on fails.
>>
>>>> Also we only did csi subdev s_stream on and during sensor subdev
>>>> s_stream on fail, actual stream dont happen and on tegra side frame
>>>> capture by HW happens only when kthreads run.
>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>> Hence just printing a warning message should be enough.
>>
>> Using dev_err to report calibration failure. Are you suggesting to
>> use dev_warn instead of dev_err?
>>
OK I think I understood what you meant.
When v4l2_subdev_call for sensor s_stream ON fails, we dont have to do
v4l2_subdev_call s_stream OFF.
As sensor drivers take care of RPM put when any failure happens during
s_stream ON
Other case when v4l2_subdev_call for sensor s_stream ON is good, then
tegra_mipi_finish_calibration fail need to call s_stream OFF for sensor.
Agree as calibration errors out in this case as its not critical in this
scenario, So will change dev_err to dev_warn and will not report this as
error so no need to call s_stream off.
>>>
>>> Could you please make a patch that factors all ON/OFF code paths into a
>>> separate functions? It's a bit difficult to follow the combined code,
>>> especially partial changes in the patches. Thanks in advance!
>>
>> what do you mean by partial changes in patches?
>>
>> Can you please be more clear?
>
> Also please specify what ON/OFF code paths you are referring to when
> you say to move into separate functions?
>
>>
>> Thanks
>>
>> Sowjanya
>>
30.07.2020 03:27, Sowjanya Komatineni пишет:
...
>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>> Hence just printing a warning message should be enough.
>>
>> Using dev_err to report calibration failure. Are you suggesting to use
>> dev_warn instead of dev_err?
I meant that failing s_stream might be unnecessary.
The dev_warn should be more appropriate for a non-critical errors.
>>> Could you please make a patch that factors all ON/OFF code paths into a
>>> separate functions? It's a bit difficult to follow the combined code,
>>> especially partial changes in the patches. Thanks in advance!
>>
>> what do you mean by partial changes in patches?
>>
>> Can you please be more clear?
>
> Also please specify what ON/OFF code paths you are referring to when you
> say to move into separate functions?
I meant to change all the code like this:
set(on) {
if (on) {
...
return;
}
if (!on)
...
return;
}
to somewhat like this:
set(on) {
if (on)
ret = enable();
else
ret = disable();
return ret;
}
On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
> 30.07.2020 03:27, Sowjanya Komatineni пишет:
> ...
>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>> Hence just printing a warning message should be enough.
>>> Using dev_err to report calibration failure. Are you suggesting to use
>>> dev_warn instead of dev_err?
> I meant that failing s_stream might be unnecessary.
>
> The dev_warn should be more appropriate for a non-critical errors.
>
>>>> Could you please make a patch that factors all ON/OFF code paths into a
>>>> separate functions? It's a bit difficult to follow the combined code,
>>>> especially partial changes in the patches. Thanks in advance!
>>> what do you mean by partial changes in patches?
>>>
>>> Can you please be more clear?
>> Also please specify what ON/OFF code paths you are referring to when you
>> say to move into separate functions?
> I meant to change all the code like this:
>
> set(on) {
> if (on) {
> ...
> return;
> }
>
> if (!on)
> ...
>
> return;
> }
>
> to somewhat like this:
>
> set(on) {
> if (on)
> ret = enable();
> else
> ret = disable();
>
> return ret;
> }
You mean to change tegra_channel_set_stream() ?
On 7/29/20 5:52 PM, Sowjanya Komatineni wrote:
>
> On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
>> 30.07.2020 03:27, Sowjanya Komatineni пишет:
>> ...
>>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>>> Hence just printing a warning message should be enough.
>>>> Using dev_err to report calibration failure. Are you suggesting to use
>>>> dev_warn instead of dev_err?
>> I meant that failing s_stream might be unnecessary.
>>
>> The dev_warn should be more appropriate for a non-critical errors.
>>
>>>>> Could you please make a patch that factors all ON/OFF code paths
>>>>> into a
>>>>> separate functions? It's a bit difficult to follow the combined code,
>>>>> especially partial changes in the patches. Thanks in advance!
>>>> what do you mean by partial changes in patches?
>>>>
>>>> Can you please be more clear?
>>> Also please specify what ON/OFF code paths you are referring to when
>>> you
>>> say to move into separate functions?
>> I meant to change all the code like this:
>>
>> set(on) {
>> if (on) {
>> ...
>> return;
>> }
>>
>> if (!on)
>> ...
>>
>> return;
>> }
>>
>> to somewhat like this:
>>
>> set(on) {
>> if (on)
>> ret = enable();
>> else
>> ret = disable();
>>
>> return ret;
>> }
>
> You mean to change tegra_channel_set_stream() ?
changing tegra_channel_set_stream() to have like below will have
redundant calls as most of the code b/w enable and disable is same
except calling them in reverse order based on on/off and doing MIPI
calibration only during ON
if (on)
ret = enable()
else
ret = disable()
return ret;
>
>
30.07.2020 03:55, Sowjanya Komatineni пишет:
>
> On 7/29/20 5:52 PM, Sowjanya Komatineni wrote:
>>
>> On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
>>> 30.07.2020 03:27, Sowjanya Komatineni пишет:
>>> ...
>>>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>>>> Hence just printing a warning message should be enough.
>>>>> Using dev_err to report calibration failure. Are you suggesting to use
>>>>> dev_warn instead of dev_err?
>>> I meant that failing s_stream might be unnecessary.
>>>
>>> The dev_warn should be more appropriate for a non-critical errors.
>>>
>>>>>> Could you please make a patch that factors all ON/OFF code paths
>>>>>> into a
>>>>>> separate functions? It's a bit difficult to follow the combined code,
>>>>>> especially partial changes in the patches. Thanks in advance!
>>>>> what do you mean by partial changes in patches?
>>>>>
>>>>> Can you please be more clear?
>>>> Also please specify what ON/OFF code paths you are referring to when
>>>> you
>>>> say to move into separate functions?
>>> I meant to change all the code like this:
>>>
>>> set(on) {
>>> if (on) {
>>> ...
>>> return;
>>> }
>>>
>>> if (!on)
>>> ...
>>>
>>> return;
>>> }
>>>
>>> to somewhat like this:
>>>
>>> set(on) {
>>> if (on)
>>> ret = enable();
>>> else
>>> ret = disable();
>>>
>>> return ret;
>>> }
>>
>> You mean to change tegra_channel_set_stream() ?
Yes, and tegra_csi_s_stream().
> changing tegra_channel_set_stream() to have like below will have
> redundant calls as most of the code b/w enable and disable is same
> except calling them in reverse order based on on/off and doing MIPI
> calibration only during ON
>
>
> if (on)
> ret = enable()
> else
> ret = disable()
> return ret;
Readability should be more important than number of lines.
On 7/29/20 5:53 PM, Dmitry Osipenko wrote:
> 30.07.2020 03:55, Sowjanya Komatineni пишет:
>> On 7/29/20 5:52 PM, Sowjanya Komatineni wrote:
>>> On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
>>>> 30.07.2020 03:27, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>>>>> Hence just printing a warning message should be enough.
>>>>>> Using dev_err to report calibration failure. Are you suggesting to use
>>>>>> dev_warn instead of dev_err?
>>>> I meant that failing s_stream might be unnecessary.
>>>>
>>>> The dev_warn should be more appropriate for a non-critical errors.
>>>>
>>>>>>> Could you please make a patch that factors all ON/OFF code paths
>>>>>>> into a
>>>>>>> separate functions? It's a bit difficult to follow the combined code,
>>>>>>> especially partial changes in the patches. Thanks in advance!
>>>>>> what do you mean by partial changes in patches?
>>>>>>
>>>>>> Can you please be more clear?
>>>>> Also please specify what ON/OFF code paths you are referring to when
>>>>> you
>>>>> say to move into separate functions?
>>>> I meant to change all the code like this:
>>>>
>>>> set(on) {
>>>> if (on) {
>>>> ...
>>>> return;
>>>> }
>>>>
>>>> if (!on)
>>>> ...
>>>>
>>>> return;
>>>> }
>>>>
>>>> to somewhat like this:
>>>>
>>>> set(on) {
>>>> if (on)
>>>> ret = enable();
>>>> else
>>>> ret = disable();
>>>>
>>>> return ret;
>>>> }
>>> You mean to change tegra_channel_set_stream() ?
> Yes, and tegra_csi_s_stream().
>
>> changing tegra_channel_set_stream() to have like below will have
>> redundant calls as most of the code b/w enable and disable is same
>> except calling them in reverse order based on on/off and doing MIPI
>> calibration only during ON
>>
>>
>> if (on)
>> ret = enable()
>> else
>> ret = disable()
>> return ret;
> Readability should be more important than number of lines.
Will have v6 and add additional patch at the end to do enable/disable
separately.
Separating this out with additional patch before adding sensor support
patch requires all patches to be updated.
So I am ok either ways. Please let me know if adding additional patch at
the end to split tegra_channel_set_stream() and tegra_csi_s_stream()
separately is ok?
30.07.2020 04:06, Sowjanya Komatineni пишет:
...
> Will have v6 and add additional patch at the end to do enable/disable
> separately.
>
> Separating this out with additional patch before adding sensor support
> patch requires all patches to be updated.
>
> So I am ok either ways. Please let me know if adding additional patch at
> the end to split tegra_channel_set_stream() and tegra_csi_s_stream()
> separately is ok?
Should be okay, thanks!