Skip to contentSkip to conversation

phy: tegra: xusb: t210: add XUSB device mode support

PATCH New Thierry Reding 1 participant 1 message Feb 4, 2019 USB

lore
Share & tools
lore

Thierry Reding

author
#0 lore raw

PATCH 1/8 phy: tegra: xusb: t210: add XUSB device mode support

Quoted 4 lines · Nagarjuna Kristam

Add support for XUSB device mode controller on Tegra210. Update PADCTL driver to set port cap based on DT config. Add code to handle property "nvidia,usb3-port-fake" Provide API's to control vbus override and utmi pad power control

You essentially list three features additions here, so it makes sense to provide each of them in a separate patch. That helps reviewers concentrate on one feature at a time without having to load context for three different things in parallel.

Also, for each feature please provide a bit more background information as well as reasons for why they are added.

Quoted 73 lines
Signed-off-by Nagarjuna Kristam <[email protected]>
---
 drivers/phy/tegra/xusb-tegra210.c | 297 +++++++++++++++++++++++++++++++++++++-
 drivers/phy/tegra/xusb.c          |  75 +++++++++-
 drivers/phy/tegra/xusb.h          |  16 +-
 include/linux/phy/tegra/xusb.h    |   7 +-
 4 files changed, 386 insertions(+), 9 deletions(-)
drivers/phy/tegra/xusb-tegra210.c +19 -1
index 05bee32..0289e10 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2014-2018, NVIDIA CORPORATION.  All rights reserved.
  * Copyright (C) 2015 Google, Inc.
  *
  * This program is free software; you can redistribute it and/or modify it

@@ -47,7 +47,10 @@
 #define XUSB_PADCTL_USB2_PAD_MUX_USB2_BIAS_PAD_XUSB 0x1
 
 #define XUSB_PADCTL_USB2_PORT_CAP 0x008
+#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DISABLED(x) (0x0 << ((x) * 4))
 #define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_HOST(x) (0x1 << ((x) * 4))
+#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DEVICE(x) (0x2 << ((x) * 4))
+#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_OTG(x) (0x3 << ((x) * 4))
 #define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_MASK(x) (0x3 << ((x) * 4))
 
 #define XUSB_PADCTL_SS_PORT_MAP 0x014

@@ -55,6 +58,7 @@
 #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_SHIFT(x) ((x) * 5)
 #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(x) (0x7 << ((x) * 5))
 #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(x, v) (((v) & 0x7) << ((x) * 5))
+#define XUSB_PADCTL_SS_PORT_MAP_PORT_DISABLED 0x7
 
 #define XUSB_PADCTL_ELPG_PROGRAM1 0x024
 #define XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN (1 << 31)

@@ -69,9 +73,14 @@
 #define XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(x) (1 << (1 + (x)))
 #define XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(x) (1 << (8 + (x)))
 
+#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(x) (0x080 + (x) * 0x40)
+#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIP (1 << 18)
+#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIN (1 << 22)
+
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(x) (0x084 + (x) * 0x40)
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_SHIFT 7
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
+#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
 
 #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)

@@ -230,6 +239,12 @@
 #define XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(x) (0xa74 + (x) * 0x40)
 #define XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL 0xfcf01368
 
