Re: [RESEND PATCH v10 2/2] phy: samsung-ufs: add UFS PHY driver for samsung SoC

[Vinod Koul <vkoul@kernel.org>]

Hi Alim,

On 25-06-20, 05:26, Alim Akhtar wrote:

> +int samsung_ufs_phy_wait_for_lock_acq(struct phy *phy)

static ?

> +{
> +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> +	const unsigned int timeout_us = 100000;
> +	const unsigned int sleep_us = 10;
> +	u32 val;
> +	int err;
> +
> +	err = readl_poll_timeout(
> +			ufs_phy->reg_pma + PHY_APB_ADDR(PHY_PLL_LOCK_STATUS),
> +			val, (val & PHY_PLL_LOCK_BIT), sleep_us, timeout_us);
> +	if (err) {
> +		dev_err(ufs_phy->dev,
> +			"failed to get phy pll lock acquisition %d\n", err);
> +		goto out;
> +	}
> +
> +	err = readl_poll_timeout(
> +			ufs_phy->reg_pma + PHY_APB_ADDR(PHY_CDR_LOCK_STATUS),
> +			val, (val & PHY_CDR_LOCK_BIT), sleep_us, timeout_us);
> +	if (err) {
> +		dev_err(ufs_phy->dev,
> +			"failed to get phy cdr lock acquisition %d\n", err);
> +		goto out;

this one can be dropped

> +	}
> +
> +out:
> +	return err;
> +}
> +
> +int samsung_ufs_phy_calibrate(struct phy *phy)

static?

> +{
> +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> +	struct samsung_ufs_phy_cfg **cfgs = ufs_phy->cfg;
> +	const struct samsung_ufs_phy_cfg *cfg;
> +	int i;
> +	int err = 0;

err before i would make it look better

> +
> +	if (unlikely(ufs_phy->ufs_phy_state < CFG_PRE_INIT ||
> +		     ufs_phy->ufs_phy_state >= CFG_TAG_MAX)) {
> +		dev_err(ufs_phy->dev, "invalid phy config index %d\n",
> +							ufs_phy->ufs_phy_state);

single line now?

> +		return -EINVAL;
> +	}
> +
> +	if (ufs_phy->is_pre_init)
> +		ufs_phy->is_pre_init = false;

that sounds bit strange, you clear it if set? Can you explain what is
going on here, and add comments

> +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy *phy)
> +{
> +	int ret = 0;

superfluous init

> +
> +	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);

so we keep tx0_symbol_clk enabled when bailing out?

> +		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);

here as well

> +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;
> +
> +	/**
> +	 * In ufs, PHY need to be calibrated at different stages / state
> +	 * mainly before Linkstartup, after Linkstartup, before power
> +	 * mode change and after power mode change.
> +	 * Below state machine initialize the initial state to handle
> +	 * PHY calibration at various stages of UFS initialization and power
> +	 * mode changes
> +	 */
> +	_phy->is_pre_init = true;
> +	_phy->is_post_init = false;
> +	_phy->is_pre_pmc = false;
> +	_phy->is_post_pmc = false;

hmm why not have phy_state and assign that
pre_init/post_init/pre_pmc/post_pmc states?

> +static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
> +					enum phy_mode mode, int submode)

pls align this to preceding line opening brace (tip: checkpatch with
--strict can tell you about these)
-- 
~Vinod