RE: [v3,2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

[Vishal Sagar]

Hi Hyun,

Thanks for reviewing. Apologies for the delayed response.

> -----Original Message-----
> From: Hyun Kwon [mailto:hyun.kwon@xxxxxxxxxx]
> Sent: Thursday, February 14, 2019 1:16 AM
> To: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> Cc: laurent.pinchart@xxxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
> hans.verkuil@xxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Dinesh Kumar <dineshk@xxxxxxxxxx>; Sandip Kothari
> <sandipk@xxxxxxxxxx>
> Subject: Re: [v3,2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem
> 
> Hi Vishal,
> 
> Thanks for the patch. Sorry for the delay.
> 
> On Fri, 2019-02-01 at 18:26:06 +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 driver is used to set the number of active lanes, if enabled
> > in hardware. 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 generic short packets received are notified to application via
> > v4l2_events.
> >
> > 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 <vishal.sagar@xxxxxxxxxx>
> > ---
> > 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
> 
> [snip]
> 
> > +};
> > +
> > +/*
> > + * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device
> structure
> > + * @dev: Platform structure
> > + * @iomem: Base address of subsystem
> > + * @irq: requested irq number
> > + * @enable_active_lanes: If number of active lanes can be modified
> > + * @max_num_lanes: Maximum number of lanes present
> > + * @datatype: Data type filter
> > + * @bayer: bayer pattern
> > + * @events: Structure to maintain event logs
> > + * @vcx_events: Structure to maintain VCX event logs
> > + * @en_vcx: If more than 4 VC are enabled
> > + * @lite_aclk: AXI4-Lite interface clock
> > + * @video_aclk: Video clock
> > + */
> > +struct xcsi2rxss_core {
> > +	struct device *dev;
> > +	void __iomem *iomem;
> > +	int irq;
> 
> This doesn't have to be stored.

Agree. I will remove it in next version.

> 
> > +	bool enable_active_lanes;
> > +	u32 max_num_lanes;
> > +	u32 datatype;
> > +	u32 bayer;
> > +	struct xcsi2rxss_event *events;
> > +	struct xcsi2rxss_event *vcx_events;
> > +	bool en_vcx;
> > +	struct clk *lite_aclk;
> > +	struct clk *video_aclk;
> > +};
> > +
> > +/**
> > + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
> > + * @core: Core structure for MIPI CSI2 Rx Subsystem
> > + * @subdev: The v4l2 subdev structure
> 
> [snip]
> 
> > +
> > +/* Print event counters */
> > +static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state)
> > +{
> > +	struct xcsi2rxss_core *core = &state->core;
> > +	int i;
> > +
> > +	for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > +		if (core->events[i].counter > 0)
> 
> Does checkpatch warn if putting {} here, and
> 
> > +			dev_info(core->dev, "%s events: %d\n",
> > +				 core->events[i].name,
> > +				 core->events[i].counter);
> > +	}
> > +
> > +	if (core->en_vcx)
> 
> here, and
> 
> > +		for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > +			if (core->vcx_events[i].counter > 0)
> 
> here? But up to you.

No checkpatch doesn't give warning. 

