Please start commit subjects with a capital letter after the prefix.
Also, please avoid t210 as abbreviation and use tegra210 instead.

The above should be something like:

    phy: tegra: xusb: tegra210: Rearrange UPHY init

Or perhaps:

    phy: tegra: xusb: Rearrange UPHY init on Tegra210

On Mon, Aug 31, 2020 at 12:40:34PM +0800, JC Kuo wrote:
> This commit is a preparation for enabling XUSB SC7 support.
> It rearranges Tegra210 XUSB PADCTL UPHY initialization sequence,
> for the following reasons:
> 
> 1. PLLE hardware power sequencer has to be enabled only after both
>    PEX UPHY PLL and SATA UPHY PLL are initialized.
>    tegra210_uphy_init() -> tegra210_pex_uphy_enable()
>                         -> tegra210_sata_uphy_enable()
>                         -> tegra210_plle_hw_sequence_start()
>                         -> tegra210_aux_mux_lp0_clamp_disable()
> 
> 2. Once UPHY PLL hardware power sequencer is enabled, do not assert
>    reset to PEX/SATA PLLs, otherwise UPHY PLL operation will be
>    broken.
>    reset_control_assert(pcie->rst) and reset_control_assert(sata->rst)
>    are removed from PEX/SATA UPHY disable procedure.
> 
> 3. At cold boot and SC7 exit, the following bits must be cleared after
>    PEX/SATA lanes are out of IDDQ (IDDQ_DISABLE=1).
>    a. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
>    b. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY
>    c. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN
> 
>    tegra210_pex_uphy_enable() and tegra210_sata_uphy_enable() are in
>    charge of bringing lanes out of IDDQ, and then AUX_MUX_LP0_* bits
>    will be cleared by tegra210_aux_mux_lp0_clamp_disable().
> 
> 4. The programming sequence in tegra210_usb3_port_enable() is required
>    for both cold boot and SC7 exit, and must be performed only after
>    PEX/SATA UPHY is initialized. Therefore, this commit moves the
>    programming sequence to .power_on() stub which is invoked after
>    .init(). PEX/SATA UPHY is initialzied in .init().
> 
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 495 ++++++++++++++++--------------
>  drivers/phy/tegra/xusb.c          |   2 +-
>  drivers/phy/tegra/xusb.h          |   6 +-
>  3 files changed, 270 insertions(+), 233 deletions(-)

You've listed 4 logically separate changes in the commit message, so I'm
wondering if it's possible to split this patch into 4 different ones. It
might not be worth doing that if they all basically fix the sequence in
one go, but it's pretty difficult to review this as-is.