+#define XUSB_PADCTL_USB2_VBUS_ID 0xc60
+#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON (1 << 14)
+#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT 18
+#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_MASK 0xf
+#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VAL 8
+
 struct tegra210_xusb_fuse_calibration {
 	u32 hs_curr_level[4];
 	u32 hs_term_range_adj;

@@ -905,6 +920,161 @@ static const struct tegra_xusb_lane_ops tegra210_usb2_lane_ops = {
 	.remove = tegra210_usb2_lane_remove,
 };
 
+/* must be called under padctl->lock */
+static void tegra210_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl,
+				struct tegra_xusb_usb2_pad *pad)

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_lane_ops tegra210_usb2_lane_ops = { · line 935

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+{
+	u32 reg;
+
+	if (pad->enable++ > 0)
+		return;
+
+	dev_dbg(padctl->dev, "power on BIAS PAD & USB2 tracking\n");
+
+	if (clk_prepare_enable(pad->clk))
+		dev_warn(padctl->dev, "failed to enable BIAS PAD & USB2 tracking\n");

Maybe keep track of the exact error and make that part of the error message?

Quoted 71 lines
drivers/phy/tegra/xusb-tegra210.c +71 -0
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+
+	reg &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
+		   (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
+	reg |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
+		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
+		 (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_VAL <<
+		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT);
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+	reg &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL0_PD;
+
+	reg &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_SHIFT) |
+		   (XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_DISCON_LEVEL_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_DISCON_LEVEL_SHIFT));
+	reg |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_DISCON_LEVEL_VAL <<
+		  XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_DISCON_LEVEL_SHIFT);
+
+	if (tegra_sku_info.revision < TEGRA_REVISION_A02)
+		reg |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_VAL <<
+			XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_SHIFT);
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+
+	udelay(1);
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+	reg &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL1_PD_TRK;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+}
+
+/* must be called under padctl->lock */
+static void tegra210_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl,
+				struct tegra_xusb_usb2_pad *pad)
+{
+	u32 reg;
+
+	if (WARN_ON(pad->enable == 0))
+		return;
+
+	if (--pad->enable > 0)
+		return;
+
+	dev_dbg(padctl->dev, "power off USB2 tracking\n");
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+	reg |= XUSB_PADCTL_USB2_BIAS_PAD_CTL1_PD_TRK;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+
+	clk_disable_unprepare(pad->clk);
+}
+
+void tegra210_utmi_pad_power_on(struct phy *phy)
+{
+	struct tegra_xusb_lane *lane;
+	struct tegra_xusb_usb2_lane *usb2;
+	struct tegra_xusb_usb2_pad *pad;
+	struct tegra_xusb_padctl *padctl;
+	unsigned int index;
+	struct device *dev;
+	u32 reg;
+
+	if (!phy)
+		return;
+
+	lane = phy_get_drvdata(phy);
+	usb2 = to_usb2_lane(lane);
+	pad = to_usb2_pad(lane->pad);

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_lane_ops tegra210_usb2_lane_ops = { · line 1016

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+	padctl = lane->pad->padctl;
+	index = lane->index;
+	dev = padctl->dev;
+
+	if (usb2->powered_on)
+		return;
+
+	mutex_lock(&padctl->lock);
+
+	dev_info(dev, "power on UTMI pads %d\n", index);

dev_dbg()? Also, index is unsigned, so you should use %u to print it.

Quoted 34 lines
drivers/phy/tegra/xusb-tegra210.c +34 -0
+
+	tegra210_utmi_bias_pad_power_on(padctl, pad);
+
+	udelay(2);
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	reg &= ~XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+	reg &= ~XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+
+	usb2->powered_on = true;
+
+	mutex_unlock(&padctl->lock);
+}
+
+void tegra210_utmi_pad_power_down(struct phy *phy)
+{
+	struct tegra_xusb_lane *lane;
+	struct tegra_xusb_usb2_lane *usb2;
+	struct tegra_xusb_usb2_pad *pad;
+	struct tegra_xusb_padctl *padctl;
+	unsigned int index;
+	struct device *dev;
+	u32 reg;
+
+	if (!phy)
+		return;
+
+	lane = phy_get_drvdata(phy);
+	usb2 = to_usb2_lane(lane);
+	pad = to_usb2_pad(lane->pad);

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_lane_ops tegra210_usb2_lane_ops = { · line 1060

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+	padctl = lane->pad->padctl;
+	index = lane->index;
+	dev = padctl->dev;
+
+	if (!usb2->powered_on)
+		return;
+
+	mutex_lock(&padctl->lock);
+
+	dev_info(dev, "power down UTMI pad %d\n", index);

Same as above.

Quoted 6 lines
drivers/phy/tegra/xusb-tegra210.c +6 -0
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_lane_ops tegra210_usb2_lane_ops = { · line 1076

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+	reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+
+	udelay(2);
+
+	tegra210_utmi_bias_pad_power_off(padctl, pad);
+	usb2->powered_on = false;
+
+	mutex_unlock(&padctl->lock);
+}

I'm not sure I understand this correctly. This seems to implement some reference counting of its own. Why is that? Is it because it is shared between multiple pads? But then, you're writing to indexed registers, which indicates to me that these are in fact separate pads and not a shared resource. So why the reference counting?

I'm also not sure why these functions need to be accessible outside this file. They are not used outside, as far as I can tell.

Quoted 117 lines
drivers/phy/tegra/xusb-tegra210.c +80 -2
 static int tegra210_usb2_phy_init(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);

@@ -948,6 +1118,34 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 
 	priv = to_tegra210_xusb_padctl(padctl);
 
+	if (port->usb3_port_fake != -1) {
+		value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
+		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
+					port->usb3_port_fake);
+		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(
+					port->usb3_port_fake, index);
+		padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
+
+		value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+		value &= ~XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_VCORE_DOWN(
+					port->usb3_port_fake);
+		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(
+					port->usb3_port_fake);
+		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(
+					port->usb3_port_fake);
+		padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+	}
+
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
 	value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_MASK <<
 		    XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_SHIFT) |

@@ -965,7 +1163,14 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_PORT_CAP);
 	value &= ~XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_MASK(index);
-	value |= XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_HOST(index);
+	if (port->port_cap == USB_PORT_DISABLED)
+		value |= XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DISABLED(index);
+	else if (port->port_cap == USB_DEVICE_CAP)
+		value |= XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DEVICE(index);
+	else if (port->port_cap == USB_HOST_CAP)
+		value |= XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_HOST(index);
+	else if (port->port_cap == USB_OTG_CAP)
+		value |= XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_OTG(index);
 	padctl_writel(padctl, value, XUSB_PADCTL_USB2_PORT_CAP);
 
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));

@@ -997,7 +1202,12 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 			     XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
 	value &= ~(XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK <<
 		   XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_SHIFT);
-	value |= XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
+	if (port->port_cap == USB_HOST_CAP)
+		value |= XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
+	else
+		value |=
+		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL <<
+		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_SHIFT;
 	padctl_writel(padctl, value,
 		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
 

@@ -1070,6 +1280,34 @@ static int tegra210_usb2_phy_power_off(struct phy *phy)
 
 	mutex_lock(&padctl->lock);
 
+	if (port->usb3_port_fake != -1) {
+		value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
+		value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(
+					port->usb3_port_fake);
+		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(
+					port->usb3_port_fake);
+		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(
+					port->usb3_port_fake);
+		padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1);
+
+		value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
+		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
+					port->usb3_port_fake);
+		value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
+					port->usb3_port_fake);
+		padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
+	}
+
 	if (WARN_ON(pad->enable == 0))
 		goto out;
 

@@ -1953,6 +2191,55 @@ static const struct tegra_xusb_port_ops tegra210_usb3_port_ops = {
 	.map = tegra210_usb3_port_map,
 };
 