> 
> > +				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].counter);
> > +		}
> > +}
> > +
> > +/**
> > + * 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
> 
> [snip]
> 
> > +
> > +/**
> > + * xcsi2rxss_s_ctrl - This is used to set the Xilinx MIPI CSI-2 V4L2 controls
> > + * @ctrl: V4L2 control to be set
> > + *
> > + * This function is used to set the V4L2 controls for the Xilinx MIPI
> > + * CSI-2 Rx Subsystem. It is used to set the active lanes in the system.
> > + * The event counters can be reset.
> > + *
> > + * Return: 0 on success, errors otherwise
> > + */
> > +static int xcsi2rxss_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct xcsi2rxss_state *xcsi2rxss =
> > +		container_of(ctrl->handler, struct xcsi2rxss_state,
> > +			     ctrl_handler);
> > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&xcsi2rxss->lock);
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_XILINX_MIPICSISS_ACT_LANES:
> > +		/*
> > +		 * This will be called only when "Enable Active Lanes" parameter
> > +		 * is set in design
> > +		 */
> > +		if (core->enable_active_lanes) {
> > +			u32 active_lanes;
> > +
> > +			xcsi2rxss_clr_and_set(core, XCSI_PCR_OFFSET,
> > +					      XCSI_PCR_ACTLANES_MASK,
> > +					      ctrl->val - 1);
> > +			/*
> > +			 * This delay is to allow the value to reflect as write
> > +			 * and read paths are different.
> > +			 */
> > +			udelay(1);
> > +			active_lanes = xcsi2rxss_read(core, XCSI_PCR_OFFSET);
> > +			active_lanes &= XCSI_PCR_ACTLANES_MASK;
> > +			active_lanes++;
> > +			if (active_lanes != ctrl->val)
> > +				dev_info(core->dev, "RxByteClkHS absent\n");
> > +			dev_dbg(core->dev, "active lanes = %d\n", ctrl->val);
> > +		} else {
> > +			ret = -EINVAL;
> > +		}
> > +		break;
> > +	case V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS:
> > +		xcsi2rxss_reset_event_counters(xcsi2rxss);
> > +		break;
> > +	default:
> 
> -EINVAL?

Agree. I will add it in next version.

> 
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&xcsi2rxss->lock);
> > +
> > +	return ret;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +static void xcsi2rxss_clk_disable(struct xcsi2rxss_core *core)
> > +{
> > +	clk_disable_unprepare(core->video_aclk);
> > +	clk_disable_unprepare(core->lite_aclk);
> > +}
> > +
> > +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> > +{
> > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > +	struct device_node *node = xcsi2rxss->core.dev->of_node;
> > +	struct device_node *ports = NULL;
> > +	struct device_node *port = NULL;
> > +	unsigned int nports;
> > +	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");
> > +		dev_dbg(core->dev, "vcx %s", core->en_vcx ? "enabled" :
> > +			"disabled");
> > +	}
> > +
> > +	dev_dbg(core->dev, "en_csi_v20 %s", en_csi_v20 ? "enabled" :
> > +		"disabled");
> 
> Would it be better to have these options visible with log?
> It can be printed as a part of final banner in probe. Up to you.
> 

Ok. Will put it under a new function xcsi2rxss_log_ipconfig()

> > +
> > +	core->enable_active_lanes =
> > +		of_property_read_bool(node, "xlnx,en-active-lanes");
> > +	dev_dbg(core->dev, "Enable active lanes property = %s\n",
> > +		core->enable_active_lanes ? "Present" : "Absent");
> > +
> > +	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;
> > +	}
> > +
> > +	dev_dbg(core->dev, "pixel format set as 0x%x\n", core->datatype);
> > +
> > +	vfb = of_property_read_bool(node, "xlnx,vfb");
> > +	dev_dbg(core->dev, "Video Format Bridge property = %s\n",
> > +		vfb ? "Present" : "Absent");
> 
> I'd collect all these print in one print or one location. But up to you.

Ok. Will put it under a new function xcsi2rxss_log_ipconfig()

> 
> > +
> > +	if (!vfb) {
> > +		dev_err(core->dev, "failed as VFB is disabled!\n");
> > +		return -EINVAL;
> > +	}
> 
> [snip]
> 
> > +	/* Register interrupt handler */
> > +	core->irq = irq_of_parse_and_map(node, 0);
> > +	ret = devm_request_irq(core->dev, core->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;
> > +	}
> > +
> > +	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 ret, num_ctrls, i;
> > +
> > +	xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL);
> > +	if (!xcsi2rxss)
> > +		return -ENOMEM;
> > +
> > +	core = &xcsi2rxss->core;
> > +	core->dev = &pdev->dev;
> > +
> > +	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 = xcsi2rxss_clk_get(core);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = xcsi2rxss_clk_enable(core);
> > +	if (ret < 0)
> > +		return ret;
> 
> Not sure if I mentioned, please consider having these with
> stream on / off later.

We don't have a mechanism to reset the video path through software if it is connected to a Processor System Reset.
So for now I will keep it here.

> 
> I only have minor comments, and it looks fine to me. Please take a look at
> those comments, then
> 
> 	Reviewed-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> 
> Thanks,
> -hyun

Thanks,
Vishal