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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jul 2020 06:47:28 +0530
From:   "Alim Akhtar" <alim.akhtar@...sung.com>
To:     "'Vinod Koul'" <vkoul@...nel.org>
Cc:     <robh+dt@...nel.org>, <krzk@...nel.org>, <kwmad.kim@...sung.com>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-samsung-soc@...r.kernel.org>, <kishon@...com>
Subject: RE: [PATCH v12 2/2] phy: samsung-ufs: add UFS PHY driver for
 samsung SoC

Hi Vinod,

> -----Original Message-----
> From: Vinod Koul <vkoul@...nel.org>
> Sent: 13 July 2020 11:48
> To: Alim Akhtar <alim.akhtar@...sung.com>
> Cc: robh+dt@...nel.org; krzk@...nel.org; kwmad.kim@...sung.com;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; linux-samsung-soc@...r.kernel.org; kishon@...com
> 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