diff mbox series

[6/9] phy: ti: tusb1210: Improve ulpi_read()/_write() error checking

Message ID 20220205164535.179231-7-hdegoede@redhat.com
State New
Headers show
Series usb/dwc3 / phy/tusb1210: Add TUSB1211 charger detection | expand

Commit Message

Hans de Goede Feb. 5, 2022, 4:45 p.m. UTC 
ulpi_read() and ulpi_write() calls can fail. Add wrapper functions to log
errors when this happens and add error checking to the read + write of
the phy parameters from the TUSB1210_VENDOR_SPECIFIC2 register.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/phy/ti/phy-tusb1210.c | 62 +++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 14 deletions(-)

Comments

Hans de Goede June 14, 2022, 11:22 a.m. UTC | #1 
Hi,

On 6/13/22 17:48, Andy Shevchenko wrote:
> +Cc: Ferry
> 
> On Sat, Feb 05, 2022 at 05:45:32PM +0100, Hans de Goede wrote:
>> ulpi_read() and ulpi_write() calls can fail. Add wrapper functions to log
>> errors when this happens and add error checking to the read + write of
>> the phy parameters from the TUSB1210_VENDOR_SPECIFIC2 register.`
> 
> 
> This patch seems to break Intel Merrifield platform.
> 
> Before:
> 
> [   36.333644] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
> [   36.339828] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1
> [   36.352566] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220f06c hci version 0x100 quirks 0x0000000002010010
> [   36.367062] xhci-hcd xhci-hcd.1.auto: irq 16, io mem 0xf9100000
> [   36.378429] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
> [   36.384705] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 2
> 
> After:
> [   32.986776] tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
> [   32.994053] phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
> [   33.000601] dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
> [   33.007486] dwc3: probe of dwc3.0.auto failed with error -110
> 
> 
> Any ideas?

In my experience with using the phy for charger-type detection on some
x86 android tablets which don't have any other way to do charger detection,
these errors indicate a real communication issue for reading/writing
phy registers. At the same time this usually does not seem to be a big
problem since the phy seems to work fine with its power-on defaults.

In case of Bay Trail these errors were related to 2 things:

1. Autosuspend of the phy-interface block in the dwc3, fixed by:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7c93a903f33ff35aa0e6b5a8032eb9755b00826

But dwc3_pci_mrfld_properties[] already sets "snps,dis_u2_susphy_quirk",
so I guess it is not this.

2. There being no delay in tusb1210_power_on() between toggling the
reset IO and then trying to communicate with the phy, fixed in:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df37c99815d9e0775e67276d70c93cbc25f31c70

Maybe the:

#define TUSB1210_RESET_TIME_MS				30

Added by that commit needs to be a bit bigger for the possibly
older phy revision used on the merifield boards?

(note it is fine to just increase it a bit everywhere).

Regards,

Hans




 
> 
> P.S> There is no bisect log, since it's done manually with a good guess by
> Ferry. I have just reverted patches on ULPI from this series and start applying
> them one-by-one.
>
diff mbox series

Patch

diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
index 15c1c79e5c29..bf7793afdc84 100644
--- a/drivers/phy/ti/phy-tusb1210.c
+++ b/drivers/phy/ti/phy-tusb1210.c
@@ -26,6 +26,33 @@  struct tusb1210 {
 	u8 vendor_specific2;
 };
 
+static int tusb1210_ulpi_write(struct tusb1210 *tusb, u8 reg, u8 val)
+{
+	int ret;
+
+	ret = ulpi_write(tusb->ulpi, reg, val);
+	if (ret)
+		dev_err(&tusb->ulpi->dev, "error %d writing val 0x%02x to reg 0x%02x\n",
+			ret, val, reg);
+
+	return ret;
+}
+
+static int tusb1210_ulpi_read(struct tusb1210 *tusb, u8 reg, u8 *val)
+{
+	int ret;
+
+	ret = ulpi_read(tusb->ulpi, reg);
+	if (ret >= 0) {
+		*val = ret;
+		ret = 0;
+	} else {
+		dev_err(&tusb->ulpi->dev, "error %d reading reg 0x%02x\n", ret, reg);
+	}
+
+	return ret;
+}
+
 static int tusb1210_power_on(struct phy *phy)
 {
 	struct tusb1210 *tusb = phy_get_drvdata(phy);
@@ -35,8 +62,8 @@  static int tusb1210_power_on(struct phy *phy)
 
 	/* Restore the optional eye diagram optimization value */
 	if (tusb->vendor_specific2)
-		ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
-			   tusb->vendor_specific2);
+		return tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2,
+					   tusb->vendor_specific2);
 
 	return 0;
 }
@@ -55,33 +82,34 @@  static int tusb1210_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 {
 	struct tusb1210 *tusb = phy_get_drvdata(phy);
 	int ret;
+	u8 reg;
 
-	ret = ulpi_read(tusb->ulpi, ULPI_OTG_CTRL);
+	ret = tusb1210_ulpi_read(tusb, ULPI_OTG_CTRL, &reg);
 	if (ret < 0)
 		return ret;
 
 	switch (mode) {
 	case PHY_MODE_USB_HOST:
-		ret |= (ULPI_OTG_CTRL_DRVVBUS_EXT
+		reg |= (ULPI_OTG_CTRL_DRVVBUS_EXT
 			| ULPI_OTG_CTRL_ID_PULLUP
 			| ULPI_OTG_CTRL_DP_PULLDOWN
 			| ULPI_OTG_CTRL_DM_PULLDOWN);
-		ulpi_write(tusb->ulpi, ULPI_OTG_CTRL, ret);
-		ret |= ULPI_OTG_CTRL_DRVVBUS;
+		tusb1210_ulpi_write(tusb, ULPI_OTG_CTRL, reg);
+		reg |= ULPI_OTG_CTRL_DRVVBUS;
 		break;
 	case PHY_MODE_USB_DEVICE:
-		ret &= ~(ULPI_OTG_CTRL_DRVVBUS
+		reg &= ~(ULPI_OTG_CTRL_DRVVBUS
 			 | ULPI_OTG_CTRL_DP_PULLDOWN
 			 | ULPI_OTG_CTRL_DM_PULLDOWN);
-		ulpi_write(tusb->ulpi, ULPI_OTG_CTRL, ret);
-		ret &= ~ULPI_OTG_CTRL_DRVVBUS_EXT;
+		tusb1210_ulpi_write(tusb, ULPI_OTG_CTRL, reg);
+		reg &= ~ULPI_OTG_CTRL_DRVVBUS_EXT;
 		break;
 	default:
 		/* nothing */
 		return 0;
 	}
 
-	return ulpi_write(tusb->ulpi, ULPI_OTG_CTRL, ret);
+	return tusb1210_ulpi_write(tusb, ULPI_OTG_CTRL, reg);
 }
 
 static const struct phy_ops phy_ops = {
@@ -95,11 +123,14 @@  static int tusb1210_probe(struct ulpi *ulpi)
 {
 	struct tusb1210 *tusb;
 	u8 val, reg;
+	int ret;
 
 	tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
 	if (!tusb)
 		return -ENOMEM;
 
+	tusb->ulpi = ulpi;
+
 	tusb->gpio_reset = devm_gpiod_get_optional(&ulpi->dev, "reset",
 						   GPIOD_OUT_LOW);
 	if (IS_ERR(tusb->gpio_reset))
@@ -119,7 +150,9 @@  static int tusb1210_probe(struct ulpi *ulpi)
 	 * diagram optimization and DP/DM swap.
 	 */
 
-	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
+	ret = tusb1210_ulpi_read(tusb, TUSB1210_VENDOR_SPECIFIC2, &reg);
+	if (ret)
+		return ret;
 
 	/* High speed output drive strength configuration */
 	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
@@ -133,15 +166,16 @@  static int tusb1210_probe(struct ulpi *ulpi)
 	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val))
 		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
 
-	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
+	ret = tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2, reg);
+	if (ret)
+		return ret;
+
 	tusb->vendor_specific2 = reg;
 
 	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
 	if (IS_ERR(tusb->phy))
 		return PTR_ERR(tusb->phy);
 
-	tusb->ulpi = ulpi;
-
 	phy_set_drvdata(tusb->phy, tusb);
 	ulpi_set_drvdata(ulpi, tusb);
 	return 0;