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

[Pawel Laszczak]

>
>+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. 

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