| Message ID | 20230721-topic-rpm_clk_cleanup-v2-6-1e506593b1bd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean up RPM bus clocks remnants | expand |
On 12/09/2023 15:31, Konrad Dybcio wrote: > These clocks are now handled from within the icc framework and are > no longer registered from within the CCF. Remove them. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > arch/arm64/boot/dts/qcom/sdm630.dtsi | 49 +++++++----------------------------- > 1 file changed, 9 insertions(+), 40 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi > index ec6003212c4d..f11d2a07508c 100644 > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi > @@ -605,9 +605,6 @@ bimc: interconnect@1008000 { > compatible = "qcom,sdm660-bimc"; > reg = <0x01008000 0x78000>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_BIMC_CLK>, > - <&rpmcc RPM_SMD_BIMC_A_CLK>; Bindings expect here two clocks, so you miss some bindings patches. > }; > > restart@10ac000 { > @@ -619,28 +616,17 @@ cnoc: interconnect@1500000 { > compatible = "qcom,sdm660-cnoc"; > reg = <0x01500000 0x10000>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_CNOC_CLK>, > - <&rpmcc RPM_SMD_CNOC_A_CLK>; > }; > > snoc: interconnect@1626000 { > compatible = "qcom,sdm660-snoc"; > reg = <0x01626000 0x7090>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_SNOC_CLK>, > - <&rpmcc RPM_SMD_SNOC_A_CLK>; > }; > > anoc2_smmu: iommu@16c0000 { > compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; > reg = <0x016c0000 0x40000>; > - > - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > - assigned-clock-rates = <1000>; > - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > - clock-names = "bus"; This is also against bindings. After your patch #4, such bus clock (or other combinations) is still required. > #global-interrupts = <2>; > #iommu-cells = <1>; > > @@ -685,16 +671,12 @@ a2noc: interconnect@1704000 { > compatible = "qcom,sdm660-a2noc"; > reg = <0x01704000 0xc100>; > #interconnect-cells = <1>; > - clock-names = "bus", > - "bus_a", > - "ipa", > + clock-names = "ipa", And which bindings does this match? Best regards, Krzysztof
On 13.09.2023 09:13, Krzysztof Kozlowski wrote: > On 12/09/2023 15:31, Konrad Dybcio wrote: >> These clocks are now handled from within the icc framework and are >> no longer registered from within the CCF. Remove them. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- [...] >> anoc2_smmu: iommu@16c0000 { >> compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; >> reg = <0x016c0000 0x40000>; >> - >> - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >> - assigned-clock-rates = <1000>; >> - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >> - clock-names = "bus"; > > This is also against bindings. After your patch #4, such bus clock (or > other combinations) is still required. So, we have 4 SMMU instances on this platform: MMSS (described, iface, mem, mem_iface) GPU (described, iface-mm, iface-smmu, bus-smmu) ANOC2 (this one, no clocks after removing rpmcc bus) LPASS (no clocks) Should I then create a new entry in the bindings, replicating what's there for msm8998[1] and dropping the entry with just "bus" from anyOf? Konrad [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/iommu/arm,smmu.yaml?h=next-20230913#n272
On 13/09/2023 14:08, Konrad Dybcio wrote: > On 13.09.2023 09:13, Krzysztof Kozlowski wrote: >> On 12/09/2023 15:31, Konrad Dybcio wrote: >>> These clocks are now handled from within the icc framework and are >>> no longer registered from within the CCF. Remove them. >>> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- > [...] > >>> anoc2_smmu: iommu@16c0000 { >>> compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; >>> reg = <0x016c0000 0x40000>; >>> - >>> - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >>> - assigned-clock-rates = <1000>; >>> - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >>> - clock-names = "bus"; >> >> This is also against bindings. After your patch #4, such bus clock (or >> other combinations) is still required. > So, we have 4 SMMU instances on this platform: > > MMSS (described, iface, mem, mem_iface) > GPU (described, iface-mm, iface-smmu, bus-smmu) > > ANOC2 (this one, no clocks after removing rpmcc bus) > LPASS (no clocks) Ah, I did not notice it. > > Should I then create a new entry in the bindings, replicating > what's there for msm8998[1] and dropping the entry with just "bus" > from anyOf? So this passes the bindings, right? anyOf: in the binding should allow also no match, so this should be fine. However indeed we need to drop the "bus" entry, because it is not valid anymore. Best regards, Krzysztof
On 14.09.2023 08:26, Krzysztof Kozlowski wrote: > On 13/09/2023 14:08, Konrad Dybcio wrote: >> On 13.09.2023 09:13, Krzysztof Kozlowski wrote: >>> On 12/09/2023 15:31, Konrad Dybcio wrote: >>>> These clocks are now handled from within the icc framework and are >>>> no longer registered from within the CCF. Remove them. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >> [...] >> >>>> anoc2_smmu: iommu@16c0000 { >>>> compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; >>>> reg = <0x016c0000 0x40000>; >>>> - >>>> - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >>>> - assigned-clock-rates = <1000>; >>>> - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; >>>> - clock-names = "bus"; >>> >>> This is also against bindings. After your patch #4, such bus clock (or >>> other combinations) is still required. >> So, we have 4 SMMU instances on this platform: >> >> MMSS (described, iface, mem, mem_iface) >> GPU (described, iface-mm, iface-smmu, bus-smmu) >> >> ANOC2 (this one, no clocks after removing rpmcc bus) >> LPASS (no clocks) > > Ah, I did not notice it. > >> >> Should I then create a new entry in the bindings, replicating >> what's there for msm8998[1] and dropping the entry with just "bus" >> from anyOf? > > So this passes the bindings, right? Yes anyOf: in the binding should allow > also no match, so this should be fine. However indeed we need to drop > the "bus" entry, because it is not valid anymore. Actually, looks like the LPASS smmu may require a single clock. We can reuse that single-"bus"-clock entry for HLOS1_VOTE_LPASS_ADSP_SMMU_CLK. The device didn't crash when trying to access LPASS SMMU with that clock absent, but I guess it may have just been luck, things may change once more hardware is parked.. Konrad
On 2023-09-12 15:31:44, Konrad Dybcio wrote: > These clocks are now handled from within the icc framework and are > no longer registered from within the CCF. Remove them. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> This makes the USB and IOMMUs probe again on SDM630 devices like the Sony Xperia XA2 Ultra. Thanks! Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> +cc Dmitry who was talking to me about this SoC. > --- > arch/arm64/boot/dts/qcom/sdm630.dtsi | 49 +++++++----------------------------- > 1 file changed, 9 insertions(+), 40 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi > index ec6003212c4d..f11d2a07508c 100644 > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi > @@ -605,9 +605,6 @@ bimc: interconnect@1008000 { > compatible = "qcom,sdm660-bimc"; > reg = <0x01008000 0x78000>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_BIMC_CLK>, > - <&rpmcc RPM_SMD_BIMC_A_CLK>; > }; > > restart@10ac000 { > @@ -619,28 +616,17 @@ cnoc: interconnect@1500000 { > compatible = "qcom,sdm660-cnoc"; > reg = <0x01500000 0x10000>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_CNOC_CLK>, > - <&rpmcc RPM_SMD_CNOC_A_CLK>; > }; > > snoc: interconnect@1626000 { > compatible = "qcom,sdm660-snoc"; > reg = <0x01626000 0x7090>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a"; > - clocks = <&rpmcc RPM_SMD_SNOC_CLK>, > - <&rpmcc RPM_SMD_SNOC_A_CLK>; > }; > > anoc2_smmu: iommu@16c0000 { > compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; > reg = <0x016c0000 0x40000>; > - > - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > - assigned-clock-rates = <1000>; > - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > - clock-names = "bus"; > #global-interrupts = <2>; > #iommu-cells = <1>; > > @@ -685,16 +671,12 @@ a2noc: interconnect@1704000 { > compatible = "qcom,sdm660-a2noc"; > reg = <0x01704000 0xc100>; > #interconnect-cells = <1>; > - clock-names = "bus", > - "bus_a", > - "ipa", > + clock-names = "ipa", > "ufs_axi", > "aggre2_ufs_axi", > "aggre2_usb3_axi", > "cfg_noc_usb2_axi"; > - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>, > - <&rpmcc RPM_SMD_AGGR2_NOC_A_CLK>, > - <&rpmcc RPM_SMD_IPA_CLK>, > + clocks = <&rpmcc RPM_SMD_IPA_CLK>, > <&gcc GCC_UFS_AXI_CLK>, > <&gcc GCC_AGGRE2_UFS_AXI_CLK>, > <&gcc GCC_AGGRE2_USB3_AXI_CLK>, > @@ -705,10 +687,8 @@ mnoc: interconnect@1745000 { > compatible = "qcom,sdm660-mnoc"; > reg = <0x01745000 0xa010>; > #interconnect-cells = <1>; > - clock-names = "bus", "bus_a", "iface"; > - clocks = <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK>, > - <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK_A>, > - <&mmcc AHB_CLK_SRC>; > + clock-names = "iface"; > + clocks = <&mmcc AHB_CLK_SRC>; > }; > > tsens: thermal-sensor@10ae000 { > @@ -1228,20 +1208,16 @@ usb3: usb@a8f8800 { > <&gcc GCC_USB30_MASTER_CLK>, > <&gcc GCC_AGGRE2_USB3_AXI_CLK>, > <&gcc GCC_USB30_SLEEP_CLK>, > - <&gcc GCC_USB30_MOCK_UTMI_CLK>, > - <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > + <&gcc GCC_USB30_MOCK_UTMI_CLK>; > clock-names = "cfg_noc", > "core", > "iface", > "sleep", > - "mock_utmi", > - "bus"; > + "mock_utmi"; > > assigned-clocks = <&gcc GCC_USB30_MOCK_UTMI_CLK>, > - <&gcc GCC_USB30_MASTER_CLK>, > - <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; > - assigned-clock-rates = <19200000>, <120000000>, > - <19200000>; > + <&gcc GCC_USB30_MASTER_CLK>; > + assigned-clock-rates = <19200000>, <120000000>; > > interrupts = <GIC_SPI 347 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH>; > @@ -2144,10 +2120,9 @@ mmss_smmu: iommu@cd00000 { > > clocks = <&mmcc MNOC_AHB_CLK>, > <&mmcc BIMC_SMMU_AHB_CLK>, > - <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK>, > <&mmcc BIMC_SMMU_AXI_CLK>; > clock-names = "iface-mm", "iface-smmu", > - "bus-mm", "bus-smmu"; > + "bus-smmu"; > #global-interrupts = <2>; > #iommu-cells = <1>; > > @@ -2264,12 +2239,6 @@ gnoc: interconnect@17900000 { > compatible = "qcom,sdm660-gnoc"; > reg = <0x17900000 0xe000>; > #interconnect-cells = <1>; > - /* > - * This one apparently features no clocks, > - * so let's not mess with the driver needlessly > - */ > - clock-names = "bus", "bus_a"; > - clocks = <&xo_board>, <&xo_board>; > }; > > apcs_glb: mailbox@17911000 { > > -- > 2.42.0 >
diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi index ec6003212c4d..f11d2a07508c 100644 --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi @@ -605,9 +605,6 @@ bimc: interconnect@1008000 { compatible = "qcom,sdm660-bimc"; reg = <0x01008000 0x78000>; #interconnect-cells = <1>; - clock-names = "bus", "bus_a"; - clocks = <&rpmcc RPM_SMD_BIMC_CLK>, - <&rpmcc RPM_SMD_BIMC_A_CLK>; }; restart@10ac000 { @@ -619,28 +616,17 @@ cnoc: interconnect@1500000 { compatible = "qcom,sdm660-cnoc"; reg = <0x01500000 0x10000>; #interconnect-cells = <1>; - clock-names = "bus", "bus_a"; - clocks = <&rpmcc RPM_SMD_CNOC_CLK>, - <&rpmcc RPM_SMD_CNOC_A_CLK>; }; snoc: interconnect@1626000 { compatible = "qcom,sdm660-snoc"; reg = <0x01626000 0x7090>; #interconnect-cells = <1>; - clock-names = "bus", "bus_a"; - clocks = <&rpmcc RPM_SMD_SNOC_CLK>, - <&rpmcc RPM_SMD_SNOC_A_CLK>; }; anoc2_smmu: iommu@16c0000 { compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2"; reg = <0x016c0000 0x40000>; - - assigned-clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; - assigned-clock-rates = <1000>; - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; - clock-names = "bus"; #global-interrupts = <2>; #iommu-cells = <1>; @@ -685,16 +671,12 @@ a2noc: interconnect@1704000 { compatible = "qcom,sdm660-a2noc"; reg = <0x01704000 0xc100>; #interconnect-cells = <1>; - clock-names = "bus", - "bus_a", - "ipa", + clock-names = "ipa", "ufs_axi", "aggre2_ufs_axi", "aggre2_usb3_axi", "cfg_noc_usb2_axi"; - clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>, - <&rpmcc RPM_SMD_AGGR2_NOC_A_CLK>, - <&rpmcc RPM_SMD_IPA_CLK>, + clocks = <&rpmcc RPM_SMD_IPA_CLK>, <&gcc GCC_UFS_AXI_CLK>, <&gcc GCC_AGGRE2_UFS_AXI_CLK>, <&gcc GCC_AGGRE2_USB3_AXI_CLK>, @@ -705,10 +687,8 @@ mnoc: interconnect@1745000 { compatible = "qcom,sdm660-mnoc"; reg = <0x01745000 0xa010>; #interconnect-cells = <1>; - clock-names = "bus", "bus_a", "iface"; - clocks = <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK>, - <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK_A>, - <&mmcc AHB_CLK_SRC>; + clock-names = "iface"; + clocks = <&mmcc AHB_CLK_SRC>; }; tsens: thermal-sensor@10ae000 { @@ -1228,20 +1208,16 @@ usb3: usb@a8f8800 { <&gcc GCC_USB30_MASTER_CLK>, <&gcc GCC_AGGRE2_USB3_AXI_CLK>, <&gcc GCC_USB30_SLEEP_CLK>, - <&gcc GCC_USB30_MOCK_UTMI_CLK>, - <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; + <&gcc GCC_USB30_MOCK_UTMI_CLK>; clock-names = "cfg_noc", "core", "iface", "sleep", - "mock_utmi", - "bus"; + "mock_utmi"; assigned-clocks = <&gcc GCC_USB30_MOCK_UTMI_CLK>, - <&gcc GCC_USB30_MASTER_CLK>, - <&rpmcc RPM_SMD_AGGR2_NOC_CLK>; - assigned-clock-rates = <19200000>, <120000000>, - <19200000>; + <&gcc GCC_USB30_MASTER_CLK>; + assigned-clock-rates = <19200000>, <120000000>; interrupts = <GIC_SPI 347 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH>; @@ -2144,10 +2120,9 @@ mmss_smmu: iommu@cd00000 { clocks = <&mmcc MNOC_AHB_CLK>, <&mmcc BIMC_SMMU_AHB_CLK>, - <&rpmcc RPM_SMD_MMSSNOC_AXI_CLK>, <&mmcc BIMC_SMMU_AXI_CLK>; clock-names = "iface-mm", "iface-smmu", - "bus-mm", "bus-smmu"; + "bus-smmu"; #global-interrupts = <2>; #iommu-cells = <1>; @@ -2264,12 +2239,6 @@ gnoc: interconnect@17900000 { compatible = "qcom,sdm660-gnoc"; reg = <0x17900000 0xe000>; #interconnect-cells = <1>; - /* - * This one apparently features no clocks, - * so let's not mess with the driver needlessly - */ - clock-names = "bus", "bus_a"; - clocks = <&xo_board>, <&xo_board>; }; apcs_glb: mailbox@17911000 {
These clocks are now handled from within the icc framework and are no longer registered from within the CCF. Remove them. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- arch/arm64/boot/dts/qcom/sdm630.dtsi | 49 +++++++----------------------------- 1 file changed, 9 insertions(+), 40 deletions(-)