[PATCH 0/8] Tegra XHCI controller ELPG support

[PATCH 0/8] Tegra XHCI controller ELPG support

[JC Kuo]

Tegra XHCI controler can be placed in ELPG (Engine Level PowerGate)
state for power saving when all of the connected USB devices are in
suspended state. This patch series includes clk, phy and pmc changes
that are required for properly place controller in ELPG and bring
controller out of ELPG.


JC Kuo (8):
  clk: tegra: Add PLLE HW power sequencer control
  clk: tegra: don't enable PLLE HW sequencer at init
  phy: tegra: xusb: t210: rearrange UPHY init
  phy: tegra: xusb: add sleepwalk and suspend/resume
  soc/tegra: pmc: support T210 USB 2.0 Sleepwalk
  phy: tegra: xusb: t210: support wake and sleepwalk
  arm64: tegra: add Tegra210 XUSB PADCTL irq
  xhci: tegra: enable ELPG for runtime/system PM

 arch/arm64/boot/dts/nvidia/tegra210.dtsi |    3 +-
 drivers/clk/tegra/clk-pll.c              |   12 -
 drivers/clk/tegra/clk-tegra210.c         |   45 +
 drivers/phy/tegra/xusb-tegra210.c        | 1023 +++++++++++++++++-----
 drivers/phy/tegra/xusb.c                 |   80 +-
 drivers/phy/tegra/xusb.h                 |   10 +
 drivers/soc/tegra/pmc.c                  |  462 ++++++++++
 drivers/usb/host/xhci-tegra.c            |  802 ++++++++++++++---
 include/linux/clk/tegra.h                |    2 +
 include/linux/phy/tegra/xusb.h           |   12 +
 include/soc/tegra/pmc.h                  |   13 +
 11 files changed, 2108 insertions(+), 356 deletions(-)

-- 
2.17.1

[PATCH 0/8] Tegra XHCI controller ELPG support

[JC Kuo]

Tegra XHCI controler can be placed in ELPG (Engine Level PowerGate)
state for power saving when all of the connected USB devices are in
suspended state. This patch series includes clk, phy and pmc changes
that are required for properly place controller in ELPG and bring
controller out of ELPG.


JC Kuo (8):
  clk: tegra: Add PLLE HW power sequencer control
  clk: tegra: don't enable PLLE HW sequencer at init
  phy: tegra: xusb: t210: rearrange UPHY init
  phy: tegra: xusb: add sleepwalk and suspend/resume
  soc/tegra: pmc: support T210 USB 2.0 Sleepwalk
  phy: tegra: xusb: t210: support wake and sleepwalk
  arm64: tegra: add Tegra210 XUSB PADCTL irq
  xhci: tegra: enable ELPG for runtime/system PM

 arch/arm64/boot/dts/nvidia/tegra210.dtsi |    3 +-
 drivers/clk/tegra/clk-pll.c              |   12 -
 drivers/clk/tegra/clk-tegra210.c         |   45 +
 drivers/phy/tegra/xusb-tegra210.c        | 1023 +++++++++++++++++-----
 drivers/phy/tegra/xusb.c                 |   80 +-
 drivers/phy/tegra/xusb.h                 |   10 +
 drivers/soc/tegra/pmc.c                  |  462 ++++++++++
 drivers/usb/host/xhci-tegra.c            |  802 ++++++++++++++---
 include/linux/clk/tegra.h                |    2 +
 include/linux/phy/tegra/xusb.h           |   12 +
 include/soc/tegra/pmc.h                  |   13 +
 11 files changed, 2108 insertions(+), 356 deletions(-)

-- 
2.17.1

[PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[JC Kuo]

PLLE hardware power sequencer has to be enabled after PEX/SATA
UPHY PLL's sequencers are enabled.

tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
PLLE hardware sequencer at proper time.

tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
check whether PLLE hardware sequencer has been enabled or not.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/clk/tegra/clk-tegra210.c | 45 ++++++++++++++++++++++++++++++++
 include/linux/clk/tegra.h        |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index e1ba62d2b1a0..14d330669f36 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -398,6 +398,14 @@ static const char *mux_pllmcp_clkm[] = {
 #define PLLRE_BASE_DEFAULT_MASK		0x1c000000
 #define PLLRE_MISC0_WRITE_MASK		0x67ffffff
 
+/* PLLE */
+#define PLLE_MISC_IDDQ_SW_CTRL		(1 << 14)
+#define PLLE_AUX_USE_LOCKDET		(1 << 3)
+#define PLLE_AUX_SS_SEQ_INCLUDE		(1 << 31)
+#define PLLE_AUX_ENABLE_SWCTL		(1 << 4)
+#define PLLE_AUX_SS_SWCTL		(1 << 6)
+#define PLLE_AUX_SEQ_ENABLE		(1 << 24)
+
 /* PLLX */
 #define PLLX_USE_DYN_RAMP		1
 #define PLLX_BASE_LOCK			(1 << 27)
@@ -484,6 +492,43 @@ static const char *mux_pllmcp_clkm[] = {
 #define PLLU_MISC0_WRITE_MASK		0xbfffffff
 #define PLLU_MISC1_WRITE_MASK		0x00000007
 
+bool tegra210_plle_hw_sequence_is_enabled(void)
+{
+	u32 val;
+
+	val = readl_relaxed(clk_base + PLLE_AUX);
+	if (val & PLLE_AUX_SEQ_ENABLE)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(tegra210_plle_hw_sequence_is_enabled);
+
+void tegra210_plle_hw_sequence_start(void)
+{
+	u32 val;
+
+	if (tegra210_plle_hw_sequence_is_enabled())
+		return;
+
+	val = readl_relaxed(clk_base + PLLE_MISC0);
+	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
+	writel_relaxed(val, clk_base + PLLE_MISC0);
+
+	val = readl_relaxed(clk_base + PLLE_AUX);
+	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
+	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
+	writel_relaxed(val, clk_base + PLLE_AUX);
+
+	fence_udelay(1, clk_base);
+
+	val |= PLLE_AUX_SEQ_ENABLE;
+	writel_relaxed(val, clk_base + PLLE_AUX);
+
+	fence_udelay(1, clk_base);
+}
+EXPORT_SYMBOL_GPL(tegra210_plle_hw_sequence_start);
+
 void tegra210_xusb_pll_hw_control_enable(void)
 {
 	u32 val;
diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
index b8aef62cc3f5..07b6d6145c95 100644
--- a/include/linux/clk/tegra.h
+++ b/include/linux/clk/tegra.h
@@ -110,6 +110,8 @@ static inline void tegra_cpu_clock_resume(void)
 }
 #endif
 
+extern void tegra210_plle_hw_sequence_start(void);
+extern bool tegra210_plle_hw_sequence_is_enabled(void);
 extern void tegra210_xusb_pll_hw_control_enable(void);
 extern void tegra210_xusb_pll_hw_sequence_start(void);
 extern void tegra210_sata_pll_hw_control_enable(void);
-- 
2.17.1

[PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[JC Kuo]

PLLE hardware power sequencer has to be enabled after PEX/SATA
UPHY PLL's sequencers are enabled.

tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
PLLE hardware sequencer at proper time.

tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
check whether PLLE hardware sequencer has been enabled or not.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/clk/tegra/clk-tegra210.c | 45 ++++++++++++++++++++++++++++++++
 include/linux/clk/tegra.h        |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index e1ba62d2b1a0..14d330669f36 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -398,6 +398,14 @@ static const char *mux_pllmcp_clkm[] = {
 #define PLLRE_BASE_DEFAULT_MASK		0x1c000000
 #define PLLRE_MISC0_WRITE_MASK		0x67ffffff
 
+/* PLLE */
+#define PLLE_MISC_IDDQ_SW_CTRL		(1 << 14)
+#define PLLE_AUX_USE_LOCKDET		(1 << 3)
+#define PLLE_AUX_SS_SEQ_INCLUDE		(1 << 31)
+#define PLLE_AUX_ENABLE_SWCTL		(1 << 4)
+#define PLLE_AUX_SS_SWCTL		(1 << 6)
+#define PLLE_AUX_SEQ_ENABLE		(1 << 24)
+
 /* PLLX */
 #define PLLX_USE_DYN_RAMP		1
 #define PLLX_BASE_LOCK			(1 << 27)
@@ -484,6 +492,43 @@ static const char *mux_pllmcp_clkm[] = {
 #define PLLU_MISC0_WRITE_MASK		0xbfffffff
 #define PLLU_MISC1_WRITE_MASK		0x00000007
 
+bool tegra210_plle_hw_sequence_is_enabled(void)
+{
+	u32 val;
+
+	val = readl_relaxed(clk_base + PLLE_AUX);
+	if (val & PLLE_AUX_SEQ_ENABLE)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(tegra210_plle_hw_sequence_is_enabled);
+
+void tegra210_plle_hw_sequence_start(void)
+{
+	u32 val;
+
+	if (tegra210_plle_hw_sequence_is_enabled())
+		return;
+
+	val = readl_relaxed(clk_base + PLLE_MISC0);
+	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
+	writel_relaxed(val, clk_base + PLLE_MISC0);
+
+	val = readl_relaxed(clk_base + PLLE_AUX);
+	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
+	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
+	writel_relaxed(val, clk_base + PLLE_AUX);
+
+	fence_udelay(1, clk_base);
+
+	val |= PLLE_AUX_SEQ_ENABLE;
+	writel_relaxed(val, clk_base + PLLE_AUX);
+
+	fence_udelay(1, clk_base);
+}
+EXPORT_SYMBOL_GPL(tegra210_plle_hw_sequence_start);
+
 void tegra210_xusb_pll_hw_control_enable(void)
 {
 	u32 val;
diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
index b8aef62cc3f5..07b6d6145c95 100644
--- a/include/linux/clk/tegra.h
+++ b/include/linux/clk/tegra.h
@@ -110,6 +110,8 @@ static inline void tegra_cpu_clock_resume(void)
 }
 #endif
 
+extern void tegra210_plle_hw_sequence_start(void);
+extern bool tegra210_plle_hw_sequence_is_enabled(void);
 extern void tegra210_xusb_pll_hw_control_enable(void);
 extern void tegra210_xusb_pll_hw_sequence_start(void);
 extern void tegra210_sata_pll_hw_control_enable(void);
-- 
2.17.1

Re: [PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> PLLE hardware power sequencer has to be enabled after PEX/SATA
> UPHY PLL's sequencers are enabled.
> 
> tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
> PLLE hardware sequencer at proper time.
> 
> tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
> check whether PLLE hardware sequencer has been enabled or not.

I think that here to be clear about what is going on you should state
that you are "adding the function tegra210_plle_hw_sequence_start() ..."

Are these functions dependent upon clk_plle_tegra210_enable() already
being called? I assume that there must be some dependency between the
above functions and the existing plle enable function. If there is a
dependency, how do you ensure the existing enable is already called?

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> PLLE hardware power sequencer has to be enabled after PEX/SATA
> UPHY PLL's sequencers are enabled.
> 
> tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
> PLLE hardware sequencer at proper time.
> 
> tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
> check whether PLLE hardware sequencer has been enabled or not.

I think that here to be clear about what is going on you should state
that you are "adding the function tegra210_plle_hw_sequence_start() ..."

Are these functions dependent upon clk_plle_tegra210_enable() already
being called? I assume that there must be some dependency between the
above functions and the existing plle enable function. If there is a
dependency, how do you ensure the existing enable is already called?

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[JC Kuo]

On 7/4/19 8:16 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> PLLE hardware power sequencer has to be enabled after PEX/SATA
>> UPHY PLL's sequencers are enabled.
>>
>> tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
>> PLLE hardware sequencer at proper time.
>>
>> tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
>> check whether PLLE hardware sequencer has been enabled or not.
> 
> I think that here to be clear about what is going on you should state
> that you are "adding the function tegra210_plle_hw_sequence_start() ..."
Thanks. I will amend the commit message accordingly.
> 
> Are these functions dependent upon clk_plle_tegra210_enable() already
> being called? I assume that there must be some dependency between the
> above functions and the existing plle enable function. If there is a
> dependency, how do you ensure the existing enable is already called?
Yes, tegra210_plle_hw_sequence_start() has to be invoked after PLLE is
enabled/locked. Caller is in charge of calling with correct sequence.
I will add a check in tegra210_plle_hw_sequence_start() to ensure that
PLLE HW sequencer will not be accidentally enabled when PLLE is not
enabled/locked yet.

Thanks,
JC

Re: [PATCH 1/8] clk: tegra: Add PLLE HW power sequencer control

[JC Kuo]

On 7/4/19 8:16 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> PLLE hardware power sequencer has to be enabled after PEX/SATA
>> UPHY PLL's sequencers are enabled.
>>
>> tegra210_plle_hw_sequence_start() for XUSB PADCTL driver to enable
>> PLLE hardware sequencer at proper time.
>>
>> tegra210_plle_hw_sequence_is_enabled() for XUSB PADCTL driver to
>> check whether PLLE hardware sequencer has been enabled or not.
> 
> I think that here to be clear about what is going on you should state
> that you are "adding the function tegra210_plle_hw_sequence_start() ..."
Thanks. I will amend the commit message accordingly.
> 
> Are these functions dependent upon clk_plle_tegra210_enable() already
> being called? I assume that there must be some dependency between the
> above functions and the existing plle enable function. If there is a
> dependency, how do you ensure the existing enable is already called?
Yes, tegra210_plle_hw_sequence_start() has to be invoked after PLLE is
enabled/locked. Caller is in charge of calling with correct sequence.
I will add a check in tegra210_plle_hw_sequence_start() to ensure that
PLLE HW sequencer will not be accidentally enabled when PLLE is not
enabled/locked yet.

Thanks,
JC

[PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[JC Kuo]

PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
power sequencers' output to enable/disable PLLE. PLLE hardware power
sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
are enabled.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 1583f5fc992f..e6de65987fd2 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
 	pll_writel(val, PLLE_SS_CTRL, pll);
 	udelay(1);
 
-	val = pll_readl_misc(pll);
-	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
-	pll_writel_misc(val, pll);
-
-	val = pll_readl(pll->params->aux_reg, pll);
-	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
-	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
-	pll_writel(val, pll->params->aux_reg, pll);
-	udelay(1);
-	val |= PLLE_AUX_SEQ_ENABLE;
-	pll_writel(val, pll->params->aux_reg, pll);
-
 out:
 	if (pll->lock)
 		spin_unlock_irqrestore(pll->lock, flags);
-- 
2.17.1

[PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[JC Kuo]

PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
power sequencers' output to enable/disable PLLE. PLLE hardware power
sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
are enabled.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 1583f5fc992f..e6de65987fd2 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
 	pll_writel(val, PLLE_SS_CTRL, pll);
 	udelay(1);
 
-	val = pll_readl_misc(pll);
-	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
-	pll_writel_misc(val, pll);
-
-	val = pll_readl(pll->params->aux_reg, pll);
-	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
-	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
-	pll_writel(val, pll->params->aux_reg, pll);
-	udelay(1);
-	val |= PLLE_AUX_SEQ_ENABLE;
-	pll_writel(val, pll->params->aux_reg, pll);
-
 out:
 	if (pll->lock)
 		spin_unlock_irqrestore(pll->lock, flags);
-- 
2.17.1

Re: [PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
> power sequencers' output to enable/disable PLLE. PLLE hardware power
> sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
> are enabled.
> 
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 1583f5fc992f..e6de65987fd2 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
>  	pll_writel(val, PLLE_SS_CTRL, pll);
>  	udelay(1);
>  
> -	val = pll_readl_misc(pll);
> -	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
> -	pll_writel_misc(val, pll);
> -
> -	val = pll_readl(pll->params->aux_reg, pll);
> -	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
> -	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
> -	pll_writel(val, pll->params->aux_reg, pll);
> -	udelay(1);
> -	val |= PLLE_AUX_SEQ_ENABLE;
> -	pll_writel(val, pll->params->aux_reg, pll);
> -
>  out:
>  	if (pll->lock)
>  		spin_unlock_irqrestore(pll->lock, flags);
> 

So this function is called clk_plle_tegra210_enable() and is called by
the CCF enable callback. However, after the above change, does this mean
that this no longer enables the PLL? I understand that that is what you
want, but from an architecture perspective, it seems incorrect to have
an enable function that when called does not enable the PLL as expected.

I don't fully understand why we need to add the new helpers from the
previous patch and we cannot use the CCF APIs directly?

If you really need to split the existing enable function, then the CCF
does have prepare and enable callbacks that can be used.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
> power sequencers' output to enable/disable PLLE. PLLE hardware power
> sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
> are enabled.
> 
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 1583f5fc992f..e6de65987fd2 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
>  	pll_writel(val, PLLE_SS_CTRL, pll);
>  	udelay(1);
>  
> -	val = pll_readl_misc(pll);
> -	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
> -	pll_writel_misc(val, pll);
> -
> -	val = pll_readl(pll->params->aux_reg, pll);
> -	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
> -	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
> -	pll_writel(val, pll->params->aux_reg, pll);
> -	udelay(1);
> -	val |= PLLE_AUX_SEQ_ENABLE;
> -	pll_writel(val, pll->params->aux_reg, pll);
> -
>  out:
>  	if (pll->lock)
>  		spin_unlock_irqrestore(pll->lock, flags);
> 

So this function is called clk_plle_tegra210_enable() and is called by
the CCF enable callback. However, after the above change, does this mean
that this no longer enables the PLL? I understand that that is what you
want, but from an architecture perspective, it seems incorrect to have
an enable function that when called does not enable the PLL as expected.

I don't fully understand why we need to add the new helpers from the
previous patch and we cannot use the CCF APIs directly?

If you really need to split the existing enable function, then the CCF
does have prepare and enable callbacks that can be used.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[JC Kuo]

On 7/4/19 8:22 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
>> power sequencers' output to enable/disable PLLE. PLLE hardware power
>> sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
>> are enabled.
>>
>> Signed-off-by: JC Kuo <jckuo@nvidia.com>
>> ---
>>  drivers/clk/tegra/clk-pll.c | 12 ------------
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
>> index 1583f5fc992f..e6de65987fd2 100644
>> --- a/drivers/clk/tegra/clk-pll.c
>> +++ b/drivers/clk/tegra/clk-pll.c
>> @@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
>>  	pll_writel(val, PLLE_SS_CTRL, pll);
>>  	udelay(1);
>>  
>> -	val = pll_readl_misc(pll);
>> -	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
>> -	pll_writel_misc(val, pll);
>> -
>> -	val = pll_readl(pll->params->aux_reg, pll);
>> -	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
>> -	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
>> -	pll_writel(val, pll->params->aux_reg, pll);
>> -	udelay(1);
>> -	val |= PLLE_AUX_SEQ_ENABLE;
>> -	pll_writel(val, pll->params->aux_reg, pll);
>> -
>>  out:
>>  	if (pll->lock)
>>  		spin_unlock_irqrestore(pll->lock, flags);
>>
> 
> So this function is called clk_plle_tegra210_enable() and is called by
> the CCF enable callback. However, after the above change, does this mean
> that this no longer enables the PLL? I understand that that is what you
> want, but from an architecture perspective, it seems incorrect to have
> an enable function that when called does not enable the PLL as expected.
> 
> I don't fully understand why we need to add the new helpers from the
> previous patch and we cannot use the CCF APIs directly?
> 
> If you really need to split the existing enable function, then the CCF
> does have prepare and enable callbacks that can be used.
> 
> Cheers
> Jon
> 
Hi Jon,
Thanks for review. With this change, clk_plle_tegra210_enable() still enables
PLLE (by setting PLLE_BASE_ENABLE bit). It just skips the procedure to enable
the hardware sequencer (controlled by PLLE_AUX_SEQ_ENABLE bit).

PLLE has a hardware power sequencer logic which is a state machine that can
power on/off PLLE without any software intervention. The sequencer has two
inputs, one from XUSB UPHY PLL and the other from SATA UPHY PLL. PLLE provides
reference clock to XUSB and SATA UPHY PLLs. When both of the downstream PLLs are
powered-off, PLLE hardware power sequencer will automatically power off PLLE for
power saving.

XUSB and SATA UPHY PLLs also have their own hardware power sequencer logic. XUSB
UPHY PLL is shared between XUSB SuperSpeed ports and PCIE controllers. The XUSB
UPHY PLL hardware power sequencer has inputs from XUSB and PCIE. When all of the
XUSB SuperSpeed ports and PCIE controllers are in low power state, XUSB UPHY PLL
hardware power sequencer automatically power off PLL and signals idle to PLLE
hardware power sequencer. Similar applies to SATA UPHY PLL.

Therefore, before enabling PLLE hardware power sequencer, software must ensure
that both XUSB and SATA UPHY PLLs hardware power sequencers are enabled properly.

Tegra210 XUSB PADCTL driver (drivers/phy/tegra/xusb-tegra210.c) is in charge of
enabling XUSB and SATA UPHY PLLs in software controlled power state. Sequencers
are managed by CAR registers, so xusb-tegra210.c invokes the following APIs to
get hardware power sequencers enabled.

  tegra210_xusb_pll_hw_control_enable()
  tegra210_xusb_pll_hw_sequence_start()
  tegra210_sata_pll_hw_control_enable()
  tegra210_sata_pll_hw_sequence_start()

Thanks,
JC

Re: [PATCH 2/8] clk: tegra: don't enable PLLE HW sequencer at init

[JC Kuo]

On 7/4/19 8:22 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware
>> power sequencers' output to enable/disable PLLE. PLLE hardware power
>> sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers
>> are enabled.
>>
>> Signed-off-by: JC Kuo <jckuo@nvidia.com>
>> ---
>>  drivers/clk/tegra/clk-pll.c | 12 ------------
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
>> index 1583f5fc992f..e6de65987fd2 100644
>> --- a/drivers/clk/tegra/clk-pll.c
>> +++ b/drivers/clk/tegra/clk-pll.c
>> @@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
>>  	pll_writel(val, PLLE_SS_CTRL, pll);
>>  	udelay(1);
>>  
>> -	val = pll_readl_misc(pll);
>> -	val &= ~PLLE_MISC_IDDQ_SW_CTRL;
>> -	pll_writel_misc(val, pll);
>> -
>> -	val = pll_readl(pll->params->aux_reg, pll);
>> -	val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE);
>> -	val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL);
>> -	pll_writel(val, pll->params->aux_reg, pll);
>> -	udelay(1);
>> -	val |= PLLE_AUX_SEQ_ENABLE;
>> -	pll_writel(val, pll->params->aux_reg, pll);
>> -
>>  out:
>>  	if (pll->lock)
>>  		spin_unlock_irqrestore(pll->lock, flags);
>>
> 
> So this function is called clk_plle_tegra210_enable() and is called by
> the CCF enable callback. However, after the above change, does this mean
> that this no longer enables the PLL? I understand that that is what you
> want, but from an architecture perspective, it seems incorrect to have
> an enable function that when called does not enable the PLL as expected.
> 
> I don't fully understand why we need to add the new helpers from the
> previous patch and we cannot use the CCF APIs directly?
> 
> If you really need to split the existing enable function, then the CCF
> does have prepare and enable callbacks that can be used.
> 
> Cheers
> Jon
> 
Hi Jon,
Thanks for review. With this change, clk_plle_tegra210_enable() still enables
PLLE (by setting PLLE_BASE_ENABLE bit). It just skips the procedure to enable
the hardware sequencer (controlled by PLLE_AUX_SEQ_ENABLE bit).

PLLE has a hardware power sequencer logic which is a state machine that can
power on/off PLLE without any software intervention. The sequencer has two
inputs, one from XUSB UPHY PLL and the other from SATA UPHY PLL. PLLE provides
reference clock to XUSB and SATA UPHY PLLs. When both of the downstream PLLs are
powered-off, PLLE hardware power sequencer will automatically power off PLLE for
power saving.

XUSB and SATA UPHY PLLs also have their own hardware power sequencer logic. XUSB
UPHY PLL is shared between XUSB SuperSpeed ports and PCIE controllers. The XUSB
UPHY PLL hardware power sequencer has inputs from XUSB and PCIE. When all of the
XUSB SuperSpeed ports and PCIE controllers are in low power state, XUSB UPHY PLL
hardware power sequencer automatically power off PLL and signals idle to PLLE
hardware power sequencer. Similar applies to SATA UPHY PLL.

Therefore, before enabling PLLE hardware power sequencer, software must ensure
that both XUSB and SATA UPHY PLLs hardware power sequencers are enabled properly.

Tegra210 XUSB PADCTL driver (drivers/phy/tegra/xusb-tegra210.c) is in charge of
enabling XUSB and SATA UPHY PLLs in software controlled power state. Sequencers
are managed by CAR registers, so xusb-tegra210.c invokes the following APIs to
get hardware power sequencers enabled.

  tegra210_xusb_pll_hw_control_enable()
  tegra210_xusb_pll_hw_sequence_start()
  tegra210_sata_pll_hw_control_enable()
  tegra210_sata_pll_hw_sequence_start()

Thanks,
JC

[PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[JC Kuo]

This commit is a preparation for enabling XUSB LP0 support.
It rearranges T210 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.

2. Once UPHY PLL hardware power sequncer is enabled, do not
   assert reset to PEX/SATA PLLs.

3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
   XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
   XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
   to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
   and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.

4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
   registers programming from tegra210_usb3_port_enable() to
   tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
   XUSB USB3 ports will be programmed at LP0 exit.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
 drivers/phy/tegra/xusb.c          |   2 +-
 drivers/phy/tegra/xusb.h          |   2 +
 3 files changed, 264 insertions(+), 183 deletions(-)

diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
index 18cea8311d22..007bf352b45e 100644
--- a/drivers/phy/tegra/xusb-tegra210.c
+++ b/drivers/phy/tegra/xusb-tegra210.c
@@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
 	return container_of(padctl, struct tegra210_xusb_padctl, base);
 }
 
+static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
+
 /* must be called under padctl->lock */
 static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
 {
@@ -453,35 +455,44 @@ 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);
-
-	mutex_lock(&padctl->lock);
+	u32 value;
+	int i;
 
 	if (WARN_ON(pcie->enable == 0))
-		goto unlock;
+		return;
 
 	if (--pcie->enable > 0)
-		goto unlock;
+		return;
 
-	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);
-
-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;
+	bool usb = false;
 
 	if (sata->enable > 0) {
 		sata->enable++;
 		return 0;
 	}
 
+	if (!lane)
+		return 0;
+
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		usb = true;
+
 	err = clk_prepare_enable(sata->pll);
 	if (err < 0)
 		return err;
@@ -695,30 +706,36 @@ 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);
-
-	mutex_lock(&padctl->lock);
+	u32 value;
+	int i;
 
 	if (WARN_ON(sata->enable == 0))
-		goto unlock;
+		return;
 
 	if (--sata->enable > 0)
-		goto unlock;
+		return;
 
-	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)
 {
-	u32 value;
+	return 0;
+}
 
-	mutex_lock(&padctl->lock);
+static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
+{
+	return 0;
+}
 
-	if (padctl->enable++ > 0)
-		goto out;
+static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)
+{
+	u32 value;
 
 	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
 	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
@@ -735,24 +752,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);
@@ -768,12 +773,76 @@ 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)
+{
+	struct tegra_xusb_pcie_pad *pcie;
+	struct tegra_xusb_sata_pad *sata;
+	u32 value;
+	int err;
+	int i;
+
+	if (tegra210_plle_hw_sequence_is_enabled()) {
+		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
+		/* skip pll initialization, update plle refcount only */
+		if (padctl->pcie) {
+			pcie = to_pcie_pad(padctl->pcie);
+			if (pcie->enable == 0) {
+				err = clk_prepare_enable(pcie->pll);
+				if (err < 0)
+					return err;
+				pcie->enable++;
+			}
+		}
+		if (padctl->sata) {
+			sata = to_sata_pad(padctl->sata);
+			if (sata->enable == 0) {
+				err = clk_prepare_enable(sata->pll);
+				if (err < 0)
+					return err;
+				sata->enable++;
+			}
+		}
+		goto skip_pll_init;
+	}
+
+	if (padctl->pcie)
+		tegra210_pex_uphy_enable(padctl);
+	if (padctl->sata)
+		tegra210_sata_uphy_enable(padctl);
+
+	tegra210_plle_hw_sequence_start();
+
+skip_pll_init:
+	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);
+	}
+
+	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);
+	}
+
+	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);
+
+	if (padctl->pcie)
+		tegra210_pex_uphy_disable(padctl);
+	if (padctl->sata)
+		tegra210_sata_uphy_disable(padctl);
+}
+
 static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 				  unsigned int index, bool idle)
 {
@@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
 	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
 };
 
