Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> John Stultz wrote:
>>>  static void __dwc3_set_mode(struct work_struct *work)
>>>  {
>>>  	struct dwc3 *dwc = work_to_dwc(work);
>>>  	unsigned long flags;
>>> +	int hw_mode;
>>>  	int ret;
>>>  	u32 reg;
>>>  
>>> @@ -154,6 +168,11 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>  		break;
>>>  	}
>>>  
>>> +	/* Execute a GCTL Core Soft Reset when switch mode in DRD*/
>>> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
>>> +		dwc3_gctl_core_soft_reset(dwc);
>>> +
>> I think this should be done inside the spin_lock.
>>
>>>  	spin_lock_irqsave(&dwc->lock, flags);
>>>  
>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>> The DRD mode change sequence should be like this if we want to switch
>> 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
>>
>> However, from code review, with this patch, it follows this sequence:
>> a. Soft reset with DCTL.CSftRst on driver probe
>> b. Reset controller with GCTL.CoreSoftReset
>> c. Set GCTL.PrtCapDir(device)
>> d. < missing DCTL.CSftRst >
>> e. Then follow up with initializing registers sequence
>>
>> It may work, but it doesn't follow the programming guide.
>>
>> For device -> host, it should be fine because the xHCI driver will do
>> USBCMD.HCRST during initialization.
> The only reason why this is needed is because SNPS saves some die area
> by mapping some host and peripheral register to the same physical area
> in the die. I still think a full soft reset is unnecessary as we have
> been running this driver without that soft reset for several years now.
>

This isn't about whether to use GCTL or DCTL for Core Soft Reset (Please
correct me if I'm wrong because I think this is what you're referring
to). It's about the programming flow when switching modes.

Both step 1 and 3 are required. Because before step 1, if the host was
in normal working mode (e.g. attached to a device), then changing the
PrtCapDir directly to device mode is not advisable and HW may exhibit
unknown behavior. The proper way is to have step 1 through 4 in
sequence. Step 3 is required because some of the HW functionality is
dependent on the PrtCapDir setting before the driver intervention is
required for proper communication with the device. The HW may be in some
intermediate state while changing the PrtCapDir. So, it is required to
reset it to have a fresh start in device mode.

Though we may not see issues even if we eliminate the steps 1 and 3, it
is not advisable and we may have some impact under certain conditions.

One change needs to made for this patch is the driver needs to wait a
certain amount of time before clearing the GCTL.CoreSoftReset for the
PHY clock to start and stabilize.

BR,
Thinh