Re: [PATCH v2 04/12] phy: tegra: xusb: t210: add lane_iddq operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Thierry,
Thanks for review. I will amend accordingly and submit a new patch.

JC

On 8/31/20 7:53 PM, Thierry Reding wrote:
> On Mon, Aug 31, 2020 at 12:40:35PM +0800, JC Kuo wrote:
>> As per Tegra210 TRM, before changing lane assignments, driver should
>> keep lanes in IDDQ and sleep state; after changing lane assignments,
>> driver should bring lanes out of IDDQ.
>> This commit implements the required operations.
>>
>> Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx>
>> ---
>>  drivers/phy/tegra/xusb-tegra210.c | 94 +++++++++++++++++++++++++++++++
>>  drivers/phy/tegra/xusb.c          |  6 ++
>>  drivers/phy/tegra/xusb.h          |  4 +-
>>  3 files changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
>> index 3a2d9797fb9f..fe1ab440424d 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -198,6 +198,18 @@
>>  #define XUSB_PADCTL_UPHY_MISC_PAD_CTL1_AUX_RX_TERM_EN BIT(18)
>>  #define XUSB_PADCTL_UPHY_MISC_PAD_CTL1_AUX_RX_MODE_OVRD BIT(13)
>>  
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(x) (0x464 + (x) * 0x40)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ BIT(0)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD BIT(1)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK (0x3 << 4)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL (0x3 << 4)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD BIT(24)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ BIT(8)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD BIT(9)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK (0x3 << 12)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL (0x3 << 12)
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD BIT(25)
>> +
>>  #define XUSB_PADCTL_UPHY_PLL_S0_CTL1 0x860
>>  
>>  #define XUSB_PADCTL_UPHY_PLL_S0_CTL2 0x864
>> @@ -209,6 +221,7 @@
>>  #define XUSB_PADCTL_UPHY_PLL_S0_CTL8 0x87c
>>  
>>  #define XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL1 0x960
>> +#define XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2 0x964
>>  
>>  #define XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(x) (0xa60 + (x) * 0x40)
>>  #define XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT 16
>> @@ -1636,6 +1649,63 @@ static const struct tegra_xusb_pad_soc tegra210_hsic_pad = {
>>  	.ops = &tegra210_hsic_ops,
>>  };
>>  
>> +static void
>> +tegra210_uphy_lane_iddq_enable(struct tegra_xusb_padctl *padctl, unsigned lane)
>> +{
>> +	u32 val, offset;
>> +
>> +	if (lane <= 6)
>> +		offset = XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(lane);
>> +	else if (lane == 7)
>> +		offset = XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2;
>> +	else {
>> +		WARN(true, "invalid lane %u\n", lane);
>> +		return;
>> +	}
>> +
>> +	val = padctl_readl(padctl, offset);
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL;
>> +	padctl_writel(padctl, val, offset);
>> +}
>> +
>> +static void
>> +tegra210_uphy_lane_iddq_disable(struct tegra_xusb_padctl *padctl, unsigned lane)
>> +{
>> +	u32 val, offset;
>> +
>> +	if (lane <= 6)
>> +		offset = XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(lane);
>> +	else if (lane == 7)
>> +		offset = XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2;
>> +	else {
>> +		WARN(true, "invalid lane %d\n", lane);
>> +		return;
>> +	}
>> +
>> +	val = padctl_readl(padctl, offset);
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ;
>> +	val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK;
>> +	val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL;
>> +	padctl_writel(padctl, val, offset);
>> +}
>> +
>> +
>>  static const char *tegra210_pcie_functions[] = {
>>  	"pcie-x1",
>>  	"usb3-ss",
>> @@ -1808,9 +1878,21 @@ static void tegra210_pcie_lane_remove(struct tegra_xusb_lane *lane)
>>  	kfree(pcie);
>>  }
>>  
>> +static void tegra210_pcie_lane_iddq_enable(struct tegra_xusb_lane *lane)
>> +{
>> +	tegra210_uphy_lane_iddq_enable(lane->pad->padctl, lane->index);
>> +}
>> +
>> +static void tegra210_pcie_lane_iddq_disable(struct tegra_xusb_lane *lane)
>> +{
>> +	tegra210_uphy_lane_iddq_disable(lane->pad->padctl, lane->index);
>> +}
>> +
>>  static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
>>  	.probe = tegra210_pcie_lane_probe,
>>  	.remove = tegra210_pcie_lane_remove,
>> +	.iddq_enable = tegra210_pcie_lane_iddq_enable,
>> +	.iddq_disable = tegra210_pcie_lane_iddq_disable,
>>  };
>>  
>>  static int tegra210_pcie_phy_init(struct phy *phy)
>> @@ -1971,9 +2053,21 @@ static void tegra210_sata_lane_remove(struct tegra_xusb_lane *lane)
>>  	kfree(sata);
>>  }
>>  
>> +static void tegra210_sata_lane_iddq_enable(struct tegra_xusb_lane *lane)
>> +{
>> +	tegra210_uphy_lane_iddq_enable(lane->pad->padctl, lane->index + 7);
>> +}
>> +
>> +static void tegra210_sata_lane_iddq_disable(struct tegra_xusb_lane *lane)
>> +{
>> +	tegra210_uphy_lane_iddq_disable(lane->pad->padctl, lane->index + 7);
>> +}
> 
> This looks like abstraction at the wrong level. You introduce this
> arbtitrary offset 7 to differentiate between the two types, whereas what
> you really only seem to be after is to get the correct offset.
> 
> Can't we instead make tegra210_uphy_lane_iddq_{enable,disable}() take
> the offset instead and push the logic to pick the offset into the
> callers? We could then have an extra helper that determines the offset
> from the lane if we want to avoid duplicating that logic.
> 
> Or perhaps an even better way would be to store the offset to this MISC
> register in struct tegra_xusb_lane_soc? Something like this perhaps:
> 
>     struct tegra_xusb_lane_soc {
>         ...
>         struct {
>             unsigned int misc;
>         } regs;
>     };
> 
> That way we don't even have to go through two layers but instead can
> operate on the struct tegra_xusb_lane directly.
> 
>> +
>>  static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
>>  	.probe = tegra210_sata_lane_probe,
>>  	.remove = tegra210_sata_lane_remove,
>> +	.iddq_enable = tegra210_sata_lane_iddq_enable,
>> +	.iddq_disable = tegra210_sata_lane_iddq_disable,
>>  };
>>  
>>  static int tegra210_sata_phy_init(struct phy *phy)
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index b48b590aa0c1..74abd67e3a31 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -321,11 +321,17 @@ static void tegra_xusb_lane_program(struct tegra_xusb_lane *lane)
>>  	if (soc->num_funcs < 2)
>>  		return;
>>  
>> +	if (lane->pad->ops->iddq_enable && lane->pad->ops->iddq_disable)
>> +		lane->pad->ops->iddq_enable(lane);
> 
> You can drop the second check because it isn't relevant here.
> 
>> +
>>  	/* choose function */
>>  	value = padctl_readl(padctl, soc->offset);
>>  	value &= ~(soc->mask << soc->shift);
>>  	value |= lane->function << soc->shift;
>>  	padctl_writel(padctl, value, soc->offset);
>> +
>> +	if (lane->pad->ops->iddq_enable && lane->pad->ops->iddq_disable)
>> +		lane->pad->ops->iddq_disable(lane);
> 
> Similarly, the first check can be dropped here because only the second
> is relevant. It might make sense to only support IDDQ if both callbacks
> are implemented, but that's not something we need to check at this
> level. The check here is only to avoid calling a NULL function. If you
> absolutely want to do sanity checks, do them at ->probe() time. But I
> don't think we need that here. It's up to the developer to get this
> right.
> 
>>  }
>>  
>>  static void tegra_xusb_pad_program(struct tegra_xusb_pad *pad)
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 0c828694cf2d..485b692a3b15 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -22,6 +22,7 @@ struct phy;
>>  struct phy_provider;
>>  struct platform_device;
>>  struct regulator;
>> +struct tegra_xusb_padctl;
>>  
>>  /*
>>   * lanes
>> @@ -126,6 +127,8 @@ struct tegra_xusb_lane_ops {
>>  					 struct device_node *np,
>>  					 unsigned int index);
>>  	void (*remove)(struct tegra_xusb_lane *lane);
>> +	void (*iddq_enable)(struct tegra_xusb_lane *lane);
>> +	void (*iddq_disable)(struct tegra_xusb_lane *lane);
>>  };
>>  
>>  bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
>> @@ -134,7 +137,6 @@ bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
>>   * pads
>>   */
>>  struct tegra_xusb_pad_soc;
>> -struct tegra_xusb_padctl;
>>  
>>  struct tegra_xusb_pad_ops {
>>  	struct tegra_xusb_pad *(*probe)(struct tegra_xusb_padctl *padctl,
> 
> These last two hunks look like leftovers. I don't see why they are
> needed here.
> 
> Thierry
> 



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux