RE: [PATCH v4 5/5] arm64: dts: Add node for ufs exynos7

From: "Alim Akhtar" <alim.akhtar@samsung.com>
To: "'Paweł Chmiel'" <pawel.mikolaj.chmiel@gmail.com>,
	robh+dt@kernel.org, devicetree@vger.kernel.org,
	linux-scsi@vger.kernel.org
Cc: <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>
Subject: RE: [PATCH v4 5/5] arm64: dts: Add node for ufs exynos7
Date: Sun, 29 Mar 2020 21:05:28 +0530	[thread overview]
Message-ID: <002e01d605df$af658440$0e308cc0$@samsung.com> (raw)
In-Reply-To: <838a17416b4ed59903ae153e09842ac62584616f.camel@gmail.com>

Hi Pawel

> -----Original Message-----
> From: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> Sent: 28 March 2020 23:17
> To: Alim Akhtar <alim.akhtar@samsung.com>; robh+dt@kernel.org;
> devicetree@vger.kernel.org; linux-scsi@vger.kernel.org
> Cc: 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
> Subject: Re: [PATCH v4 5/5] arm64: dts: Add node for ufs exynos7
> 
> On Sat, 2020-03-28 at 21:05 +0530, Alim Akhtar wrote:
> > Hi Pawel
> >
> > > -----Original Message-----
> > > From: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > Sent: 28 March 2020 19:00
> > > To: Alim Akhtar <alim.akhtar@samsung.com>; robh+dt@kernel.org;
> > > devicetree@vger.kernel.org; linux-scsi@vger.kernel.org
> > > Cc: 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
> > > Subject: Re: [PATCH v4 5/5] arm64: dts: Add node for ufs exynos7
> > >
> > > On Fri, 2020-03-27 at 22:36 +0530, Alim Akhtar wrote:
> > > > Adding dt node foe UFS and UFS-PHY for exynos7 SoC.
> > > >
> > > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> > > > ---
> > > >  .../boot/dts/exynos/exynos7-espresso.dts      | 16 +++++++
> > > >  arch/arm64/boot/dts/exynos/exynos7.dtsi       | 43 ++++++++++++++++++-
> > > >  2 files changed, 57 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts
> > > > b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts
> > > > index 7af288fa9475..b59a0a32620a 100644
> > > > --- a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts
> > > > +++ b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts
> > > > @@ -406,6 +406,22 @@
> > > >  	};
> > > >  };
> > > >
> > > > +&ufs {
> > > > +	status = "okay";
> > > > +	pinctrl-names = "default";
> > > > +	pinctrl-0 = <&ufs_rst_n &ufs_refclk_out>;
> > > > +	ufs,pwr-attr-mode = "FAST";
> > > > +	ufs,pwr-attr-lane = <2>;
> > > > +	ufs,pwr-attr-gear = <2>;
> > > > +	ufs,pwr-attr-hs-series = "HS_rate_b";
> > > > +	ufs-rx-adv-fine-gran-sup_en = <1>;
> > > > +	ufs-rx-adv-fine-gran-step = <3>;
> > > > +	ufs-rx-adv-min-activate-time-cap = <9>;
> > > > +	ufs-pa-granularity = <6>;
> > > > +	ufs-pa-tacctivate = <3>;
> > > > +	ufs-pa-hibern8time = <20>;
> > > > +};
> > > > +
> > > >  &usbdrd_phy {
> > > >  	vbus-supply = <&usb30_vbus_reg>;
> > > >  	vbus-boost-supply = <&usb3drd_boost_5v>; diff --git
> > > > a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> > > > b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> > > > index 5558045637ac..9d16c90edd07 100644
> > > > --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> > > > +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> > > > @@ -220,9 +220,14 @@
> > > >  			#clock-cells = <1>;
> > > >  			clocks = <&fin_pll>, <&clock_top1
> > > DOUT_ACLK_FSYS1_200>,
> > > >  				 <&clock_top1 DOUT_SCLK_MMC0>,
> > > > -				 <&clock_top1 DOUT_SCLK_MMC1>;
> > > > +				 <&clock_top1 DOUT_SCLK_MMC1>,
> > > > +				 <&clock_top1 DOUT_SCLK_UFSUNIPRO20>,
> > > > +				 <&clock_top1 DOUT_SCLK_PHY_FSYS1>,
> > > > +				 <&clock_top1 DOUT_SCLK_PHY_FSYS1_26M>;
> > > >  			clock-names = "fin_pll", "dout_aclk_fsys1_200",
> > > > -				      "dout_sclk_mmc0", "dout_sclk_mmc1";
> > > > +				      "dout_sclk_mmc0", "dout_sclk_mmc1",
> > > > +				      "dout_sclk_ufsunipro20",
> > > "dout_sclk_phy_fsys1",
> > > > +				      "dout_sclk_phy_fsys1_26m";
> > > >  		};
> > > >
> > > >  		serial_0: serial@13630000 {
> > > > @@ -601,6 +606,40 @@
> > > >  			};
> > > >  		};
> > > >
> > > > +		ufs: ufs@15570000 {
> > > > +			compatible = "samsung,exynos7-ufs";
> > > > +			#address-cells = <1>;
> > > > +			#size-cells = <1>;
> > > > +			ranges;
> > > > +			reg = <0x15570000 0x100>,  /* 0: HCI standard */
> > > > +				<0x15570100 0x100>,  /* 1: Vendor specificed
> > > */
> > > > +				<0x15571000 0x200>,  /* 2: UNIPRO */
> > > > +				<0x15572000 0x300>;  /* 3: UFS protector */
> > > > +			reg-names = "hci", "vs_hci", "unipro", "ufsp";
> > > > +			interrupts = <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>;
> > > > +			clocks = <&clock_fsys1 ACLK_UFS20_LINK>,
> > > > +				<&clock_fsys1 SCLK_UFSUNIPRO20_USER>;
> > > > +			clock-names = "core_clk", "sclk_unipro_main";
> > > > +			freq-table-hz = <0 0>, <0 0>;
> > > > +			pclk-freq-avail-range = <70000000 133000000>;
> > > > +			ufs,pwr-local-l2-timer = <8000 28000 20000>;
> > > > +			ufs,pwr-remote-l2-timer = <12000 32000 16000>;
> > > > +			phys = <&ufs_phy>;
> > > > +			phy-names = "ufs-phy";
> > > > +			status = "disabled";
> > > > +		};
> > > > +
> > > > +		ufs_phy: ufs-phy@0x15571800 {
> > > > +			compatible = "samsung,exynos7-ufs-phy";
> > > > +			reg = <0x15571800 0x240>;
> > > > +			reg-names = "phy-pma";
> > > > +			samsung,pmu-syscon = <&pmu_system_controller>;
> > > > +			#phy-cells = <0>;
> > > > +			clocks = <&clock_fsys1 MOUT_FSYS1_PHYCLK_SEL1>,
> > > > +				<&clock_top1 CLK_SCLK_PHY_FSYS1_26M>;
> > > > +			clock-names = "ref_clk_parent", "ref_clk";
> > > Hi
> > > Is this correct (aren't child and parent clock swapped)?
> > > This will set MOUT_FSYS1_PHYCLK_SEL1 to be parent clock of
> > > CLK_SCLK_PHY_FSYS1_26M.
> >
> > Looks like in one of my rebase it got swap, will correct it.  Thanks for pointing
> out.
> >
> > > I've tested this on Exynos7420 (which looks like can use the same
> > > clock driver as
> > > exynos7) and after adding some debug code (because currently we're
> > > not handling result of samsung_ufs_phy_clks_init) i got
> > >
> > > samsung-ufs-phy 15571800.ufs-phy: clk_set_parent result: -22
> > >
> > I will check if I overlooked this error.
> > > On vendor sources for this device, driver was setting
> > > CLK_SCLK_PHY_FSYS1_26M to be parent of MOUT_FSYS1_PHYCLK_SEL1,
> and then it did run without error.
> > >
> > > samsung-ufs-phy 15571800.ufs-phy: clk_set_parent result: 0
> > >
> > With this change, does linkup worked for you?
> Hi
> Sadly not yet, so someone needs to check it on different hw.
> 
> I've added some debug code to ufshcd and it looks like it fails (in my
> case) at ufshcd_dme_link_startup which returns 1 (because
> ufshcd_wait_for_uic_cmd returns 1). The same for second retry and at third
> retry it's getting -110 from ufshcd_wait_for_uic_cmd.
> 
Mostly device is not responding to any UIC commands.

