Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: wcheng=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org F3186C433CA
Subject: Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following
 programming guide
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>
Cc:     Felipe Balbi <balbi@kernel.org>,
        Tejas Joglekar <Tejas.Joglekar@synopsys.com>,
        Yang Fei <fei.yang@intel.com>,
        YongQin Liu <yongqin.liu@linaro.org>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        Jun Li <lijun.kernel@gmail.com>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
References: <20210108015115.27920-1-john.stultz@linaro.org>
 <20210108015115.27920-2-john.stultz@linaro.org>
 <eb4b8540-a57c-53cc-a371-cf68178bec15@codeaurora.org>
 <e696b018-b310-5811-5c80-3c50dde297e7@synopsys.com>
From:   Wesley Cheng <wcheng@codeaurora.org>
Message-ID: <b08386c0-503d-a333-46be-9df77122ec4e@codeaurora.org>
Date:   Mon, 29 Mar 2021 15:17:54 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <e696b018-b310-5811-5c80-3c50dde297e7@synopsys.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/6/2021 3:39 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>> On 1/7/2021 5:51 PM, John Stultz wrote:
>>> In reviewing the previous patch, Thinh Nguyen pointed out that
>>> the DRD mode change sequence should be like the following when
>>> switching from host -> device according to the programming guide
>>> (for all DRD IPs):
>>> 1. Reset controller with GCTL.CoreSoftReset
>>> 2. Set GCTL.PrtCapDir(device)
>>> 3. Soft reset with DCTL.CSftRst
>>> 4. Then follow up with the initializing registers sequence
>>>
>>> The current code does:
>>> a. Soft reset with DCTL.CSftRst on driver probe
>>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>>    patch)
>>> c. Set GCTL.PrtCapDir(device)
>>> d. < missing DCTL.CSftRst >
>>> e. Then follow up with initializing registers sequence
>>>
>>> So this patch adds the DCTL.CSftRst soft reset that was currently
>>> missing from the dwc3 mode switching.
>>>
>>> Cc: Felipe Balbi <balbi@kernel.org>
>>> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
>>> Cc: Yang Fei <fei.yang@intel.com>
>>> Cc: YongQin Liu <yongqin.liu@linaro.org>
>>> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>> Cc: Thinh Nguyen <thinhn@synopsys.com>
>>> Cc: Jun Li <lijun.kernel@gmail.com>
>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>> Feedback would be appreciated. I'm a little worried I should be
>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>>> I'm really not sure what the right thing to do is for non-DRD
>>> mode controllers.
>>> ---
>>>  drivers/usb/dwc3/core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index b6a6b90eb2d5..71f8b07ecb99 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -40,6 +40,8 @@
>>>  
>>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>>  
>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>>> +
>>>  /**
>>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>>   * @dwc: pointer to our context structure
>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>  
>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>>  
>>> +	dwc3_core_soft_reset(dwc);
>> Hi John/Thinh/Felipe,
>>
>> I actually added this change into my local branch, because we were
>> seeing an issue when switching from host mode --> peripheral mode.  What
>> was happening was that the RXFIFO register did not update back to the
>> expected value for peripheral mode by the time
>> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
>> calculate the EP max packet limit based on RXFIFO reg, this caused all
>> EPs to be set with an EP max limit of 0.
>>
>> With this change, it seemed to help with the above issue.  However, can
>> we consider moving the core soft reset outside the spinlock?  At least
>> with our PHY init routines, we have some msleep() calls for waiting for
>> the PHYs to be ready, which will end up as a sleeping while atomic bug.
>> (not sure if PHY init is required to be called in atomic context)
>>
>> Thanks
>> Wesley Cheng
> 
> Hi Wesley,
> 
> Thanks for letting us know the issue you're having also.
> 
> Yes, you need to wait a certain amount of time to synchronize with the
> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
> less for older versions). When removing the spinlock to use msleep(),
> just make sure that there's no race issue. BTW, how long does your setup
> need to msleep()?
> 
Hi Thinh,

Sorry for the late response.  My mistake, its actually just a usleep()
for a less than 100uS (polling for a status bit change, so it will exit
early if possible).  For this change, can we just move the
dwc3_core_soft_reset() outside of the spinlock?

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project