Re: [RFC PATCH v9 5/9] dt-binding: tegra: Add VI and CSI bindings

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 4/22/20 10:47 AM, Laurent Pinchart wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sowjanya,
>
> On Wed, Apr 22, 2020 at 10:26:20AM -0700, Sowjanya Komatineni wrote:
>> On 4/22/20 10:20 AM, Laurent Pinchart wrote:
>>> On Tue, Apr 21, 2020 at 11:18:55PM -0700, Sowjanya Komatineni wrote:
>>>> Tegra contains VI controller which can support up to 6 MIPI CSI
>>>> camera sensors.
>>>>
>>>> Each Tegra CSI port from CSI unit can be one-to-one mapper to
>>>> VI channel and can capture from an external camera sensor or
>>>> from built-in test pattern generator.
>>>>
>>>> This patch adds dt-bindings for Tegra VI and CSI.
>>>>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>    .../display/tegra/nvidia,tegra20-host1x.txt        | 73 ++++++++++++++++++----
>>>>    1 file changed, 60 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
>>>> index 9999255..4731921 100644
>>>> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
>>>> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
>>>> @@ -40,14 +40,30 @@ of the following host1x client modules:
>>>>
>>>>      Required properties:
>>>>      - compatible: "nvidia,tegra<chip>-vi"
>>>> -  - reg: Physical base address and length of the controller's registers.
>>>> +  - reg: Physical base address and length of the controller registers.
>>>>      - interrupts: The interrupt outputs from the controller.
>>>> -  - clocks: Must contain one entry, for the module clock.
>>>> +  - clocks: clocks: Must contain one entry, for the module clock.
>>>>        See ../clocks/clock-bindings.txt for details.
>>>> -  - resets: Must contain an entry for each entry in reset-names.
>>>> -    See ../reset/reset.txt for details.
>>>> -  - reset-names: Must include the following entries:
>>>> -    - vi
>>>> +  - Tegra20/Tegra30/Tegra114/Tegra124:
>>>> +    - resets: Must contain an entry for each entry in reset-names.
>>>> +      See ../reset/reset.txt for details.
>>>> +    - reset-names: Must include the following entries:
>>>> +      - vi
>>>> +  - Tegra210:
>>>> +    - power-domains: Must include venc powergate node as vi is in VE partition.
>>>> +  - Tegra210 has CSI part of VI sharing same host interface and register space.
>>>> +    So, VI device node should have CSI child node.
>>>> +
>>>> +    - csi: mipi csi interface to vi
>>>> +
>>>> +      Required properties:
>>>> +      - compatible: "nvidia,tegra210-csi"
>>>> +      - reg: Physical base address offset to parent and length of the controller
>>>> +        registers.
>>>> +      - clocks: Must contain entries csi, cilab, cilcd, cile, csi_tpg clocks.
>>>> +        See ../clocks/clock-bindings.txt for details.
>>>> +      - power-domains: Must include sor powergate node as csicil is in
>>>> +        SOR partition.
>>> A bit of a stupid question maybe, but why is this needed ? Can't the
>>> driver that handles the vi DT node ("nvidia,tegra20-vi") handle all the
>>> registers for all the sub-blocks ? Can't we move the clocks and power
>>> domains from the CSI node to the VI node ?
>> CSI is separate device driver and VI is separate device driver.
> That's fine, but that's a software design decision, it should not affect
> the DT bindings. It's possible (even if I don't necessarily recommend
> doing so) to create a platform device manually in the VI driver to get
> it bound to the CSI driver. In any case DT should describe the system
> architecture and shouldn't be influenced by hardware design.

CSI is a separate device for Tegra186 onwards and VI/CSI is common driver.

DT bindings reflects the chip design. Also clocks and power domain for 
CSI and VI are different and CSI is separate device.

>
>> For T210, CSI shares register space under VI but for later Tegras its
>> separate register space.
> This is useful information. How about interrupts though ? You don't
> specify any interrupt line here, how are the CSI interrutps handled ?

All frame events are through syncpoint HW like mentioned in earlier 
feedbacks.

This driver synchronizes capture events through host1x syncpt

>
>> So CSI and VI drivers are separate with their corresponding clocks and
>> power domains in their nodes.
>>
>>> Regardless of the answer to this question, I think this is missing port
>>> nodes for the physical CSI-2 inputs, to connect them to sensors. I
>>> haven't seen anywhere in this series how a CSI-2 sensor is linked to the
>>> VI.
>> This patch series is only for Tegra internal TPG and tegra video driver
>> creates hard media links between CSI and VI,
> Could you share the output of media-ctl --print-dot to show how this
> looks like ?


