RE: [PATCH v12 2/2] phy: samsung-ufs: add UFS PHY driver for samsung SoC

[Alim Akhtar]

Hi Vinod,

> -----Original Message-----
> From: Vinod Koul <vkoul@xxxxxxxxxx>
> Sent: 13 July 2020 11:48
> To: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; krzk@xxxxxxxxxx; kwmad.kim@xxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; kishon@xxxxxx
> Subject: Re: [PATCH v12 2/2] phy: samsung-ufs: add UFS PHY driver for
samsung
> SoC
> 
> On 03-07-20, 22:41, Alim Akhtar wrote:
> 
> > +static const struct samsung_ufs_phy_cfg exynos7_post_init_cfg[] = {
> > +	END_UFS_PHY_CFG
> > +};
> 
> This is dummy, why not add a check to make config optional?
> 
Currently this is dummy, however this might be used for the similar platform
which do some phy tunning post init.
Will just remove this for now for this platform, will add this check in
driver.

> > +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy
> > +*phy) {
> > +	int ret = 0;
> 
> superfluous init, am sure I flagged it before as well
> 
Yes, you did, but 0-DAY CI kernel test gave warning [1], so I kept this as
it is.
[1] https://lkml.org/lkml/2020/7/3/81

> > +
> > +	phy->tx0_symbol_clk = devm_clk_get(phy->dev, "tx0_symbol_clk");
> > +	if (IS_ERR(phy->tx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get tx0_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	phy->rx0_symbol_clk = devm_clk_get(phy->dev, "rx0_symbol_clk");
> > +	if (IS_ERR(phy->rx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get rx0_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	phy->rx1_symbol_clk = devm_clk_get(phy->dev, "rx1_symbol_clk");
> > +	if (IS_ERR(phy->rx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get rx1_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	ret = clk_prepare_enable(phy->tx0_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: tx0_symbol_clk enable failed %d\n",
> __func__, ret);
> > +		goto out;
> > +	}
> > +
> > +	ret = clk_prepare_enable(phy->rx0_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: rx0_symbol_clk enable failed %d\n",
> __func__, ret);
> > +		clk_disable_unprepare(phy->tx0_symbol_clk);
> > +		goto out;
> > +	}
> > +
> > +	ret = clk_prepare_enable(phy->rx1_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: rx1_symbol_clk enable failed %d\n",
> __func__, ret);
> > +		clk_disable_unprepare(phy->tx0_symbol_clk);
> > +		clk_disable_unprepare(phy->rx0_symbol_clk);
> 
> maybe it will look better if we add common rollback and jump to proper
labels
> 
Sure, will change in next version.

> > +static int samsung_ufs_phy_clks_init(struct samsung_ufs_phy *phy) {
> > +	int ret;
> > +
> > +	phy->ref_clk = devm_clk_get(phy->dev, "ref_clk");
> > +	if (IS_ERR(phy->ref_clk))
> > +		dev_err(phy->dev, "failed to get ref_clk clock\n");
> > +
> > +	ret = clk_prepare_enable(phy->ref_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: ref_clk enable failed %d\n",
__func__,
> ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_info(phy->dev, "UFS MPHY ref_clk_rate = %ld\n",
> > +clk_get_rate(phy->ref_clk));
> 
> debug pls
> 
Sure, will change

> > +static int samsung_ufs_phy_init(struct phy *phy) {
> > +	struct samsung_ufs_phy *_phy = get_samsung_ufs_phy(phy);
> 
> ss_phy perhaps?
> 
Sure, will change 

> > +	int ret;
> > +
> > +	_phy->lane_cnt = phy->attrs.bus_width;
> > +	_phy->ufs_phy_state = CFG_PRE_INIT;
> > +
> > +	if (_phy->drvdata->has_symbol_clk) {
> > +		ret = samsung_ufs_phy_symbol_clk_init(_phy);
> > +		if (ret)
> > +			dev_err(_phy->dev, "failed to set ufs phy symbol
> clocks\n");
> > +	}
> > +
> > +	ret = samsung_ufs_phy_clks_init(_phy);
> > +	if (ret)
> > +		dev_err(_phy->dev, "failed to set ufs phy  clocks\n");
> > +
> > +	samsung_ufs_phy_calibrate(phy);
> > +
> > +	return 0;
> 
> not return samsung_ufs_phy_calibrate() ?
> --
Will add an error path.

> ~Vinod