Re: [PATCH v8 09/13] phy: tegra: xusb: Add wake/sleepwalk for Tegra210

[Vinod Koul <vkoul@kernel.org>]

On 25-03-21, 17:40, Thierry Reding wrote:

> +static int tegra210_usb3_enable_phy_sleepwalk(struct tegra_xusb_lane *lane,
> +					      enum usb_device_speed speed)
> +{
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	int port = tegra210_usb3_lane_map(lane);
> +	struct device *dev = padctl->dev;
> +	u32 value;
> +
> +	if (port < 0) {
> +		dev_err(dev, "invalid usb3 port number\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "phy enable sleepwalk usb3 %d\n", port);

Too much noise for my taste :)
(here and other places)

> +static int tegra210_pmc_utmi_enable_phy_sleepwalk(struct tegra_xusb_lane *lane,
> +						  enum usb_device_speed speed)
> +{
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra210_xusb_padctl *priv = to_tegra210_xusb_padctl(padctl);
> +	struct device *dev = padctl->dev;
> +	unsigned int port = lane->index;
> +	u32 value, tctrl, pctrl, rpd_ctrl;
> +
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;
> +
> +	if (speed > USB_SPEED_HIGH)
> +		return -EINVAL;
> +
> +	dev_dbg(dev, "phy enable sleepwalk usb2 %d speed %d\n", port, speed);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
> +	tctrl = TCTRL_VALUE(value);
> +	pctrl = PCTRL_VALUE(value);
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(port));
> +	rpd_ctrl = RPD_CTRL_VALUE(value);
> +
> +	/* ensure sleepwalk logic is disabled */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_SLEEP_CFG(port));
> +	value &= ~UTMIP_MASTER_ENABLE(port);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_SLEEP_CFG(port));
> +
> +	/* ensure sleepwalk logics are in low power mode */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_MASTER_CONFIG);
> +	value |= UTMIP_PWR(port);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_MASTER_CONFIG);

We really should have a read_modify_write() helper.. quite repeat of
this here

> +
> +	/* set debounce time */
> +	value = padctl_pmc_readl(priv, PMC_USB_DEBOUNCE_DEL);
> +	value &= ~UTMIP_LINE_DEB_CNT(~0);
> +	value |= UTMIP_LINE_DEB_CNT(0x1);
> +	padctl_pmc_writel(priv, value, PMC_USB_DEBOUNCE_DEL);
> +
> +	/* ensure fake events of sleepwalk logic are desiabled */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_FAKE(port));
> +	value &= ~(UTMIP_FAKE_USBOP_VAL(port) | UTMIP_FAKE_USBON_VAL(port) |
> +			UTMIP_FAKE_USBOP_EN(port) | UTMIP_FAKE_USBON_EN(port));
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_FAKE(port));
> +
> +	/* ensure wake events of sleepwalk logic are not latched */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_LINE_WAKEUP);
> +	value &= ~UTMIP_LINE_WAKEUP_EN(port);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_LINE_WAKEUP);
> +
> +	/* disable wake event triggers of sleepwalk logic */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_SLEEP_CFG(port));
> +	value &= ~UTMIP_WAKE_VAL(port, ~0);
> +	value |= UTMIP_WAKE_VAL_NONE(port);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_SLEEP_CFG(port));
> +
> +	/* power down the line state detectors of the pad */
> +	value = padctl_pmc_readl(priv, PMC_USB_AO);
> +	value |= (USBOP_VAL_PD(port) | USBON_VAL_PD(port));
> +	padctl_pmc_writel(priv, value, PMC_USB_AO);
> +
> +	/* save state per speed */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_SAVED_STATE(port));
> +	value &= ~SPEED(port, ~0);
> +	if (speed == USB_SPEED_HIGH)
> +		value |= UTMI_HS(port);
> +	else if (speed == USB_SPEED_FULL)
> +		value |= UTMI_FS(port);
> +	else if (speed == USB_SPEED_LOW)
> +		value |= UTMI_LS(port);
> +	else
> +		value |= UTMI_RST(port);