+static int tegra210_usb3_phy_power_on(struct phy *phy)
+{
+	struct device *dev = &phy->dev;
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
+					    tegra210_usb3_lane_map(lane));
+	int index;
+	u32 value;
+
+	if (!usb3) {
+		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
+		return -ENODEV;
+	}
+	index = usb3->base.index;
+
+	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
+
+	if (!usb3->internal)
+		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
+	else
+		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
+
+	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
+	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
+	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
+
+	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
+		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
+
+	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
+		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	return 0;
+}
+
+static int tegra210_usb3_phy_power_off(struct phy *phy)
+{
+	struct device *dev = &phy->dev;
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
+					    tegra210_usb3_lane_map(lane));
+	int index;
+	u32 value;
+
+	if (!usb3) {
+		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
+		return -ENODEV;
+	}
+	index = usb3->base.index;
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(250, 350);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	return 0;
+}
 static struct tegra_xusb_lane *
 tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
 			 unsigned int index)
@@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
 static int tegra210_pcie_phy_init(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+
+	mutex_lock(&padctl->lock);
+
+	tegra210_uphy_init(padctl);
+
+	mutex_unlock(&padctl->lock);
 
 	return tegra210_xusb_padctl_enable(lane->pad->padctl);
 }
@@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
-	int err;
+	int err = 0;
 
 	mutex_lock(&padctl->lock);
 
-	err = tegra210_pex_uphy_enable(padctl);
-	if (err < 0)
-		goto unlock;
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_on(phy);
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
-
-unlock:
 	mutex_unlock(&padctl->lock);
 	return err;
 }
@@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
+	int err = 0;
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	mutex_lock(&padctl->lock);
 
-	tegra210_pex_uphy_disable(padctl);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_off(phy);
 
-	return 0;
+	mutex_unlock(&padctl->lock);
+	return err;
 }
 
 static const struct phy_ops tegra210_pcie_phy_ops = {
@@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
 static int tegra210_sata_phy_init(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+
+	mutex_lock(&padctl->lock);
+
+	tegra210_uphy_init(padctl);
 
+	mutex_unlock(&padctl->lock);
 	return tegra210_xusb_padctl_enable(lane->pad->padctl);
 }
 
@@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
-	int err;
+	int err = 0;
 
 	mutex_lock(&padctl->lock);
 
-	err = tegra210_sata_uphy_enable(padctl, false);
-	if (err < 0)
-		goto unlock;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_on(phy);
 
-unlock:
 	mutex_unlock(&padctl->lock);
 	return err;
 }
@@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
+	int err = 0;
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	mutex_lock(&padctl->lock);
 
-	tegra210_sata_uphy_disable(lane->pad->padctl);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_off(phy);
 
-	return 0;
+	mutex_unlock(&padctl->lock);
+	return err;
 }
 
 static const struct phy_ops tegra210_sata_phy_ops = {
@@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
 
 static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
 {
-	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
-	struct tegra_xusb_padctl *padctl = port->padctl;
-	struct tegra_xusb_lane *lane = usb3->base.lane;
-	unsigned int index = port->index;
-	u32 value;
-	int err;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
-
-	if (!usb3->internal)
-		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
-	else
-		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
-
-	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
-	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
-	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
-
-	/*
-	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
-	 * and conditionalize based on mux function? This seems to work, but
-	 * might not be the exact proper sequence.
-	 */
-	err = regulator_enable(usb3->supply);
-	if (err < 0)
-		return err;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
-
-	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
-		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
-
-	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
-		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
-
-	if (lane->pad == padctl->sata)
-		err = tegra210_sata_uphy_enable(padctl, true);
-	else
-		err = tegra210_pex_uphy_enable(padctl);
-
-	if (err) {
-		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
-			__func__, err);
-		return err;
-	}
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
 	return 0;
 }
 
 static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
 {
-	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
-	struct tegra_xusb_padctl *padctl = port->padctl;
-	struct tegra_xusb_lane *lane = port->lane;
-	unsigned int index = port->index;
-	u32 value;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(250, 350);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	if (lane->pad == padctl->sata)
-		tegra210_sata_uphy_disable(padctl);
-	else
-		tegra210_pex_uphy_disable(padctl);
-
-	regulator_disable(usb3->supply);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
-	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
-	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
-	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
 }
 
 static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
@@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
 	{ 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",
+				lane->pad->soc->lanes[lane->index].name,
+				map->port);
+			return map->port;
+		}
+	}
+
+	return -1;
+}
+
 static struct tegra_xusb_lane *
 tegra210_usb3_port_map(struct tegra_xusb_port *port)
 {
diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
index 2ea8497af82a..7fbba53f6097 100644
--- a/drivers/phy/tegra/xusb.c
+++ b/drivers/phy/tegra/xusb.c
@@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
 	return 0;
 }
 
-static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
+bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
 				  const char *function)
 {
 	const char *func = lane->soc->funcs[lane->function];
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 093076ca27fd..1bfe14b2a274 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
 	void (*remove)(struct tegra_xusb_lane *lane);
 };
 
+bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
+
 /*
  * pads
  */
-- 
2.17.1

[PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[JC Kuo]

This commit is a preparation for enabling XUSB LP0 support.
It rearranges T210 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.

2. Once UPHY PLL hardware power sequncer is enabled, do not
   assert reset to PEX/SATA PLLs.

3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
   XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
   XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
   to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
   and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.

4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
   registers programming from tegra210_usb3_port_enable() to
   tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
   XUSB USB3 ports will be programmed at LP0 exit.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
 drivers/phy/tegra/xusb.c          |   2 +-
 drivers/phy/tegra/xusb.h          |   2 +
 3 files changed, 264 insertions(+), 183 deletions(-)

diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
index 18cea8311d22..007bf352b45e 100644
--- a/drivers/phy/tegra/xusb-tegra210.c
+++ b/drivers/phy/tegra/xusb-tegra210.c
@@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
 	return container_of(padctl, struct tegra210_xusb_padctl, base);
 }
 
+static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
+
 /* must be called under padctl->lock */
 static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
 {
@@ -453,35 +455,44 @@ 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);
-
-	mutex_lock(&padctl->lock);
+	u32 value;
+	int i;
 
 	if (WARN_ON(pcie->enable == 0))
-		goto unlock;
+		return;
 
 	if (--pcie->enable > 0)
-		goto unlock;
+		return;
 
-	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);
-
-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;
+	bool usb = false;
 
 	if (sata->enable > 0) {
 		sata->enable++;
 		return 0;
 	}
 
+	if (!lane)
+		return 0;
+
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		usb = true;
+
 	err = clk_prepare_enable(sata->pll);
 	if (err < 0)
 		return err;
@@ -695,30 +706,36 @@ 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);
-
-	mutex_lock(&padctl->lock);
+	u32 value;
+	int i;
 
 	if (WARN_ON(sata->enable == 0))
-		goto unlock;
+		return;
 
 	if (--sata->enable > 0)
-		goto unlock;
+		return;
 
-	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)
 {
-	u32 value;
+	return 0;
+}
 
-	mutex_lock(&padctl->lock);
+static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
+{
+	return 0;
+}
 
-	if (padctl->enable++ > 0)
-		goto out;
+static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)
+{
+	u32 value;
 
 	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
 	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
@@ -735,24 +752,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);
@@ -768,12 +773,76 @@ 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)
+{
+	struct tegra_xusb_pcie_pad *pcie;
+	struct tegra_xusb_sata_pad *sata;
+	u32 value;
+	int err;
+	int i;
+
+	if (tegra210_plle_hw_sequence_is_enabled()) {
+		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
+		/* skip pll initialization, update plle refcount only */
+		if (padctl->pcie) {
+			pcie = to_pcie_pad(padctl->pcie);
+			if (pcie->enable == 0) {
+				err = clk_prepare_enable(pcie->pll);
+				if (err < 0)
+					return err;
+				pcie->enable++;
+			}
+		}
+		if (padctl->sata) {
+			sata = to_sata_pad(padctl->sata);
+			if (sata->enable == 0) {
+				err = clk_prepare_enable(sata->pll);
+				if (err < 0)
+					return err;
+				sata->enable++;
+			}
+		}
+		goto skip_pll_init;
+	}
+
+	if (padctl->pcie)
+		tegra210_pex_uphy_enable(padctl);
+	if (padctl->sata)
+		tegra210_sata_uphy_enable(padctl);
+
+	tegra210_plle_hw_sequence_start();
+
+skip_pll_init:
+	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);
+	}
+
+	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);
+	}
+
+	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);
+
+	if (padctl->pcie)
+		tegra210_pex_uphy_disable(padctl);
+	if (padctl->sata)
+		tegra210_sata_uphy_disable(padctl);
+}
+
 static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 				  unsigned int index, bool idle)
 {
@@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
 	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
 };
 
+static int tegra210_usb3_phy_power_on(struct phy *phy)
+{
+	struct device *dev = &phy->dev;
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
+					    tegra210_usb3_lane_map(lane));
+	int index;
+	u32 value;
+
+	if (!usb3) {
+		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
+		return -ENODEV;
+	}
+	index = usb3->base.index;
+
+	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
+
+	if (!usb3->internal)
+		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
+	else
+		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
+
+	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
+	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
+	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
+
+	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
+		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
+	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
+		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
+	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
+		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
+	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
+
+	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
+		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	return 0;
+}
+
+static int tegra210_usb3_phy_power_off(struct phy *phy)
+{
+	struct device *dev = &phy->dev;
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
+					    tegra210_usb3_lane_map(lane));
+	int index;
+	u32 value;
+
+	if (!usb3) {
+		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
+		return -ENODEV;
+	}
+	index = usb3->base.index;
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(100, 200);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	usleep_range(250, 350);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
+	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+	return 0;
+}
 static struct tegra_xusb_lane *
 tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
 			 unsigned int index)
@@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
 static int tegra210_pcie_phy_init(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+
+	mutex_lock(&padctl->lock);
+
+	tegra210_uphy_init(padctl);
+
+	mutex_unlock(&padctl->lock);
 
 	return tegra210_xusb_padctl_enable(lane->pad->padctl);
 }
@@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
-	int err;
+	int err = 0;
 
 	mutex_lock(&padctl->lock);
 
-	err = tegra210_pex_uphy_enable(padctl);
-	if (err < 0)
-		goto unlock;
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_on(phy);
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
-
-unlock:
 	mutex_unlock(&padctl->lock);
 	return err;
 }
@@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
+	int err = 0;
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	mutex_lock(&padctl->lock);
 
-	tegra210_pex_uphy_disable(padctl);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_off(phy);
 
-	return 0;
+	mutex_unlock(&padctl->lock);
+	return err;
 }
 
 static const struct phy_ops tegra210_pcie_phy_ops = {
@@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
 static int tegra210_sata_phy_init(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+
+	mutex_lock(&padctl->lock);
+
+	tegra210_uphy_init(padctl);
 
+	mutex_unlock(&padctl->lock);
 	return tegra210_xusb_padctl_enable(lane->pad->padctl);
 }
 
@@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
-	int err;
+	int err = 0;
 
 	mutex_lock(&padctl->lock);
 
-	err = tegra210_sata_uphy_enable(padctl, false);
-	if (err < 0)
-		goto unlock;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_on(phy);
 
-unlock:
 	mutex_unlock(&padctl->lock);
 	return err;
 }
@@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	u32 value;
+	int err = 0;
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
-	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
+	mutex_lock(&padctl->lock);
 
-	tegra210_sata_uphy_disable(lane->pad->padctl);
+	if (tegra_xusb_lane_check(lane, "usb3-ss"))
+		err = tegra210_usb3_phy_power_off(phy);
 
-	return 0;
+	mutex_unlock(&padctl->lock);
+	return err;
 }
 
 static const struct phy_ops tegra210_sata_phy_ops = {
@@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
 
 static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
 {
-	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
-	struct tegra_xusb_padctl *padctl = port->padctl;
-	struct tegra_xusb_lane *lane = usb3->base.lane;
-	unsigned int index = port->index;
-	u32 value;
-	int err;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
-
-	if (!usb3->internal)
-		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
-	else
-		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
-
-	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
-	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
-	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
-
-	/*
-	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
-	 * and conditionalize based on mux function? This seems to work, but
-	 * might not be the exact proper sequence.
-	 */
-	err = regulator_enable(usb3->supply);
-	if (err < 0)
-		return err;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
-
-	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
-		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
-
-	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
-	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
-		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
-	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
-		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
-	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
-
-	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
-		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
-
-	if (lane->pad == padctl->sata)
-		err = tegra210_sata_uphy_enable(padctl, true);
-	else
-		err = tegra210_pex_uphy_enable(padctl);
-
-	if (err) {
-		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
-			__func__, err);
-		return err;
-	}
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
 	return 0;
 }
 
 static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
 {
-	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
-	struct tegra_xusb_padctl *padctl = port->padctl;
-	struct tegra_xusb_lane *lane = port->lane;
-	unsigned int index = port->index;
-	u32 value;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(100, 200);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	usleep_range(250, 350);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
-	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
-	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
-
-	if (lane->pad == padctl->sata)
-		tegra210_sata_uphy_disable(padctl);
-	else
-		tegra210_pex_uphy_disable(padctl);
-
-	regulator_disable(usb3->supply);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
-	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
-	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
-	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
 }
 
 static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
@@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
 	{ 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",
+				lane->pad->soc->lanes[lane->index].name,
+				map->port);
+			return map->port;
+		}
+	}
+
+	return -1;
+}
+
 static struct tegra_xusb_lane *
 tegra210_usb3_port_map(struct tegra_xusb_port *port)
 {
diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
index 2ea8497af82a..7fbba53f6097 100644
--- a/drivers/phy/tegra/xusb.c
+++ b/drivers/phy/tegra/xusb.c
@@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
 	return 0;
 }
 
-static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
+bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
 				  const char *function)
 {
 	const char *func = lane->soc->funcs[lane->function];
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 093076ca27fd..1bfe14b2a274 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
 	void (*remove)(struct tegra_xusb_lane *lane);
 };
 
+bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
+
 /*
  * pads
  */
-- 
2.17.1

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> This commit is a preparation for enabling XUSB LP0 support.

By LP0 do you mean ELPG? If so please stick to using one name for
referring to the power-state in question.

> It rearranges T210 XUSB PADCTL UPHY initialization sequence,

Please use Tegra210 and not T210.

> 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.
> 
> 2. Once UPHY PLL hardware power sequncer is enabled, do not

s/sequncer/sequencer

>    assert reset to PEX/SATA PLLs.

Maybe worth clarifying why here.
	
> 
> 3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
>    to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
>    and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.
> 
> 4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
>    registers programming from tegra210_usb3_port_enable() to
>    tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
>    XUSB USB3 ports will be programmed at LP0 exit.

Looks like you are moving all the code from the port enable to the phy
enable and after this change the port enable does nothing. Do we not
differentiate between phy and port? I think a bit more description is
necessary here to describe the impact of this change.

> 
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
>  drivers/phy/tegra/xusb.c          |   2 +-
>  drivers/phy/tegra/xusb.h          |   2 +
>  3 files changed, 264 insertions(+), 183 deletions(-)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 18cea8311d22..007bf352b45e 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
>  	return container_of(padctl, struct tegra210_xusb_padctl, base);
>  }
>  
> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
> +

Can we avoid adding this prototype?

>  /* must be called under padctl->lock */
>  static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>  {
> @@ -453,35 +455,44 @@ 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);
> -
> -	mutex_lock(&padctl->lock);
> +	u32 value;
> +	int i;
>  
>  	if (WARN_ON(pcie->enable == 0))
> -		goto unlock;
> +		return;
>  
>  	if (--pcie->enable > 0)
> -		goto unlock;
> +		return;
>  
> -	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);
> -
> -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;
> +	bool usb = false;
>  
>  	if (sata->enable > 0) {
>  		sata->enable++;
>  		return 0;
>  	}
>  
> +	if (!lane)
> +		return 0;
> +
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		usb = true;

This return a boolean type so you can just ...

	usb = tegra_xusb_lane_check(lane, "usb3-ss");

> +
>  	err = clk_prepare_enable(sata->pll);
>  	if (err < 0)
>  		return err;
> @@ -695,30 +706,36 @@ 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);
> -
> -	mutex_lock(&padctl->lock);
> +	u32 value;
> +	int i;
>  
>  	if (WARN_ON(sata->enable == 0))
> -		goto unlock;
> +		return;
>  
>  	if (--sata->enable > 0)
> -		goto unlock;
> +		return;
>  
> -	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)
>  {
> -	u32 value;
> +	return 0;
> +}
>  
> -	mutex_lock(&padctl->lock);
> +static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
> +{
> +	return 0;
> +}

Why bother keeping these functions at all if they now do nothing?

>  
> -	if (padctl->enable++ > 0)
> -		goto out;
> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)

Any reason for renaming these? These appear to deal with the XUSB_PADCTL
and so the previous names seem fine.

> +{
> +	u32 value;
>  
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
> @@ -735,24 +752,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);
> @@ -768,12 +773,76 @@ 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)
> +{
> +	struct tegra_xusb_pcie_pad *pcie;
> +	struct tegra_xusb_sata_pad *sata;
> +	u32 value;
> +	int err;
> +	int i;
> +
> +	if (tegra210_plle_hw_sequence_is_enabled()) {
> +		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
> +		/* skip pll initialization, update plle refcount only */
> +		if (padctl->pcie) {
> +			pcie = to_pcie_pad(padctl->pcie);
> +			if (pcie->enable == 0) {
> +				err = clk_prepare_enable(pcie->pll);
> +				if (err < 0)
> +					return err;
> +				pcie->enable++;

Do we need all this additional ref counting around clk_prepare_enable?

> +			}
> +		}
> +		if (padctl->sata) {
> +			sata = to_sata_pad(padctl->sata);
> +			if (sata->enable == 0) {
> +				err = clk_prepare_enable(sata->pll);
> +				if (err < 0)
> +					return err;
> +				sata->enable++;

Same here.

> +			}
> +		}
> +		goto skip_pll_init;
> +	}
> +
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_enable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_enable(padctl);
> +
> +	tegra210_plle_hw_sequence_start();
> +
> +skip_pll_init:
> +	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);
> +	}
> +
> +	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);
> +	}
> +
> +	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);
> +
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_disable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_disable(padctl);

What about the clocks that were enabled?

> +}
> +
>  static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
>  				  unsigned int index, bool idle)
>  {
> @@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
>  	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
>  };
>  
> +static int tegra210_usb3_phy_power_on(struct phy *phy)
> +{
> +	struct device *dev = &phy->dev;
> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
> +					    tegra210_usb3_lane_map(lane));

I think that this should be placed on separate lines. Other places you
check the return value of tegra210_usb3_lane_map() but here you don't.
We should be consistent.

> +	int index;
> +	u32 value;
> +
> +	if (!usb3) {
> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
> +		return -ENODEV;
> +	}
> +	index = usb3->base.index;
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> +
> +	if (!usb3->internal)
> +		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> +	else
> +		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> +
> +	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> +	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
> +	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> +
> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> +
> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	return 0;
> +}
> +
> +static int tegra210_usb3_phy_power_off(struct phy *phy)
> +{
> +	struct device *dev = &phy->dev;
> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
> +					    tegra210_usb3_lane_map(lane));
> +	int index;
> +	u32 value;
> +
> +	if (!usb3) {
> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
> +		return -ENODEV;
> +	}
> +	index = usb3->base.index;
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(250, 350);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	return 0;
> +}
>  static struct tegra_xusb_lane *
>  tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
>  			 unsigned int index)
> @@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
>  static int tegra210_pcie_phy_init(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +
> +	mutex_lock(&padctl->lock);
> +
> +	tegra210_uphy_init(padctl);
> +
> +	mutex_unlock(&padctl->lock);
>  
>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>  }
> @@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> -	int err;
> +	int err = 0;
>  
>  	mutex_lock(&padctl->lock);
>  
> -	err = tegra210_pex_uphy_enable(padctl);
> -	if (err < 0)
> -		goto unlock;
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_on(phy);
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> -
> -unlock:
>  	mutex_unlock(&padctl->lock);
>  	return err;
>  }
> @@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> +	int err = 0;
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	mutex_lock(&padctl->lock);
>  
> -	tegra210_pex_uphy_disable(padctl);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_off(phy);
>  
> -	return 0;
> +	mutex_unlock(&padctl->lock);
> +	return err;
>  }
>  
>  static const struct phy_ops tegra210_pcie_phy_ops = {
> @@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
>  static int tegra210_sata_phy_init(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +
> +	mutex_lock(&padctl->lock);
> +
> +	tegra210_uphy_init(padctl);
>  
> +	mutex_unlock(&padctl->lock);
>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>  }
>  
> @@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> -	int err;
> +	int err = 0;
>  
>  	mutex_lock(&padctl->lock);
>  
> -	err = tegra210_sata_uphy_enable(padctl, false);
> -	if (err < 0)
> -		goto unlock;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_on(phy);
>  
> -unlock:
>  	mutex_unlock(&padctl->lock);
>  	return err;
>  }
> @@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> +	int err = 0;
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	mutex_lock(&padctl->lock);
>  
> -	tegra210_sata_uphy_disable(lane->pad->padctl);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_off(phy);
>  
> -	return 0;
> +	mutex_unlock(&padctl->lock);
> +	return err;
>  }
>  
>  static const struct phy_ops tegra210_sata_phy_ops = {
> @@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
>  
>  static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
>  {
> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
> -	struct tegra_xusb_padctl *padctl = port->padctl;
> -	struct tegra_xusb_lane *lane = usb3->base.lane;
> -	unsigned int index = port->index;
> -	u32 value;
> -	int err;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> -
> -	if (!usb3->internal)
> -		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> -	else
> -		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> -
> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
> -
> -	/*
> -	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
> -	 * and conditionalize based on mux function? This seems to work, but
> -	 * might not be the exact proper sequence.
> -	 */
> -	err = regulator_enable(usb3->supply);
> -	if (err < 0)
> -		return err;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> -
> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> -
> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
> -
> -	if (lane->pad == padctl->sata)
> -		err = tegra210_sata_uphy_enable(padctl, true);
> -	else
> -		err = tegra210_pex_uphy_enable(padctl);
> -
> -	if (err) {
> -		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
> -			__func__, err);
> -		return err;
> -	}
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
>  	return 0;
>  }
>  
>  static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
>  {
> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
> -	struct tegra_xusb_padctl *padctl = port->padctl;
> -	struct tegra_xusb_lane *lane = port->lane;
> -	unsigned int index = port->index;
> -	u32 value;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(250, 350);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	if (lane->pad == padctl->sata)
> -		tegra210_sata_uphy_disable(padctl);
> -	else
> -		tegra210_pex_uphy_disable(padctl);
> -
> -	regulator_disable(usb3->supply);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>  }
>  
>  static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
> @@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>  	{ 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",
> +				lane->pad->soc->lanes[lane->index].name,
> +				map->port);
> +			return map->port;
> +		}
> +	}
> +
> +	return -1;

Return a proper errno please.

> +}
> +
>  static struct tegra_xusb_lane *
>  tegra210_usb3_port_map(struct tegra_xusb_port *port)
>  {
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 2ea8497af82a..7fbba53f6097 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
>  	return 0;
>  }
>  
> -static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>  				  const char *function)
>  {
>  	const char *func = lane->soc->funcs[lane->function];
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 093076ca27fd..1bfe14b2a274 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
>  	void (*remove)(struct tegra_xusb_lane *lane);
>  };
>  
> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
> +
>  /*
>   * pads
>   */
> 

-- 
nvpublic

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> This commit is a preparation for enabling XUSB LP0 support.

By LP0 do you mean ELPG? If so please stick to using one name for
referring to the power-state in question.

> It rearranges T210 XUSB PADCTL UPHY initialization sequence,

Please use Tegra210 and not T210.

> 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.
> 
> 2. Once UPHY PLL hardware power sequncer is enabled, do not

s/sequncer/sequencer

>    assert reset to PEX/SATA PLLs.

Maybe worth clarifying why here.
	
> 
> 3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
>    to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
>    and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.
> 
> 4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
>    registers programming from tegra210_usb3_port_enable() to
>    tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
>    XUSB USB3 ports will be programmed at LP0 exit.

Looks like you are moving all the code from the port enable to the phy
enable and after this change the port enable does nothing. Do we not
differentiate between phy and port? I think a bit more description is
necessary here to describe the impact of this change.

> 
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
>  drivers/phy/tegra/xusb.c          |   2 +-
>  drivers/phy/tegra/xusb.h          |   2 +
>  3 files changed, 264 insertions(+), 183 deletions(-)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 18cea8311d22..007bf352b45e 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
>  	return container_of(padctl, struct tegra210_xusb_padctl, base);
>  }
>  
> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
> +

Can we avoid adding this prototype?

>  /* must be called under padctl->lock */
>  static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>  {
> @@ -453,35 +455,44 @@ 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);
> -
> -	mutex_lock(&padctl->lock);
> +	u32 value;
> +	int i;
>  
>  	if (WARN_ON(pcie->enable == 0))
> -		goto unlock;
> +		return;
>  
>  	if (--pcie->enable > 0)
> -		goto unlock;
> +		return;
>  
> -	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);
> -
> -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;
> +	bool usb = false;
>  
>  	if (sata->enable > 0) {
>  		sata->enable++;
>  		return 0;
>  	}
>  
> +	if (!lane)
> +		return 0;
> +
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		usb = true;

This return a boolean type so you can just ...

	usb = tegra_xusb_lane_check(lane, "usb3-ss");

> +
>  	err = clk_prepare_enable(sata->pll);
>  	if (err < 0)
>  		return err;
> @@ -695,30 +706,36 @@ 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);
> -
> -	mutex_lock(&padctl->lock);
> +	u32 value;
> +	int i;
>  
>  	if (WARN_ON(sata->enable == 0))
> -		goto unlock;
> +		return;
>  
>  	if (--sata->enable > 0)
> -		goto unlock;
> +		return;
>  
> -	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)
>  {
> -	u32 value;
> +	return 0;
> +}
>  
> -	mutex_lock(&padctl->lock);
> +static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
> +{
> +	return 0;
> +}

Why bother keeping these functions at all if they now do nothing?

>  
> -	if (padctl->enable++ > 0)
> -		goto out;
> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)

Any reason for renaming these? These appear to deal with the XUSB_PADCTL
and so the previous names seem fine.

> +{
> +	u32 value;
>  
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
> @@ -735,24 +752,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);
> @@ -768,12 +773,76 @@ 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)
> +{
> +	struct tegra_xusb_pcie_pad *pcie;
> +	struct tegra_xusb_sata_pad *sata;
> +	u32 value;
> +	int err;
> +	int i;
> +
> +	if (tegra210_plle_hw_sequence_is_enabled()) {
> +		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
> +		/* skip pll initialization, update plle refcount only */
> +		if (padctl->pcie) {
> +			pcie = to_pcie_pad(padctl->pcie);
> +			if (pcie->enable == 0) {
> +				err = clk_prepare_enable(pcie->pll);
> +				if (err < 0)
> +					return err;
> +				pcie->enable++;

Do we need all this additional ref counting around clk_prepare_enable?

> +			}
> +		}
> +		if (padctl->sata) {
> +			sata = to_sata_pad(padctl->sata);
> +			if (sata->enable == 0) {
> +				err = clk_prepare_enable(sata->pll);
> +				if (err < 0)
> +					return err;
> +				sata->enable++;

Same here.

> +			}
> +		}
> +		goto skip_pll_init;
> +	}
> +
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_enable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_enable(padctl);
> +
> +	tegra210_plle_hw_sequence_start();
> +
> +skip_pll_init:
> +	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);
> +	}
> +
> +	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);
> +	}
> +
> +	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);
> +
> +	if (padctl->pcie)
> +		tegra210_pex_uphy_disable(padctl);
> +	if (padctl->sata)
> +		tegra210_sata_uphy_disable(padctl);

What about the clocks that were enabled?

> +}
> +
>  static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
>  				  unsigned int index, bool idle)
>  {
> @@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
>  	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
>  };
>  
> +static int tegra210_usb3_phy_power_on(struct phy *phy)
> +{
> +	struct device *dev = &phy->dev;
> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
> +					    tegra210_usb3_lane_map(lane));

I think that this should be placed on separate lines. Other places you
check the return value of tegra210_usb3_lane_map() but here you don't.
We should be consistent.

> +	int index;
> +	u32 value;
> +
> +	if (!usb3) {
> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
> +		return -ENODEV;
> +	}
> +	index = usb3->base.index;
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> +
> +	if (!usb3->internal)
> +		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> +	else
> +		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> +
> +	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> +	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
> +	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> +
> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> +
> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	return 0;
> +}
> +
> +static int tegra210_usb3_phy_power_off(struct phy *phy)
> +{
> +	struct device *dev = &phy->dev;
> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
> +					    tegra210_usb3_lane_map(lane));
> +	int index;
> +	u32 value;
> +
> +	if (!usb3) {
> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
> +		return -ENODEV;
> +	}
> +	index = usb3->base.index;
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(100, 200);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	usleep_range(250, 350);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> +
> +	return 0;
> +}
>  static struct tegra_xusb_lane *
>  tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
>  			 unsigned int index)
> @@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
>  static int tegra210_pcie_phy_init(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +
> +	mutex_lock(&padctl->lock);
> +
> +	tegra210_uphy_init(padctl);
> +
> +	mutex_unlock(&padctl->lock);
>  
>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>  }
> @@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> -	int err;
> +	int err = 0;
>  
>  	mutex_lock(&padctl->lock);
>  
> -	err = tegra210_pex_uphy_enable(padctl);
> -	if (err < 0)
> -		goto unlock;
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_on(phy);
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> -
> -unlock:
>  	mutex_unlock(&padctl->lock);
>  	return err;
>  }
> @@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> +	int err = 0;
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	mutex_lock(&padctl->lock);
>  
> -	tegra210_pex_uphy_disable(padctl);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_off(phy);
>  
> -	return 0;
> +	mutex_unlock(&padctl->lock);
> +	return err;
>  }
>  
>  static const struct phy_ops tegra210_pcie_phy_ops = {
> @@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
>  static int tegra210_sata_phy_init(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +
> +	mutex_lock(&padctl->lock);
> +
> +	tegra210_uphy_init(padctl);
>  
> +	mutex_unlock(&padctl->lock);
>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>  }
>  
> @@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> -	int err;
> +	int err = 0;
>  
>  	mutex_lock(&padctl->lock);
>  
> -	err = tegra210_sata_uphy_enable(padctl, false);
> -	if (err < 0)
> -		goto unlock;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_on(phy);
>  
> -unlock:
>  	mutex_unlock(&padctl->lock);
>  	return err;
>  }
> @@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
>  {
>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> -	u32 value;
> +	int err = 0;
>  
> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
> +	mutex_lock(&padctl->lock);
>  
> -	tegra210_sata_uphy_disable(lane->pad->padctl);
> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
> +		err = tegra210_usb3_phy_power_off(phy);
>  
> -	return 0;
> +	mutex_unlock(&padctl->lock);
> +	return err;
>  }
>  
>  static const struct phy_ops tegra210_sata_phy_ops = {
> @@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
>  
>  static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
>  {
> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
> -	struct tegra_xusb_padctl *padctl = port->padctl;
> -	struct tegra_xusb_lane *lane = usb3->base.lane;
> -	unsigned int index = port->index;
> -	u32 value;
> -	int err;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> -
> -	if (!usb3->internal)
> -		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> -	else
> -		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
> -
> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
> -
> -	/*
> -	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
> -	 * and conditionalize based on mux function? This seems to work, but
> -	 * might not be the exact proper sequence.
> -	 */
> -	err = regulator_enable(usb3->supply);
> -	if (err < 0)
> -		return err;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
> -
> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
> -
> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
> -
> -	if (lane->pad == padctl->sata)
> -		err = tegra210_sata_uphy_enable(padctl, true);
> -	else
> -		err = tegra210_pex_uphy_enable(padctl);
> -
> -	if (err) {
> -		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
> -			__func__, err);
> -		return err;
> -	}
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
>  	return 0;
>  }
>  
>  static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
>  {
> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
> -	struct tegra_xusb_padctl *padctl = port->padctl;
> -	struct tegra_xusb_lane *lane = port->lane;
> -	unsigned int index = port->index;
> -	u32 value;
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(100, 200);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	usleep_range(250, 350);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
> -
> -	if (lane->pad == padctl->sata)
> -		tegra210_sata_uphy_disable(padctl);
> -	else
> -		tegra210_pex_uphy_disable(padctl);
> -
> -	regulator_disable(usb3->supply);
> -
> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>  }
>  
>  static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
> @@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>  	{ 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",
> +				lane->pad->soc->lanes[lane->index].name,
> +				map->port);
> +			return map->port;
> +		}
> +	}
> +
> +	return -1;

Return a proper errno please.

> +}
> +
>  static struct tegra_xusb_lane *
>  tegra210_usb3_port_map(struct tegra_xusb_port *port)
>  {
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 2ea8497af82a..7fbba53f6097 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
>  	return 0;
>  }
>  
> -static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>  				  const char *function)
>  {
>  	const char *func = lane->soc->funcs[lane->function];
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 093076ca27fd..1bfe14b2a274 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
>  	void (*remove)(struct tegra_xusb_lane *lane);
>  };
>  
> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
> +
>  /*
>   * pads
>   */
> 

-- 
nvpublic

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[JC Kuo]

On 7/4/19 9:32 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> This commit is a preparation for enabling XUSB LP0 support.
> 
> By LP0 do you mean ELPG? If so please stick to using one name for
> referring to the power-state in question.
> 
>> It rearranges T210 XUSB PADCTL UPHY initialization sequence,
> 
> Please use Tegra210 and not T210.
Thanks, I will correct this.
> 
>> 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.
>>
>> 2. Once UPHY PLL hardware power sequncer is enabled, do not
> 
> s/sequncer/sequencer
Thanks, I will correct this.
> 
>>    assert reset to PEX/SATA PLLs.
> 
> Maybe worth clarifying why here.
When UPHY PLLs are managed by hardware power sequencers, asserting reset to the
PLL will break PLL and sequencer functionality.
> 	
>>
>> 3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
>>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
>>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
>>    to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
>>    and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.
>>
>> 4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
>>    registers programming from tegra210_usb3_port_enable() to
>>    tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
>>    XUSB USB3 ports will be programmed at LP0 exit.
> 
> Looks like you are moving all the code from the port enable to the phy
> enable and after this change the port enable does nothing. Do we not
> differentiate between phy and port? I think a bit more description is
> necessary here to describe the impact of this change.
Sorry that I am not sure whether I should use "LP0" or "SC7" for Linux system
suspend (either to ram or disk). Should I use SC7 instead of LP0?
*_port_enable() APIs will only get invoked once in driver .probe(). At system
resume, hardware (XUSB PADCTL block) is in power-on-reset state. We need to
program hardware once again, so I moved the programming sequence to
*_phy_power_on() that will be invoked at system resume.
> 
>>
>> Signed-off-by: JC Kuo <jckuo@nvidia.com>
>> ---
>>  drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
>>  drivers/phy/tegra/xusb.c          |   2 +-
>>  drivers/phy/tegra/xusb.h          |   2 +
>>  3 files changed, 264 insertions(+), 183 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
>> index 18cea8311d22..007bf352b45e 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
>>  	return container_of(padctl, struct tegra210_xusb_padctl, base);
>>  }
>>  
>> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
>> +
> 
> Can we avoid adding this prototype?
Thanks, I will move tegra210_usb3_lane_map() function here.
> 
>>  /* must be called under padctl->lock */
>>  static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>>  {
>> @@ -453,35 +455,44 @@ 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);
>> -
>> -	mutex_lock(&padctl->lock);
>> +	u32 value;
>> +	int i;
>>  
>>  	if (WARN_ON(pcie->enable == 0))
>> -		goto unlock;
>> +		return;
>>  
>>  	if (--pcie->enable > 0)
>> -		goto unlock;
>> +		return;
>>  
>> -	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);
>> -
>> -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;
>> +	bool usb = false;
>>  
>>  	if (sata->enable > 0) {
>>  		sata->enable++;
>>  		return 0;
>>  	}
>>  
>> +	if (!lane)
>> +		return 0;
>> +
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		usb = true;
> 
> This return a boolean type so you can just ...
> 
> 	usb = tegra_xusb_lane_check(lane, "usb3-ss");
Thanks. I will modify this.
> 
>> +
>>  	err = clk_prepare_enable(sata->pll);
>>  	if (err < 0)
>>  		return err;
>> @@ -695,30 +706,36 @@ 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);
>> -
>> -	mutex_lock(&padctl->lock);
>> +	u32 value;
>> +	int i;
>>  
>>  	if (WARN_ON(sata->enable == 0))
>> -		goto unlock;
>> +		return;
>>  
>>  	if (--sata->enable > 0)
>> -		goto unlock;
>> +		return;
>>  
>> -	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)
>>  {
>> -	u32 value;
>> +	return 0;
>> +}
>>  
>> -	mutex_lock(&padctl->lock);
>> +static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
>> +{
>> +	return 0;
>> +}
> 
> Why bother keeping these functions at all if they now do nothing?
Thanks. I will remove the functions.
> 
>>  
>> -	if (padctl->enable++ > 0)
>> -		goto out;
>> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)
> 
> Any reason for renaming these? These appear to deal with the XUSB_PADCTL
> and so the previous names seem fine.
> 
To me, enabling XUSB_PADCTL means the sequence of 1) enabling clock to XUSB
PADCTL and 2) de-asserting reset to XUSB PADCTL; disabling XUSB_PADCTL means the
reverse operation. These two functions are for toggling clamping and vcore_down
signals which can be done only after XUSB_PADCTL is enabled.

