Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
