On Thu, Jan 03, 2019 at 03:34:52PM +0530, Nagarjuna Kristam wrote:
> 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.
> Signed-off-by: Nagarjuna Kristam <nkristam@xxxxxxxxxx>
> ---
> 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(-)
>
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 05bee32..0289e10 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -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)
> +{
> + 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?
> +
> + 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);
> + 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.
> +
> + 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);
> + 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.
> +
> + 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));
> +
> + 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.
> 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;
> + 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.
> +
> +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);
> +
> + 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.
> 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 = {
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 5b3b886..9a25b5d 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -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");
>
> + 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.
> + 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);
> }
> @@ -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.
> }
> 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;
> + 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.
> +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);
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?
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index b49dbc3..74acc08 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -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 *
> @@ -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?
> +
> 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 {
> diff --git a/include/linux/phy/tegra/xusb.h b/include/linux/phy/tegra/xusb.h
> index 8e1a57a..143af44 100644
> --- a/include/linux/phy/tegra/xusb.h
> +++ b/include/linux/phy/tegra/xusb.h
> @@ -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,
> @@ -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
Attachment:
signature.asc
Description: PGP signature
