Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Alim Akhtar" <alim.akhtar@samsung.com>
To:     "'Kishon Vijay Abraham I'" <kishon@ti.com>, <robh@kernel.org>
Cc:     <devicetree@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
        <krzk@kernel.org>, <avri.altman@wdc.com>,
        <martin.petersen@oracle.com>, <kwmad.kim@samsung.com>,
        <stanley.chu@mediatek.com>, <cang@codeaurora.org>,
        <linux-samsung-soc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, "'Vinod Koul'" <vkoul@kernel.org>
In-Reply-To: <b0239aa5-004e-fc88-93a4-5b0d6f174ca3@ti.com>
Subject: RE: [PATCH v7 07/10] phy: samsung-ufs: add UFS PHY driver for
 samsung SoC
Date:   Sat, 9 May 2020 06:02:36 +0530
Message-ID: <006701d62599$5fbc2c80$1f348580$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQINaKJiJYqhfY8pF8dQGQjTbFgO/QI75FnYAdd4tKsCTMaX9af9XhAA
Content-Language: en-in
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <20200426173024.63069-1-alim.akhtar@samsung.com>
        <CGME20200426174217epcas5p2c7d1606b641b73f67a169b8d22f0637d@epcas5p2.samsung.com>
        <20200426173024.63069-8-alim.akhtar@samsung.com>
        <b0239aa5-004e-fc88-93a4-5b0d6f174ca3@ti.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kishon,
Thanks for review.

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon=40ti.com>
> Sent: 07 May 2020 10:49
> To: Alim Akhtar <alim.akhtar=40samsung.com>; robh=40kernel.org
> Cc: devicetree=40vger.kernel.org; linux-scsi=40vger.kernel.org; krzk=40ke=
rnel.org;
> avri.altman=40wdc.com; martin.petersen=40oracle.com;
> kwmad.kim=40samsung.com; stanley.chu=40mediatek.com;
> cang=40codeaurora.org; linux-samsung-soc=40vger.kernel.org; linux-arm-
> kernel=40lists.infradead.org; linux-kernel=40vger.kernel.org; Vinod Koul
> <vkoul=40kernel.org>
> Subject: Re: =5BPATCH v7 07/10=5D phy: samsung-ufs: add UFS PHY driver fo=
r
> samsung SoC
>=20
=2E
=2E
=2E
> Okay, here you are using a state machine for the PHY configuration becaus=
e of
> the way the PHY is integrated with the UFS. Would be nice to have the sta=
te
> 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 =3D=3D CFG_POST_PWR_HS)
> > +		err =3D samsung_ufs_phy_wait_for_lock_acq(phy);
> > +out:
> > +	return err;
> > +=7D
> > +
> > +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy
> > +*phy) =7B
> > +	struct clk *clk;
> > +	int ret =3D 0;
> > +
> > +	clk =3D devm_clk_get(phy->dev, =22tx0_symbol_clk=22);
>=20
> There is no =22exit=22 callback in phy_ops which means if there are multi=
ple phy_init
> calls, this clock will not be freed. This could be moved to =22probe=22 I=
MO.

Ok, will add exit callback.

> > +	if (IS_ERR(clk)) =7B
> > +		dev_err(phy->dev, =22failed to get tx0_symbol_clk clock=5Cn=22);
> > +		goto out;
> > +	=7D else =7B
>=20
> =22else=22 here and below is not required. Something like below
>=20
Ack
> 	clk =3D devm_clk_get(phy->dev, =22tx0_symbol_clk=22);
> 	if (IS_ERR(clk)) =7B
> 		dev_err(phy->dev, =22failed to get tx0_symbol_clk clock=5Cn=22);
> 		goto out;
> 	=7D
> 	phy->tx0_symbol_clk =3D clk;
>=20
> > +		phy->tx0_symbol_clk =3D clk;
> > +	=7D
> > +
> > +	clk =3D devm_clk_get(phy->dev, =22rx0_symbol_clk=22);
> > +	if (IS_ERR(clk)) =7B
> > +		dev_err(phy->dev, =22failed to get rx0_symbol_clk clock=5Cn=22);
> > +		goto out;
> > +	=7D else =7B
> > +		phy->rx0_symbol_clk =3D clk;
> > +	=7D
> > +
> > +	clk =3D devm_clk_get(phy->dev, =22rx1_symbol_clk=22);
> > +	if (IS_ERR(clk)) =7B
> > +		dev_err(phy->dev, =22failed to get rx1_symbol_clk clock=5Cn=22);
> > +		goto out;
> > +	=7D else =7B
> > +		phy->rx1_symbol_clk =3D clk;
> > +	=7D
> > +
> > +	ret =3D clk_prepare_enable(phy->tx0_symbol_clk);
> > +	if (ret) =7B
> > +		dev_err(phy->dev, =22%s: tx0_symbol_clk enable failed %d=5Cn=22,
> > +				__func__, ret);
> > +		goto out;
> > +	=7D
> > +	ret =3D clk_prepare_enable(phy->rx0_symbol_clk);
> > +	if (ret) =7B
> > +		dev_err(phy->dev, =22%s: rx0_symbol_clk enable failed %d=5Cn=22,
> > +				__func__, ret);
> > +		goto out;
> > +	=7D
> > +	ret =3D clk_prepare_enable(phy->rx1_symbol_clk);
> > +	if (ret) =7B
> > +		dev_err(phy->dev, =22%s: rx1_symbol_clk enable failed %d=5Cn=22,
> > +				__func__, ret);
> > +		goto out;
> > +	=7D
>=20
> All these clocks are never disabled?
Sure, will add disabling of clocks in exit callback=20