>> +{
>> +	u32 value;
>>  
>>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
>> @@ -735,24 +752,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);
>> @@ -768,12 +773,76 @@ 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)
>> +{
>> +	struct tegra_xusb_pcie_pad *pcie;
>> +	struct tegra_xusb_sata_pad *sata;
>> +	u32 value;
>> +	int err;
>> +	int i;
>> +
>> +	if (tegra210_plle_hw_sequence_is_enabled()) {
>> +		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
>> +		/* skip pll initialization, update plle refcount only */
>> +		if (padctl->pcie) {
>> +			pcie = to_pcie_pad(padctl->pcie);
>> +			if (pcie->enable == 0) {
>> +				err = clk_prepare_enable(pcie->pll);
>> +				if (err < 0)
>> +					return err;
>> +				pcie->enable++;
> 
> Do we need all this additional ref counting around clk_prepare_enable?
I will change pcie->enable/sata->enable to be boolean type variable. Thanks.
> 
>> +			}
>> +		}
>> +		if (padctl->sata) {
>> +			sata = to_sata_pad(padctl->sata);
>> +			if (sata->enable == 0) {
>> +				err = clk_prepare_enable(sata->pll);
>> +				if (err < 0)
>> +					return err;
>> +				sata->enable++;
> 
> Same here.
> 
>> +			}
>> +		}
>> +		goto skip_pll_init;
>> +	}
>> +
>> +	if (padctl->pcie)
>> +		tegra210_pex_uphy_enable(padctl);
>> +	if (padctl->sata)
>> +		tegra210_sata_uphy_enable(padctl);
>> +
>> +	tegra210_plle_hw_sequence_start();
>> +
>> +skip_pll_init:
>> +	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);
>> +	}
>> +
>> +	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);
>> +	}
>> +
>> +	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);
>> +
>> +	if (padctl->pcie)
>> +		tegra210_pex_uphy_disable(padctl);
>> +	if (padctl->sata)
>> +		tegra210_sata_uphy_disable(padctl);
> 
> What about the clocks that were enabled?
pcie->pll will be disabled in tegra210_pex_uphy_disable().
sata->pll will be disabled in tegra210_sata_uphy_disable().

> 
>> +}
>> +
>>  static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
>>  				  unsigned int index, bool idle)
>>  {
>> @@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
>>  	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
>>  };
>>  
>> +static int tegra210_usb3_phy_power_on(struct phy *phy)
>> +{
>> +	struct device *dev = &phy->dev;
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
>> +					    tegra210_usb3_lane_map(lane));
> 
> I think that this should be placed on separate lines. Other places you
> check the return value of tegra210_usb3_lane_map() but here you don't.
> We should be consistent.
Thanks. I will modify this.
> 
>> +	int index;
>> +	u32 value;
>> +
>> +	if (!usb3) {
>> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
>> +		return -ENODEV;
>> +	}
>> +	index = usb3->base.index;
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> +
>> +	if (!usb3->internal)
>> +		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> +	else
>> +		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> +
>> +	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> +	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> +
>> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
>> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> +
>> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
>> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra210_usb3_phy_power_off(struct phy *phy)
>> +{
>> +	struct device *dev = &phy->dev;
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
>> +					    tegra210_usb3_lane_map(lane));
>> +	int index;
>> +	u32 value;
>> +
>> +	if (!usb3) {
>> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
>> +		return -ENODEV;
>> +	}
>> +	index = usb3->base.index;
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(250, 350);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	return 0;
>> +}
>>  static struct tegra_xusb_lane *
>>  tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
>>  			 unsigned int index)
>> @@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
>>  static int tegra210_pcie_phy_init(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	mutex_lock(&padctl->lock);
>> +
>> +	tegra210_uphy_init(padctl);
>> +
>> +	mutex_unlock(&padctl->lock);
>>  
>>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>>  }
>> @@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> -	int err;
>> +	int err = 0;
>>  
>>  	mutex_lock(&padctl->lock);
>>  
>> -	err = tegra210_pex_uphy_enable(padctl);
>> -	if (err < 0)
>> -		goto unlock;
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_on(phy);
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> -
>> -unlock:
>>  	mutex_unlock(&padctl->lock);
>>  	return err;
>>  }
>> @@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> +	int err = 0;
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	mutex_lock(&padctl->lock);
>>  
>> -	tegra210_pex_uphy_disable(padctl);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_off(phy);
>>  
>> -	return 0;
>> +	mutex_unlock(&padctl->lock);
>> +	return err;
>>  }
>>  
>>  static const struct phy_ops tegra210_pcie_phy_ops = {
>> @@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
>>  static int tegra210_sata_phy_init(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	mutex_lock(&padctl->lock);
>> +
>> +	tegra210_uphy_init(padctl);
>>  
>> +	mutex_unlock(&padctl->lock);
>>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>>  }
>>  
>> @@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> -	int err;
>> +	int err = 0;
>>  
>>  	mutex_lock(&padctl->lock);
>>  
>> -	err = tegra210_sata_uphy_enable(padctl, false);
>> -	if (err < 0)
>> -		goto unlock;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_on(phy);
>>  
>> -unlock:
>>  	mutex_unlock(&padctl->lock);
>>  	return err;
>>  }
>> @@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> +	int err = 0;
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	mutex_lock(&padctl->lock);
>>  
>> -	tegra210_sata_uphy_disable(lane->pad->padctl);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_off(phy);
>>  
>> -	return 0;
>> +	mutex_unlock(&padctl->lock);
>> +	return err;
>>  }
>>  
>>  static const struct phy_ops tegra210_sata_phy_ops = {
>> @@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
>>  
>>  static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
>>  {
>> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
>> -	struct tegra_xusb_padctl *padctl = port->padctl;
>> -	struct tegra_xusb_lane *lane = usb3->base.lane;
>> -	unsigned int index = port->index;
>> -	u32 value;
>> -	int err;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> -
>> -	if (!usb3->internal)
>> -		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> -	else
>> -		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> -
>> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> -
>> -	/*
>> -	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
>> -	 * and conditionalize based on mux function? This seems to work, but
>> -	 * might not be the exact proper sequence.
>> -	 */
>> -	err = regulator_enable(usb3->supply);
>> -	if (err < 0)
>> -		return err;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> -
>> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
>> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> -
>> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
>> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
>> -
>> -	if (lane->pad == padctl->sata)
>> -		err = tegra210_sata_uphy_enable(padctl, true);
>> -	else
>> -		err = tegra210_pex_uphy_enable(padctl);
>> -
>> -	if (err) {
>> -		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
>> -			__func__, err);
>> -		return err;
>> -	}
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>>  	return 0;
>>  }
>>  
>>  static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
>>  {
>> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
>> -	struct tegra_xusb_padctl *padctl = port->padctl;
>> -	struct tegra_xusb_lane *lane = port->lane;
>> -	unsigned int index = port->index;
>> -	u32 value;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(250, 350);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	if (lane->pad == padctl->sata)
>> -		tegra210_sata_uphy_disable(padctl);
>> -	else
>> -		tegra210_pex_uphy_disable(padctl);
>> -
>> -	regulator_disable(usb3->supply);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>>  }
>>  
>>  static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>> @@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>>  	{ 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",
>> +				lane->pad->soc->lanes[lane->index].name,
>> +				map->port);
>> +			return map->port;
>> +		}
>> +	}
>> +
>> +	return -1;
> 
> Return a proper errno please.
I will change the errno to be -EINVAL. Thanks.
> 
>> +}
>> +
>>  static struct tegra_xusb_lane *
>>  tegra210_usb3_port_map(struct tegra_xusb_port *port)
>>  {
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index 2ea8497af82a..7fbba53f6097 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
>>  	return 0;
>>  }
>>  
>> -static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>>  				  const char *function)
>>  {
>>  	const char *func = lane->soc->funcs[lane->function];
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 093076ca27fd..1bfe14b2a274 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
>>  	void (*remove)(struct tegra_xusb_lane *lane);
>>  };
>>  
>> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
>> +
>>  /*
>>   * pads
>>   */
>>
> 

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[JC Kuo]

On 7/4/19 9:32 PM, Jon Hunter wrote:
> 
> On 14/06/2019 08:46, JC Kuo wrote:
>> This commit is a preparation for enabling XUSB LP0 support.
> 
> By LP0 do you mean ELPG? If so please stick to using one name for
> referring to the power-state in question.
> 
>> It rearranges T210 XUSB PADCTL UPHY initialization sequence,
> 
> Please use Tegra210 and not T210.
Thanks, I will correct this.
> 
>> 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.
>>
>> 2. Once UPHY PLL hardware power sequncer is enabled, do not
> 
> s/sequncer/sequencer
Thanks, I will correct this.
> 
>>    assert reset to PEX/SATA PLLs.
> 
> Maybe worth clarifying why here.
When UPHY PLLs are managed by hardware power sequencers, asserting reset to the
PLL will break PLL and sequencer functionality.
> 	
>>
>> 3. At LP0 exit, XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN,
>>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY, and
>>    XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN bits have
>>    to be cleared after XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE
>>    and XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE bits get set.
>>
>> 4. Move XUSB_PADCTL_SS_PORT_MAP and XUSB_PADCTL_UPHY_USB3_PADX_ECTL*
>>    registers programming from tegra210_usb3_port_enable() to
>>    tegra210_pcie_phy_power_on()/tegra210_sata_phy_power_on() so that
>>    XUSB USB3 ports will be programmed at LP0 exit.
> 
> Looks like you are moving all the code from the port enable to the phy
> enable and after this change the port enable does nothing. Do we not
> differentiate between phy and port? I think a bit more description is
> necessary here to describe the impact of this change.
Sorry that I am not sure whether I should use "LP0" or "SC7" for Linux system
suspend (either to ram or disk). Should I use SC7 instead of LP0?
*_port_enable() APIs will only get invoked once in driver .probe(). At system
resume, hardware (XUSB PADCTL block) is in power-on-reset state. We need to
program hardware once again, so I moved the programming sequence to
*_phy_power_on() that will be invoked at system resume.
> 
>>
>> Signed-off-by: JC Kuo <jckuo@nvidia.com>
>> ---
>>  drivers/phy/tegra/xusb-tegra210.c | 443 ++++++++++++++++++------------
>>  drivers/phy/tegra/xusb.c          |   2 +-
>>  drivers/phy/tegra/xusb.h          |   2 +
>>  3 files changed, 264 insertions(+), 183 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
>> index 18cea8311d22..007bf352b45e 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -240,6 +240,8 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl)
>>  	return container_of(padctl, struct tegra210_xusb_padctl, base);
>>  }
>>  
>> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane);
>> +
> 
> Can we avoid adding this prototype?
Thanks, I will move tegra210_usb3_lane_map() function here.
> 
>>  /* must be called under padctl->lock */
>>  static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl)
>>  {
>> @@ -453,35 +455,44 @@ 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);
>> -
>> -	mutex_lock(&padctl->lock);
>> +	u32 value;
>> +	int i;
>>  
>>  	if (WARN_ON(pcie->enable == 0))
>> -		goto unlock;
>> +		return;
>>  
>>  	if (--pcie->enable > 0)
>> -		goto unlock;
>> +		return;
>>  
>> -	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);
>> -
>> -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;
>> +	bool usb = false;
>>  
>>  	if (sata->enable > 0) {
>>  		sata->enable++;
>>  		return 0;
>>  	}
>>  
>> +	if (!lane)
>> +		return 0;
>> +
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		usb = true;
> 
> This return a boolean type so you can just ...
> 
> 	usb = tegra_xusb_lane_check(lane, "usb3-ss");
Thanks. I will modify this.
> 
>> +
>>  	err = clk_prepare_enable(sata->pll);
>>  	if (err < 0)
>>  		return err;
>> @@ -695,30 +706,36 @@ 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);
>> -
>> -	mutex_lock(&padctl->lock);
>> +	u32 value;
>> +	int i;
>>  
>>  	if (WARN_ON(sata->enable == 0))
>> -		goto unlock;
>> +		return;
>>  
>>  	if (--sata->enable > 0)
>> -		goto unlock;
>> +		return;
>>  
>> -	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)
>>  {
>> -	u32 value;
>> +	return 0;
>> +}
>>  
>> -	mutex_lock(&padctl->lock);
>> +static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl)
>> +{
>> +	return 0;
>> +}
> 
> Why bother keeping these functions at all if they now do nothing?
Thanks. I will remove the functions.
> 
>>  
>> -	if (padctl->enable++ > 0)
>> -		goto out;
>> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl)
> 
> Any reason for renaming these? These appear to deal with the XUSB_PADCTL
> and so the previous names seem fine.
> 
To me, enabling XUSB_PADCTL means the sequence of 1) enabling clock to XUSB
PADCTL and 2) de-asserting reset to XUSB PADCTL; disabling XUSB_PADCTL means the
reverse operation. These two functions are for toggling clamping and vcore_down
signals which can be done only after XUSB_PADCTL is enabled.

