RE: [PATCH v11 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface

Hello Laurent,

Thank you for your review.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Tuesday, July 23, 2024 12:21 AM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]
Cc: Hans Verkuil <redacted>; Mauro Carvalho Chehab
[off-list ref]; Rob Herring [off-list ref]; Krzysztof Kozlowski
[off-list ref]; Conor Dooley [off-list ref]; Mark Brown
[off-list ref]; Sakari Ailus [off-list ref]; iwamatsu
nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ)
[off-list ref]; linux-media@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; Krzysztof Kozlowski
[off-list ref]
Subject: Re: [PATCH v11 1/6] dt-bindings: media: platform: visconti: Add
Toshiba Visconti Video Input Interface

Hello Ishikawa-san,

Thank you for the patch.

On Tue, Jul 09, 2024 at 09:08:43AM +0900, Yuji Ishikawa wrote:

quoted

Adds the Device Tree binding documentation that allows to describe the
Video Input Interface found in Toshiba Visconti SoCs.

Signed-off-by: Yuji Ishikawa <redacted>
Reviewed-by: Nobuhiro Iwamatsu <redacted>
Reviewed-by: Krzysztof Kozlowski <redacted>
---
Changelog v2:
- no change

Changelog v3:
- no change

Changelog v4:
- fix style problems at the v3 patch
- remove "index" member
- update example

Changelog v5:
- no change

Changelog v6:
- add register definition of BUS-IF and MPU

Changelog v7:
- remove trailing "bindings" from commit header message
- remove trailing "Device Tree Bindings" from title
- fix text wrapping of description
- change compatible to visconti5-viif
- explicitly define allowed properties for port::endpoint

Changelog v8:
- Suggestion from Krzysztof Kozlowski
  - rename bindings description file
  - use block style array instead of inline style
  - remove clock-lane (as it is fixed at position 0)
  - update sample node's name
  - use lowercase hex for literals
- Suggestion from Laurent Pinchart
  - update description message port::description
  - remove port::endpoint::bus-type as it is fixed to <4>
  - remove port::endpoint::clock-lanes from example
  - add port::endpoint::data-lanes to required parameters list
  - fix sequence of data-lanes: <1 2 3 4> because current driver does not

support data reordering

quoted

  - update port::endpoint::data-lanes::description
  - remove redundant type definition for port::endpoint::data-lanes

Changelog v9:
- place "required" after "properties"
- dictionary ordering of properties

Changelog v10:
- no change

Changelog v11:
- no change

 .../media/toshiba,visconti5-viif.yaml         | 105

++++++++++++++++++

quoted

 1 file changed, 105 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml

diff --git
a/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml
b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml
new file mode 100644
index 0000000000..97e8bda427

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.y
+++ aml

@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/toshiba,visconti5-viif.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti5 SoC Video Input Interface
+
+maintainers:
+  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+
+description: |-
+  Toshiba Visconti5 SoC Video Input Interface (VIIF) receives MIPI
+CSI2 video
+  stream, processes the stream with image signal processors (L1ISP,
+L2ISP),
+  then stores pictures to main memory.
+
+properties:
+  compatible:
+    const: toshiba,visconti5-viif
+
+  reg:
+    items:
+      - description: Registers for capture control
+      - description: Registers for CSI2 receiver control
+      - description: Registers for bus interface unit control
+      - description: Registers for Memory Protection Unit

Is this part of the VIIF, or some sort of more standalone IOMMU ? In the latter
case, should it be handled by a separate driver ?

The MPU does not have the functionality of a general purpose IOMMU as described in the Linux IOMMU framework.
It just blocks VIIF's memory accesses to the specified address range.
Because it is configured to block all the accesses by default, I have to set it to by-pass mode at every power cycle.

quoted

+
+  interrupts:
+    items:
+      - description: Sync Interrupt
+      - description: Status (Error) Interrupt
+      - description: CSI2 Receiver Interrupt
+      - description: L1ISP Interrupt
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+    description: CSI-2 input port, with a single endpoint connected to the

CSI-2 transmitter.

quoted

+
+    properties:
+      endpoint:
+        $ref: video-interfaces.yaml#
+        additionalProperties: false
+
+        properties:
+          data-lanes:
+            description: VIIF supports 1, 2, 3 or 4 data lanes
+            minItems: 1
+            items:
+              - const: 1
+              - const: 2
+              - const: 3
+              - const: 4
+
+          clock-noncontinuous: true
+          link-frequencies: true
+          remote-endpoint: true
+
+        required:
+          - clock-noncontinuous

Does the hardware support the non-continuous clock mode only, or is it
configurable ? This is a boolean property, so if both mode are supported, the
property should not be required.

The hardware supports both clock mode and switches them automatically.
Therefore, I'll drop this property.

quoted

+          - data-lanes
+          - link-frequencies
+          - remote-endpoint
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        video@1c000000 {
+            compatible = "toshiba,visconti5-viif";
+            reg = <0 0x1c000000 0 0x6000>,
+                  <0 0x1c008000 0 0x400>,
+                  <0 0x1c00e000 0 0x1000>,
+                  <0 0x2417a000 0 0x1000>;
+            interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+
+            port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                csi_in0: endpoint {
+                    clock-noncontinuous;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <456000000>;
+                    remote-endpoint = <&imx219_out0>;
+                };
+            };
+        };
+    };

--
Regards,

Laurent Pinchart

Regards,
Yuji Ishikawa