> > +out:
> > +	return ret;
> > +=7D
> > +
> > +static int samsung_ufs_phy_clks_init(struct samsung_ufs_phy *phy) =7B
> > +	struct clk *phy_ref_clk;
> > +	int ret;
> > +
> > +	phy_ref_clk =3D devm_clk_get(phy->dev, =22ref_clk=22);
> > +	if (IS_ERR(phy_ref_clk))
> > +		dev_err(phy->dev, =22failed to get ref_clk clock=5Cn=22);
> > +	else
> > +		phy->ref_clk =3D phy_ref_clk;
> > +
> > +	ret =3D clk_prepare_enable(phy->ref_clk);
> > +	if (ret) =7B
> > +		dev_err(phy->dev, =22%s: ref_clk enable failed %d=5Cn=22,
> > +				__func__, ret);
> > +		return ret;
> > +	=7D
> > +
> > +	dev_info(phy->dev, =22UFS MPHY ref_clk_rate =3D %ld=5Cn=22,
> > +clk_get_rate(phy_ref_clk));
> > +
> > +	return 0;
> > +=7D
> > +
> > +static int samsung_ufs_phy_init(struct phy *phy) =7B
> > +	struct samsung_ufs_phy *_phy =3D get_samsung_ufs_phy(phy);
> > +	int ret;
> > +
> > +	_phy->lane_cnt =3D phy->attrs.bus_width;
> > +	_phy->ufs_phy_state =3D CFG_PRE_INIT;
> > +
> > +	_phy->is_pre_init =3D true;
> > +	_phy->is_post_init =3D false;
> > +	_phy->is_pre_pmc =3D false;
> > +	_phy->is_post_pmc =3D false;
> > +
> > +
> > +	if (of_device_is_compatible(_phy->dev->of_node,
> > +				=22samsung,exynos7-ufs-phy=22)) =7B
>=20
> Can't it be added in driver data for this compatible?
Sure, will handle via driver data.

> > +		ret =3D samsung_ufs_phy_symbol_clk_init(_phy);
> > +		if (ret)
> > +			dev_err(_phy->dev,
> > +				=22failed to set ufs phy symbol clocks=5Cn=22);
> > +	=7D
> > +
=2E
=2E
=2E
> > +static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
> > +					enum phy_mode mode, int submode) =7B
> > +	struct samsung_ufs_phy *_phy =3D get_samsung_ufs_phy(generic_phy);
> > +
> > +	_phy->mode =3D PHY_MODE_INVALID;
> > +
> > +	if (mode > 0)
> > +		_phy->mode =3D mode;
> > +
> > +	return 0;
> > +=7D
> > +
> > +static struct phy_ops samsung_ufs_phy_ops =3D =7B
> > +	.init		=3D samsung_ufs_phy_init,
> > +	.power_on	=3D samsung_ufs_phy_power_on,
> > +	.power_off	=3D samsung_ufs_phy_power_off,
> > +	.calibrate	=3D samsung_ufs_phy_calibrate,
> > +	.set_mode	=3D samsung_ufs_phy_set_mode,
>=20
> missing .owner.
Ack,

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

> Thanks
> Kishon