Re: [PATCH 2/3] ARM64: add tegra-vi support in T210 device-tree

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

Hi Bryan,

This patchset really needs to be Cc'ed to linux-tegra@vger.kernel.org,
it's becoming impossible to track it otherwise.

On Mon, Sep 21, 2015 at 11:55:54AM -0700, Bryan Wu wrote:
[...]
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index 1168bcd..3f6501f 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -109,9 +109,181 @@
>  
>  		vi@0,54080000 {
>  			compatible = "nvidia,tegra210-vi";
> -			reg = <0x0 0x54080000 0x0 0x00040000>;
> +			reg = <0x0 0x54080000 0x0 0x800>;

This now no longer matches the address map in the TRM.

>  			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
>  			status = "disabled";
> +			clocks = <&tegra_car TEGRA210_CLK_VI>,
> +				 <&tegra_car TEGRA210_CLK_CSI>,
> +				 <&tegra_car TEGRA210_CLK_PLL_C>;
> +			clock-names = "vi", "csi", "parent";
> +			resets = <&tegra_car 20>;
> +			reset-names = "vi";
> +
> +			power-domains = <&pmc TEGRA_POWERGATE_VENC>;

As an aside, this will currently prevent the driver from probing because
the generic power domain code will return -EPROBE_DEFER if this property
is encountered but the corresponding driver (for the PMC) hasn't
registered any power domains yet. So until the PMC driver changes have
been merged we can't add these power-domains properties.

That's not something for you to worry about, though. I'll make sure to
strip this out if it happens to get merged before the PMC driver changes
and add it back it afterwards.

> +
> +			iommus = <&mc TEGRA_SWGROUP_VI>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					vi_in0: endpoint {
> +						remote-endpoint = <&csi_out0>;
> +					};
> +				};
> +				port@1 {
> +					reg = <1>;
> +
> +					vi_in1: endpoint {
> +						remote-endpoint = <&csi_out1>;
> +					};
> +				};
> +				port@2 {
> +					reg = <2>;
> +
> +					vi_in2: endpoint {
> +						remote-endpoint = <&csi_out2>;
> +					};
> +				};
> +				port@3 {
> +					reg = <3>;
> +
> +					vi_in3: endpoint {
> +						remote-endpoint = <&csi_out3>;
> +					};
> +				};
> +				port@4 {
> +					reg = <4>;
> +
> +					vi_in4: endpoint {
> +						remote-endpoint = <&csi_out4>;
> +					};
> +				};
> +				port@5 {
> +					reg = <5>;
> +
> +					vi_in5: endpoint {
> +						remote-endpoint = <&csi_out5>;
> +					};
> +				};
> +
> +			};
> +		};
> +
> +		csi@0,54080838 {

I think this and its siblings should be children of the vi node. They
are within the same memory aperture according to the TRM and the fact
that the VI needs to reference the "CSI" clock indicates that the
coupling is tighter than this current DT layout makes it out to be.

> +			compatible = "nvidia,tegra210-csi";
> +			reg = <0x0 0x54080838 0x0 0x700>;

Some of the internal register documentation indicates that the register
range actually starts at an offset of 0x800, it just so happens that we
don't use any of the registers from 0x800 to 0x837. So I think this
needs to be adapted.

> +			clocks = <&tegra_car TEGRA210_CLK_CILAB>;
> +			clock-names = "cil";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					csi_in0: endpoint@0 {
> +						reg = <0x0>;
> +					};
> +					csi_out0: endpoint@1 {
> +						reg = <0x1>;
> +						remote-endpoint = <&vi_in0>;
> +					};
> +				};
> +				port@1 {
> +					reg = <1>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					csi_in1: endpoint@0 {
> +						reg = <0>;
> +					};
> +					csi_out1: endpoint@1 {
> +						reg = <1>;
> +						remote-endpoint = <&vi_in1>;
> +					};
> +				};
> +			};

This, and the ports subnode of the vi node, is *a lot* of lines to
describe what's effectively a hard-coded association. Nothing in here
can be configured, so I doubt that it is necessary to describe the VI
to this extent in DT.

It's quite difficult to judge because we don't have an actual use-case
yet where real sensors need to be hooked up. Do we have some internal
boards that we can use to prototype this from a DT point of view? What
we'd need is just something that has any sensor connected to one of the
CSI ports so we can see what we really need to fully describe such a
setup.

I'm reluctant to apply something like this, or the corresponding
binding, without at least having attempted to describe a real user.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]