| Message ID | 20220205164535.179231-7-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb/dwc3 / phy/tusb1210: Add TUSB1211 charger detection | expand |
+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? 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.
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 --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, ®); 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, ®); + 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(®, 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;
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(-)