RE: [PATCH v7 07/10] phy: samsung-ufs: add UFS PHY driver for samsung SoC

[Alim Akhtar]

Hi Kishon,
Thanks for review.

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@xxxxxx>
> Sent: 07 May 2020 10:49
> To: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>; robh@xxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; krzk@xxxxxxxxxx;
> avri.altman@xxxxxxx; martin.petersen@xxxxxxxxxx;
> kwmad.kim@xxxxxxxxxxx; stanley.chu@xxxxxxxxxxxx;
> cang@xxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Vinod Koul
> <vkoul@xxxxxxxxxx>
> Subject: Re: [PATCH v7 07/10] phy: samsung-ufs: add UFS PHY driver for
> samsung SoC
> 
.
.
.
> Okay, here you are using a state machine for the PHY configuration because of
> the way the PHY is integrated with the UFS. Would be nice to have the state
> machine documented somewhere. I only have the PHY patch in my inbox.
Ok, will document in the driver file as well as in the header file.

> > +
> > +	if (ufs_phy->ufs_phy_state == CFG_POST_PWR_HS)
> > +		err = samsung_ufs_phy_wait_for_lock_acq(phy);
> > +out:
> > +	return err;
> > +}
> > +
> > +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy
> > +*phy) {
> > +	struct clk *clk;
> > +	int ret = 0;
> > +
> > +	clk = devm_clk_get(phy->dev, "tx0_symbol_clk");
> 
> There is no "exit" callback in phy_ops which means if there are multiple phy_init
> calls, this clock will not be freed. This could be moved to "probe" IMO.

Ok, will add exit callback.

> > +	if (IS_ERR(clk)) {
> > +		dev_err(phy->dev, "failed to get tx0_symbol_clk clock\n");
> > +		goto out;
> > +	} else {
> 
> "else" here and below is not required. Something like below
> 
Ack
> 	clk = devm_clk_get(phy->dev, "tx0_symbol_clk");
> 	if (IS_ERR(clk)) {
> 		dev_err(phy->dev, "failed to get tx0_symbol_clk clock\n");
> 		goto out;
> 	}
> 	phy->tx0_symbol_clk = clk;
> 
> > +		phy->tx0_symbol_clk = clk;
> > +	}
> > +
> > +	clk = devm_clk_get(phy->dev, "rx0_symbol_clk");
> > +	if (IS_ERR(clk)) {
> > +		dev_err(phy->dev, "failed to get rx0_symbol_clk clock\n");
> > +		goto out;
> > +	} else {
> > +		phy->rx0_symbol_clk = clk;
> > +	}
> > +
> > +	clk = devm_clk_get(phy->dev, "rx1_symbol_clk");
> > +	if (IS_ERR(clk)) {
> > +		dev_err(phy->dev, "failed to get rx1_symbol_clk clock\n");
> > +		goto out;
> > +	} else {
> > +		phy->rx1_symbol_clk = clk;
> > +	}
> > +
> > +	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);
> > +		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);
> > +		goto out;
> > +	}
> 
> All these clocks are never disabled?
Sure, will add disabling of clocks in exit callback 

> > +out:
> > +	return ret;
> > +}
> > +
> > +static int samsung_ufs_phy_clks_init(struct samsung_ufs_phy *phy) {
> > +	struct clk *phy_ref_clk;
> > +	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");
> > +	else
> > +		phy->ref_clk = phy_ref_clk;
> > +
> > +	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));
> > +
> > +	return 0;
> > +}
> > +
> > +static int samsung_ufs_phy_init(struct phy *phy) {
> > +	struct samsung_ufs_phy *_phy = get_samsung_ufs_phy(phy);
> > +	int ret;
> > +
> > +	_phy->lane_cnt = phy->attrs.bus_width;
> > +	_phy->ufs_phy_state = CFG_PRE_INIT;
> > +
> > +	_phy->is_pre_init = true;
> > +	_phy->is_post_init = false;
> > +	_phy->is_pre_pmc = false;
> > +	_phy->is_post_pmc = false;
> > +
> > +
> > +	if (of_device_is_compatible(_phy->dev->of_node,
> > +				"samsung,exynos7-ufs-phy")) {
> 
> Can't it be added in driver data for this compatible?
Sure, will handle via driver data.

> > +		ret = samsung_ufs_phy_symbol_clk_init(_phy);
> > +		if (ret)
> > +			dev_err(_phy->dev,
> > +				"failed to set ufs phy symbol clocks\n");
> > +	}
> > +
.
.
.
> > +static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
> > +					enum phy_mode mode, int submode) {
> > +	struct samsung_ufs_phy *_phy = get_samsung_ufs_phy(generic_phy);
> > +
> > +	_phy->mode = PHY_MODE_INVALID;
> > +
> > +	if (mode > 0)
> > +		_phy->mode = mode;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct phy_ops samsung_ufs_phy_ops = {
> > +	.init		= samsung_ufs_phy_init,
> > +	.power_on	= samsung_ufs_phy_power_on,
> > +	.power_off	= samsung_ufs_phy_power_off,
> > +	.calibrate	= samsung_ufs_phy_calibrate,
> > +	.set_mode	= samsung_ufs_phy_set_mode,
> 
> missing .owner.
Ack,

> > +}
> > +;
.
.
> > +++ b/drivers/phy/samsung/phy-samsung-ufs.h
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UFS PHY driver for Samsung EXYNOS SoC
> > + *
> > + * Copyright (C) 2015 Samsung Electronics Co., Ltd.
> 
> 2020
> 
Sure, will update.

> Thanks
> Kishon