Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Op 10-11-2022 om 13:45 schreef Ferry Toth:
(sorry sent html with previous attempt)

On 10-11-2022 01:06, Thinh Nguyen wrote:
Hi Ferry,

On Wed, Nov 09, 2022, Ferry Toth wrote:
Since commit 0f010171
Dual Role support on Intel Merrifield platform broke due to rearranging
the call to dwc3_get_extcon().

It appears to be caused by ulpi_read_id() on the first test write failing with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via DT when the test write fails and returns 0 in that case even if DT does not
provide the phy. Due to the timeout being masked dwc3 probe continues by
calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happens to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeds.

This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs and
catches the error in dwc3_core_init(). It handles the error by calling
dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On deferred
probe ulpi_read_id() again succeeds.

Signed-off-by: Ferry Toth<ftoth@xxxxxxxxxxxxxx>
---
  drivers/usb/common/ulpi.c | 5 +++--
  drivers/usb/dwc3/core.c   | 5 ++++-
  2 files changed, 7 insertions(+), 3 deletions(-)

Can you split the dwc3 change and ulpi change to separate patches?

Thanks for your comments.

I will send v2

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index d7c8461976ce..d8f22bc2f9d0 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
      /* Test the interface */
      ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
-    if (ret < 0)
-        goto err;
+    if (ret < 0) {
+        return ret;
+    }
      ret = ulpi_read(ulpi, ULPI_SCRATCH);
      if (ret < 0)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 648f1c570021..e293ef70039b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
      if (!dwc->ulpi_ready) {
          ret = dwc3_core_ulpi_init(dwc);
-        if (ret)
+        if (ret) {
+            dwc3_core_soft_reset(dwc);
We shouldn't need to do soft reset here. The controller shouldn't be at
a bad/incorrect state at this point to warrant a soft-reset. There will
be a soft-reset when it goes through the initialization again.

It doesn't go through the initialization again unless we set -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it will goto err0 here, so skips dwc3_core_soft_reset.

Do you mean you prefer something like:

if (ret) {

     if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;

     else goto err0;

}

I just tested, and calling dwc3_core_soft_reset() proves to be necessary as we need to goto err0 directly after. Else ret is overwritten and -EPROBE_DEFER lost.

+            ret = -EPROBE_DEFER;
We shouldn't automatically set every error status to correspond to
-EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
any other?).
Other could be -ENOMEM. I think no need to do any new handling for that.
              goto err0;
+        }
          dwc->ulpi_ready = true;
      }
--
2.34.1

Thanks,
Thinh



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux