| Message ID | 20221027133127.27592-4-jilin@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xhci: tegra: USB2 pad power controls | expand |
On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > Program USB2 pad PD controls during port connect/disconnect, port > suspend/resume, and test mode, to reduce power consumption on > disconnect or suspend. > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > Signed-off-by: JC Kuo <jckuo@nvidia.com> > Signed-off-by: Jim Lin <jilin@nvidia.com> Who is the author here? These do not seem to be in the correct order if you are the author, right? > > --- > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > v3: No change on copyright > v4: Remove hcd_to_tegra_xusb() function which is used only once. > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > Invoke xhci_hub_control() directly (xhci-tegra.c) > > drivers/usb/host/xhci-tegra.c | 131 +++++++++++++++++++++++++++++++++- > 1 file changed, 130 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index c8af2cd2216d..f685bb7459ba 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -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,6 +281,7 @@ struct tegra_xusb { > > bool suspended; > struct tegra_xusb_context context; > + u32 enable_utmi_pad_after_lp0_exit; This is a bitfield, how do we know it will fit in a u32? What is the range you are putting in here? thanks, greg k-h
On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote: > External email: Use caution opening links or attachments > > > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > > Program USB2 pad PD controls during port connect/disconnect, port > > suspend/resume, and test mode, to reduce power consumption on > > disconnect or suspend. > > > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > > Signed-off-by: JC Kuo <jckuo@nvidia.com> > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > Who is the author here? These do not seem to be in the correct order > if > you are the author, right? > > This is an old patch. Each time went with some small modification. Petlozu is author for local Kernel 3.18 Then JC for local Kernel 4.4 Now my turn for Kernel 5.xx > > > > > --- > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > > v3: No change on copyright > > v4: Remove hcd_to_tegra_xusb() function which is used only once. > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > > Invoke xhci_hub_control() directly (xhci-tegra.c) > > > > drivers/usb/host/xhci-tegra.c | 131 > > +++++++++++++++++++++++++++++++++- > > 1 file changed, 130 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- > > tegra.c > > index c8af2cd2216d..f685bb7459ba 100644 > > --- a/drivers/usb/host/xhci-tegra.c > > +++ b/drivers/usb/host/xhci-tegra.c > > @@ -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,6 +281,7 @@ struct tegra_xusb { > > > > bool suspended; > > struct tegra_xusb_context context; > > + u32 enable_utmi_pad_after_lp0_exit; > > This is a bitfield, how do we know it will fit in a u32? What is the > range you are putting in here? > > thanks, > > greg k-h static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra) { unsigned int i; for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { if (!is_host_mode_phy(tegra, USB2_PHY, i)) continue; /* USB2 */ if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) : How many bits to be used is based on tegra->soc- >phy_types[USB2_PHY].num which is defined like static const struct tegra_xusb_phy_type tegra210_phy_types[] = { : { .name = "usb2", .num = 4, }, : }; static const struct tegra_xusb_phy_type tegra194_phy_types[] = { : { .name = "usb2", .num = 4, }, }; , so far at most 4. Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough. --nvpublic
On Fri, Oct 28, 2022 at 03:11:31AM +0000, Jim Lin wrote: > On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > > > Program USB2 pad PD controls during port connect/disconnect, port > > > suspend/resume, and test mode, to reduce power consumption on > > > disconnect or suspend. > > > > > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > > > Signed-off-by: JC Kuo <jckuo@nvidia.com> > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > > > Who is the author here? These do not seem to be in the correct order > > if > > you are the author, right? > > > This is an old patch. Each time went with some small modification. > > > Petlozu is author for local Kernel 3.18 > > Then JC for local Kernel 4.4 > Now my turn for Kernel 5.xx Then you all need to figure out how to proper document this (hint, read the documentation for how to do that...) > > > --- > > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > > > v3: No change on copyright > > > v4: Remove hcd_to_tegra_xusb() function which is used only once. > > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > > > Invoke xhci_hub_control() directly (xhci-tegra.c) > > > > > > drivers/usb/host/xhci-tegra.c | 131 > > > +++++++++++++++++++++++++++++++++- > > > 1 file changed, 130 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- > > > tegra.c > > > index c8af2cd2216d..f685bb7459ba 100644 > > > --- a/drivers/usb/host/xhci-tegra.c > > > +++ b/drivers/usb/host/xhci-tegra.c > > > @@ -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,6 +281,7 @@ struct tegra_xusb { > > > > > > bool suspended; > > > struct tegra_xusb_context context; > > > + u32 enable_utmi_pad_after_lp0_exit; > > > > This is a bitfield, how do we know it will fit in a u32? What is the > > range you are putting in here? > > > > thanks, > > > > greg k-h > static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb > *tegra) > { > unsigned int i; > > for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { > if (!is_host_mode_phy(tegra, USB2_PHY, i)) > continue; > /* USB2 */ > if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) > : > How many bits to be used is based on tegra->soc- > >phy_types[USB2_PHY].num which is defined like > > static const struct tegra_xusb_phy_type tegra210_phy_types[] = { > : > { .name = "usb2", .num = 4, }, > : > }; > > static const struct tegra_xusb_phy_type tegra194_phy_types[] = { > : > { .name = "usb2", .num = 4, }, > }; > , so far at most 4. > > Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough. I am sorry, I do not understand. If you are needing to treat this as a bitfield, then properly define it that way so it is obvious what is happening. As it is, this is very confusing and I do not understand what is going on at all. What is the relationship to "num" and the bit being set? Why set a bit at all? thanks, greg k-h
On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > Program USB2 pad PD controls during port connect/disconnect, port > suspend/resume, and test mode, to reduce power consumption on > disconnect or suspend. > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > Signed-off-by: JC Kuo <jckuo@nvidia.com> > Signed-off-by: Jim Lin <jilin@nvidia.com> As Greg mentioned, these Signed-off-by: lines should be paired up with Co-developed-by: lines to give additional information about what their respective role was in the patch submission. A Signed-off-by alone has no additional information about the actor, so the implication is that they simply forwarded the patches, which would be what a maintainer would typically do. Signed-off-by: lines should also be in chronological order, so So if Petlozu and JC did not author and simply forward the patches, then you as the author (taken from the From: field) would need to appear before both of them. Also in that case, you wouldn't typically be sending the patch. Co-developed-by: in this case provides additional information, indicating that Petlozu and JC co-authored the patch. A Signed-off-by from them is still required, though. Your From: line says that you also authored it (i.e. you become the primary author). Since you then send it upstream the Signed-off-by is then in the right place. Also, since the authorship is already implied from the From: header, no need to include a Co-developed-by: line for yourself. > --- > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > v3: No change on copyright > v4: Remove hcd_to_tegra_xusb() function which is used only once. > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > Invoke xhci_hub_control() directly (xhci-tegra.c) > > drivers/usb/host/xhci-tegra.c | 131 +++++++++++++++++++++++++++++++++- > 1 file changed, 130 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index c8af2cd2216d..f685bb7459ba 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { > } fpci; > }; > > +enum tegra_xhci_phy_type { > + USB3_PHY, > + USB2_PHY, > + HSIC_PHY, > + MAX_PHY_TYPES, > +}; This is a little messy. This is the default order that we define, but there's no strict requirement for the entries in tegra_xusb_phy_type to be in this order. To avoid this, we've used string comparisions in other places, which isn't great either, but it's at least a bit more consistent. > + > struct tegra_xusb_soc { > const char *firmware; > const char * const *supply_names; > @@ -274,6 +281,7 @@ struct tegra_xusb { > > bool suspended; > struct tegra_xusb_context context; > + u32 enable_utmi_pad_after_lp0_exit; Perhaps call this lp0_utmi_pad_mask or something to make it clearer that this represents a mask of bits, one for each UTMI pad. > }; > > static struct hc_driver __read_mostly tegra_xhci_hc_driver; > @@ -1949,12 +1957,30 @@ 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; > > 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->phy_types[USB3_PHY].num) { > + /* USB3 */ > + j = i; > + } else if (i < (tegra->soc->phy_types[USB3_PHY].num + > + tegra->soc->phy_types[USB2_PHY].num)) { > + /* USB2 */ > + j = i - tegra->soc->phy_types[USB3_PHY].num; > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); > + } else { > + /* HSIC */ > + j = i - (tegra->soc->phy_types[USB3_PHY].num + > + tegra->soc->phy_types[USB2_PHY].num); > + } > + dev_dbg(tegra->dev, > + "%s port %u (0 based) remote wake detected\n", > + dev_name(&tegra->phys[i]->dev), j); It looks like the only reason we do all this is to print out a debug message. Can we not just stick with dev_name(&tegra->phys[i]->dev)? That should already be unique. I guess the only downside is that the index included in that string is not 0 based (PHY core uses a global index for some reason), but I'm not sure all this extra effort is warranted just for that. If it is, perhaps we need to find some other way to get at the actual index. Maybe the PHY driver can be modified to make the device name reflect a more meaningful name. Or perhaps yet another solution would be to print the PHY's OF node name: "%pOFn", phy->dev.of_node > This blank line was originally after the "continue;" above, to make things a bit more readable, so put it back there. > + } > tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); Again, could use a blank line after the closing } for readability. > } > } > @@ -1972,6 +1998,23 @@ static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) > } > } > > +static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra) > +{ > + unsigned int i; > + > + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { > + if (!is_host_mode_phy(tegra, USB2_PHY, i)) > + continue; > + /* USB2 */ > + if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); > + else > + tegra_phy_xusb_utmi_pad_power_down( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); This is all very confusing. The following should get you the right PHY in a more straightforward way: struct phy *phy = tegra_xusb_get_phy(tegra, "usb2", i); > + } > +} > + > static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > { > struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); > @@ -1980,6 +2023,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 +2037,15 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > goto out; > } > > + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; 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); > @@ -2066,6 +2119,8 @@ 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,8 +2492,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 = dev_get_drvdata(hcd->self.controller); > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + struct xhci_hub *rhub; > + struct xhci_bus_state *bus_state; > + int port = (index & 0xff) - 1; > + int i; "unsigned int i" to match rhub->num_ports; The variable name choice is a little confusing to me here. I wonder if perhaps this would be clearer if you didn't reuse "i" for the PHY index and instead used the tegra_xusb_get_phy() as before. See below... > + struct xhci_port **ports; > + u32 portsc; > + int ret; > + > + rhub = &xhci->usb2_rhub; > + bus_state = &rhub->bus_state; > + if (bus_state->resuming_ports && hcd->speed == HCD_USB2) { > + ports = rhub->ports; > + i = rhub->num_ports; > + while (i--) { > + if (!test_bit(i, &bus_state->resuming_ports)) > + continue; > + portsc = readl(ports[i]->addr); > + if ((portsc & PORT_PLS_MASK) == XDEV_RESUME) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); This could then be: phy = tegra_xusb_get_phy(tegra, "usb2", i); tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + } > + > + if (hcd->speed == HCD_USB2) { > + i = tegra->soc->phy_types[USB3_PHY].num + port; Similarly: phy = tegra_xusb_get_phy(tegra, "usb2", i); > + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) { > + if (!index || index > rhub->num_ports) > + return -EPIPE; > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); And: tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_RESET)) { > + if (!index || index > rhub->num_ports) > + return -EPIPE; > + ports = rhub->ports; > + portsc = readl(ports[port]->addr); > + if (portsc & PORT_CONNECT) > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); And, finally: tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + } > + > + ret = xhci_hub_control(hcd, type_req, value, index, buf, length); > + if (ret < 0) > + return ret; > + > + if (hcd->speed == HCD_USB2) { It might be a good idea to add a comment here about where "i" (or "phy" in the alternative I proposed) has been set. I kept thinking that this was accidentally using an old value, whereas it's really doing so on purpose. Maybe that could be made a bit clearer by making this second check: if (phy) { ... } If you then declare phy = NULL at the top level of this function, it becomes clearer that it's reusing something that was previously obtained. That's less clear (to me) from looking at the current check. > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) > + /* We don't 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[i]); > + > + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_C_CONNECTION)) { > + ports = rhub->ports; > + portsc = readl(ports[port]->addr); > + if (!(portsc & PORT_CONNECT)) { > + /* We don't 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[i]); > + } > + } > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_TEST)) > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); > + } > + > + return ret; > +} Honestly, after going through all this I'm beginning to wonder if perhaps we should encapsulate all of this better. On one hand we've got a bunch of low-level stuff being done here in the XHCI driver that would fit better into the XUSB pad control driver (where the PHY implementation lives). We may be missing a bit of infrastructure here, but perhaps we can add something to improve this overall? Secondly, this whole .hub_control patching seems overly complicated to me. But maybe I'm being a bit naive and there's really no better way to do this currently. Thierry > + > static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { > .reset = tegra_xhci_setup, > + .hub_control = tegra_xhci_hub_control, > }; > > static int __init tegra_xusb_init(void) > -- > 2.17.1 >
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index c8af2cd2216d..f685bb7459ba 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -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,6 +281,7 @@ 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; @@ -1949,12 +1957,30 @@ 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; 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->phy_types[USB3_PHY].num) { + /* USB3 */ + j = i; + } else if (i < (tegra->soc->phy_types[USB3_PHY].num + + tegra->soc->phy_types[USB2_PHY].num)) { + /* USB2 */ + j = i - tegra->soc->phy_types[USB3_PHY].num; + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); + } else { + /* HSIC */ + j = i - (tegra->soc->phy_types[USB3_PHY].num + + tegra->soc->phy_types[USB2_PHY].num); + } + dev_dbg(tegra->dev, + "%s port %u (0 based) remote wake detected\n", + dev_name(&tegra->phys[i]->dev), j); + } tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); } } @@ -1972,6 +1998,23 @@ static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) } } +static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra) +{ + unsigned int i; + + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { + if (!is_host_mode_phy(tegra, USB2_PHY, i)) + continue; + /* USB2 */ + if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); + else + tegra_phy_xusb_utmi_pad_power_down( + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); + } +} + static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) { struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); @@ -1980,6 +2023,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 +2037,15 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) goto out; } + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; 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); @@ -2066,6 +2119,8 @@ 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,8 +2492,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 = dev_get_drvdata(hcd->self.controller); + struct xhci_hcd *xhci = hcd_to_xhci(hcd); + struct xhci_hub *rhub; + struct xhci_bus_state *bus_state; + int port = (index & 0xff) - 1; + int i; + struct xhci_port **ports; + u32 portsc; + int ret; + + rhub = &xhci->usb2_rhub; + bus_state = &rhub->bus_state; + if (bus_state->resuming_ports && hcd->speed == HCD_USB2) { + ports = rhub->ports; + i = rhub->num_ports; + while (i--) { + if (!test_bit(i, &bus_state->resuming_ports)) + continue; + portsc = readl(ports[i]->addr); + if ((portsc & PORT_PLS_MASK) == XDEV_RESUME) + tegra_phy_xusb_utmi_pad_power_on( + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); + } + } + + if (hcd->speed == HCD_USB2) { + i = tegra->soc->phy_types[USB3_PHY].num + port; + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) { + if (!index || index > rhub->num_ports) + return -EPIPE; + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); + } + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_RESET)) { + if (!index || index > rhub->num_ports) + return -EPIPE; + ports = rhub->ports; + portsc = readl(ports[port]->addr); + if (portsc & PORT_CONNECT) + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); + } + } + + ret = xhci_hub_control(hcd, type_req, value, index, buf, length); + if (ret < 0) + return ret; + + if (hcd->speed == HCD_USB2) { + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) + /* We don't 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[i]); + + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_C_CONNECTION)) { + ports = rhub->ports; + portsc = readl(ports[port]->addr); + if (!(portsc & PORT_CONNECT)) { + /* We don't 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[i]); + } + } + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_TEST)) + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); + } + + return ret; +} + static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { .reset = tegra_xhci_setup, + .hub_control = tegra_xhci_hub_control, }; static int __init tegra_xusb_init(void)