+static int tegra210_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
+					      bool set)
+{
+	u32 reg;
+
+	dev_dbg(padctl->dev, "%s vbus override\n", set ? "set" : "clear");
+
+	reg = padctl_readl(padctl, XUSB_PADCTL_USB2_VBUS_ID);
+	if (set) {
+		reg |= XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON;

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_port_ops tegra210_usb3_port_ops = { · line 2213

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+		reg &= ~(XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_MASK <<
+			   XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT);
+		reg |= XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VAL <<
+			 XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT;
+	} else
+		reg &= ~XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON;
+	padctl_writel(padctl, reg, XUSB_PADCTL_USB2_VBUS_ID);
+
+	return 0;
+}

It'd be good to briefly explain in a comment (or the commit message) what exactly this does and what it's used for.

Quoted 18 lines
drivers/phy/tegra/xusb-tegra210.c +18 -0
+
+static int tegra210_utmi_port_reset_quirk(struct phy *phy)
+{
+	struct tegra_xusb_padctl *padctl;
+	struct tegra_xusb_lane *lane;
+	struct device *dev;
+	u32 reg;
+
+	if (!phy)
+		return -ENODEV;
+
+	lane = phy_get_drvdata(phy);
+	padctl = lane->pad->padctl;
+	dev = padctl->dev;
+
+	reg = padctl_readl(padctl,
+				XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(0));
+	dev_dbg(dev, "BATTERY_CHRG_OTGPADX_CTL0(0): 0x%x\n", reg);

drivers/phy/tegra/xusb-tegra210.c static const struct tegra_xusb_port_ops tegra210_usb3_port_ops = { · line 2241

Quoted 10 lines
drivers/phy/tegra/xusb-tegra210.c +10 -0
+
+	if ((reg & XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIP) ||
+	    (reg & XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIN)) {
+		dev_dbg(dev, "Toggle vbus\n");
+		tegra210_xusb_padctl_vbus_override(padctl, false);
+		tegra210_xusb_padctl_vbus_override(padctl, true);
+		return 1;
+	}
+	return 0;
+}

This uses the VBUS override mechanism, but at the same time you also export it to make use of it in the XUDC driver (presumably). Why have two ways of running this code? Again, an explanation of what exactly this is used for would clarify.

Quoted 33 lines
drivers/phy/tegra/xusb-tegra210.c +4 -0
 static int
 tegra210_xusb_read_fuse_calibration(struct tegra210_xusb_fuse_calibration *fuse)
 {

@@ -2015,6 +2302,10 @@ static const struct tegra_xusb_padctl_ops tegra210_xusb_padctl_ops = {
 	.remove = tegra210_xusb_padctl_remove,
 	.usb3_set_lfps_detect = tegra210_usb3_set_lfps_detect,
 	.hsic_set_idle = tegra210_hsic_set_idle,
+	.vbus_override = tegra210_xusb_padctl_vbus_override,
+	.utmi_pad_power_on = tegra210_utmi_pad_power_on,
+	.utmi_pad_power_down = tegra210_utmi_pad_power_down,
+	.utmi_port_reset_quirk = tegra210_utmi_port_reset_quirk,
 };
 
 const struct tegra_xusb_padctl_soc tegra210_xusb_padctl_soc = {
drivers/phy/tegra/xusb.c +3 -1
index 5b3b886..9a25b5d 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014-2015, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2014-2018, NVIDIA CORPORATION.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,

@@ -546,9 +546,26 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
 {
 	struct tegra_xusb_port *port = &usb2->base;
 	struct device_node *np = port->dev.of_node;
+	const char *prop_string;
+	u32 value;
 
 	usb2->internal = of_property_read_bool(np, "nvidia,internal");

drivers/phy/tegra/xusb.c static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) · line 562

Quoted 10 lines
drivers/phy/tegra/xusb.c +9 -0
 
+	usb2->port_cap = USB_PORT_DISABLED; /* default */
+	if (!of_property_read_string(np, "mode", &prop_string)) {
+		if (!strcmp("host", prop_string))
+			usb2->port_cap = USB_HOST_CAP;
+		else if (!strcmp("device", prop_string))
+			usb2->port_cap = USB_DEVICE_CAP;
+		else if (!strcmp("otg", prop_string))
+			usb2->port_cap = USB_OTG_CAP;
+	}

This seems to fulfill the same purpose as this:

	http://patchwork.ozlabs.org/patch/1031014/

Although, looking more closely at that, we can probably just the usb_get_dr_mode() helper for this. There's also a standard property for the dual-role mode, named "dr_mode", albeit the name does not seem to match standard best practices. Seems like it would still make sense to move to that standard property name.

Quoted 9 lines
drivers/phy/tegra/xusb.c +5 -0
+	if (!of_property_read_u32(np, "nvidia,usb3-port-fake", &value))
+		usb2->usb3_port_fake = value;
+	else
+		usb2->usb3_port_fake = -1; /* default */
+
 	usb2->supply = devm_regulator_get(&port->dev, "vbus");
 	return PTR_ERR_OR_ZERO(usb2->supply);
 }

drivers/phy/tegra/xusb.c int tegra_xusb_padctl_usb3_save_context(struct tegra_xusb_padctl *padctl, · line 996

Quoted 5 lines
drivers/phy/tegra/xusb.c +1 -1
@@ -976,7 +993,7 @@ int tegra_xusb_padctl_usb3_save_context(struct tegra_xusb_padctl *padctl,
 	if (padctl->soc->ops->usb3_save_context)
 		return padctl->soc->ops->usb3_save_context(padctl, port);
 
-	return -ENOSYS;
+	return -EINVAL;

Why change the error code for these? EINVAL has a pretty standard meaning ("invalid argument") and doesn't match the error condition at all. ENOSYS was chosen here because it's the closest there is to "not implemented". ENOSYS is somewhat special in relation to syscalls, but since we never propagate errors from here to userspace, it's safe to use.

Quoted 17 lines
drivers/phy/tegra/xusb.c +1 -2
 }
 EXPORT_SYMBOL_GPL(tegra_xusb_padctl_usb3_save_context);
 

@@ -986,7 +1003,7 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
 	if (padctl->soc->ops->hsic_set_idle)
 		return padctl->soc->ops->hsic_set_idle(padctl, port, idle);
 
-	return -ENOSYS;
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(tegra_xusb_padctl_hsic_set_idle);
 

@@ -997,10 +1014,60 @@ int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
 		return padctl->soc->ops->usb3_set_lfps_detect(padctl, port,
 							      enable);
 
-	return -ENOSYS;

drivers/phy/tegra/xusb.c int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl, · line 1026

Quoted 10 lines
drivers/phy/tegra/xusb.c +7 -0
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(tegra_xusb_padctl_usb3_set_lfps_detect);
 
+int tegra_xusb_padctl_set_vbus_override(struct tegra_xusb_padctl *padctl)
+{
+	if (padctl->soc->ops->vbus_override)
+		return padctl->soc->ops->vbus_override(padctl, true);
+
+	return -EINVAL;

It'd be best to stick to the -ENOSYS error code here.

Quoted 9 lines
drivers/phy/tegra/xusb.c +9 -0
+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);

drivers/phy/tegra/xusb.c int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl, · line 1045

Quoted 10 lines
drivers/phy/tegra/xusb.c +10 -0
+
+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);

I already briefly touched on this earlier, bu why do these have to be exported functions? Can't these simply be abstracted behind the PHY abstraction?

Quoted 20 lines
drivers/phy/tegra/xusb.h +2 -1
index b49dbc3..74acc08 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014-2015, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2014-2018, NVIDIA CORPORATION.  All rights reserved.
  * Copyright (c) 2015, Google Inc.
  *
  * This program is free software; you can redistribute it and/or modify it

@@ -58,6 +58,7 @@ struct tegra_xusb_usb2_lane {
 	struct tegra_xusb_lane base;
 
 	u32 hs_curr_level_offset;
+	bool powered_on;
 };
 
 static inline struct tegra_xusb_usb2_lane *

drivers/phy/tegra/xusb.h struct tegra_xusb_port * · line 276

Quoted 9 lines
drivers/phy/tegra/xusb.h +6 -0
@@ -267,11 +268,20 @@ struct tegra_xusb_port *
 tegra_xusb_find_port(struct tegra_xusb_padctl *padctl, const char *type,
 		     unsigned int index);
 
+enum tegra_xusb_usb_port_cap {
+	USB_PORT_DISABLED = 0,
+	USB_HOST_CAP,
+	USB_DEVICE_CAP,
+	USB_OTG_CAP,
+};

Maybe use enum usb_dr_mode from include/linux/usb/otg.h instead of essentially duplicating it?

Quoted 34 lines
drivers/phy/tegra/xusb.h +7 -0
+
 struct tegra_xusb_usb2_port {
 	struct tegra_xusb_port base;
 
 	struct regulator *supply;
 	bool internal;
+	enum tegra_xusb_usb_port_cap port_cap;
+	int usb3_port_fake;
 };
 
 static inline struct tegra_xusb_usb2_port *

@@ -353,6 +363,10 @@ struct tegra_xusb_padctl_ops {
 			     unsigned int index, bool idle);
 	int (*usb3_set_lfps_detect)(struct tegra_xusb_padctl *padctl,
 				    unsigned int index, bool enable);
+	int (*vbus_override)(struct tegra_xusb_padctl *padctl, bool set);
+	void (*utmi_pad_power_on)(struct phy *phy);
+	void (*utmi_pad_power_down)(struct phy *phy);
+	int (*utmi_port_reset_quirk)(struct phy *phy);
 };
 
 struct tegra_xusb_padctl_soc {
include/linux/phy/tegra/xusb.h +1 -1
index 8e1a57a..143af44 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2016-2018, NVIDIA CORPORATION.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,

include/linux/phy/tegra/xusb.h int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl, · line 34

Quoted 9 lines
include/linux/phy/tegra/xusb.h +5 -0
@@ -26,5 +26,10 @@ 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_set_vbus_override(struct tegra_xusb_padctl *padctl);
+int tegra_xusb_padctl_clear_vbus_override(struct tegra_xusb_padctl *padctl);
 
+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_quirk(struct phy *phy);

We already have more custom APIs here than I'd like, so I really want to make sure that we're not adding to these if not absolutely necessary.

Are there any reasons why we can't somehow hide these behind the existing PHY framework?

tegra_phy_xusb_utmi_port_reset_quirk() seems like something that we may be able to implement using the phy_ops->reset() callback. The power_on() and power_off() for the UTMI pad seem like they should be just part of the regular pad ->power_on() and ->power_off() callback implementations (perhaps this means that we need to introduce another pad for this?).

Set/clear VBUS override seems like maybe something that could be hidden behind phy_ops->configure(). This is a fairly recent addition, but sounds like we could also push things like HSIC idle and USB3 LPFS detection into that.

Thierry