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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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/[email protected]/
--
Best wishes,
Vladimir
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
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
>
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
>
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
>> ---
>> 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