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
