| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <X9teRPo/MadN79NI@ulmo>
Date: Thu, 17 Dec 2020 14:33:56 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Dmitry Osipenko <digetx@...il.com>
Cc: Jonathan Hunter <jonathanh@...dia.com>,
Peter Chen <Peter.Chen@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alan Stern <stern@...land.harvard.edu>,
Felipe Balbi <balbi@...nel.org>,
Matt Merhar <mattmerhar@...tonmail.com>,
Nicolas Chauvet <kwizart@...il.com>,
Peter Geis <pgwipeout@...il.com>,
Ion Agorria <ion@...rria.com>, linux-tegra@...r.kernel.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/8] usb: phy: tegra: Support waking up from a low
power mode
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@...tonmail.com>
> Tested-by: Nicolas Chauvet <kwizart@...il.com>
> Tested-by: Peter Geis <pgwipeout@...il.com>
> Tested-by: Ion Agorria <ion@...rria.com>
> Signed-off-by: Dmitry Osipenko <digetx@...il.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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists