Re: [PATCH 4/5] arm64: dts: qcom: sm8750: Add UFS nodes for SM8750 SoC

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 8.02.2025 11:06 PM, Dmitry Baryshkov wrote:
> On Sun, Feb 09, 2025 at 12:47:56AM +0530, Nitin Rawat wrote:
>>
>>
>> On 1/14/2025 4:22 PM, Dmitry Baryshkov wrote:
>>> On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote:
>>>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>
>>>> Add UFS host controller and PHY nodes for SM8750 SoC.
>>>>
>>>> Co-developed-by: Manish Pandey <quic_mapa@quicinc.com>
>>>> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>
>>>> ---

[...]

>>> Use OPP table instead
>>
>> Currently, OPP is not enabled in the device tree for any previous targets. I
> 
> Excuse me? ufs_opp_table is present on SM8250, SM8550 and SDM845 (and
> QCS615). So this is not correct
> 
>> plan to enable OPP in a separate patch at a later stage. This is because
>> there is an ongoing patch in the upstream that aims to enable multiple-level
>> clock scaling using OPP, which may introduce changes to the device tree
>> entries. To avoid extra efforts, I intend to enable OPP once that patch is
>> merged.
> 
> Whatever changes are introduced, old DT must still continue to work.
> There is no reason to use legacy freq-table-hz if you can use OPP table.
> 
>> Please let me know if you have any concerns.

Go ahead with the OPP table. freq-table-hz is ancient and doesn't describe
e.g. the required RPMh levels for core clock frequencies.

You should then drop required-opps from the UFS node.

>>>> +
>>>> +			resets = <&gcc GCC_UFS_PHY_BCR>;
>>>> +			reset-names = "rst";
>>>> +
>>>> +
>>>> +			interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
>>>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
>>>> +					 &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>;
>>>
>>> Shouldn't cpu-ufs be ACTIVE_ONLY?
>>
>> As per ufs driver implementation, Icc voting from ufs driver is removed as
>> part of low power mode (suspend or clock gating) and voted again in
>> resume/ungating path. Hence TAG_ALWAYS will have no power concern.
>> All previous targets have the same configuration.
> 
> arch/arm64/boot/dts/qcom/qcs615.dtsi:                                    &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
> 
> It might be a mistake for that target though. Your explanation sounds
> fine to me.

Let's use QCOM_ICC_TAG_ACTIVE_ONLY for the CPU path to clear up confusion.

Toggling it from the driver makes sense for UFS-idling-while-CPUs-are-online
cases and accidentally also does what RPMh does internally in the other case.

Konrad