2023-09-12 18:07:27

by Konrad Dybcio

[permalink] [raw]
Subject: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>
---
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


2023-09-13 18:36:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>
> ---
> 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

2023-09-13 22:35:02

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>
>> ---
[...]

>> 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

2023-09-14 11:32:14

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>
>>> ---
> [...]
>
>>> 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

2023-09-14 12:10:20

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>
>>>> ---
>> [...]
>>
>>>> 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

2024-01-02 21:26:49

by Marijn Suijten

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks

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 <[email protected]>

This makes the USB and IOMMUs probe again on SDM630 devices like the Sony Xperia
XA2 Ultra. Thanks!

Reviewed-by: Marijn Suijten <[email protected]>

+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
>