>> +{
>> +	u32 value;
>>  
>>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>>  	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN;
>> @@ -735,24 +752,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);
>> @@ -768,12 +773,76 @@ 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)
>> +{
>> +	struct tegra_xusb_pcie_pad *pcie;
>> +	struct tegra_xusb_sata_pad *sata;
>> +	u32 value;
>> +	int err;
>> +	int i;
>> +
>> +	if (tegra210_plle_hw_sequence_is_enabled()) {
>> +		dev_dbg(padctl->dev, "PLLE is already in HW control\n");
>> +		/* skip pll initialization, update plle refcount only */
>> +		if (padctl->pcie) {
>> +			pcie = to_pcie_pad(padctl->pcie);
>> +			if (pcie->enable == 0) {
>> +				err = clk_prepare_enable(pcie->pll);
>> +				if (err < 0)
>> +					return err;
>> +				pcie->enable++;
> 
> Do we need all this additional ref counting around clk_prepare_enable?
I will change pcie->enable/sata->enable to be boolean type variable. Thanks.
> 
>> +			}
>> +		}
>> +		if (padctl->sata) {
>> +			sata = to_sata_pad(padctl->sata);
>> +			if (sata->enable == 0) {
>> +				err = clk_prepare_enable(sata->pll);
>> +				if (err < 0)
>> +					return err;
>> +				sata->enable++;
> 
> Same here.
> 
>> +			}
>> +		}
>> +		goto skip_pll_init;
>> +	}
>> +
>> +	if (padctl->pcie)
>> +		tegra210_pex_uphy_enable(padctl);
>> +	if (padctl->sata)
>> +		tegra210_sata_uphy_enable(padctl);
>> +
>> +	tegra210_plle_hw_sequence_start();
>> +
>> +skip_pll_init:
>> +	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);
>> +	}
>> +
>> +	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);
>> +	}
>> +
>> +	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);
>> +
>> +	if (padctl->pcie)
>> +		tegra210_pex_uphy_disable(padctl);
>> +	if (padctl->sata)
>> +		tegra210_sata_uphy_disable(padctl);
> 
> What about the clocks that were enabled?
pcie->pll will be disabled in tegra210_pex_uphy_disable().
sata->pll will be disabled in tegra210_sata_uphy_disable().

> 
>> +}
>> +
>>  static int tegra210_hsic_set_idle(struct tegra_xusb_padctl *padctl,
>>  				  unsigned int index, bool idle)
>>  {
>> @@ -1420,6 +1489,113 @@ static const struct tegra_xusb_lane_soc tegra210_pcie_lanes[] = {
>>  	TEGRA210_LANE("pcie-6", 0x028, 24, 0x3, pcie),
>>  };
>>  
>> +static int tegra210_usb3_phy_power_on(struct phy *phy)
>> +{
>> +	struct device *dev = &phy->dev;
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
>> +					    tegra210_usb3_lane_map(lane));
> 
> I think that this should be placed on separate lines. Other places you
> check the return value of tegra210_usb3_lane_map() but here you don't.
> We should be consistent.
Thanks. I will modify this.
> 
>> +	int index;
>> +	u32 value;
>> +
>> +	if (!usb3) {
>> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
>> +		return -ENODEV;
>> +	}
>> +	index = usb3->base.index;
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> +
>> +	if (!usb3->internal)
>> +		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> +	else
>> +		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> +
>> +	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> +	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> +
>> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
>> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> +	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
>> +		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
>> +	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
>> +		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> +
>> +	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
>> +		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra210_usb3_phy_power_off(struct phy *phy)
>> +{
>> +	struct device *dev = &phy->dev;
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +	struct tegra_xusb_usb3_port *usb3 = tegra_xusb_find_usb3_port(padctl,
>> +					    tegra210_usb3_lane_map(lane));
>> +	int index;
>> +	u32 value;
>> +
>> +	if (!usb3) {
>> +		dev_err(dev, "no USB3 port found for lane %u\n", lane->index);
>> +		return -ENODEV;
>> +	}
>> +	index = usb3->base.index;
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(100, 200);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	usleep_range(250, 350);
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> +	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> +
>> +	return 0;
>> +}
>>  static struct tegra_xusb_lane *
>>  tegra210_pcie_lane_probe(struct tegra_xusb_pad *pad, struct device_node *np,
>>  			 unsigned int index)
>> @@ -1461,6 +1637,13 @@ static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
>>  static int tegra210_pcie_phy_init(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	mutex_lock(&padctl->lock);
>> +
>> +	tegra210_uphy_init(padctl);
>> +
>> +	mutex_unlock(&padctl->lock);
>>  
>>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>>  }
>> @@ -1476,20 +1659,13 @@ static int tegra210_pcie_phy_power_on(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> -	int err;
>> +	int err = 0;
>>  
>>  	mutex_lock(&padctl->lock);
>>  
>> -	err = tegra210_pex_uphy_enable(padctl);
>> -	if (err < 0)
>> -		goto unlock;
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_on(phy);
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> -
>> -unlock:
>>  	mutex_unlock(&padctl->lock);
>>  	return err;
>>  }
>> @@ -1498,15 +1674,15 @@ static int tegra210_pcie_phy_power_off(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> +	int err = 0;
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	mutex_lock(&padctl->lock);
>>  
>> -	tegra210_pex_uphy_disable(padctl);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_off(phy);
>>  
>> -	return 0;
>> +	mutex_unlock(&padctl->lock);
>> +	return err;
>>  }
>>  
>>  static const struct phy_ops tegra210_pcie_phy_ops = {
>> @@ -1632,7 +1808,13 @@ static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
>>  static int tegra210_sata_phy_init(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	mutex_lock(&padctl->lock);
>> +
>> +	tegra210_uphy_init(padctl);
>>  
>> +	mutex_unlock(&padctl->lock);
>>  	return tegra210_xusb_padctl_enable(lane->pad->padctl);
>>  }
>>  
>> @@ -1647,20 +1829,13 @@ static int tegra210_sata_phy_power_on(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> -	int err;
>> +	int err = 0;
>>  
>>  	mutex_lock(&padctl->lock);
>>  
>> -	err = tegra210_sata_uphy_enable(padctl, false);
>> -	if (err < 0)
>> -		goto unlock;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_on(phy);
>>  
>> -unlock:
>>  	mutex_unlock(&padctl->lock);
>>  	return err;
>>  }
>> @@ -1669,15 +1844,15 @@ static int tegra210_sata_phy_power_off(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> -	u32 value;
>> +	int err = 0;
>>  
>> -	value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX);
>> -	value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(lane->index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX);
>> +	mutex_lock(&padctl->lock);
>>  
>> -	tegra210_sata_uphy_disable(lane->pad->padctl);
>> +	if (tegra_xusb_lane_check(lane, "usb3-ss"))
>> +		err = tegra210_usb3_phy_power_off(phy);
>>  
>> -	return 0;
>> +	mutex_unlock(&padctl->lock);
>> +	return err;
>>  }
>>  
>>  static const struct phy_ops tegra210_sata_phy_ops = {
>> @@ -1802,125 +1977,11 @@ static const struct tegra_xusb_port_ops tegra210_hsic_port_ops = {
>>  
>>  static int tegra210_usb3_port_enable(struct tegra_xusb_port *port)
>>  {
>> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
>> -	struct tegra_xusb_padctl *padctl = port->padctl;
>> -	struct tegra_xusb_lane *lane = usb3->base.lane;
>> -	unsigned int index = port->index;
>> -	u32 value;
>> -	int err;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> -
>> -	if (!usb3->internal)
>> -		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> -	else
>> -		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_INTERNAL(index);
>> -
>> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, usb3->port);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> -
>> -	/*
>> -	 * TODO: move this code into the PCIe/SATA PHY ->power_on() callbacks
>> -	 * and conditionalize based on mux function? This seems to work, but
>> -	 * might not be the exact proper sequence.
>> -	 */
>> -	err = regulator_enable(usb3->supply);
>> -	if (err < 0)
>> -		return err;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(index));
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL2_RX_CTLE_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL2(index));
>> -
>> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL3_RX_DFE_VAL,
>> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL3(index));
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> -	value &= ~(XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_MASK <<
>> -		   XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT);
>> -	value |= XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_VAL <<
>> -		 XUSB_PADCTL_UPHY_USB3_PAD_ECTL4_RX_CDR_CTRL_SHIFT;
>> -	padctl_writel(padctl, value, XUSB_PADCTL_UPHY_USB3_PADX_ECTL4(index));
>> -
>> -	padctl_writel(padctl, XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL,
>> -		      XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(index));
>> -
>> -	if (lane->pad == padctl->sata)
>> -		err = tegra210_sata_uphy_enable(padctl, true);
>> -	else
>> -		err = tegra210_pex_uphy_enable(padctl);
>> -
>> -	if (err) {
>> -		dev_err(&port->dev, "%s: failed to enable UPHY: %d\n",
>> -			__func__, err);
>> -		return err;
>> -	}
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>>  	return 0;
>>  }
>>  
>>  static void tegra210_usb3_port_disable(struct tegra_xusb_port *port)
>>  {
>> -	struct tegra_xusb_usb3_port *usb3 = to_usb3_port(port);
>> -	struct tegra_xusb_padctl *padctl = port->padctl;
>> -	struct tegra_xusb_lane *lane = port->lane;
>> -	unsigned int index = port->index;
>> -	u32 value;
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(100, 200);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	usleep_range(250, 350);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
>> -	value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(index);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
>> -
>> -	if (lane->pad == padctl->sata)
>> -		tegra210_sata_uphy_disable(padctl);
>> -	else
>> -		tegra210_pex_uphy_disable(padctl);
>> -
>> -	regulator_disable(usb3->supply);
>> -
>> -	value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> -	value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(index);
>> -	value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(index, 0x7);
>> -	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>>  }
>>  
>>  static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>> @@ -1933,6 +1994,24 @@ static const struct tegra_xusb_lane_map tegra210_usb3_map[] = {
>>  	{ 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",
>> +				lane->pad->soc->lanes[lane->index].name,
>> +				map->port);
>> +			return map->port;
>> +		}
>> +	}
>> +
>> +	return -1;
> 
> Return a proper errno please.
I will change the errno to be -EINVAL. Thanks.
> 
>> +}
>> +
>>  static struct tegra_xusb_lane *
>>  tegra210_usb3_port_map(struct tegra_xusb_port *port)
>>  {
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index 2ea8497af82a..7fbba53f6097 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -370,7 +370,7 @@ static int tegra_xusb_setup_pads(struct tegra_xusb_padctl *padctl)
>>  	return 0;
>>  }
>>  
>> -static bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane,
>>  				  const char *function)
>>  {
>>  	const char *func = lane->soc->funcs[lane->function];
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 093076ca27fd..1bfe14b2a274 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -127,6 +127,8 @@ struct tegra_xusb_lane_ops {
>>  	void (*remove)(struct tegra_xusb_lane *lane);
>>  };
>>  
>> +bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
>> +
>>  /*
>>   * pads
>>   */
>>
> 

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[Peter De Schrijver]

On Fri, Jul 05, 2019 at 02:48:49PM +0800, JC Kuo wrote:
> > Looks like you are moving all the code from the port enable to the phy
> > enable and after this change the port enable does nothing. Do we not
> > differentiate between phy and port? I think a bit more description is
> > necessary here to describe the impact of this change.
> Sorry that I am not sure whether I should use "LP0" or "SC7" for Linux system
> suspend (either to ram or disk). Should I use SC7 instead of LP0?

Please use SC7 rather than LP0.

Peter.

Re: [PATCH 3/8] phy: tegra: xusb: t210: rearrange UPHY init

[Peter De Schrijver]

On Fri, Jul 05, 2019 at 02:48:49PM +0800, JC Kuo wrote:
> > Looks like you are moving all the code from the port enable to the phy
> > enable and after this change the port enable does nothing. Do we not
> > differentiate between phy and port? I think a bit more description is
> > necessary here to describe the impact of this change.
> Sorry that I am not sure whether I should use "LP0" or "SC7" for Linux system
> suspend (either to ram or disk). Should I use SC7 instead of LP0?

Please use SC7 rather than LP0.

Peter.

[PATCH 4/8] phy: tegra: xusb: add sleepwalk and suspend/resume

[JC Kuo]

This commit adds sleepwalk/wake and suspend/resume interfaces
to Tegra XUSB PHY driver.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/phy/tegra/xusb.c       | 78 ++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.h       |  8 ++++
 include/linux/phy/tegra/xusb.h | 12 ++++++
 3 files changed, 98 insertions(+)

diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
index 7fbba53f6097..14461d59b947 100644
--- a/drivers/phy/tegra/xusb.c
+++ b/drivers/phy/tegra/xusb.c
@@ -975,10 +975,36 @@ static int tegra_xusb_padctl_remove(struct platform_device *pdev)
 	return err;
 }
 
