LinuxLists.cc - [PATCH v11 0/2] Add support for Xilinx CSI2 Receiver Subsystem

[permalink] [raw]

Subject: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
for image processing. Please refer to PG232 for details.

The CSI2 Rx controller filters out all packets except for the packets
with data type fixed in hardware. RAW8 packets are always allowed to
pass through.

It is also used to setup and handle interrupts and enable the core. It
logs all the events in respective counters between streaming on and off.

The driver supports only the video format bridge enabled configuration.
Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when the
CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
the maximum number of virtual channels becomes 16 from 4.

Signed-off-by: Vishal Sagar <[email protected]>
Reviewed-by: Hyun Kwon <[email protected]>
---
v11
- Fixed changes as suggested by Sakari Ailus
- Removed VIDEO_XILINX from KConfig
- Minor formatting
- Start / Stop upstream sub-device in xcsi2rxss_start_stream()
and xcsi2rxss_stop_stream()
- Added v4l2_subdev_link_validate_default() in v4l2_subdev_pad_ops()
- Use fwnode_graph_get_endpoint_by_id() instead of parsing by self
- Set bus type as V4L2_MBUS_CSI2_DPHY in struct v4l2_fwnode_endpoint
- Remove num_clks from core. Instead use ARRAY_SIZE()
- Fixed SPDX header to GPL-2.0
- Update copyright year to 2020

v10
- Removed all V4L2 controls and events based on Sakari's comments.
- Now stop_stream() before toggling rst_gpio
- Updated init_mbus() to throw error on array out of bound access
- Make events and vcx_events as counters instead of structures
- Minor fixes in set_format() enum_mbus_code() as suggested by Sakari

v9
- Moved all controls and events to xilinx-csi2rxss.h
- Updated name and description of controls and events
- Get control base address from v4l2-controls.h (0x10c0)
- Fix KConfig for dependency on VIDEO_XILINX
- Added enum_mbus_code() support
- Added default format to be returned on open()
- Mark variables are const
- Remove references to short packet in comments
- Add check for streaming before setting active lanes control
- strlcpy -> strscpy
- Fix xcsi2rxss_set_format()

v8
- Use clk_bulk* APIs
- Add gpio reset for asserting video_aresetn when stream line buffer occurs
- Removed short packet related events and irq handling
- V4L2_EVENT_XLNXCSIRX_SPKT and V4L2_EVENT_XLNXCSIRX_SPKT_OVF removed
- Removed frame counter control V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER
and xcsi2rxss_g_volatile_ctrl()
- Minor formatting fixes

v7
- No change

v6
- No change

v5
- Removed bayer and updated related parts like set default format based
on Luca Cersoli's comments.
- Added correct YUV422 10bpc media bus format

v4
- Removed irq member from core structure
- Consolidated IP config prints in xcsi2rxss_log_ipconfig()
- Return -EINVAL in case of invalid ioctl
- Code formatting
- Added reviewed by Hyun Kwon

v3
- Fixed comments given by Hyun.
- Removed DPHY 200 MHz clock. This will be controlled by DPHY driver
- Minor code formatting
- en_csi_v20 and vfb members removed from struct and made local to dt parsing
- lock description updated
- changed to ratelimited type for all dev prints in irq handler
- Removed YUV 422 10bpc media format

v2
- Fixed comments given by Hyun and Sakari.
- Made all bitmask using BIT() and GENMASK()
- Removed unused definitions
- Removed DPHY access. This will be done by separate DPHY PHY driver.
- Added support for CSI v2.0 for YUV 422 10bpc, RAW16, RAW20 and extra
virtual channels
- Fixed the ports as sink and source
- Now use the v4l2fwnode API to get number of data-lanes
- Added clock framework support
- Removed the close() function
- updated the set format function
- support only VFB enabled configuration

drivers/media/platform/xilinx/Kconfig | 10 +
drivers/media/platform/xilinx/Makefile | 1 +
.../media/platform/xilinx/xilinx-csi2rxss.c | 1234 +++++++++++++++++
3 files changed, 1245 insertions(+)
create mode 100644 drivers/media/platform/xilinx/xilinx-csi2rxss.c

diff --git a/drivers/media/platform/xilinx/Kconfig b/drivers/media/platform/xilinx/Kconfig
index a2773ad7c185..cd1a0fdde4df 100644
--- a/drivers/media/platform/xilinx/Kconfig
+++ b/drivers/media/platform/xilinx/Kconfig
@@ -10,6 +10,16 @@ config VIDEO_XILINX

if VIDEO_XILINX

+config VIDEO_XILINX_CSI2RXSS
+ tristate "Xilinx CSI2 Rx Subsystem"
+ help
+ Driver for Xilinx MIPI CSI2 Rx Subsystem. This is a V4L sub-device
+ based driver that takes input from CSI2 Tx source and converts
+ it into an AXI4-Stream. The subsystem comprises of a CSI2 Rx
+ controller, DPHY, an optional I2C controller and a Video Format
+ Bridge. The driver is used to set the number of active lanes and
+ get short packet data.
+
config VIDEO_XILINX_TPG
tristate "Xilinx Video Test Pattern Generator"
depends on VIDEO_XILINX
diff --git a/drivers/media/platform/xilinx/Makefile b/drivers/media/platform/xilinx/Makefile
index 4cdc0b1ec7a5..6119a34f3043 100644
--- a/drivers/media/platform/xilinx/Makefile
+++ b/drivers/media/platform/xilinx/Makefile
@@ -3,5 +3,6 @@
xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o

obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o
+obj-$(CONFIG_VIDEO_XILINX_CSI2RXSS) += xilinx-csi2rxss.o
obj-$(CONFIG_VIDEO_XILINX_TPG) += xilinx-tpg.o
obj-$(CONFIG_VIDEO_XILINX_VTC) += xilinx-vtc.o
diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
new file mode 100644
index 000000000000..083422768ebd
--- /dev/null
+++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
@@ -0,0 +1,1234 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Xilinx MIPI CSI2 Rx Subsystem
+ *
+ * Copyright (C) 2016 - 2020 Xilinx, Inc.
+ *
+ * Contacts: Vishal Sagar <[email protected]>
+ *
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/v4l2-subdev.h>
+#include <media/media-entity.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+#include "xilinx-vip.h"
+
+/* Register register map */
+#define XCSI_CCR_OFFSET 0x00
+#define XCSI_CCR_SOFTRESET BIT(1)
+#define XCSI_CCR_ENABLE BIT(0)
+
+#define XCSI_PCR_OFFSET 0x04
+#define XCSI_PCR_MAXLANES_MASK GENMASK(4, 3)
+#define XCSI_PCR_ACTLANES_MASK GENMASK(1, 0)
+
+#define XCSI_CSR_OFFSET 0x10
+#define XCSI_CSR_PKTCNT GENMASK(31, 16)
+#define XCSI_CSR_SPFIFOFULL BIT(3)
+#define XCSI_CSR_SPFIFONE BIT(2)
+#define XCSI_CSR_SLBF BIT(1)
+#define XCSI_CSR_RIPCD BIT(0)
+
+#define XCSI_GIER_OFFSET 0x20
+#define XCSI_GIER_GIE BIT(0)
+
+#define XCSI_ISR_OFFSET 0x24
+#define XCSI_IER_OFFSET 0x28
+
+#define XCSI_ISR_FR BIT(31)
+#define XCSI_ISR_VCXFE BIT(30)
+#define XCSI_ISR_WCC BIT(22)
+#define XCSI_ISR_ILC BIT(21)
+#define XCSI_ISR_SPFIFOF BIT(20)
+#define XCSI_ISR_SPFIFONE BIT(19)
+#define XCSI_ISR_SLBF BIT(18)
+#define XCSI_ISR_STOP BIT(17)
+#define XCSI_ISR_SOTERR BIT(13)
+#define XCSI_ISR_SOTSYNCERR BIT(12)
+#define XCSI_ISR_ECC2BERR BIT(11)
+#define XCSI_ISR_ECC1BERR BIT(10)
+#define XCSI_ISR_CRCERR BIT(9)
+#define XCSI_ISR_DATAIDERR BIT(8)
+#define XCSI_ISR_VC3FSYNCERR BIT(7)
+#define XCSI_ISR_VC3FLVLERR BIT(6)
+#define XCSI_ISR_VC2FSYNCERR BIT(5)
+#define XCSI_ISR_VC2FLVLERR BIT(4)
+#define XCSI_ISR_VC1FSYNCERR BIT(3)
+#define XCSI_ISR_VC1FLVLERR BIT(2)
+#define XCSI_ISR_VC0FSYNCERR BIT(1)
+#define XCSI_ISR_VC0FLVLERR BIT(0)
+
+#define XCSI_INTR_PROT_MASK (XCSI_ISR_VC3FSYNCERR | XCSI_ISR_VC3FLVLERR |\
+ XCSI_ISR_VC2FSYNCERR | XCSI_ISR_VC2FLVLERR |\
+ XCSI_ISR_VC1FSYNCERR | XCSI_ISR_VC1FLVLERR |\
+ XCSI_ISR_VC0FSYNCERR | XCSI_ISR_VC0FLVLERR |\
+ XCSI_ISR_VCXFE)
+
+#define XCSI_INTR_PKTLVL_MASK (XCSI_ISR_ECC2BERR | XCSI_ISR_ECC1BERR |\
+ XCSI_ISR_CRCERR | XCSI_ISR_DATAIDERR)
+
+#define XCSI_INTR_DPHY_MASK (XCSI_ISR_SOTERR | XCSI_ISR_SOTSYNCERR)
+
+#define XCSI_INTR_SPKT_MASK (XCSI_ISR_SPFIFOF | XCSI_ISR_SPFIFONE)
+
+#define XCSI_INTR_ERR_MASK (XCSI_ISR_WCC | XCSI_ISR_ILC | XCSI_ISR_SLBF |\
+ XCSI_ISR_STOP)
+
+#define XCSI_INTR_FRAMERCVD_MASK (XCSI_ISR_FR)
+
+#define XCSI_ISR_ALLINTR_MASK (XCSI_INTR_PROT_MASK | XCSI_INTR_PKTLVL_MASK |\
+ XCSI_INTR_DPHY_MASK | XCSI_INTR_SPKT_MASK |\
+ XCSI_INTR_ERR_MASK | XCSI_INTR_FRAMERCVD_MASK)
+
+/*
+ * Removed VCXFE mask as it doesn't exist in IER
+ * Removed STOP state irq as this will keep driver in irq handler only
+ */
+#define XCSI_IER_INTR_MASK (XCSI_ISR_ALLINTR_MASK &\
+ ~(XCSI_ISR_STOP | XCSI_ISR_VCXFE))
+
+#define XCSI_SPKTR_OFFSET 0x30
+#define XCSI_SPKTR_DATA GENMASK(23, 8)
+#define XCSI_SPKTR_VC GENMASK(7, 6)
+#define XCSI_SPKTR_DT GENMASK(5, 0)
+
+#define XCSI_VCXR_OFFSET 0x34
+#define XCSI_VCXR_VCERR GENMASK(23, 0)
+#define XCSI_VCXR_VCSTART 4
+#define XCSI_VCXR_VCEND 15
+#define XCSI_VCXR_FSYNCERR BIT(1)
+#define XCSI_VCXR_FLVLERR BIT(0)
+
+#define XCSI_CLKINFR_OFFSET 0x3C
+#define XCSI_CLKINFR_STOP BIT(1)
+
+#define XCSI_DLXINFR_OFFSET 0x40
+#define XCSI_DLXINFR_STOP BIT(5)
+#define XCSI_DLXINFR_SOTERR BIT(1)
+#define XCSI_DLXINFR_SOTSYNCERR BIT(0)
+#define XCSI_MAXDL_COUNT 0x4
+
+#define XCSI_VCXINF1R_OFFSET 0x60
+#define XCSI_VCXINF1R_LINECOUNT GENMASK(31, 16)
+#define XCSI_VCXINF1R_LINECOUNT_SHIFT 16
+#define XCSI_VCXINF1R_BYTECOUNT GENMASK(15, 0)
+
+#define XCSI_VCXINF2R_OFFSET 0x64
+#define XCSI_VCXINF2R_DT GENMASK(5, 0)
+#define XCSI_MAXVCX_COUNT 16
+
+/*
+ * The core takes less than 100 video clock cycles to reset.
+ * So choosing a timeout value larger than this.
+ */
+#define XCSI_TIMEOUT_VAL 1000 /* us */
+
+/*
+ * Sink pad connected to sensor source pad.
+ * Source pad connected to next module like demosaic.
+ */
+#define XCSI_MEDIA_PADS 2
+#define XCSI_DEFAULT_WIDTH 1920
+#define XCSI_DEFAULT_HEIGHT 1080
+
+/* Max media bus formats supported for enumeration */
+#define XCSI_MAX_MBUS_FMTS 16
+
+/* Max string length for CSI Data type string */
+#define XCSI_PXLFMT_STRLEN_MAX 16
+
+/* MIPI CSI2 Data Types from spec */
+#define XCSI_DT_YUV4228B 0x1E
+#define XCSI_DT_YUV42210B 0x1F
+#define XCSI_DT_RGB444 0x20
+#define XCSI_DT_RGB555 0x21
+#define XCSI_DT_RGB565 0x22
+#define XCSI_DT_RGB666 0x23
+#define XCSI_DT_RGB888 0x24
+#define XCSI_DT_RAW6 0x28
+#define XCSI_DT_RAW7 0x29
+#define XCSI_DT_RAW8 0x2A
+#define XCSI_DT_RAW10 0x2B
+#define XCSI_DT_RAW12 0x2C
+#define XCSI_DT_RAW14 0x2D
+#define XCSI_DT_RAW16 0x2E
+#define XCSI_DT_RAW20 0x2F
+
+#define XCSI_VCX_START 4
+#define XCSI_MAX_VC 4
+#define XCSI_MAX_VCX 16
+
+#define XCSI_NEXTREG_OFFSET 4
+
+/* There are 2 events frame sync and frame level error per VC */
+#define XCSI_VCX_NUM_EVENTS ((XCSI_MAX_VCX - XCSI_MAX_VC) * 2)
+
+/* Macro to return "true" or "false" string if bit is set */
+#define XCSI_GET_BITSET_STR(val, mask) (val) & (mask) ? "true" : "false"
+
+#define XADD_MBUS(state, mbus_fmt) \
+ do { \
+ if ((state)->mbus_fmts_count < XCSI_MAX_MBUS_FMTS) { \
+ (state)->mbus_fmts[(state)->mbus_fmts_count++] =\
+ (mbus_fmt); \
+ } else { \
+ dev_err((state)->core.dev, \
+ "accessing array out of bounds!\n"); \
+ } \
+ } while (0)
+
+/**
+ * struct xcsi2rxss_event - Event log structure
+ * @mask: Event mask
+ * @name: Name of the event
+ */
+struct xcsi2rxss_event {
+ u32 mask;
+ const char *name;
+};
+
+static const struct xcsi2rxss_event xcsi2rxss_events[] = {
+ { XCSI_ISR_FR, "Frame Received" },
+ { XCSI_ISR_VCXFE, "VCX Frame Errors" },
+ { XCSI_ISR_WCC, "Word Count Errors" },
+ { XCSI_ISR_ILC, "Invalid Lane Count Error" },
+ { XCSI_ISR_SPFIFOF, "Short Packet FIFO OverFlow Error" },
+ { XCSI_ISR_SPFIFONE, "Short Packet FIFO Not Empty" },
+ { XCSI_ISR_SLBF, "Streamline Buffer Full Error" },
+ { XCSI_ISR_STOP, "Lane Stop State" },
+ { XCSI_ISR_SOTERR, "SOT Error" },
+ { XCSI_ISR_SOTSYNCERR, "SOT Sync Error" },
+ { XCSI_ISR_ECC2BERR, "2 Bit ECC Unrecoverable Error" },
+ { XCSI_ISR_ECC1BERR, "1 Bit ECC Recoverable Error" },
+ { XCSI_ISR_CRCERR, "CRC Error" },
+ { XCSI_ISR_DATAIDERR, "Data Id Error" },
+ { XCSI_ISR_VC3FSYNCERR, "Virtual Channel 3 Frame Sync Error" },
+ { XCSI_ISR_VC3FLVLERR, "Virtual Channel 3 Frame Level Error" },
+ { XCSI_ISR_VC2FSYNCERR, "Virtual Channel 2 Frame Sync Error" },
+ { XCSI_ISR_VC2FLVLERR, "Virtual Channel 2 Frame Level Error" },
+ { XCSI_ISR_VC1FSYNCERR, "Virtual Channel 1 Frame Sync Error" },
+ { XCSI_ISR_VC1FLVLERR, "Virtual Channel 1 Frame Level Error" },
+ { XCSI_ISR_VC0FSYNCERR, "Virtual Channel 0 Frame Sync Error" },
+ { XCSI_ISR_VC0FLVLERR, "Virtual Channel 0 Frame Level Error" }
+};
+
+#define XCSI_NUM_EVENTS ARRAY_SIZE(xcsi2rxss_events)
+
+/*
+ * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device structure
+ * @dev: Platform structure
+ * @iomem: Base address of subsystem
+ * @enable_active_lanes: If number of active lanes can be modified
+ * @max_num_lanes: Maximum number of lanes present
+ * @datatype: Data type filter
+ * @events: counter for events
+ * @vcx_events: counter for vcx_events
+ * @en_vcx: If more than 4 VC are enabled
+ * @clks: array of clocks
+ * @rst_gpio: reset to video_aresetn
+ */
+struct xcsi2rxss_core {
+ struct device *dev;
+ void __iomem *iomem;
+ bool enable_active_lanes;
+ u32 max_num_lanes;
+ u32 datatype;
+ u32 events[XCSI_NUM_EVENTS];
+ u32 vcx_events[XCSI_VCX_NUM_EVENTS];
+ bool en_vcx;
+ struct clk_bulk_data *clks;
+ struct gpio_desc *rst_gpio;
+};
+
+/**
+ * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
+ * @core: Core structure for MIPI CSI2 Rx Subsystem
+ * @subdev: The v4l2 subdev structure
+ * @format: Active V4L2 formats on each pad
+ * @default_format: Default V4L2 format
+ * @lock: mutex for accessing this structure
+ * @pads: media pads
+ * @mbus_fmts: List of media bus formats for enum_mbus_code
+ * @mbus_fmts_count: Number of media bus formats
+ * @streaming: Flag for storing streaming state
+ *
+ * This structure contains the device driver related parameters
+ */
+struct xcsi2rxss_state {
+ struct xcsi2rxss_core core;
+ struct v4l2_subdev subdev;
+ struct v4l2_mbus_framefmt format;
+ struct v4l2_mbus_framefmt default_format;
+ /* used to protect access to this struct */
+ struct mutex lock;
+ struct media_pad pads[XCSI_MEDIA_PADS];
+ u32 mbus_fmts[XCSI_MAX_MBUS_FMTS];
+ u32 mbus_fmts_count;
+ bool streaming;
+};
+
+static const struct clk_bulk_data xcsi2rxss_clks[] = {
+ { .id = "lite_aclk" },
+ { .id = "video_aclk" },
+};
+
+static inline struct xcsi2rxss_state *
+to_xcsi2rxssstate(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct xcsi2rxss_state, subdev);
+}
+
+/*
+ * Register related operations
+ */
+static inline u32 xcsi2rxss_read(struct xcsi2rxss_core *xcsi2rxss, u32 addr)
+{
+ return ioread32(xcsi2rxss->iomem + addr);
+}
+
+static inline void xcsi2rxss_write(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
+ u32 value)
+{
+ iowrite32(value, xcsi2rxss->iomem + addr);
+}
+
+static inline void xcsi2rxss_clr(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
+ u32 clr)
+{
+ xcsi2rxss_write(xcsi2rxss, addr,
+ xcsi2rxss_read(xcsi2rxss, addr) & ~clr);
+}
+
+static inline void xcsi2rxss_set(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
+ u32 set)
+{
+ xcsi2rxss_write(xcsi2rxss, addr, xcsi2rxss_read(xcsi2rxss, addr) | set);
+}
+
+static void xcsi2rxss_enable(struct xcsi2rxss_core *core)
+{
+ xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
+}
+
+static void xcsi2rxss_disable(struct xcsi2rxss_core *core)
+{
+ xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
+}
+
+static void xcsi2rxss_intr_enable(struct xcsi2rxss_core *core)
+{
+ xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
+ xcsi2rxss_write(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
+ xcsi2rxss_set(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
+}
+
+static void xcsi2rxss_intr_disable(struct xcsi2rxss_core *core)
+{
+ xcsi2rxss_clr(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
+ xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
+}
+
+/**
+ * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
+ * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
+ *
+ * Core takes less than 100 video clock cycles to reset.
+ * So a larger timeout value is chosen for margin.
+ *
+ * Return: 0 - on success OR -ETIME if reset times out
+ */
+static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
+{
+ u32 timeout = XCSI_TIMEOUT_VAL;
+
+ xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
+
+ while (xcsi2rxss_read(core, XCSI_CSR_OFFSET) & XCSI_CSR_RIPCD) {
+ if (timeout == 0) {
+ dev_err(core->dev, "soft reset timed out!\n");
+ return -ETIME;
+ }
+
+ timeout--;
+ udelay(1);
+ }
+
+ xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
+ return 0;
+}
+
+static void xcsi2rxss_reset_event_counters(struct xcsi2rxss_state *state)
+{
+ unsigned int i;
+
+ for (i = 0; i < XCSI_NUM_EVENTS; i++)
+ state->core.events[i] = 0;
+
+ for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++)
+ state->core.vcx_events[i] = 0;
+}
+
+/* Print event counters */
+static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+ unsigned int i;
+
+ for (i = 0; i < XCSI_NUM_EVENTS; i++) {
+ if (core->events[i] > 0) {
+ dev_info(core->dev, "%s events: %d\n",
+ xcsi2rxss_events[i].name,
+ core->events[i]);
+ }
+ }
+
+ if (core->en_vcx) {
+ for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
+ if (core->vcx_events[i] > 0) {
+ dev_info(core->dev,
+ "VC %d Frame %s err vcx events: %d\n",
+ (i / 2) + XCSI_VCX_START,
+ i & 1 ? "Sync" : "Level",
+ core->vcx_events[i]);
+ }
+ }
+ }
+}
+
+static void xcsi2rxss_log_ipconfig(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+
+ dev_dbg(core->dev, "****** Xilinx MIPI CSI2 Rx SS IP Config ******\n");
+ dev_dbg(core->dev, "vcx is %s", core->en_vcx ? "enabled" : "disabled");
+ dev_dbg(core->dev, "Enable active lanes property is %s\n",
+ core->enable_active_lanes ? "present" : "absent");
+ dev_dbg(core->dev, "Max lanes = %d", core->max_num_lanes);
+ dev_dbg(core->dev, "Pixel format set as 0x%x\n", core->datatype);
+ dev_dbg(core->dev, "**********************************************\n");
+}
+
+/**
+ * xcsi2rxss_log_status - Logs the status of the CSI-2 Receiver
+ * @sd: Pointer to V4L2 subdevice structure
+ *
+ * This function prints the current status of Xilinx MIPI CSI-2
+ *
+ * Return: 0 on success
+ */
+static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
+{
+ struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+ struct xcsi2rxss_core *core = &xcsi2rxss->core;
+ u32 reg, data;
+ unsigned int i, max_vc;
+
+ mutex_lock(&xcsi2rxss->lock);
+
+ xcsi2rxss_log_ipconfig(xcsi2rxss);
+
+ xcsi2rxss_log_counters(xcsi2rxss);
+
+ dev_info(core->dev, "***** Core Status *****\n");
+ data = xcsi2rxss_read(core, XCSI_CSR_OFFSET);
+ dev_info(core->dev, "Short Packet FIFO Full = %s\n",
+ XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFOFULL));
+ dev_info(core->dev, "Short Packet FIFO Not Empty = %s\n",
+ XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFONE));
+ dev_info(core->dev, "Stream line buffer full = %s\n",
+ XCSI_GET_BITSET_STR(data, XCSI_CSR_SLBF));
+ dev_info(core->dev, "Soft reset/Core disable in progress = %s\n",
+ XCSI_GET_BITSET_STR(data, XCSI_CSR_RIPCD));
+
+ /* Clk & Lane Info */
+ dev_info(core->dev, "******** Clock Lane Info *********\n");
+ data = xcsi2rxss_read(core, XCSI_CLKINFR_OFFSET);
+ dev_info(core->dev, "Clock Lane in Stop State = %s\n",
+ XCSI_GET_BITSET_STR(data, XCSI_CLKINFR_STOP));
+
+ dev_info(core->dev, "******** Data Lane Info *********\n");
+ dev_info(core->dev, "Lane\tSoT Error\tSoT Sync Error\tStop State\n");
+ reg = XCSI_DLXINFR_OFFSET;
+ for (i = 0; i < XCSI_MAXDL_COUNT; i++) {
+ data = xcsi2rxss_read(core, reg);
+
+ dev_info(core->dev, "%d\t%s\t\t%s\t\t%s\n", i,
+ XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTERR),
+ XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTSYNCERR),
+ XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_STOP));
+
+ reg += XCSI_NEXTREG_OFFSET;
+ }
+
+ /* Virtual Channel Image Information */
+ dev_info(core->dev, "********** Virtual Channel Info ************\n");
+ dev_info(core->dev, "VC\tLine Count\tByte Count\tData Type\n");
+ if (core->en_vcx)
+ max_vc = XCSI_MAX_VCX;
+ else
+ max_vc = XCSI_MAX_VC;
+
+ reg = XCSI_VCXINF1R_OFFSET;
+ for (i = 0; i < max_vc; i++) {
+ u32 line_count, byte_count, data_type;
+
+ /* Get line and byte count from VCXINFR1 Register */
+ data = xcsi2rxss_read(core, reg);
+ byte_count = data & XCSI_VCXINF1R_BYTECOUNT;
+ line_count = data & XCSI_VCXINF1R_LINECOUNT;
+ line_count >>= XCSI_VCXINF1R_LINECOUNT_SHIFT;
+
+ /* Get data type from VCXINFR2 Register */
+ reg += XCSI_NEXTREG_OFFSET;
+ data = xcsi2rxss_read(core, reg);
+ data_type = data & XCSI_VCXINF2R_DT;
+
+ dev_info(core->dev, "%d\t%d\t\t%d\t\t0x%x\n", i, line_count,
+ byte_count, data_type);
+
+ /* Move to next pair of VC Info registers */
+ reg += XCSI_NEXTREG_OFFSET;
+ }
+
+ mutex_unlock(&xcsi2rxss->lock);
+
+ return 0;
+}
+
+static struct v4l2_subdev *xcsi2rxss_get_remote_subdev(struct media_pad *local)
+{
+ struct media_pad *remote;
+
+ remote = media_entity_remote_pad(local);
+ if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
+ return NULL;
+
+ return media_entity_to_v4l2_subdev(remote->entity);
+}
+
+static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+ struct v4l2_subdev *rsubdev;
+ int ret = 0;
+
+ xcsi2rxss_enable(core);
+
+ ret = xcsi2rxss_reset(core);
+ if (ret < 0) {
+ state->streaming = false;
+ return ret;
+ }
+
+ xcsi2rxss_intr_enable(core);
+ state->streaming = true;
+
+ rsubdev = xcsi2rxss_get_remote_subdev(&state->pads[XVIP_PAD_SINK]);
+ ret = v4l2_subdev_call(rsubdev, video, s_stream, 1);
+
+ return ret;
+}
+
+static void xcsi2rxss_stop_stream(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+ struct v4l2_subdev *rsubdev;
+
+ rsubdev = xcsi2rxss_get_remote_subdev(&state->pads[XVIP_PAD_SINK]);
+ v4l2_subdev_call(rsubdev, video, s_stream, 0);
+
+ xcsi2rxss_intr_disable(core);
+ xcsi2rxss_disable(core);
+ state->streaming = false;
+}
+
+/**
+ * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
+ * @irq: IRQ number
+ * @dev_id: Pointer to device state
+ *
+ * In the interrupt handler, a list of event counters are updated for
+ * corresponding interrupts. This is useful to get status / debug.
+ *
+ * Return: IRQ_HANDLED after handling interrupts
+ * IRQ_NONE is no interrupts
+ */
+static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
+{
+ struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
+ struct xcsi2rxss_core *core = &state->core;
+ u32 status;
+
+ status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
+ dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
+
+ if (!status)
+ return IRQ_NONE;
+
+ /* Received a short packet */
+ if (status & XCSI_ISR_SPFIFONE) {
+ dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
+ xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
+ }
+
+ /* Short packet FIFO overflow */
+ if (status & XCSI_ISR_SPFIFOF)
+ dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
+
+ /*
+ * Stream line buffer full
+ * This means there is a backpressure from downstream IP
+ */
+ if (status & XCSI_ISR_SLBF) {
+ dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
+ xcsi2rxss_stop_stream(state);
+ if (core->rst_gpio) {
+ gpiod_set_value(core->rst_gpio, 1);
+ /* minimum 40 dphy_clk_200M cycles */
+ ndelay(250);
+ gpiod_set_value(core->rst_gpio, 0);
+ }
+ }
+
+ /* Increment event counters */
+ if (status & XCSI_ISR_ALLINTR_MASK) {
+ unsigned int i;
+
+ for (i = 0; i < XCSI_NUM_EVENTS; i++) {
+ if (!(status & xcsi2rxss_events[i].mask))
+ continue;
+ core->events[i]++;
+ dev_dbg_ratelimited(core->dev, "%s: %d\n",
+ xcsi2rxss_events[i].name,
+ core->events[i]);
+ }
+
+ if (status & XCSI_ISR_VCXFE && core->en_vcx) {
+ u32 vcxstatus;
+
+ vcxstatus = xcsi2rxss_read(core, XCSI_VCXR_OFFSET);
+ vcxstatus &= XCSI_VCXR_VCERR;
+ for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
+ if (!(vcxstatus & (1 << i)))
+ continue;
+ core->vcx_events[i]++;
+ }
+ xcsi2rxss_write(core, XCSI_VCXR_OFFSET, vcxstatus);
+ }
+ }
+
+ xcsi2rxss_write(core, XCSI_ISR_OFFSET, status);
+ return IRQ_HANDLED;
+}
+
+/**
+ * xcsi2rxss_s_stream - It is used to start/stop the streaming.
+ * @sd: V4L2 Sub device
+ * @enable: Flag (True / False)
+ *
+ * This function controls the start or stop of streaming for the
+ * Xilinx MIPI CSI-2 Rx Subsystem.
+ *
+ * Return: 0 on success, errors otherwise
+ */
+static int xcsi2rxss_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+ int ret = 0;
+
+ mutex_lock(&xcsi2rxss->lock);
+
+ if (enable) {
+ if (!xcsi2rxss->streaming) {
+ /* reset the event counters */
+ xcsi2rxss_reset_event_counters(xcsi2rxss);
+ ret = xcsi2rxss_start_stream(xcsi2rxss);
+ }
+ } else {
+ if (xcsi2rxss->streaming) {
+ struct gpio_desc *rst = xcsi2rxss->core.rst_gpio;
+
+ xcsi2rxss_stop_stream(xcsi2rxss);
+ if (rst) {
+ gpiod_set_value_cansleep(rst, 1);
+ usleep_range(1, 2);
+ gpiod_set_value_cansleep(rst, 0);
+ }
+ }
+ }
+
+ mutex_unlock(&xcsi2rxss->lock);
+ return ret;
+}
+
+static struct v4l2_mbus_framefmt *
+__xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
+ struct v4l2_subdev_pad_config *cfg,
+ unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(&xcsi2rxss->subdev, cfg, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &xcsi2rxss->format;
+ default:
+ return NULL;
+ }
+}
+
+/**
+ * xcsi2rxss_get_format - Get the pad format
+ * @sd: Pointer to V4L2 Sub device structure
+ * @cfg: Pointer to sub device pad information structure
+ * @fmt: Pointer to pad level media bus format
+ *
+ * This function is used to get the pad format information.
+ *
+ * Return: 0 on success
+ */
+static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+
+ mutex_lock(&xcsi2rxss->lock);
+ fmt->format = *__xcsi2rxss_get_pad_format(xcsi2rxss, cfg, fmt->pad,
+ fmt->which);
+ mutex_unlock(&xcsi2rxss->lock);
+
+ return 0;
+}
+
+/**
+ * xcsi2rxss_set_format - This is used to set the pad format
+ * @sd: Pointer to V4L2 Sub device structure
+ * @cfg: Pointer to sub device pad information structure
+ * @fmt: Pointer to pad level media bus format
+ *
+ * This function is used to set the pad format. Since the pad format is fixed
+ * in hardware, it can't be modified on run time. So when a format set is
+ * requested by application, all parameters except the format type is saved
+ * for the pad and the original pad format is sent back to the application.
+ *
+ * Return: 0 on success
+ */
+static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+ struct xcsi2rxss_core *core = &xcsi2rxss->core;
+ struct v4l2_mbus_framefmt *__format;
+
+ /* only sink pad format can be updated */
+ mutex_lock(&xcsi2rxss->lock);
+
+ /*
+ * Only the format->code parameter matters for CSI as the
+ * CSI format cannot be changed at runtime.
+ * Ensure that format to set is copied to over to CSI pad format
+ */
+ __format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
+ fmt->pad, fmt->which);
+
+ if (fmt->pad == XVIP_PAD_SOURCE) {
+ fmt->format = *__format;
+ mutex_unlock(&xcsi2rxss->lock);
+ return 0;
+ }
+
+ /*
+ * RAW8 is supported in all datatypes. So if requested media bus format
+ * is of RAW8 type, then allow to be set. In case core is configured to
+ * other RAW, YUV422 8/10 or RGB888, set appropriate media bus format.
+ */
+ if (!((fmt->format.code == MEDIA_BUS_FMT_SBGGR8_1X8 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGBRG8_1X8 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGRBG8_1X8 ||
+ fmt->format.code == MEDIA_BUS_FMT_SRGGB8_1X8) ||
+ (core->datatype == XCSI_DT_RAW10 &&
+ (fmt->format.code == MEDIA_BUS_FMT_SBGGR10_1X10 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
+ fmt->format.code == MEDIA_BUS_FMT_SRGGB10_1X10)) ||
+ (core->datatype == XCSI_DT_RAW12 &&
+ (fmt->format.code == MEDIA_BUS_FMT_SBGGR12_1X12 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGBRG12_1X12 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGRBG12_1X12 ||
+ fmt->format.code == MEDIA_BUS_FMT_SRGGB12_1X12)) ||
+ (core->datatype == XCSI_DT_RAW14 &&
+ (fmt->format.code == MEDIA_BUS_FMT_SBGGR14_1X14 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGBRG14_1X14 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGRBG14_1X14 ||
+ fmt->format.code == MEDIA_BUS_FMT_SRGGB14_1X14)) ||
+ (core->datatype == XCSI_DT_RAW16 &&
+ (fmt->format.code == MEDIA_BUS_FMT_SBGGR16_1X16 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGBRG16_1X16 ||
+ fmt->format.code == MEDIA_BUS_FMT_SGRBG16_1X16 ||
+ fmt->format.code == MEDIA_BUS_FMT_SRGGB16_1X16)) ||
+ (core->datatype == XCSI_DT_YUV4228B &&
+ fmt->format.code == MEDIA_BUS_FMT_UYVY8_1X16) ||
+ (core->datatype == XCSI_DT_YUV42210B &&
+ fmt->format.code == MEDIA_BUS_FMT_UYVY10_1X20) ||
+ (core->datatype == XCSI_DT_RGB888 &&
+ fmt->format.code == MEDIA_BUS_FMT_RBG888_1X24))) {
+ /* Restore the original pad format code */
+ dev_dbg(core->dev, "Unsupported media bus format");
+ fmt->format.code = __format->code;
+ }
+
+ *__format = fmt->format;
+ mutex_unlock(&xcsi2rxss->lock);
+
+ return 0;
+}
+
+/*
+ * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
+ * @sd : pointer to v4l2 subdev structure
+ * @cfg: V4L2 subdev pad configuration
+ * @code : pointer to v4l2_subdev_mbus_code_enum structure
+ *
+ * Return: -EINVAL or zero on success
+ */
+int xcsi2rxss_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct xcsi2rxss_state *state = to_xcsi2rxssstate(sd);
+
+ if (code->index >= state->mbus_fmts_count)
+ return -EINVAL;
+
+ code->code = state->mbus_fmts[code->index];
+
+ return 0;
+}
+
+/**
+ * xcsi2rxss_open - Called on v4l2_open()
+ * @sd: Pointer to V4L2 sub device structure
+ * @fh: Pointer to V4L2 File handle
+ *
+ * This function is called on v4l2_open(). It sets the default format
+ * for both pads.
+ *
+ * Return: 0 on success
+ */
+static int xcsi2rxss_open(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh)
+{
+ struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+ struct v4l2_mbus_framefmt *format;
+ unsigned int i;
+
+ for (i = 0; i < XCSI_MEDIA_PADS; i++) {
+ format = v4l2_subdev_get_try_format(sd, fh->pad, i);
+ *format = xcsi2rxss->default_format;
+ }
+
+ return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * Media Operations
+ */
+
+static const struct media_entity_operations xcsi2rxss_media_ops = {
+ .link_validate = v4l2_subdev_link_validate
+};
+
+static const struct v4l2_subdev_core_ops xcsi2rxss_core_ops = {
+ .log_status = xcsi2rxss_log_status,
+};
+
+static const struct v4l2_subdev_video_ops xcsi2rxss_video_ops = {
+ .s_stream = xcsi2rxss_s_stream
+};
+
+static const struct v4l2_subdev_pad_ops xcsi2rxss_pad_ops = {
+ .get_fmt = xcsi2rxss_get_format,
+ .set_fmt = xcsi2rxss_set_format,
+ .enum_mbus_code = xcsi2rxss_enum_mbus_code,
+ .link_validate = v4l2_subdev_link_validate_default,
+};
+
+static const struct v4l2_subdev_ops xcsi2rxss_ops = {
+ .core = &xcsi2rxss_core_ops,
+ .video = &xcsi2rxss_video_ops,
+ .pad = &xcsi2rxss_pad_ops
+};
+
+static const struct v4l2_subdev_internal_ops xcsi2rxss_internal_ops = {
+ .open = xcsi2rxss_open,
+};
+
+static void xcsi2rxss_set_default_format(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+
+ memset(&state->default_format, 0, sizeof(state->default_format));
+
+ switch (core->datatype) {
+ case XCSI_DT_YUV4228B:
+ state->default_format.code = MEDIA_BUS_FMT_UYVY8_1X16;
+ break;
+ case XCSI_DT_RGB888:
+ state->default_format.code = MEDIA_BUS_FMT_RBG888_1X24;
+ break;
+ case XCSI_DT_YUV42210B:
+ state->default_format.code = MEDIA_BUS_FMT_UYVY10_1X20;
+ break;
+ case XCSI_DT_RAW10:
+ state->default_format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+ break;
+ case XCSI_DT_RAW12:
+ state->default_format.code = MEDIA_BUS_FMT_SRGGB12_1X12;
+ break;
+ case XCSI_DT_RAW14:
+ state->default_format.code = MEDIA_BUS_FMT_SRGGB14_1X14;
+ break;
+ case XCSI_DT_RAW16:
+ state->default_format.code = MEDIA_BUS_FMT_SRGGB16_1X16;
+ break;
+ case XCSI_DT_RAW8:
+ case XCSI_DT_RGB444:
+ case XCSI_DT_RGB555:
+ case XCSI_DT_RGB565:
+ case XCSI_DT_RGB666:
+ state->default_format.code = MEDIA_BUS_FMT_SRGGB8_1X8;
+ break;
+ }
+
+ state->default_format.field = V4L2_FIELD_NONE;
+ state->default_format.colorspace = V4L2_COLORSPACE_SRGB;
+ state->default_format.width = XCSI_DEFAULT_WIDTH;
+ state->default_format.height = XCSI_DEFAULT_HEIGHT;
+
+ dev_dbg(core->dev, "default mediabus format = 0x%x",
+ state->default_format.code);
+}
+
+static void xcsi2rxss_init_mbus_fmts(struct xcsi2rxss_state *state)
+{
+ struct xcsi2rxss_core *core = &state->core;
+
+ XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB8_1X8);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR8_1X8);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG8_1X8);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG8_1X8);
+
+ switch (core->datatype) {
+ case XCSI_DT_RAW10:
+ XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB10_1X10);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR10_1X10);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG10_1X10);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG10_1X10);
+ break;
+ case XCSI_DT_RAW12:
+ XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB12_1X12);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR12_1X12);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG12_1X12);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG12_1X12);
+ break;
+ case XCSI_DT_RAW14:
+ XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB14_1X14);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR14_1X14);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG14_1X14);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG14_1X14);
+ break;
+ case XCSI_DT_RAW16:
+ XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB16_1X16);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR16_1X16);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG16_1X16);
+ XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG16_1X16);
+ break;
+ case XCSI_DT_YUV4228B:
+ XADD_MBUS(state, MEDIA_BUS_FMT_UYVY8_1X16);
+ break;
+ case XCSI_DT_RGB888:
+ XADD_MBUS(state, MEDIA_BUS_FMT_RBG888_1X24);
+ break;
+ case XCSI_DT_YUV42210B:
+ XADD_MBUS(state, MEDIA_BUS_FMT_UYVY10_1X20);
+ break;
+ default:
+ dev_err(core->dev, "Invalid data type!\n");
+ }
+}
+
+static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
+{
+ struct xcsi2rxss_core *core = &xcsi2rxss->core;
+ struct device_node *node = xcsi2rxss->core.dev->of_node;
+ unsigned int nports, irq;
+ bool en_csi_v20, vfb;
+ int ret;
+
+ en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
+ if (en_csi_v20)
+ core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx");
+
+ core->enable_active_lanes =
+ of_property_read_bool(node, "xlnx,en-active-lanes");
+
+ ret = of_property_read_u32(node, "xlnx,csi-pxl-format",
+ &core->datatype);
+ if (ret < 0) {
+ dev_err(core->dev, "missing xlnx,csi-pxl-format property\n");
+ return ret;
+ }
+
+ switch (core->datatype) {
+ case XCSI_DT_YUV4228B:
+ case XCSI_DT_RGB444:
+ case XCSI_DT_RGB555:
+ case XCSI_DT_RGB565:
+ case XCSI_DT_RGB666:
+ case XCSI_DT_RGB888:
+ case XCSI_DT_RAW6:
+ case XCSI_DT_RAW7:
+ case XCSI_DT_RAW8:
+ case XCSI_DT_RAW10:
+ case XCSI_DT_RAW12:
+ case XCSI_DT_RAW14:
+ break;
+ case XCSI_DT_YUV42210B:
+ case XCSI_DT_RAW16:
+ case XCSI_DT_RAW20:
+ if (!en_csi_v20) {
+ ret = -EINVAL;
+ dev_dbg(core->dev, "enable csi v2 for this pixel format");
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ if (ret < 0) {
+ dev_err(core->dev, "invalid csi-pxl-format property!\n");
+ return ret;
+ }
+
+ vfb = of_property_read_bool(node, "xlnx,vfb");
+ if (!vfb) {
+ dev_err(core->dev, "failed as VFB is disabled!\n");
+ return -EINVAL;
+ }
+
+ for (nports = 0; nports < XCSI_MEDIA_PADS; nports++) {
+ struct fwnode_handle *ep;
+ struct v4l2_fwnode_endpoint vep = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
+
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(core->dev),
+ nports, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT);
+ if (!ep)
+ break;
+ /*
+ * since first port is sink port and it contains
+ * all info about data-lanes and cfa-pattern,
+ * don't parse second port but only check if exists
+ */
+ if (nports == XVIP_PAD_SOURCE) {
+ dev_dbg(core->dev, "no need to parse source port");
+ fwnode_handle_put(ep);
+ continue;
+ }
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ if (ret) {
+ dev_err(core->dev, "error parsing sink port");
+ fwnode_handle_put(ep);
+ return ret;
+ }
+
+ dev_dbg(core->dev, "port %d bus type = %d\n", nports,
+ vep.bus_type);
+
+ if (vep.bus_type == V4L2_MBUS_CSI2_DPHY) {
+ dev_dbg(core->dev, "base.port = %d base.id = %d\n",
+ vep.base.port, vep.base.id);
+
+ dev_dbg(core->dev, "mipi number lanes = %d\n",
+ vep.bus.mipi_csi2.num_data_lanes);
+
+ core->max_num_lanes =
+ vep.bus.mipi_csi2.num_data_lanes;
+ }
+ fwnode_handle_put(ep);
+ }
+
+ if (nports != XCSI_MEDIA_PADS) {
+ dev_err(core->dev, "invalid number of ports %u\n", nports);
+ return -EINVAL;
+ }
+
+ /* Register interrupt handler */
+ irq = irq_of_parse_and_map(node, 0);
+ ret = devm_request_irq(core->dev, irq, xcsi2rxss_irq_handler,
+ IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss);
+ if (ret) {
+ dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n",
+ ret);
+ return ret;
+ }
+
+ xcsi2rxss_log_ipconfig(xcsi2rxss);
+
+ return 0;
+}
+
+static int xcsi2rxss_probe(struct platform_device *pdev)
+{
+ struct v4l2_subdev *subdev;
+ struct xcsi2rxss_state *xcsi2rxss;
+ struct xcsi2rxss_core *core;
+ struct resource *res;
+ int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
+ int ret;
+
+ xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL);
+ if (!xcsi2rxss)
+ return -ENOMEM;
+
+ core = &xcsi2rxss->core;
+ core->dev = &pdev->dev;
+
+ core->clks = devm_kmemdup(core->dev, xcsi2rxss_clks,
+ sizeof(xcsi2rxss_clks), GFP_KERNEL);
+ if (!core->clks)
+ return -ENOMEM;
+
+ /* Reset GPIO */
+ core->rst_gpio = devm_gpiod_get_optional(core->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(core->rst_gpio)) {
+ if (PTR_ERR(core->rst_gpio) != -EPROBE_DEFER)
+ dev_err(core->dev, "Video Reset GPIO not setup in DT");
+ return PTR_ERR(core->rst_gpio);
+ }
+
+ mutex_init(&xcsi2rxss->lock);
+
+ ret = xcsi2rxss_parse_of(xcsi2rxss);
+ if (ret < 0)
+ return ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ core->iomem = devm_ioremap_resource(core->dev, res);
+ if (IS_ERR(core->iomem))
+ return PTR_ERR(core->iomem);
+
+ ret = clk_bulk_get(core->dev, num_clks, core->clks);
+ if (ret)
+ return ret;
+
+ ret = clk_bulk_prepare_enable(num_clks, core->clks);
+ if (ret)
+ goto err_clk_put;
+
+ if (core->rst_gpio) {
+ gpiod_set_value_cansleep(core->rst_gpio, 1);
+ /* minimum of 40 dphy_clk_200M cycles */
+ usleep_range(1, 2);
+ gpiod_set_value_cansleep(core->rst_gpio, 0);
+ }
+
+ xcsi2rxss_reset(core);
+
+ /* Initialize V4L2 subdevice and media entity */
+ xcsi2rxss->pads[XVIP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+ xcsi2rxss->pads[XVIP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+
+ /* Initialize the default format */
+ xcsi2rxss_set_default_format(xcsi2rxss);
+ xcsi2rxss->format = xcsi2rxss->default_format;
+
+ /* Initialize the mbus formats supported */
+ xcsi2rxss_init_mbus_fmts(xcsi2rxss);
+
+ /* Initialize V4L2 subdevice and media entity */
+ subdev = &xcsi2rxss->subdev;
+ v4l2_subdev_init(subdev, &xcsi2rxss_ops);
+ subdev->dev = &pdev->dev;
+ subdev->internal_ops = &xcsi2rxss_internal_ops;
+ strscpy(subdev->name, dev_name(&pdev->dev), sizeof(subdev->name));
+ subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
+ subdev->entity.ops = &xcsi2rxss_media_ops;
+ v4l2_set_subdevdata(subdev, xcsi2rxss);
+
+ ret = media_entity_pads_init(&subdev->entity, XCSI_MEDIA_PADS,
+ xcsi2rxss->pads);
+ if (ret < 0)
+ goto error;
+
+ platform_set_drvdata(pdev, xcsi2rxss);
+
+ ret = v4l2_async_register_subdev(subdev);
+ if (ret < 0) {
+ dev_err(core->dev, "failed to register subdev\n");
+ goto error;
+ }
+
+ dev_info(core->dev, "Xilinx CSI2 Rx Subsystem device found!\n");
+
+ return 0;
+error:
+ media_entity_cleanup(&subdev->entity);
+ mutex_destroy(&xcsi2rxss->lock);
+ clk_bulk_disable_unprepare(num_clks, core->clks);
+err_clk_put:
+ clk_bulk_put(num_clks, core->clks);
+ return ret;
+}
+
+static int xcsi2rxss_remove(struct platform_device *pdev)
+{
+ struct xcsi2rxss_state *xcsi2rxss = platform_get_drvdata(pdev);
+ struct xcsi2rxss_core *core = &xcsi2rxss->core;
+ struct v4l2_subdev *subdev = &xcsi2rxss->subdev;
+ int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
+
+ v4l2_async_unregister_subdev(subdev);
+ media_entity_cleanup(&subdev->entity);
+ mutex_destroy(&xcsi2rxss->lock);
+ clk_bulk_disable_unprepare(num_clks, core->clks);
+ clk_bulk_put(num_clks, core->clks);
+
+ return 0;
+}
+
+static const struct of_device_id xcsi2rxss_of_id_table[] = {
+ { .compatible = "xlnx,mipi-csi2-rx-subsystem-5.0", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, xcsi2rxss_of_id_table);
+
+static struct platform_driver xcsi2rxss_driver = {
+ .driver = {
+ .name = "xilinx-csi2rxss",
+ .of_match_table = xcsi2rxss_of_id_table,
+ },
+ .probe = xcsi2rxss_probe,
+ .remove = xcsi2rxss_remove,
+};
+
+module_platform_driver(xcsi2rxss_driver);
+
+MODULE_AUTHOR("Vishal Sagar <[email protected]>");
+MODULE_DESCRIPTION("Xilinx MIPI CSI2 Rx Subsystem Driver");
+MODULE_LICENSE("GPL v2");
--
2.21.0

2020-04-09 19:50:26

[permalink] [raw]

Subject: [PATCH v11 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.

The Xilinx MIPI CSI-2 Rx Subsystem consists of a CSI-2 Rx controller, a
DPHY in Rx mode, an optional I2C controller and a Video Format Bridge.

Signed-off-by: Vishal Sagar <[email protected]>
Reviewed-by: Hyun Kwon <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Luca Ceresoli <[email protected]>
---
v11
- Modify compatible string from 4.0 to 5.0

v10
- No changes

v9
- Fix xlnx,vfb description.
- s/Optional/Required endpoint property.
- Move data-lanes description from Ports to endpoint property section.

v8
- Added reset-gpios optional property to assert video_aresetn

v7
- Removed the control name from dt bindings
- Updated the example dt node name to csi2rx

v6
- Added "control" after V4L2_CID_XILINX_MIPICSISS_ACT_LANES as suggested by Luca
- Added reviewed by Rob Herring

v5
- Incorporated comments by Luca Cersoli
- Removed DPHY clock from description and example
- Removed bayer pattern from device tree MIPI CSI IP
doesn't deal with bayer pattern.

v4
- Added reviewed by Hyun Kwon

v3
- removed interrupt parent as suggested by Rob
- removed dphy clock
- moved vfb to optional properties
- Added required and optional port properties section
- Added endpoint property section

v2
- updated the compatible string to latest version supported
- removed DPHY related parameters
- added CSI v2.0 related property (including VCX for supporting upto 16
virtual channels).
- modified csi-pxl-format from string to unsigned int type where the value
is as per the CSI specification
- Defined port 0 and port 1 as sink and source ports.
- Removed max-lanes property as suggested by Rob and Sakari
.../bindings/media/xilinx/xlnx,csi2rxss.txt | 116 ++++++++++++++++++
1 file changed, 116 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt

diff --git a/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
new file mode 100644
index 000000000000..9269a5c880aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
@@ -0,0 +1,116 @@
+Xilinx MIPI CSI2 Receiver Subsystem Device Tree Bindings
+--------------------------------------------------------
+
+The Xilinx MIPI CSI2 Receiver Subsystem is used to capture MIPI CSI2 traffic
+from compliant camera sensors and send the output as AXI4 Stream video data
+for image processing.
+
+The subsystem consists of a MIPI DPHY in slave mode which captures the
+data packets. This is passed along the MIPI CSI2 Rx IP which extracts the
+packet data. The optional Video Format Bridge (VFB) converts this data to
+AXI4 Stream video data.
+
+For more details, please refer to PG232 Xilinx MIPI CSI-2 Receiver Subsystem.
+
+Required properties:
+--------------------
+- compatible: Must contain "xlnx,mipi-csi2-rx-subsystem-5.0".
+- reg: Physical base address and length of the registers set for the device.
+- interrupts: Property with a value describing the interrupt number.
+- clocks: List of phandles to AXI Lite and Video clocks.
+- clock-names: Must contain "lite_aclk" and "video_aclk" in the same order
+ as clocks listed in clocks property.
+- xlnx,csi-pxl-format: This denotes the CSI Data type selected in hw design.
+ Packets other than this data type (except for RAW8 and User defined data
+ types) will be filtered out. Possible values are as below -
+ 0x1E - YUV4228B
+ 0x1F - YUV42210B
+ 0x20 - RGB444
+ 0x21 - RGB555
+ 0x22 - RGB565
+ 0x23 - RGB666
+ 0x24 - RGB888
+ 0x28 - RAW6
+ 0x29 - RAW7
+ 0x2A - RAW8
+ 0x2B - RAW10
+ 0x2C - RAW12
+ 0x2D - RAW14
+ 0x2E - RAW16
+ 0x2F - RAW20
+
+
+Optional properties:
+--------------------
+- xlnx,vfb: Present when Video Format Bridge is enabled in IP configuration
+- xlnx,en-csi-v2-0: Present if CSI v2 is enabled in IP configuration.
+- xlnx,en-vcx: When present, there are maximum 16 virtual channels, else
+ only 4. This is present only if xlnx,en-csi-v2-0 is present.
+- xlnx,en-active-lanes: present if the number of active lanes can be
+ re-configured at runtime in the Protocol Configuration Register.
+ Otherwise all lanes, as set in IP configuration, are always active.
+- reset-gpios: Optional specifier for a GPIO that asserts video_aresetn.
+
+Ports
+-----
+The device node shall contain two 'port' child nodes as defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+The port@0 is a sink port and shall connect to CSI2 source like camera.
+
+The port@1 is a source port and can be connected to any video processing IP
+which can work with AXI4 Stream data.
+
+Required port properties:
+--------------------
+- reg: 0 - for sink port.
+ 1 - for source port.
+
+Required endpoint property:
+---------------------------
+- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-interfaces.txt.
+ This is required only in the sink port 0 endpoint which connects to MIPI CSI2
+ source like sensor. The possible values are:
+ 1 - For 1 lane enabled in IP.
+ 1 2 - For 2 lanes enabled in IP.
+ 1 2 3 - For 3 lanes enabled in IP.
+ 1 2 3 4 - For 4 lanes enabled in IP.
+
+Example:
+
+ xcsi2rxss_1: csi2rx@a0020000 {
+ compatible = "xlnx,mipi-csi2-rx-subsystem-5.0";
+ reg = <0x0 0xa0020000 0x0 0x10000>;
+ interrupt-parent = <&gic>;
+ interrupts = <0 95 4>;
+ xlnx,csi-pxl-format = <0x2a>;
+ xlnx,vfb;
+ xlnx,en-active-lanes;
+ xlnx,en-csi-v2-0;
+ xlnx,en-vcx;
+ clock-names = "lite_aclk", "video_aclk";
+ clocks = <&misc_clk_0>, <&misc_clk_1>;
+ reset-gpios = <&gpio 86 GPIO_ACTIVE_LOW>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ /* Sink port */
+ reg = <0>;
+ csiss_in: endpoint {
+ data-lanes = <1 2 3 4>;
+ /* MIPI CSI2 Camera handle */
+ remote-endpoint = <&camera_out>;
+ };
+ };
+ port@1 {
+ /* Source port */
+ reg = <1>;
+ csiss_out: endpoint {
+ remote-endpoint = <&vproc_in>;
+ };
+ };
+ };
+ };
--
2.21.0

2020-04-19 15:46:48

[permalink] [raw]

Subject: Re: [PATCH v11 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Hi Vishal,

Thank you for the patch.

On Fri, Apr 10, 2020 at 01:14:23AM +0530, Vishal Sagar wrote:
> Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
>
> The Xilinx MIPI CSI-2 Rx Subsystem consists of a CSI-2 Rx controller, a
> DPHY in Rx mode, an optional I2C controller and a Video Format Bridge.

The AXI IIC was removed from the subsystem in v4.1, you could drop it
from the commit message too.

>
> Signed-off-by: Vishal Sagar <[email protected]>
> Reviewed-by: Hyun Kwon <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Luca Ceresoli <[email protected]>
> ---
> v11
> - Modify compatible string from 4.0 to 5.0
>
> v10
> - No changes
>
> v9
> - Fix xlnx,vfb description.
> - s/Optional/Required endpoint property.
> - Move data-lanes description from Ports to endpoint property section.
>
> v8
> - Added reset-gpios optional property to assert video_aresetn
>
> v7
> - Removed the control name from dt bindings
> - Updated the example dt node name to csi2rx
>
> v6
> - Added "control" after V4L2_CID_XILINX_MIPICSISS_ACT_LANES as suggested by Luca
> - Added reviewed by Rob Herring
>
> v5
> - Incorporated comments by Luca Cersoli
> - Removed DPHY clock from description and example
> - Removed bayer pattern from device tree MIPI CSI IP
> doesn't deal with bayer pattern.
>
> v4
> - Added reviewed by Hyun Kwon
>
> v3
> - removed interrupt parent as suggested by Rob
> - removed dphy clock
> - moved vfb to optional properties
> - Added required and optional port properties section
> - Added endpoint property section
>
> v2
> - updated the compatible string to latest version supported
> - removed DPHY related parameters
> - added CSI v2.0 related property (including VCX for supporting upto 16
> virtual channels).
> - modified csi-pxl-format from string to unsigned int type where the value
> is as per the CSI specification
> - Defined port 0 and port 1 as sink and source ports.
> - Removed max-lanes property as suggested by Rob and Sakari
> .../bindings/media/xilinx/xlnx,csi2rxss.txt | 116 ++++++++++++++++++
> 1 file changed, 116 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
>
> diff --git a/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> new file mode 100644
> index 000000000000..9269a5c880aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt

YAML is the recommended form for new DT bindings. This wasn't a
requirement when the first version of this series was submitted, and I
understand it can be frustrating to chase a moving target, so I can help
with the YAML conversion once we sort out the questions below.

> @@ -0,0 +1,116 @@
> +Xilinx MIPI CSI2 Receiver Subsystem Device Tree Bindings

Nitpicking, it's CSI-2, not CSI2.

> +--------------------------------------------------------
> +
> +The Xilinx MIPI CSI2 Receiver Subsystem is used to capture MIPI CSI2 traffic
> +from compliant camera sensors and send the output as AXI4 Stream video data
> +for image processing.
> +
> +The subsystem consists of a MIPI DPHY in slave mode which captures the

And D-PHY, not DPHY :-)

> +data packets. This is passed along the MIPI CSI2 Rx IP which extracts the
> +packet data. The optional Video Format Bridge (VFB) converts this data to
> +AXI4 Stream video data.
> +
> +For more details, please refer to PG232 Xilinx MIPI CSI-2 Receiver Subsystem.

If I understand correctly, this DT binding covers the CSI-2 RX
Controller and the optional Video Format Bridge, but leaves the D-PHY
out, right ? I think this should be clarified, as the "CSI-2 receiver
subsystem" includes the D-PHY.

> +
> +Required properties:
> +--------------------
> +- compatible: Must contain "xlnx,mipi-csi2-rx-subsystem-5.0".

Is PG232 v5.0 available ? The most recent version I've found was PG232
v4.1.

> +- reg: Physical base address and length of the registers set for the device.
> +- interrupts: Property with a value describing the interrupt number.
> +- clocks: List of phandles to AXI Lite and Video clocks.
> +- clock-names: Must contain "lite_aclk" and "video_aclk" in the same order
> + as clocks listed in clocks property.

The subsystem documentation also mentions a dphy_clk_200M. Is that
routed to the D-PHY only, or is it also needed for the CSI-2 RX ?

> +- xlnx,csi-pxl-format: This denotes the CSI Data type selected in hw design.
> + Packets other than this data type (except for RAW8 and User defined data
> + types) will be filtered out. Possible values are as below -
> + 0x1E - YUV4228B
> + 0x1F - YUV42210B
> + 0x20 - RGB444
> + 0x21 - RGB555
> + 0x22 - RGB565
> + 0x23 - RGB666
> + 0x24 - RGB888
> + 0x28 - RAW6
> + 0x29 - RAW7
> + 0x2A - RAW8
> + 0x2B - RAW10
> + 0x2C - RAW12
> + 0x2D - RAW14
> + 0x2E - RAW16
> + 0x2F - RAW20

Isn't this property required only when the VFB is present ?

> +
> +
> +Optional properties:
> +--------------------
> +- xlnx,vfb: Present when Video Format Bridge is enabled in IP configuration
> +- xlnx,en-csi-v2-0: Present if CSI v2 is enabled in IP configuration.

Unless I'm mistaken, this feature is available starting at v4 of the IP
core.

> +- xlnx,en-vcx: When present, there are maximum 16 virtual channels, else
> + only 4. This is present only if xlnx,en-csi-v2-0 is present.
> +- xlnx,en-active-lanes: present if the number of active lanes can be
> + re-configured at runtime in the Protocol Configuration Register.
> + Otherwise all lanes, as set in IP configuration, are always active.
> +- reset-gpios: Optional specifier for a GPIO that asserts video_aresetn.

Should lite_aresetn also be supported ? We can add a lite-reset-gpios
property later, but maybe we should name this video-reset-gpios ? As the
video_aresetn signal is the main reset I don't mind keeping the name
reset-gpios either. It's up to you.

> +
> +Ports
> +-----
> +The device node shall contain two 'port' child nodes as defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +The port@0 is a sink port and shall connect to CSI2 source like camera.
> +
> +The port@1 is a source port and can be connected to any video processing IP
> +which can work with AXI4 Stream data.
> +
> +Required port properties:
> +--------------------
> +- reg: 0 - for sink port.
> + 1 - for source port.

Don't you need a second source port for embedded non-image data ? If my
understanding is correct that port can be enabled or disabled through
the CSI_EMB_NON_IMG parameter, so it should be optional in DT too. We
can possibly leave it out for now, it can be added later in a
backward-compatible way.

> +
> +Required endpoint property:
> +---------------------------
> +- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-interfaces.txt.
> + This is required only in the sink port 0 endpoint which connects to MIPI CSI2
> + source like sensor. The possible values are:
> + 1 - For 1 lane enabled in IP.
> + 1 2 - For 2 lanes enabled in IP.
> + 1 2 3 - For 3 lanes enabled in IP.
> + 1 2 3 4 - For 4 lanes enabled in IP.
> +
> +Example:
> +
> + xcsi2rxss_1: csi2rx@a0020000 {
> + compatible = "xlnx,mipi-csi2-rx-subsystem-5.0";
> + reg = <0x0 0xa0020000 0x0 0x10000>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 95 4>;
> + xlnx,csi-pxl-format = <0x2a>;
> + xlnx,vfb;
> + xlnx,en-active-lanes;
> + xlnx,en-csi-v2-0;
> + xlnx,en-vcx;
> + clock-names = "lite_aclk", "video_aclk";
> + clocks = <&misc_clk_0>, <&misc_clk_1>;
> + reset-gpios = <&gpio 86 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + /* Sink port */
> + reg = <0>;
> + csiss_in: endpoint {
> + data-lanes = <1 2 3 4>;
> + /* MIPI CSI2 Camera handle */
> + remote-endpoint = <&camera_out>;
> + };
> + };
> + port@1 {
> + /* Source port */
> + reg = <1>;
> + csiss_out: endpoint {
> + remote-endpoint = <&vproc_in>;
> + };
> + };
> + };
> + };

