arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields
PATCH 1/1 arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields
Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong. Ignored and undocumented with upstream UFS driver so delete for now.
--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 --- 1 file changed, 3 deletions(-)
arch/arm64/boot/dts/qcom/sm8450.dtsi +0 -3
@@ -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",
Signature · 1 line
2.26.1
Quoted 2 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 11 lines
Fixes aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")Signed-off-by Jonathan Marek <[email protected]>--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 --- 1 file changed, 3 deletions(-)arch/arm64/boot/dts/qcom/sm8450.dtsi +0
-0
arch/arm64/boot/dts/qcom/sm8450.dtsi ufs_mem_hc: ufshc@1d84000 { · line 1668
Quoted 9 lines
arch/arm64/boot/dts/qcom/sm8450.dtsi +0
-3@@ -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/[email protected]/
--
Best wishes,
Vladimir
Quoted 3 lines · Vladimir Zapolskiy
Quoted 2 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 6 lines
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
Quoted 17 lines · Krzysztof Kozlowski
Quoted 3 lines · Vladimir Zapolskiy
Quoted 2 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 6 lines
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.
Quoted 8 lines
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.
Quoted 4 lines
Best regards,
Krzysztof
Quoted 27 lines · Jonathan Marek
Quoted 17 lines · Krzysztof Kozlowski
Quoted 3 lines · Vladimir Zapolskiy
Quoted 2 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 6 lines
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.
Quoted 8 lines
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...
Quoted 4 lines
writing-bindings.rstsays 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).
Quoted 5 lines
462c5c0aa798wasn'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
Quoted 39 lines · Bjorn Andersson
Quoted 27 lines · Jonathan Marek
Quoted 17 lines · Krzysztof Kozlowski
Quoted 3 lines · Vladimir Zapolskiy
Quoted 2 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 6 lines
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.
Quoted 8 lines
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...
Quoted 4 lines
writing-bindings.rstsays 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.
Quoted 5 lines
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)
Quoted 11 lines
Quoted 5 lines
462c5c0aa798wasn'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_stateuntil 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).
Quoted 3 lines
Regards,
Bjorn
Quoted 3 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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
Quoted 23 lines
Fixes aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")Signed-off-by Jonathan Marek <[email protected]>--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 --- 1 file changed, 3 deletions(-)arch/arm64/boot/dts/qcom/sm8450.dtsi +0
-3@@ -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",Signature · 1 line
2.26.1
Quoted 6 lines · Bjorn Andersson
Quoted 3 lines · Jonathan Marek
Upstream
sm8450.dtsihas #interconnect-cells = <2>; so these are wrong. Ignored and undocumented with upstream UFS driver so delete for now.
Quoted 6 lines
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?)
Quoted 26 lines
Regards,
BjornQuoted 23 lines
Fixes aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")Signed-off-by Jonathan Marek <[email protected]>--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 --- 1 file changed, 3 deletions(-)arch/arm64/boot/dts/qcom/sm8450.dtsi +0
-3@@ -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",Signature · 1 line
2.26.1
Quoted 11 lines · Bjorn Andersson
Quoted 3 lines · Jonathan Marek
Upstream
sm8450.dtsihas #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.
Quoted 27 lines
Regards,
BjornQuoted 23 lines
Fixes aa2d0bf04a3c ("arm64: dts: qcom: sm8450: add interconnect nodes")Signed-off-by Jonathan Marek <[email protected]>--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 --- 1 file changed, 3 deletions(-)arch/arm64/boot/dts/qcom/sm8450.dtsi +0
-3@@ -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",Signature · 1 line
2.26.1
Signature · 2 lines
With best wishes Dmitry