[PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Jonathan Marek]

Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
Ignored and undocumented with upstream UFS driver so delete for now.

Fixes: aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 2c18e1ef9e82d..90cdbec3cac99 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1663,9 +1663,6 @@ ufs_mem_hc: ufshc@1d84000 {
 
 			iommus = <&apps_smmu 0xe0 0x0>;
 
-			interconnects = <&aggre1_noc MASTER_UFS_MEM &mc_virt SLAVE_EBI1>,
-					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_UFS_MEM_CFG>;
-			interconnect-names = "ufs-ddr", "cpu-ufs";
 			clock-names =
 				"core_clk",
 				"bus_aggr_clk",
-- 
2.26.1

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Vladimir Zapolskiy]

On 4/7/22 20:21, Jonathan Marek wrote:
> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
> Ignored and undocumented with upstream UFS driver so delete for now.

Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
qcom,ufs: convert to dtschema").

It's questionable, if an example in the new yaml file is totally correct
in connection to the discussed issue.

> Fixes: aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 2c18e1ef9e82d..90cdbec3cac99 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -1663,9 +1663,6 @@ ufs_mem_hc: ufshc@1d84000 {
>   
>   			iommus = <&apps_smmu 0xe0 0x0>;
>   
> -			interconnects = <&aggre1_noc MASTER_UFS_MEM &mc_virt SLAVE_EBI1>,
> -					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_UFS_MEM_CFG>;
> -			interconnect-names = "ufs-ddr", "cpu-ufs";
>   			clock-names =
>   				"core_clk",
>   				"bus_aggr_clk",

You may look at https://lore.kernel.org/linux-arm-msm/20220310221934.1560729-1-vladimir.zapolskiy@linaro.org/

--
Best wishes,
Vladimir

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Krzysztof Kozlowski]

On 07/04/2022 21:40, Vladimir Zapolskiy wrote:
> On 4/7/22 20:21, Jonathan Marek wrote:
>> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
>> Ignored and undocumented with upstream UFS driver so delete for now.

This is the upstream and they are documented here, although as pointed
by Vladimir this was rather a reverse-documentation. The documentation
might be incorrect, but then the bindings should be corrected instead of
only modifying the DTS.

> 
> Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
> qcom,ufs: convert to dtschema").
> 
> It's questionable, if an example in the new yaml file is totally correct
> in connection to the discussed issue.

To be honest - the example probably is not correct, because it was based
on existing DTS without your patch. :)

Another question is whether the interconnect properties are here correct
at all. I assumed that DTS is correct because it should describe the
hardware, even if driver does not use it. However maybe that was a false
assumption...


Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Jonathan Marek]

On 4/7/22 5:16 PM, Krzysztof Kozlowski wrote:
> On 07/04/2022 21:40, Vladimir Zapolskiy wrote:
>> On 4/7/22 20:21, Jonathan Marek wrote:
>>> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
>>> Ignored and undocumented with upstream UFS driver so delete for now.
> 
> This is the upstream and they are documented here, although as pointed
> by Vladimir this was rather a reverse-documentation. The documentation
> might be incorrect, but then the bindings should be corrected instead of
> only modifying the DTS.
> 
>>
>> Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
>> qcom,ufs: convert to dtschema").
>>
>> It's questionable, if an example in the new yaml file is totally correct
>> in connection to the discussed issue.
> 
> To be honest - the example probably is not correct, because it was based
> on existing DTS without your patch. :)
> 
> Another question is whether the interconnect properties are here correct
> at all. I assumed that DTS is correct because it should describe the
> hardware, even if driver does not use it. However maybe that was a false
> assumption...
> 

writing-bindings.rst says it is OK to document even if it isn't used by 
the driver (seems wrong to me, at least for interconnects which are a 
firmware abstraction and not hardware).

462c5c0aa798 wasn't in my 5.17+ tree pulled after dts changes were 
merged (I guess doc changes come later), so my commit message is 
incorrect, but I think it makes more sense to have the documentation 
reflect the driver. Its also not an important issue, so I'll let others 
sort it out.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Bjorn Andersson]

On Thu 07 Apr 17:38 CDT 2022, Jonathan Marek wrote:

> On 4/7/22 5:16 PM, Krzysztof Kozlowski wrote:
> > On 07/04/2022 21:40, Vladimir Zapolskiy wrote:
> > > On 4/7/22 20:21, Jonathan Marek wrote:
> > > > Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
> > > > Ignored and undocumented with upstream UFS driver so delete for now.
> > 
> > This is the upstream and they are documented here, although as pointed
> > by Vladimir this was rather a reverse-documentation. The documentation
> > might be incorrect, but then the bindings should be corrected instead of
> > only modifying the DTS.
> > 
> > > 
> > > Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
> > > qcom,ufs: convert to dtschema").
> > > 
> > > It's questionable, if an example in the new yaml file is totally correct
> > > in connection to the discussed issue.
> > 
> > To be honest - the example probably is not correct, because it was based
> > on existing DTS without your patch. :)
> > 
> > Another question is whether the interconnect properties are here correct
> > at all. I assumed that DTS is correct because it should describe the
> > hardware, even if driver does not use it. However maybe that was a false
> > assumption...
> > 
> 
> writing-bindings.rst says it is OK to document even if it isn't used by the
> driver (seems wrong to me, at least for interconnects which are a firmware
> abstraction and not hardware).
> 

The devicetree, and hence the binding, should describe the hardware, so
that an implementation can make use of the hardware. So there's no
problem expressing the interconnect in the binding/dts even though the
driver isn't using it.

I'm not sure if I'm misunderstanding you, the interconnect paths
described here are a description of the hardware requirements for this
device. (I.e. it need the buses between ufs and ddr, and cpu and ufs to
operate).

> 462c5c0aa798 wasn't in my 5.17+ tree pulled after dts changes were merged (I
> guess doc changes come later), so my commit message is incorrect, but I
> think it makes more sense to have the documentation reflect the driver. Its
> also not an important issue, so I'll let others sort it out.
> 

I believe that the correctness of the interconnect property will ensure
that the interconnect provider doesn't hit sync_state until the ufs
driver has probed - regardless of the driver actually implementing the
interconnect voting. But perhaps I've misunderstood the magic involved?

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Jonathan Marek]

On 4/11/22 10:16 PM, Bjorn Andersson wrote:
> On Thu 07 Apr 17:38 CDT 2022, Jonathan Marek wrote:
> 
>> On 4/7/22 5:16 PM, Krzysztof Kozlowski wrote:
>>> On 07/04/2022 21:40, Vladimir Zapolskiy wrote:
>>>> On 4/7/22 20:21, Jonathan Marek wrote:
>>>>> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
>>>>> Ignored and undocumented with upstream UFS driver so delete for now.
>>>
>>> This is the upstream and they are documented here, although as pointed
>>> by Vladimir this was rather a reverse-documentation. The documentation
>>> might be incorrect, but then the bindings should be corrected instead of
>>> only modifying the DTS.
>>>
>>>>
>>>> Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
>>>> qcom,ufs: convert to dtschema").
>>>>
>>>> It's questionable, if an example in the new yaml file is totally correct
>>>> in connection to the discussed issue.
>>>
>>> To be honest - the example probably is not correct, because it was based
>>> on existing DTS without your patch. :)
>>>
>>> Another question is whether the interconnect properties are here correct
>>> at all. I assumed that DTS is correct because it should describe the
>>> hardware, even if driver does not use it. However maybe that was a false
>>> assumption...
>>>
>>
>> writing-bindings.rst says it is OK to document even if it isn't used by the
>> driver (seems wrong to me, at least for interconnects which are a firmware
>> abstraction and not hardware).
>>
> 
> The devicetree, and hence the binding, should describe the hardware, so
> that an implementation can make use of the hardware. So there's no
> problem expressing the interconnect in the binding/dts even though the
> driver isn't using it.
> 
> I'm not sure if I'm misunderstanding you, the interconnect paths
> described here are a description of the hardware requirements for this
> device. (I.e. it need the buses between ufs and ddr, and cpu and ufs to
> operate).
> 

This is pedantic but what if my kernel lives in imem and not ddr. Or it 
runs on adsp and not cpu? "ufs-ddr" and "cpu-ufs" are not necessarily 
hardware requirements.

(I was thinking of something else when I wrote that comment, but it 
doesn't actually matter if its firmware/hardware if a driver can 
implement the same functionality either way)

>> 462c5c0aa798 wasn't in my 5.17+ tree pulled after dts changes were merged (I
>> guess doc changes come later), so my commit message is incorrect, but I
>> think it makes more sense to have the documentation reflect the driver. Its
>> also not an important issue, so I'll let others sort it out.
>>
> 
> I believe that the correctness of the interconnect property will ensure
> that the interconnect provider doesn't hit sync_state until the ufs
> driver has probed - regardless of the driver actually implementing the
> interconnect voting. But perhaps I've misunderstood the magic involved?
> 

