[v8,1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Commit Message

Vishal Sagar June 3, 2019, 9:59 a.m. UTC

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 <[e-mail]>
Reviewed-by: Hyun Kwon <[e-mail]>
Reviewed-by: Rob Herring <[e-mail]>
Reviewed-by: Luca Ceresoli <[e-mail]>
---
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        | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt

Comments

Sakari Ailus June 4, 2019, 3:25 p.m. UTC | #1

Hi Vishal,

Thanks for the update. A few small issues below...

On Mon, Jun 03, 2019 at 03:29:30PM +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.
> 
> Signed-off-by: Vishal Sagar <[e-mail]>
> Reviewed-by: Hyun Kwon <[e-mail]>
> Reviewed-by: Rob Herring <[e-mail]>
> Reviewed-by: Luca Ceresoli <[e-mail]>
> ---
> 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        | 119 +++++++++++++++++++++
>  1 file changed, 119 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 0000000..fef8179
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> @@ -0,0 +1,119 @@
> +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-4.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: This is present when Video Format Bridge is enabled.
> +  Without this property the driver won't be loaded as IP won't be able to generate
> +  media bus format compliant stream output.
> +- 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.
> +It must have the data-lanes property.

Technically this is wrong as data-lanes is an endpoint property. I'd just
leave it out here.

> +
> +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.
> +
> +Optional endpoint property:

s/Optional/Required/

> +---------------------------
> +- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-interfaces.txt.
> +  This should be in the sink port endpoint which connects to MIPI CSI2 source
> +  like sensor. The possible values are:

And add here that it's only needed for port 0. Or put it to the heading.

I think this is good with these changes.

> +  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-4.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>;
> +				};
> +			};
> +		};
> +	};
> -- 
> 1.8.3.1

How old is that? :-)

Sakari Ailus June 4, 2019, 7:23 p.m. UTC | #2

Hi Vishal,

On Mon, Jun 03, 2019 at 03:29:30PM +0530, Vishal Sagar wrote:

> +Optional properties:
> +--------------------
> +- xlnx,vfb: This is present when Video Format Bridge is enabled.
> +  Without this property the driver won't be loaded as IP won't be able to generate
> +  media bus format compliant stream output.

I think we previously concluded that the format will be just different in
this case. So the description appears incorrect.

Vishal Sagar June 6, 2019, 11:54 a.m. UTC | #3

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus [mailto:[e-mail]]
> Sent: Wednesday, June 05, 2019 12:54 AM
> To: Vishal Sagar <[e-mail]>
> Cc: Hyun Kwon <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Michal
> Simek <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Dinesh Kumar
> <[e-mail]>; Sandip Kothari <[e-mail]>; Luca Ceresoli
> <[e-mail]>; Jacopo Mondi <[e-mail]>
> Subject: Re: [PATCH v8 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI
> CSI-2 Rx Subsystem
> 
> EXTERNAL EMAIL
> 
> Hi Vishal,
> 
> On Mon, Jun 03, 2019 at 03:29:30PM +0530, Vishal Sagar wrote:
> 
> > +Optional properties:
> > +--------------------
> > +- xlnx,vfb: This is present when Video Format Bridge is enabled.
> > +  Without this property the driver won't be loaded as IP won't be able to
> generate
> > +  media bus format compliant stream output.
> 
> I think we previously concluded that the format will be just different in
> this case. So the description appears incorrect.
> 

Referring to your email https://lkml.org/lkml/2019/3/22/1823 in response to v6 patches,
if the video format bridge is not enabled, then the way in which pixels are transported on
the bus don't correspond to the existing media bus formats in Linux.

If not loading the driver is incorrect way, is it ok for the driver to allow same media bus format
for packed and unpacked data type on the sink pad?

Or is it ok for the driver to not validate the media bus format set on the sink pad?

> --
> Sakari Ailus
> [e-mail]

Regards
Vishal Sagar

Vishal Sagar June 6, 2019, 11:54 a.m. UTC | #4

Hi Sakari,

Thanks for reviewing this.

> -----Original Message-----
> From: [e-mail] [mailto:linux-media-
> [e-mail]] On Behalf Of Sakari Ailus
> Sent: Tuesday, June 04, 2019 8:56 PM
> To: Vishal Sagar <[e-mail]>
> Cc: Hyun Kwon <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Michal
> Simek <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Dinesh Kumar
> <[e-mail]>; Sandip Kothari <[e-mail]>; Luca Ceresoli
> <[e-mail]>; Jacopo Mondi <[e-mail]>
> Subject: Re: [PATCH v8 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI
> CSI-2 Rx Subsystem
> 
> Hi Vishal,
> 
> Thanks for the update. A few small issues below...
> 
> On Mon, Jun 03, 2019 at 03:29:30PM +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.
> >
> > Signed-off-by: Vishal Sagar <[e-mail]>
> > Reviewed-by: Hyun Kwon <[e-mail]>
> > Reviewed-by: Rob Herring <[e-mail]>
> > Reviewed-by: Luca Ceresoli <[e-mail]>
> > ---
> > 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        | 119
> +++++++++++++++++++++
> >  1 file changed, 119 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 0000000..fef8179
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
> > @@ -0,0 +1,119 @@
> > +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-4.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: This is present when Video Format Bridge is enabled.
> > +  Without this property the driver won't be loaded as IP won't be able to
> generate
> > +  media bus format compliant stream output.
> > +- 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.
> > +It must have the data-lanes property.
> 
> Technically this is wrong as data-lanes is an endpoint property. I'd just
> leave it out here.
> 

Ok I will remove it from here.

> > +
> > +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.
> > +
> > +Optional endpoint property:
> 
> s/Optional/Required/
> 
Noted. I will change it to required.

> > +---------------------------
> > +- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-
> interfaces.txt.
> > +  This should be in the sink port endpoint which connects to MIPI CSI2 source
> > +  like sensor. The possible values are:
> 
> And add here that it's only needed for port 0. Or put it to the heading.

Ok.

> 
> I think this is good with these changes.
> 
> > +  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-4.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>;
> > +				};
> > +			};
> > +		};
> > +	};
> > --
> > 1.8.3.1
> 
> How old is that? :-)
> 

