Hi Laurent and Rob,

Thanks for reviewing this patch.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Wednesday, July 15, 2020 9:59 PM
To: Vishal Sagar <redacted>
Cc: Rob Herring <robh@kernel.org>; Hyun Kwon <redacted>;
hverkuil@xs4all.nl; mchehab@kernel.org; mark.rutland@arm.com; Michal
Simek [off-list ref]; linux-media@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; joe@perches.com; Sandip Kothari
[off-list ref]; Dinesh Kumar [off-list ref]
Subject: Re: [PATCH v3 2/3] media: dt-bindings: media: xilinx: Add Xilinx UHD-
SDI Receiver Subsystem

Hi Vishal,

Thank you for the patch.

On Mon, Jul 13, 2020 at 12:54:47PM -0600, Rob Herring wrote:

quoted

On Thu, Jun 18, 2020 at 11:03:03AM +0530, Vishal Sagar wrote:

quoted

Add bindings documentation for Xilinx UHD-SDI Receiver Subsystem.

The Xilinx UHD-SDI Receiver Subsystem consists of SMPTE UHD-SDI (RX) IP
core, an SDI RX to Video Bridge IP core to convert SDI video to native
video and a Video In to AXI4-Stream IP core to convert native video to
AXI4-Stream.

Signed-off-by: Vishal Sagar <redacted>
---
v3
- bpc instead of bpp
- removed bpc as required property (default to 10 bpc)
- add dt-bindings/media/xilinx-sdi.h
- made line-rate as u32 instead of string
- fixed reg
- fixed s/upto/up to/

v2
- Removed references to xlnx,video*
- Fixed as per Sakari Ailus and Rob Herring's comments
- Converted to yaml format

 .../bindings/media/xilinx/xlnx,sdirxss.yaml   | 132 ++++++++++++++++++
 include/dt-bindings/media/xilinx-sdi.h        |  20 +++
 2 files changed, 152 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml

quoted

quoted

 create mode 100644 include/dt-bindings/media/xilinx-sdi.h

diff --git

a/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml

quoted

quoted

new file mode 100644
index 000000000000..6cfc18ca435f

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml

@@ -0,0 +1,132 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/xilinx/xlnx,sdirxss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+

I think a single blank line is enough.

Ok I will remove extra empty line in next version.

quoted

quoted

+title: Xilinx SMPTE UHD-SDI Receiver Subsystem
+
+maintainers:
+  - Vishal Sagar [off-list ref]
+
+description: |
+  The SMPTE UHD-SDI Receiver (RX) Subsystem allows you to quickly

create systems

quoted

quoted

+  based on SMPTE SDI protocols. It receives unaligned native SDI streams

from

quoted

quoted

+  the SDI GT PHY and outputs an AXI4-Stream video stream, native video,

or

quoted

quoted

+  native SDI using Xilinx transceivers as the physical layer.
+
+  The subsystem consists of
+  1 - SMPTE UHD-SDI Rx
+  2 - SDI Rx to Native Video Bridge
+  3 - Video In to AXI4-Stream Bridge
+
+  The subsystem can capture SDI streams in up to 12G mode 8 data

streams and output

quoted

quoted

+  a dual pixel per clock RGB/YUV444,422/420 10/12 bits per component

AXI4-Stream.

quoted

quoted

+
+properties:
+  compatible:
+    items:
+      - enum:
+        - xlnx,v-smpte-uhdsdi-rx-ss-2.0

Should be indented 2 more spaces.

Or you could simply use

properties:
  compatible:
    const: xlnx,v-smpte-uhdsdi-rx-ss-2.0

Ok I will fix this in the next version.

quoted

quoted

+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description: List of clock specifiers

Drop. That's every 'clocks' property.

Ok I will drop the description in next version.

quoted

quoted

+    items:
+      - description: AXI4-Lite clock
+      - description: SMPTE UHD-SDI Rx core clock
+      - description: Video clock
+
+  clock-names:
+    items:
+      - const: s_axi_aclk
+      - const: sdi_rx_clk
+      - const: video_out_clk
+
+  xlnx,bpc:
+    description: Bits per component supported. Can be 10 or 12 bits per

component only.

quoted

quoted

+    allOf:

You can drop the 'allOf' now.

