[v2,1/2] phy: tegra: xusb: add utmi pad power on/down ops

Add utmi_pad_power_on/down ops for each SOC instead of exporting
tegra_phy_xusb_utmi_pad_power_on/down directly for Tegra186 chip.

Signed-off-by: BH Hsieh <bhsieh@nvidia.com>
Signed-off-by: Jim Lin <jilin@nvidia.com>
---
v2: update copyright year 

 drivers/phy/tegra/xusb-tegra186.c | 19 ++++++++++++-------
 drivers/phy/tegra/xusb.c          | 22 +++++++++++++++++++++-
 drivers/phy/tegra/xusb.h          |  4 +++-
 include/linux/phy/tegra/xusb.h    |  4 +++-
 4 files changed, 39 insertions(+), 10 deletions(-)

Hi Vinod,
Before the device or host is being attached, we can keep most of the
transceivers powered down (PD=1/PD_DR=1) to minimize power consumption. At this
stage, in .phy_power_on(), we enable only the single-ended receiver (PD_ZI=0)
for detecting connection. Upon detecting device's or host's connection, host or
controller driver will invoke tegra_phy_xusb_utmi_pad_power_on() to power on all
of the transceivers (PD=0/PD_DR=0) to equip full link functionality.

Thanks,
JC


On 9/4/22 22:45, Vinod Koul wrote:
> On 16-08-22, 16:23, Jim Lin wrote:
>> Add utmi_pad_power_on/down ops for each SOC instead of exporting
>> tegra_phy_xusb_utmi_pad_power_on/down directly for Tegra186 chip.
> 
> Can you please help me understand why do we need to utmi power_on/down
> exported and cant be handled thry phy-ops..
> 
>>
>> Signed-off-by: BH Hsieh <bhsieh@nvidia.com>
>> Signed-off-by: Jim Lin <jilin@nvidia.com>
>> ---
>> v2: update copyright year 
>>
>>  drivers/phy/tegra/xusb-tegra186.c | 19 ++++++++++++-------
>>  drivers/phy/tegra/xusb.c          | 22 +++++++++++++++++++++-
>>  drivers/phy/tegra/xusb.h          |  4 +++-
>>  include/linux/phy/tegra/xusb.h    |  4 +++-
>>  4 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
>> index ae3915ed9fef..5abdf81aa143 100644
>> --- a/drivers/phy/tegra/xusb-tegra186.c
>> +++ b/drivers/phy/tegra/xusb-tegra186.c
>> @@ -1,6 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /*
>> - * Copyright (c) 2016-2020, NVIDIA CORPORATION.  All rights reserved.
>> + * Copyright (c) 2016-2022, NVIDIA CORPORATION.  All rights reserved.
>>   */
>>  
>>  #include <linux/delay.h>
>> @@ -638,7 +638,7 @@ static void tegra186_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl)
>>  	mutex_unlock(&padctl->lock);
>>  }
>>  
>> -static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
>> +static void tegra186_utmi_pad_power_on(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> @@ -656,6 +656,8 @@ static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
>>  		return;
>>  	}
>>  
>> +	dev_dbg(dev, "power on UTMI pad %u\n", index);
>> +
>>  	tegra186_utmi_bias_pad_power_on(padctl);
>>  
>>  	udelay(2);
>> @@ -669,7 +671,7 @@ static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
>>  	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
>>  }
>>  
>> -static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
>> +static void tegra186_utmi_pad_power_down(struct phy *phy)
>>  {
>>  	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>>  	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> @@ -679,6 +681,8 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
>>  	if (!phy)
>>  		return;
>>  
>> +	dev_dbg(padctl->dev, "power down UTMI pad %u\n", index);
>> +
>>  	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>>  	value |= USB2_OTG_PD;
>>  	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> @@ -849,15 +853,14 @@ static int tegra186_utmi_phy_power_on(struct phy *phy)
>>  	value |= RPD_CTRL(priv->calib.rpd_ctrl);
>>  	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
>>  
>> -	/* TODO: pad power saving */
>> -	tegra_phy_xusb_utmi_pad_power_on(phy);
>> +	tegra186_utmi_pad_power_on(phy);
>> +
>>  	return 0;
>>  }
>>  
>>  static int tegra186_utmi_phy_power_off(struct phy *phy)
>>  {
>> -	/* TODO: pad power saving */
>> -	tegra_phy_xusb_utmi_pad_power_down(phy);
>> +	tegra186_utmi_pad_power_down(phy);
>>  
>>  	return 0;
>>  }
>> @@ -1486,6 +1489,8 @@ static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
>>  	.suspend_noirq = tegra186_xusb_padctl_suspend_noirq,
>>  	.resume_noirq = tegra186_xusb_padctl_resume_noirq,
>>  	.vbus_override = tegra186_xusb_padctl_vbus_override,
>> +	.utmi_pad_power_on = tegra186_utmi_pad_power_on,
>> +	.utmi_pad_power_down = tegra186_utmi_pad_power_down,
>>  };
>>  
>>  #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index 963de5913e50..49873718d54a 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -1,6 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>>  /*
>> - * Copyright (c) 2014-2020, NVIDIA CORPORATION.  All rights reserved.
>> + * Copyright (c) 2014-2022, NVIDIA CORPORATION.  All rights reserved.
>>   */
>>  
>>  #include <linux/delay.h>
>> @@ -1458,6 +1458,26 @@ int tegra_phy_xusb_utmi_port_reset(struct phy *phy)
>>  }
>>  EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_port_reset);
>>  
>> +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
>> +{
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	if (padctl->soc->ops->utmi_pad_power_on)
>> +		padctl->soc->ops->utmi_pad_power_on(phy);
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_on);
>> +
>> +void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
>> +{
>> +	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> +
>> +	if (padctl->soc->ops->utmi_pad_power_down)
>> +		padctl->soc->ops->utmi_pad_power_down(phy);
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_down);
>> +
>>  int tegra_xusb_padctl_get_usb3_companion(struct tegra_xusb_padctl *padctl,
>>  				    unsigned int port)
>>  {
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 034f7a2c28d6..8cfbbdbd6e0c 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: GPL-2.0-only */
>>  /*
>> - * Copyright (c) 2014-2020, NVIDIA CORPORATION.  All rights reserved.
>> + * Copyright (c) 2014-2022, NVIDIA CORPORATION.  All rights reserved.
>>   * Copyright (c) 2015, Google Inc.
>>   */
>>  
>> @@ -412,6 +412,8 @@ struct tegra_xusb_padctl_ops {
>>  				    unsigned int index, bool enable);
>>  	int (*vbus_override)(struct tegra_xusb_padctl *padctl, bool set);
>>  	int (*utmi_port_reset)(struct phy *phy);
>> +	void (*utmi_pad_power_on)(struct phy *phy);
>> +	void (*utmi_pad_power_down)(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 3a35e74cdc61..70998e6dd6fd 100644
>> --- a/include/linux/phy/tegra/xusb.h
>> +++ b/include/linux/phy/tegra/xusb.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: GPL-2.0-only */
>>  /*
>> - * Copyright (c) 2016-2020, NVIDIA CORPORATION.  All rights reserved.
>> + * Copyright (c) 2016-2022, NVIDIA CORPORATION.  All rights reserved.
>>   */
>>  
>>  #ifndef PHY_TEGRA_XUSB_H
>> @@ -21,6 +21,8 @@ int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
>>  					   unsigned int port, bool enable);
>>  int tegra_xusb_padctl_set_vbus_override(struct tegra_xusb_padctl *padctl,
>>  					bool val);
>> +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy);
>> +void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy);
>>  int tegra_phy_xusb_utmi_port_reset(struct phy *phy);
>>  int tegra_xusb_padctl_get_usb3_companion(struct tegra_xusb_padctl *padctl,
>>  					 unsigned int port);
>> -- 
>> 2.17.1
>

