RE: [PATCH v2 1/2] media: dt-bindings: media: xilinx: Add Xilinx UHD-SDI Receiver Subsystem

[Vishal Sagar]

Hi Laurent,

Thanks for the review.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, May 6, 2020 6:32 PM
> To: Vishal Sagar <vsagar@xxxxxxxxxx>
> Cc: Hyun Kwon <hyunk@xxxxxxxxxx>; mchehab@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; hans.verkuil@xxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Dinesh Kumar
> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>; Joe Perches
> <joe@xxxxxxxxxxx>
> Subject: Re: [PATCH v2 1/2] media: dt-bindings: media: xilinx: Add Xilinx UHD-
> SDI Receiver Subsystem
> 
> Hi Vishal,
> 
> Thank you for the patch.
> 
> On Wed, Apr 29, 2020 at 07:47:03PM +0530, Vishal Sagar wrote:
> > 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 <vishal.sagar@xxxxxxxxxx>
> > ---
> > 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 ++++++++++++++++++
> >  1 file changed, 132 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> b/Documentation/devicetree/bindings/media/xilinx/xlnx,sdirxss.yaml
> > new file mode 100644
> > index 000000000000..9133ad19df55
> > --- /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#
> > +
> > +
> > +title: Xilinx SMPTE UHD-SDI Receiver Subsystem
> > +
> > +maintainers:
> > +  - Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> > +
> > +description: |
> > +  The SMPTE UHD-SDI Receiver (RX) Subsystem allows you to quickly create
> systems
> > +  based on SMPTE SDI protocols. It receives unaligned native SDI streams
> from
> > +  the SDI GT PHY and outputs an AXI4-Stream video stream, native video, or
> > +  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 upto 12G mode 8 data streams
> and output
> 
> s/upto/up to/
> 

I will fix this in next version. 

> > +  a dual pixel per clock RGB/YUV444,422/420 10/12 bits per component
> AXI4-Stream.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - xlnx,v-smpte-uhdsdi-rx-ss-2.0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: List of clock specifiers
> > +    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,bpp:
> > +    description: Bits per pixel supported. Can be 10 or 12 bits per pixel only.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum: [10, 12]
> 
> I don't see this as a design parameter in the documentation (pg290,
> v2.0). What does it correspond to ? All the BPC mentions in the
> documentation always state that 10-bit is the only supported value.
> 

The new version of IP being released will have 10 and 12 bit support. It is already in the Xilinx linux-xlnx repo.
I will rename this to "xlnx,bpc" instead of "xlnx,bpp" to refer to bits per component.

> > +
> > +  xlnx,line-rate:
> > +    description: |
> > +      The maximum mode supported by the design. Possible values are as
> below
> > +      12G_SDI_8DS - 12G mode with 8 data streams
> > +      6G_SDI      -  6G mode
> > +      3G_SDI      -  3G mode
> > +    enum:
> > +      - 12G_SDI_8DS
> > +      - 6G_SDI
> > +      - 3G_SDI
> 
> How about making this an integer property, with #define in
> include/dt-bindings/media/xilinx-sdi.h ? As far as I understand, the SDI
> TX subsystem has the same parameter, so the #define could be shared
> between the two.
> 

Yes that is ok with me. I will add this in the next version.

> > +
> > +  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
> > +      which is independently controlled. Hence port@0 is a source port which
> can be
> > +      connected to downstream IP which can work with AXI4 Stream data.
> 
> We should still have an input port. It can be connected to a DT node for
> a physical SDI connector, or any other component in the platform (I
> expect the former to be the common case). There are DT bindings for
> connectors in Documentation/devicetree/bindings/display/connector/, we
> should add one for SDI.
> 

Yes the input port is a physical SDI connector connected to an equipment like broadcast camera.
But the camera/equipment can't be controlled by the V4L2 pipeline and SDI protocol is unidirectional.

If we add another dt node, then I think another dummy v4l subdev driver will need to implemented and loaded
to complete the pipe as Xilinx Video driver will need it.

Could you please share the reason to have this input port in the SDI Rx driver?

> > +    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
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - xlnx,line-rate
> > +  - xlnx,bpp
> > +  - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    uhdsdirxss: v-smpte-uhdsdi-rxss@80000000 {
> > +      compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0";
> > +      interrupt-parent = <&gic>;
> > +      interrupts = <0 89 4>;
> > +      reg = <0x0 0x80000000 0x0 0x10000>;
> > +      xlnx,include-edh;
> > +      xlnx,line-rate = "12G_SDI_8DS";
> > +      clocks = <&clk_1>, <&si570_1>, <&clk_2>;
> > +      clock-names = "s_axi_aclk", "sdi_rx_clk", "video_out_clk";
> > +      xlnx,bpp = <10>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        port@0 {
> > +          reg = <0>;
> > +          sdirx_out: endpoint {
> > +            remote-endpoint = <&vcap_sdirx_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> 
> --
> Regards,
> 
> Laurent Pinchart

Regards
Vishal Sagar