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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

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?

I'm not sure who is accessing the config path other than the CPUs.

- Mani

-- 
மணிவண்ணன் சதாசிவம்