Re: [PATCH v4 6/6] arm64: dts: qcom: sm8250: Add OPP table support to UFSHC

[Manivannan Sadhasivam]

On Tue, Oct 03, 2023 at 06:25:22PM +0300, Dmitry Baryshkov wrote:
> On Tue, 3 Oct 2023 at 14:16, Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> >
> > UFS host controller, when scaling gears, should choose appropriate
> > performance state of RPMh power domain controller along with clock
> > frequency. So let's add the OPP table support to specify both clock
> > frequency and RPMh performance states replacing the old "freq-table-hz"
> > property.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8250.dtsi | 39 +++++++++++++++++++++-------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > index a4e58ad731c3..33abd84aae53 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > @@ -2198,21 +2198,42 @@ ufs_mem_hc: ufshc@1d84000 {
> >                                 <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
> >                                 <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
> >                                 <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
> > -                       freq-table-hz =
> > -                               <37500000 300000000>,
> > -                               <0 0>,
> > -                               <0 0>,
> > -                               <37500000 300000000>,
> > -                               <0 0>,
> > -                               <0 0>,
> > -                               <0 0>,
> > -                               <0 0>;
> > +
> > +                       operating-points-v2 = <&ufs_opp_table>;
> >
> >                         interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>,
> >                                         <&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
> >                         interconnect-names = "ufs-ddr", "cpu-ufs";
> >
> >                         status = "disabled";
> > +
> > +                       ufs_opp_table: opp-table {
> > +                               compatible = "operating-points-v2";
> > +
> > +                               opp-37500000 {
> > +                                       opp-hz = /bits/ 64 <37500000>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <37500000>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>;
> 
> I must say I still consider this to be uglier than hard coding clock
> names in the driver.
> 

It is all about choosing the less uglier one... First of all, it is not a good
practice to hardcode clk names in the driver as the driver has to trust what is
being supplied from DT. Also, the OPP support is added in the generic
"ufshcd-platfrm" driver. Now for getting the clk names, I need to introduce a
method to pass the names from the vendor drivers. There are already many such
methods going between these two drivers making it messy and adding one more
would only add up the worse.

So I'd like to stick to this approach.

- Mani

> > +                                       required-opps = <&rpmhpd_opp_low_svs>;
> > +                               };
> > +
> > +                               opp-300000000 {
> > +                                       opp-hz = /bits/ 64 <300000000>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <300000000>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>,
> > +                                                /bits/ 64 <0>;
> > +                                       required-opps = <&rpmhpd_opp_nom>;
> > +                               };
> > +                       };
> >                 };
> >
> >                 ufs_mem_phy: phy@1d87000 {
> > --
> > 2.25.1
> >
> 
> 
> -- 
> With best wishes
> Dmitry

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