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@xxxxxxxxxxx> >>>> >>>> Add UFS host controller and PHY nodes for SM8750 SoC. >>>> >>>> Co-developed-by: Manish Pandey <quic_mapa@xxxxxxxxxxx> >>>> Signed-off-by: Manish Pandey <quic_mapa@xxxxxxxxxxx> >>>> Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx> >>>> Signed-off-by: Melody Olvera <quic_molvera@xxxxxxxxxxx> >>>> --- [...] >>> 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
