Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c8ce4a22-7394-5d20-1a8c-627c5cb666e7@quicinc.com>
Date:   Thu, 14 Jul 2022 16:44:33 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET
 during bootup
Content-Language: en-CA
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>
CC:     "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20220712174423.20484-1-quic_rkollals@quicinc.com>
 <889f7a73-d8d9-2e40-3386-e8f3f30cad24@synopsys.com>
From:   Rohith Kollalsi <quic_rkollals@quicinc.com>
In-Reply-To: <889f7a73-d8d9-2e40-3386-e8f3f30cad24@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/14/2022 6:36 AM, Thinh Nguyen wrote:
> On 7/12/2022, Rohith Kollalsi wrote:
>> According to the programming guide, it is recommended to
>> perform a GCTL_CORE_SOFTRESET only when switching the mode
>> from device to host or host to device. However, it is found
>> that during bootup when __dwc3_set_mode() is called for the
>> first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
>> of DWC3_GUSB3PIPECTL set. This some times leads to issues
>> like controller going into bad state and controller registers
>> reading value zero. Until GCTL_CORESOFTRESET is done and
>> run/stop bit is set core initialization is not complete.
>> Setting suspendable bit of DWC3_GUSB3PIPECTL and then
>> performing GCTL_CORESOFTRESET is therefore not recommended.
>> Avoid this by only performing the reset if current_dr_role is set,
>> that is, when doing subsequent role switching.
>>
>> Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
>> Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com>
>> ---
>>    drivers/usb/dwc3/core.c | 10 +++++++---
>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 050b2ba5986d..7ce0e033d755 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>>    		break;
>>    	}
>>    
>> -	/* For DRD host or device mode only */
>> -	if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
>> -	    dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
>> +	/*
>> +	 * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.
> 
> The line above doesn't really explain why. Can you rephrase it to
> something like this or the equivalent:
> "When current_dr_role is not set, there's no role switching. Only
> perform GCTL.CoreSoftReset when there's DRD role switching."
> 
> Thanks,
> Thinh

Thanks for the review Thinh, I have updated the next version of the 
patch with this suggestion.

Thanks,
Rohith

> 
>> +	 * Perform it only for DRD host or device mode.
>> +	 */
>> +	if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
>> +			DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
>> +			dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
>>    		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>>    		reg |= DWC3_GCTL_CORESOFTRESET;
>>    		dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>