[v3,2/2] ARM: dts: tegra20: Add video decoder node

Message ID f58be69f6004393711c9ff3cb4b52aed33e2611a.1507752381.git.digetx@gmail.com
State New
Headers show
Series
  • NVIDIA Tegra20 video decoder driver
Related show

Commit Message

Dmitry Osipenko Oct. 11, 2017, 8:08 p.m.
Add a device node for the video decoder engine found on Tegra20.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Vladimir Zapolskiy Oct. 12, 2017, 7:43 a.m. | #1
Hello Dmitry,

On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
> Add a device node for the video decoder engine found on Tegra20.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 7c85f97f72ea..1b5d54b6c0cb 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -249,6 +249,23 @@
>  		*/
>  	};
>  
> +	vde@6001a000 {
> +		compatible = "nvidia,tegra20-vde";
> +		reg = <0x6001a000 0x3D00    /* VDE registers */
> +		       0x40000400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it
may lead to conflicts for IRAM resource between users.

My proposal is to add a valid device tree node to describe an IRAM region
firstly, then reserve a subregion in it by using a new "iram" property.

----8<----
From: Vladimir Zapolskiy <vz@mleia.com>
Date: Thu, 12 Oct 2017 10:25:45 +0300
Subject: [PATCH] ARM: tegra: add device tree node to describe IRAM on Tegra20

All Tegra20 SoCs contain 256KiB IRAM, which is used to store
resume code and by a video decoder engine.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 7c85f97f72ea..fd2843c90920 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -9,6 +9,14 @@
 	compatible = "nvidia,tegra20";
 	interrupt-parent = <&lic>;
 
+	iram@40000000 {
+		compatible = "mmio-sram";
+		reg = <0x40000000 0x40000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x40000000 0x40000>;
+	};
+
 	host1x@50000000 {
 		compatible = "nvidia,tegra20-host1x", "simple-bus";
 		reg = <0x50000000 0x00024000>;
----8<----

Please add the change above to your next version of the series, or
if you wish I can send it separately for review by Thierry.

After applying that change you do define a region in IRAM for the exclusive
usage by a video decoder engine and add an 'iram' property:

----8<----
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index fd2843c90920..5133fbac2185 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -15,6 +15,11 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0 0x40000000 0x40000>;
+
+		vde_pool: vde {
+			reg = <0x400 0x3fc00>;
+			pool;
+		};
 	};
 
 	host1x@50000000 {
[snip]

+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3d00>;	/* VDE registers */
+		iram = <&vde_pool>;		/* IRAM region */
[snip]
----8<----

And finally in the driver you'll use genalloc API to access the IRAM
region, for that you can find ready examples in the kernel source code.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter Oct. 12, 2017, 8:49 a.m. | #2
On 11/10/17 21:08, Dmitry Osipenko wrote:
> Add a device node for the video decoder engine found on Tegra20.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 7c85f97f72ea..1b5d54b6c0cb 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -249,6 +249,23 @@
>  		*/
>  	};
>  
> +	vde@6001a000 {
> +		compatible = "nvidia,tegra20-vde";
> +		reg = <0x6001a000 0x3D00    /* VDE registers */
> +		       0x40000400 0x3FC00>; /* IRAM region */
> +		reg-names = "regs", "iram";
> +		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
> +			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
> +			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
> +			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
> +			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
> +		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
> +		clocks = <&tegra_car TEGRA20_CLK_VDE>;
> +		clock-names = "vde";
> +		resets = <&tegra_car 61>;
> +		reset-names = "vde";
> +	};
> +

I don't see any binding documentation for this node. We need to make
sure we add this.

Jon
Dmitry Osipenko Oct. 12, 2017, 10:51 a.m. | #3
On 12.10.2017 11:49, Jon Hunter wrote:
> 
> On 11/10/17 21:08, Dmitry Osipenko wrote:
>> Add a device node for the video decoder engine found on Tegra20.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>> --- a/arch/arm/boot/dts/tegra20.dtsi
>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>> @@ -249,6 +249,23 @@
>>  		*/
>>  	};
>>  
>> +	vde@6001a000 {
>> +		compatible = "nvidia,tegra20-vde";
>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>> +		       0x40000400 0x3FC00>; /* IRAM region */
>> +		reg-names = "regs", "iram";
>> +		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
>> +			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
>> +			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
>> +			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
>> +			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
>> +		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
>> +		clocks = <&tegra_car TEGRA20_CLK_VDE>;
>> +		clock-names = "vde";
>> +		resets = <&tegra_car 61>;
>> +		reset-names = "vde";
>> +	};
>> +
> 
> I don't see any binding documentation for this node. We need to make
> sure we add this.
> 

