2023-07-19 07:41:24

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: qcom: sdm630: remove refs to nonexistent clocks

On 19/07/2023 09:35, Alexey Minnekhanov wrote:
> Since commit d6edc31f3a68 ("clk: qcom: smd-rpm: Separate out
> interconnect bus clocks") rpmcc-sdm660 no longer provides
> RPM_SMD_AGGR2_NOC_CLK and RPM_SMD_AGGR2_NOC_A_CLK clocks.
> Remove them to fix various probe failures and get devices
> booting again.

So that commit broke DTS?

>
> Signed-off-by: Alexey Minnekhanov <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sdm630.dtsi | 24 ++++++------------------
> 1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> index 038ec7a41412..8bea611b246b 100644
> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> @@ -638,10 +638,6 @@ 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";

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof



2023-07-19 08:50:12

by Alexey Minnekhanov

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: qcom: sdm630: remove refs to nonexistent clocks

On 19.07.2023 10:39, Krzysztof Kozlowski wrote:
> On 19/07/2023 09:35, Alexey Minnekhanov wrote:
>> Since commit d6edc31f3a68 ("clk: qcom: smd-rpm: Separate out
>> interconnect bus clocks") rpmcc-sdm660 no longer provides
>> RPM_SMD_AGGR2_NOC_CLK and RPM_SMD_AGGR2_NOC_A_CLK clocks.
>> Remove them to fix various probe failures and get devices
>> booting again.
>
> So that commit broke DTS?
>

Yes, this is my understanding of the situation.
The commit in subject [1] is only in linux-next for a few days, so it
broke booting only on 6.5-rc (rc2 currently). Konrad said: "these clocks
references were API abuses; referencing the bus clocks was circumventing
the interconnect layer. That loophole is now gone and the abusers are
now apparent"

>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check`
>

If DT schema for interconnect requires bus clocks to be specified, I
don't even know what to put there now. Can we change schema?

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d6edc31f3a68d8d0636e0cfcd9eced7460ad32f4

--
Regards,
Alexey Minnekhanov
postmarketOS developer

2023-07-19 10:04:23

by Stephan Gerhold

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: qcom: sdm630: remove refs to nonexistent clocks

On Wed, Jul 19, 2023 at 11:36:46AM +0300, Alexey Minnekhanov wrote:
> On 19.07.2023 10:39, Krzysztof Kozlowski wrote:
> > It does not look like you tested the DTS against bindings. Please run
> > `make dtbs_check`
> >
>
> If DT schema for interconnect requires bus clocks to be specified, I don't
> even know what to put there now. Can we change schema?
>

I think I mentioned the DT schema updates during the review of Konrad's
interconnect changes and he mentioned he would like to clean those up
after getting the series in. (Which would be sometime soon now I guess)

For now, having the &rpmcc "bus"/"bus_a"/"ipa" clocks specified on the
interconnect@... nodes is still valid. At runtime they will just be
ignored. Feel free to just keep them there for this initial fix.

For the other two usages (iommu@, usb@) these votes with minimal
frequency look a bit related to the "keep_alive" stuff Konrad added. [1]
Maybe that could be used here instead of bypassing interconnect with the
clock votes?

Thanks,
Stephan

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b979049c38e170286158e97290c892957c836903