Message ID | 20201217094007.19336-3-digetx@gmail.com (mailing list archive) |
---|---|
State | Superseded |
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:33, Thierry Reding пишет: >> + /* 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. I'll change it in v3, thanks.
On Thu, Dec 17, 2020 at 04:47:50PM +0300, Dmitry Osipenko wrote: > 17.12.2020 16:33, Thierry Reding пишет: > >> + /* 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. > > I'll change it in v3, thanks. Given that my other comment about the WARN_ONCE seems to have been resolved, with the above gotos replaced by conditionals as I suggested: Acked-by: Thierry Reding <treding@nvidia.com>
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: 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: 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) { + 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; + } + return 0; } @@ -827,6 +899,15 @@ static void tegra_usb_phy_shutdown(struct usb_phy *u_phy) phy->freq = NULL; } +static int tegra_usb_phy_set_wakeup(struct usb_phy *u_phy, bool enable) +{ + struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy); + + phy->wakeup_enabled = enable; + + return 0; +} + static int tegra_usb_phy_set_suspend(struct usb_phy *u_phy, int suspend) { struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy); @@ -1198,6 +1279,7 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) tegra_phy->u_phy.dev = &pdev->dev; tegra_phy->u_phy.init = tegra_usb_phy_init; tegra_phy->u_phy.shutdown = tegra_usb_phy_shutdown; + tegra_phy->u_phy.set_wakeup = tegra_usb_phy_set_wakeup; tegra_phy->u_phy.set_suspend = tegra_usb_phy_set_suspend; platform_set_drvdata(pdev, tegra_phy); diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h index c29d1b4c9381..fd1c9f6a4e37 100644 --- a/include/linux/usb/tegra_usb_phy.h +++ b/include/linux/usb/tegra_usb_phy.h @@ -79,6 +79,8 @@ struct tegra_usb_phy { bool is_ulpi_phy; struct gpio_desc *reset_gpio; struct reset_control *pad_rst; + bool wakeup_enabled; + bool pad_wakeup; bool powered_on; };