| Message ID | 20201217094007.19336-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | Support Runtime PM and host mode by Tegra ChipIdea USB driver | expand |
On Thu, Dec 17, 2020 at 12:40:01PM +0300, Dmitry Osipenko wrote: > Support programming of waking up from a low power mode by implementing the > generic set_wakeup() callback of the USB PHY API. > > Tested-by: Matt Merhar <mattmerhar@protonmail.com> > Tested-by: Nicolas Chauvet <kwizart@gmail.com> > Tested-by: Peter Geis <pgwipeout@gmail.com> > Tested-by: Ion Agorria <ion@agorria.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/usb/phy/phy-tegra-usb.c | 94 +++++++++++++++++++++++++++++-- > include/linux/usb/tegra_usb_phy.h | 2 + > 2 files changed, 90 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c > index 1296524e1bee..46a1f61877ad 100644 > --- a/drivers/usb/phy/phy-tegra-usb.c > +++ b/drivers/usb/phy/phy-tegra-usb.c > @@ -45,6 +45,7 @@ > #define TEGRA_PORTSC1_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC) > > #define USB_SUSP_CTRL 0x400 > +#define USB_WAKE_ON_RESUME_EN BIT(2) > #define USB_WAKE_ON_CNNT_EN_DEV BIT(3) > #define USB_WAKE_ON_DISCON_EN_DEV BIT(4) > #define USB_SUSP_CLR BIT(5) > @@ -56,6 +57,15 @@ > #define USB_SUSP_SET BIT(14) > #define USB_WAKEUP_DEBOUNCE_COUNT(x) (((x) & 0x7) << 16) > > +#define USB_PHY_VBUS_SENSORS 0x404 > +#define B_SESS_VLD_WAKEUP_EN BIT(6) > +#define B_VBUS_VLD_WAKEUP_EN BIT(14) > +#define A_SESS_VLD_WAKEUP_EN BIT(22) > +#define A_VBUS_VLD_WAKEUP_EN BIT(30) > + > +#define USB_PHY_VBUS_WAKEUP_ID 0x408 > +#define VBUS_WAKEUP_WAKEUP_EN BIT(30) > + > #define USB1_LEGACY_CTRL 0x410 > #define USB1_NO_LEGACY_MODE BIT(0) > #define USB1_VBUS_SENSE_CTL_MASK (3 << 1) > @@ -334,6 +344,11 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy) > writel_relaxed(val, base + UTMIP_BIAS_CFG0); > } > > + if (phy->pad_wakeup) { > + phy->pad_wakeup = false; > + utmip_pad_count--; > + } > + > spin_unlock(&utmip_pad_lock); > > clk_disable_unprepare(phy->pad_clk); > @@ -359,6 +374,17 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy) > goto ulock; > } > > + /* > + * In accordance to TRM, OTG and Bias pad circuits could be turned off > + * to save power if wake is enabled, but the VBUS-change detection > + * method is board-specific and these circuits may need to be enabled > + * to generate wakeup event, hence we will just keep them both enabled. > + */ > + if (phy->wakeup_enabled) { > + phy->pad_wakeup = true; > + utmip_pad_count++; > + } > + > if (--utmip_pad_count == 0) { > val = readl_relaxed(base + UTMIP_BIAS_CFG0); > val |= UTMIP_OTGPD | UTMIP_BIASPD; > @@ -503,11 +529,24 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy) > writel_relaxed(val, base + UTMIP_PLL_CFG1); > } > > + val = readl_relaxed(base + USB_SUSP_CTRL); > + val &= ~USB_WAKE_ON_RESUME_EN; > + writel_relaxed(val, base + USB_SUSP_CTRL); > + > if (phy->mode == USB_DR_MODE_PERIPHERAL) { > val = readl_relaxed(base + USB_SUSP_CTRL); > val &= ~(USB_WAKE_ON_CNNT_EN_DEV | USB_WAKE_ON_DISCON_EN_DEV); > writel_relaxed(val, base + USB_SUSP_CTRL); > > + val = readl_relaxed(base + USB_PHY_VBUS_WAKEUP_ID); > + val &= ~VBUS_WAKEUP_WAKEUP_EN; > + writel_relaxed(val, base + USB_PHY_VBUS_WAKEUP_ID); > + > + val = readl_relaxed(base + USB_PHY_VBUS_SENSORS); > + val &= ~(A_VBUS_VLD_WAKEUP_EN | A_SESS_VLD_WAKEUP_EN); > + val &= ~(B_VBUS_VLD_WAKEUP_EN | B_SESS_VLD_WAKEUP_EN); > + writel_relaxed(val, base + USB_PHY_VBUS_SENSORS); > + > val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0); > val &= ~UTMIP_PD_CHRG; > writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0); > @@ -605,31 +644,55 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy) > > utmi_phy_clk_disable(phy); > > - if (phy->mode == USB_DR_MODE_PERIPHERAL) { > - val = readl_relaxed(base + USB_SUSP_CTRL); > - val &= ~USB_WAKEUP_DEBOUNCE_COUNT(~0); > - val |= USB_WAKE_ON_CNNT_EN_DEV | USB_WAKEUP_DEBOUNCE_COUNT(5); > - writel_relaxed(val, base + USB_SUSP_CTRL); > - } > + /* PHY won't resume if reset is asserted */ > + if (phy->wakeup_enabled) > + goto chrg_cfg0; > > val = readl_relaxed(base + USB_SUSP_CTRL); > val |= UTMIP_RESET; > writel_relaxed(val, base + USB_SUSP_CTRL); > > +chrg_cfg0: I found this diffcult to read until I realized that it was basically just the equivalent of this: if (!phy->wakeup_enabled) { val = readl_relaxed(base + USB_SUSP_CTRL); val |= UTMIP_RESET; writel_relaxed(val, base + USB_SUSP_CTRL); } > val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0); > val |= UTMIP_PD_CHRG; > writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0); > > + if (phy->wakeup_enabled) > + goto xcvr_cfg1; > + > val = readl_relaxed(base + UTMIP_XCVR_CFG0); > val |= UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN | > UTMIP_FORCE_PDZI_POWERDOWN; > writel_relaxed(val, base + UTMIP_XCVR_CFG0); > > +xcvr_cfg1: Similarly, I think this is more readable as: if (!phy->wakeup_enabled) { val = readl_relaxed(base + UTMIP_XCVR_CFG0); val |= UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN | UTMIP_FORCE_PDZI_POWERDOWN; writel_relaxed(val, base + UTMIP_XCVR_CFG0); } > val = readl_relaxed(base + UTMIP_XCVR_CFG1); > val |= UTMIP_FORCE_PDDISC_POWERDOWN | UTMIP_FORCE_PDCHRP_POWERDOWN | > UTMIP_FORCE_PDDR_POWERDOWN; > writel_relaxed(val, base + UTMIP_XCVR_CFG1); > > + if (phy->wakeup_enabled) { Which then also matches the style of this conditional here. > + val = readl_relaxed(base + USB_SUSP_CTRL); > + val &= ~USB_WAKEUP_DEBOUNCE_COUNT(~0); > + val |= USB_WAKEUP_DEBOUNCE_COUNT(5); > + val |= USB_WAKE_ON_RESUME_EN; > + writel_relaxed(val, base + USB_SUSP_CTRL); > + > + /* > + * Ask VBUS sensor to generate wake event once cable is > + * connected. > + */ > + if (phy->mode == USB_DR_MODE_PERIPHERAL) { > + val = readl_relaxed(base + USB_PHY_VBUS_WAKEUP_ID); > + val |= VBUS_WAKEUP_WAKEUP_EN; > + writel_relaxed(val, base + USB_PHY_VBUS_WAKEUP_ID); > + > + val = readl_relaxed(base + USB_PHY_VBUS_SENSORS); > + val |= A_VBUS_VLD_WAKEUP_EN; > + writel_relaxed(val, base + USB_PHY_VBUS_SENSORS); > + } > + } > + > return utmip_pad_power_off(phy); > } > > @@ -765,6 +828,15 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy) > usleep_range(5000, 6000); > clk_disable_unprepare(phy->clk); > > + /* > + * Wakeup currently unimplemented for ULPI, thus PHY needs to be > + * force-resumed. > + */ > + if (WARN_ON_ONCE(phy->wakeup_enabled)) { > + ulpi_phy_power_on(phy); > + return -EOPNOTSUPP; > + } How do we control phy->wakeup_enabled? Is this something that we can set in device tree, for example? I hope so, because otherwise this would cause a nasty splat every time we try to power-off a ULPI PHY. Thierry
17.12.2020 16:36, Thierry Reding пишет: > On Thu, Dec 17, 2020 at 12:40:03PM +0300, Dmitry Osipenko wrote: >> Rename all occurrences in the code from "udc" to "usb" and change the >> Kconfig entry in order to show that this driver supports USB modes other >> than device-only mode. The follow up patch will add host-mode support and >> it will be cleaner to perform the renaming separately, i.e. in this patch. >> >> Tested-by: Matt Merhar <mattmerhar@protonmail.com> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com> >> Tested-by: Peter Geis <pgwipeout@gmail.com> >> Tested-by: Ion Agorria <ion@agorria.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/usb/chipidea/Kconfig | 2 +- >> drivers/usb/chipidea/ci_hdrc_tegra.c | 78 ++++++++++++++-------------- >> 2 files changed, 40 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig >> index 8bafcfc6080d..8685ead6ccc7 100644 >> --- a/drivers/usb/chipidea/Kconfig >> +++ b/drivers/usb/chipidea/Kconfig >> @@ -53,7 +53,7 @@ config USB_CHIPIDEA_GENERIC >> default USB_CHIPIDEA >> >> config USB_CHIPIDEA_TEGRA >> - tristate "Enable Tegra UDC glue driver" if EMBEDDED >> + tristate "Enable Tegra USB glue driver" if EMBEDDED >> depends on OF >> depends on USB_CHIPIDEA_UDC >> default USB_CHIPIDEA >> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c >> index 10eaaba2a3f0..d8efa80aa1c2 100644 >> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c >> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c >> @@ -12,7 +12,7 @@ >> >> #include "ci.h" >> >> -struct tegra_udc { >> +struct tegra_usb { >> struct ci_hdrc_platform_data data; >> struct platform_device *dev; >> >> @@ -20,15 +20,15 @@ struct tegra_udc { >> struct clk *clk; >> }; >> >> -struct tegra_udc_soc_info { >> +struct tegra_usb_soc_info { >> unsigned long flags; >> }; >> >> -static const struct tegra_udc_soc_info tegra_udc_soc_info = { >> +static const struct tegra_usb_soc_info tegra_udc_soc_info = { >> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA, >> }; >> >> -static const struct of_device_id tegra_udc_of_match[] = { >> +static const struct of_device_id tegra_usb_of_match[] = { >> { >> .compatible = "nvidia,tegra20-udc", > > Do we perhaps also want to add a new tegra20-usb compatible string here > and deprecate the old one since this now no longer properly describes > the device. Ideally it should have been "tegra20-otg" to match TRM, but UDC is also okay since it's a part of OTG and kinda presumes the OTG support of USB1 controller for anyone who read the TRM. Hence there is no need to change the compatible, IMO. > In either case, this looks fine: > > Acked-by: Thierry Reding <treding@nvidia.com> > thanks
On Thu, Dec 17, 2020 at 04:45:03PM +0300, Dmitry Osipenko wrote: > 17.12.2020 16:41, Thierry Reding пишет: > > On Thu, Dec 17, 2020 at 12:40:05PM +0300, Dmitry Osipenko wrote: > >> Tegra PHY driver now supports waking up controller from a low power mode. > >> Enable runtime PM in order to put controller into the LPM during idle. > >> > >> Tested-by: Matt Merhar <mattmerhar@protonmail.com> > >> Tested-by: Nicolas Chauvet <kwizart@gmail.com> > >> Tested-by: Peter Geis <pgwipeout@gmail.com> > >> Tested-by: Ion Agorria <ion@agorria.com> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/usb/chipidea/ci_hdrc_tegra.c | 13 ++++++++++--- > >> 1 file changed, 10 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c > >> index 5fccdeeefc64..655671159511 100644 > >> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c > >> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c > >> @@ -38,21 +38,24 @@ struct tegra_usb_soc_info { > >> > >> static const struct tegra_usb_soc_info tegra20_ehci_soc_info = { > >> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | > >> - CI_HDRC_OVERRIDE_PHY_CONTROL, > >> + CI_HDRC_OVERRIDE_PHY_CONTROL | > >> + CI_HDRC_SUPPORTS_RUNTIME_PM, > >> .dr_mode = USB_DR_MODE_HOST, > >> .txfifothresh = 10, > >> }; > >> > >> static const struct tegra_usb_soc_info tegra30_ehci_soc_info = { > >> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | > >> - CI_HDRC_OVERRIDE_PHY_CONTROL, > >> + CI_HDRC_OVERRIDE_PHY_CONTROL | > >> + CI_HDRC_SUPPORTS_RUNTIME_PM, > >> .dr_mode = USB_DR_MODE_HOST, > >> .txfifothresh = 16, > >> }; > >> > >> static const struct tegra_usb_soc_info tegra_udc_soc_info = { > >> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | > >> - CI_HDRC_OVERRIDE_PHY_CONTROL, > >> + CI_HDRC_OVERRIDE_PHY_CONTROL | > >> + CI_HDRC_SUPPORTS_RUNTIME_PM, > >> .dr_mode = USB_DR_MODE_UNKNOWN, > >> }; > >> > >> @@ -323,6 +326,10 @@ static int tegra_usb_probe(struct platform_device *pdev) > >> usb->data.hub_control = tegra_ehci_hub_control; > >> usb->data.notify_event = tegra_usb_notify_event; > >> > >> + /* Tegra PHY driver currently doesn't support LPM for ULPI */ > >> + if (of_usb_get_phy_mode(pdev->dev.of_node) == USBPHY_INTERFACE_MODE_ULPI) > >> + usb->data.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM; > >> + > > > > Does this prevent the wakeup_enabled from being set for ULPI PHYs? > > Yes Okay, it should be fine then to keep that WARN_ONCE in that prior patch since we should really only get there if there's something seriously wrong. Thierry
17.12.2020 18:02, Thierry Reding пишет: > On Thu, Dec 17, 2020 at 04:45:03PM +0300, Dmitry Osipenko wrote: >> 17.12.2020 16:41, Thierry Reding пишет: >>> On Thu, Dec 17, 2020 at 12:40:05PM +0300, Dmitry Osipenko wrote: >>>> Tegra PHY driver now supports waking up controller from a low power mode. >>>> Enable runtime PM in order to put controller into the LPM during idle. >>>> >>>> Tested-by: Matt Merhar <mattmerhar@protonmail.com> >>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> >>>> Tested-by: Peter Geis <pgwipeout@gmail.com> >>>> Tested-by: Ion Agorria <ion@agorria.com> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/usb/chipidea/ci_hdrc_tegra.c | 13 ++++++++++--- >>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c >>>> index 5fccdeeefc64..655671159511 100644 >>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c >>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c >>>> @@ -38,21 +38,24 @@ struct tegra_usb_soc_info { >>>> >>>> static const struct tegra_usb_soc_info tegra20_ehci_soc_info = { >>>> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | >>>> - CI_HDRC_OVERRIDE_PHY_CONTROL, >>>> + CI_HDRC_OVERRIDE_PHY_CONTROL | >>>> + CI_HDRC_SUPPORTS_RUNTIME_PM, >>>> .dr_mode = USB_DR_MODE_HOST, >>>> .txfifothresh = 10, >>>> }; >>>> >>>> static const struct tegra_usb_soc_info tegra30_ehci_soc_info = { >>>> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | >>>> - CI_HDRC_OVERRIDE_PHY_CONTROL, >>>> + CI_HDRC_OVERRIDE_PHY_CONTROL | >>>> + CI_HDRC_SUPPORTS_RUNTIME_PM, >>>> .dr_mode = USB_DR_MODE_HOST, >>>> .txfifothresh = 16, >>>> }; >>>> >>>> static const struct tegra_usb_soc_info tegra_udc_soc_info = { >>>> .flags = CI_HDRC_REQUIRES_ALIGNED_DMA | >>>> - CI_HDRC_OVERRIDE_PHY_CONTROL, >>>> + CI_HDRC_OVERRIDE_PHY_CONTROL | >>>> + CI_HDRC_SUPPORTS_RUNTIME_PM, >>>> .dr_mode = USB_DR_MODE_UNKNOWN, >>>> }; >>>> >>>> @@ -323,6 +326,10 @@ static int tegra_usb_probe(struct platform_device *pdev) >>>> usb->data.hub_control = tegra_ehci_hub_control; >>>> usb->data.notify_event = tegra_usb_notify_event; >>>> >>>> + /* Tegra PHY driver currently doesn't support LPM for ULPI */ >>>> + if (of_usb_get_phy_mode(pdev->dev.of_node) == USBPHY_INTERFACE_MODE_ULPI) >>>> + usb->data.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM; >>>> + >>> >>> Does this prevent the wakeup_enabled from being set for ULPI PHYs? >> >> Yes > > Okay, it should be fine then to keep that WARN_ONCE in that prior patch > since we should really only get there if there's something seriously > wrong. Correct, that is the actual intention of the warning.
This series implements Runtime PM support for the Tegra ChipIdea USB driver. It also squashes the older ehci-tegra driver into the ChipIdea driver, hence the RPM is supported by both UDC and host controllers, secondly this opens opportunity for implementing OTG support in the future. Patchset was tested on various Tegra20, Tegra30 and Tegra124 devices. Thanks to Peter Geis, Matt Merhar, Nicolas Chauvet and Ion Agorria for helping with the extensive and productive testing! Changelog: v2: - Improved comments in the code as it was suggested by Peter Chen and Sergei Shtylyov for v1. - Replaced mdelay() with fsleep() and made ci->hdc to reset to NULL in a error code path, like it was suggested by Peter Chen. - Redirected deprecated USB_EHCI_TEGRA Kconfig entry to USB_CHIPIDEA_TEGRA as was suggested by Alan Stern. - Improved commit message and added ACK from Thierry Reding to the patch that removes MODULE_ALIAS. - Fixed UDC PHY waking up on ASUS TF201 tablet device by utilizing additional VBUS sensor. This was reported and tested by Ion Agorria. - Added t-b from Ion Agorria. Dmitry Osipenko (7): usb: phy: tegra: Add delay after power up usb: phy: tegra: Support waking up from a low power mode usb: chipidea: tegra: Remove MODULE_ALIAS usb: chipidea: tegra: Rename UDC to USB usb: chipidea: tegra: Support runtime PM usb: host: ehci-tegra: Remove the driver ARM: tegra_defconfig: Enable USB_CHIPIDEA and remove USB_EHCI_TEGRA Peter Geis (1): usb: chipidea: tegra: Support host mode arch/arm/configs/tegra_defconfig | 3 +- drivers/usb/chipidea/Kconfig | 3 +- drivers/usb/chipidea/ci_hdrc_tegra.c | 327 +++++++++++++-- drivers/usb/chipidea/core.c | 10 +- drivers/usb/chipidea/host.c | 104 ++++- drivers/usb/host/Kconfig | 8 +- drivers/usb/host/Makefile | 1 - drivers/usb/host/ehci-tegra.c | 604 --------------------------- drivers/usb/phy/phy-tegra-usb.c | 97 ++++- include/linux/usb/chipidea.h | 6 + include/linux/usb/tegra_usb_phy.h | 2 + 11 files changed, 503 insertions(+), 662 deletions(-) delete mode 100644 drivers/usb/host/ehci-tegra.c