--
Regards,

Laurent Pinchart

2020-04-19 15:50:12

[permalink] [raw]

Subject: Re: [PATCH v11 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Hi Vishal,

On Sun, Apr 19, 2020 at 06:43:07PM +0300, Laurent Pinchart wrote:
> On Fri, Apr 10, 2020 at 01:14:23AM +0530, Vishal Sagar wrote:
> > Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
> >
> > The Xilinx MIPI CSI-2 Rx Subsystem consists of a CSI-2 Rx controller, a
> > DPHY in Rx mode, an optional I2C controller and a Video Format Bridge.
>
> The AXI IIC was removed from the subsystem in v4.1, you could drop it
> from the commit message too.
>
> > Signed-off-by: Vishal Sagar <[email protected]>
> > Reviewed-by: Hyun Kwon <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> > Reviewed-by: Luca Ceresoli <[email protected]>
> > ---
> > v11
> > - Modify compatible string from 4.0 to 5.0
> >
> > v10
> > - No changes
> >
> > v9
> > - Fix xlnx,vfb description.
> > - s/Optional/Required endpoint property.
> > - Move data-lanes description from Ports to endpoint property section.
> >
> > v8
> > - Added reset-gpios optional property to assert video_aresetn
> >
> > v7
> > - Removed the control name from dt bindings
> > - Updated the example dt node name to csi2rx
> >
> > v6
> > - Added "control" after V4L2_CID_XILINX_MIPICSISS_ACT_LANES as suggested by Luca
> > - Added reviewed by Rob Herring
> >
> > v5
> > - Incorporated comments by Luca Cersoli
> > - Removed DPHY clock from description and example
> > - Removed bayer pattern from device tree MIPI CSI IP
> > doesn't deal with bayer pattern.
> >
> > v4
> > - Added reviewed by Hyun Kwon
> >
> > v3
> > - removed interrupt parent as suggested by Rob
> > - removed dphy clock
> > - moved vfb to optional properties
> > - Added required and optional port properties section
> > - Added endpoint property section
> >
> > v2
> > - updated the compatible string to latest version supported
> > - removed DPHY related parameters
> > - added CSI v2.0 related property (including VCX for supporting upto 16
> > virtual channels).
> > - modified csi-pxl-format from string to unsigned int type where the value
> > is as per the CSI specification
> > - Defined port 0 and port 1 as sink and source ports.
> > - Removed max-lanes property as suggested by Rob and Sakari
> > .../bindings/media/xilinx/xlnx,csi2rxss.txt | 116 ++++++++++++++++++
> > 1 file changed, 116 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> > new file mode 100644
> > index 000000000000..9269a5c880aa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
>
> YAML is the recommended form for new DT bindings. This wasn't a
> requirement when the first version of this series was submitted, and I
> understand it can be frustrating to chase a moving target, so I can help
> with the YAML conversion once we sort out the questions below.
>
> > @@ -0,0 +1,116 @@
> > +Xilinx MIPI CSI2 Receiver Subsystem Device Tree Bindings
>
> Nitpicking, it's CSI-2, not CSI2.
>
> > +--------------------------------------------------------
> > +
> > +The Xilinx MIPI CSI2 Receiver Subsystem is used to capture MIPI CSI2 traffic
> > +from compliant camera sensors and send the output as AXI4 Stream video data
> > +for image processing.
> > +
> > +The subsystem consists of a MIPI DPHY in slave mode which captures the
>
> And D-PHY, not DPHY :-)
>
> > +data packets. This is passed along the MIPI CSI2 Rx IP which extracts the
> > +packet data. The optional Video Format Bridge (VFB) converts this data to
> > +AXI4 Stream video data.
> > +
> > +For more details, please refer to PG232 Xilinx MIPI CSI-2 Receiver Subsystem.
>
> If I understand correctly, this DT binding covers the CSI-2 RX
> Controller and the optional Video Format Bridge, but leaves the D-PHY
> out, right ? I think this should be clarified, as the "CSI-2 receiver
> subsystem" includes the D-PHY.

I forgot to mention that if the PHY isn't included in the bindings, then
it should be referenced through a "phys" property.

> > +
> > +Required properties:
> > +--------------------
> > +- compatible: Must contain "xlnx,mipi-csi2-rx-subsystem-5.0".
>
> Is PG232 v5.0 available ? The most recent version I've found was PG232
> v4.1.
>
> > +- reg: Physical base address and length of the registers set for the device.
> > +- interrupts: Property with a value describing the interrupt number.
> > +- clocks: List of phandles to AXI Lite and Video clocks.
> > +- clock-names: Must contain "lite_aclk" and "video_aclk" in the same order
> > + as clocks listed in clocks property.
>
> The subsystem documentation also mentions a dphy_clk_200M. Is that
> routed to the D-PHY only, or is it also needed for the CSI-2 RX ?
>
> > +- xlnx,csi-pxl-format: This denotes the CSI Data type selected in hw design.
> > + Packets other than this data type (except for RAW8 and User defined data
> > + types) will be filtered out. Possible values are as below -
> > + 0x1E - YUV4228B
> > + 0x1F - YUV42210B
> > + 0x20 - RGB444
> > + 0x21 - RGB555
> > + 0x22 - RGB565
> > + 0x23 - RGB666
> > + 0x24 - RGB888
> > + 0x28 - RAW6
> > + 0x29 - RAW7
> > + 0x2A - RAW8
> > + 0x2B - RAW10
> > + 0x2C - RAW12
> > + 0x2D - RAW14
> > + 0x2E - RAW16
> > + 0x2F - RAW20
>
> Isn't this property required only when the VFB is present ?
>
> > +
> > +
> > +Optional properties:
> > +--------------------
> > +- xlnx,vfb: Present when Video Format Bridge is enabled in IP configuration
> > +- xlnx,en-csi-v2-0: Present if CSI v2 is enabled in IP configuration.
>
> Unless I'm mistaken, this feature is available starting at v4 of the IP
> core.
>
> > +- xlnx,en-vcx: When present, there are maximum 16 virtual channels, else
> > + only 4. This is present only if xlnx,en-csi-v2-0 is present.
> > +- xlnx,en-active-lanes: present if the number of active lanes can be
> > + re-configured at runtime in the Protocol Configuration Register.
> > + Otherwise all lanes, as set in IP configuration, are always active.
> > +- reset-gpios: Optional specifier for a GPIO that asserts video_aresetn.
>
> Should lite_aresetn also be supported ? We can add a lite-reset-gpios
> property later, but maybe we should name this video-reset-gpios ? As the
> video_aresetn signal is the main reset I don't mind keeping the name
> reset-gpios either. It's up to you.
>
> > +
> > +Ports
> > +-----
> > +The device node shall contain two 'port' child nodes as defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +
> > +The port@0 is a sink port and shall connect to CSI2 source like camera.
> > +
> > +The port@1 is a source port and can be connected to any video processing IP
> > +which can work with AXI4 Stream data.
> > +
> > +Required port properties:
> > +--------------------
> > +- reg: 0 - for sink port.
> > + 1 - for source port.
>
> Don't you need a second source port for embedded non-image data ? If my
> understanding is correct that port can be enabled or disabled through
> the CSI_EMB_NON_IMG parameter, so it should be optional in DT too. We
> can possibly leave it out for now, it can be added later in a
> backward-compatible way.
>
> > +
> > +Required endpoint property:
> > +---------------------------
> > +- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-interfaces.txt.
> > + This is required only in the sink port 0 endpoint which connects to MIPI CSI2
> > + source like sensor. The possible values are:
> > + 1 - For 1 lane enabled in IP.
> > + 1 2 - For 2 lanes enabled in IP.
> > + 1 2 3 - For 3 lanes enabled in IP.
> > + 1 2 3 4 - For 4 lanes enabled in IP.
> > +
> > +Example:
> > +
> > + xcsi2rxss_1: csi2rx@a0020000 {
> > + compatible = "xlnx,mipi-csi2-rx-subsystem-5.0";
> > + reg = <0x0 0xa0020000 0x0 0x10000>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 95 4>;
> > + xlnx,csi-pxl-format = <0x2a>;
> > + xlnx,vfb;
> > + xlnx,en-active-lanes;
> > + xlnx,en-csi-v2-0;
> > + xlnx,en-vcx;
> > + clock-names = "lite_aclk", "video_aclk";
> > + clocks = <&misc_clk_0>, <&misc_clk_1>;
> > + reset-gpios = <&gpio 86 GPIO_ACTIVE_LOW>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + /* Sink port */
> > + reg = <0>;
> > + csiss_in: endpoint {
> > + data-lanes = <1 2 3 4>;
> > + /* MIPI CSI2 Camera handle */
> > + remote-endpoint = <&camera_out>;
> > + };
> > + };
> > + port@1 {
> > + /* Source port */
> > + reg = <1>;
> > + csiss_out: endpoint {
> > + remote-endpoint = <&vproc_in>;
> > + };
> > + };
> > + };
> > + };

--
Regards,

Laurent Pinchart

2020-04-19 18:32:37

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Vishal,

Thank you for the patch.

On Fri, Apr 10, 2020 at 01:14:24AM +0530, Vishal Sagar wrote:
> The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> for image processing. Please refer to PG232 for details.
>
> The CSI2 Rx controller filters out all packets except for the packets
> with data type fixed in hardware. RAW8 packets are always allowed to
> pass through.
>
> It is also used to setup and handle interrupts and enable the core. It
> logs all the events in respective counters between streaming on and off.
>
> The driver supports only the video format bridge enabled configuration.
> Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when the
> CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> the maximum number of virtual channels becomes 16 from 4.
>
> Signed-off-by: Vishal Sagar <[email protected]>
> Reviewed-by: Hyun Kwon <[email protected]>
> ---
> v11
> - Fixed changes as suggested by Sakari Ailus
> - Removed VIDEO_XILINX from KConfig
> - Minor formatting
> - Start / Stop upstream sub-device in xcsi2rxss_start_stream()
> and xcsi2rxss_stop_stream()
> - Added v4l2_subdev_link_validate_default() in v4l2_subdev_pad_ops()
> - Use fwnode_graph_get_endpoint_by_id() instead of parsing by self
> - Set bus type as V4L2_MBUS_CSI2_DPHY in struct v4l2_fwnode_endpoint
> - Remove num_clks from core. Instead use ARRAY_SIZE()
> - Fixed SPDX header to GPL-2.0
> - Update copyright year to 2020
>
> v10
> - Removed all V4L2 controls and events based on Sakari's comments.
> - Now stop_stream() before toggling rst_gpio
> - Updated init_mbus() to throw error on array out of bound access
> - Make events and vcx_events as counters instead of structures
> - Minor fixes in set_format() enum_mbus_code() as suggested by Sakari
>
> v9
> - Moved all controls and events to xilinx-csi2rxss.h
> - Updated name and description of controls and events
> - Get control base address from v4l2-controls.h (0x10c0)
> - Fix KConfig for dependency on VIDEO_XILINX
> - Added enum_mbus_code() support
> - Added default format to be returned on open()
> - Mark variables are const
> - Remove references to short packet in comments
> - Add check for streaming before setting active lanes control
> - strlcpy -> strscpy
> - Fix xcsi2rxss_set_format()
>
> v8
> - Use clk_bulk* APIs
> - Add gpio reset for asserting video_aresetn when stream line buffer occurs
> - Removed short packet related events and irq handling
> - V4L2_EVENT_XLNXCSIRX_SPKT and V4L2_EVENT_XLNXCSIRX_SPKT_OVF removed
> - Removed frame counter control V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER
> and xcsi2rxss_g_volatile_ctrl()
> - Minor formatting fixes
>
> v7
> - No change
>
> v6
> - No change
>
> v5
> - Removed bayer and updated related parts like set default format based
> on Luca Cersoli's comments.
> - Added correct YUV422 10bpc media bus format
>
> v4
> - Removed irq member from core structure
> - Consolidated IP config prints in xcsi2rxss_log_ipconfig()
> - Return -EINVAL in case of invalid ioctl
> - Code formatting
> - Added reviewed by Hyun Kwon
>
> v3
> - Fixed comments given by Hyun.
> - Removed DPHY 200 MHz clock. This will be controlled by DPHY driver
> - Minor code formatting
> - en_csi_v20 and vfb members removed from struct and made local to dt parsing
> - lock description updated
> - changed to ratelimited type for all dev prints in irq handler
> - Removed YUV 422 10bpc media format
>
> v2
> - Fixed comments given by Hyun and Sakari.
> - Made all bitmask using BIT() and GENMASK()
> - Removed unused definitions
> - Removed DPHY access. This will be done by separate DPHY PHY driver.
> - Added support for CSI v2.0 for YUV 422 10bpc, RAW16, RAW20 and extra
> virtual channels
> - Fixed the ports as sink and source
> - Now use the v4l2fwnode API to get number of data-lanes
> - Added clock framework support
> - Removed the close() function
> - updated the set format function
> - support only VFB enabled configuration
>
> drivers/media/platform/xilinx/Kconfig | 10 +
> drivers/media/platform/xilinx/Makefile | 1 +
> .../media/platform/xilinx/xilinx-csi2rxss.c | 1234 +++++++++++++++++
> 3 files changed, 1245 insertions(+)
> create mode 100644 drivers/media/platform/xilinx/xilinx-csi2rxss.c
>
> diff --git a/drivers/media/platform/xilinx/Kconfig b/drivers/media/platform/xilinx/Kconfig
> index a2773ad7c185..cd1a0fdde4df 100644
> --- a/drivers/media/platform/xilinx/Kconfig
> +++ b/drivers/media/platform/xilinx/Kconfig
> @@ -10,6 +10,16 @@ config VIDEO_XILINX
>
> if VIDEO_XILINX
>
> +config VIDEO_XILINX_CSI2RXSS
> + tristate "Xilinx CSI2 Rx Subsystem"
> + help
> + Driver for Xilinx MIPI CSI2 Rx Subsystem. This is a V4L sub-device

As for the bindings, s/CSI2/CSI-2/ and s/DPHY/D-PHY/ through this patch.

> + based driver that takes input from CSI2 Tx source and converts
> + it into an AXI4-Stream. The subsystem comprises of a CSI2 Rx
> + controller, DPHY, an optional I2C controller and a Video Format

The I2C controller has been removed in v4.1, it should be dropped from
here.

> + Bridge. The driver is used to set the number of active lanes and
> + get short packet data.
> +
> config VIDEO_XILINX_TPG
> tristate "Xilinx Video Test Pattern Generator"
> depends on VIDEO_XILINX
> diff --git a/drivers/media/platform/xilinx/Makefile b/drivers/media/platform/xilinx/Makefile
> index 4cdc0b1ec7a5..6119a34f3043 100644
> --- a/drivers/media/platform/xilinx/Makefile
> +++ b/drivers/media/platform/xilinx/Makefile
> @@ -3,5 +3,6 @@
> xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o
>
> obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o
> +obj-$(CONFIG_VIDEO_XILINX_CSI2RXSS) += xilinx-csi2rxss.o
> obj-$(CONFIG_VIDEO_XILINX_TPG) += xilinx-tpg.o
> obj-$(CONFIG_VIDEO_XILINX_VTC) += xilinx-vtc.o
> diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> new file mode 100644
> index 000000000000..083422768ebd
> --- /dev/null
> +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> @@ -0,0 +1,1234 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Xilinx MIPI CSI2 Rx Subsystem
> + *
> + * Copyright (C) 2016 - 2020 Xilinx, Inc.
> + *
> + * Contacts: Vishal Sagar <[email protected]>
> + *
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/v4l2-subdev.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +#include "xilinx-vip.h"
> +
> +/* Register register map */
> +#define XCSI_CCR_OFFSET 0x00
> +#define XCSI_CCR_SOFTRESET BIT(1)
> +#define XCSI_CCR_ENABLE BIT(0)
> +
> +#define XCSI_PCR_OFFSET 0x04
> +#define XCSI_PCR_MAXLANES_MASK GENMASK(4, 3)
> +#define XCSI_PCR_ACTLANES_MASK GENMASK(1, 0)
> +
> +#define XCSI_CSR_OFFSET 0x10
> +#define XCSI_CSR_PKTCNT GENMASK(31, 16)
> +#define XCSI_CSR_SPFIFOFULL BIT(3)
> +#define XCSI_CSR_SPFIFONE BIT(2)
> +#define XCSI_CSR_SLBF BIT(1)
> +#define XCSI_CSR_RIPCD BIT(0)
> +
> +#define XCSI_GIER_OFFSET 0x20
> +#define XCSI_GIER_GIE BIT(0)
> +
> +#define XCSI_ISR_OFFSET 0x24
> +#define XCSI_IER_OFFSET 0x28
> +
> +#define XCSI_ISR_FR BIT(31)
> +#define XCSI_ISR_VCXFE BIT(30)
> +#define XCSI_ISR_WCC BIT(22)
> +#define XCSI_ISR_ILC BIT(21)
> +#define XCSI_ISR_SPFIFOF BIT(20)
> +#define XCSI_ISR_SPFIFONE BIT(19)
> +#define XCSI_ISR_SLBF BIT(18)
> +#define XCSI_ISR_STOP BIT(17)
> +#define XCSI_ISR_SOTERR BIT(13)
> +#define XCSI_ISR_SOTSYNCERR BIT(12)
> +#define XCSI_ISR_ECC2BERR BIT(11)
> +#define XCSI_ISR_ECC1BERR BIT(10)
> +#define XCSI_ISR_CRCERR BIT(9)
> +#define XCSI_ISR_DATAIDERR BIT(8)
> +#define XCSI_ISR_VC3FSYNCERR BIT(7)
> +#define XCSI_ISR_VC3FLVLERR BIT(6)
> +#define XCSI_ISR_VC2FSYNCERR BIT(5)
> +#define XCSI_ISR_VC2FLVLERR BIT(4)
> +#define XCSI_ISR_VC1FSYNCERR BIT(3)
> +#define XCSI_ISR_VC1FLVLERR BIT(2)
> +#define XCSI_ISR_VC0FSYNCERR BIT(1)
> +#define XCSI_ISR_VC0FLVLERR BIT(0)
> +
> +#define XCSI_INTR_PROT_MASK (XCSI_ISR_VC3FSYNCERR | XCSI_ISR_VC3FLVLERR |\
> + XCSI_ISR_VC2FSYNCERR | XCSI_ISR_VC2FLVLERR |\
> + XCSI_ISR_VC1FSYNCERR | XCSI_ISR_VC1FLVLERR |\
> + XCSI_ISR_VC0FSYNCERR | XCSI_ISR_VC0FLVLERR |\
> + XCSI_ISR_VCXFE)
> +
> +#define XCSI_INTR_PKTLVL_MASK (XCSI_ISR_ECC2BERR | XCSI_ISR_ECC1BERR |\
> + XCSI_ISR_CRCERR | XCSI_ISR_DATAIDERR)
> +
> +#define XCSI_INTR_DPHY_MASK (XCSI_ISR_SOTERR | XCSI_ISR_SOTSYNCERR)
> +
> +#define XCSI_INTR_SPKT_MASK (XCSI_ISR_SPFIFOF | XCSI_ISR_SPFIFONE)
> +
> +#define XCSI_INTR_ERR_MASK (XCSI_ISR_WCC | XCSI_ISR_ILC | XCSI_ISR_SLBF |\
> + XCSI_ISR_STOP)
> +
> +#define XCSI_INTR_FRAMERCVD_MASK (XCSI_ISR_FR)

These macros are only used in XCSI_ISR_ALLINTR_MASK, do we need them ?

> +
> +#define XCSI_ISR_ALLINTR_MASK (XCSI_INTR_PROT_MASK | XCSI_INTR_PKTLVL_MASK |\
> + XCSI_INTR_DPHY_MASK | XCSI_INTR_SPKT_MASK |\
> + XCSI_INTR_ERR_MASK | XCSI_INTR_FRAMERCVD_MASK)
> +
> +/*
> + * Removed VCXFE mask as it doesn't exist in IER
> + * Removed STOP state irq as this will keep driver in irq handler only
> + */
> +#define XCSI_IER_INTR_MASK (XCSI_ISR_ALLINTR_MASK &\
> + ~(XCSI_ISR_STOP | XCSI_ISR_VCXFE))
> +
> +#define XCSI_SPKTR_OFFSET 0x30
> +#define XCSI_SPKTR_DATA GENMASK(23, 8)
> +#define XCSI_SPKTR_VC GENMASK(7, 6)
> +#define XCSI_SPKTR_DT GENMASK(5, 0)
> +
> +#define XCSI_VCXR_OFFSET 0x34
> +#define XCSI_VCXR_VCERR GENMASK(23, 0)
> +#define XCSI_VCXR_VCSTART 4
> +#define XCSI_VCXR_VCEND 15

Those two macros are not used, maybe we could drop them ?

> +#define XCSI_VCXR_FSYNCERR BIT(1)
> +#define XCSI_VCXR_FLVLERR BIT(0)
> +
> +#define XCSI_CLKINFR_OFFSET 0x3C
> +#define XCSI_CLKINFR_STOP BIT(1)
> +
> +#define XCSI_DLXINFR_OFFSET 0x40
> +#define XCSI_DLXINFR_STOP BIT(5)
> +#define XCSI_DLXINFR_SOTERR BIT(1)
> +#define XCSI_DLXINFR_SOTSYNCERR BIT(0)
> +#define XCSI_MAXDL_COUNT 0x4
> +
> +#define XCSI_VCXINF1R_OFFSET 0x60
> +#define XCSI_VCXINF1R_LINECOUNT GENMASK(31, 16)
> +#define XCSI_VCXINF1R_LINECOUNT_SHIFT 16
> +#define XCSI_VCXINF1R_BYTECOUNT GENMASK(15, 0)
> +
> +#define XCSI_VCXINF2R_OFFSET 0x64
> +#define XCSI_VCXINF2R_DT GENMASK(5, 0)
> +#define XCSI_MAXVCX_COUNT 16
> +
> +/*
> + * The core takes less than 100 video clock cycles to reset.
> + * So choosing a timeout value larger than this.
> + */
> +#define XCSI_TIMEOUT_VAL 1000 /* us */
> +
> +/*
> + * Sink pad connected to sensor source pad.
> + * Source pad connected to next module like demosaic.
> + */
> +#define XCSI_MEDIA_PADS 2
> +#define XCSI_DEFAULT_WIDTH 1920
> +#define XCSI_DEFAULT_HEIGHT 1080
> +
> +/* Max media bus formats supported for enumeration */
> +#define XCSI_MAX_MBUS_FMTS 16

Isn't 8 enough ?

> +
> +/* Max string length for CSI Data type string */
> +#define XCSI_PXLFMT_STRLEN_MAX 16

This isn't used either.

> +
> +/* MIPI CSI2 Data Types from spec */
> +#define XCSI_DT_YUV4228B 0x1E
> +#define XCSI_DT_YUV42210B 0x1F
> +#define XCSI_DT_RGB444 0x20
> +#define XCSI_DT_RGB555 0x21
> +#define XCSI_DT_RGB565 0x22
> +#define XCSI_DT_RGB666 0x23
> +#define XCSI_DT_RGB888 0x24
> +#define XCSI_DT_RAW6 0x28
> +#define XCSI_DT_RAW7 0x29
> +#define XCSI_DT_RAW8 0x2A
> +#define XCSI_DT_RAW10 0x2B
> +#define XCSI_DT_RAW12 0x2C
> +#define XCSI_DT_RAW14 0x2D
> +#define XCSI_DT_RAW16 0x2E
> +#define XCSI_DT_RAW20 0x2F

Could you please use lower-case for hex constants ?

> +
> +#define XCSI_VCX_START 4
> +#define XCSI_MAX_VC 4
> +#define XCSI_MAX_VCX 16
> +
> +#define XCSI_NEXTREG_OFFSET 4
> +
> +/* There are 2 events frame sync and frame level error per VC */
> +#define XCSI_VCX_NUM_EVENTS ((XCSI_MAX_VCX - XCSI_MAX_VC) * 2)
> +
> +/* Macro to return "true" or "false" string if bit is set */
> +#define XCSI_GET_BITSET_STR(val, mask) (val) & (mask) ? "true" : "false"

I would inline this in xcsi2rxss_log_status() where the macro is used.

> +
> +#define XADD_MBUS(state, mbus_fmt) \
> + do { \
> + if ((state)->mbus_fmts_count < XCSI_MAX_MBUS_FMTS) { \
> + (state)->mbus_fmts[(state)->mbus_fmts_count++] =\
> + (mbus_fmt); \
> + } else { \
> + dev_err((state)->core.dev, \
> + "accessing array out of bounds!\n"); \
> + } \
> + } while (0)
> +
> +/**
> + * struct xcsi2rxss_event - Event log structure
> + * @mask: Event mask
> + * @name: Name of the event
> + */
> +struct xcsi2rxss_event {
> + u32 mask;
> + const char *name;
> +};
> +
> +static const struct xcsi2rxss_event xcsi2rxss_events[] = {
> + { XCSI_ISR_FR, "Frame Received" },
> + { XCSI_ISR_VCXFE, "VCX Frame Errors" },
> + { XCSI_ISR_WCC, "Word Count Errors" },
> + { XCSI_ISR_ILC, "Invalid Lane Count Error" },
> + { XCSI_ISR_SPFIFOF, "Short Packet FIFO OverFlow Error" },
> + { XCSI_ISR_SPFIFONE, "Short Packet FIFO Not Empty" },
> + { XCSI_ISR_SLBF, "Streamline Buffer Full Error" },
> + { XCSI_ISR_STOP, "Lane Stop State" },
> + { XCSI_ISR_SOTERR, "SOT Error" },
> + { XCSI_ISR_SOTSYNCERR, "SOT Sync Error" },
> + { XCSI_ISR_ECC2BERR, "2 Bit ECC Unrecoverable Error" },
> + { XCSI_ISR_ECC1BERR, "1 Bit ECC Recoverable Error" },
> + { XCSI_ISR_CRCERR, "CRC Error" },
> + { XCSI_ISR_DATAIDERR, "Data Id Error" },
> + { XCSI_ISR_VC3FSYNCERR, "Virtual Channel 3 Frame Sync Error" },
> + { XCSI_ISR_VC3FLVLERR, "Virtual Channel 3 Frame Level Error" },
> + { XCSI_ISR_VC2FSYNCERR, "Virtual Channel 2 Frame Sync Error" },
> + { XCSI_ISR_VC2FLVLERR, "Virtual Channel 2 Frame Level Error" },
> + { XCSI_ISR_VC1FSYNCERR, "Virtual Channel 1 Frame Sync Error" },
> + { XCSI_ISR_VC1FLVLERR, "Virtual Channel 1 Frame Level Error" },
> + { XCSI_ISR_VC0FSYNCERR, "Virtual Channel 0 Frame Sync Error" },
> + { XCSI_ISR_VC0FLVLERR, "Virtual Channel 0 Frame Level Error" }
> +};
> +
> +#define XCSI_NUM_EVENTS ARRAY_SIZE(xcsi2rxss_events)
> +
> +/*
> + * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device structure
> + * @dev: Platform structure
> + * @iomem: Base address of subsystem
> + * @enable_active_lanes: If number of active lanes can be modified
> + * @max_num_lanes: Maximum number of lanes present
> + * @datatype: Data type filter
> + * @events: counter for events
> + * @vcx_events: counter for vcx_events
> + * @en_vcx: If more than 4 VC are enabled
> + * @clks: array of clocks
> + * @rst_gpio: reset to video_aresetn
> + */
> +struct xcsi2rxss_core {
> + struct device *dev;
> + void __iomem *iomem;
> + bool enable_active_lanes;
> + u32 max_num_lanes;
> + u32 datatype;
> + u32 events[XCSI_NUM_EVENTS];
> + u32 vcx_events[XCSI_VCX_NUM_EVENTS];
> + bool en_vcx;
> + struct clk_bulk_data *clks;
> + struct gpio_desc *rst_gpio;
> +};
> +
> +/**
> + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
> + * @core: Core structure for MIPI CSI2 Rx Subsystem
> + * @subdev: The v4l2 subdev structure
> + * @format: Active V4L2 formats on each pad
> + * @default_format: Default V4L2 format
> + * @lock: mutex for accessing this structure
> + * @pads: media pads
> + * @mbus_fmts: List of media bus formats for enum_mbus_code
> + * @mbus_fmts_count: Number of media bus formats
> + * @streaming: Flag for storing streaming state
> + *
> + * This structure contains the device driver related parameters
> + */
> +struct xcsi2rxss_state {
> + struct xcsi2rxss_core core;
> + struct v4l2_subdev subdev;
> + struct v4l2_mbus_framefmt format;
> + struct v4l2_mbus_framefmt default_format;
> + /* used to protect access to this struct */
> + struct mutex lock;
> + struct media_pad pads[XCSI_MEDIA_PADS];
> + u32 mbus_fmts[XCSI_MAX_MBUS_FMTS];
> + u32 mbus_fmts_count;
> + bool streaming;
> +};

Is there a need to have two separate data structures, can't they be
merged ?

> +
> +static const struct clk_bulk_data xcsi2rxss_clks[] = {
> + { .id = "lite_aclk" },
> + { .id = "video_aclk" },
> +};
> +
> +static inline struct xcsi2rxss_state *
> +to_xcsi2rxssstate(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct xcsi2rxss_state, subdev);
> +}
> +
> +/*
> + * Register related operations
> + */
> +static inline u32 xcsi2rxss_read(struct xcsi2rxss_core *xcsi2rxss, u32 addr)
> +{
> + return ioread32(xcsi2rxss->iomem + addr);
> +}
> +
> +static inline void xcsi2rxss_write(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> + u32 value)
> +{
> + iowrite32(value, xcsi2rxss->iomem + addr);
> +}
> +
> +static inline void xcsi2rxss_clr(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> + u32 clr)
> +{
> + xcsi2rxss_write(xcsi2rxss, addr,
> + xcsi2rxss_read(xcsi2rxss, addr) & ~clr);
> +}
> +
> +static inline void xcsi2rxss_set(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> + u32 set)
> +{
> + xcsi2rxss_write(xcsi2rxss, addr, xcsi2rxss_read(xcsi2rxss, addr) | set);
> +}
> +
> +static void xcsi2rxss_enable(struct xcsi2rxss_core *core)
> +{
> + xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> +}
> +
> +static void xcsi2rxss_disable(struct xcsi2rxss_core *core)
> +{
> + xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> +}
> +
> +static void xcsi2rxss_intr_enable(struct xcsi2rxss_core *core)
> +{
> + xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> + xcsi2rxss_write(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> + xcsi2rxss_set(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> +}
> +
> +static void xcsi2rxss_intr_disable(struct xcsi2rxss_core *core)
> +{
> + xcsi2rxss_clr(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> + xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> +}

I'd find the code more readable if we inlined those four functions in
their single call site, with a one-line comment explaining what happens
(e.g. /* Enable interrupts */), but that may be a matter of personal
taste.

> +
> +/**
> + * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
> + * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
> + *
> + * Core takes less than 100 video clock cycles to reset.
> + * So a larger timeout value is chosen for margin.
> + *
> + * Return: 0 - on success OR -ETIME if reset times out
> + */
> +static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
> +{
> + u32 timeout = XCSI_TIMEOUT_VAL;
> +
> + xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> +
> + while (xcsi2rxss_read(core, XCSI_CSR_OFFSET) & XCSI_CSR_RIPCD) {
> + if (timeout == 0) {
> + dev_err(core->dev, "soft reset timed out!\n");
> + return -ETIME;
> + }
> +
> + timeout--;
> + udelay(1);

I understand the reset time is guaranteed to be lower than 100 video
clock cycles, but what is the typical time ? It's a bit pointless to
busy loop with a very short delay if a larger delay is typically needed.

> + }
> +
> + xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> + return 0;
> +}
> +
> +static void xcsi2rxss_reset_event_counters(struct xcsi2rxss_state *state)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < XCSI_NUM_EVENTS; i++)
> + state->core.events[i] = 0;
> +
> + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++)
> + state->core.vcx_events[i] = 0;
> +}
> +
> +/* Print event counters */
> +static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> + unsigned int i;
> +
> + for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> + if (core->events[i] > 0) {
> + dev_info(core->dev, "%s events: %d\n",
> + xcsi2rxss_events[i].name,
> + core->events[i]);
> + }
> + }
> +
> + if (core->en_vcx) {
> + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> + if (core->vcx_events[i] > 0) {
> + dev_info(core->dev,
> + "VC %d Frame %s err vcx events: %d\n",
> + (i / 2) + XCSI_VCX_START,
> + i & 1 ? "Sync" : "Level",
> + core->vcx_events[i]);
> + }
> + }
> + }
> +}
> +
> +static void xcsi2rxss_log_ipconfig(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> +
> + dev_dbg(core->dev, "****** Xilinx MIPI CSI2 Rx SS IP Config ******\n");
> + dev_dbg(core->dev, "vcx is %s", core->en_vcx ? "enabled" : "disabled");
> + dev_dbg(core->dev, "Enable active lanes property is %s\n",
> + core->enable_active_lanes ? "present" : "absent");
> + dev_dbg(core->dev, "Max lanes = %d", core->max_num_lanes);
> + dev_dbg(core->dev, "Pixel format set as 0x%x\n", core->datatype);
> + dev_dbg(core->dev, "**********************************************\n");

How about making this more compact ?

dev_dbg(core->dev, "vcx %s, %u data lanes (%s), data type 0x%02x\n",
core->en_vcx ? "enabled" : "disabled",
core->max_num_lanes,
core->enable_active_lanes ? "dynamic" : "static",
core->datatype);

> +}
> +
> +/**
> + * xcsi2rxss_log_status - Logs the status of the CSI-2 Receiver
> + * @sd: Pointer to V4L2 subdevice structure
> + *
> + * This function prints the current status of Xilinx MIPI CSI-2
> + *
> + * Return: 0 on success
> + */
> +static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> + u32 reg, data;
> + unsigned int i, max_vc;
> +
> + mutex_lock(&xcsi2rxss->lock);
> +
> + xcsi2rxss_log_ipconfig(xcsi2rxss);

Do we need to print this here, as it's printed at probe time already and
only contains static data ? I would inline xcsi2rxss_log_ipconfig() in
xcsi2rxss_parse_of().

> +
> + xcsi2rxss_log_counters(xcsi2rxss);
> +
> + dev_info(core->dev, "***** Core Status *****\n");
> + data = xcsi2rxss_read(core, XCSI_CSR_OFFSET);
> + dev_info(core->dev, "Short Packet FIFO Full = %s\n",
> + XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFOFULL));
> + dev_info(core->dev, "Short Packet FIFO Not Empty = %s\n",
> + XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFONE));
> + dev_info(core->dev, "Stream line buffer full = %s\n",
> + XCSI_GET_BITSET_STR(data, XCSI_CSR_SLBF));
> + dev_info(core->dev, "Soft reset/Core disable in progress = %s\n",
> + XCSI_GET_BITSET_STR(data, XCSI_CSR_RIPCD));
> +
> + /* Clk & Lane Info */
> + dev_info(core->dev, "******** Clock Lane Info *********\n");
> + data = xcsi2rxss_read(core, XCSI_CLKINFR_OFFSET);
> + dev_info(core->dev, "Clock Lane in Stop State = %s\n",
> + XCSI_GET_BITSET_STR(data, XCSI_CLKINFR_STOP));
> +
> + dev_info(core->dev, "******** Data Lane Info *********\n");
> + dev_info(core->dev, "Lane\tSoT Error\tSoT Sync Error\tStop State\n");
> + reg = XCSI_DLXINFR_OFFSET;
> + for (i = 0; i < XCSI_MAXDL_COUNT; i++) {
> + data = xcsi2rxss_read(core, reg);
> +
> + dev_info(core->dev, "%d\t%s\t\t%s\t\t%s\n", i,
> + XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTERR),
> + XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTSYNCERR),
> + XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_STOP));
> +
> + reg += XCSI_NEXTREG_OFFSET;
> + }
> +
> + /* Virtual Channel Image Information */
> + dev_info(core->dev, "********** Virtual Channel Info ************\n");
> + dev_info(core->dev, "VC\tLine Count\tByte Count\tData Type\n");
> + if (core->en_vcx)
> + max_vc = XCSI_MAX_VCX;
> + else
> + max_vc = XCSI_MAX_VC;
> +
> + reg = XCSI_VCXINF1R_OFFSET;
> + for (i = 0; i < max_vc; i++) {
> + u32 line_count, byte_count, data_type;
> +
> + /* Get line and byte count from VCXINFR1 Register */
> + data = xcsi2rxss_read(core, reg);
> + byte_count = data & XCSI_VCXINF1R_BYTECOUNT;
> + line_count = data & XCSI_VCXINF1R_LINECOUNT;
> + line_count >>= XCSI_VCXINF1R_LINECOUNT_SHIFT;
> +
> + /* Get data type from VCXINFR2 Register */
> + reg += XCSI_NEXTREG_OFFSET;
> + data = xcsi2rxss_read(core, reg);
> + data_type = data & XCSI_VCXINF2R_DT;
> +
> + dev_info(core->dev, "%d\t%d\t\t%d\t\t0x%x\n", i, line_count,
> + byte_count, data_type);
> +
> + /* Move to next pair of VC Info registers */
> + reg += XCSI_NEXTREG_OFFSET;
> + }
> +
> + mutex_unlock(&xcsi2rxss->lock);
> +
> + return 0;
> +}
> +
> +static struct v4l2_subdev *xcsi2rxss_get_remote_subdev(struct media_pad *local)
> +{
> + struct media_pad *remote;
> +
> + remote = media_entity_remote_pad(local);
> + if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> + return NULL;
> +
> + return media_entity_to_v4l2_subdev(remote->entity);
> +}
> +
> +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> + struct v4l2_subdev *rsubdev;
> + int ret = 0;
> +
> + xcsi2rxss_enable(core);
> +
> + ret = xcsi2rxss_reset(core);
> + if (ret < 0) {
> + state->streaming = false;
> + return ret;
> + }
> +
> + xcsi2rxss_intr_enable(core);
> + state->streaming = true;
> +
> + rsubdev = xcsi2rxss_get_remote_subdev(&state->pads[XVIP_PAD_SINK]);

How about storing the remote subdev pointer in the xcsi2rxss_state
structure, either at probe time, or at streamon time ?

> + ret = v4l2_subdev_call(rsubdev, video, s_stream, 1);

If this failes, you should disabled interrupts, disable the receiver,
and set state->streaming to false. I would actually set state->streaming
= enable at the end of xcsi2rxss_s_stream() instead of here and in
xcsi2rxss_stop_stream().

> +
> + return ret;
> +}
> +
> +static void xcsi2rxss_stop_stream(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> + struct v4l2_subdev *rsubdev;
> +
> + rsubdev = xcsi2rxss_get_remote_subdev(&state->pads[XVIP_PAD_SINK]);
> + v4l2_subdev_call(rsubdev, video, s_stream, 0);
> +
> + xcsi2rxss_intr_disable(core);
> + xcsi2rxss_disable(core);
> + state->streaming = false;
> +}
> +
> +/**
> + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
> + * @irq: IRQ number
> + * @dev_id: Pointer to device state
> + *
> + * In the interrupt handler, a list of event counters are updated for
> + * corresponding interrupts. This is useful to get status / debug.
> + *
> + * Return: IRQ_HANDLED after handling interrupts
> + * IRQ_NONE is no interrupts
> + */
> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> +{
> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> + struct xcsi2rxss_core *core = &state->core;
> + u32 status;
> +
> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);

As this is expected to occur for every frame, I would drop the message,
even if rate-limited.

> +
> + if (!status)
> + return IRQ_NONE;
> +
> + /* Received a short packet */
> + if (status & XCSI_ISR_SPFIFONE) {
> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> + }

Same here, this will occur all the time, I'd remove this message. You
need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
multiple short packets are received before the interrupt handler
executes.

I also wonder if it would make sense to extract the frame number from
the FS short packet, and make it available through the subdev API. I
think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
can be implemented later.

> +
> + /* Short packet FIFO overflow */
> + if (status & XCSI_ISR_SPFIFOF)
> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
> +
> + /*
> + * Stream line buffer full
> + * This means there is a backpressure from downstream IP
> + */
> + if (status & XCSI_ISR_SLBF) {
> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
> + xcsi2rxss_stop_stream(state);
> + if (core->rst_gpio) {
> + gpiod_set_value(core->rst_gpio, 1);
> + /* minimum 40 dphy_clk_200M cycles */
> + ndelay(250);
> + gpiod_set_value(core->rst_gpio, 0);
> + }

I don't think you should stop the core here. xcsi2rxss_stop_stream()
calls the source .s_stream(0) operation, which usually involves I2C
writes that will sleep.

You should instead report an event to userspace (it looks like we have
no error event defined in V4L2, one should be added), and rely on the
normal stop procedure.

> + }
> +
> + /* Increment event counters */
> + if (status & XCSI_ISR_ALLINTR_MASK) {
> + unsigned int i;
> +
> + for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> + if (!(status & xcsi2rxss_events[i].mask))
> + continue;
> + core->events[i]++;
> + dev_dbg_ratelimited(core->dev, "%s: %d\n",

The counter is unsigned, this should be %u instead of %d.

> + xcsi2rxss_events[i].name,
> + core->events[i]);
> + }
> +
> + if (status & XCSI_ISR_VCXFE && core->en_vcx) {
> + u32 vcxstatus;
> +
> + vcxstatus = xcsi2rxss_read(core, XCSI_VCXR_OFFSET);
> + vcxstatus &= XCSI_VCXR_VCERR;
> + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> + if (!(vcxstatus & (1 << i)))
> + continue;
> + core->vcx_events[i]++;
> + }
> + xcsi2rxss_write(core, XCSI_VCXR_OFFSET, vcxstatus);
> + }
> + }
> +
> + xcsi2rxss_write(core, XCSI_ISR_OFFSET, status);

To lower the probability of losing events, shouldn't you write the
register right after reading it above ? Same for XCSI_VCXR_OFFSET.

> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * xcsi2rxss_s_stream - It is used to start/stop the streaming.
> + * @sd: V4L2 Sub device
> + * @enable: Flag (True / False)
> + *
> + * This function controls the start or stop of streaming for the
> + * Xilinx MIPI CSI-2 Rx Subsystem.
> + *
> + * Return: 0 on success, errors otherwise
> + */
> +static int xcsi2rxss_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> + int ret = 0;
> +
> + mutex_lock(&xcsi2rxss->lock);
> +

You could add

if (enable == xcsi2rxss->streaming)
goto done;

with a done label just before mutex_unlock(), and simplify the code
below by removing the xcsi2rxss->streaming checks.

> + if (enable) {
> + if (!xcsi2rxss->streaming) {
> + /* reset the event counters */
> + xcsi2rxss_reset_event_counters(xcsi2rxss);
> + ret = xcsi2rxss_start_stream(xcsi2rxss);
> + }
> + } else {
> + if (xcsi2rxss->streaming) {
> + struct gpio_desc *rst = xcsi2rxss->core.rst_gpio;
> +
> + xcsi2rxss_stop_stream(xcsi2rxss);
> + if (rst) {
> + gpiod_set_value_cansleep(rst, 1);
> + usleep_range(1, 2);
> + gpiod_set_value_cansleep(rst, 0);
> + }
> + }
> + }
> +
> + mutex_unlock(&xcsi2rxss->lock);
> + return ret;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +__xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad, u32 which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(&xcsi2rxss->subdev, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &xcsi2rxss->format;
> + default:
> + return NULL;
> + }
> +}
> +
> +/**
> + * xcsi2rxss_get_format - Get the pad format
> + * @sd: Pointer to V4L2 Sub device structure
> + * @cfg: Pointer to sub device pad information structure
> + * @fmt: Pointer to pad level media bus format
> + *
> + * This function is used to get the pad format information.
> + *
> + * Return: 0 on success
> + */
> +static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> +
> + mutex_lock(&xcsi2rxss->lock);
> + fmt->format = *__xcsi2rxss_get_pad_format(xcsi2rxss, cfg, fmt->pad,
> + fmt->which);
> + mutex_unlock(&xcsi2rxss->lock);
> +
> + return 0;
> +}
> +
> +/**
> + * xcsi2rxss_set_format - This is used to set the pad format
> + * @sd: Pointer to V4L2 Sub device structure
> + * @cfg: Pointer to sub device pad information structure
> + * @fmt: Pointer to pad level media bus format
> + *
> + * This function is used to set the pad format. Since the pad format is fixed
> + * in hardware, it can't be modified on run time. So when a format set is
> + * requested by application, all parameters except the format type is saved
> + * for the pad and the original pad format is sent back to the application.
> + *
> + * Return: 0 on success
> + */
> +static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> + struct v4l2_mbus_framefmt *__format;
> +
> + /* only sink pad format can be updated */
> + mutex_lock(&xcsi2rxss->lock);
> +
> + /*
> + * Only the format->code parameter matters for CSI as the
> + * CSI format cannot be changed at runtime.
> + * Ensure that format to set is copied to over to CSI pad format
> + */
> + __format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
> + fmt->pad, fmt->which);
> +
> + if (fmt->pad == XVIP_PAD_SOURCE) {
> + fmt->format = *__format;
> + mutex_unlock(&xcsi2rxss->lock);
> + return 0;
> + }
> +
> + /*
> + * RAW8 is supported in all datatypes. So if requested media bus format
> + * is of RAW8 type, then allow to be set. In case core is configured to
> + * other RAW, YUV422 8/10 or RGB888, set appropriate media bus format.
> + */
> + if (!((fmt->format.code == MEDIA_BUS_FMT_SBGGR8_1X8 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGBRG8_1X8 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGRBG8_1X8 ||
> + fmt->format.code == MEDIA_BUS_FMT_SRGGB8_1X8) ||
> + (core->datatype == XCSI_DT_RAW10 &&
> + (fmt->format.code == MEDIA_BUS_FMT_SBGGR10_1X10 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> + fmt->format.code == MEDIA_BUS_FMT_SRGGB10_1X10)) ||
> + (core->datatype == XCSI_DT_RAW12 &&
> + (fmt->format.code == MEDIA_BUS_FMT_SBGGR12_1X12 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGBRG12_1X12 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGRBG12_1X12 ||
> + fmt->format.code == MEDIA_BUS_FMT_SRGGB12_1X12)) ||
> + (core->datatype == XCSI_DT_RAW14 &&
> + (fmt->format.code == MEDIA_BUS_FMT_SBGGR14_1X14 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGBRG14_1X14 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGRBG14_1X14 ||
> + fmt->format.code == MEDIA_BUS_FMT_SRGGB14_1X14)) ||
> + (core->datatype == XCSI_DT_RAW16 &&
> + (fmt->format.code == MEDIA_BUS_FMT_SBGGR16_1X16 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGBRG16_1X16 ||
> + fmt->format.code == MEDIA_BUS_FMT_SGRBG16_1X16 ||
> + fmt->format.code == MEDIA_BUS_FMT_SRGGB16_1X16)) ||
> + (core->datatype == XCSI_DT_YUV4228B &&
> + fmt->format.code == MEDIA_BUS_FMT_UYVY8_1X16) ||
> + (core->datatype == XCSI_DT_YUV42210B &&
> + fmt->format.code == MEDIA_BUS_FMT_UYVY10_1X20) ||
> + (core->datatype == XCSI_DT_RGB888 &&
> + fmt->format.code == MEDIA_BUS_FMT_RBG888_1X24))) {

I think you should create a static const table of format information,
mapping media bus codes to CSI-2 data types. It can be useful here, in
xcsi2rxss_set_default_format() and in xcsi2rxss_init_mbus_fmts().

> + /* Restore the original pad format code */
> + dev_dbg(core->dev, "Unsupported media bus format");
> + fmt->format.code = __format->code;

You should use the default here instead of the current format.

> + }
> +
> + *__format = fmt->format;
> + mutex_unlock(&xcsi2rxss->lock);
> +
> + return 0;
> +}
> +
> +/*
> + * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
> + * @sd : pointer to v4l2 subdev structure
> + * @cfg: V4L2 subdev pad configuration
> + * @code : pointer to v4l2_subdev_mbus_code_enum structure
> + *
> + * Return: -EINVAL or zero on success
> + */
> +int xcsi2rxss_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct xcsi2rxss_state *state = to_xcsi2rxssstate(sd);
> +
> + if (code->index >= state->mbus_fmts_count)
> + return -EINVAL;
> +
> + code->code = state->mbus_fmts[code->index];

Instead of storing mbus_fmts and mbus_fmts_count in the state, how about
moving the logic from xcsi2rxss_init_mbus_fmts() to here ?

