| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7d183747-af9e-4607-8219-e89ddcb79654@ti.com>
Date: Mon, 13 Oct 2025 11:51:06 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <mchehab@...nel.org>,
<robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<hverkuil+cisco@...nel.org>
CC: <sakari.ailus@...ux.intel.com>, <bparrot@...com>,
<jai.luthra@...asonboard.com>, <dale@...nsworth.org>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <u-kumar1@...com>,
Sukrut Bellary
<sbellary@...libre.com>
Subject: Re: [PATCH V3 4/5] dt-bindings: media: ti: vpe: Add support for Video
Input Port
Hi Krzysztof,
Apologies for the delay in response.
On 09/09/25 14:06, Krzysztof Kozlowski wrote:
> On 09/09/2025 10:07, Yemike Abhilash Chandra wrote:
>> From: Dale Farnsworth <dale@...nsworth.org>
>>
>> Add device tree bindings for the Video Input Port. Video Input Port (VIP)
>> can be found on devices such as DRA7xx and provides a parallel interface
>> to a video source such as a sensor or TV decoder.
>>
>> Signed-off-by: Dale Farnsworth <dale@...nsworth.org>
>> Signed-off-by: Benoit Parrot <bparrot@...com>
>> Signed-off-by: Sukrut Bellary <sbellary@...libre.com>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
>> ---
>> Change log:
>> Changes in v3:
>> - Remove redundant labels in bindings
>> - Remove minItems in interrupts and mandate exactly 2 interrupts using items
>> - Rename phandle from ti,vip-clk-polarity to ti,ctrl-module and explain why this is required by the device
>> - Make the phandle verifiable instead of just descriptive
>> - Drop entire sensor node from example DT
>> - Fix ports hierarchy using appropriate references and descriptions
>> - Use generic node names
>> - Add two new properties ti,vip-pixel-mux and ti,vip-channels with appropriate types and descriptions
>>
>> .../devicetree/bindings/media/ti,vip.yaml | 178 ++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 179 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/ti,vip.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/ti,vip.yaml b/Documentation/devicetree/bindings/media/ti,vip.yaml
>> new file mode 100644
>> index 000000000000..c0bce44725db
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,vip.yaml
>> @@ -0,0 +1,178 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2025 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/ti,vip.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments DRA7x VIDEO INPUT PORT (VIP).
>
> Titles are not sentences.
>
> Also, drop CAPS, WE DO NOT NEED TO EMPHASIZE THAT. It is a Video Input Port.
>
I will fix this in v4.
>
>> +
>> +maintainers:
>> + - Yemike Abhilash Chandra <y-abhilashchandra@...com>
>> +
>> +description: |-
>> + Video Input Port (VIP) can be found on devices such as DRA7xx and
>> + provides the system interface and the processing capability to
>> + connect parallel image-sensor as well as BT.656/1120 capable encoder
>> + chip to DRA7x device.
>> +
>> + Each VIP instance supports 2 independently configurable external
>> + video input capture slices (Slice 0 and Slice 1) each providing
>> + up to two video input ports (Port A and Port B).
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - ti,dra7-vip
>> +
>> + reg:
>> + items:
>> + - description: The VIP main register region
>> + - description: Video Data Parser (PARSER) register region for Slice0
>> + - description: Color Space Conversion (CSC) register region for Slice0
>> + - description: Scaler (SC) register region for Slice0
>> + - description: Video Data Parser (PARSER) register region for Slice1
>> + - description: Color Space Conversion (CSC) register region for Slice1
>> + - description: Scaler (SC) register region for Slice1
>> + - description: Video Port Direct Memory Access (VPDMA) register region
>> +
>> + reg-names:
>> + items:
>> + - const: vip
>> + - const: parser0
>> + - const: csc0
>> + - const: sc0
>> + - const: parser1
>> + - const: csc1
>> + - const: sc1
>> + - const: vpdma
>> +
>> + interrupts:
>> + items:
>> + - description: IRQ index 0 is used for Slice0 interrupts
>> + - description: IRQ index 1 is used for Slice1 interrupts
>> +
>> + ti,ctrl-module:
>> + description:
>> + Reference to the device control module that provides clock-edge
>> + inversion control for VIP ports. These controls allow the
>> + VIP to sample pixel data on the correct clock edge.
>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>> + items:
>> + items:
>> + - description: phandle to device control module
>> + - description: offset to the CTRL_CORE_SMA_SW_1 register
>> + - description: Bit field to slice 0 port A
>> + - description: Bit field to slice 0 port B
>> + - description: Bit field to slice 1 port A
>> + - description: Bit field to slice 1 port B
>> + maxItems: 1
>> +
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + patternProperties:
>> + '^port@[0-3]$':
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + unevaluatedProperties: false
>> + description: |
>> + Each VIP instance supports 2 independently configurable external video
>> + input capture slices (Slice 0 and Slice 1) each providing up to two video
>> + input ports (Port A and Port B). These ports represent the following
>> + port@0 -> Slice 0 Port A
>> + port@1 -> Slice 0 Port B
>> + port@2 -> Slice 1 Port A
>> + port@3 -> Slice 1 Port B
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + bus-width:
>> + enum: [8, 16, 24]
>> + default: 8
>> +
>> + ti,vip-pixel-mux:
>> + type: boolean
>> + description:
>> + In BT656/1120 mode, this will enable pixel-muxing if
>> + the number of channels is either 1, 2 or 4. If this
>> + property is present then pixel-muxing is enabled
>> + otherwise it will use line-muxing.
>
> This feels like runtime choice.
>
>> +
>> + ti,vip-channels:
>> + $ref: /schemas/types.yaml#/definitions/uint8-array
>> + minItems: 1
>> + maxItems: 16
>> + description:
>> + In BT656/1120 mode, list of channel ids to be captured.
>> + If the property is not present then 1 channel is assumed.
>
> Also feels like runtime.
>
I will drop these in v4.
>> +
>> + remote-endpoint: true
>
> Drop
>
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reg-names
>> + - interrupts
>> + - ti,ctrl-module
>> + - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + vip1: video@...70000 {
>> + compatible = "ti,dra7-vip";
>> + reg = <0x48970000 0x114>,
>> + <0x48975500 0xD8>,
>> + <0x48975700 0x18>,
>> + <0x48975800 0x80>,
>> + <0x48975a00 0xD8>,
>> + <0x48975c00 0x18>,
>> + <0x48975d00 0x80>,
>
> Are you really, REALLY sure these are separate address spaces? Some
> people thing that gaps means address space finishes...
>
>
> How does your datasheet define these?
>
These are not separate address spaces. The datasheet defines them as a
single address region: VIP1 (0x4897_0000 –> 0x4897_FFFF, 64 KiB).
We created three common libraries sc.c (scalar), csc.c (color space
converter),
and vpdma.c, which are used by both VPE and VIP drivers. The helper
functions
in these libraries were originally written in a way that assumes sc0, csc0,
sc1, and csc1 are separate address spaces. Since VPE has already been
upstreamed using this approach. I have tried to use the same kind of
approach.
But I now understand that, this might not be correct to define these as
separate
address spaces.
One solution would be to rewrite these helpers and update both VPE and
VIP accordingly, but changing these helpers now may risk breaking backward
compatibility for VPE.
Alternatively, we could make the VIP driver standalone by avoiding the
use of these
helpers. I was able to achieve this for csc.c and sc.c, but for vpdma.c,
I had to
export a specific function from vpdma.c, since the VIP driver no longer
relies on the
helpers provided by vpdma.c (In the previous approach the helper would
call this function)
I request suggestions on how to proceed here.
>> + <0x4897d000 0x400>;
>> + reg-names = "vip",
>> + "parser0",
>> + "csc0",
>> + "sc0",
>> + "parser1",
>> + "csc1",
>> + "sc1",
>> + "vpdma";
>> + interrupts = <GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 392 IRQ_TYPE_LEVEL_HIGH>;
>> + ti,ctrl-module = <&scm_conf 0x534 0x0 0x2 0x1 0x3>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + vin1a: port@0 {
>> + reg = <0>;
>> +
>> + vin1a_ep: endpoint {
>> + remote-endpoint = <&camera1>;
>> + hsync-active = <1>;
>> + vsync-active = <1>;
>> + pclk-sample = <0>;
>> + bus-width = <8>;
>
> Make example complete, also other properties.
>
>> + };
>> + };
>> + vin1b: port@1 {
>> + reg = <1>;
>
> Why no endpoints?
>
Thanks, I will add endpoints and make example complete.
>> + };
>> + vin2a: port@2 {
>> + reg = <2>;
>> + };
>> + vin2b: port@3 {
>> + reg = <3>;
>
>
>
> Best regards,
> Krzysztof
Thanks and Regards,
Yemike Abhilash Chandra
Powered by blists - more mailing lists