> 
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 66bd4613835b..3a2d9797fb9f 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -256,23 +256,52 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
>  	return container_of(padctl, struct tegra210_xusb_padctl, base);
>  }
>  
> +static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
> +	{ 0, "pcie", 6 },
> +	{ 1, "pcie", 5 },
> +	{ 2, "pcie", 0 },
> +	{ 2, "pcie", 3 },
> +	{ 3, "pcie", 4 },
> +	{ 3, "pcie", 4 },
> +	{ 0, NULL,   0 }
> +};
> +
> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane)
> +{
> +	const struct tegra_xusb_lane_map *map;
> +
> +	for (map = tegra210_usb3_map; map->type; map++) {
> +		if (map->index == lane->index &&
> +		    strcmp(map->type, lane->pad->soc->name) == 0) {
> +			dev_dbg(lane->pad->padctl->dev,
> +				"lane = %s map to port = usb3-%d\n",

"mapped to port"?

> +				lane->pad->soc->lanes[lane->index].name,
> +				map->port);
> +			return map->port;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /* must be called under padctl->lock */
>  static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>  {
>  	struct tegra_xusb_pcie_pad *pcie = to_pcie_pad(padctl->pcie);
>  	unsigned long timeout;
>  	u32 value;
> -	int err;
> +	int err, i;

i should be unsigned to match the type of padctl->pcie->soc->num_lanes.

>  
> -	if (pcie->enable > 0) {
> -		pcie->enable++;
> +	if (pcie->enable)
>  		return 0;
> -	}
>  
>  	err = clk_prepare_enable(pcie->pll);
>  	if (err < 0)
>  		return err;
>  
> +	if (tegra210_plle_hw_sequence_is_enabled())
> +		goto skip_pll_init;
> +
>  	err = reset_control_deassert(pcie->rst);

Is it guaranteed that the reset is asserted if the PLLE HW sequencer is
enabled? I suppose with the change to not enable the sequencer by
default in one of the earlier patches this may indeed be a valid
assumption.

>  	if (err < 0)
>  		goto disable;
> @@ -455,7 +484,14 @@ static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>  
>  	tegra210_xusb_pll_hw_sequence_start();
>  
> -	pcie->enable++;
> +skip_pll_init:
> +	pcie->enable = true;
> +
> +	for (i = 0; i < padctl->pcie->soc->num_lanes; i++) {
> +		value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> +		value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(i);
> +		padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	}
>  
>  	return 0;
>  
> @@ -469,34 +505,42 @@ static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>  static void tegra210_pex_uphy_disable(struct tegra_xusb_padctl *padctl)
>  {
>  	struct tegra_xusb_pcie_pad *pcie = to_pcie_pad(padctl->pcie);
> +	u32 value;
> +	int i;

Same as above.

>  
> -	mutex_lock(&padctl->lock);
> -
> -	if (WARN_ON(pcie->enable == 0))
> -		goto unlock;
> +	if (WARN_ON(!pcie->enable))
> +		return;
>  
> -	if (--pcie->enable > 0)
> -		goto unlock;
> +	pcie->enable = false;
>  
> -	reset_control_assert(pcie->rst);
> +	for (i = 0; i < padctl->pcie->soc->num_lanes; i++) {
> +		value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> +		value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(i);
> +		padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	}
>  	clk_disable_unprepare(pcie->pll);

Please leave a blank line after a block for better readability.

> -
> -unlock:
> -	mutex_unlock(&padctl->lock);
>  }
>  
>  /* must be called under padctl->lock */
> -static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb)
> +static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl)
>  {
>  	struct tegra_xusb_sata_pad *sata = to_sata_pad(padctl->sata);
> +	struct tegra_xusb_lane *lane = tegra_xusb_find_lane(padctl, "sata", 0);
>  	unsigned long timeout;
>  	u32 value;
> -	int err;
> +	int err, i;

Same comment as above for "i".

> +	bool usb;
>  
> -	if (sata->enable > 0) {
> -		sata->enable++;
> +	if (sata->enable)

Do we want a WARN_ON() here for symmetry with the implementation of
tegra210_sata_uphy_disable() below?

>  		return 0;
> -	}
> +
> +	if (!lane)
> +		return 0;
> +
> +	if (tegra210_plle_hw_sequence_is_enabled())
> +		goto skip_pll_init;
> +
> +	usb = tegra_xusb_lane_check(lane, "usb3-ss");
>  
>  	err = clk_prepare_enable(sata->pll);
>  	if (err < 0)
> @@ -697,7 +741,14 @@ static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb)
>  
>  	tegra210_sata_pll_hw_sequence_start();
>  
> -	sata->enable++;
> +skip_pll_init:
> +	sata->enable = true;
> +
> +	for (i = 0; i < padctl->sata->soc->num_lanes; i++) {
> +		value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> +		value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(i);
> +		padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	}
>  
>  	return 0;
>  
> @@ -711,31 +762,26 @@ static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb)
>  static void tegra210_sata_uphy_disable(struct tegra_xusb_padctl *padctl)
>  {
>  	struct tegra_xusb_sata_pad *sata = to_sata_pad(padctl->sata);
> +	u32 value;
> +	int i;

unsigned int, please.

>  
> -	mutex_lock(&padctl->lock);
> -
> -	if (WARN_ON(sata->enable == 0))
> -		goto unlock;
> +	if (WARN_ON(!sata->enable))
> +		return;
>  
> -	if (--sata->enable > 0)
> -		goto unlock;
> +	sata->enable = false;
>  
> -	reset_control_assert(sata->rst);
> +	for (i = 0; i < padctl->sata->soc->num_lanes; i++) {
> +		value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> +		value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(i);
> +		padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	}
>  	clk_disable_unprepare(sata->pll);
> -
> -unlock:
> -	mutex_unlock(&padctl->lock);
>  }
>  
> -static int tegra210_xusb_padctl_enable(struct tegra_xusb_padctl *padctl)
> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)
>  {
>  	u32 value;
>  
> -	mutex_lock(&padctl->lock);
> -
> -	if (padctl->enable++ > 0)
> -		goto out;
> -
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
>  	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> @@ -751,24 +797,12 @@ static int tegra210_xusb_padctl_enable(struct tegra_xusb_padctl *padctl)
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN;
>  	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -out:
> -	mutex_unlock(&padctl->lock);
> -	return 0;
>  }
>  
> -static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
> +static void tegra210_aux_mux_lp0_clamp_enable(struct tegra_xusb_padctl *padctl)
>  {
>  	u32 value;
>  
> -	mutex_lock(&padctl->lock);
> -
> -	if (WARN_ON(padctl->enable == 0))
> -		goto out;
> -
> -	if (--padctl->enable > 0)
> -		goto out;
> -
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value |= XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN;
>  	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> @@ -784,12 +818,36 @@ static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value |= XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
>  	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +}
> +
> +static int tegra210_uphy_init(struct tegra_xusb_padctl *padctl)
> +{
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_enable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_enable(padctl);
> +
> +	if (!tegra210_plle_hw_sequence_is_enabled())
> +		tegra210_plle_hw_sequence_start();
> +	else
> +		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
> +
> +	tegra210_aux_mux_lp0_clamp_disable(padctl);
>  
> -out:
> -	mutex_unlock(&padctl->lock);
>  	return 0;
>  }
>  
> +static void __maybe_unused
> +tegra210_uphy_deinit(struct tegra_xusb_padctl *padctl)
> +{
> +	tegra210_aux_mux_lp0_clamp_enable(padctl);

Do we need tegra210_plle_hw_sequence_stop() here?

> +
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_disable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_disable(padctl);

Maybe reverse the order of these two so that they are symmetrical with
tegra210_uphy_init()? Also, single blank lines between the two blocks
make this easier to read, in my opinion.

Thierry