digraph board {
rankdir=TB
         n00000001 [label="{{} | tpg-0\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000001:port0 -> n00000003
         n00000003 [label="54080000.vi-output-0\n/dev/video0", 
shape=box, style=filled, fillcolor=yellow]
         n00000009 [label="{{} | tpg-1\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000009:port0 -> n0000000b
         n0000000b [label="54080000.vi-output-1\n/dev/video1", 
shape=box, style=filled, fillcolor=yellow]
         n00000011 [label="{{} | tpg-2\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000011:port0 -> n00000013
         n00000013 [label="54080000.vi-output-2\n/dev/video2", 
shape=box, style=filled, fillcolor=yellow]
         n00000019 [label="{{} | tpg-3\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000019:port0 -> n0000001b
         n0000001b [label="54080000.vi-output-3\n/dev/video3", 
shape=box, style=filled, fillcolor=yellow]
         n00000021 [label="{{} | tpg-4\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000021:port0 -> n00000023
         n00000023 [label="54080000.vi-output-4\n/dev/video4", 
shape=box, style=filled, fillcolor=yellow]
         n00000029 [label="{{} | tpg-5\n | {<port0> 0}}", shape=Mrecord, 
style=filled, fillcolor=green]
         n00000029:port0 -> n0000002b
         n0000002b [label="54080000.vi-output-5\n/dev/video5", 
shape=box, style=filled, fillcolor=yellow]
}


>
>> Sensor support will be in Series-2 where port nodes will be used for
>> real sensor <-> csi <-> vi endpoints
>>
>>>>    - epp: encoder pre-processor
>>>>
>>>> @@ -309,13 +325,44 @@ Example:
>>>>                         reset-names = "mpe";
>>>>                 };
>>>>
>>>> -             vi {
>>>> -                     compatible = "nvidia,tegra20-vi";
>>>> -                     reg = <0x54080000 0x00040000>;
>>>> -                     interrupts = <0 69 0x04>;
>>>> -                     clocks = <&tegra_car TEGRA20_CLK_VI>;
>>>> -                     resets = <&tegra_car 100>;
>>>> -                     reset-names = "vi";
>>>> +             vi@54080000 {
>>>> +                     compatible = "nvidia,tegra210-vi";
>>>> +                     reg = <0x0 0x54080000 0x0 0x700>;
>>>> +                     interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                     assigned-clocks = <&tegra_car TEGRA210_CLK_VI>;
>>>> +                     assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
>>>> +
>>>> +                     clocks = <&tegra_car TEGRA210_CLK_VI>;
>>>> +                     power-domains = <&pd_venc>;
>>>> +
>>>> +                     #address-cells = <1>;
>>>> +                     #size-cells = <1>;
>>>> +
>>>> +                     ranges = <0x0 0x0 0x54080000 0x2000>;
>>>> +
>>>> +                     csi@838 {
>>>> +                             compatible = "nvidia,tegra210-csi";
>>>> +                             reg = <0x838 0x1300>;
>>>> +                             assigned-clocks = <&tegra_car TEGRA210_CLK_CILAB>,
>>>> +                                               <&tegra_car TEGRA210_CLK_CILCD>,
>>>> +                                               <&tegra_car TEGRA210_CLK_CILE>,
>>>> +                                               <&tegra_car TEGRA210_CLK_CSI_TPG>;
>>>> +                             assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_P>,
>>>> +                                                      <&tegra_car TEGRA210_CLK_PLL_P>,
>>>> +                                                      <&tegra_car TEGRA210_CLK_PLL_P>;
>>>> +                             assigned-clock-rates = <102000000>,
>>>> +                                                    <102000000>,
>>>> +                                                    <102000000>,
>>>> +                                                    <972000000>;
>>>> +
>>>> +                             clocks = <&tegra_car TEGRA210_CLK_CSI>,
>>>> +                                      <&tegra_car TEGRA210_CLK_CILAB>,
>>>> +                                      <&tegra_car TEGRA210_CLK_CILCD>,
>>>> +                                      <&tegra_car TEGRA210_CLK_CILE>,
>>>> +                                      <&tegra_car TEGRA210_CLK_CSI_TPG>;
>>>> +                             clock-names = "csi", "cilab", "cilcd", "cile", "csi_tpg";
>>>> +                             power-domains = <&pd_sor>;
>>>> +                     };
>>>>                 };
>>>>
>>>>                 epp {
> --
> Regards,
>
> Laurent Pinchart