| Message ID | 20220407172145.31903-1-jonathan@marek.ca (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields | expand |
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
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 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 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 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 >
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 >>
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 >>
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",
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(-)