Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
From: Minas Harutyunyan
Date: Mon Oct 23 2017 - 05:20:50 EST
On 10/20/2017 3:57 PM, Minas Harutyunyan wrote:
> On 10/20/2017 12:20 AM, John Stultz wrote:
>> On Wed, Oct 18, 2017 at 11:46 PM, Minas Harutyunyan
>> <Minas.Harutyunyan@xxxxxxxxxxxx> wrote:
>>> Could you please apply this patch. Please not apply your patch series
>>> "[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch.
>>> If you confirm that this patch fix your issue with "Transaction Error"
>>> and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch.
>>> Then can be applied your patch series, without patch 1/3 (changing in
>>> connector id status change) to correctly set UDC states.
>>>
>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index
>>> f4ef159b538e..7da22152df68 100644
>>> --- a/drivers/usb/dwc2/hcd.c
>>> +++ b/drivers/usb/dwc2/hcd.c
>>> @@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>> usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
>>> usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
>>>
>>> + /* Set HS/FS Timeout Calibration */
>>> + usbcfg |= GUSBCFG_TOUTCAL(7);
>>> +
>>> switch (hsotg->hw_params.op_mode) {
>>> case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
>>> if (hsotg->params.otg_cap ==
>>
>>
>> So while using this patch and the earlier one from Vardan, I don't see
>> the "Transaction Error"
>> and " ChHltd set, but reason is unknown" messages, but I do see:
>>
>> [ 272.290459] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290476] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290491] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290507] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290522] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290538] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290554] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290570] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290585] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290687] dwc2 f72c0000.usb: dwc2_hsotg_ep_stop_xfr: timeout DIEPINT.NAKEFF
>> [ 272.290702] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>> [ 272.290717] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently
>> in Host mode
>>
>> After which the USB eth device stops functioning.
>>
>> So its not really much functional change from without the patch from
>> my perspective.
>>
>> Additionally, I'm still not seeing any disconnect calls when removing
>> the B plug.
>>
>> Re-adding my three patches ontop of this change and Vardan's does fix
>> both the UDC state handling and avoids usb devices from failing when
>> the board is in host mode and the /dev/usb-ffs/adb/ep0 file is closed
>> by adbd.
>>
>>
>> thanks
>> -john
>>
>
> Hi John Stultz,
>
> It's good news that you not see more "Transaction error" and "ChHlt set,
> but reason is unknown"!
>
> To avoid "Mode Mismatch" interrupt please apply your patch 2/3.
>
> Additionally I see that in your setup used UTMI+ 8-bit PHY but
> GUSBCFG.USBTRDTIM set to 5, but should be 9.
> In device mode in function dwc2_hsotg_core_init_disconnected() GUSBCFG
> related fields TOUTCAL and USBTRDTIM setting again, but in Host mode
> these fields are not set at all. This can be reason why using your patch
> 1/3 you overcome TOUTCAL and USBTRDTIM issue.
>
> So, suggesting another patch for USBTRDTIM field bellow:
>
> @@ -2311,10 +2314,17 @@ static int dwc2_core_init(struct dwc2_hsotg
> *hsotg, bool initial_setup)
> */
> static void dwc2_core_host_init(struct dwc2_hsotg *hsotg)
> {
> - u32 hcfg, hfir, otgctl;
> + u32 hcfg, hfir, otgctl, usbcfg, trdtrim;
>
> dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg);
>
> + /* Set USBTrdTim value */
> + usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
> + usbcfg &= ~GUSBCFG_USBTRDTIM_MASK;
> + trdtrim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5;
> + usbcfg |= (trdtrim << GUSBCFG_USBTRDTIM_SHIFT);
> + dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
> +
> /* Restart the Phy Clock */
> dwc2_writel(0, hsotg->regs + PCGCTL);
>
>
> Please apply follow patches:
> 1. Vardan's patch.
> 2. Patch for TOUTCAL.
> 3. Patch for USBTRDTIM (see above).
> 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts.
> 5. Your patch 3/3 to set udc state to "not attached".
>
> Unfortunately, we haven't hikey board in our lab and can't fully test
> patches on our side.
>
>
> Thanks,
> Minas
>
Hi John Stultz,
Could you please verify on your setup follow patches:
1. Vardan's patch.
2. Patch for TOUTCAL&USBTRDTIM programming (new version see below).
4. Your patch 2/3 to avoid "Mode Mismatch" interrupts.
5. Your patch 3/3 to set udc state to "not attached".
6. Your patch 1/3, but remove dwc2_hsotg_core_init_disconnected()
function call from Host starting brnch, keep *only*
dwc2_hsotg_disconnect() to change UDC state to "not attached".
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index f4ef159b538e..1f65464055b9 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2311,10 +2311,17 @@ static int dwc2_core_init(struct dwc2_hsotg
*hsotg, bool initial_setup)
*/
static void dwc2_core_host_init(struct dwc2_hsotg *hsotg)
{
- u32 hcfg, hfir, otgctl;
+ u32 hcfg, hfir, otgctl, usbcfg, val;
dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg);
+ /* Set HS/FS Timeout Calibration and USBTrdTim */
+ usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+ usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_USBTRDTIM_MASK);
+ val = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5;
+ usbcfg |= (GUSBCFG_TOUTCAL(7) | (val << GUSBCFG_USBTRDTIM_SHIFT));
+ dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
+
/* Restart the Phy Clock */
dwc2_writel(0, hsotg->regs + PCGCTL);
Thanks,
Minas