> +
> + return 0;
> +}
> +
> +/**
> + * xcsi2rxss_open - Called on v4l2_open()
> + * @sd: Pointer to V4L2 sub device structure
> + * @fh: Pointer to V4L2 File handle
> + *
> + * This function is called on v4l2_open(). It sets the default format
> + * for both pads.
> + *
> + * Return: 0 on success
> + */
> +static int xcsi2rxss_open(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> + struct v4l2_mbus_framefmt *format;
> + unsigned int i;
> +
> + for (i = 0; i < XCSI_MEDIA_PADS; i++) {
> + format = v4l2_subdev_get_try_format(sd, fh->pad, i);
> + *format = xcsi2rxss->default_format;
> + }

I would recommend moving this logic to the .init_cfg() pad operation,
and removing the .open() internal operation.

> +
> + return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Media Operations
> + */
> +
> +static const struct media_entity_operations xcsi2rxss_media_ops = {
> + .link_validate = v4l2_subdev_link_validate
> +};
> +
> +static const struct v4l2_subdev_core_ops xcsi2rxss_core_ops = {
> + .log_status = xcsi2rxss_log_status,
> +};
> +
> +static const struct v4l2_subdev_video_ops xcsi2rxss_video_ops = {
> + .s_stream = xcsi2rxss_s_stream
> +};
> +
> +static const struct v4l2_subdev_pad_ops xcsi2rxss_pad_ops = {
> + .get_fmt = xcsi2rxss_get_format,
> + .set_fmt = xcsi2rxss_set_format,
> + .enum_mbus_code = xcsi2rxss_enum_mbus_code,
> + .link_validate = v4l2_subdev_link_validate_default,
> +};
> +
> +static const struct v4l2_subdev_ops xcsi2rxss_ops = {
> + .core = &xcsi2rxss_core_ops,
> + .video = &xcsi2rxss_video_ops,
> + .pad = &xcsi2rxss_pad_ops
> +};
> +
> +static const struct v4l2_subdev_internal_ops xcsi2rxss_internal_ops = {
> + .open = xcsi2rxss_open,
> +};
> +
> +static void xcsi2rxss_set_default_format(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> +
> + memset(&state->default_format, 0, sizeof(state->default_format));

state is kzalloc'ed, so you can skip this.

> +
> + switch (core->datatype) {
> + case XCSI_DT_YUV4228B:
> + state->default_format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> + break;
> + case XCSI_DT_RGB888:
> + state->default_format.code = MEDIA_BUS_FMT_RBG888_1X24;
> + break;
> + case XCSI_DT_YUV42210B:
> + state->default_format.code = MEDIA_BUS_FMT_UYVY10_1X20;
> + break;
> + case XCSI_DT_RAW10:
> + state->default_format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> + break;
> + case XCSI_DT_RAW12:
> + state->default_format.code = MEDIA_BUS_FMT_SRGGB12_1X12;
> + break;
> + case XCSI_DT_RAW14:
> + state->default_format.code = MEDIA_BUS_FMT_SRGGB14_1X14;
> + break;
> + case XCSI_DT_RAW16:
> + state->default_format.code = MEDIA_BUS_FMT_SRGGB16_1X16;
> + break;
> + case XCSI_DT_RAW8:
> + case XCSI_DT_RGB444:
> + case XCSI_DT_RGB555:
> + case XCSI_DT_RGB565:
> + case XCSI_DT_RGB666:
> + state->default_format.code = MEDIA_BUS_FMT_SRGGB8_1X8;

This is correct for XCSI_DT_RAW8 but not for the other RGB data types.

> + break;
> + }
> +
> + state->default_format.field = V4L2_FIELD_NONE;
> + state->default_format.colorspace = V4L2_COLORSPACE_SRGB;
> + state->default_format.width = XCSI_DEFAULT_WIDTH;
> + state->default_format.height = XCSI_DEFAULT_HEIGHT;
> +
> + dev_dbg(core->dev, "default mediabus format = 0x%x",
> + state->default_format.code);

I'd drop this as there's already a debug message that prints the data
type.

> +}
> +
> +static void xcsi2rxss_init_mbus_fmts(struct xcsi2rxss_state *state)
> +{
> + struct xcsi2rxss_core *core = &state->core;
> +
> + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB8_1X8);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR8_1X8);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG8_1X8);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG8_1X8);
> +
> + switch (core->datatype) {
> + case XCSI_DT_RAW10:
> + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB10_1X10);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR10_1X10);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG10_1X10);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG10_1X10);
> + break;
> + case XCSI_DT_RAW12:
> + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB12_1X12);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR12_1X12);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG12_1X12);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG12_1X12);
> + break;
> + case XCSI_DT_RAW14:
> + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB14_1X14);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR14_1X14);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG14_1X14);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG14_1X14);
> + break;
> + case XCSI_DT_RAW16:
> + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB16_1X16);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR16_1X16);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG16_1X16);
> + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG16_1X16);
> + break;
> + case XCSI_DT_YUV4228B:
> + XADD_MBUS(state, MEDIA_BUS_FMT_UYVY8_1X16);
> + break;
> + case XCSI_DT_RGB888:
> + XADD_MBUS(state, MEDIA_BUS_FMT_RBG888_1X24);
> + break;
> + case XCSI_DT_YUV42210B:
> + XADD_MBUS(state, MEDIA_BUS_FMT_UYVY10_1X20);
> + break;
> + default:
> + dev_err(core->dev, "Invalid data type!\n");
> + }
> +}
> +
> +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> +{
> + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> + struct device_node *node = xcsi2rxss->core.dev->of_node;
> + unsigned int nports, irq;
> + bool en_csi_v20, vfb;
> + int ret;
> +
> + en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
> + if (en_csi_v20)
> + core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx");
> +
> + core->enable_active_lanes =
> + of_property_read_bool(node, "xlnx,en-active-lanes");
> +
> + ret = of_property_read_u32(node, "xlnx,csi-pxl-format",
> + &core->datatype);
> + if (ret < 0) {
> + dev_err(core->dev, "missing xlnx,csi-pxl-format property\n");
> + return ret;
> + }
> +
> + switch (core->datatype) {
> + case XCSI_DT_YUV4228B:
> + case XCSI_DT_RGB444:
> + case XCSI_DT_RGB555:
> + case XCSI_DT_RGB565:
> + case XCSI_DT_RGB666:
> + case XCSI_DT_RGB888:
> + case XCSI_DT_RAW6:
> + case XCSI_DT_RAW7:
> + case XCSI_DT_RAW8:
> + case XCSI_DT_RAW10:
> + case XCSI_DT_RAW12:
> + case XCSI_DT_RAW14:
> + break;
> + case XCSI_DT_YUV42210B:
> + case XCSI_DT_RAW16:
> + case XCSI_DT_RAW20:
> + if (!en_csi_v20) {
> + ret = -EINVAL;
> + dev_dbg(core->dev, "enable csi v2 for this pixel format");
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + if (ret < 0) {
> + dev_err(core->dev, "invalid csi-pxl-format property!\n");
> + return ret;
> + }
> +
> + vfb = of_property_read_bool(node, "xlnx,vfb");
> + if (!vfb) {
> + dev_err(core->dev, "failed as VFB is disabled!\n");

I'd write this "operation without VFB is not supported\n". Do you plan
to add support for this later ?

> + return -EINVAL;
> + }
> +
> + for (nports = 0; nports < XCSI_MEDIA_PADS; nports++) {
> + struct fwnode_handle *ep;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(core->dev),
> + nports, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + break;
> + /*
> + * since first port is sink port and it contains
> + * all info about data-lanes and cfa-pattern,
> + * don't parse second port but only check if exists
> + */
> + if (nports == XVIP_PAD_SOURCE) {
> + dev_dbg(core->dev, "no need to parse source port");
> + fwnode_handle_put(ep);
> + continue;
> + }

How about removing the loop and only handling port 0 then ?

> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);

You can add

fwnode_handle_put(ep);

here,

> + if (ret) {
> + dev_err(core->dev, "error parsing sink port");
> + fwnode_handle_put(ep);

and drop it from there and from the end of the loop.

> + return ret;
> + }
> +
> + dev_dbg(core->dev, "port %d bus type = %d\n", nports,
> + vep.bus_type);

As bus_type is hardcoded to V4L2_MBUS_CSI2_DPHY and nports can always be
0 here, I think you can drop this message.

> +
> + if (vep.bus_type == V4L2_MBUS_CSI2_DPHY) {

Can bus_type be different than V4L2_MBUS_CSI2_DPHY here, as you set it
when initializing vep above ? I think you can remove the condition
check, as well as the first debug message below.

> + dev_dbg(core->dev, "base.port = %d base.id = %d\n",
> + vep.base.port, vep.base.id);
> +
> + dev_dbg(core->dev, "mipi number lanes = %d\n",
> + vep.bus.mipi_csi2.num_data_lanes);
> +
> + core->max_num_lanes =
> + vep.bus.mipi_csi2.num_data_lanes;
> + }
> + fwnode_handle_put(ep);
> + }
> +
> + if (nports != XCSI_MEDIA_PADS) {
> + dev_err(core->dev, "invalid number of ports %u\n", nports);
> + return -EINVAL;
> + }

I think we can drop this check too.

> +
> + /* Register interrupt handler */
> + irq = irq_of_parse_and_map(node, 0);

This should be turned into platform_get_irq().

> + ret = devm_request_irq(core->dev, irq, xcsi2rxss_irq_handler,
> + IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss);
> + if (ret) {
> + dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n",
> + ret);
> + return ret;
> + }

And IRQ handling should be moved to xcsi2rxss_probe() as it's not
related to OF parsing.

> +
> + xcsi2rxss_log_ipconfig(xcsi2rxss);
> +
> + return 0;
> +}
> +
> +static int xcsi2rxss_probe(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *subdev;
> + struct xcsi2rxss_state *xcsi2rxss;
> + struct xcsi2rxss_core *core;
> + struct resource *res;
> + int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
> + int ret;
> +
> + xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL);
> + if (!xcsi2rxss)
> + return -ENOMEM;
> +
> + core = &xcsi2rxss->core;
> + core->dev = &pdev->dev;
> +
> + core->clks = devm_kmemdup(core->dev, xcsi2rxss_clks,
> + sizeof(xcsi2rxss_clks), GFP_KERNEL);
> + if (!core->clks)
> + return -ENOMEM;
> +
> + /* Reset GPIO */
> + core->rst_gpio = devm_gpiod_get_optional(core->dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(core->rst_gpio)) {
> + if (PTR_ERR(core->rst_gpio) != -EPROBE_DEFER)
> + dev_err(core->dev, "Video Reset GPIO not setup in DT");
> + return PTR_ERR(core->rst_gpio);
> + }
> +
> + mutex_init(&xcsi2rxss->lock);
> +
> + ret = xcsi2rxss_parse_of(xcsi2rxss);
> + if (ret < 0)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + core->iomem = devm_ioremap_resource(core->dev, res);

You can replace those two lines with

core->iomem = devm_platform_ioremap_resource(pdev, 0);

> + if (IS_ERR(core->iomem))
> + return PTR_ERR(core->iomem);
> +
> + ret = clk_bulk_get(core->dev, num_clks, core->clks);
> + if (ret)
> + return ret;
> +
> + ret = clk_bulk_prepare_enable(num_clks, core->clks);
> + if (ret)
> + goto err_clk_put;

Shouldn't be clock enabled when starting the stream, and disabled when
stopping it ?

> +
> + if (core->rst_gpio) {
> + gpiod_set_value_cansleep(core->rst_gpio, 1);
> + /* minimum of 40 dphy_clk_200M cycles */
> + usleep_range(1, 2);
> + gpiod_set_value_cansleep(core->rst_gpio, 0);
> + }

This is done in xcsi2rxss_s_stream() too, I would move the logic to a
xcsi2rxss_hard_reset() function.

> +
> + xcsi2rxss_reset(core);

And rename this xcsi2rxss_soft_reset();

> +
> + /* Initialize V4L2 subdevice and media entity */
> + xcsi2rxss->pads[XVIP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + xcsi2rxss->pads[XVIP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +
> + /* Initialize the default format */
> + xcsi2rxss_set_default_format(xcsi2rxss);
> + xcsi2rxss->format = xcsi2rxss->default_format;
> +
> + /* Initialize the mbus formats supported */
> + xcsi2rxss_init_mbus_fmts(xcsi2rxss);
> +
> + /* Initialize V4L2 subdevice and media entity */
> + subdev = &xcsi2rxss->subdev;
> + v4l2_subdev_init(subdev, &xcsi2rxss_ops);
> + subdev->dev = &pdev->dev;
> + subdev->internal_ops = &xcsi2rxss_internal_ops;
> + strscpy(subdev->name, dev_name(&pdev->dev), sizeof(subdev->name));
> + subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
> + subdev->entity.ops = &xcsi2rxss_media_ops;
> + v4l2_set_subdevdata(subdev, xcsi2rxss);
> +
> + ret = media_entity_pads_init(&subdev->entity, XCSI_MEDIA_PADS,
> + xcsi2rxss->pads);
> + if (ret < 0)
> + goto error;
> +
> + platform_set_drvdata(pdev, xcsi2rxss);
> +
> + ret = v4l2_async_register_subdev(subdev);
> + if (ret < 0) {
> + dev_err(core->dev, "failed to register subdev\n");
> + goto error;
> + }
> +
> + dev_info(core->dev, "Xilinx CSI2 Rx Subsystem device found!\n");

I'd drop the "!".

> +
> + return 0;
> +error:
> + media_entity_cleanup(&subdev->entity);
> + mutex_destroy(&xcsi2rxss->lock);

This should go below, just before return ret, and you should add another
error label there as mutex_destroy() should be called for a few error
cases that currently return directly.

> + clk_bulk_disable_unprepare(num_clks, core->clks);
> +err_clk_put:
> + clk_bulk_put(num_clks, core->clks);
> + return ret;
> +}
> +
> +static int xcsi2rxss_remove(struct platform_device *pdev)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = platform_get_drvdata(pdev);
> + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> + struct v4l2_subdev *subdev = &xcsi2rxss->subdev;
> + int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
> +
> + v4l2_async_unregister_subdev(subdev);
> + media_entity_cleanup(&subdev->entity);
> + mutex_destroy(&xcsi2rxss->lock);
> + clk_bulk_disable_unprepare(num_clks, core->clks);
> + clk_bulk_put(num_clks, core->clks);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id xcsi2rxss_of_id_table[] = {
> + { .compatible = "xlnx,mipi-csi2-rx-subsystem-5.0", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, xcsi2rxss_of_id_table);
> +
> +static struct platform_driver xcsi2rxss_driver = {
> + .driver = {
> + .name = "xilinx-csi2rxss",
> + .of_match_table = xcsi2rxss_of_id_table,
> + },
> + .probe = xcsi2rxss_probe,
> + .remove = xcsi2rxss_remove,
> +};
> +
> +module_platform_driver(xcsi2rxss_driver);
> +
> +MODULE_AUTHOR("Vishal Sagar <[email protected]>");
> +MODULE_DESCRIPTION("Xilinx MIPI CSI2 Rx Subsystem Driver");
> +MODULE_LICENSE("GPL v2");

--
Regards,

Laurent Pinchart

2020-04-20 18:05:16

[permalink] [raw]

Subject: RE: [PATCH v11 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Hi Laurent,

Thanks for reviewing.

> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Sunday, April 19, 2020 9:13 PM
> To: Vishal Sagar <[email protected]>
> Cc: Hyun Kwon <[email protected]>; [email protected];
> [email protected]; [email protected]; Michal Simek
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; Dinesh Kumar
> <[email protected]>; Sandip Kothari <[email protected]>; Luca Ceresoli
> <[email protected]>; Jacopo Mondi <[email protected]>; Hyun Kwon
> <[email protected]>; Rob Herring <[email protected]>
> Subject: Re: [PATCH v11 1/2] media: dt-bindings: media: xilinx: Add Xilinx
> MIPI CSI-2 Rx Subsystem
>
> Hi Vishal,
>
> Thank you for the patch.
>
> On Fri, Apr 10, 2020 at 01:14:23AM +0530, Vishal Sagar wrote:
> > Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
> >
> > The Xilinx MIPI CSI-2 Rx Subsystem consists of a CSI-2 Rx controller,
> > a DPHY in Rx mode, an optional I2C controller and a Video Format Bridge.
>
> The AXI IIC was removed from the subsystem in v4.1, you could drop it from
> the commit message too.
>

Agree. I will remove it.

> >
> > Signed-off-by: Vishal Sagar <[email protected]>
> > Reviewed-by: Hyun Kwon <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> > Reviewed-by: Luca Ceresoli <[email protected]>
> > ---
> > v11
> > - Modify compatible string from 4.0 to 5.0
> >
> > v10
> > - No changes
> >
> > v9
> > - Fix xlnx,vfb description.
> > - s/Optional/Required endpoint property.
> > - Move data-lanes description from Ports to endpoint property section.
> >
> > v8
> > - Added reset-gpios optional property to assert video_aresetn
> >
> > v7
> > - Removed the control name from dt bindings
> > - Updated the example dt node name to csi2rx
> >
> > v6
> > - Added "control" after V4L2_CID_XILINX_MIPICSISS_ACT_LANES as
> > suggested by Luca
> > - Added reviewed by Rob Herring
> >
> > v5
> > - Incorporated comments by Luca Cersoli
> > - Removed DPHY clock from description and example
> > - Removed bayer pattern from device tree MIPI CSI IP
> > doesn't deal with bayer pattern.
> >
> > v4
> > - Added reviewed by Hyun Kwon
> >
> > v3
> > - removed interrupt parent as suggested by Rob
> > - removed dphy clock
> > - moved vfb to optional properties
> > - Added required and optional port properties section
> > - Added endpoint property section
> >
> > v2
> > - updated the compatible string to latest version supported
> > - removed DPHY related parameters
> > - added CSI v2.0 related property (including VCX for supporting upto 16
> > virtual channels).
> > - modified csi-pxl-format from string to unsigned int type where the value
> > is as per the CSI specification
> > - Defined port 0 and port 1 as sink and source ports.
> > - Removed max-lanes property as suggested by Rob and Sakari
> > .../bindings/media/xilinx/xlnx,csi2rxss.txt | 116 ++++++++++++++++++
> > 1 file changed, 116 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> > b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> > new file mode 100644
> > index 000000000000..9269a5c880aa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
>
> YAML is the recommended form for new DT bindings. This wasn't a
> requirement when the first version of this series was submitted, and I
> understand it can be frustrating to chase a moving target, so I can help with
> the YAML conversion once we sort out the questions below.
>

Sure. I will try to convert to YAML and send in next patch.

> > @@ -0,0 +1,116 @@
> > +Xilinx MIPI CSI2 Receiver Subsystem Device Tree Bindings
>
> Nitpicking, it's CSI-2, not CSI2.
>
> > +--------------------------------------------------------
> > +
> > +The Xilinx MIPI CSI2 Receiver Subsystem is used to capture MIPI CSI2
> > +traffic from compliant camera sensors and send the output as AXI4
> > +Stream video data for image processing.
> > +
> > +The subsystem consists of a MIPI DPHY in slave mode which captures
> > +the
>
> And D-PHY, not DPHY :-)
>

Ok. I will update the two here and elsewhere.

> > +data packets. This is passed along the MIPI CSI2 Rx IP which extracts
> > +the packet data. The optional Video Format Bridge (VFB) converts this
> > +data to
> > +AXI4 Stream video data.
> > +
> > +For more details, please refer to PG232 Xilinx MIPI CSI-2 Receiver
> Subsystem.
>
> If I understand correctly, this DT binding covers the CSI-2 RX Controller and
> the optional Video Format Bridge, but leaves the D-PHY out, right ? I think
> this should be clarified, as the "CSI-2 receiver subsystem" includes the D-PHY.
>

Ok I will add this line.

> > +
> > +Required properties:
> > +--------------------
> > +- compatible: Must contain "xlnx,mipi-csi2-rx-subsystem-5.0".
>
> Is PG232 v5.0 available ? The most recent version I've found was PG232 v4.1.
>

No not yet. This is the most recent one that we are releasing.
Refer to https://github.com/Xilinx/linux-xlnx/blob/master/drivers/media/platform/xilinx/xilinx-csi2rxss.c

> > +- reg: Physical base address and length of the registers set for the device.
> > +- interrupts: Property with a value describing the interrupt number.
> > +- clocks: List of phandles to AXI Lite and Video clocks.
> > +- clock-names: Must contain "lite_aclk" and "video_aclk" in the same
> > +order
> > + as clocks listed in clocks property.
>
> The subsystem documentation also mentions a dphy_clk_200M. Is that
> routed to the D-PHY only, or is it also needed for the CSI-2 RX ?
>

The dphy_clk_200M is only for D-PHY.

> > +- xlnx,csi-pxl-format: This denotes the CSI Data type selected in hw design.
> > + Packets other than this data type (except for RAW8 and User defined
> > +data
> > + types) will be filtered out. Possible values are as below -
> > + 0x1E - YUV4228B
> > + 0x1F - YUV42210B
> > + 0x20 - RGB444
> > + 0x21 - RGB555
> > + 0x22 - RGB565
> > + 0x23 - RGB666
> > + 0x24 - RGB888
> > + 0x28 - RAW6
> > + 0x29 - RAW7
> > + 0x2A - RAW8
> > + 0x2B - RAW10
> > + 0x2C - RAW12
> > + 0x2D - RAW14
> > + 0x2E - RAW16
> > + 0x2F - RAW20
>
> Isn't this property required only when the VFB is present ?

This will be present irrespective of VFB being enabled.
With VFB, the data on the bus will be as per Xilinx UG934 which is similar to media bus formats.

Without VFB, it will just be plain data as it comes in data packets. Refer to the Xilinx PG 232
"Pixel Packing When Video Format Bridge is Not Present"

So the driver is currently made to load only in case VFB is enabled.

>
> > +
> > +
> > +Optional properties:
> > +--------------------
> > +- xlnx,vfb: Present when Video Format Bridge is enabled in IP
> > +configuration
> > +- xlnx,en-csi-v2-0: Present if CSI v2 is enabled in IP configuration.
>
> Unless I'm mistaken, this feature is available starting at v4 of the IP core.
>
Correct.

> > +- xlnx,en-vcx: When present, there are maximum 16 virtual channels,
> > +else
> > + only 4. This is present only if xlnx,en-csi-v2-0 is present.
> > +- xlnx,en-active-lanes: present if the number of active lanes can be
> > + re-configured at runtime in the Protocol Configuration Register.
> > + Otherwise all lanes, as set in IP configuration, are always active.
> > +- reset-gpios: Optional specifier for a GPIO that asserts video_aresetn.
>
> Should lite_aresetn also be supported ? We can add a lite-reset-gpios
> property later, but maybe we should name this video-reset-gpios ? As the
> video_aresetn signal is the main reset I don't mind keeping the name reset-
> gpios either. It's up to you.
>

Yes I agree. I will rename this as video-reset-gpios in next version.

> > +
> > +Ports
> > +-----
> > +The device node shall contain two 'port' child nodes as defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +
> > +The port@0 is a sink port and shall connect to CSI2 source like camera.
> > +
> > +The port@1 is a source port and can be connected to any video
> > +processing IP which can work with AXI4 Stream data.
> > +
> > +Required port properties:
> > +--------------------
> > +- reg: 0 - for sink port.
> > + 1 - for source port.
>
> Don't you need a second source port for embedded non-image data ? If my
> understanding is correct that port can be enabled or disabled through the
> CSI_EMB_NON_IMG parameter, so it should be optional in DT too. We can
> possibly leave it out for now, it can be added later in a backward-compatible
> way.
>

Correct. The extra port is present only when CSI_EMB_NON_IMG parameter is enabled.
This can be added later.

> > +
> > +Required endpoint property:
> > +---------------------------
> > +- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-
> interfaces.txt.
> > + This is required only in the sink port 0 endpoint which connects to
> > +MIPI CSI2
> > + source like sensor. The possible values are:
> > + 1 - For 1 lane enabled in IP.
> > + 1 2 - For 2 lanes enabled in IP.
> > + 1 2 3 - For 3 lanes enabled in IP.
> > + 1 2 3 4 - For 4 lanes enabled in IP.
> > +
> > +Example:
> > +
> > + xcsi2rxss_1: csi2rx@a0020000 {
> > + compatible = "xlnx,mipi-csi2-rx-subsystem-5.0";
> > + reg = <0x0 0xa0020000 0x0 0x10000>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 95 4>;
> > + xlnx,csi-pxl-format = <0x2a>;
> > + xlnx,vfb;
> > + xlnx,en-active-lanes;
> > + xlnx,en-csi-v2-0;
> > + xlnx,en-vcx;
> > + clock-names = "lite_aclk", "video_aclk";
> > + clocks = <&misc_clk_0>, <&misc_clk_1>;
> > + reset-gpios = <&gpio 86 GPIO_ACTIVE_LOW>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + /* Sink port */
> > + reg = <0>;
> > + csiss_in: endpoint {
> > + data-lanes = <1 2 3 4>;
> > + /* MIPI CSI2 Camera handle */
> > + remote-endpoint = <&camera_out>;
> > + };
> > + };
> > + port@1 {
> > + /* Source port */
> > + reg = <1>;
> > + csiss_out: endpoint {
> > + remote-endpoint = <&vproc_in>;
> > + };
> > + };
> > + };
> > + };
>
> --
> Regards,
>
> Laurent Pinchart

Regards
Vishal Sagar

2020-04-20 18:10:56

[permalink] [raw]

Subject: RE: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Laurent,

Thanks for reviewing the patch in depth.

> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Sunday, April 19, 2020 11:32 PM
> To: Vishal Sagar <[email protected]>
> Cc: Hyun Kwon <[email protected]>; [email protected];
> [email protected]; [email protected]; Michal Simek
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; Dinesh Kumar
> <[email protected]>; Sandip Kothari <[email protected]>; Luca Ceresoli
> <[email protected]>; Jacopo Mondi <[email protected]>; Hyun Kwon
> <[email protected]>
> Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem driver
>
> Hi Vishal,
>
> Thank you for the patch.
>
> On Fri, Apr 10, 2020 at 01:14:24AM +0530, Vishal Sagar wrote:
> > The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> > from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> > for image processing. Please refer to PG232 for details.
> >
> > The CSI2 Rx controller filters out all packets except for the packets
> > with data type fixed in hardware. RAW8 packets are always allowed to
> > pass through.
> >
> > It is also used to setup and handle interrupts and enable the core. It
> > logs all the events in respective counters between streaming on and off.
> >
> > The driver supports only the video format bridge enabled configuration.
> > Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
> the
> > CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> > the maximum number of virtual channels becomes 16 from 4.
> >
> > Signed-off-by: Vishal Sagar <[email protected]>
> > Reviewed-by: Hyun Kwon <[email protected]>
> > ---
> > v11
> > - Fixed changes as suggested by Sakari Ailus
> > - Removed VIDEO_XILINX from KConfig
> > - Minor formatting
> > - Start / Stop upstream sub-device in xcsi2rxss_start_stream()
> > and xcsi2rxss_stop_stream()
> > - Added v4l2_subdev_link_validate_default() in v4l2_subdev_pad_ops()
> > - Use fwnode_graph_get_endpoint_by_id() instead of parsing by self
> > - Set bus type as V4L2_MBUS_CSI2_DPHY in struct v4l2_fwnode_endpoint
> > - Remove num_clks from core. Instead use ARRAY_SIZE()
> > - Fixed SPDX header to GPL-2.0
> > - Update copyright year to 2020
> >
> > v10
> > - Removed all V4L2 controls and events based on Sakari's comments.
> > - Now stop_stream() before toggling rst_gpio
> > - Updated init_mbus() to throw error on array out of bound access
> > - Make events and vcx_events as counters instead of structures
> > - Minor fixes in set_format() enum_mbus_code() as suggested by Sakari
> >
> > v9
> > - Moved all controls and events to xilinx-csi2rxss.h
> > - Updated name and description of controls and events
> > - Get control base address from v4l2-controls.h (0x10c0)
> > - Fix KConfig for dependency on VIDEO_XILINX
> > - Added enum_mbus_code() support
> > - Added default format to be returned on open()
> > - Mark variables are const
> > - Remove references to short packet in comments
> > - Add check for streaming before setting active lanes control
> > - strlcpy -> strscpy
> > - Fix xcsi2rxss_set_format()
> >
> > v8
> > - Use clk_bulk* APIs
> > - Add gpio reset for asserting video_aresetn when stream line buffer occurs
> > - Removed short packet related events and irq handling
> > - V4L2_EVENT_XLNXCSIRX_SPKT and V4L2_EVENT_XLNXCSIRX_SPKT_OVF
> removed
> > - Removed frame counter control
> V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER
> > and xcsi2rxss_g_volatile_ctrl()
> > - Minor formatting fixes
> >
> > v7
> > - No change
> >
> > v6
> > - No change
> >
> > v5
> > - Removed bayer and updated related parts like set default format based
> > on Luca Cersoli's comments.
> > - Added correct YUV422 10bpc media bus format
> >
> > v4
> > - Removed irq member from core structure
> > - Consolidated IP config prints in xcsi2rxss_log_ipconfig()
> > - Return -EINVAL in case of invalid ioctl
> > - Code formatting
> > - Added reviewed by Hyun Kwon
> >
> > v3
> > - Fixed comments given by Hyun.
> > - Removed DPHY 200 MHz clock. This will be controlled by DPHY driver
> > - Minor code formatting
> > - en_csi_v20 and vfb members removed from struct and made local to dt
> parsing
> > - lock description updated
> > - changed to ratelimited type for all dev prints in irq handler
> > - Removed YUV 422 10bpc media format
> >
> > v2
> > - Fixed comments given by Hyun and Sakari.
> > - Made all bitmask using BIT() and GENMASK()
> > - Removed unused definitions
> > - Removed DPHY access. This will be done by separate DPHY PHY driver.
> > - Added support for CSI v2.0 for YUV 422 10bpc, RAW16, RAW20 and extra
> > virtual channels
> > - Fixed the ports as sink and source
> > - Now use the v4l2fwnode API to get number of data-lanes
> > - Added clock framework support
> > - Removed the close() function
> > - updated the set format function
> > - support only VFB enabled configuration
> >
> > drivers/media/platform/xilinx/Kconfig | 10 +
> > drivers/media/platform/xilinx/Makefile | 1 +
> > .../media/platform/xilinx/xilinx-csi2rxss.c | 1234 +++++++++++++++++
> > 3 files changed, 1245 insertions(+)
> > create mode 100644 drivers/media/platform/xilinx/xilinx-csi2rxss.c
> >
> > diff --git a/drivers/media/platform/xilinx/Kconfig
> b/drivers/media/platform/xilinx/Kconfig
> > index a2773ad7c185..cd1a0fdde4df 100644
> > --- a/drivers/media/platform/xilinx/Kconfig
> > +++ b/drivers/media/platform/xilinx/Kconfig
> > @@ -10,6 +10,16 @@ config VIDEO_XILINX
> >
> > if VIDEO_XILINX
> >
> > +config VIDEO_XILINX_CSI2RXSS
> > + tristate "Xilinx CSI2 Rx Subsystem"
> > + help
> > + Driver for Xilinx MIPI CSI2 Rx Subsystem. This is a V4L sub-device
>
> As for the bindings, s/CSI2/CSI-2/ and s/DPHY/D-PHY/ through this patch.
>

Agreed. I will update in next patch version.

> > + based driver that takes input from CSI2 Tx source and converts
> > + it into an AXI4-Stream. The subsystem comprises of a CSI2 Rx
> > + controller, DPHY, an optional I2C controller and a Video Format
>
> The I2C controller has been removed in v4.1, it should be dropped from
> here.
>

Yes. I will update this in next version.

> > + Bridge. The driver is used to set the number of active lanes and
> > + get short packet data.
> > +
> > config VIDEO_XILINX_TPG
> > tristate "Xilinx Video Test Pattern Generator"
> > depends on VIDEO_XILINX
> > diff --git a/drivers/media/platform/xilinx/Makefile
> b/drivers/media/platform/xilinx/Makefile
> > index 4cdc0b1ec7a5..6119a34f3043 100644
> > --- a/drivers/media/platform/xilinx/Makefile
> > +++ b/drivers/media/platform/xilinx/Makefile
> > @@ -3,5 +3,6 @@
> > xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o
> >
> > obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o
> > +obj-$(CONFIG_VIDEO_XILINX_CSI2RXSS) += xilinx-csi2rxss.o
> > obj-$(CONFIG_VIDEO_XILINX_TPG) += xilinx-tpg.o
> > obj-$(CONFIG_VIDEO_XILINX_VTC) += xilinx-vtc.o
> > diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > new file mode 100644
> > index 000000000000..083422768ebd
> > --- /dev/null
> > +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > @@ -0,0 +1,1234 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Xilinx MIPI CSI2 Rx Subsystem
> > + *
> > + * Copyright (C) 2016 - 2020 Xilinx, Inc.
> > + *
> > + * Contacts: Vishal Sagar <[email protected]>
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/v4l2-subdev.h>
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-subdev.h>
> > +#include "xilinx-vip.h"
> > +
> > +/* Register register map */
> > +#define XCSI_CCR_OFFSET 0x00
> > +#define XCSI_CCR_SOFTRESET BIT(1)
> > +#define XCSI_CCR_ENABLE BIT(0)
> > +
> > +#define XCSI_PCR_OFFSET 0x04
> > +#define XCSI_PCR_MAXLANES_MASK GENMASK(4, 3)
> > +#define XCSI_PCR_ACTLANES_MASK GENMASK(1, 0)
> > +
> > +#define XCSI_CSR_OFFSET 0x10
> > +#define XCSI_CSR_PKTCNT GENMASK(31, 16)
> > +#define XCSI_CSR_SPFIFOFULL BIT(3)
> > +#define XCSI_CSR_SPFIFONE BIT(2)
> > +#define XCSI_CSR_SLBF BIT(1)
> > +#define XCSI_CSR_RIPCD BIT(0)
> > +
> > +#define XCSI_GIER_OFFSET 0x20
> > +#define XCSI_GIER_GIE BIT(0)
> > +
> > +#define XCSI_ISR_OFFSET 0x24
> > +#define XCSI_IER_OFFSET 0x28
> > +
> > +#define XCSI_ISR_FR BIT(31)
> > +#define XCSI_ISR_VCXFE BIT(30)
> > +#define XCSI_ISR_WCC BIT(22)
> > +#define XCSI_ISR_ILC BIT(21)
> > +#define XCSI_ISR_SPFIFOF BIT(20)
> > +#define XCSI_ISR_SPFIFONE BIT(19)
> > +#define XCSI_ISR_SLBF BIT(18)
> > +#define XCSI_ISR_STOP BIT(17)
> > +#define XCSI_ISR_SOTERR BIT(13)
> > +#define XCSI_ISR_SOTSYNCERR BIT(12)
> > +#define XCSI_ISR_ECC2BERR BIT(11)
> > +#define XCSI_ISR_ECC1BERR BIT(10)
> > +#define XCSI_ISR_CRCERR BIT(9)
> > +#define XCSI_ISR_DATAIDERR BIT(8)
> > +#define XCSI_ISR_VC3FSYNCERR BIT(7)
> > +#define XCSI_ISR_VC3FLVLERR BIT(6)
> > +#define XCSI_ISR_VC2FSYNCERR BIT(5)
> > +#define XCSI_ISR_VC2FLVLERR BIT(4)
> > +#define XCSI_ISR_VC1FSYNCERR BIT(3)
> > +#define XCSI_ISR_VC1FLVLERR BIT(2)
> > +#define XCSI_ISR_VC0FSYNCERR BIT(1)
> > +#define XCSI_ISR_VC0FLVLERR BIT(0)
> > +
> > +#define XCSI_INTR_PROT_MASK (XCSI_ISR_VC3FSYNCERR |
> XCSI_ISR_VC3FLVLERR |\
> > + XCSI_ISR_VC2FSYNCERR |
> XCSI_ISR_VC2FLVLERR |\
> > + XCSI_ISR_VC1FSYNCERR |
> XCSI_ISR_VC1FLVLERR |\
> > + XCSI_ISR_VC0FSYNCERR |
> XCSI_ISR_VC0FLVLERR |\
> > + XCSI_ISR_VCXFE)
> > +
> > +#define XCSI_INTR_PKTLVL_MASK (XCSI_ISR_ECC2BERR |
> XCSI_ISR_ECC1BERR |\
> > + XCSI_ISR_CRCERR | XCSI_ISR_DATAIDERR)
> > +
> > +#define XCSI_INTR_DPHY_MASK (XCSI_ISR_SOTERR |
> XCSI_ISR_SOTSYNCERR)
> > +
> > +#define XCSI_INTR_SPKT_MASK (XCSI_ISR_SPFIFOF |
> XCSI_ISR_SPFIFONE)
> > +
> > +#define XCSI_INTR_ERR_MASK (XCSI_ISR_WCC | XCSI_ISR_ILC |
> XCSI_ISR_SLBF |\
> > + XCSI_ISR_STOP)
> > +
> > +#define XCSI_INTR_FRAMERCVD_MASK (XCSI_ISR_FR)
>
> These macros are only used in XCSI_ISR_ALLINTR_MASK, do we need them ?
>

I kept them so that it is in sync with what we had for bare metal driver.
I will remove them from the code in next version.

> > +
> > +#define XCSI_ISR_ALLINTR_MASK (XCSI_INTR_PROT_MASK |
> XCSI_INTR_PKTLVL_MASK |\
> > + XCSI_INTR_DPHY_MASK |
> XCSI_INTR_SPKT_MASK |\
> > + XCSI_INTR_ERR_MASK |
> XCSI_INTR_FRAMERCVD_MASK)
> > +
> > +/*
> > + * Removed VCXFE mask as it doesn't exist in IER
> > + * Removed STOP state irq as this will keep driver in irq handler only
> > + */
> > +#define XCSI_IER_INTR_MASK (XCSI_ISR_ALLINTR_MASK &\
> > + ~(XCSI_ISR_STOP | XCSI_ISR_VCXFE))
> > +
> > +#define XCSI_SPKTR_OFFSET 0x30
> > +#define XCSI_SPKTR_DATA GENMASK(23, 8)
> > +#define XCSI_SPKTR_VC GENMASK(7, 6)
> > +#define XCSI_SPKTR_DT GENMASK(5, 0)
> > +
> > +#define XCSI_VCXR_OFFSET 0x34
> > +#define XCSI_VCXR_VCERR GENMASK(23, 0)
> > +#define XCSI_VCXR_VCSTART 4
> > +#define XCSI_VCXR_VCEND 15
>
> Those two macros are not used, maybe we could drop them ?
>

I will remove these unused macros in next version.

> > +#define XCSI_VCXR_FSYNCERR BIT(1)
> > +#define XCSI_VCXR_FLVLERR BIT(0)
> > +
> > +#define XCSI_CLKINFR_OFFSET 0x3C
> > +#define XCSI_CLKINFR_STOP BIT(1)
> > +
> > +#define XCSI_DLXINFR_OFFSET 0x40
> > +#define XCSI_DLXINFR_STOP BIT(5)
> > +#define XCSI_DLXINFR_SOTERR BIT(1)
> > +#define XCSI_DLXINFR_SOTSYNCERR BIT(0)
> > +#define XCSI_MAXDL_COUNT 0x4
> > +
> > +#define XCSI_VCXINF1R_OFFSET 0x60
> > +#define XCSI_VCXINF1R_LINECOUNT GENMASK(31, 16)
> > +#define XCSI_VCXINF1R_LINECOUNT_SHIFT 16
> > +#define XCSI_VCXINF1R_BYTECOUNT GENMASK(15, 0)
> > +
> > +#define XCSI_VCXINF2R_OFFSET 0x64
> > +#define XCSI_VCXINF2R_DT GENMASK(5, 0)
> > +#define XCSI_MAXVCX_COUNT 16
> > +
> > +/*
> > + * The core takes less than 100 video clock cycles to reset.
> > + * So choosing a timeout value larger than this.
> > + */
> > +#define XCSI_TIMEOUT_VAL 1000 /* us */
> > +
> > +/*
> > + * Sink pad connected to sensor source pad.
> > + * Source pad connected to next module like demosaic.
> > + */
> > +#define XCSI_MEDIA_PADS 2
> > +#define XCSI_DEFAULT_WIDTH 1920
> > +#define XCSI_DEFAULT_HEIGHT 1080
> > +
> > +/* Max media bus formats supported for enumeration */
> > +#define XCSI_MAX_MBUS_FMTS 16
>
> Isn't 8 enough ?
>

Yes. 8 are enough. I will update this in next version.

> > +
> > +/* Max string length for CSI Data type string */
> > +#define XCSI_PXLFMT_STRLEN_MAX 16
>
> This isn't used either.
>

I will remove this.

> > +
> > +/* MIPI CSI2 Data Types from spec */
> > +#define XCSI_DT_YUV4228B 0x1E
> > +#define XCSI_DT_YUV42210B 0x1F
> > +#define XCSI_DT_RGB444 0x20
> > +#define XCSI_DT_RGB555 0x21
> > +#define XCSI_DT_RGB565 0x22
> > +#define XCSI_DT_RGB666 0x23
> > +#define XCSI_DT_RGB888 0x24
> > +#define XCSI_DT_RAW6 0x28
> > +#define XCSI_DT_RAW7 0x29
> > +#define XCSI_DT_RAW8 0x2A
> > +#define XCSI_DT_RAW10 0x2B
> > +#define XCSI_DT_RAW12 0x2C
> > +#define XCSI_DT_RAW14 0x2D
> > +#define XCSI_DT_RAW16 0x2E
> > +#define XCSI_DT_RAW20 0x2F
>
> Could you please use lower-case for hex constants ?
>

Ok. I will do so in next version.

> > +
> > +#define XCSI_VCX_START 4
> > +#define XCSI_MAX_VC 4
> > +#define XCSI_MAX_VCX 16
> > +
> > +#define XCSI_NEXTREG_OFFSET 4
> > +
> > +/* There are 2 events frame sync and frame level error per VC */
> > +#define XCSI_VCX_NUM_EVENTS ((XCSI_MAX_VCX - XCSI_MAX_VC) *
> 2)
> > +
> > +/* Macro to return "true" or "false" string if bit is set */
> > +#define XCSI_GET_BITSET_STR(val, mask) (val) & (mask) ? "true" :
> "false"
>
> I would inline this in xcsi2rxss_log_status() where the macro is used.
>

I didn't get this exactly. Do you want this to be a static inline function
static inline const char *xcsi_get_bitset_str(u32 val, u32 mask) {
return val & mask ? "true" : "false";
}

OR

dev_info(core->dev, "Short Packet FIFO Full = %s\n",
XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFOFULL));

should be replaced with

dev_info(core->dev, "Short Packet FIFO Full = %s\n",
data & XCSI_CSR_SPFIFOFULL: "true", "false");
?

> > +
> > +#define XADD_MBUS(state, mbus_fmt)
> \
> > + do { \
> > + if ((state)->mbus_fmts_count < XCSI_MAX_MBUS_FMTS) {
> \
> > + (state)->mbus_fmts[(state)->mbus_fmts_count++] =\
> > + (mbus_fmt); \
> > + } else { \
> > + dev_err((state)->core.dev, \
> > + "accessing array out of bounds!\n"); \
> > + } \
> > + } while (0)
> > +
> > +/**
> > + * struct xcsi2rxss_event - Event log structure
> > + * @mask: Event mask
> > + * @name: Name of the event
> > + */
> > +struct xcsi2rxss_event {
> > + u32 mask;
> > + const char *name;
> > +};
> > +
> > +static const struct xcsi2rxss_event xcsi2rxss_events[] = {
> > + { XCSI_ISR_FR, "Frame Received" },
> > + { XCSI_ISR_VCXFE, "VCX Frame Errors" },
> > + { XCSI_ISR_WCC, "Word Count Errors" },
> > + { XCSI_ISR_ILC, "Invalid Lane Count Error" },
> > + { XCSI_ISR_SPFIFOF, "Short Packet FIFO OverFlow Error" },
> > + { XCSI_ISR_SPFIFONE, "Short Packet FIFO Not Empty" },
> > + { XCSI_ISR_SLBF, "Streamline Buffer Full Error" },
> > + { XCSI_ISR_STOP, "Lane Stop State" },
> > + { XCSI_ISR_SOTERR, "SOT Error" },
> > + { XCSI_ISR_SOTSYNCERR, "SOT Sync Error" },
> > + { XCSI_ISR_ECC2BERR, "2 Bit ECC Unrecoverable Error" },
> > + { XCSI_ISR_ECC1BERR, "1 Bit ECC Recoverable Error" },
> > + { XCSI_ISR_CRCERR, "CRC Error" },
> > + { XCSI_ISR_DATAIDERR, "Data Id Error" },
> > + { XCSI_ISR_VC3FSYNCERR, "Virtual Channel 3 Frame Sync Error" },
> > + { XCSI_ISR_VC3FLVLERR, "Virtual Channel 3 Frame Level Error" },
> > + { XCSI_ISR_VC2FSYNCERR, "Virtual Channel 2 Frame Sync Error" },
> > + { XCSI_ISR_VC2FLVLERR, "Virtual Channel 2 Frame Level Error" },
> > + { XCSI_ISR_VC1FSYNCERR, "Virtual Channel 1 Frame Sync Error" },
> > + { XCSI_ISR_VC1FLVLERR, "Virtual Channel 1 Frame Level Error" },
> > + { XCSI_ISR_VC0FSYNCERR, "Virtual Channel 0 Frame Sync Error" },
> > + { XCSI_ISR_VC0FLVLERR, "Virtual Channel 0 Frame Level Error" }
> > +};
> > +
> > +#define XCSI_NUM_EVENTS ARRAY_SIZE(xcsi2rxss_events)
> > +
> > +/*
> > + * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device
> structure
> > + * @dev: Platform structure
> > + * @iomem: Base address of subsystem
> > + * @enable_active_lanes: If number of active lanes can be modified
> > + * @max_num_lanes: Maximum number of lanes present
> > + * @datatype: Data type filter
> > + * @events: counter for events
> > + * @vcx_events: counter for vcx_events
> > + * @en_vcx: If more than 4 VC are enabled
> > + * @clks: array of clocks
> > + * @rst_gpio: reset to video_aresetn
> > + */
> > +struct xcsi2rxss_core {
> > + struct device *dev;
> > + void __iomem *iomem;
> > + bool enable_active_lanes;
> > + u32 max_num_lanes;
> > + u32 datatype;
> > + u32 events[XCSI_NUM_EVENTS];
> > + u32 vcx_events[XCSI_VCX_NUM_EVENTS];
> > + bool en_vcx;
> > + struct clk_bulk_data *clks;
> > + struct gpio_desc *rst_gpio;
> > +};
> > +
> > +/**
> > + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
> > + * @core: Core structure for MIPI CSI2 Rx Subsystem
> > + * @subdev: The v4l2 subdev structure
> > + * @format: Active V4L2 formats on each pad
> > + * @default_format: Default V4L2 format
> > + * @lock: mutex for accessing this structure
> > + * @pads: media pads
> > + * @mbus_fmts: List of media bus formats for enum_mbus_code
> > + * @mbus_fmts_count: Number of media bus formats
> > + * @streaming: Flag for storing streaming state
> > + *
> > + * This structure contains the device driver related parameters
> > + */
> > +struct xcsi2rxss_state {
> > + struct xcsi2rxss_core core;
> > + struct v4l2_subdev subdev;
> > + struct v4l2_mbus_framefmt format;
> > + struct v4l2_mbus_framefmt default_format;
> > + /* used to protect access to this struct */
> > + struct mutex lock;
> > + struct media_pad pads[XCSI_MEDIA_PADS];
> > + u32 mbus_fmts[XCSI_MAX_MBUS_FMTS];
> > + u32 mbus_fmts_count;
> > + bool streaming;
> > +};
>
> Is there a need to have two separate data structures, can't they be
> merged ?
>

They can be merged. The reason to keep them separate was to keep the IP core related different from driver state variables.

> > +
> > +static const struct clk_bulk_data xcsi2rxss_clks[] = {
> > + { .id = "lite_aclk" },
> > + { .id = "video_aclk" },
> > +};
> > +
> > +static inline struct xcsi2rxss_state *
> > +to_xcsi2rxssstate(struct v4l2_subdev *subdev)
> > +{
> > + return container_of(subdev, struct xcsi2rxss_state, subdev);
> > +}
> > +
> > +/*
> > + * Register related operations
> > + */
> > +static inline u32 xcsi2rxss_read(struct xcsi2rxss_core *xcsi2rxss, u32 addr)
> > +{
> > + return ioread32(xcsi2rxss->iomem + addr);
> > +}
> > +
> > +static inline void xcsi2rxss_write(struct xcsi2rxss_core *xcsi2rxss, u32
> addr,
> > + u32 value)
> > +{
> > + iowrite32(value, xcsi2rxss->iomem + addr);
> > +}
> > +
> > +static inline void xcsi2rxss_clr(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> > + u32 clr)
> > +{
> > + xcsi2rxss_write(xcsi2rxss, addr,
> > + xcsi2rxss_read(xcsi2rxss, addr) & ~clr);
> > +}
> > +
> > +static inline void xcsi2rxss_set(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> > + u32 set)
> > +{
> > + xcsi2rxss_write(xcsi2rxss, addr, xcsi2rxss_read(xcsi2rxss, addr) | set);
> > +}
> > +
> > +static void xcsi2rxss_enable(struct xcsi2rxss_core *core)
> > +{
> > + xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > +}
> > +
> > +static void xcsi2rxss_disable(struct xcsi2rxss_core *core)
> > +{
> > + xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > +}
> > +
> > +static void xcsi2rxss_intr_enable(struct xcsi2rxss_core *core)
> > +{
> > + xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > + xcsi2rxss_write(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> > + xcsi2rxss_set(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > +}
> > +
> > +static void xcsi2rxss_intr_disable(struct xcsi2rxss_core *core)
> > +{
> > + xcsi2rxss_clr(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> > + xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > +}
>
> I'd find the code more readable if we inlined those four functions in
> their single call site, with a one-line comment explaining what happens
> (e.g. /* Enable interrupts */), but that may be a matter of personal
> taste.
>

If that is preferable I will do it that way in next version.

> > +
> > +/**
> > + * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
> > + * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
> > + *
> > + * Core takes less than 100 video clock cycles to reset.
> > + * So a larger timeout value is chosen for margin.
> > + *
> > + * Return: 0 - on success OR -ETIME if reset times out
> > + */
> > +static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
> > +{
> > + u32 timeout = XCSI_TIMEOUT_VAL;
> > +
> > + xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> > +
> > + while (xcsi2rxss_read(core, XCSI_CSR_OFFSET) & XCSI_CSR_RIPCD) {
> > + if (timeout == 0) {
> > + dev_err(core->dev, "soft reset timed out!\n");
> > + return -ETIME;
> > + }
> > +
> > + timeout--;
> > + udelay(1);
>
> I understand the reset time is guaranteed to be lower than 100 video
> clock cycles, but what is the typical time ? It's a bit pointless to
> busy loop with a very short delay if a larger delay is typically needed.
>

With say a video clock of 148.5 MHz we need at max 674 ns.
We are actually overdoing the delay a bit here.
But I think it shouldn't matter as long as we are doing this before starting streaming.
Let me know otherwise and I can make it ndelay(100).

> > + }
> > +
> > + xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> > + return 0;
> > +}
> > +
> > +static void xcsi2rxss_reset_event_counters(struct xcsi2rxss_state *state)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < XCSI_NUM_EVENTS; i++)
> > + state->core.events[i] = 0;
> > +
> > + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++)
> > + state->core.vcx_events[i] = 0;
> > +}
> > +
> > +/* Print event counters */
> > +static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > + unsigned int i;
> > +
> > + for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > + if (core->events[i] > 0) {
> > + dev_info(core->dev, "%s events: %d\n",
> > + xcsi2rxss_events[i].name,
> > + core->events[i]);
> > + }
> > + }
> > +
> > + if (core->en_vcx) {
> > + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > + if (core->vcx_events[i] > 0) {
> > + dev_info(core->dev,
> > + "VC %d Frame %s err vcx events:
> %d\n",
> > + (i / 2) + XCSI_VCX_START,
> > + i & 1 ? "Sync" : "Level",
> > + core->vcx_events[i]);
> > + }
> > + }
> > + }
> > +}
> > +
> > +static void xcsi2rxss_log_ipconfig(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > +
> > + dev_dbg(core->dev, "****** Xilinx MIPI CSI2 Rx SS IP Config
> ******\n");
> > + dev_dbg(core->dev, "vcx is %s", core->en_vcx ? "enabled" :
> "disabled");
> > + dev_dbg(core->dev, "Enable active lanes property is %s\n",
> > + core->enable_active_lanes ? "present" : "absent");
> > + dev_dbg(core->dev, "Max lanes = %d", core->max_num_lanes);
> > + dev_dbg(core->dev, "Pixel format set as 0x%x\n", core->datatype);
> > + dev_dbg(core->dev,
> "**********************************************\n");
>
> How about making this more compact ?
>
> dev_dbg(core->dev, "vcx %s, %u data lanes (%s), data type
> 0x%02x\n",
> core->en_vcx ? "enabled" : "disabled",
> core->max_num_lanes,
> core->enable_active_lanes ? "dynamic" : "static",
> core->datatype);
>

This is better. I will incorporate it in my next version.

> > +}
> > +
> > +/**
> > + * xcsi2rxss_log_status - Logs the status of the CSI-2 Receiver
> > + * @sd: Pointer to V4L2 subdevice structure
> > + *
> > + * This function prints the current status of Xilinx MIPI CSI-2
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > + u32 reg, data;
> > + unsigned int i, max_vc;
> > +
> > + mutex_lock(&xcsi2rxss->lock);
> > +
> > + xcsi2rxss_log_ipconfig(xcsi2rxss);
>
> Do we need to print this here, as it's printed at probe time already and
> only contains static data ? I would inline xcsi2rxss_log_ipconfig() in
> xcsi2rxss_parse_of().
>

Ok. I will remove log_ipconfig() from here.

> > +
> > + xcsi2rxss_log_counters(xcsi2rxss);
> > +
> > + dev_info(core->dev, "***** Core Status *****\n");
> > + data = xcsi2rxss_read(core, XCSI_CSR_OFFSET);
> > + dev_info(core->dev, "Short Packet FIFO Full = %s\n",
> > + XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFOFULL));
> > + dev_info(core->dev, "Short Packet FIFO Not Empty = %s\n",
> > + XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFONE));
> > + dev_info(core->dev, "Stream line buffer full = %s\n",
> > + XCSI_GET_BITSET_STR(data, XCSI_CSR_SLBF));
> > + dev_info(core->dev, "Soft reset/Core disable in progress = %s\n",
> > + XCSI_GET_BITSET_STR(data, XCSI_CSR_RIPCD));
> > +
> > + /* Clk & Lane Info */
> > + dev_info(core->dev, "******** Clock Lane Info *********\n");
> > + data = xcsi2rxss_read(core, XCSI_CLKINFR_OFFSET);
> > + dev_info(core->dev, "Clock Lane in Stop State = %s\n",
> > + XCSI_GET_BITSET_STR(data, XCSI_CLKINFR_STOP));
> > +
> > + dev_info(core->dev, "******** Data Lane Info *********\n");
> > + dev_info(core->dev, "Lane\tSoT Error\tSoT Sync Error\tStop
> State\n");
> > + reg = XCSI_DLXINFR_OFFSET;
> > + for (i = 0; i < XCSI_MAXDL_COUNT; i++) {
> > + data = xcsi2rxss_read(core, reg);
> > +
> > + dev_info(core->dev, "%d\t%s\t\t%s\t\t%s\n", i,
> > + XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTERR),
> > + XCSI_GET_BITSET_STR(data,
> XCSI_DLXINFR_SOTSYNCERR),
> > + XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_STOP));
> > +
> > + reg += XCSI_NEXTREG_OFFSET;
> > + }
> > +
> > + /* Virtual Channel Image Information */
> > + dev_info(core->dev, "********** Virtual Channel Info
> ************\n");
> > + dev_info(core->dev, "VC\tLine Count\tByte Count\tData Type\n");
> > + if (core->en_vcx)
> > + max_vc = XCSI_MAX_VCX;
> > + else
> > + max_vc = XCSI_MAX_VC;
> > +
> > + reg = XCSI_VCXINF1R_OFFSET;
> > + for (i = 0; i < max_vc; i++) {
> > + u32 line_count, byte_count, data_type;
> > +
> > + /* Get line and byte count from VCXINFR1 Register */
> > + data = xcsi2rxss_read(core, reg);
> > + byte_count = data & XCSI_VCXINF1R_BYTECOUNT;
> > + line_count = data & XCSI_VCXINF1R_LINECOUNT;
> > + line_count >>= XCSI_VCXINF1R_LINECOUNT_SHIFT;
> > +
> > + /* Get data type from VCXINFR2 Register */
> > + reg += XCSI_NEXTREG_OFFSET;
> > + data = xcsi2rxss_read(core, reg);
> > + data_type = data & XCSI_VCXINF2R_DT;
> > +
> > + dev_info(core->dev, "%d\t%d\t\t%d\t\t0x%x\n", i,
> line_count,
> > + byte_count, data_type);
> > +
> > + /* Move to next pair of VC Info registers */
> > + reg += XCSI_NEXTREG_OFFSET;
> > + }
> > +
> > + mutex_unlock(&xcsi2rxss->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static struct v4l2_subdev *xcsi2rxss_get_remote_subdev(struct
> media_pad *local)
> > +{
> > + struct media_pad *remote;
> > +
> > + remote = media_entity_remote_pad(local);
> > + if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> > + return NULL;
> > +
> > + return media_entity_to_v4l2_subdev(remote->entity);
> > +}
> > +
> > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > + struct v4l2_subdev *rsubdev;
> > + int ret = 0;
> > +
> > + xcsi2rxss_enable(core);
> > +
> > + ret = xcsi2rxss_reset(core);
> > + if (ret < 0) {
> > + state->streaming = false;
> > + return ret;
> > + }
> > +
> > + xcsi2rxss_intr_enable(core);
> > + state->streaming = true;
> > +
> > + rsubdev = xcsi2rxss_get_remote_subdev(&state-
> >pads[XVIP_PAD_SINK]);
>
> How about storing the remote subdev pointer in the xcsi2rxss_state
> structure, either at probe time, or at streamon time ?
>

As mentioned in my response to Sakari or v10, the sink to source calling of streamon()
already happens in xvip_pipeline_start_stop().
I can add the remote subdev in state structure which can be parsed and stored during streamon() and directly used in streamoff().

> > + ret = v4l2_subdev_call(rsubdev, video, s_stream, 1);
>
> If this failes, you should disabled interrupts, disable the receiver,
> and set state->streaming to false. I would actually set state->streaming
> = enable at the end of xcsi2rxss_s_stream() instead of here and in
> xcsi2rxss_stop_stream().
>

Ok. I will fix this in next version.

> > +
> > + return ret;
> > +}
> > +
> > +static void xcsi2rxss_stop_stream(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > + struct v4l2_subdev *rsubdev;
> > +
> > + rsubdev = xcsi2rxss_get_remote_subdev(&state-
> >pads[XVIP_PAD_SINK]);
> > + v4l2_subdev_call(rsubdev, video, s_stream, 0);
> > +
> > + xcsi2rxss_intr_disable(core);
> > + xcsi2rxss_disable(core);
> > + state->streaming = false;
> > +}
> > +
> > +/**
> > + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
> > + * @irq: IRQ number
> > + * @dev_id: Pointer to device state
> > + *
> > + * In the interrupt handler, a list of event counters are updated for
> > + * corresponding interrupts. This is useful to get status / debug.
> > + *
> > + * Return: IRQ_HANDLED after handling interrupts
> > + * IRQ_NONE is no interrupts
> > + */
> > +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> > +{
> > + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> > + struct xcsi2rxss_core *core = &state->core;
> > + u32 status;
> > +
> > + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) &
> XCSI_ISR_ALLINTR_MASK;
> > + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n",
> status);
>
> As this is expected to occur for every frame, I would drop the message,
> even if rate-limited.
>

