| Message ID | 20220727105314.14681-1-jilin@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xhci: tegra: USB2 pad power controls | expand |
On Wed, Jul 27, 2022 at 06:53:14PM +0800, Jim Lin wrote: > Program USB2 pad PD controls during port > connect/disconnect, port suspend/resume etc > as suggested by HW to reduce power consumption. You do have a full 72 columns to use :) And this does not explain what this commit does at all, or why we would want to take it. Please read the kernel documentation for how to write a good changelog commit. > Squash following fixes from local kernel 4.9 to this commit: > ce4e7e5 usb: host: tegra: Power on utmi pads > 3a10c61 usb: tegra: Program USB2 pad PD controls > 4e62fbb xhci: tegra: move pad power on to non-atomic place > ed0fb0a usb: xhci: tegra: don't use hs_pls in xhci-iov > 401801a usb: xhci: add USB2 pad PD control for Test Mode This makes no sense, as these commits are not in our kernel tree at all. Also they are not even in the correct format, if we were to take them (hint, you need to fix your development process to not take lines like this.) > > Signed-off-by: Allie Liu <alliel@nvidia.com> > Signed-off-by: Jim Lin <jilin@nvidia.com> > --- > drivers/phy/tegra/xusb-tegra186.c | 27 +++-- > drivers/phy/tegra/xusb.c | 32 +++++- > drivers/phy/tegra/xusb.h | 4 +- > drivers/usb/gadget/udc/tegra-xudc.c | 8 +- > drivers/usb/host/xhci-hub.c | 2 + > drivers/usb/host/xhci-tegra.c | 146 +++++++++++++++++++++++++++- > include/linux/phy/tegra/xusb.h | 4 +- > 7 files changed, 209 insertions(+), 14 deletions(-) Are you sure you want to touch all of these files in a single commit? Why not submit a patch series? thanks, greg k-h
On Wed, Jul 27, 2022 at 06:53:14PM +0800, Jim Lin wrote: > Program USB2 pad PD controls during port > connect/disconnect, port suspend/resume etc > as suggested by HW to reduce power consumption. > > Squash following fixes from local kernel 4.9 to this commit: > ce4e7e5 usb: host: tegra: Power on utmi pads > 3a10c61 usb: tegra: Program USB2 pad PD controls > 4e62fbb xhci: tegra: move pad power on to non-atomic place > ed0fb0a usb: xhci: tegra: don't use hs_pls in xhci-iov > 401801a usb: xhci: add USB2 pad PD control for Test Mode Is there any particular reason why we can't submit these changes in the above form? Why do they need to be submitted as a single patch? If this is because there are build-time dependencies between these, you can still send them as individual patches and provide some dependency information in the cover-letter for the series so that the respective maintainers can work out how to merge them. In this particular case it would probably be fine for Greg to pick up all the patches with Acked-bys from Vinod or Kishon (for PHY) and Felipe (for USB gadget). Leaving this in the original form makes this a lot easier to review and presumably solves the problem of the commit description as well. > Signed-off-by: Allie Liu <alliel@nvidia.com> > Signed-off-by: Jim Lin <jilin@nvidia.com> > --- > drivers/phy/tegra/xusb-tegra186.c | 27 +++-- > drivers/phy/tegra/xusb.c | 32 +++++- > drivers/phy/tegra/xusb.h | 4 +- > drivers/usb/gadget/udc/tegra-xudc.c | 8 +- > drivers/usb/host/xhci-hub.c | 2 + > drivers/usb/host/xhci-tegra.c | 146 +++++++++++++++++++++++++++- > include/linux/phy/tegra/xusb.h | 4 +- > 7 files changed, 209 insertions(+), 14 deletions(-) > > diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c > index ae3915ed9fef..4eb29189ebfc 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> > @@ -642,20 +642,25 @@ static 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; > + struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane); > struct tegra_xusb_usb2_port *port; > - struct device *dev = padctl->dev; This temporary variable was introduced on purpose to keep the call sites briefer. You're now adding one callsite, so dropping this is counter- productive. The usage of this variable is admittedly a bit questionable, but if you want to remove it, better do it in a separate patch so it doesn't detract from the important bits in this patch. > unsigned int index = lane->index; > u32 value; > > + dev_dbg(padctl->dev, "power on UTMI pads %u\n", index); > + > if (!phy) > return; > > port = tegra_xusb_find_usb2_port(padctl, index); > if (!port) { > - dev_err(dev, "no port found for USB2 lane %u\n", index); > + dev_err(padctl->dev, "no port found for USB2 lane %u\n", index); > return; > } > > + if (usb2->powered_on) > + return; > + > tegra186_utmi_bias_pad_power_on(padctl); > > udelay(2); > @@ -667,18 +672,26 @@ static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy) > value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); > value &= ~USB2_OTG_PD_DR; > padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); > + > + usb2->powered_on = true; > } > > static 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; > + struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane); > unsigned int index = lane->index; > u32 value; > > + dev_dbg(padctl->dev, "power down UTMI pad %u\n", index); > + > if (!phy) > return; > > + if (!usb2->powered_on) > + return; > + > 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)); > @@ -688,8 +701,9 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy) > padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); > > udelay(2); > - This blank line was here for readability, no reason to drop it. > tegra186_utmi_bias_pad_power_off(padctl); > + > + usb2->powered_on = false; > } > > static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl, > @@ -849,14 +863,11 @@ 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); > return 0; > } > > static int tegra186_utmi_phy_power_off(struct phy *phy) > { > - /* TODO: pad power saving */ > tegra_phy_xusb_utmi_pad_power_down(phy); You've removed the tegra_phy_xusb_utmi_pad_power_on() call from tegra186_utmi_phy_power_on(), so by not removing the pad power down call from here makes this asymmetric. Instead, these calls are now sprinkled all over the place in the XHCI and UDC drivers. The PHY abstraction layer was supposed to help avoid any custom APIs. Is there no way we can use something like phy_power_on() or phy_power_off() instead of these? > > return 0; > @@ -1486,6 +1497,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 = tegra_phy_xusb_utmi_pad_power_on, > + .utmi_pad_power_down = tegra_phy_xusb_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..a6c7550e3a3a 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> > @@ -1446,6 +1446,36 @@ int tegra_xusb_padctl_set_vbus_override(struct tegra_xusb_padctl *padctl, > } > EXPORT_SYMBOL_GPL(tegra_xusb_padctl_set_vbus_override); > > +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy) > +{ > + struct tegra_xusb_lane *lane; > + struct tegra_xusb_padctl *padctl; > + > + if (!phy) > + return; > + > + lane = phy_get_drvdata(phy); > + 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; > + struct tegra_xusb_padctl *padctl; > + > + if (!phy) > + return; > + > + lane = phy_get_drvdata(phy); > + 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_phy_xusb_utmi_port_reset(struct phy *phy) > { > struct tegra_xusb_lane *lane = phy_get_drvdata(phy); > diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h > index 034f7a2c28d6..02cc5c6a588e 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. > */ > > @@ -411,6 +411,8 @@ struct tegra_xusb_padctl_ops { > 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)(struct phy *phy); > }; > > diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c > index 43f1b0d461c1..bca059b58bc9 100644 > --- a/drivers/usb/gadget/udc/tegra-xudc.c > +++ b/drivers/usb/gadget/udc/tegra-xudc.c > @@ -2,7 +2,7 @@ > /* > * NVIDIA Tegra XUSB device mode controller > * > - * Copyright (c) 2013-2019, NVIDIA CORPORATION. All rights reserved. > + * Copyright (c) 2013-2022, NVIDIA CORPORATION. All rights reserved. > * Copyright (c) 2015, Google Inc. > */ > > @@ -703,6 +703,9 @@ static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc) > > pm_runtime_get_sync(xudc->dev); > > + if (xudc->curr_utmi_phy) > + tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy); I think you can omit the check here since the function aborts early if the PHY that's passed in is NULL. > + > err = phy_power_on(xudc->curr_utmi_phy); > if (err < 0) > dev_err(xudc->dev, "UTMI power on failed: %d\n", err); > @@ -757,6 +760,9 @@ static void tegra_xudc_device_mode_off(struct tegra_xudc *xudc) > /* Make sure interrupt handler has completed before powergating. */ > synchronize_irq(xudc->irq); > > + if (xudc->curr_utmi_phy) > + tegra_phy_xusb_utmi_pad_power_down(xudc->curr_utmi_phy); Same here. > + > err = phy_power_off(xudc->curr_utmi_phy); > if (err < 0) > dev_err(xudc->dev, "UTMI PHY power off failed: %d\n", err); > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index af946c42b6f0..9e458a748a4f 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -646,6 +646,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd) > return &xhci->usb3_rhub; > return &xhci->usb2_rhub; > } > +EXPORT_SYMBOL_GPL(xhci_get_rhub); > > /* > * xhci_set_port_power() must be called with xhci->lock held. > @@ -1604,6 +1605,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > spin_unlock_irqrestore(&xhci->lock, flags); > return retval; > } > +EXPORT_SYMBOL_GPL(xhci_hub_control); > > /* > * Returns 0 if the status hasn't changed, or the number of bytes in buf. > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index c8af2cd2216d..ea7863cae180 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -2,7 +2,7 @@ > /* > * NVIDIA Tegra xHCI host controller driver > * > - * Copyright (c) 2014-2020, NVIDIA CORPORATION. All rights reserved. > + * Copyright (c) 2014-2022, NVIDIA CORPORATION. All rights reserved. > * Copyright (C) 2014 Google, Inc. > */ > > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { > } fpci; > }; > > +enum tegra_xhci_phy_type { > + USB3_PHY, > + USB2_PHY, > + HSIC_PHY, > + MAX_PHY_TYPES, > +}; > + > struct tegra_xusb_soc { > const char *firmware; > const char * const *supply_names; > @@ -274,10 +281,16 @@ struct tegra_xusb { > > bool suspended; > struct tegra_xusb_context context; > + u32 enable_utmi_pad_after_lp0_exit; > }; > > static struct hc_driver __read_mostly tegra_xhci_hc_driver; > > +static inline struct tegra_xusb *hcd_to_tegra_xusb(struct usb_hcd *hcd) > +{ > + return (struct tegra_xusb *) dev_get_drvdata(hcd->self.controller); > +} > + > static inline u32 fpci_readl(struct tegra_xusb *tegra, unsigned int offset) > { > return readl(tegra->fpci_base + offset); > @@ -1949,12 +1962,32 @@ static void tegra_xhci_enable_phy_sleepwalk_wake(struct tegra_xusb *tegra) > static void tegra_xhci_disable_phy_wake(struct tegra_xusb *tegra) > { > struct tegra_xusb_padctl *padctl = tegra->padctl; > - unsigned int i; > + unsigned int i, j; > + char phy_name[5]; > > for (i = 0; i < tegra->num_phys; i++) { > if (!tegra->phys[i]) > continue; > + if (tegra_xusb_padctl_remote_wake_detected(padctl, tegra->phys[i])) { > + if (i < (tegra->soc->ports.usb3.offset + > + tegra->soc->ports.usb3.count)) { > + j = i; > + strcpy(phy_name, "usb3"); > + } else if (i < (tegra->soc->ports.usb2.offset + > + tegra->soc->ports.usb2.count)) { > + j = i - tegra->soc->ports.usb2.offset; > + strcpy(phy_name, "usb2"); > + > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); > + } else { > + j = i - (tegra->soc->ports.usb2.offset + > + tegra->soc->ports.usb2.count); > + strcpy(phy_name, "hsic"); > + } > + dev_dbg(tegra->dev, > + "%s port %u (0 based) remote wake detected\n", phy_name, j); That's a lot of code for just a debug message. Instead of doing this, can't you simply use something like dev_name(&tegra->phys[i].dev), which should print something like this: phy-usb2.0 for the first USB2 port, etc. That's not *exactly* what you print above, but I think it's close enough and allows us to avoid all that extra code. > > + } > tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); > } > } > @@ -1967,11 +2000,27 @@ static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) > for (i = 0; i < tegra->num_phys; i++) { > if (!tegra->phys[i]) > continue; > - Again, no need to remove this blank line. > tegra_xusb_padctl_disable_phy_sleepwalk(padctl, tegra->phys[i]); > } > } > > +static void tegra_xhci_program_utmi_power_lp0_exit( > + struct tegra_xusb *tegra) This should all fit on a single line, no need to wrap it. > +{ > + unsigned int i; > + > + for (i = 0; i < tegra->soc->ports.usb2.count; i++) { > + if (!is_host_mode_phy(tegra, USB2_PHY, i)) > + continue; > + if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->ports.usb2.offset + i]); > + else > + tegra_phy_xusb_utmi_pad_power_down( > + tegra->phys[tegra->soc->ports.usb2.offset + i]); > + } > +} > + > static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > { > struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); > @@ -1980,6 +2029,7 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > unsigned int i; > int err; > u32 usbcmd; > + u32 portsc; > > dev_dbg(dev, "entering ELPG\n"); > > @@ -1993,6 +2043,16 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > goto out; > } > > + for (i = 0; i < tegra->soc->ports.usb2.count; i++) { > + if (!xhci->usb2_rhub.ports[i]) > + continue; > + portsc = readl(xhci->usb2_rhub.ports[i]->addr); > + tegra->enable_utmi_pad_after_lp0_exit &= ~BIT(i); > + if (((portsc & PORT_PLS_MASK) == XDEV_U3) || > + ((portsc & DEV_SPEED_MASK) == XDEV_FS)) > + tegra->enable_utmi_pad_after_lp0_exit |= BIT(i); > + } > + > err = xhci_suspend(xhci, wakeup); > if (err < 0) { > dev_err(tegra->dev, "failed to suspend XHCI: %d\n", err); > @@ -2067,6 +2127,9 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime) > phy_power_on(tegra->phys[i]); > } > > + if (tegra->suspended) > + tegra_xhci_program_utmi_power_lp0_exit(tegra); > + > tegra_xusb_config(tegra); > tegra_xusb_restore_context(tegra); > > @@ -2437,6 +2500,82 @@ static int tegra_xhci_setup(struct usb_hcd *hcd) > return xhci_gen_setup(hcd, tegra_xhci_quirks); > } > > +static int tegra_xhci_hub_control(struct usb_hcd *hcd, u16 type_req, > + u16 value, u16 index, char *buf, u16 length) > + Indentation is wrong here. Align arguments on wrapped lines with the first argument on the first line. Also, you should be able to fit a bit more on the first line (the limit is 100 characters). There's also a redundant blank line there. > +{ > + struct tegra_xusb *tegra = hcd_to_tegra_xusb(hcd); > + struct xhci_hub *rhub; > + struct xhci_bus_state *bus_state; > + int port = (index & 0xff) - 1; > + int port_index; > + struct xhci_port **ports; > + u32 portsc; > + int ret; > + > + rhub = xhci_get_rhub(hcd); > + bus_state = &rhub->bus_state; > + if (bus_state->resuming_ports && hcd->speed == HCD_USB2) { > + ports = rhub->ports; > + port_index = rhub->num_ports; > + while (port_index--) { > + if (!test_bit(port_index, &bus_state->resuming_ports)) > + continue; > + portsc = readl(ports[port_index]->addr); > + if ((port_index < tegra->soc->ports.usb2.count) How can that even happen? port_index should never exceed the USB2 port count. > + && ((portsc & PORT_PLS_MASK) == XDEV_RESUME)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->ports.usb2.offset + port_index]); > + } > + } > + > + if (hcd->speed == HCD_USB2) { > + if ((type_req == ClearPortFeature) && > + (value == USB_PORT_FEAT_SUSPEND)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->ports.usb2.offset + port]); This reference to the USB2 port PHY is repeated a lot, so it might be worth adding a temporary variable for it. > + } > + > + ret = xhci_hub_control(hcd, type_req, value, index, buf, length); > + > + if ((hcd->speed == HCD_USB2) && (ret == 0)) { This looks odd. Perhaps it'd look a bit more intuitive if you switched this around, something like this: if (ret < 0) return ret; if (hcd->speed == HCD_USB2) { ... } > + if ((type_req == SetPortFeature) && > + (value == USB_PORT_FEAT_SUSPEND)) > + /* We dont suspend the PAD while HNP role swap happens > + * on the OTG port > + */ > + if (!((hcd->self.otg_port == (port + 1)) && > + hcd->self.b_hnp_enable)) > + tegra_phy_xusb_utmi_pad_power_down( > + tegra->phys[tegra->soc->ports.usb2.offset + port]); > + > + if ((type_req == ClearPortFeature) && > + (value == USB_PORT_FEAT_C_CONNECTION)) { > + rhub = xhci_get_rhub(hcd); > + ports = rhub->ports; > + portsc = readl(ports[port]->addr); > + if (portsc & PORT_CONNECT) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->ports.usb2.offset + port]); > + else { > + /* We dont suspend the PAD while HNP > + * role swap happens on the OTG port > + */ > + if (!((hcd->self.otg_port == (port + 1)) > + && hcd->self.b_hnp_enable)) > + tegra_phy_xusb_utmi_pad_power_down( > + tegra->phys[tegra->soc->ports.usb2.offset + port]); > + } > + } > + if ((type_req == SetPortFeature) && > + (value == USB_PORT_FEAT_TEST)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->ports.usb2.offset + port]); > + } Some of the above can take advantage of more horizontal space to avoid wrapping unnecessarily. > + > + return ret; > +} > + > static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { > .reset = tegra_xhci_setup, > }; > @@ -2444,6 +2583,7 @@ static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { > static int __init tegra_xusb_init(void) > { > xhci_init_driver(&tegra_xhci_hc_driver, &tegra_xhci_overrides); > + tegra_xhci_hc_driver.hub_control = tegra_xhci_hub_control; > > return platform_driver_register(&tegra_xusb_driver); > } > 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 Thierry
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Thu, Jul 28, 2022 at 08:39:42AM +0000, Jim Lin wrote:
> Thanks for review, will follow up and then resend.
Also note that html emails are rejected by the mailing lists. Please
fix up your email client so that you can participate in the kernel
development process.
thanks,
greg k-h
diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c index ae3915ed9fef..4eb29189ebfc 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> @@ -642,20 +642,25 @@ static 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; + struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane); struct tegra_xusb_usb2_port *port; - struct device *dev = padctl->dev; unsigned int index = lane->index; u32 value; + dev_dbg(padctl->dev, "power on UTMI pads %u\n", index); + if (!phy) return; port = tegra_xusb_find_usb2_port(padctl, index); if (!port) { - dev_err(dev, "no port found for USB2 lane %u\n", index); + dev_err(padctl->dev, "no port found for USB2 lane %u\n", index); return; } + if (usb2->powered_on) + return; + tegra186_utmi_bias_pad_power_on(padctl); udelay(2); @@ -667,18 +672,26 @@ static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy) value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); value &= ~USB2_OTG_PD_DR; padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); + + usb2->powered_on = true; } static 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; + struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane); unsigned int index = lane->index; u32 value; + dev_dbg(padctl->dev, "power down UTMI pad %u\n", index); + if (!phy) return; + if (!usb2->powered_on) + return; + 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)); @@ -688,8 +701,9 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy) padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); udelay(2); - tegra186_utmi_bias_pad_power_off(padctl); + + usb2->powered_on = false; } static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl, @@ -849,14 +863,11 @@ 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); return 0; } static int tegra186_utmi_phy_power_off(struct phy *phy) { - /* TODO: pad power saving */ tegra_phy_xusb_utmi_pad_power_down(phy); return 0; @@ -1486,6 +1497,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 = tegra_phy_xusb_utmi_pad_power_on, + .utmi_pad_power_down = tegra_phy_xusb_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..a6c7550e3a3a 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> @@ -1446,6 +1446,36 @@ int tegra_xusb_padctl_set_vbus_override(struct tegra_xusb_padctl *padctl, } EXPORT_SYMBOL_GPL(tegra_xusb_padctl_set_vbus_override); +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy) +{ + struct tegra_xusb_lane *lane; + struct tegra_xusb_padctl *padctl; + + if (!phy) + return; + + lane = phy_get_drvdata(phy); + 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; + struct tegra_xusb_padctl *padctl; + + if (!phy) + return; + + lane = phy_get_drvdata(phy); + 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_phy_xusb_utmi_port_reset(struct phy *phy) { struct tegra_xusb_lane *lane = phy_get_drvdata(phy); diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h index 034f7a2c28d6..02cc5c6a588e 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. */ @@ -411,6 +411,8 @@ struct tegra_xusb_padctl_ops { 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)(struct phy *phy); }; diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c index 43f1b0d461c1..bca059b58bc9 100644 --- a/drivers/usb/gadget/udc/tegra-xudc.c +++ b/drivers/usb/gadget/udc/tegra-xudc.c @@ -2,7 +2,7 @@ /* * NVIDIA Tegra XUSB device mode controller * - * Copyright (c) 2013-2019, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 2013-2022, NVIDIA CORPORATION. All rights reserved. * Copyright (c) 2015, Google Inc. */ @@ -703,6 +703,9 @@ static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc) pm_runtime_get_sync(xudc->dev); + if (xudc->curr_utmi_phy) + tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy); + err = phy_power_on(xudc->curr_utmi_phy); if (err < 0) dev_err(xudc->dev, "UTMI power on failed: %d\n", err); @@ -757,6 +760,9 @@ static void tegra_xudc_device_mode_off(struct tegra_xudc *xudc) /* Make sure interrupt handler has completed before powergating. */ synchronize_irq(xudc->irq); + if (xudc->curr_utmi_phy) + tegra_phy_xusb_utmi_pad_power_down(xudc->curr_utmi_phy); + err = phy_power_off(xudc->curr_utmi_phy); if (err < 0) dev_err(xudc->dev, "UTMI PHY power off failed: %d\n", err); diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index af946c42b6f0..9e458a748a4f 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -646,6 +646,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd) return &xhci->usb3_rhub; return &xhci->usb2_rhub; } +EXPORT_SYMBOL_GPL(xhci_get_rhub); /* * xhci_set_port_power() must be called with xhci->lock held. @@ -1604,6 +1605,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, spin_unlock_irqrestore(&xhci->lock, flags); return retval; } +EXPORT_SYMBOL_GPL(xhci_hub_control); /* * Returns 0 if the status hasn't changed, or the number of bytes in buf. diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index c8af2cd2216d..ea7863cae180 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -2,7 +2,7 @@ /* * NVIDIA Tegra xHCI host controller driver * - * Copyright (c) 2014-2020, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 2014-2022, NVIDIA CORPORATION. All rights reserved. * Copyright (C) 2014 Google, Inc. */ @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { } fpci; }; +enum tegra_xhci_phy_type { + USB3_PHY, + USB2_PHY, + HSIC_PHY, + MAX_PHY_TYPES, +}; + struct tegra_xusb_soc { const char *firmware; const char * const *supply_names; @@ -274,10 +281,16 @@ struct tegra_xusb { bool suspended; struct tegra_xusb_context context; + u32 enable_utmi_pad_after_lp0_exit; }; static struct hc_driver __read_mostly tegra_xhci_hc_driver; +static inline struct tegra_xusb *hcd_to_tegra_xusb(struct usb_hcd *hcd) +{ + return (struct tegra_xusb *) dev_get_drvdata(hcd->self.controller); +} + static inline u32 fpci_readl(struct tegra_xusb *tegra, unsigned int offset) { return readl(tegra->fpci_base + offset); @@ -1949,12 +1962,32 @@ static void tegra_xhci_enable_phy_sleepwalk_wake(struct tegra_xusb *tegra) static void tegra_xhci_disable_phy_wake(struct tegra_xusb *tegra) { struct tegra_xusb_padctl *padctl = tegra->padctl; - unsigned int i; + unsigned int i, j; + char phy_name[5]; for (i = 0; i < tegra->num_phys; i++) { if (!tegra->phys[i]) continue; + if (tegra_xusb_padctl_remote_wake_detected(padctl, tegra->phys[i])) { + if (i < (tegra->soc->ports.usb3.offset + + tegra->soc->ports.usb3.count)) { + j = i; + strcpy(phy_name, "usb3"); + } else if (i < (tegra->soc->ports.usb2.offset + + tegra->soc->ports.usb2.count)) { + j = i - tegra->soc->ports.usb2.offset; + strcpy(phy_name, "usb2"); + + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); + } else { + j = i - (tegra->soc->ports.usb2.offset + + tegra->soc->ports.usb2.count); + strcpy(phy_name, "hsic"); + } + dev_dbg(tegra->dev, + "%s port %u (0 based) remote wake detected\n", phy_name, j); + } tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); } } @@ -1967,11 +2000,27 @@ static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) for (i = 0; i < tegra->num_phys; i++) { if (!tegra->phys[i]) continue; - tegra_xusb_padctl_disable_phy_sleepwalk(padctl, tegra->phys[i]); } } +static void tegra_xhci_program_utmi_power_lp0_exit( + struct tegra_xusb *tegra) +{ + unsigned int i; + + for (i = 0; i < tegra->soc->ports.usb2.count; i++) { + if (!is_host_mode_phy(tegra, USB2_PHY, i)) + continue; + if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->ports.usb2.offset + i]); + else + tegra_phy_xusb_utmi_pad_power_down( + tegra->phys[tegra->soc->ports.usb2.offset + i]); + } +} + static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) { struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); @@ -1980,6 +2029,7 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) unsigned int i; int err; u32 usbcmd; + u32 portsc; dev_dbg(dev, "entering ELPG\n"); @@ -1993,6 +2043,16 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) goto out; } + for (i = 0; i < tegra->soc->ports.usb2.count; i++) { + if (!xhci->usb2_rhub.ports[i]) + continue; + portsc = readl(xhci->usb2_rhub.ports[i]->addr); + tegra->enable_utmi_pad_after_lp0_exit &= ~BIT(i); + if (((portsc & PORT_PLS_MASK) == XDEV_U3) || + ((portsc & DEV_SPEED_MASK) == XDEV_FS)) + tegra->enable_utmi_pad_after_lp0_exit |= BIT(i); + } + err = xhci_suspend(xhci, wakeup); if (err < 0) { dev_err(tegra->dev, "failed to suspend XHCI: %d\n", err); @@ -2067,6 +2127,9 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime) phy_power_on(tegra->phys[i]); } + if (tegra->suspended) + tegra_xhci_program_utmi_power_lp0_exit(tegra); + tegra_xusb_config(tegra); tegra_xusb_restore_context(tegra); @@ -2437,6 +2500,82 @@ static int tegra_xhci_setup(struct usb_hcd *hcd) return xhci_gen_setup(hcd, tegra_xhci_quirks); } +static int tegra_xhci_hub_control(struct usb_hcd *hcd, u16 type_req, + u16 value, u16 index, char *buf, u16 length) + +{ + struct tegra_xusb *tegra = hcd_to_tegra_xusb(hcd); + struct xhci_hub *rhub; + struct xhci_bus_state *bus_state; + int port = (index & 0xff) - 1; + int port_index; + struct xhci_port **ports; + u32 portsc; + int ret; + + rhub = xhci_get_rhub(hcd); + bus_state = &rhub->bus_state; + if (bus_state->resuming_ports && hcd->speed == HCD_USB2) { + ports = rhub->ports; + port_index = rhub->num_ports; + while (port_index--) { + if (!test_bit(port_index, &bus_state->resuming_ports)) + continue; + portsc = readl(ports[port_index]->addr); + if ((port_index < tegra->soc->ports.usb2.count) + && ((portsc & PORT_PLS_MASK) == XDEV_RESUME)) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->ports.usb2.offset + port_index]); + } + } + + if (hcd->speed == HCD_USB2) { + if ((type_req == ClearPortFeature) && + (value == USB_PORT_FEAT_SUSPEND)) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->ports.usb2.offset + port]); + } + + ret = xhci_hub_control(hcd, type_req, value, index, buf, length); + + if ((hcd->speed == HCD_USB2) && (ret == 0)) { + if ((type_req == SetPortFeature) && + (value == USB_PORT_FEAT_SUSPEND)) + /* We dont suspend the PAD while HNP role swap happens + * on the OTG port + */ + if (!((hcd->self.otg_port == (port + 1)) && + hcd->self.b_hnp_enable)) + tegra_phy_xusb_utmi_pad_power_down( + tegra->phys[tegra->soc->ports.usb2.offset + port]); + + if ((type_req == ClearPortFeature) && + (value == USB_PORT_FEAT_C_CONNECTION)) { + rhub = xhci_get_rhub(hcd); + ports = rhub->ports; + portsc = readl(ports[port]->addr); + if (portsc & PORT_CONNECT) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->ports.usb2.offset + port]); + else { + /* We dont suspend the PAD while HNP + * role swap happens on the OTG port + */ + if (!((hcd->self.otg_port == (port + 1)) + && hcd->self.b_hnp_enable)) + tegra_phy_xusb_utmi_pad_power_down( + tegra->phys[tegra->soc->ports.usb2.offset + port]); + } + } + if ((type_req == SetPortFeature) && + (value == USB_PORT_FEAT_TEST)) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->ports.usb2.offset + port]); + } + + return ret; +} + static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { .reset = tegra_xhci_setup, }; @@ -2444,6 +2583,7 @@ static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { static int __init tegra_xusb_init(void) { xhci_init_driver(&tegra_xhci_hc_driver, &tegra_xhci_overrides); + tegra_xhci_hc_driver.hub_control = tegra_xhci_hub_control; return platform_driver_register(&tegra_xusb_driver); } 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);