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