Ok I will remove this.

> > +
> > + if (!status)
> > + return IRQ_NONE;
> > +
> > + /* Received a short packet */
> > + if (status & XCSI_ISR_SPFIFONE) {
> > + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> > + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> > + }
>
> Same here, this will occur all the time, I'd remove this message. You
> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
> multiple short packets are received before the interrupt handler
> executes.
>
> I also wonder if it would make sense to extract the frame number from
> the FS short packet, and make it available through the subdev API. I
> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
> can be implemented later.
>

This will not occur all the time as these are generic short packets and not the regular short packets like that for Frame Start, Frame end, etc.
I was earlier sending across the generic packet data to the userspace app via v4l2 event.
But this idea was not accepted. Hence I am printing the generic short packet data here for debug purpose only.

> > +
> > + /* Short packet FIFO overflow */
> > + if (status & XCSI_ISR_SPFIFOF)
> > + dev_dbg_ratelimited(core->dev, "Short packet FIFO
> overflowed\n");
> > +
> > + /*
> > + * Stream line buffer full
> > + * This means there is a backpressure from downstream IP
> > + */
> > + if (status & XCSI_ISR_SLBF) {
> > + dev_alert_ratelimited(core->dev, "Stream Line Buffer
> Full!\n");
> > + xcsi2rxss_stop_stream(state);
> > + if (core->rst_gpio) {
> > + gpiod_set_value(core->rst_gpio, 1);
> > + /* minimum 40 dphy_clk_200M cycles */
> > + ndelay(250);
> > + gpiod_set_value(core->rst_gpio, 0);
> > + }
>
> I don't think you should stop the core here. xcsi2rxss_stop_stream()
> calls the source .s_stream(0) operation, which usually involves I2C
> writes that will sleep.
>
> You should instead report an event to userspace (it looks like we have
> no error event defined in V4L2, one should be added), and rely on the
> normal stop procedure.
>

I agree about the I2C transactions part. I will add normal stop streaming part here w/o calling the remote subdev stop first.

I had proposed V4L2_EVENT_XLNXCSIRX_SLBF event for this in earlier patches.

> > + }
> > +
> > + /* Increment event counters */
> > + if (status & XCSI_ISR_ALLINTR_MASK) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > + if (!(status & xcsi2rxss_events[i].mask))
> > + continue;
> > + core->events[i]++;
> > + dev_dbg_ratelimited(core->dev, "%s: %d\n",
>
> The counter is unsigned, this should be %u instead of %d.

Agreed. I will fix this in next version.

>
> > + xcsi2rxss_events[i].name,
> > + core->events[i]);
> > + }
> > +
> > + if (status & XCSI_ISR_VCXFE && core->en_vcx) {
> > + u32 vcxstatus;
> > +
> > + vcxstatus = xcsi2rxss_read(core, XCSI_VCXR_OFFSET);
> > + vcxstatus &= XCSI_VCXR_VCERR;
> > + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > + if (!(vcxstatus & (1 << i)))
> > + continue;
> > + core->vcx_events[i]++;
> > + }
> > + xcsi2rxss_write(core, XCSI_VCXR_OFFSET, vcxstatus);
> > + }
> > + }
> > +
> > + xcsi2rxss_write(core, XCSI_ISR_OFFSET, status);
>
> To lower the probability of losing events, shouldn't you write the
> register right after reading it above ? Same for XCSI_VCXR_OFFSET.
>

Right I will move this write to just after read.

> > + return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * xcsi2rxss_s_stream - It is used to start/stop the streaming.
> > + * @sd: V4L2 Sub device
> > + * @enable: Flag (True / False)
> > + *
> > + * This function controls the start or stop of streaming for the
> > + * Xilinx MIPI CSI-2 Rx Subsystem.
> > + *
> > + * Return: 0 on success, errors otherwise
> > + */
> > +static int xcsi2rxss_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > + int ret = 0;
> > +
> > + mutex_lock(&xcsi2rxss->lock);
> > +
>
> You could add
>
> if (enable == xcsi2rxss->streaming)
> goto done;
>
> with a done label just before mutex_unlock(), and simplify the code
> below by removing the xcsi2rxss->streaming checks.
>

Ok I will make this change in next version.

