Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve NXP CSI and MIPI_CSI2Rx drivers #76475

Merged
merged 17 commits into from
Oct 25, 2024
Merged

Improve NXP CSI and MIPI_CSI2Rx drivers #76475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
31e9847
dts-bindings: video: ov7725: Use video interfaces binding
ngphibang Oct 15, 2024
8507244
dts-bindings: video: mt9m114: Use video interfaces binding
ngphibang Oct 17, 2024
5af4c2b
dts-bindings: video: ov5640: Use video interfaces binding
ngphibang Oct 15, 2024
ac2e131
dts-bindings: video: mipicsi2rx: Use video interfaces bindings
ngphibang Oct 15, 2024
36be625
dts-bindings: video: csi: Use video interfaces bindings
ngphibang Oct 15, 2024
95c31de
boards: shields: nxp_btb44_ov5640: Add some endpoint properties
ngphibang Oct 15, 2024
0732cf1
drivers: video: mipi_csi2rx: Get data lanes number from devicetree
ngphibang Jun 19, 2024
5a2ddea
drivers: video: mipi_csi2rx: Drop sensor device phandle reference
ngphibang Oct 21, 2024
e95d199
drivers: video: mipi_csi2rx: Add set_ctrl callback
0xFarahFl Jul 16, 2024
7528267
drivers: video: mipi_csi2rx: Set clocks according to pixel rate
trunghieulenxp Jul 24, 2024
f4bb2da
drivers: video: mipi_csi2rx: Add support for changing frame rate
trunghieulenxp Jul 23, 2024
9a61086
include: video: Add an utility function to get bytes per pixel
ngphibang Oct 23, 2024
c957768
drivers: video: csi: Add support for changing frame rate
trunghieulenxp Jul 22, 2024
c9849af
drivers: video: csi: Remove obsolete comment
ngphibang Jul 24, 2024
aea820d
drivers: video: csi: Drop source device phandle reference
ngphibang Oct 21, 2024
361c47f
drivers: video: csi: Increase init priority
ngphibang Oct 21, 2024
d07370c
drivers: video: csi: Add NXP copyright
ngphibang Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions boards/madmachine/mm_swiftio/mm_swiftio.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@

port {
ov7725_ep_out: endpoint {
remote-endpoint = <&csi_ep_in>;
remote-endpoint-label = "csi_ep_in";
};
};
};
Expand Down Expand Up @@ -192,13 +192,12 @@

&csi {
status = "okay";
source = <&ov7725>;
pinctrl-0 = <&pinmux_csi>;
pinctrl-names = "default";

port {
csi_ep_in: endpoint {
remote-endpoint = <&ov7725_ep_out>;
remote-endpoint-label = "ov7725_ep_out";
};
};
};
Expand Down
9 changes: 0 additions & 9 deletions boards/shields/dvp_fpc24_mt9m114/boards/mimxrt1060_evkb.overlay
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions boards/shields/dvp_fpc24_mt9m114/boards/mimxrt1064_evk.overlay
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions boards/shields/dvp_fpc24_mt9m114/dvp_fpc24_mt9m114.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

port {
mt9m114_ep_out: endpoint {
remote-endpoint = <&dfi_ep_in>;
remote-endpoint-label = "dfi_ep_in";
};
};
};
Expand All @@ -28,7 +28,7 @@

port {
dfi_ep_in: endpoint {
remote-endpoint = <&mt9m114_ep_out>;
remote-endpoint-label = "mt9m114_ep_out";
};
};
};
11 changes: 7 additions & 4 deletions boards/shields/nxp_btb44_ov5640/nxp_btb44_ov5640.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/dt-bindings/video/video-interfaces.h>
ngphibang marked this conversation as resolved.
Show resolved Hide resolved