Found out that version is nearly 6 years old.  :-)
I will update git.

> --
> Kind regards,
> 
> Sakari Ailus
> [e-mail]

Regards
Vishal Sagar

Sakari Ailus June 6, 2019, 12:12 p.m. UTC | #5

Hi Vishal,

On Thu, Jun 06, 2019 at 11:54:19AM +0000, Vishal Sagar wrote:
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus [mailto:[e-mail]]
> > Sent: Wednesday, June 05, 2019 12:54 AM
> > To: Vishal Sagar <[e-mail]>
> > Cc: Hyun Kwon <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; Michal
> > Simek <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]; Dinesh Kumar
> > <[e-mail]>; Sandip Kothari <[e-mail]>; Luca Ceresoli
> > <[e-mail]>; Jacopo Mondi <[e-mail]>
> > Subject: Re: [PATCH v8 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI
> > CSI-2 Rx Subsystem
> > 
> > EXTERNAL EMAIL
> > 
> > Hi Vishal,
> > 
> > On Mon, Jun 03, 2019 at 03:29:30PM +0530, Vishal Sagar wrote:
> > 
> > > +Optional properties:
> > > +--------------------
> > > +- .
> > > +  Without this property the driver won't be loaded as IP won't be able to
> > generate
> > > +  media bus format compliant stream output.
> > 
> > I think we previously concluded that the format will be just different in
> > this case. So the description appears incorrect.
> > 
> 
> Referring to your email https://lkml.org/lkml/2019/3/22/1823 in response to v6 patches,
> if the video format bridge is not enabled, then the way in which pixels are transported on
> the bus don't correspond to the existing media bus formats in Linux.
> 
> If not loading the driver is incorrect way, is it ok for the driver to allow same media bus format
> for packed and unpacked data type on the sink pad?
> 
> Or is it ok for the driver to not validate the media bus format set on the sink pad?

Taking a fresh look at the issue --- usually such unpacking is done by the
DMA engine, or the same device contains both the CSI-2 RX and DMA. But here
it actually affects the input of that DMA engine. You're right in saying we
don't have format definitions from which you could tell which case it is,
and we also don't have other pre-existing means to tell them apart.

Feel free to keep the check in the driver, but we can't refer to the driver
loading in DT binding documentation: this is really not supposed to be
related to that driver, or even Linux at all.

How about changing this to:

xlnx,vfb: Present when Video Format Bridge is enabled in IP configuration.

That'd be aligned with the other properties and would more accurately
convey what this means.

Vishal Sagar June 6, 2019, 12:23 p.m. UTC | #6

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus [mailto:[e-mail]]
> Sent: Thursday, June 06, 2019 5:43 PM
> To: Vishal Sagar <[e-mail]>
> Cc: Vishal Sagar <[e-mail]>; Hyun Kwon <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Michal Simek
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Dinesh Kumar
> <[e-mail]>; Sandip Kothari <[e-mail]>; Luca Ceresoli
> <[e-mail]>; Jacopo Mondi <[e-mail]>
> Subject: Re: [PATCH v8 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI
> CSI-2 Rx Subsystem
> 
> Hi Vishal,
> 
> On Thu, Jun 06, 2019 at 11:54:19AM +0000, Vishal Sagar wrote:
> > Hi Sakari,
> >
> > > -----Original Message-----
> > > From: Sakari Ailus [mailto:[e-mail]]
> > > Sent: Wednesday, June 05, 2019 12:54 AM
> > > To: Vishal Sagar <[e-mail]>
> > > Cc: Hyun Kwon <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> Michal
> > > Simek <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; linux-arm-
> > > [e-mail]; [e-mail]; Dinesh Kumar
> > > <[e-mail]>; Sandip Kothari <[e-mail]>; Luca Ceresoli
> > > <[e-mail]>; Jacopo Mondi <[e-mail]>
> > > Subject: Re: [PATCH v8 1/2] media: dt-bindings: media: xilinx: Add Xilinx
> MIPI
> > > CSI-2 Rx Subsystem
> > >
> > > EXTERNAL EMAIL
> > >
> > > Hi Vishal,
> > >
> > > On Mon, Jun 03, 2019 at 03:29:30PM +0530, Vishal Sagar wrote:
> > >
> > > > +Optional properties:
> > > > +--------------------
> > > > +- .
> > > > +  Without this property the driver won't be loaded as IP won't be able to
> > > generate
> > > > +  media bus format compliant stream output.
> > >
> > > I think we previously concluded that the format will be just different in
> > > this case. So the description appears incorrect.
> > >
> >
> > Referring to your email https://lkml.org/lkml/2019/3/22/1823 in response to
> v6 patches,
> > if the video format bridge is not enabled, then the way in which pixels are
> transported on
> > the bus don't correspond to the existing media bus formats in Linux.
> >
> > If not loading the driver is incorrect way, is it ok for the driver to allow same
> media bus format
> > for packed and unpacked data type on the sink pad?
> >
> > Or is it ok for the driver to not validate the media bus format set on the sink
> pad?
> 
> Taking a fresh look at the issue --- usually such unpacking is done by the
> DMA engine, or the same device contains both the CSI-2 RX and DMA. But here
> it actually affects the input of that DMA engine. You're right in saying we
> don't have format definitions from which you could tell which case it is,
> and we also don't have other pre-existing means to tell them apart.
> 
> Feel free to keep the check in the driver, but we can't refer to the driver
> loading in DT binding documentation: this is really not supposed to be
> related to that driver, or even Linux at all.
> 

Ok got it. The description here shouldn't be specific to driver.
I will keep the check in driver.

> How about changing this to:
> 
> xlnx,vfb: Present when Video Format Bridge is enabled in IP configuration.
> 
> That'd be aligned with the other properties and would more accurately
> convey what this means.

Agree this is a good way to define the property. I will update it the description as mentioned here.

> 
> --
> Kind regards,
> 
> Sakari Ailus
> [e-mail]

Regards
Vishal Sagar

Patch

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 0000000..fef8179
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.txt
@@ -0,0 +1,119 @@ 
+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-4.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: This is present when Video Format Bridge is enabled.
+  Without this property the driver won't be loaded as IP won't be able to generate
+  media bus format compliant stream output.
+- 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.
+It must have the data-lanes property.
+
+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.
+
+Optional endpoint property:
+---------------------------
+- data-lanes: specifies MIPI CSI-2 data lanes as covered in video-interfaces.txt.
+  This should be in the sink port 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-4.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>;
+				};
+			};
+		};
+	};