> Need to check:
> - in vendor there was 10 clocks used by ufs/ufs-phy (where this driver uses 4)
> - if calibration is the same in this driver vs vendor
> 
All clocks are not mandatory, what I have mentioned is only UFS HCI core clock, unipro clock and MPHY clocks.

> Maybe it's because of missing EXYNOS FMP Driver (my device has
> secureos) and/or some missing smc calls (like in case of smu config)?
FMP will come into picture a little later, it does not affect _link_startup though.
SMU does matter, so make sure SMU is by passed initially.

Another think that comes into my mind is, if possible just disabled PMIC (intension is to keep all the rails __always_on__)
The reason is sometime UFS_RST_N which is hooked to RESET_N of the UFS device is also controlled via one of the PMIC LDO.
(In your case I don't know which LDO, so keep all the rails always ON).
Also cross check if you have these gpios configured properly.
pinctrl-0 = <&ufs_rst_n &ufs_refclk_out>;
Give it a try.

> >
> > > Also looking at clk-exynos7 driver seems to confirm this.
> > >
> > > > +		};
> > > > +
> > > >  		usbdrd_phy: phy@15500000 {
> > > >  			compatible = "samsung,exynos7-usbdrd-phy";
> > > >  			reg = <0x15500000 0x100>;
> >
> >