/{
chosen {
zephyr,camera = &nxp_csi;
Expand All @@ -21,7 +23,9 @@

port {
ov5640_ep_out: endpoint {
remote-endpoint = <&mipi_csi2rx_ep_in>;
Copy link
Collaborator

@danieldegrasse danieldegrasse Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand here, is the reason we want to use remote-endpoint-label over remote-endpoint that it avoids the "chicken-and-egg" dependency problem we have with display and camera drivers? I primarily ask because this deviates from how Linux describes CSI devices in devicetree, right?

Copy link
Contributor Author

@ngphibang ngphibang Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR, we need the video-interfaces binding for the mipi csi2rx simply to be able to use the data-lanes property and for the ov5640 to be able to use the bus-type (and data-lanes) property as the driver now supports both CSI2 and DVP modes. And the video-interfaces binding requires remote-endpoint-label (required: true) to be declared.

If the question is for the video-interfaces binding (which is merged), replacing remote-endpoint (phandle) by remote-endpoint-label (string) is mandatory as we cannot introduce any binding for endpoint without doing this because it simply does not compile (due to circular dependency).

I primarily ask because this deviates from how Linux describes CSI devices in devicetree, right?

Apart from the above reason, remote-endpoint-label also helps to bypass the technical obstacle in Zephyr (circular dependency). My thinking is in the opposite, it helps to keep Zephyr does NOT deviates from Linux. As you can see in Linux, they don't directly reference the peer devices in DT with direct phandle like we did in Zephyr (with "source-dev", "sensor-dev", etc). All is done through remote-endpoint (some advantages : it's bidirectional, can connect elements at runtime, etc.). And now we can do the same in Zephyr with remote-endpoint-label. Someday in the future, if Zephyr supports circular dependency, we can switch from remote-endpoint-label to remote-endpoint easily as all the mechanisms will remain the same (it's much easier than the direct phandle reference with source-dev, sensor-dev we did today).

remote-endpoint-label = "mipi_csi2rx_ep_in";
bus-type = <VIDEO_BUS_TYPE_CSI2_DPHY>;
data-lanes = <1 2>;
};
};
};
Expand All @@ -30,14 +34,13 @@
&nxp_mipi_csi {
status = "okay";

sensor = <&ov5640>;

ports {
port@1 {
reg = <1>;

mipi_csi2rx_ep_in: endpoint {
remote-endpoint = <&ov5640_ep_out>;
remote-endpoint-label = "ov5640_ep_out";
data-lanes = <1 2>;
};
};
};
Expand Down
23 changes: 23 additions & 0 deletions drivers/clock_control/clock_control_mcux_ccm_rev2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,19 @@ static int mcux_ccm_get_subsys_rate(const struct device *dev,
clock_root = kCLOCK_Root_Netc;
break;
#endif

#if defined(CONFIG_VIDEO_MCUX_MIPI_CSI2RX)
case IMX_CCM_MIPI_CSI2RX_ROOT_CLK:
clock_root = kCLOCK_Root_Csi2;
break;
case IMX_CCM_MIPI_CSI2RX_ESC_CLK:
clock_root = kCLOCK_Root_Csi2_Esc;
break;
case IMX_CCM_MIPI_CSI2RX_UI_CLK:
clock_root = kCLOCK_Root_Csi2_Ui;
break;
#endif

default:
return -EINVAL;
}
Expand Down Expand Up @@ -264,6 +277,16 @@ static int CCM_SET_FUNC_ATTR mcux_ccm_set_subsys_rate(const struct device *dev,
*/
return flexspi_clock_set_freq(clock_name, clock_rate);
#endif

#if defined(CONFIG_VIDEO_MCUX_MIPI_CSI2RX)
case IMX_CCM_MIPI_CSI2RX_ROOT_CLK:
return mipi_csi2rx_clock_set_freq(kCLOCK_Root_Csi2, clock_rate);
case IMX_CCM_MIPI_CSI2RX_UI_CLK:
return mipi_csi2rx_clock_set_freq(kCLOCK_Root_Csi2_Ui, clock_rate);
case IMX_CCM_MIPI_CSI2RX_ESC_CLK:
return mipi_csi2rx_clock_set_freq(kCLOCK_Root_Csi2_Esc, clock_rate);
#endif

default:
/* Silence unused variable warning */
ARG_UNUSED(clock_rate);
Expand Down
2 changes: 1 addition & 1 deletion drivers/video/Kconfig.mcux_csi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ config VIDEO_MCUX_CSI

config VIDEO_MCUX_CSI_INIT_PRIORITY
int "NXP MCUX CSI init priority"
default 61
default 59
depends on VIDEO_MCUX_CSI
help
Initialization priority for the CSI interface on an NXP MCUX device.
76 changes: 51 additions & 25 deletions drivers/video/video_mcux_csi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2019, Linaro Limited
* Copyright 2024 NXP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be in the first commit where we touch the CSI file in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, we touched the CSI driver several times in the past and I explictly added the NXP Copyright here when seeing that the driver is changed enough by NXP (although I don't know how much change is "enough" to add a copyright).

*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -17,6 +18,16 @@
#include <fsl_cache.h>
#endif

#if defined(CONFIG_VIDEO_MCUX_MIPI_CSI2RX)
#define DEVICE_DT_INST_GET_SOURCE_DEV(n) \
DEVICE_DT_GET(DT_PARENT(DT_GPARENT(DT_NODELABEL(DT_STRING_TOKEN( \
DT_CHILD(DT_INST_CHILD(n, port), endpoint), remote_endpoint_label)))))
#else
#define DEVICE_DT_INST_GET_SOURCE_DEV(n) \
DEVICE_DT_GET(DT_GPARENT(DT_NODELABEL(DT_STRING_TOKEN( \
DT_CHILD(DT_INST_CHILD(n, port), endpoint), remote_endpoint_label))))
#endif

struct video_mcux_csi_config {
CSI_Type *base;
const struct device *source_dev;
Expand All @@ -32,25 +43,6 @@ struct video_mcux_csi_data {
struct k_poll_signal *signal;
};

static inline unsigned int video_pix_fmt_bpp(uint32_t pixelformat)
ngphibang marked this conversation as resolved.
Show resolved Hide resolved
{
switch (pixelformat) {
case VIDEO_PIX_FMT_BGGR8:
case VIDEO_PIX_FMT_GBRG8:
case VIDEO_PIX_FMT_GRBG8:
case VIDEO_PIX_FMT_RGGB8:
return 1;
case VIDEO_PIX_FMT_RGB565:
case VIDEO_PIX_FMT_YUYV:
return 2;
case VIDEO_PIX_FMT_XRGB32:
case VIDEO_PIX_FMT_XYUV32:
return 4;
default:
return 0;
}
}

static void __frame_done_cb(CSI_Type *base, csi_handle_t *handle, status_t status, void *user_data)
{
struct video_mcux_csi_data *data = user_data;
Expand Down Expand Up @@ -450,6 +442,42 @@ static int video_mcux_csi_set_signal(const struct device *dev, enum video_endpoi
}
#endif

static int video_mcux_csi_set_frmival(const struct device *dev, enum video_endpoint_id ep,
struct video_frmival *frmival)
{
const struct video_mcux_csi_config *config = dev->config;

return video_set_frmival(config->source_dev, ep, frmival);
}

static int video_mcux_csi_get_frmival(const struct device *dev, enum video_endpoint_id ep,
struct video_frmival *frmival)
{
const struct video_mcux_csi_config *config = dev->config;

return video_get_frmival(config->source_dev, ep, frmival);
}

static int video_mcux_csi_enum_frmival(const struct device *dev, enum video_endpoint_id ep,
struct video_frmival_enum *fie)
{
const struct video_mcux_csi_config *config = dev->config;
const struct video_format *fie_fmt = fie->format;
int ret;

#if defined(CONFIG_VIDEO_MCUX_MIPI_CSI2RX)
struct video_format converted_fmt = *fie->format;

video_pix_fmt_convert(&converted_fmt, false);
fie->format = &converted_fmt;
#endif

ret = video_enum_frmival(config->source_dev, ep, fie);
fie->format = fie_fmt;

return ret;
}

static const struct video_driver_api video_mcux_csi_driver_api = {
.set_format = video_mcux_csi_set_fmt,
.get_format = video_mcux_csi_get_fmt,
Expand All @@ -461,6 +489,9 @@ static const struct video_driver_api video_mcux_csi_driver_api = {
.set_ctrl = video_mcux_csi_set_ctrl,
.get_ctrl = video_mcux_csi_get_ctrl,
.get_caps = video_mcux_csi_get_caps,
.set_frmival = video_mcux_csi_set_frmival,
.get_frmival = video_mcux_csi_get_frmival,
.enum_frmival = video_mcux_csi_enum_frmival,
#ifdef CONFIG_POLL
.set_signal = video_mcux_csi_set_signal,
#endif
Expand All @@ -471,7 +502,7 @@ PINCTRL_DT_INST_DEFINE(0);

static const struct video_mcux_csi_config video_mcux_csi_config_0 = {
.base = (CSI_Type *)DT_INST_REG_ADDR(0),
.source_dev = DEVICE_DT_GET(DT_INST_PHANDLE(0, source)),
.source_dev = DEVICE_DT_INST_GET_SOURCE_DEV(0),
.pincfg = PINCTRL_DT_INST_DEV_CONFIG_GET(0),
};

Expand All @@ -490,11 +521,6 @@ static int video_mcux_csi_init_0(const struct device *dev)
return video_mcux_csi_init(dev);
}

/* CONFIG_KERNEL_INIT_PRIORITY_DEVICE is used to make sure the
* CSI peripheral is initialized before the camera, which is
* necessary since the clock to the camera is provided by the
* CSI peripheral.
*/
DEVICE_DT_INST_DEFINE(0, &video_mcux_csi_init_0, NULL, &video_mcux_csi_data_0,
&video_mcux_csi_config_0, POST_KERNEL, CONFIG_VIDEO_MCUX_CSI_INIT_PRIORITY,
&video_mcux_csi_driver_api);
Expand Down
Loading
Loading