Re: [PATCH v3 09/24] scsi: ufs: mediatek: Rework the crypt-boost stuff

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Tuesday, 4 November 2025 08:28:18 Central European Standard Time Chaotian Jing (井朝天) wrote:
> On Thu, 2025-10-23 at 21:49 +0200, Nicolas Frattaroli wrote:
> > I don't know whether the crypt-boost functionality as it is currently
> > implemented is even appropriate for mainline. It might be better done
> > in
> > some generic way. But what I do know is that I can rework the code to
> > make it less obtuse.
> > 
> > Prefix the boost stuff with the appropriate vendor prefix, remove the
> > pointless clock wrappers, and rework the function.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/ufs/host/ufs-mediatek.c | 91 +++++++++++++++--------------
> > ------------
> >  1 file changed, 32 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> > mediatek.c
> > index 131f71145a12..9c0ac72d6e43 100644
> > --- a/drivers/ufs/host/ufs-mediatek.c
> > +++ b/drivers/ufs/host/ufs-mediatek.c
> > @@ -562,21 +562,6 @@ static int ufs_mtk_mphy_power_on(struct ufs_hba
> > *hba, bool on)
> >  	return 0;
> >  }
> >  
> > -static int ufs_mtk_get_host_clk(struct device *dev, const char
> > *name,
> > -				struct clk **clk_out)
> > -{
> > -	struct clk *clk;
> > -	int err = 0;
> > -
> > -	clk = devm_clk_get(dev, name);
> > -	if (IS_ERR(clk))
> > -		err = PTR_ERR(clk);
> > -	else
> > -		*clk_out = clk;
> > -
> > -	return err;
> > -}
> > -
> >  static void ufs_mtk_boost_crypt(struct ufs_hba *hba, bool boost)
> >  {
> >  	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> > @@ -633,65 +618,53 @@ static void ufs_mtk_boost_crypt(struct ufs_hba
> > *hba, bool boost)
> >  	clk_disable_unprepare(cfg->clk_crypt_mux);
> >  }
> >  
> > -static int ufs_mtk_init_host_clk(struct ufs_hba *hba, const char
> > *name,
> > -				 struct clk **clk)
> > -{
> > -	int ret;
> > -
> > -	ret = ufs_mtk_get_host_clk(hba->dev, name, clk);
> > -	if (ret) {
> > -		dev_info(hba->dev, "%s: failed to get %s: %d",
> > __func__,
> > -			 name, ret);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -static void ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
> > +static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
> You change the return type but never checked the return value when
> calling this function ?

Yeah, I should probably check the return in ufs_mtk_init_host_caps
instead of continuing on silently.

> >  {
> >  	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> >  	struct ufs_mtk_crypt_cfg *cfg;
> >  	struct device *dev = hba->dev;
> > -	struct regulator *reg;
> > -	u32 volt;
> > +	int ret;
> >  
> > -	host->crypt = devm_kzalloc(dev, sizeof(*(host->crypt)),
> > -				   GFP_KERNEL);
> > -	if (!host->crypt)
> > -		goto disable_caps;
> > +	cfg = devm_kzalloc(dev, sizeof(*cfg), GFP_KERNEL);
> > +	if (!cfg)
> > +		return -ENOMEM;
> >  
> > -	reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
> > -	if (IS_ERR(reg)) {
> > -		dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> > -			 PTR_ERR(reg));
> > -		goto disable_caps;
> > +	cfg->reg_vcore = devm_regulator_get_optional(dev, "dvfsrc-
> > vcore");
> > +	if (IS_ERR(cfg->reg_vcore)) {
> > +		dev_err(dev, "Failed to get dvfsrc-vcore: %pe", cfg-
> > >reg_vcore);
> Since this regulator is optional, why use dev_err ? and why retune an
> error since you never check the return value ?

Because get_optional doesn't mean what you think it means. It means
the function returns -ENODEV if the regulator is absent, instead of
creating a dummy supply. We want to hard error out if the regulator
is absent, because the regulator is needed.

Whether or not the return code is checked makes no functional
difference in this case. If this function exits early,
UFS_MTK_CAP_BOOST_CRYPT_ENGINE isn't added to the host caps,
and the host->crypt member isn't set to cfg.

The return code may be useful for additional logging, which is not
critical to the correctness of the code.

> > +		return PTR_ERR(cfg->reg_vcore);
> >  	}
> >  
> > -	if (of_property_read_u32(dev->of_node, "boost-crypt-vcore-min",
> > -				 &volt)) {
> > -		dev_info(dev, "failed to get boost-crypt-vcore-min");
> > -		goto disable_caps;
> > +	ret = of_property_read_u32(dev->of_node, "mediatek,boost-crypt-
> > vcore-min",
> > +				   &cfg->vcore_volt);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get mediatek,boost-crypt-vcore-
> > min: %pe\n",
> > +			ERR_PTR(ret));
> > +		return ret;
> >  	}
> >  
> > -	cfg = host->crypt;
> > -	if (ufs_mtk_init_host_clk(hba, "crypt_mux",
> > -				  &cfg->clk_crypt_mux))
> > -		goto disable_caps;
> > +	cfg->clk_crypt_mux = devm_clk_get(dev, "crypt_mux");
> > +	if (IS_ERR(cfg->clk_crypt_mux)) {
> > +		dev_err(dev, "Failed to get clock crypt_mux: %pe\n",
> > cfg->clk_crypt_mux);
> > +		return PTR_ERR(cfg->clk_crypt_mux);
> > +	}
> >  
> > -	if (ufs_mtk_init_host_clk(hba, "crypt_lp",
> > -				  &cfg->clk_crypt_lp))
> > -		goto disable_caps;
> > +	cfg->clk_crypt_lp = devm_clk_get(dev, "crypt_lp");
> > +	if (IS_ERR(cfg->clk_crypt_lp)) {
> > +		dev_err(dev, "Failed to get clock crypt_lp: %pe\n",
> > cfg->clk_crypt_lp);
> > +		return PTR_ERR(cfg->clk_crypt_lp);
> > +	}
> >  
> > -	if (ufs_mtk_init_host_clk(hba, "crypt_perf",
> > -				  &cfg->clk_crypt_perf))
> > -		goto disable_caps;
> > +	cfg->clk_crypt_perf = devm_clk_get(dev, "crypt_perf");
> > +	if (IS_ERR(cfg->clk_crypt_perf)) {
> > +		dev_err(dev, "Failed to get clock crypt_perf: %pe\n",
> > cfg->clk_crypt_perf);
> > +		return PTR_ERR(cfg->clk_crypt_perf);
> > +	}
> >  
> > -	cfg->reg_vcore = reg;
> > -	cfg->vcore_volt = volt;
> > +	host->crypt = cfg;
> >  	host->caps |= UFS_MTK_CAP_BOOST_CRYPT_ENGINE;
> >  
> > -disable_caps:
> > -	return;
> > +	return 0;
> >  }
> >  
> >  static void ufs_mtk_init_host_caps(struct ufs_hba *hba)
> > 
>