[PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Mohan Kumar]

Add iommus property for hda and enable the node for P3737 + P3701
platform.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
 arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
index 34d6a01ee1c6..156d5d95fde7 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
@@ -1751,6 +1751,7 @@
 
 		hda@3510000 {
 			nvidia,model = "NVIDIA Jetson AGX Orin HDA";
+			status = "okay";
 		};
 	};
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index aaace605bdaa..704c50c24654 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -709,6 +709,7 @@
 			interconnects = <&mc TEGRA234_MEMORY_CLIENT_HDAR &emc>,
 					<&mc TEGRA234_MEMORY_CLIENT_HDAW &emc>;
 			interconnect-names = "dma-mem", "write";
+			iommus = <&smmu_niso0 TEGRA234_SID_HDA>;
 			status = "disabled";
 		};
 
-- 
2.17.1

Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Krzysztof Kozlowski]

On 25/03/2022 08:21, Mohan Kumar wrote:
> Add iommus property for hda and enable the node for P3737 + P3701
> platform.
> 
> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> index 34d6a01ee1c6..156d5d95fde7 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> @@ -1751,6 +1751,7 @@
>  
>  		hda@3510000 {
>  			nvidia,model = "NVIDIA Jetson AGX Orin HDA";
> +			status = "okay";

Nodes are enabled by default. Why do you need this?


Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Mohan Kumar D]

On 3/25/2022 12:56 PM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 25/03/2022 08:21, Mohan Kumar wrote:
>> Add iommus property for hda and enable the node for P3737 + P3701
>> platform.
>>
>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
>> ---
>>   arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
>>   arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> index 34d6a01ee1c6..156d5d95fde7 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> @@ -1751,6 +1751,7 @@
>>
>>                hda@3510000 {
>>                        nvidia,model = "NVIDIA Jetson AGX Orin HDA";
>> +                     status = "okay";
> Nodes are enabled by default. Why do you need this?
hda node status is set to "disabled" by default in soc dts file 
tegra234.dtsi. The enable is controlled by platform specific dts files.
>
>
> Best regards,
> Krzysztof

Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Krzysztof Kozlowski]

On 25/03/2022 10:31, Mohan Kumar D wrote:
> 
> On 3/25/2022 12:56 PM, Krzysztof Kozlowski wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 25/03/2022 08:21, Mohan Kumar wrote:
>>> Add iommus property for hda and enable the node for P3737 + P3701
>>> platform.
>>>
>>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
>>> ---
>>>   arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
>>>   arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>> index 34d6a01ee1c6..156d5d95fde7 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>> @@ -1751,6 +1751,7 @@
>>>
>>>                hda@3510000 {
>>>                        nvidia,model = "NVIDIA Jetson AGX Orin HDA";
>>> +                     status = "okay";
>> Nodes are enabled by default. Why do you need this?
> hda node status is set to "disabled" by default in soc dts file 
> tegra234.dtsi. The enable is controlled by platform specific dts files.

Oh, surprise... why do you override nodes with full path? This is
error-prone and makes any changes (like node name fixing) difficult.
This should be overridden by label.


Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Mohan Kumar D]

On 3/25/2022 3:12 PM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 25/03/2022 10:31, Mohan Kumar D wrote:
>> On 3/25/2022 12:56 PM, Krzysztof Kozlowski wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 25/03/2022 08:21, Mohan Kumar wrote:
>>>> Add iommus property for hda and enable the node for P3737 + P3701
>>>> platform.
>>>>
>>>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
>>>> ---
>>>>    arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
>>>>    arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
>>>>    2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> index 34d6a01ee1c6..156d5d95fde7 100644
>>>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> @@ -1751,6 +1751,7 @@
>>>>
>>>>                 hda@3510000 {
>>>>                         nvidia,model = "NVIDIA Jetson AGX Orin HDA";
>>>> +                     status = "okay";
>>> Nodes are enabled by default. Why do you need this?
>> hda node status is set to "disabled" by default in soc dts file
>> tegra234.dtsi. The enable is controlled by platform specific dts files.
> Oh, surprise... why do you override nodes with full path? This is
> error-prone and makes any changes (like node name fixing) difficult.
> This should be overridden by label.
    I see similar method is followed for earlier boards too for hda and 
other nodes. As chance of changing the node name is most unlikely due to 
node name is closely tied to HW base address for a given chip. If still 
there is a concern will fix with label in next patch.
>
>
> Best regards,
> Krzysztof

Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Thierry Reding]

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