This could look better with a switch statement

> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_SAVED_STATE(port));
> +
> +	/* enable the trigger of the sleepwalk logic */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_SLEEPWALK_CFG(port));
> +	value |= UTMIP_LINEVAL_WALK_EN(port);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_SLEEPWALK_CFG(port));
> +
> +	/* reset the walk pointer and clear the alarm of the sleepwalk logic,
> +	 * as well as capture the configuration of the USB2.0 pad
> +	 */

/*
 * multi
 * line style please
 */

> +	value = padctl_pmc_readl(priv, PMC_UTMIP_UHSIC_TRIGGERS);
> +	value |= (UTMIP_CLR_WALK_PTR(port) | UTMIP_CLR_WAKE_ALARM(port) |
> +		UTMIP_CAP_CFG(port));
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_UHSIC_TRIGGERS);
> +
> +	/* program electrical parameters read from XUSB PADCTL */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_TERM_PAD_CFG);
> +	value &= ~(TCTRL_VAL(~0) | PCTRL_VAL(~0));
> +	value |= (TCTRL_VAL(tctrl) | PCTRL_VAL(pctrl));
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_TERM_PAD_CFG);
> +
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_PAD_CFGX(port));
> +	value &= ~RPD_CTRL_PX(~0);
> +	value |= RPD_CTRL_PX(rpd_ctrl);
> +	padctl_pmc_writel(priv, value, PMC_UTMIP_PAD_CFGX(port));
> +
> +	/* setup the pull-ups and pull-downs of the signals during the four
> +	 * stages of sleepwalk.
> +	 * if device is connected, program sleepwalk logic to maintain a J and
> +	 * keep driving K upon seeing remote wake.
> +	 */
> +	value = padctl_pmc_readl(priv, PMC_UTMIP_SLEEPWALK_PX(port));
> +	value = (UTMIP_USBOP_RPD_A | UTMIP_USBOP_RPD_B | UTMIP_USBOP_RPD_C |
> +		UTMIP_USBOP_RPD_D);
> +	value |= (UTMIP_USBON_RPD_A | UTMIP_USBON_RPD_B | UTMIP_USBON_RPD_C |
> +		UTMIP_USBON_RPD_D);
> +	if (speed == USB_SPEED_UNKNOWN) {
> +		value |= (UTMIP_HIGHZ_A | UTMIP_HIGHZ_B | UTMIP_HIGHZ_C |
> +			UTMIP_HIGHZ_D);
> +	} else if ((speed == USB_SPEED_HIGH) || (speed == USB_SPEED_FULL)) {
> +		/* J state: D+/D- = high/low, K state: D+/D- = low/high */
> +		value |= UTMIP_HIGHZ_A;
> +		value |= UTMIP_AP_A;
> +		value |= (UTMIP_AN_B | UTMIP_AN_C | UTMIP_AN_D);
> +	} else if (speed == USB_SPEED_LOW) {
> +		/* J state: D+/D- = low/high, K state: D+/D- = high/low */
> +		value |= UTMIP_HIGHZ_A;
> +		value |= UTMIP_AN_A;
> +		value |= (UTMIP_AP_B | UTMIP_AP_C | UTMIP_AP_D);
> +	}

no else? err case? Also this could use a switch too

> +static int tegra210_pmc_utmi_disable_phy_sleepwalk(struct tegra_xusb_lane *lane)
> +{
> +	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> +	struct tegra210_xusb_padctl *priv = to_tegra210_xusb_padctl(padctl);
> +	struct device *dev = padctl->dev;
> +	unsigned int port = lane->index;
> +	u32 value;
> +
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;

That should be an error like EIO as we always expect regmap to be set,
no an unsupported error right?

-- 
~Vinod