It's in the first patch.

+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter Oct. 12, 2017, 10:57 a.m. | #4
On 12/10/17 11:51, Dmitry Osipenko wrote:
> On 12.10.2017 11:49, Jon Hunter wrote:
>>
>> On 11/10/17 21:08, Dmitry Osipenko wrote:
>>> Add a device node for the video decoder engine found on Tegra20.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>>> --- a/arch/arm/boot/dts/tegra20.dtsi
>>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>>> @@ -249,6 +249,23 @@
>>>  		*/
>>>  	};
>>>  
>>> +	vde@6001a000 {
>>> +		compatible = "nvidia,tegra20-vde";
>>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>>> +		       0x40000400 0x3FC00>; /* IRAM region */
>>> +		reg-names = "regs", "iram";
>>> +		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
>>> +			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
>>> +			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
>>> +			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
>>> +			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
>>> +		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
>>> +		clocks = <&tegra_car TEGRA20_CLK_VDE>;
>>> +		clock-names = "vde";
>>> +		resets = <&tegra_car 61>;
>>> +		reset-names = "vde";
>>> +	};
>>> +
>>
>> I don't see any binding documentation for this node. We need to make
>> sure we add this.
>>
> 
> It's in the first patch.
> 
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
>

Ah yes indeed, then that needs to be a separate patch.

Cheers
Jon
Dmitry Osipenko Oct. 12, 2017, 11:11 a.m. | #5
On 12.10.2017 13:57, Jon Hunter wrote:
> 
> On 12/10/17 11:51, Dmitry Osipenko wrote:
>> On 12.10.2017 11:49, Jon Hunter wrote:
>>>
>>> On 11/10/17 21:08, Dmitry Osipenko wrote:
>>>> Add a device node for the video decoder engine found on Tegra20.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>>>> --- a/arch/arm/boot/dts/tegra20.dtsi
>>>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>>>> @@ -249,6 +249,23 @@
>>>>  		*/
>>>>  	};
>>>>  
>>>> +	vde@6001a000 {
>>>> +		compatible = "nvidia,tegra20-vde";
>>>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>>>> +		       0x40000400 0x3FC00>; /* IRAM region */
>>>> +		reg-names = "regs", "iram";
>>>> +		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
>>>> +			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
>>>> +			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
>>>> +			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
>>>> +			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
>>>> +		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
>>>> +		clocks = <&tegra_car TEGRA20_CLK_VDE>;
>>>> +		clock-names = "vde";
>>>> +		resets = <&tegra_car 61>;
>>>> +		reset-names = "vde";
>>>> +	};
>>>> +
>>>
>>> I don't see any binding documentation for this node. We need to make
>>> sure we add this.
>>>
>>
>> It's in the first patch.
>>
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
>>
> 
> Ah yes indeed, then that needs to be a separate patch.
> 

Okay
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Osipenko Oct. 12, 2017, 12:06 p.m. | #6
Hello Vladimir,

On 12.10.2017 10:43, Vladimir Zapolskiy wrote:
> Hello Dmitry,
> 
> On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
>> Add a device node for the video decoder engine found on Tegra20.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>> --- a/arch/arm/boot/dts/tegra20.dtsi
>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>> @@ -249,6 +249,23 @@
>>  		*/
>>  	};
>>  
>> +	vde@6001a000 {
>> +		compatible = "nvidia,tegra20-vde";
>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>> +		       0x40000400 0x3FC00>; /* IRAM region */
> 
> this notation of a used region in IRAM is non-standard and potentially it
> may lead to conflicts for IRAM resource between users.
> 
> My proposal is to add a valid device tree node to describe an IRAM region
> firstly, then reserve a subregion in it by using a new "iram" property.
> 

The defined in DT IRAM region used by VDE isn't exactly correct, actually it
should be much smaller. I don't know exactly what parts of IRAM VDE uses, for
now it is just safer to assign the rest of the IRAM region to VDE.

I'm not sure whether it really worthy to use a dynamic allocator for a single
static allocation, but maybe it would come handy later.. Stephen / Jon /
Thierry, what do you think?

> ----8<----
> From: Vladimir Zapolskiy <vz@mleia.com>
> Date: Thu, 12 Oct 2017 10:25:45 +0300
> Subject: [PATCH] ARM: tegra: add device tree node to describe IRAM on Tegra20
> 
> All Tegra20 SoCs contain 256KiB IRAM, which is used to store
> resume code and by a video decoder engine.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 7c85f97f72ea..fd2843c90920 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -9,6 +9,14 @@
>  	compatible = "nvidia,tegra20";
>  	interrupt-parent = <&lic>;
>  
> +	iram@40000000 {
> +		compatible = "mmio-sram";
> +		reg = <0x40000000 0x40000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x40000000 0x40000>;
> +	};
> +
>  	host1x@50000000 {
>  		compatible = "nvidia,tegra20-host1x", "simple-bus";
>  		reg = <0x50000000 0x00024000>;
> ----8<----
> 
> Please add the change above to your next version of the series, or
> if you wish I can send it separately for review by Thierry.
> 
> After applying that change you do define a region in IRAM for the exclusive
> usage by a video decoder engine and add an 'iram' property:
> 

Newer Tegra generations also have the IRAM, so I think Tegra30/114/124 DT's
should also include the same IRAM node for consistency. I'll extend your patch
to cover other Tegra's and include it in v4 if you don't mind and if Stephen /
Jon / Thierry would approve your proposal.

> ----8<----
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index fd2843c90920..5133fbac2185 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -15,6 +15,11 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  		ranges = <0 0x40000000 0x40000>;
> +
> +		vde_pool: vde {
> +			reg = <0x400 0x3fc00>;
> +			pool;
> +		};
>  	};
>  
>  	host1x@50000000 {
> [snip]
> 
> +	vde@6001a000 {
> +		compatible = "nvidia,tegra20-vde";
> +		reg = <0x6001a000 0x3d00>;	/* VDE registers */
> +		iram = <&vde_pool>;		/* IRAM region */
> [snip]
> ----8<----
> 
> And finally in the driver you'll use genalloc API to access the IRAM
> region, for that you can find ready examples in the kernel source code.
> 

Thank you very much for taking a look at the patch!
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 12, 2017, 1:25 p.m. | #7
On Thu, Oct 12, 2017 at 03:06:17PM +0300, Dmitry Osipenko wrote:
> Hello Vladimir,
> 
> On 12.10.2017 10:43, Vladimir Zapolskiy wrote:
> > Hello Dmitry,
> > 
> > On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
> >> Add a device node for the video decoder engine found on Tegra20.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> >> index 7c85f97f72ea..1b5d54b6c0cb 100644
> >> --- a/arch/arm/boot/dts/tegra20.dtsi
> >> +++ b/arch/arm/boot/dts/tegra20.dtsi
> >> @@ -249,6 +249,23 @@
> >>  		*/
> >>  	};
> >>  
> >> +	vde@6001a000 {
> >> +		compatible = "nvidia,tegra20-vde";
> >> +		reg = <0x6001a000 0x3D00    /* VDE registers */
> >> +		       0x40000400 0x3FC00>; /* IRAM region */
> > 
> > this notation of a used region in IRAM is non-standard and potentially it
> > may lead to conflicts for IRAM resource between users.
> > 
> > My proposal is to add a valid device tree node to describe an IRAM region
> > firstly, then reserve a subregion in it by using a new "iram" property.
> > 
> 
> The defined in DT IRAM region used by VDE isn't exactly correct, actually it
> should be much smaller. I don't know exactly what parts of IRAM VDE uses, for
> now it is just safer to assign the rest of the IRAM region to VDE.
> 
> I'm not sure whether it really worthy to use a dynamic allocator for a single
> static allocation, but maybe it would come handy later.. Stephen / Jon /
> Thierry, what do you think?

This sounds like a good idea. I agree that this currently doesn't seem
to be warranted, but consider what would happen if at some point we have
more devices requiring access to the IRAM. Spreading individual reg
properties all across the DT will make it very difficult to ensure they
don't overlap.

Presumably the mmio-sram driver will check that pool don't overlap. Or
even if it doesn't it will make it a lot easier to verify because it's
all in the same DT node and then consumers only reference it.

I like Vladimir's proposal. I also suspect that Rob may want us to stick
to a standardized way referencing such external memory.

Thierry
Jon Hunter Oct. 12, 2017, 1:45 p.m. | #8
On 12/10/17 14:25, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Oct 12, 2017 at 03:06:17PM +0300, Dmitry Osipenko wrote:
>> Hello Vladimir,
>>
>> On 12.10.2017 10:43, Vladimir Zapolskiy wrote:
>>> Hello Dmitry,
>>>
>>> On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
>>>> Add a device node for the video decoder engine found on Tegra20.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>>>> --- a/arch/arm/boot/dts/tegra20.dtsi
>>>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>>>> @@ -249,6 +249,23 @@
>>>>  		*/
>>>>  	};
>>>>  
>>>> +	vde@6001a000 {
>>>> +		compatible = "nvidia,tegra20-vde";
>>>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>>>> +		       0x40000400 0x3FC00>; /* IRAM region */
>>>
>>> this notation of a used region in IRAM is non-standard and potentially it
>>> may lead to conflicts for IRAM resource between users.
>>>
>>> My proposal is to add a valid device tree node to describe an IRAM region
>>> firstly, then reserve a subregion in it by using a new "iram" property.
>>>
>>
>> The defined in DT IRAM region used by VDE isn't exactly correct, actually it
>> should be much smaller. I don't know exactly what parts of IRAM VDE uses, for
>> now it is just safer to assign the rest of the IRAM region to VDE.
>>
>> I'm not sure whether it really worthy to use a dynamic allocator for a single
>> static allocation, but maybe it would come handy later.. Stephen / Jon /
>> Thierry, what do you think?
> 
> This sounds like a good idea. I agree that this currently doesn't seem
> to be warranted, but consider what would happen if at some point we have
> more devices requiring access to the IRAM. Spreading individual reg
> properties all across the DT will make it very difficult to ensure they
> don't overlap.
> 
> Presumably the mmio-sram driver will check that pool don't overlap. Or
> even if it doesn't it will make it a lot easier to verify because it's
> all in the same DT node and then consumers only reference it.
> 
> I like Vladimir's proposal. I also suspect that Rob may want us to stick
> to a standardized way referencing such external memory.

FWIW I agree. Seems like a nice approach and describes the h/w accurately.

Cheers
Jon
Dmitry Osipenko Oct. 12, 2017, 3:43 p.m. | #9
On 12.10.2017 16:45, Jon Hunter wrote:
> 
> On 12/10/17 14:25, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Thu, Oct 12, 2017 at 03:06:17PM +0300, Dmitry Osipenko wrote:
>>> Hello Vladimir,
>>>
>>> On 12.10.2017 10:43, Vladimir Zapolskiy wrote:
>>>> Hello Dmitry,
>>>>
>>>> On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
>>>>> Add a device node for the video decoder engine found on Tegra20.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
>>>>>  1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>>>> index 7c85f97f72ea..1b5d54b6c0cb 100644
>>>>> --- a/arch/arm/boot/dts/tegra20.dtsi
>>>>> +++ b/arch/arm/boot/dts/tegra20.dtsi
>>>>> @@ -249,6 +249,23 @@
>>>>>  		*/
>>>>>  	};
>>>>>  
>>>>> +	vde@6001a000 {
>>>>> +		compatible = "nvidia,tegra20-vde";
>>>>> +		reg = <0x6001a000 0x3D00    /* VDE registers */
>>>>> +		       0x40000400 0x3FC00>; /* IRAM region */
>>>>
>>>> this notation of a used region in IRAM is non-standard and potentially it
>>>> may lead to conflicts for IRAM resource between users.
>>>>
>>>> My proposal is to add a valid device tree node to describe an IRAM region
>>>> firstly, then reserve a subregion in it by using a new "iram" property.
>>>>
>>>
>>> The defined in DT IRAM region used by VDE isn't exactly correct, actually it
>>> should be much smaller. I don't know exactly what parts of IRAM VDE uses, for
>>> now it is just safer to assign the rest of the IRAM region to VDE.
>>>
>>> I'm not sure whether it really worthy to use a dynamic allocator for a single
>>> static allocation, but maybe it would come handy later.. Stephen / Jon /
>>> Thierry, what do you think?
>>
>> This sounds like a good idea. I agree that this currently doesn't seem
>> to be warranted, but consider what would happen if at some point we have
>> more devices requiring access to the IRAM. Spreading individual reg
>> properties all across the DT will make it very difficult to ensure they
>> don't overlap.
>>
>> Presumably the mmio-sram driver will check that pool don't overlap. Or
>> even if it doesn't it will make it a lot easier to verify because it's
>> all in the same DT node and then consumers only reference it.
>>
>> I like Vladimir's proposal. I also suspect that Rob may want us to stick
>> to a standardized way referencing such external memory.
> 
> FWIW I agree. Seems like a nice approach and describes the h/w accurately.
> 

Alright :)
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 7c85f97f72ea..1b5d54b6c0cb 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -249,6 +249,23 @@ 
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */
+		reg-names = "regs", "iram";
+		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
+			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
+			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
+			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
+		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
+		clocks = <&tegra_car TEGRA20_CLK_VDE>;
+		clock-names = "vde";
+		resets = <&tegra_car 61>;
+		reset-names = "vde";
+	};
+
 	apbmisc@70000800 {
 		compatible = "nvidia,tegra20-apbmisc";
 		reg = <0x70000800 0x64   /* Chip revision */