| Message ID | 20240709000848.1108788-2-yuji2.ishikawa@toshiba.co.jp |
|---|---|
| State | New |
| Headers | show |
| Series | Add Toshiba Visconti Video Input Interface driver | expand |
Hello Ishikawa-san, Thank you for the patch. On Tue, Jul 09, 2024 at 09:08:43AM +0900, Yuji Ishikawa wrote: > Adds the Device Tree binding documentation that allows to describe > the Video Input Interface found in Toshiba Visconti SoCs. > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > 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 > - 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 ++++++++++++++++++ > 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.yaml > @@ -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 ? > + > + 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. > + > + 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. > + - 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>; > + }; > + }; > + }; > + };
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(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp> > Cc: Hans Verkuil <hverkuil@xs4all.nl>; Mauro Carvalho Chehab > <mchehab@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mark Brown > <broonie@kernel.org>; Sakari Ailus <sakari.ailus@linux.intel.com>; iwamatsu > nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp>; linux-media@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> > 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: > > Adds the Device Tree binding documentation that allows to describe the > > Video Input Interface found in Toshiba Visconti SoCs. > > > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > 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 > > - 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 > ++++++++++++++++++ > > 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. > > + > > + 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. > > + > > + 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. > > + - 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
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.yaml @@ -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 + + 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. + + 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 + - 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>; + }; + }; + }; + };