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

[Pawel Laszczak]

>
>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?
>

Setting usb_conf.CFGRST doesn't reset the  PUSB_PWR_FST_REG_ACCESS.

Regards,
Pawel

>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