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;
Date:   Tue, 19 May 2020 12:22:51 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Srinath Mannam <srinath.mannam@broadcom.com>,
        linux-kernel@vger.kernel.org,
        Bharat Gooty <bharat.gooty@broadcom.com>
Subject: Re: [PATCH v1 1/1] drivers: phy: sr-usb: do not use internal fsm for
 USB2 phy init
Message-ID: <20200519065251.GK374218@vkoul-mobl.Dlink>
References: <20200513173947.10919-1-rayagonda.kokatanur@broadcom.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200513173947.10919-1-rayagonda.kokatanur@broadcom.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 13-05-20, 23:09, Rayagonda Kokatanur wrote:
> From: Bharat Gooty <bharat.gooty@broadcom.com>
> 
> During different reboot cycles, USB PHY PLL may not always lock
> during initialization and therefore can cause USB to be not usable.

Ok

> Hence do not use internal FSM programming sequence for the USB
> PHY initialization.

And what is the impact of not using FSM programming sequence? If not
impact why was it added in the first place ?

> Fixes: 4dcddbb38b64 ("phy: sr-usb: Add Stingray USB PHY driver")
> Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  drivers/phy/broadcom/phy-bcm-sr-usb.c | 55 +--------------------------
>  1 file changed, 2 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/phy/broadcom/phy-bcm-sr-usb.c b/drivers/phy/broadcom/phy-bcm-sr-usb.c
> index fe6c58910e4c..7c7862b4f41f 100644
> --- a/drivers/phy/broadcom/phy-bcm-sr-usb.c
> +++ b/drivers/phy/broadcom/phy-bcm-sr-usb.c
> @@ -16,8 +16,6 @@ enum bcm_usb_phy_version {
>  };
>  
>  enum bcm_usb_phy_reg {
> -	PLL_NDIV_FRAC,
> -	PLL_NDIV_INT,
>  	PLL_CTRL,
>  	PHY_CTRL,
>  	PHY_PLL_CTRL,
> @@ -31,18 +29,11 @@ static const u8 bcm_usb_combo_phy_ss[] = {
>  };
>  
>  static const u8 bcm_usb_combo_phy_hs[] = {
> -	[PLL_NDIV_FRAC]	= 0x04,
> -	[PLL_NDIV_INT]	= 0x08,
>  	[PLL_CTRL]	= 0x0c,
>  	[PHY_CTRL]	= 0x10,
>  };
>  
> -#define HSPLL_NDIV_INT_VAL	0x13
> -#define HSPLL_NDIV_FRAC_VAL	0x1005
> -
>  static const u8 bcm_usb_hs_phy[] = {
> -	[PLL_NDIV_FRAC]	= 0x0,
> -	[PLL_NDIV_INT]	= 0x4,
>  	[PLL_CTRL]	= 0x8,
>  	[PHY_CTRL]	= 0xc,
>  };
> @@ -52,7 +43,6 @@ enum pll_ctrl_bits {
>  	SSPLL_SUSPEND_EN,
>  	PLL_SEQ_START,
>  	PLL_LOCK,
> -	PLL_PDIV,
>  };
>  
>  static const u8 u3pll_ctrl[] = {
> @@ -66,29 +56,17 @@ static const u8 u3pll_ctrl[] = {
>  #define HSPLL_PDIV_VAL		0x1
>  
>  static const u8 u2pll_ctrl[] = {
> -	[PLL_PDIV]	= 1,
>  	[PLL_RESETB]	= 5,
>  	[PLL_LOCK]	= 6,
>  };
>  
>  enum bcm_usb_phy_ctrl_bits {
>  	CORERDY,
> -	AFE_LDO_PWRDWNB,
> -	AFE_PLL_PWRDWNB,
> -	AFE_BG_PWRDWNB,
> -	PHY_ISO,
>  	PHY_RESETB,
>  	PHY_PCTL,
>  };
>  
>  #define PHY_PCTL_MASK	0xffff
> -/*
> - * 0x0806 of PCTL_VAL has below bits set
> - * BIT-8 : refclk divider 1
> - * BIT-3:2: device mode; mode is not effect
> - * BIT-1: soft reset active low
> - */
> -#define HSPHY_PCTL_VAL	0x0806
>  #define SSPHY_PCTL_VAL	0x0006
>  
>  static const u8 u3phy_ctrl[] = {
> @@ -98,10 +76,6 @@ static const u8 u3phy_ctrl[] = {
>  
>  static const u8 u2phy_ctrl[] = {
>  	[CORERDY]		= 0,
> -	[AFE_LDO_PWRDWNB]	= 1,
> -	[AFE_PLL_PWRDWNB]	= 2,
> -	[AFE_BG_PWRDWNB]	= 3,
> -	[PHY_ISO]		= 4,
>  	[PHY_RESETB]		= 5,
>  	[PHY_PCTL]		= 6,
>  };
> @@ -186,38 +160,13 @@ static int bcm_usb_hs_phy_init(struct bcm_usb_phy_cfg *phy_cfg)
>  	int ret = 0;
>  	void __iomem *regs = phy_cfg->regs;
>  	const u8 *offset;
> -	u32 rd_data;
>  
>  	offset = phy_cfg->offset;
>  
> -	writel(HSPLL_NDIV_INT_VAL, regs + offset[PLL_NDIV_INT]);
> -	writel(HSPLL_NDIV_FRAC_VAL, regs + offset[PLL_NDIV_FRAC]);
> -
> -	rd_data = readl(regs + offset[PLL_CTRL]);
> -	rd_data &= ~(HSPLL_PDIV_MASK << u2pll_ctrl[PLL_PDIV]);
> -	rd_data |= (HSPLL_PDIV_VAL << u2pll_ctrl[PLL_PDIV]);
> -	writel(rd_data, regs + offset[PLL_CTRL]);
> -
> -	/* Set Core Ready high */
> -	bcm_usb_reg32_setbits(regs + offset[PHY_CTRL],
> -			      BIT(u2phy_ctrl[CORERDY]));
> -
> -	/* Maximum timeout for Core Ready done */
> -	msleep(30);
> -
> +	bcm_usb_reg32_clrbits(regs + offset[PLL_CTRL],
> +			      BIT(u2pll_ctrl[PLL_RESETB]));
>  	bcm_usb_reg32_setbits(regs + offset[PLL_CTRL],
>  			      BIT(u2pll_ctrl[PLL_RESETB]));
> -	bcm_usb_reg32_setbits(regs + offset[PHY_CTRL],
> -			      BIT(u2phy_ctrl[PHY_RESETB]));
> -
> -
> -	rd_data = readl(regs + offset[PHY_CTRL]);
> -	rd_data &= ~(PHY_PCTL_MASK << u2phy_ctrl[PHY_PCTL]);
> -	rd_data |= (HSPHY_PCTL_VAL << u2phy_ctrl[PHY_PCTL]);
> -	writel(rd_data, regs + offset[PHY_CTRL]);
> -
> -	/* Maximum timeout for PLL reset done */
> -	msleep(30);
>  
>  	ret = bcm_usb_pll_lock_check(regs + offset[PLL_CTRL],
>  				     BIT(u2pll_ctrl[PLL_LOCK]));
> -- 
> 2.17.1

-- 
~Vinod