On 14.02.2025 7:50 AM, Manivannan Sadhasivam wrote: > On Mon, Feb 10, 2025 at 08:20:27PM +0100, Konrad Dybcio wrote: >> 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. >> > > Shouldn't it be applied to config path of all peripherals then? If > QCOM_ICC_TAG_ACTIVE_ONLY translates to 'resource getting voted only if the CPUSS > is active', then the same constraint should apply to all peripherals, isn't it? Yes and lately we've been trying to catch that in review Konrad > I'm not sure who is accessing the config path other than the CPUs. >>hopefully<, no one Konrad
