Messages in this thread | | | Date | Fri, 18 Nov 2022 23:29:29 +0100 | Subject | Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout | From | Ferry Toth <> |
| |
Hi,
Op 11-11-2022 om 02:31 schreef Thinh Nguyen: > On Thu, Nov 10, 2022, Ferry Toth wrote: >> 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@exalondelft.nl> >>>>> --- >>>>> 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; > > Why "else"? But I saw you remove that in the new patch. > >>> >>> } >> >> 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. > > Looks like there's a strange dependency problem here. > * The setup needs a soft-reset before ulpi registration > * The ulpi registration needs to go before the phy initialization > * The soft-reset should be called after the phy initialization > > I can't explain the actual issue here, and we can't debug further > because to look into it further would require looking at internal > signals. > > This soft-reset and -EPROBE_DEFER seems to be a workaround to this > dependency problem. Instead of using -EPROBE_DEFER, can you do this: > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 2f0a9679686f..5a1aaf3741ec 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1097,6 +1097,8 @@ static int dwc3_core_init(struct dwc3 *dwc) > goto err0; > > if (!dwc->ulpi_ready) { > + /* Add comment */ > + dwc3_core_soft_reset(dwc); > ret = dwc3_core_ulpi_init(dwc); > if (ret) > goto err0; >
This indeed fixes the issue as well. Here is the trace:
# tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 0) | /* start_event: (dwc3_probe+0x0/0x1910) */ 0) 7.070 us | dwc3_clk_enable.part.0(); 0) 5.480 us | extcon_get_extcon_dev(); 0) + 10.230 us | dwc3_runtime_idle(); 0) | /* end_event: (platform_probe+0x3f/0xa0 <- dwc3_probe) */
** multiple defers while waiting for extcon
0) | /* start_event: (dwc3_probe+0x0/0x1910) */ 0) 7.320 us | dwc3_clk_enable.part.0(); 0) 6.830 us | extcon_get_extcon_dev(); 0) | dwc3_core_init() { 0) + 29.200 us | dwc3_core_soft_reset.part.0(); 0) | dwc3_ulpi_init() { 0) | ulpi_register_interface() { 0) | dwc3_ulpi_write() { 0) 3.380 us | dwc3_ulpi_busyloop();
** without this patch this one times out after 10000us
0) 7.710 us | } 0) | dwc3_ulpi_read() { 0) 3.060 us | dwc3_ulpi_busyloop(); 0) 7.210 us | } 0) | dwc3_ulpi_read() { 0) 2.830 us | dwc3_ulpi_busyloop(); 0) 6.690 us | } 0) | dwc3_ulpi_read() { 0) 2.880 us | dwc3_ulpi_busyloop(); 0) 6.670 us | } 0) | dwc3_ulpi_read() { 0) 2.940 us | dwc3_ulpi_busyloop(); 0) 6.690 us | } 0) | dwc3_ulpi_read() { 0) 2.870 us | dwc3_ulpi_busyloop(); 0) 6.620 us | } 0) + 18.150 us | ulpi_uevent(); 0) 5.990 us | ulpi_match(); 0) | ulpi_probe() { 0) | tusb1210_probe() { 0) | ulpi_read() { 0) | dwc3_ulpi_read() { 0) 4.440 us | dwc3_ulpi_busyloop(); 0) 9.600 us | } 0) + 15.770 us | } 0) | ulpi_write() { 0) | dwc3_ulpi_write() { 0) 3.270 us | dwc3_ulpi_busyloop(); 0) 6.820 us | } 0) + 11.020 us | } 0) ! 407.540 us | } 0) ! 416.980 us | } 0) 9.800 us | ulpi_uevent(); 0) * 18604.00 us | } 0) * 18611.20 us | } 0) + 30.570 us | dwc3_core_soft_reset.part.0(); 0) | tusb1210_power_on() { 1) | extcon_set_state_sync() { 1) 5.330 us | extcon_set_state.part.0(); 1) + 90.550 us | extcon_sync.part.0(); 1) ! 113.670 us | } 1) + 19.450 us | ulpi_uevent(); 0) + 13.640 us | ulpi_uevent(); 1) + 13.980 us | ulpi_uevent(); 0) + 15.960 us | ulpi_uevent(); 0) | ulpi_write() { 0) | dwc3_ulpi_write() { 0) * 10239.47 us | dwc3_ulpi_busyloop(); 0) * 10250.57 us | } 0) * 10265.09 us | } 0) * 69518.95 us | } 0) 5.740 us | dwc3_event_buffers_setup(); 0) * 88241.02 us | } /* dwc3_core_init */ 0) ! 104.900 us | dwc3_debugfs_init(); 0) | dwc3_drd_init() { 0) 4.720 us | extcon_register_notifier(); 0) | extcon_get_state() { 0) 2.640 us | extcon_get_state.part.0(); 0) 6.460 us | } 0) + 14.460 us | dwc3_set_mode(); 0) + 43.300 us | } 0) | /* end_event: (platform_probe+0x3f/0xa0 <- dwc3_probe) */
Maybe this is the preferred way to go if the dwc3_core_soft_reset() doesn't hurt other users?
>> >>>>> + 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. > > There's also -EINVAL and some others too. Regardless, if it's -ENOMEM, > then it should not return -EPROBE_DEFER. > >>>>> goto err0; >>>>> + } >>>>> dwc->ulpi_ready = true; >>>>> } >>>>> > > Thanks, > Thinh
| |