| Message ID | 20221012102511.3093-1-jilin@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] xhci: tegra: USB2 pad power controls | expand |
On Wed, Oct 12, 2022 at 06:25:11PM +0800, Jim Lin wrote: > +static inline struct tegra_xusb *hcd_to_tegra_xusb(struct usb_hcd *hcd) > +{ > + return (struct tegra_xusb *) dev_get_drvdata(hcd->self.controller); No need for the cast (and if there was, no need for the extra space). thanks, greg k-h
On 12/10/2022 12:18, Greg KH wrote: > On Wed, Oct 12, 2022 at 06:25:11PM +0800, Jim Lin wrote: >> +static inline struct tegra_xusb *hcd_to_tegra_xusb(struct usb_hcd *hcd) >> +{ >> + return (struct tegra_xusb *) dev_get_drvdata(hcd->self.controller); > > No need for the cast (and if there was, no need for the extra space). May be best to drop this inline function completely and call dev_get_drvdata() directly. I only see it used in one place. Jon
On 12/10/2022 11:25, 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: Jim Lin <jilin@nvidia.com> > --- > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > v3: No change on copyright > > drivers/usb/host/xhci-tegra.c | 139 +++++++++++++++++++++++++++++++++- > 1 file changed, 138 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index c8af2cd2216d..996182a1959f 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,9 +281,17 @@ 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 int (*original_xhci_hub_control)(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, > + char *buf, u16 wLength); Is it better to add this function pointer to the tegra_xusb structure? Jon
On Wed, 2022-10-12 at 12:43 +0100, Jon Hunter wrote: > On 12/10/2022 12:18, Greg KH wrote: > > On Wed, Oct 12, 2022 at 06:25:11PM +0800, Jim Lin wrote: > > > +static inline struct tegra_xusb *hcd_to_tegra_xusb(struct > > > usb_hcd *hcd) > > > +{ > > > + return (struct tegra_xusb *) dev_get_drvdata(hcd- > > > >self.controller); > > > > No need for the cast (and if there was, no need for the extra > > space). > > May be best to drop this inline function completely and call > dev_get_drvdata() directly. I only see it used in one place. > > Jon > Thanks, will do. --nvpublic
On Wed, 2022-10-12 at 12:45 +0100, Jon Hunter wrote: > On 12/10/2022 11:25, 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: Jim Lin <jilin@nvidia.com> > > --- > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > > v3: No change on copyright > > > > drivers/usb/host/xhci-tegra.c | 139 > > +++++++++++++++++++++++++++++++++- > > 1 file changed, 138 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- > > tegra.c > > index c8af2cd2216d..996182a1959f 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,9 +281,17 @@ 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 int (*original_xhci_hub_control)(struct usb_hcd *hcd, u16 > > typeReq, u16 wValue, u16 wIndex, > > + char *buf, u16 wLength); > > Is it better to add this function pointer to the tegra_xusb > structure? > > Jon > Do you mean removing variable "original_xhci_hub_control" and save function pointer to the tegra_xusb structure ? But that doesn't look possible over here to point to tegra_xusb structure and save pointer (to tegra_xhci_hc_driver.hub_control) into it. " static int __init tegra_xusb_init(void) { xhci_init_driver(&tegra_xhci_hc_driver, &tegra_xhci_overrides); return platform_driver_register(&tegra_xusb_driver); } module_init(tegra_xusb_init); " --nvpublic
On 17/10/2022 10:36, Jim Lin wrote: > On Wed, 2022-10-12 at 12:45 +0100, Jon Hunter wrote: >> On 12/10/2022 11:25, 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: Jim Lin <jilin@nvidia.com> >>> --- >>> v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 >>> v3: No change on copyright >>> >>> drivers/usb/host/xhci-tegra.c | 139 >>> +++++++++++++++++++++++++++++++++- >>> 1 file changed, 138 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- >>> tegra.c >>> index c8af2cd2216d..996182a1959f 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,9 +281,17 @@ 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 int (*original_xhci_hub_control)(struct usb_hcd *hcd, u16 >>> typeReq, u16 wValue, u16 wIndex, >>> + char *buf, u16 wLength); >> >> Is it better to add this function pointer to the tegra_xusb >> structure? >> >> Jon >> > Do you mean removing variable "original_xhci_hub_control" and save > function pointer to the tegra_xusb structure ? > > But that doesn't look possible over here to point to tegra_xusb > structure and save pointer (to tegra_xhci_hc_driver.hub_control) into > it. Ah yes, this is in the init function and so we can't do that. Any issue with calling xhci_hub_control() directly in the function tegra_xhci_hub_control()? Jon
On Mon, 2022-10-17 at 21:17 +0100, Jon Hunter wrote: > On 17/10/2022 10:36, Jim Lin wrote: > > On Wed, 2022-10-12 at 12:45 +0100, Jon Hunter wrote: > > > On 12/10/2022 11:25, 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: Jim Lin <jilin@nvidia.com> > > > > --- > > > > v2: Fix issue that wrong tegra->phys[] may be accessed on > > > > tegra124 > > > > v3: No change on copyright > > > > > > > > drivers/usb/host/xhci-tegra.c | 139 > > > > +++++++++++++++++++++++++++++++++- > > > > 1 file changed, 138 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/host/xhci-tegra.c > > > > b/drivers/usb/host/xhci- > > > > tegra.c > > > > index c8af2cd2216d..996182a1959f 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,9 +281,17 @@ 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 int (*original_xhci_hub_control)(struct usb_hcd *hcd, > > > > u16 > > > > typeReq, u16 wValue, u16 wIndex, > > > > + char *buf, u16 wLength); > > > > > > Is it better to add this function pointer to the tegra_xusb > > > structure? > > > > > > Jon > > > > > > > Do you mean removing variable "original_xhci_hub_control" and save > > function pointer to the tegra_xusb structure ? > > > > But that doesn't look possible over here to point to tegra_xusb > > structure and save pointer (to tegra_xhci_hc_driver.hub_control) > > into > > it. > > > Ah yes, this is in the init function and so we can't do that. Any > issue > with calling xhci_hub_control() directly in the function > tegra_xhci_hub_control()? > > Jon > Thanks, will try. --nvpublic
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index c8af2cd2216d..996182a1959f 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,9 +281,17 @@ 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 int (*original_xhci_hub_control)(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, + char *buf, u16 wLength); + +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) { @@ -1949,12 +1964,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 +2005,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 +2030,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 +2044,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 +2126,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,6 +2499,79 @@ 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_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 = (*original_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, }; @@ -2444,6 +2579,8 @@ 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); + original_xhci_hub_control = tegra_xhci_hc_driver.hub_control; + tegra_xhci_hc_driver.hub_control = tegra_xhci_hub_control; return platform_driver_register(&tegra_xusb_driver); }
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: Jim Lin <jilin@nvidia.com> --- v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 v3: No change on copyright drivers/usb/host/xhci-tegra.c | 139 +++++++++++++++++++++++++++++++++- 1 file changed, 138 insertions(+), 1 deletion(-)