AFAICT, if its not used by the driver it will be ignored completely, 
unless you use OPP (which has some interconnect magic).

> Regards,
> Bjorn
> 

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Bjorn Andersson]

On Thu 07 Apr 12:21 CDT 2022, Jonathan Marek wrote:

> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
> Ignored and undocumented with upstream UFS driver so delete for now.
> 

Just to clarify, the binding do document interconnects and the property
should be there in the end. v1 (why isn't this marked v2?) was correct.

What I asked for was a statement on why it should be picked up for
v5.18-rc (as Dmitry requested).

Regards,
Bjorn

> Fixes: aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 2c18e1ef9e82d..90cdbec3cac99 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -1663,9 +1663,6 @@ ufs_mem_hc: ufshc@1d84000 {
>  
>  			iommus = <&apps_smmu 0xe0 0x0>;
>  
> -			interconnects = <&aggre1_noc MASTER_UFS_MEM &mc_virt SLAVE_EBI1>,
> -					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_UFS_MEM_CFG>;
> -			interconnect-names = "ufs-ddr", "cpu-ufs";
>  			clock-names =
>  				"core_clk",
>  				"bus_aggr_clk",
> -- 
> 2.26.1
> 

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Jonathan Marek]

On 4/12/22 4:51 PM, Bjorn Andersson wrote:
> On Thu 07 Apr 12:21 CDT 2022, Jonathan Marek wrote:
> 
>> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
>> Ignored and undocumented with upstream UFS driver so delete for now.
>>
> 
> Just to clarify, the binding do document interconnects and the property
> should be there in the end. v1 (why isn't this marked v2?) was correct.
> 
> What I asked for was a statement on why it should be picked up for
> v5.18-rc (as Dmitry requested).
> 

This isn't a v2, I sent this without seeing there was already patch for 
the same "problem".

A reason for picking it up is that if you have a patch adding the 
interconnect support to the UFS driver in your tree, the incorrect dts 
will prevent it from probing  (so the 5.18-rc1 dts could fail with newer 
kernel eventually, not sure if upstream cares about that?)

> Regards,
> Bjorn
> 
>> Fixes: aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 2c18e1ef9e82d..90cdbec3cac99 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -1663,9 +1663,6 @@ ufs_mem_hc: ufshc@1d84000 {
>>   
>>   			iommus = <&apps_smmu 0xe0 0x0>;
>>   
>> -			interconnects = <&aggre1_noc MASTER_UFS_MEM &mc_virt SLAVE_EBI1>,
>> -					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_UFS_MEM_CFG>;
>> -			interconnect-names = "ufs-ddr", "cpu-ufs";
>>   			clock-names =
>>   				"core_clk",
>>   				"bus_aggr_clk",
>> -- 
>> 2.26.1
>>

Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Dmitry Baryshkov]

On 12/04/2022 23:51, Bjorn Andersson wrote:
> On Thu 07 Apr 12:21 CDT 2022, Jonathan Marek wrote:
> 
>> Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
>> Ignored and undocumented with upstream UFS driver so delete for now.
>>
> 
> Just to clarify, the binding do document interconnects and the property
> should be there in the end. v1 (why isn't this marked v2?) was correct.
> 
> What I asked for was a statement on why it should be picked up for
> v5.18-rc (as Dmitry requested).

I have a slight preference for fixing the icc rather than dropping it.

However I'm fine with either of the patches.

The ufs/qcom,ufs.yaml describes these interconnects (basing on the 
sm8450 if I understand correctly). Thus if decide to drop interconnect 
properties, we should also update the binding.


> 
> Regards,
> Bjorn
> 
>> Fixes: aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 2c18e1ef9e82d..90cdbec3cac99 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -1663,9 +1663,6 @@ ufs_mem_hc: ufshc@1d84000 {
>>   
>>   			iommus = <&apps_smmu 0xe0 0x0>;
>>   
>> -			interconnects = <&aggre1_noc MASTER_UFS_MEM &mc_virt SLAVE_EBI1>,
>> -					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_UFS_MEM_CFG>;
>> -			interconnect-names = "ufs-ddr", "cpu-ufs";
>>   			clock-names =
>>   				"core_clk",
>>   				"bus_aggr_clk",
>> -- 
>> 2.26.1
>>


-- 
With best wishes
Dmitry