Hi Thierry,
On 04-02-2019 17:57, Thierry Reding wrote:
> 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.
>
For XUSB device control Feature support, we needed all three and hence
added them as part of one change. If we can have group of individuals CL's
for providing one feature support, will split them accordingly.
Doing above will mark current changes as a different patch series, considering
new patch series will be made, is it fine to move USB gadget driver changes to
a new series ?
Please share your inputs
>> 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?
>
Will do
>> +
>> + 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.
>
Will change to dev_dbg and print index as %u
>> +
>> + 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.
>
will do
>> +
>> + 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.
>
Lock was added, since the API area called from external context(XUDC driver)
and to protect against on/off, mutex_lock was done.
Based on later suggestion in the review, will move utmi pad and utmi bias pad
power on functions to phy_power_on, with that, mutex lock is not needed any more.
>> 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.
>
vbus_override setting is needed to inform controller who drives vbus.
For device mode, gadget driver needs to set override so that vbus is
detected on connected host.
Will Add the comment accordingly
>> +
>> +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.
>
This is a reset Quirk executed by XUDC driver on PLC inactive case and is
planned to be done only if bits ZIN or ZIP are set in XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(0)
register.
Will add comment accordingly
>> 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.
>
Yes, this part of change is what below patch is doing
http://patchwork.ozlabs.org/patch/1031014/
w.r.t using usb_get_dr_mode(), it parses dr_mode string if present in fwnode
But in our case, we have mode part of port setting and is not inline with
expectation of API usb_get_dr_mode(). I believe we need to stay with implementation
as in above link.
Considering current patch and changes in patch/1031014/ are same,
please suggest, if changes w.r.t mode reading will be taken as part of
patch/1031014/ or shall i continue in current patch series
>> + 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.
>
ENOSYS only add correct error info.
will revert change to return type and also use ENOSYS for new export functions
>> }
>> 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.
>
will do
>> +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?
>
yes, they can be part of phy_power_xxx(). Will remove
tegra_phy_xusb_utmi_pad_power_xx export functions
>> 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?
>
Yes, its the same, will re-use usb_dr_mode
>> +
>> 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?).
>
As mentioned earlier, will move utmi pad and utmi bias pad power function
can be to phy power functions.
tegra_phy_xusb_utmi_port_reset_quirk() does a vbus override only on a specific
condition, so it cannot be direct replacement of phy_ops->reset.
> 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.
>
AFAIU, we can not use phy_ops->configure(), since the same has enum argument
phy_configure_opts_mipi_dphy, which is not inline with needs of vbus override.
Also, API is not present in current kernel.
So i beleive we need to stay with vbus override API, which instead of 2, will
export 1(and take argument as set/clear) and other port_reset_quirk
> Thierry
>
- Nagarjuna
