[EXTERNAL] Re: [PATCH] usb: cdns3: gadget.c: Set fast access bit

[Ravi Gunasekaran]

Pawel,

On 11/05/23 11:00 am, Pawel Laszczak wrote:
>>
>> +Pawel & Peter
>>
>> On 09/05/2023 08:58, Ravi Gunasekaran wrote:
>>> Hi Roger,
>>>
>>> On 05/05/23 6:02 pm, Roger Quadros wrote:
>>>> Hi Ravi,
>>>>
>>>> On 05/05/2023 15:13, Ravi Gunasekaran wrote:
>>>>> From: Aswath Govindraju <a-govindraju at ti.com>
>>>>>
>>>>> When the device port is in a low power state [U3/L2/Not Connected],
>>>>> accesses to usb device registers may take a long time. This could
>>>>> lead to potential core hang when the controller registers are
>>>>> accessed after the port is disabled by setting DEVDS field. Setting
>>>>> the fast register access bit ensures that the PHY clock is keeping up in
>> active state.
>>>>>
>>>>> Therefore, set fast access bit to ensure the accesses to device
>>>>> registers are quick even in low power states.
>>>>>
>>>>> Signed-off-by: Aswath Govindraju <a-govindraju at ti.com>
>>>>> ---
>>>>>  drivers/usb/cdns3/gadget.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>> index fcaeab9cc1..fddc8c931a 100644
>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>> @@ -2321,6 +2321,9 @@ static void cdns3_gadget_config(struct
>> cdns3_device *priv_dev)
>>>>>  	writel(USB_IEN_INIT, &regs->usb_ien);
>>>>>  	writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, &regs->usb_conf);
>>>>>
>>>>> +	/* Set the Fast access bit */
>>>>> +	writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr);
>>>>> +
>>>>
>>>> Should this be done in cdns3_gadget_udc_start() so it is symmetric?
>>>>
>>>
>>> cdns3_gadget_config() is called in cdns3_gadget_udc_start() and
>>> cdns3_gadget_resume(). These settings seems to be needed during resume
>>> as well.
>>
>> But this bit was never cleared in suspend so why do you need to set it again it
>> in resume?
>>
> 
> In my opinion setting PUSB_PWR_FST_REG_ACCESS is not needed in
> cdns3_gadget_resume, but it should not have any negative impact for 
> driver. The cdns3_gadget_config function seems like a good place for
> setting this bit. 
> 

In the suspend(), USB_CONF_CFGRST (register: usb_conf.CFGRST) is set,
resetting the USB device configuration. Does this mean
PUSB_PWR_FST_REG_ACCESS will also be reset?

Regards,
Ravi

> Regards,
> Pawel Laszczak
> 
>> The commit log says that this bit must be kept set in low power states.
>>
>>>
>>>>>  	cdns3_configure_dmult(priv_dev, NULL);
>>>>>
>>>>>  	cdns3_gadget_pullup(&priv_dev->gadget, 1); @@ -2378,6 +2381,7
>> @@
>>>>> static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
>>>>>
>>>>>  	/* disable interrupt for device */
>>>>>  	writel(0, &priv_dev->regs->usb_ien);
>>>>> +	writel(0, &priv_dev->regs->usb_pwr);
>>>>>  	writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf);
>>>>>
>>>>>  	return ret;
>>>>>
>>>>> base-commit: a25dcda452bf6a6de72764a8d990d72e5def643d
>>>>
>>
>> --
>> cheers,
>> -roger