Ok will update this in next version.

quoted

quoted

+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum: [10, 12]

Seems like this should be a standard property?

Rob, if my understanding is correct, this tells for how many bits per
component the IP core has been synthesized. I think it qualifies as a
vendor property, as how to express constraints on supported formats (for
IP cores that can be synthesized with different options) is highly
vendor-specific.

Right Laurent. This is specific to Xilinx Video IP cores.

Vishal, I think the question I asked in the review of v2 fell through
the cracks. Is the documentation for the new IP core version available ?
Should this property only be allowed for the new version, given that in
v2.0 the BPC is fixed to 10 ?

The new IP core is released with Vivado 2020.1 but the documentation isn't yet
publicly updated.

This property is optional and the driver defaults to 10bpc in case this property is not specified.

quoted

quoted

+
+  xlnx,line-rate:
+    description: |
+      The maximum mode supported by the design. Possible values are as

below

quoted

quoted

+      0 - XSDI_STD_3G      -  3G mode
+      1 - XSDI_STD_6G      -  6G mode
+      2 - XSDI_STD_12G_8DS - 12G mode with 8 data streams
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum: [0, 1, 2]

Standard?

For this one, I'm not sure. There's little support for SDI in the
kernel, and I'm sure we'll get this wrong the first time. I'd rather try
not to over-standardize properties before we have more examples.

Right. These are specific to Xilinx SDI Rx IP configuration.

quoted

quoted

+
+  xlnx,include-edh:
+    type: boolean
+    description: |
+      This is present when the Error Detection and Handling processor is
+      enabled in design.
+
+  ports:
+    type: object
+    description: |
+      Generally the SDI port is connected to a device like SDI Broadcast

camera

quoted

quoted

+      which is independently controlled. Hence port@0 is a source port

which can be

quoted

quoted

+      connected to downstream IP which can work with AXI4 Stream data.
+    properties:
+      port@0:
+        type: object
+        description: Source port
+        properties:
+          reg:
+            const: 0
+          endpoint:
+            type: object
+            properties:
+              remote-endpoint: true
+            required:
+              - remote-endpoint
+            additionalProperties: false
+        additionalProperties: false

Same here, I explained in the review of v2 that we should have an input
port.

I will add the input / sink port to this device tree node.

quoted

quoted

+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - xlnx,line-rate
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/media/xilinx-sdi.h>
+    uhdsdirxss: v-smpte-uhdsdi-rxss@80000000 {

The label is not used, you can drop it.

Ok. I will remove this in the next version.

quoted

quoted

+      compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0";
+      interrupt-parent = <&gic>;
+      interrupts = <0 89 4>;
+      reg = <0x80000000 0x10000>;
+      xlnx,include-edh;
+      xlnx,line-rate = <XSDI_STD_12G_8DS>;
+      clocks = <&clk_1>, <&si570_1>, <&clk_2>;
+      clock-names = "s_axi_aclk", "sdi_rx_clk", "video_out_clk";
+      xlnx,bpc = <10>;

I would group the xlnx,* properties after the standard properties.

Noted. Will update in next version.

quoted

quoted

+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        port@0 {
+          reg = <0>;
+          sdirx_out: endpoint {
+            remote-endpoint = <&vcap_sdirx_in>;
+          };
+        };
+      };
+    };
+...

diff --git a/include/dt-bindings/media/xilinx-sdi.h b/include/dt-

bindings/media/xilinx-sdi.h

quoted

quoted

new file mode 100644
index 000000000000..11938fade041

--- /dev/null
+++ b/include/dt-bindings/media/xilinx-sdi.h

@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Xilinx SDI device tree bindings
+ *
+ * Copyright (C) 2020 Xilinx, Inc.
+ *
+ * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
+ */
+
+#ifndef __DT_BINDINGS_MEDIA_XILINX_SDI_H__
+#define __DT_BINDINGS_MEDIA_XILINX_SDI_H__
+
+/*
+ * SDI Configurations
+ */
+#define XSDI_STD_3G		0
+#define XSDI_STD_6G		1
+#define XSDI_STD_12G_8DS	2
+
+#endif /* __DT_BINDINGS_MEDIA_XILINX_SDI_H__ */

--
Regards,

Laurent Pinchart

Regards
Vishal Sagar