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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0b19c544-f773-435e-9829-aaaa1c6daf7a@wolfvision.net>
Date: Mon, 24 Feb 2025 11:21:41 +0100
From: Michael Riesch <michael.riesch@...fvision.net>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Paul Kocialkowski <paulk@...-base.io>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 03/11] media: dt-bindings: media: add bindings for
 rockchip rk3568 vicap

Hi Sakari,

Thanks for the review.

On 2/21/25 15:10, Sakari Ailus wrote:
> Hi Michael,
> 
> Thanks for the update.
> 
> On Wed, Feb 19, 2025 at 11:16:34AM +0100, Michael Riesch wrote:
>> Add documentation for the Rockchip RK3568 Video Capture (VICAP) unit.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
>> ---
>>  .../bindings/media/rockchip,rk3568-vicap.yaml      | 168 +++++++++++++++++++++
>>  MAINTAINERS                                        |   1 +
>>  2 files changed, 169 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
>> new file mode 100644
>> index 000000000000..3dc15efeb32e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
>> @@ -0,0 +1,168 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/rockchip,rk3568-vicap.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip RK3568 Video Capture (VICAP)
>> +
>> +maintainers:
>> +  - Michael Riesch <michael.riesch@...fvision.net>
>> +
>> +description:
>> +  The Rockchip RK3568 Video Capture (VICAP) block features a digital video
>> +  port (DVP, a parallel video interface) and a MIPI CSI-2 port. It receives
>> +  the data from camera sensors, video decoders, or other companion ICs and
>> +  transfers it into system main memory by AXI bus.
>> +
>> +properties:
>> +  compatible:
>> +    const: rockchip,rk3568-vicap
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: ACLK
>> +      - description: HCLK
>> +      - description: DCLK
>> +      - description: ICLK
>> +
>> +  clock-names:
>> +    items:
>> +      - const: aclk
>> +      - const: hclk
>> +      - const: dclk
>> +      - const: iclk
>> +
>> +  rockchip,cif-clk-delaynum:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 127
>> +    description:
>> +      Delay the DVP path clock input to align the sampling phase, only valid
>> +      in dual edge sampling mode.
> 
> I suppose there's further documentation on this somewhere else? A reference
> would be nice.

I like your optimism :-) No, I am afraid this single sentence is all the
the RK3568 TRM has to say about it. I can add a reference to the TRM
page, but everyone who actually follows this reference will be
disappointed...

>> +
>> +  iommus:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    items:
>> +      - description: ARST
>> +      - description: HRST
>> +      - description: DRST
>> +      - description: PRST
>> +      - description: IRST
>> +
>> +  reset-names:
>> +    items:
>> +      - const: arst
>> +      - const: hrst
>> +      - const: drst
>> +      - const: prst
>> +      - const: irst
>> +
>> +  rockchip,grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to general register file used for video input block control.
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description:
>> +          The digital video port (DVP, a parallel video interface).
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              bus-type:
>> +                enum: [5, 6]
>> +
>> +            required:
>> +              - bus-type
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description: The MIPI CSI-2 port.
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: video-interfaces.yaml#
>> +            unevaluatedProperties: false
> 
> Don't you need things like data-lanes here? Or is this a single lane
> receiver?

This may be a bit confusing, and I probably should extend the
description a bit. This port/endpoint faces the MIPI CSI Host, which has
its own driver provided in patch 6. The connection in between is a link
with some internal format. Hence, no properties required.

This is the same issue as the one discussed in the other thread, since
the other end of this connection is discussed there. I'll fix the issue
on both ends using Rob's suggestion.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/rk3568-cru.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/power/rk3568-power.h>
>> +    #include <dt-bindings/media/video-interfaces.h>
>> +
>> +    parent {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        vicap: video-capture@...e0000 {
>> +            compatible = "rockchip,rk3568-vicap";
>> +            reg = <0x0 0xfdfe0000 0x0 0x200>;
>> +            interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
>> +            assigned-clocks = <&cru DCLK_VICAP>;
>> +            assigned-clock-rates = <300000000>;
>> +            clocks = <&cru ACLK_VICAP>, <&cru HCLK_VICAP>,
>> +                     <&cru DCLK_VICAP>, <&cru ICLK_VICAP_G>;
>> +            clock-names = "aclk", "hclk", "dclk", "iclk";
>> +            iommus = <&vicap_mmu>;
>> +            power-domains = <&power RK3568_PD_VI>;
>> +            resets = <&cru SRST_A_VICAP>, <&cru SRST_H_VICAP>,
>> +                     <&cru SRST_D_VICAP>, <&cru SRST_P_VICAP>,
>> +                     <&cru SRST_I_VICAP>;
>> +            reset-names = "arst", "hrst", "drst", "prst", "irst";
>> +            rockchip,grf = <&grf>;
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                vicap_dvp: port@0 {
>> +                    reg = <0>;
>> +
>> +                    vicap_dvp_input: endpoint {
>> +                        bus-type = <MEDIA_BUS_TYPE_BT656>;
>> +                        bus-width = <16>;
>> +                        pclk-sample = <MEDIA_PCLK_SAMPLE_DUAL_EDGE>;
>> +                        remote-endpoint = <&it6801_output>;
>> +                    };
>> +                };
>> +
>> +                vicap_mipi: port@1 {
>> +                    reg = <1>;
> 
> Where is the endpoint?

I'll add the endpoint in the example.

Regards,
Michael

> 
>> +                };
>> +            };
>> +        };
>> +    };
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bbfaf35d50c6..cd8fa1afe5eb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20407,6 +20407,7 @@ M:	Michael Riesch <michael.riesch@...fvision.net>
>>  L:	linux-media@...r.kernel.org
>>  S:	Maintained
>>  F:	Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
>> +F:	Documentation/devicetree/bindings/media/rockchip,rk3568-vicap.yaml
>>  
>>  ROCKCHIP CRYPTO DRIVERS
>>  M:	Corentin Labbe <clabbe@...libre.com>
>>
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