> > + if (enable) {
> > + if (!xcsi2rxss->streaming) {
> > + /* reset the event counters */
> > + xcsi2rxss_reset_event_counters(xcsi2rxss);
> > + ret = xcsi2rxss_start_stream(xcsi2rxss);
> > + }
> > + } else {
> > + if (xcsi2rxss->streaming) {
> > + struct gpio_desc *rst = xcsi2rxss->core.rst_gpio;
> > +
> > + xcsi2rxss_stop_stream(xcsi2rxss);
> > + if (rst) {
> > + gpiod_set_value_cansleep(rst, 1);
> > + usleep_range(1, 2);
> > + gpiod_set_value_cansleep(rst, 0);
> > + }
> > + }
> > + }
> > +
> > + mutex_unlock(&xcsi2rxss->lock);
> > + return ret;
> > +}
> > +
> > +static struct v4l2_mbus_framefmt *
> > +__xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
> > + struct v4l2_subdev_pad_config *cfg,
> > + unsigned int pad, u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_format(&xcsi2rxss->subdev, cfg,
> pad);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &xcsi2rxss->format;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +/**
> > + * xcsi2rxss_get_format - Get the pad format
> > + * @sd: Pointer to V4L2 Sub device structure
> > + * @cfg: Pointer to sub device pad information structure
> > + * @fmt: Pointer to pad level media bus format
> > + *
> > + * This function is used to get the pad format information.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > +
> > + mutex_lock(&xcsi2rxss->lock);
> > + fmt->format = *__xcsi2rxss_get_pad_format(xcsi2rxss, cfg, fmt->pad,
> > + fmt->which);
> > + mutex_unlock(&xcsi2rxss->lock);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcsi2rxss_set_format - This is used to set the pad format
> > + * @sd: Pointer to V4L2 Sub device structure
> > + * @cfg: Pointer to sub device pad information structure
> > + * @fmt: Pointer to pad level media bus format
> > + *
> > + * This function is used to set the pad format. Since the pad format is fixed
> > + * in hardware, it can't be modified on run time. So when a format set is
> > + * requested by application, all parameters except the format type is
> saved
> > + * for the pad and the original pad format is sent back to the application.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > + struct v4l2_mbus_framefmt *__format;
> > +
> > + /* only sink pad format can be updated */
> > + mutex_lock(&xcsi2rxss->lock);
> > +
> > + /*
> > + * Only the format->code parameter matters for CSI as the
> > + * CSI format cannot be changed at runtime.
> > + * Ensure that format to set is copied to over to CSI pad format
> > + */
> > + __format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
> > + fmt->pad, fmt->which);
> > +
> > + if (fmt->pad == XVIP_PAD_SOURCE) {
> > + fmt->format = *__format;
> > + mutex_unlock(&xcsi2rxss->lock);
> > + return 0;
> > + }
> > +
> > + /*
> > + * RAW8 is supported in all datatypes. So if requested media bus
> format
> > + * is of RAW8 type, then allow to be set. In case core is configured to
> > + * other RAW, YUV422 8/10 or RGB888, set appropriate media bus
> format.
> > + */
> > + if (!((fmt->format.code == MEDIA_BUS_FMT_SBGGR8_1X8 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGBRG8_1X8 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGRBG8_1X8 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SRGGB8_1X8) ||
> > + (core->datatype == XCSI_DT_RAW10 &&
> > + (fmt->format.code == MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SRGGB10_1X10)) ||
> > + (core->datatype == XCSI_DT_RAW12 &&
> > + (fmt->format.code == MEDIA_BUS_FMT_SBGGR12_1X12 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGBRG12_1X12 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGRBG12_1X12 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SRGGB12_1X12)) ||
> > + (core->datatype == XCSI_DT_RAW14 &&
> > + (fmt->format.code == MEDIA_BUS_FMT_SBGGR14_1X14 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGBRG14_1X14 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGRBG14_1X14 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SRGGB14_1X14)) ||
> > + (core->datatype == XCSI_DT_RAW16 &&
> > + (fmt->format.code == MEDIA_BUS_FMT_SBGGR16_1X16 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGBRG16_1X16 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SGRBG16_1X16 ||
> > + fmt->format.code == MEDIA_BUS_FMT_SRGGB16_1X16)) ||
> > + (core->datatype == XCSI_DT_YUV4228B &&
> > + fmt->format.code == MEDIA_BUS_FMT_UYVY8_1X16) ||
> > + (core->datatype == XCSI_DT_YUV42210B &&
> > + fmt->format.code == MEDIA_BUS_FMT_UYVY10_1X20) ||
> > + (core->datatype == XCSI_DT_RGB888 &&
> > + fmt->format.code == MEDIA_BUS_FMT_RBG888_1X24))) {
>
> I think you should create a static const table of format information,
> mapping media bus codes to CSI-2 data types. It can be useful here, in
> xcsi2rxss_set_default_format() and in xcsi2rxss_init_mbus_fmts().
>

Ok. I will create static const table with media bus code and data type mapping
And use it here, set_default_format and init_mbus_fmts()

> > + /* Restore the original pad format code */
> > + dev_dbg(core->dev, "Unsupported media bus format");
> > + fmt->format.code = __format->code;
>
> You should use the default here instead of the current format.
>

Agree.

> > + }
> > +
> > + *__format = fmt->format;
> > + mutex_unlock(&xcsi2rxss->lock);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
> > + * @sd : pointer to v4l2 subdev structure
> > + * @cfg: V4L2 subdev pad configuration
> > + * @code : pointer to v4l2_subdev_mbus_code_enum structure
> > + *
> > + * Return: -EINVAL or zero on success
> > + */
> > +int xcsi2rxss_enum_mbus_code(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + struct xcsi2rxss_state *state = to_xcsi2rxssstate(sd);
> > +
> > + if (code->index >= state->mbus_fmts_count)
> > + return -EINVAL;
> > +
> > + code->code = state->mbus_fmts[code->index];
>
> Instead of storing mbus_fmts and mbus_fmts_count in the state, how about
> moving the logic from xcsi2rxss_init_mbus_fmts() to here ?

I will update this based on the static const mapping table.

>
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcsi2rxss_open - Called on v4l2_open()
> > + * @sd: Pointer to V4L2 sub device structure
> > + * @fh: Pointer to V4L2 File handle
> > + *
> > + * This function is called on v4l2_open(). It sets the default format
> > + * for both pads.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xcsi2rxss_open(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_fh *fh)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > + struct v4l2_mbus_framefmt *format;
> > + unsigned int i;
> > +
> > + for (i = 0; i < XCSI_MEDIA_PADS; i++) {
> > + format = v4l2_subdev_get_try_format(sd, fh->pad, i);
> > + *format = xcsi2rxss->default_format;
> > + }
>
> I would recommend moving this logic to the .init_cfg() pad operation,
> and removing the .open() internal operation.

Ok. I will do that.

>
> > +
> > + return 0;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Media Operations
> > + */
> > +
> > +static const struct media_entity_operations xcsi2rxss_media_ops = {
> > + .link_validate = v4l2_subdev_link_validate
> > +};
> > +
> > +static const struct v4l2_subdev_core_ops xcsi2rxss_core_ops = {
> > + .log_status = xcsi2rxss_log_status,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops xcsi2rxss_video_ops = {
> > + .s_stream = xcsi2rxss_s_stream
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops xcsi2rxss_pad_ops = {
> > + .get_fmt = xcsi2rxss_get_format,
> > + .set_fmt = xcsi2rxss_set_format,
> > + .enum_mbus_code = xcsi2rxss_enum_mbus_code,
> > + .link_validate = v4l2_subdev_link_validate_default,
> > +};
> > +
> > +static const struct v4l2_subdev_ops xcsi2rxss_ops = {
> > + .core = &xcsi2rxss_core_ops,
> > + .video = &xcsi2rxss_video_ops,
> > + .pad = &xcsi2rxss_pad_ops
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops xcsi2rxss_internal_ops = {
> > + .open = xcsi2rxss_open,
> > +};
> > +
> > +static void xcsi2rxss_set_default_format(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > +
> > + memset(&state->default_format, 0, sizeof(state->default_format));
>
> state is kzalloc'ed, so you can skip this.

Right. I will remove this in the next version.

>
> > +
> > + switch (core->datatype) {
> > + case XCSI_DT_YUV4228B:
> > + state->default_format.code =
> MEDIA_BUS_FMT_UYVY8_1X16;
> > + break;
> > + case XCSI_DT_RGB888:
> > + state->default_format.code =
> MEDIA_BUS_FMT_RBG888_1X24;
> > + break;
> > + case XCSI_DT_YUV42210B:
> > + state->default_format.code =
> MEDIA_BUS_FMT_UYVY10_1X20;
> > + break;
> > + case XCSI_DT_RAW10:
> > + state->default_format.code =
> MEDIA_BUS_FMT_SRGGB10_1X10;
> > + break;
> > + case XCSI_DT_RAW12:
> > + state->default_format.code =
> MEDIA_BUS_FMT_SRGGB12_1X12;
> > + break;
> > + case XCSI_DT_RAW14:
> > + state->default_format.code =
> MEDIA_BUS_FMT_SRGGB14_1X14;
> > + break;
> > + case XCSI_DT_RAW16:
> > + state->default_format.code =
> MEDIA_BUS_FMT_SRGGB16_1X16;
> > + break;
> > + case XCSI_DT_RAW8:
> > + case XCSI_DT_RGB444:
> > + case XCSI_DT_RGB555:
> > + case XCSI_DT_RGB565:
> > + case XCSI_DT_RGB666:
> > + state->default_format.code =
> MEDIA_BUS_FMT_SRGGB8_1X8;
>
> This is correct for XCSI_DT_RAW8 but not for the other RGB data types.

For the other RGB data types there is no supporting media bus format.
Hence they are initialized to default format as SRGGB8.

>
> > + break;
> > + }
> > +
> > + state->default_format.field = V4L2_FIELD_NONE;
> > + state->default_format.colorspace = V4L2_COLORSPACE_SRGB;
> > + state->default_format.width = XCSI_DEFAULT_WIDTH;
> > + state->default_format.height = XCSI_DEFAULT_HEIGHT;
> > +
> > + dev_dbg(core->dev, "default mediabus format = 0x%x",
> > + state->default_format.code);
>
> I'd drop this as there's already a debug message that prints the data
> type.
>

Ok.

> > +}
> > +
> > +static void xcsi2rxss_init_mbus_fmts(struct xcsi2rxss_state *state)
> > +{
> > + struct xcsi2rxss_core *core = &state->core;
> > +
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB8_1X8);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR8_1X8);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG8_1X8);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG8_1X8);
> > +
> > + switch (core->datatype) {
> > + case XCSI_DT_RAW10:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB10_1X10);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR10_1X10);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG10_1X10);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG10_1X10);
> > + break;
> > + case XCSI_DT_RAW12:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB12_1X12);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR12_1X12);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG12_1X12);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG12_1X12);
> > + break;
> > + case XCSI_DT_RAW14:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB14_1X14);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR14_1X14);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG14_1X14);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG14_1X14);
> > + break;
> > + case XCSI_DT_RAW16:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SRGGB16_1X16);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SBGGR16_1X16);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGRBG16_1X16);
> > + XADD_MBUS(state, MEDIA_BUS_FMT_SGBRG16_1X16);
> > + break;
> > + case XCSI_DT_YUV4228B:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_UYVY8_1X16);
> > + break;
> > + case XCSI_DT_RGB888:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_RBG888_1X24);
> > + break;
> > + case XCSI_DT_YUV42210B:
> > + XADD_MBUS(state, MEDIA_BUS_FMT_UYVY10_1X20);
> > + break;
> > + default:
> > + dev_err(core->dev, "Invalid data type!\n");
> > + }
> > +}
> > +
> > +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> > +{
> > + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > + struct device_node *node = xcsi2rxss->core.dev->of_node;
> > + unsigned int nports, irq;
> > + bool en_csi_v20, vfb;
> > + int ret;
> > +
> > + en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
> > + if (en_csi_v20)
> > + core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx");
> > +
> > + core->enable_active_lanes =
> > + of_property_read_bool(node, "xlnx,en-active-lanes");
> > +
> > + ret = of_property_read_u32(node, "xlnx,csi-pxl-format",
> > + &core->datatype);
> > + if (ret < 0) {
> > + dev_err(core->dev, "missing xlnx,csi-pxl-format property\n");
> > + return ret;
> > + }
> > +
> > + switch (core->datatype) {
> > + case XCSI_DT_YUV4228B:
> > + case XCSI_DT_RGB444:
> > + case XCSI_DT_RGB555:
> > + case XCSI_DT_RGB565:
> > + case XCSI_DT_RGB666:
> > + case XCSI_DT_RGB888:
> > + case XCSI_DT_RAW6:
> > + case XCSI_DT_RAW7:
> > + case XCSI_DT_RAW8:
> > + case XCSI_DT_RAW10:
> > + case XCSI_DT_RAW12:
> > + case XCSI_DT_RAW14:
> > + break;
> > + case XCSI_DT_YUV42210B:
> > + case XCSI_DT_RAW16:
> > + case XCSI_DT_RAW20:
> > + if (!en_csi_v20) {
> > + ret = -EINVAL;
> > + dev_dbg(core->dev, "enable csi v2 for this pixel
> format");
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > + if (ret < 0) {
> > + dev_err(core->dev, "invalid csi-pxl-format property!\n");
> > + return ret;
> > + }
> > +
> > + vfb = of_property_read_bool(node, "xlnx,vfb");
> > + if (!vfb) {
> > + dev_err(core->dev, "failed as VFB is disabled!\n");
>
> I'd write this "operation without VFB is not supported\n". Do you plan
> to add support for this later ?
>

Ok I will update the message accordingly.
Without VFB the data on the bus won't match media bus format description.
Refer to the PG232 for details on how data is sent on the bus when VFB is disabled.

> > + return -EINVAL;
> > + }
> > +
> > + for (nports = 0; nports < XCSI_MEDIA_PADS; nports++) {
> > + struct fwnode_handle *ep;
> > + struct v4l2_fwnode_endpoint vep = {
> > + .bus_type = V4L2_MBUS_CSI2_DPHY
> > + };
> > +
> > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(core-
> >dev),
> > + nports, 0,
> > +
> FWNODE_GRAPH_ENDPOINT_NEXT);
> > + if (!ep)
> > + break;
> > + /*
> > + * since first port is sink port and it contains
> > + * all info about data-lanes and cfa-pattern,
> > + * don't parse second port but only check if exists
> > + */
> > + if (nports == XVIP_PAD_SOURCE) {
> > + dev_dbg(core->dev, "no need to parse source port");
> > + fwnode_handle_put(ep);
> > + continue;
> > + }
>
> How about removing the loop and only handling port 0 then ?

I will remove the loop.

>
> > +
> > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>
> You can add
>
> fwnode_handle_put(ep);
>
> here,
>
> > + if (ret) {
> > + dev_err(core->dev, "error parsing sink port");
> > + fwnode_handle_put(ep);
>
> and drop it from there and from the end of the loop.
>

Ok got it.

> > + return ret;
> > + }
> > +
> > + dev_dbg(core->dev, "port %d bus type = %d\n", nports,
> > + vep.bus_type);
>
> As bus_type is hardcoded to V4L2_MBUS_CSI2_DPHY and nports can always
> be
> 0 here, I think you can drop this message.
>

Agreed. I will remove it in next version.

> > +
> > + if (vep.bus_type == V4L2_MBUS_CSI2_DPHY) {
>
> Can bus_type be different than V4L2_MBUS_CSI2_DPHY here, as you set it
> when initializing vep above ? I think you can remove the condition
> check, as well as the first debug message below.
>

Ok I will check and remove.

> > + dev_dbg(core->dev, "base.port = %d base.id = %d\n",
> > + vep.base.port, vep.base.id);
> > +
> > + dev_dbg(core->dev, "mipi number lanes = %d\n",
> > + vep.bus.mipi_csi2.num_data_lanes);
> > +
> > + core->max_num_lanes =
> > + vep.bus.mipi_csi2.num_data_lanes;
> > + }
> > + fwnode_handle_put(ep);
> > + }
> > +
> > + if (nports != XCSI_MEDIA_PADS) {
> > + dev_err(core->dev, "invalid number of ports %u\n", nports);
> > + return -EINVAL;
> > + }
>
> I think we can drop this check too.

Ok I will drop this.

>
> > +
> > + /* Register interrupt handler */
> > + irq = irq_of_parse_and_map(node, 0);
>
> This should be turned into platform_get_irq().

Agreed. I will update in the next version.

>
> > + ret = devm_request_irq(core->dev, irq, xcsi2rxss_irq_handler,
> > + IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss);
> > + if (ret) {
> > + dev_err(core->dev, "Err = %d Interrupt handler reg
> failed!\n",
> > + ret);
> > + return ret;
> > + }
>
> And IRQ handling should be moved to xcsi2rxss_probe() as it's not
> related to OF parsing.
>

Ditto.

> > +
> > + xcsi2rxss_log_ipconfig(xcsi2rxss);
> > +
> > + return 0;
> > +}
> > +
> > +static int xcsi2rxss_probe(struct platform_device *pdev)
> > +{
> > + struct v4l2_subdev *subdev;
> > + struct xcsi2rxss_state *xcsi2rxss;
> > + struct xcsi2rxss_core *core;
> > + struct resource *res;
> > + int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
> > + int ret;
> > +
> > + xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss),
> GFP_KERNEL);
> > + if (!xcsi2rxss)
> > + return -ENOMEM;
> > +
> > + core = &xcsi2rxss->core;
> > + core->dev = &pdev->dev;
> > +
> > + core->clks = devm_kmemdup(core->dev, xcsi2rxss_clks,
> > + sizeof(xcsi2rxss_clks), GFP_KERNEL);
> > + if (!core->clks)
> > + return -ENOMEM;
> > +
> > + /* Reset GPIO */
> > + core->rst_gpio = devm_gpiod_get_optional(core->dev, "reset",
> > + GPIOD_OUT_HIGH);
> > + if (IS_ERR(core->rst_gpio)) {
> > + if (PTR_ERR(core->rst_gpio) != -EPROBE_DEFER)
> > + dev_err(core->dev, "Video Reset GPIO not setup in
> DT");
> > + return PTR_ERR(core->rst_gpio);
> > + }
> > +
> > + mutex_init(&xcsi2rxss->lock);
> > +
> > + ret = xcsi2rxss_parse_of(xcsi2rxss);
> > + if (ret < 0)
> > + return ret;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + core->iomem = devm_ioremap_resource(core->dev, res);
>
> You can replace those two lines with
>
> core->iomem = devm_platform_ioremap_resource(pdev, 0);
>

I didn't know this existed. Thanks I will update this.

> > + if (IS_ERR(core->iomem))
> > + return PTR_ERR(core->iomem);
> > +
> > + ret = clk_bulk_get(core->dev, num_clks, core->clks);
> > + if (ret)
> > + return ret;
> > +
> > + ret = clk_bulk_prepare_enable(num_clks, core->clks);
> > + if (ret)
> > + goto err_clk_put;
>
> Shouldn't be clock enabled when starting the stream, and disabled when
> stopping it ?
>

Agree but I would like to do it this way for now. We can revisit the clocking mechanism in later patch.

> > +
> > + if (core->rst_gpio) {
> > + gpiod_set_value_cansleep(core->rst_gpio, 1);
> > + /* minimum of 40 dphy_clk_200M cycles */
> > + usleep_range(1, 2);
> > + gpiod_set_value_cansleep(core->rst_gpio, 0);
> > + }
>
> This is done in xcsi2rxss_s_stream() too, I would move the logic to a
> xcsi2rxss_hard_reset() function.
>

Ok I will create the new function.

> > +
> > + xcsi2rxss_reset(core);
>
> And rename this xcsi2rxss_soft_reset();
>

Agreed. I will do this in the next version.

> > +
> > + /* Initialize V4L2 subdevice and media entity */
> > + xcsi2rxss->pads[XVIP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > + xcsi2rxss->pads[XVIP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > + /* Initialize the default format */
> > + xcsi2rxss_set_default_format(xcsi2rxss);
> > + xcsi2rxss->format = xcsi2rxss->default_format;
> > +
> > + /* Initialize the mbus formats supported */
> > + xcsi2rxss_init_mbus_fmts(xcsi2rxss);
> > +
> > + /* Initialize V4L2 subdevice and media entity */
> > + subdev = &xcsi2rxss->subdev;
> > + v4l2_subdev_init(subdev, &xcsi2rxss_ops);
> > + subdev->dev = &pdev->dev;
> > + subdev->internal_ops = &xcsi2rxss_internal_ops;
> > + strscpy(subdev->name, dev_name(&pdev->dev), sizeof(subdev-
> >name));
> > + subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS |
> V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + subdev->entity.ops = &xcsi2rxss_media_ops;
> > + v4l2_set_subdevdata(subdev, xcsi2rxss);
> > +
> > + ret = media_entity_pads_init(&subdev->entity, XCSI_MEDIA_PADS,
> > + xcsi2rxss->pads);
> > + if (ret < 0)
> > + goto error;
> > +
> > + platform_set_drvdata(pdev, xcsi2rxss);
> > +
> > + ret = v4l2_async_register_subdev(subdev);
> > + if (ret < 0) {
> > + dev_err(core->dev, "failed to register subdev\n");
> > + goto error;
> > + }
> > +
> > + dev_info(core->dev, "Xilinx CSI2 Rx Subsystem device found!\n");
>
> I'd drop the "!".
>

Noted for the next version.

> > +
> > + return 0;
> > +error:
> > + media_entity_cleanup(&subdev->entity);
> > + mutex_destroy(&xcsi2rxss->lock);
>
> This should go below, just before return ret, and you should add another
> error label there as mutex_destroy() should be called for a few error
> cases that currently return directly.
>
I will check and update in the next version.

> > + clk_bulk_disable_unprepare(num_clks, core->clks);
> > +err_clk_put:
> > + clk_bulk_put(num_clks, core->clks);
> > + return ret;
> > +}
> > +
> > +static int xcsi2rxss_remove(struct platform_device *pdev)
> > +{
> > + struct xcsi2rxss_state *xcsi2rxss = platform_get_drvdata(pdev);
> > + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > + struct v4l2_subdev *subdev = &xcsi2rxss->subdev;
> > + int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
> > +
> > + v4l2_async_unregister_subdev(subdev);
> > + media_entity_cleanup(&subdev->entity);
> > + mutex_destroy(&xcsi2rxss->lock);
> > + clk_bulk_disable_unprepare(num_clks, core->clks);
> > + clk_bulk_put(num_clks, core->clks);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id xcsi2rxss_of_id_table[] = {
> > + { .compatible = "xlnx,mipi-csi2-rx-subsystem-5.0", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, xcsi2rxss_of_id_table);
> > +
> > +static struct platform_driver xcsi2rxss_driver = {
> > + .driver = {
> > + .name = "xilinx-csi2rxss",
> > + .of_match_table = xcsi2rxss_of_id_table,
> > + },
> > + .probe = xcsi2rxss_probe,
> > + .remove = xcsi2rxss_remove,
> > +};
> > +
> > +module_platform_driver(xcsi2rxss_driver);
> > +
> > +MODULE_AUTHOR("Vishal Sagar <[email protected]>");
> > +MODULE_DESCRIPTION("Xilinx MIPI CSI2 Rx Subsystem Driver");
> > +MODULE_LICENSE("GPL v2");
>
> --
> Regards,
>
> Laurent Pinchart

Regards
Vishal Sagar

2020-04-20 19:26:07

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Laurent, Vishal,

On 19/04/20 20:02, Laurent Pinchart wrote:
[...]
>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
>> +{
>> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
>> + struct xcsi2rxss_core *core = &state->core;
>> + u32 status;
>> +
>> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
>> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
>
> As this is expected to occur for every frame, I would drop the message,
> even if rate-limited.
>
>> +
>> + if (!status)
>> + return IRQ_NONE;
>> +
>> + /* Received a short packet */
>> + if (status & XCSI_ISR_SPFIFONE) {
>> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
>> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
>> + }
>
> Same here, this will occur all the time, I'd remove this message. You
> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
> multiple short packets are received before the interrupt handler
> executes.
>
> I also wonder if it would make sense to extract the frame number from
> the FS short packet, and make it available through the subdev API. I
> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
> can be implemented later.
>
>> +
>> + /* Short packet FIFO overflow */
>> + if (status & XCSI_ISR_SPFIFOF)
>> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
>> +
>> + /*
>> + * Stream line buffer full
>> + * This means there is a backpressure from downstream IP
>> + */
>> + if (status & XCSI_ISR_SLBF) {
>> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
>> + xcsi2rxss_stop_stream(state);
>> + if (core->rst_gpio) {
>> + gpiod_set_value(core->rst_gpio, 1);
>> + /* minimum 40 dphy_clk_200M cycles */
>> + ndelay(250);
>> + gpiod_set_value(core->rst_gpio, 0);
>> + }
>
> I don't think you should stop the core here. xcsi2rxss_stop_stream()
> calls the source .s_stream(0) operation, which usually involves I2C
> writes that will sleep.
>
> You should instead report an event to userspace (it looks like we have
> no error event defined in V4L2, one should be added), and rely on the
> normal stop procedure.

FWIW, since a long time I've been using a modified version of this
routine, where after a Stream Line Buffer Full condition I just stop and
restart the csi2rx core and the stream continues after a minimal glitch.
Other subdev are unaware that anything has happened and keep on streaming.

Not sure this is the correct thing to do, but it's working for me. Also
I proposed this topic in one of the previous iterations of this patch,
but the situation was different because the stream on/off was not
propagated back at that time.

--
Luca

2020-04-20 19:59:47

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Luca,

On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
> On 19/04/20 20:02, Laurent Pinchart wrote:
> [...]
> >> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> >> +{
> >> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> >> + struct xcsi2rxss_core *core = &state->core;
> >> + u32 status;
> >> +
> >> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
> >> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
> >
> > As this is expected to occur for every frame, I would drop the message,
> > even if rate-limited.
> >
> >> +
> >> + if (!status)
> >> + return IRQ_NONE;
> >> +
> >> + /* Received a short packet */
> >> + if (status & XCSI_ISR_SPFIFONE) {
> >> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> >> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> >> + }
> >
> > Same here, this will occur all the time, I'd remove this message. You
> > need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
> > until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
> > multiple short packets are received before the interrupt handler
> > executes.
> >
> > I also wonder if it would make sense to extract the frame number from
> > the FS short packet, and make it available through the subdev API. I
> > think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
> > can be implemented later.
> >
> >> +
> >> + /* Short packet FIFO overflow */
> >> + if (status & XCSI_ISR_SPFIFOF)
> >> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
> >> +
> >> + /*
> >> + * Stream line buffer full
> >> + * This means there is a backpressure from downstream IP
> >> + */
> >> + if (status & XCSI_ISR_SLBF) {
> >> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
> >> + xcsi2rxss_stop_stream(state);
> >> + if (core->rst_gpio) {
> >> + gpiod_set_value(core->rst_gpio, 1);
> >> + /* minimum 40 dphy_clk_200M cycles */
> >> + ndelay(250);
> >> + gpiod_set_value(core->rst_gpio, 0);
> >> + }
> >
> > I don't think you should stop the core here. xcsi2rxss_stop_stream()
> > calls the source .s_stream(0) operation, which usually involves I2C
> > writes that will sleep.
> >
> > You should instead report an event to userspace (it looks like we have
> > no error event defined in V4L2, one should be added), and rely on the
> > normal stop procedure.
>
> FWIW, since a long time I've been using a modified version of this
> routine, where after a Stream Line Buffer Full condition I just stop and
> restart the csi2rx core and the stream continues after a minimal glitch.
> Other subdev are unaware that anything has happened and keep on streaming.
>
> Not sure this is the correct thing to do, but it's working for me. Also
> I proposed this topic in one of the previous iterations of this patch,
> but the situation was different because the stream on/off was not
> propagated back at that time.

Thanks for the feedback. How often does this occur in practice ?

--
Regards,

Laurent Pinchart

2020-04-21 07:47:34

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Laurent,

On 20/04/20 21:57, Laurent Pinchart wrote:
> Hi Luca,
>
> On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
>> On 19/04/20 20:02, Laurent Pinchart wrote:
>> [...]
>>>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
>>>> +{
>>>> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
>>>> + struct xcsi2rxss_core *core = &state->core;
>>>> + u32 status;
>>>> +
>>>> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
>>>> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
>>>
>>> As this is expected to occur for every frame, I would drop the message,
>>> even if rate-limited.
>>>
>>>> +
>>>> + if (!status)
>>>> + return IRQ_NONE;
>>>> +
>>>> + /* Received a short packet */
>>>> + if (status & XCSI_ISR_SPFIFONE) {
>>>> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
>>>> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
>>>> + }
>>>
>>> Same here, this will occur all the time, I'd remove this message. You
>>> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
>>> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
>>> multiple short packets are received before the interrupt handler
>>> executes.
>>>
>>> I also wonder if it would make sense to extract the frame number from
>>> the FS short packet, and make it available through the subdev API. I
>>> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
>>> can be implemented later.
>>>
>>>> +
>>>> + /* Short packet FIFO overflow */
>>>> + if (status & XCSI_ISR_SPFIFOF)
>>>> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
>>>> +
>>>> + /*
>>>> + * Stream line buffer full
>>>> + * This means there is a backpressure from downstream IP
>>>> + */
>>>> + if (status & XCSI_ISR_SLBF) {
>>>> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
>>>> + xcsi2rxss_stop_stream(state);
>>>> + if (core->rst_gpio) {
>>>> + gpiod_set_value(core->rst_gpio, 1);
>>>> + /* minimum 40 dphy_clk_200M cycles */
>>>> + ndelay(250);
>>>> + gpiod_set_value(core->rst_gpio, 0);
>>>> + }
>>>
>>> I don't think you should stop the core here. xcsi2rxss_stop_stream()
>>> calls the source .s_stream(0) operation, which usually involves I2C
>>> writes that will sleep.
>>>
>>> You should instead report an event to userspace (it looks like we have
>>> no error event defined in V4L2, one should be added), and rely on the
>>> normal stop procedure.
>>
>> FWIW, since a long time I've been using a modified version of this
>> routine, where after a Stream Line Buffer Full condition I just stop and
>> restart the csi2rx core and the stream continues after a minimal glitch.
>> Other subdev are unaware that anything has happened and keep on streaming.
>>
>> Not sure this is the correct thing to do, but it's working for me. Also
>> I proposed this topic in one of the previous iterations of this patch,
>> but the situation was different because the stream on/off was not
>> propagated back at that time.
>
> Thanks for the feedback. How often does this occur in practice ?

Quite often indeed in my case, as the MIPI stream comes from a remote
sensor via a video serdes chipset, and both the cable and the remote
sensor module are subject to heavy EMI. Depending on the setup I
observed SLBF happening up to 5~10 times per hour.

--
Luca

2020-04-21 08:42:35

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Luca,

On Tue, Apr 21, 2020 at 09:45:56AM +0200, Luca Ceresoli wrote:
> On 20/04/20 21:57, Laurent Pinchart wrote:
> > On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
> >> On 19/04/20 20:02, Laurent Pinchart wrote:
> >> [...]
> >>>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> >>>> +{
> >>>> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> >>>> + struct xcsi2rxss_core *core = &state->core;
> >>>> + u32 status;
> >>>> +
> >>>> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
> >>>> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
> >>>
> >>> As this is expected to occur for every frame, I would drop the message,
> >>> even if rate-limited.
> >>>
> >>>> +
> >>>> + if (!status)
> >>>> + return IRQ_NONE;
> >>>> +
> >>>> + /* Received a short packet */
> >>>> + if (status & XCSI_ISR_SPFIFONE) {
> >>>> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> >>>> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> >>>> + }
> >>>
> >>> Same here, this will occur all the time, I'd remove this message. You
> >>> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
> >>> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
> >>> multiple short packets are received before the interrupt handler
> >>> executes.
> >>>
> >>> I also wonder if it would make sense to extract the frame number from
> >>> the FS short packet, and make it available through the subdev API. I
> >>> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
> >>> can be implemented later.
> >>>
> >>>> +
> >>>> + /* Short packet FIFO overflow */
> >>>> + if (status & XCSI_ISR_SPFIFOF)
> >>>> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
> >>>> +
> >>>> + /*
> >>>> + * Stream line buffer full
> >>>> + * This means there is a backpressure from downstream IP
> >>>> + */
> >>>> + if (status & XCSI_ISR_SLBF) {
> >>>> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
> >>>> + xcsi2rxss_stop_stream(state);
> >>>> + if (core->rst_gpio) {
> >>>> + gpiod_set_value(core->rst_gpio, 1);
> >>>> + /* minimum 40 dphy_clk_200M cycles */
> >>>> + ndelay(250);
> >>>> + gpiod_set_value(core->rst_gpio, 0);
> >>>> + }
> >>>
> >>> I don't think you should stop the core here. xcsi2rxss_stop_stream()
> >>> calls the source .s_stream(0) operation, which usually involves I2C
> >>> writes that will sleep.
> >>>
> >>> You should instead report an event to userspace (it looks like we have
> >>> no error event defined in V4L2, one should be added), and rely on the
> >>> normal stop procedure.
> >>
> >> FWIW, since a long time I've been using a modified version of this
> >> routine, where after a Stream Line Buffer Full condition I just stop and
> >> restart the csi2rx core and the stream continues after a minimal glitch.
> >> Other subdev are unaware that anything has happened and keep on streaming.
> >>
> >> Not sure this is the correct thing to do, but it's working for me. Also
> >> I proposed this topic in one of the previous iterations of this patch,
> >> but the situation was different because the stream on/off was not
> >> propagated back at that time.
> >
> > Thanks for the feedback. How often does this occur in practice ?
>
> Quite often indeed in my case, as the MIPI stream comes from a remote
> sensor via a video serdes chipset, and both the cable and the remote
> sensor module are subject to heavy EMI. Depending on the setup I
> observed SLBF happening up to 5~10 times per hour.

Ouch, that is a lot ! Is that really caused by EMI though ? I thought
SLBF was due to the downstream components applying backpressure.

--
Regards,

Laurent Pinchart

2020-04-21 08:47:31

[permalink] [raw]

Subject: RE: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Luca,

> -----Original Message-----
> From: Luca Ceresoli <[email protected]>
> Sent: Tuesday, April 21, 2020 1:16 PM
> To: Laurent Pinchart <[email protected]>
> Cc: Vishal Sagar <[email protected]>; Hyun Kwon <[email protected]>;
> [email protected]; [email protected]; [email protected]; Michal
> Simek <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; Dinesh Kumar
> <[email protected]>; Sandip Kothari <[email protected]>; Jacopo Mondi
> <[email protected]>; Hyun Kwon <[email protected]>
> Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem driver
>
> Hi Laurent,
>
> On 20/04/20 21:57, Laurent Pinchart wrote:
> > Hi Luca,
> >
> > On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
> >> On 19/04/20 20:02, Laurent Pinchart wrote:
> >> [...]
> >>>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id) {
> >>>> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> >>>> + struct xcsi2rxss_core *core = &state->core;
> >>>> + u32 status;
> >>>> +
> >>>> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) &
> XCSI_ISR_ALLINTR_MASK;
> >>>> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n",
> >>>> +status);
> >>>
> >>> As this is expected to occur for every frame, I would drop the
> >>> message, even if rate-limited.
> >>>
> >>>> +
> >>>> + if (!status)
> >>>> + return IRQ_NONE;
> >>>> +
> >>>> + /* Received a short packet */
> >>>> + if (status & XCSI_ISR_SPFIFONE) {
> >>>> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> >>>> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> >>>> + }
> >>>
> >>> Same here, this will occur all the time, I'd remove this message.
> >>> You need to read XCSI_SPKTR_OFFSET though, and you should do so in a
> >>> loop until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in
> >>> case multiple short packets are received before the interrupt
> >>> handler executes.
> >>>
> >>> I also wonder if it would make sense to extract the frame number
> >>> from the FS short packet, and make it available through the subdev
> >>> API. I think it should be reported through a V4L2_EVENT_FRAME_SYNC
> >>> event. This can be implemented later.
> >>>
> >>>> +
> >>>> + /* Short packet FIFO overflow */
> >>>> + if (status & XCSI_ISR_SPFIFOF)
> >>>> + dev_dbg_ratelimited(core->dev, "Short packet FIFO
> >>>> +overflowed\n");
> >>>> +
> >>>> + /*
> >>>> + * Stream line buffer full
> >>>> + * This means there is a backpressure from downstream IP
> >>>> + */
> >>>> + if (status & XCSI_ISR_SLBF) {
> >>>> + dev_alert_ratelimited(core->dev, "Stream Line Buffer
> Full!\n");
> >>>> + xcsi2rxss_stop_stream(state);
> >>>> + if (core->rst_gpio) {
> >>>> + gpiod_set_value(core->rst_gpio, 1);
> >>>> + /* minimum 40 dphy_clk_200M cycles */
> >>>> + ndelay(250);
> >>>> + gpiod_set_value(core->rst_gpio, 0);
> >>>> + }
> >>>
> >>> I don't think you should stop the core here. xcsi2rxss_stop_stream()
> >>> calls the source .s_stream(0) operation, which usually involves I2C
> >>> writes that will sleep.
> >>>
> >>> You should instead report an event to userspace (it looks like we
> >>> have no error event defined in V4L2, one should be added), and rely
> >>> on the normal stop procedure.
> >>
> >> FWIW, since a long time I've been using a modified version of this
> >> routine, where after a Stream Line Buffer Full condition I just stop
> >> and restart the csi2rx core and the stream continues after a minimal
> glitch.
> >> Other subdev are unaware that anything has happened and keep on
> streaming.
> >>
> >> Not sure this is the correct thing to do, but it's working for me.
> >> Also I proposed this topic in one of the previous iterations of this
> >> patch, but the situation was different because the stream on/off was
> >> not propagated back at that time.
> >
> > Thanks for the feedback. How often does this occur in practice ?
>
> Quite often indeed in my case, as the MIPI stream comes from a remote
> sensor via a video serdes chipset, and both the cable and the remote sensor
> module are subject to heavy EMI. Depending on the setup I observed SLBF
> happening up to 5~10 times per hour.
>
> --
> Luca

Thanks for sharing your observation.
Getting a stream line buffer full condition indicates a design issue.
Stopping, resetting using video_aresetn and starting is valid way to start MIPI CSI-2 Rx SS but masks the issue.
Hence the current implementation is to warn and stop streaming.

Regards
Vishal Sagar

2020-04-21 09:55:54

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Laurent,

On 21/04/20 10:38, Laurent Pinchart wrote:
> Hi Luca,
>
> On Tue, Apr 21, 2020 at 09:45:56AM +0200, Luca Ceresoli wrote:
>> On 20/04/20 21:57, Laurent Pinchart wrote:
>>> On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
>>>> On 19/04/20 20:02, Laurent Pinchart wrote:
>>>> [...]
>>>>>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
>>>>>> +{
>>>>>> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
>>>>>> + struct xcsi2rxss_core *core = &state->core;
>>>>>> + u32 status;
>>>>>> +
>>>>>> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
>>>>>> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
>>>>>
>>>>> As this is expected to occur for every frame, I would drop the message,
>>>>> even if rate-limited.
>>>>>
>>>>>> +
>>>>>> + if (!status)
>>>>>> + return IRQ_NONE;
>>>>>> +
>>>>>> + /* Received a short packet */
>>>>>> + if (status & XCSI_ISR_SPFIFONE) {
>>>>>> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
>>>>>> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
>>>>>> + }
>>>>>
>>>>> Same here, this will occur all the time, I'd remove this message. You
>>>>> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
>>>>> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
>>>>> multiple short packets are received before the interrupt handler
>>>>> executes.
>>>>>
>>>>> I also wonder if it would make sense to extract the frame number from
>>>>> the FS short packet, and make it available through the subdev API. I
>>>>> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
>>>>> can be implemented later.
>>>>>
>>>>>> +
>>>>>> + /* Short packet FIFO overflow */
>>>>>> + if (status & XCSI_ISR_SPFIFOF)
>>>>>> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
>>>>>> +
>>>>>> + /*
>>>>>> + * Stream line buffer full
>>>>>> + * This means there is a backpressure from downstream IP
>>>>>> + */
>>>>>> + if (status & XCSI_ISR_SLBF) {
>>>>>> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
>>>>>> + xcsi2rxss_stop_stream(state);
>>>>>> + if (core->rst_gpio) {
>>>>>> + gpiod_set_value(core->rst_gpio, 1);
>>>>>> + /* minimum 40 dphy_clk_200M cycles */
>>>>>> + ndelay(250);
>>>>>> + gpiod_set_value(core->rst_gpio, 0);
>>>>>> + }
>>>>>
>>>>> I don't think you should stop the core here. xcsi2rxss_stop_stream()
>>>>> calls the source .s_stream(0) operation, which usually involves I2C
>>>>> writes that will sleep.
>>>>>
>>>>> You should instead report an event to userspace (it looks like we have
>>>>> no error event defined in V4L2, one should be added), and rely on the
>>>>> normal stop procedure.
>>>>
>>>> FWIW, since a long time I've been using a modified version of this
>>>> routine, where after a Stream Line Buffer Full condition I just stop and
>>>> restart the csi2rx core and the stream continues after a minimal glitch.
>>>> Other subdev are unaware that anything has happened and keep on streaming.
>>>>
>>>> Not sure this is the correct thing to do, but it's working for me. Also
>>>> I proposed this topic in one of the previous iterations of this patch,
>>>> but the situation was different because the stream on/off was not
>>>> propagated back at that time.
>>>
>>> Thanks for the feedback. How often does this occur in practice ?
>>
>> Quite often indeed in my case, as the MIPI stream comes from a remote
>> sensor via a video serdes chipset, and both the cable and the remote
>> sensor module are subject to heavy EMI. Depending on the setup I
>> observed SLBF happening up to 5~10 times per hour.
>
> Ouch, that is a lot !

That is the worst case, but yes, its a lot.

> Is that really caused by EMI though ? I thought
> SLBF was due to the downstream components applying backpressure.

Hum, good point. I might be wrong, I did the tests several months ago
and cannot do them again at the moment to confirm. But at some point my
suspect was that in case of noise at the upstream side, on the MIPI line
there can be an excess of packets w.r.t. the normal streams (perhaps
short packets?) that produces frames with more lines than expected. But
it's just a wild idea I got, never had an opportunity to examine it in
depth, sorry.

--
Luca

2020-04-21 10:32:24

[permalink] [raw]

Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Vishal,

thanks for having resumed this patchset!

On 09/04/20 21:44, Vishal Sagar wrote:
[...]
> +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> +{
> + struct xcsi2rxss_core *core = &xcsi2rxss->core;
> + struct device_node *node = xcsi2rxss->core.dev->of_node;

Can be simplified as:

struct device_node *node = core.dev->of_node;

> + unsigned int nports, irq;
> + bool en_csi_v20, vfb;
> + int ret;
> +
> + en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
> + if (en_csi_v20)
> + core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx");
> +
> + core->enable_active_lanes =
> + of_property_read_bool(node, "xlnx,en-active-lanes");
> +
> + ret = of_property_read_u32(node, "xlnx,csi-pxl-format",
> + &core->datatype);
> + if (ret < 0) {
> + dev_err(core->dev, "missing xlnx,csi-pxl-format property\n");
> + return ret;
> + }
> +
> + switch (core->datatype) {
> + case XCSI_DT_YUV4228B:
> + case XCSI_DT_RGB444:
> + case XCSI_DT_RGB555:
> + case XCSI_DT_RGB565:
> + case XCSI_DT_RGB666:
> + case XCSI_DT_RGB888:
> + case XCSI_DT_RAW6:
> + case XCSI_DT_RAW7:
> + case XCSI_DT_RAW8:
> + case XCSI_DT_RAW10:
> + case XCSI_DT_RAW12:
> + case XCSI_DT_RAW14:
> + break;
> + case XCSI_DT_YUV42210B:
> + case XCSI_DT_RAW16:
> + case XCSI_DT_RAW20:
> + if (!en_csi_v20) {
> + ret = -EINVAL;
> + dev_dbg(core->dev, "enable csi v2 for this pixel format");
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + if (ret < 0) {
> + dev_err(core->dev, "invalid csi-pxl-format property!\n");
> + return ret;
> + }
> +
> + vfb = of_property_read_bool(node, "xlnx,vfb");
> + if (!vfb) {
> + dev_err(core->dev, "failed as VFB is disabled!\n");
> + return -EINVAL;
> + }
> +
> + for (nports = 0; nports < XCSI_MEDIA_PADS; nports++) {
> + struct fwnode_handle *ep;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(core->dev),
> + nports, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + break;
> + /*
> + * since first port is sink port and it contains
> + * all info about data-lanes and cfa-pattern,
> + * don't parse second port but only check if exists
> + */
> + if (nports == XVIP_PAD_SOURCE) {
> + dev_dbg(core->dev, "no need to parse source port");
> + fwnode_handle_put(ep);
> + continue;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret) {
> + dev_err(core->dev, "error parsing sink port");
> + fwnode_handle_put(ep);
> + return ret;
> + }
> +
> + dev_dbg(core->dev, "port %d bus type = %d\n", nports,
> + vep.bus_type);
> +
> + if (vep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> + dev_dbg(core->dev, "base.port = %d base.id = %d\n",
> + vep.base.port, vep.base.id);
> +
> + dev_dbg(core->dev, "mipi number lanes = %d\n",
> + vep.bus.mipi_csi2.num_data_lanes);
> +
> + core->max_num_lanes =
> + vep.bus.mipi_csi2.num_data_lanes;
> + }
> + fwnode_handle_put(ep);
> + }
> +
> + if (nports != XCSI_MEDIA_PADS) {
> + dev_err(core->dev, "invalid number of ports %u\n", nports);
> + return -EINVAL;
> + }
> +
> + /* Register interrupt handler */
> + irq = irq_of_parse_and_map(node, 0);
> + ret = devm_request_irq(core->dev, irq, xcsi2rxss_irq_handler,
> + IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss);
> + if (ret) {
> + dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n",
> + ret);
> + return ret;
> + }

When using this driver I have changed this to a threaded IRQ, moving
most of the management out of interrupt context. The patch is super
simple and it works fine, for my use case at least. Do you think a
strict IRQ is really needed for some reason?

> + xcsi2rxss_log_ipconfig(xcsi2rxss);
> +
> + return 0;

This function references 'core->dev' a lot of times, so I'd rather add
at the top of the function:

struct device * const dev = &pdev->dev;

and then use simply 'dev' everywhere. This would keep lines shorter and
more readable. It is also handy when copying/moving a line of code from
one function to another if all of them have 'dev' called the same way so
I tend to do use this pattern often.

> +}
> +
> +static int xcsi2rxss_probe(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *subdev;
> + struct xcsi2rxss_state *xcsi2rxss;
> + struct xcsi2rxss_core *core;
> + struct resource *res;
> + int num_clks = ARRAY_SIZE(xcsi2rxss_clks);
> + int ret;
> +
> + xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL);
> + if (!xcsi2rxss)
> + return -ENOMEM;
> +
> + core = &xcsi2rxss->core;
> + core->dev = &pdev->dev;

This function references 'dev' many times, sometimes as &pdev->dev,
thers as 'core->dev', thus as above why not adding at the top of the
function:

struct device * const dev = &pdev->dev;

and simplify code using 'dev' always?

> + core->clks = devm_kmemdup(core->dev, xcsi2rxss_clks,
> + sizeof(xcsi2rxss_clks), GFP_KERNEL);
> + if (!core->clks)
> + return -ENOMEM;
> +
> + /* Reset GPIO */
> + core->rst_gpio = devm_gpiod_get_optional(core->dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(core->rst_gpio)) {
> + if (PTR_ERR(core->rst_gpio) != -EPROBE_DEFER)
> + dev_err(core->dev, "Video Reset GPIO not setup in DT");
> + return PTR_ERR(core->rst_gpio);
> + }
> +
> + mutex_init(&xcsi2rxss->lock);

There are 3 'return' statements after this call, and mutex_destroy()
won't be called if they trigger. Ok, probably no real effect as
mutex_init() is just initializing data, but for the sake of well-written
code you can simply move mutex_init()...

> + ret = xcsi2rxss_parse_of(xcsi2rxss);
> + if (ret < 0)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + core->iomem = devm_ioremap_resource(core->dev, res);
> + if (IS_ERR(core->iomem))
> + return PTR_ERR(core->iomem);
> +
> + ret = clk_bulk_get(core->dev, num_clks, core->clks);
> + if (ret)
> + return ret;
> +
> + ret = clk_bulk_prepare_enable(num_clks, core->clks);
> + if (ret)
> + goto err_clk_put;

...here.

> + if (core->rst_gpio) {
> + gpiod_set_value_cansleep(core->rst_gpio, 1);
> + /* minimum of 40 dphy_clk_200M cycles */
> + usleep_range(1, 2);
> + gpiod_set_value_cansleep(core->rst_gpio, 0);
> + }

--
Luca

2020-04-21 11:43:37