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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8934 bytes --]

On Thu, Apr 01, 2021 at 12:15:08PM +0530, Vinod Koul wrote:
> 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)

These are purely for debugging purposes. The idea being that they can be
quickly enabled by defining DEBUG. Otherwise if something ever goes
wrong in this code somebody will have to add this exact same code again
just to troubleshoot.

But if you insist I can remove this and similar other debug messages.

> > +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

I actually tried this once a while back, but the resulting code didn't
look much better, in my opinion. In this particular case it might
actually work because what we modify is simple, but consider the case
of some of the other expressions where individual arguments to the
functions will become really long and then the code ends up very
unreadable.

One alternative that I had considered was to have a mix and use the
read/modify/write helper only where it wouldn't hurt readability, but
that is perhaps half of the time and it would make register accesses
completely inconsistent.

I agree that this looks like a bit of boilerplate, but on the other hand
it's consistent and it's also very trivial to follow.

> > +	/* 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

Agreed, I've switched this to 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
>  */

Done.

> > +	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

There's no else or error case here because all other values have already
been rejected by the "speed > USB_SPEED_HIGH" check earlier in this
function. I agree that this looks better as a switch, though, so I've
changed to that.

> > +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?

This is for backwards-compatibility with older device trees. Device
trees prior to this patch series don't have a reference to that regmap,
but the code should keep working with such device trees, otherwise USB
will be broken on newer kernel versions if you don't update the device
tree.

"unsupported" here means that this particular feature isn't supported
because the regmap is missing, so we can't reprogram the registers
needed to make this work.

FWIW, the ->probe() function will print a warning if the regmap is
absent, so in combination with this -EOPNOTSUPP that is hopefully going
to make this clearer to users.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy