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

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

[Ravi Gunasekaran]

From: Aswath Govindraju <a-govindraju@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@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);
+
 	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
-- 
2.17.1

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

[Roger Quadros]

Hi Ravi,

On 05/05/2023 15:13, Ravi Gunasekaran wrote:
> From: Aswath Govindraju <a-govindraju@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@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_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

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

[Ravi Gunasekaran]

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

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

-- 
Regards,
Ravi

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

[Roger Quadros]

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

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

RE: [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@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@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

Re: [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@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@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

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

[Roger Quadros]

On 05/05/2023 15:13, Ravi Gunasekaran wrote:
> From: Aswath Govindraju <a-govindraju@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@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

RE: [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@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@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