On 06-09-22, 10:43, JC Kuo wrote:
> Hi Vinod,

Please _do_ _not_ _top_ _post_

> Before the device or host is being attached, we can keep most of the
> transceivers powered down (PD=1/PD_DR=1) to minimize power consumption. At this
> stage, in .phy_power_on(), we enable only the single-ended receiver (PD_ZI=0)
> for detecting connection. Upon detecting device's or host's connection, host or
> controller driver will invoke tegra_phy_xusb_utmi_pad_power_on() to power on all
> of the transceivers (PD=0/PD_DR=0) to equip full link functionality.

Thanks for this explanation... It helps!

Just a suggestion, can this be moved into phy_init() you have detected
connection in phy_power_on(), the transceiver can be enabled in
phy_int... Would that work?

On 9/13/22 22:34, Vinod Koul wrote:
> On 06-09-22, 10:43, JC Kuo wrote:
>> Hi Vinod,
> 
> Please _do_ _not_ _top_ _post_
> 
>> Before the device or host is being attached, we can keep most of the
>> transceivers powered down (PD=1/PD_DR=1) to minimize power consumption. At this
>> stage, in .phy_power_on(), we enable only the single-ended receiver (PD_ZI=0)
>> for detecting connection. Upon detecting device's or host's connection, host or
>> controller driver will invoke tegra_phy_xusb_utmi_pad_power_on() to power on all
>> of the transceivers (PD=0/PD_DR=0) to equip full link functionality.
> 
> Thanks for this explanation... It helps!
> 
> Just a suggestion, can this be moved into phy_init() you have detected
> connection in phy_power_on(), the transceiver can be enabled in
> phy_int... Would that work?
> That would work, too. However, because Tegra USB has separate phys for USB3 SS
and USB2, I'd like to keep the USB2 phy operations as they are now, so that USB
host and device controller drivers do not have to distinguish the phy type and
invoke different phy stubs. Furthermore, PD_ZI=0 does really power on the USB2
phy, partially.

For example:
1. in .probe(),
    for_each_usb_phy {
         phy_init(phy);
    }

    for_each_usb3_phy {
         phy_power_on(phy);
    };

2. upon detecting connection,

    phy_power_on(the_target_usb2_phy);

Thanks,
JC

On 14-09-22, 10:59, JC Kuo wrote:
> On 9/13/22 22:34, Vinod Koul wrote:
> > On 06-09-22, 10:43, JC Kuo wrote:

> > Thanks for this explanation... It helps!
> > 
> > Just a suggestion, can this be moved into phy_init() you have detected
> > connection in phy_power_on(), the transceiver can be enabled in
> > phy_int... Would that work?
> > That would work, too. However, because Tegra USB has separate phys for USB3 SS
> and USB2, I'd like to keep the USB2 phy operations as they are now, so that USB
> host and device controller drivers do not have to distinguish the phy type and
> invoke different phy stubs. Furthermore, PD_ZI=0 does really power on the USB2
> phy, partially.
> 
> For example:
> 1. in .probe(),
>     for_each_usb_phy {
>          phy_init(phy);
>     }
> 
>     for_each_usb3_phy {
>          phy_power_on(phy);
>     };
> 
> 2. upon detecting connection,
> 
>     phy_power_on(the_target_usb2_phy);

It should be always phy_init() in probe and once detection
phy_power_on()

that should be generic flow for all...