+static int tegra_xusb_padctl_suspend_noirq(struct device *dev)
+{
+	struct tegra_xusb_padctl *padctl = dev_get_drvdata(dev);
+
+	if (padctl->soc->ops->suspend_noirq)
+		return padctl->soc->ops->suspend_noirq(padctl);
+
+	return 0;
+}
+
+static int tegra_xusb_padctl_resume_noirq(struct device *dev)
+{
+	struct tegra_xusb_padctl *padctl = dev_get_drvdata(dev);
+
+	if (padctl->soc->ops->resume_noirq)
+		return padctl->soc->ops->resume_noirq(padctl);
+
+	return 0;
+}
+
+static const struct dev_pm_ops tegra_xusb_padctl_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_xusb_padctl_suspend_noirq,
+				      tegra_xusb_padctl_resume_noirq)
+};
+
 static struct platform_driver tegra_xusb_padctl_driver = {
 	.driver = {
 		.name = "tegra-xusb-padctl",
 		.of_match_table = tegra_xusb_padctl_of_match,
+		.pm = &tegra_xusb_padctl_pm_ops,
 	},
 	.probe = tegra_xusb_padctl_probe,
 	.remove = tegra_xusb_padctl_remove,
@@ -1045,6 +1071,58 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 }
 EXPORT_SYMBOL_GPL(tegra_xusb_padctl_hsic_set_idle);
 
+int tegra_xusb_padctl_enable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy,
+					   enum usb_device_speed speed)
+{
+	if (padctl->soc->ops->phy_sleepwalk)
+		return padctl->soc->ops->phy_sleepwalk(padctl, phy, true,
+						       speed);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_enable_phy_sleepwalk);
+
+int tegra_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					    struct phy *phy)
+{
+	if (padctl->soc->ops->phy_sleepwalk)
+		return padctl->soc->ops->phy_sleepwalk(padctl, phy, false, 0);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_disable_phy_sleepwalk);
+
+int tegra_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl,
+				      struct phy *phy)
+{
+	if (padctl->soc->ops->phy_wake)
+		return padctl->soc->ops->phy_wake(padctl, phy, true);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_enable_phy_wake);
+
+int tegra_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl,
+				       struct phy *phy)
+{
+	if (padctl->soc->ops->phy_wake)
+		return padctl->soc->ops->phy_wake(padctl, phy, false);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_disable_phy_wake);
+
+int tegra_xusb_padctl_remote_wake_detected(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy)
+{
+	if (padctl->soc->ops->remote_wake_detected)
+		return padctl->soc->ops->remote_wake_detected(phy);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_remote_wake_detected);
+
 int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
 					   unsigned int port, bool enable)
 {
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 1bfe14b2a274..9482914536ac 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -11,6 +11,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 
+#include <linux/usb/ch9.h>
 #include <linux/usb/otg.h>
 
 /* legacy entry points for backwards-compatibility */
@@ -368,12 +369,19 @@ struct tegra_xusb_padctl_ops {
 			 const struct tegra_xusb_padctl_soc *soc);
 	void (*remove)(struct tegra_xusb_padctl *padctl);
 
+	int (*suspend_noirq)(struct tegra_xusb_padctl *padctl);
+	int (*resume_noirq)(struct tegra_xusb_padctl *padctl);
 	int (*usb3_save_context)(struct tegra_xusb_padctl *padctl,
 				 unsigned int index);
 	int (*hsic_set_idle)(struct tegra_xusb_padctl *padctl,
 			     unsigned int index, bool idle);
 	int (*usb3_set_lfps_detect)(struct tegra_xusb_padctl *padctl,
 				    unsigned int index, bool enable);
+	int (*phy_sleepwalk)(struct tegra_xusb_padctl *padctl, struct phy *phy,
+			     bool enable, enum usb_device_speed speed);
+	int (*phy_wake)(struct tegra_xusb_padctl *padctl, struct phy *phy,
+			bool enable);
+	int (*remote_wake_detected)(struct phy *phy);
 };
 
 struct tegra_xusb_padctl_soc {
diff --git a/include/linux/phy/tegra/xusb.h b/include/linux/phy/tegra/xusb.h
index ee59562c8354..2fb12e3baaee 100644
--- a/include/linux/phy/tegra/xusb.h
+++ b/include/linux/phy/tegra/xusb.h
@@ -8,6 +8,7 @@
 
 struct tegra_xusb_padctl;
 struct device;
+enum usb_device_speed;
 
 struct tegra_xusb_padctl *tegra_xusb_padctl_get(struct device *dev);
 void tegra_xusb_padctl_put(struct tegra_xusb_padctl *padctl);
@@ -18,5 +19,16 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 				    unsigned int port, bool idle);
 int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
 					   unsigned int port, bool enable);
+int tegra_xusb_padctl_enable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy,
+					   enum usb_device_speed speed);
+int tegra_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy);
+int tegra_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl,
+				      struct phy *phy);
+int tegra_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl,
+				       struct phy *phy);
+int tegra_xusb_padctl_remote_wake_detected(struct tegra_xusb_padctl *padctl,
+					struct phy *phy);
 
 #endif /* PHY_TEGRA_XUSB_H */
-- 
2.17.1

[PATCH 4/8] phy: tegra: xusb: add sleepwalk and suspend/resume

[JC Kuo]

This commit adds sleepwalk/wake and suspend/resume interfaces
to Tegra XUSB PHY driver.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/phy/tegra/xusb.c       | 78 ++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.h       |  8 ++++
 include/linux/phy/tegra/xusb.h | 12 ++++++
 3 files changed, 98 insertions(+)

diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
index 7fbba53f6097..14461d59b947 100644
--- a/drivers/phy/tegra/xusb.c
+++ b/drivers/phy/tegra/xusb.c
@@ -975,10 +975,36 @@ static int tegra_xusb_padctl_remove(struct platform_device *pdev)
 	return err;
 }
 
+static int tegra_xusb_padctl_suspend_noirq(struct device *dev)
+{
+	struct tegra_xusb_padctl *padctl = dev_get_drvdata(dev);
+
+	if (padctl->soc->ops->suspend_noirq)
+		return padctl->soc->ops->suspend_noirq(padctl);
+
+	return 0;
+}
+
+static int tegra_xusb_padctl_resume_noirq(struct device *dev)
+{
+	struct tegra_xusb_padctl *padctl = dev_get_drvdata(dev);
+
+	if (padctl->soc->ops->resume_noirq)
+		return padctl->soc->ops->resume_noirq(padctl);
+
+	return 0;
+}
+
+static const struct dev_pm_ops tegra_xusb_padctl_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_xusb_padctl_suspend_noirq,
+				      tegra_xusb_padctl_resume_noirq)
+};
+
 static struct platform_driver tegra_xusb_padctl_driver = {
 	.driver = {
 		.name = "tegra-xusb-padctl",
 		.of_match_table = tegra_xusb_padctl_of_match,
+		.pm = &tegra_xusb_padctl_pm_ops,
 	},
 	.probe = tegra_xusb_padctl_probe,
 	.remove = tegra_xusb_padctl_remove,
@@ -1045,6 +1071,58 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 }
 EXPORT_SYMBOL_GPL(tegra_xusb_padctl_hsic_set_idle);
 
+int tegra_xusb_padctl_enable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy,
+					   enum usb_device_speed speed)
+{
+	if (padctl->soc->ops->phy_sleepwalk)
+		return padctl->soc->ops->phy_sleepwalk(padctl, phy, true,
+						       speed);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_enable_phy_sleepwalk);
+
+int tegra_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					    struct phy *phy)
+{
+	if (padctl->soc->ops->phy_sleepwalk)
+		return padctl->soc->ops->phy_sleepwalk(padctl, phy, false, 0);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_disable_phy_sleepwalk);
+
+int tegra_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl,
+				      struct phy *phy)
+{
+	if (padctl->soc->ops->phy_wake)
+		return padctl->soc->ops->phy_wake(padctl, phy, true);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_enable_phy_wake);
+
+int tegra_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl,
+				       struct phy *phy)
+{
+	if (padctl->soc->ops->phy_wake)
+		return padctl->soc->ops->phy_wake(padctl, phy, false);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_disable_phy_wake);
+
+int tegra_xusb_padctl_remote_wake_detected(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy)
+{
+	if (padctl->soc->ops->remote_wake_detected)
+		return padctl->soc->ops->remote_wake_detected(phy);
+
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_xusb_padctl_remote_wake_detected);
+
 int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
 					   unsigned int port, bool enable)
 {
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 1bfe14b2a274..9482914536ac 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -11,6 +11,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 
+#include <linux/usb/ch9.h>
 #include <linux/usb/otg.h>
 
 /* legacy entry points for backwards-compatibility */
@@ -368,12 +369,19 @@ struct tegra_xusb_padctl_ops {
 			 const struct tegra_xusb_padctl_soc *soc);
 	void (*remove)(struct tegra_xusb_padctl *padctl);
 
+	int (*suspend_noirq)(struct tegra_xusb_padctl *padctl);
+	int (*resume_noirq)(struct tegra_xusb_padctl *padctl);
 	int (*usb3_save_context)(struct tegra_xusb_padctl *padctl,
 				 unsigned int index);
 	int (*hsic_set_idle)(struct tegra_xusb_padctl *padctl,
 			     unsigned int index, bool idle);
 	int (*usb3_set_lfps_detect)(struct tegra_xusb_padctl *padctl,
 				    unsigned int index, bool enable);
+	int (*phy_sleepwalk)(struct tegra_xusb_padctl *padctl, struct phy *phy,
+			     bool enable, enum usb_device_speed speed);
+	int (*phy_wake)(struct tegra_xusb_padctl *padctl, struct phy *phy,
+			bool enable);
+	int (*remote_wake_detected)(struct phy *phy);
 };
 
 struct tegra_xusb_padctl_soc {
diff --git a/include/linux/phy/tegra/xusb.h b/include/linux/phy/tegra/xusb.h
index ee59562c8354..2fb12e3baaee 100644
--- a/include/linux/phy/tegra/xusb.h
+++ b/include/linux/phy/tegra/xusb.h
@@ -8,6 +8,7 @@
 
 struct tegra_xusb_padctl;
 struct device;
+enum usb_device_speed;
 
 struct tegra_xusb_padctl *tegra_xusb_padctl_get(struct device *dev);
 void tegra_xusb_padctl_put(struct tegra_xusb_padctl *padctl);
@@ -18,5 +19,16 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 				    unsigned int port, bool idle);
 int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
 					   unsigned int port, bool enable);
+int tegra_xusb_padctl_enable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy,
+					   enum usb_device_speed speed);
+int tegra_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
+					   struct phy *phy);
+int tegra_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl,
+				      struct phy *phy);
+int tegra_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl,
+				       struct phy *phy);
+int tegra_xusb_padctl_remote_wake_detected(struct tegra_xusb_padctl *padctl,
+					struct phy *phy);
 
 #endif /* PHY_TEGRA_XUSB_H */
-- 
2.17.1

Re: [PATCH 4/8] phy: tegra: xusb: add sleepwalk and suspend/resume

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> This commit adds sleepwalk/wake and suspend/resume interfaces
> to Tegra XUSB PHY driver.

If you adding new custom global APIs, you really need to explain why
these are needed and how they are used. If we can avoid adding such
global APIs, it is preferred.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 4/8] phy: tegra: xusb: add sleepwalk and suspend/resume

[Jon Hunter]

On 14/06/2019 08:46, JC Kuo wrote:
> This commit adds sleepwalk/wake and suspend/resume interfaces
> to Tegra XUSB PHY driver.

If you adding new custom global APIs, you really need to explain why
these are needed and how they are used. If we can avoid adding such
global APIs, it is preferred.

Cheers
Jon

-- 
nvpublic