On Fri, Mar 25, 2022 at 10:42:17AM +0100, Krzysztof Kozlowski wrote:
> On 25/03/2022 10:31, Mohan Kumar D wrote:
> > 
> > On 3/25/2022 12:56 PM, Krzysztof Kozlowski wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 25/03/2022 08:21, Mohan Kumar wrote:
> >>> Add iommus property for hda and enable the node for P3737 + P3701
> >>> platform.
> >>>
> >>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> >>> ---
> >>>   arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
> >>>   arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
> >>>   2 files changed, 2 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> >>> index 34d6a01ee1c6..156d5d95fde7 100644
> >>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> >>> @@ -1751,6 +1751,7 @@
> >>>
> >>>                hda@3510000 {
> >>>                        nvidia,model = "NVIDIA Jetson AGX Orin HDA";
> >>> +                     status = "okay";
> >> Nodes are enabled by default. Why do you need this?
> > hda node status is set to "disabled" by default in soc dts file 
> > tegra234.dtsi. The enable is controlled by platform specific dts files.
> 
> Oh, surprise... why do you override nodes with full path? This is
> error-prone and makes any changes (like node name fixing) difficult.
> This should be overridden by label.

I disagree, though I admit that this is probably very subjective. In my
experience label references lead to completely unreadable DTS files.
We've had bad experiences with these kinds of references early on, so at
some point we decided to discontinue that method on Tegra.

Reflecting the tree hierarchy in board-level DTS files on the other hand
makes it very clear what you're changing and keeps the board-level DTS
quite readable.

Fixing node names hasn't been a big problem for us and any potential
remaining issues are entirely gone now that we can validate DTS files
using dt-schema.

Thierry

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

RE: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

[Mohan Kumar D]

Thierry,
Thanks for your explanation.

Krzysztof, 
Let me know if you still have any concern on the current patch.

-----Original Message-----
From: Thierry Reding <thierry.reding@gmail.com> 
Sent: Wednesday, March 30, 2022 4:18 PM
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Mohan Kumar D <mkumard@nvidia.com>; robh+dt@kernel.org; krzk+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan Hunter <jonathanh@nvidia.com>
Subject: Re: [PATCH] arm64: tegra: Enable hda node for P3737 + P3701

On Fri, Mar 25, 2022 at 10:42:17AM +0100, Krzysztof Kozlowski wrote:
> On 25/03/2022 10:31, Mohan Kumar D wrote:
> > 
> > On 3/25/2022 12:56 PM, Krzysztof Kozlowski wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 25/03/2022 08:21, Mohan Kumar wrote:
> >>> Add iommus property for hda and enable the node for P3737 + P3701 
> >>> platform.
> >>>
> >>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> >>> ---
> >>>   arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts | 1 +
> >>>   arch/arm64/boot/dts/nvidia/tegra234.dtsi                      | 1 +
> >>>   2 files changed, 2 insertions(+)
> >>>
> >>> diff --git 
> >>> a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts 
> >>> b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> >>> index 34d6a01ee1c6..156d5d95fde7 100644
> >>> --- 
> >>> a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dt
> >>> +++ s
> >>> @@ -1751,6 +1751,7 @@
> >>>
> >>>                hda@3510000 {
> >>>                        nvidia,model = "NVIDIA Jetson AGX Orin 
> >>> HDA";
> >>> +                     status = "okay";
> >> Nodes are enabled by default. Why do you need this?
> > hda node status is set to "disabled" by default in soc dts file 
> > tegra234.dtsi. The enable is controlled by platform specific dts files.
> 
> Oh, surprise... why do you override nodes with full path? This is 
> error-prone and makes any changes (like node name fixing) difficult.
> This should be overridden by label.

I disagree, though I admit that this is probably very subjective. In my experience label references lead to completely unreadable DTS files.
We've had bad experiences with these kinds of references early on, so at some point we decided to discontinue that method on Tegra.

Reflecting the tree hierarchy in board-level DTS files on the other hand makes it very clear what you're changing and keeps the board-level DTS quite readable.

Fixing node names hasn't been a big problem for us and any potential remaining issues are entirely gone now that we can validate DTS files using dt-schema.

Thierry