[PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[Bruce Chen]

From: Bruce Chen <bruce.chen@unisoc.com>

If disable vbus/id irq, it will lead to wakeup system fail
in unisoc platform. In unisoc platform, Irq enable and irq
wakeup are the same interrupt line. So remove disable vbus/id
irq operation is a way to solve the issue.

Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>
---
 drivers/extcon/extcon-usb-gpio.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index f2b65d9..40d967a 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
 		}
 	}
 
-	/*
-	 * We don't want to process any IRQs after this point
-	 * as GPIOs used behind I2C subsystem might not be
-	 * accessible until resume completes. So disable IRQ.
-	 */
-	if (info->id_gpiod)
-		disable_irq(info->id_irq);
-	if (info->vbus_gpiod)
-		disable_irq(info->vbus_irq);
-
 	if (!device_may_wakeup(dev))
 		pinctrl_pm_select_sleep_state(dev);
 
@@ -267,11 +257,6 @@ static int usb_extcon_resume(struct device *dev)
 		}
 	}
 
-	if (info->id_gpiod)
-		enable_irq(info->id_irq);
-	if (info->vbus_gpiod)
-		enable_irq(info->vbus_irq);
-
 	queue_delayed_work(system_power_efficient_wq,
 			   &info->wq_detcable, 0);
 
-- 
1.9.1

Re: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[Roger Quadros]

+TI folks Aswath & Vignesh.

Hi Bruce,

On 03/03/2022 09:36, Bruce Chen wrote:
> From: Bruce Chen <bruce.chen@unisoc.com>
> 
> If disable vbus/id irq, it will lead to wakeup system fail
> in unisoc platform. In unisoc platform, Irq enable and irq
> wakeup are the same interrupt line. So remove disable vbus/id
> irq operation is a way to solve the issue> 
> Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>
> ---
>  drivers/extcon/extcon-usb-gpio.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index f2b65d9..40d967a 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
>  		}
>  	}
>  
> -	/*
> -	 * We don't want to process any IRQs after this point
> -	 * as GPIOs used behind I2C subsystem might not be
> -	 * accessible until resume completes. So disable IRQ.
> -	 */

I think the reason this was added was that if USB cable is plugged/unplugged
while system was suspending it was causing some faults on TI DRA7 EVM
at usb_irq_handler()/usb_extcon_detect_cable() when it tries to read GPIO status
when I2C based GPIO controller was used but already suspended.
But looks like disabling the IRQ is not the right approach either so your patch
is right.

However I'm not sure if the above issue will still exist on DRA7 and other TI EVMS
using ID/VBUS GPIO over I2C device.

Let's wait for Aswath/Vignesh to confirm. If no issues I'm OK with this patch.

> -	if (info->id_gpiod)
> -		disable_irq(info->id_irq);
> -	if (info->vbus_gpiod)
> -		disable_irq(info->vbus_irq);
> -
>  	if (!device_may_wakeup(dev))
>  		pinctrl_pm_select_sleep_state(dev);
>  
> @@ -267,11 +257,6 @@ static int usb_extcon_resume(struct device *dev)
>  		}
>  	}
>  
> -	if (info->id_gpiod)
> -		enable_irq(info->id_irq);
> -	if (info->vbus_gpiod)
> -		enable_irq(info->vbus_irq);
> -
>  	queue_delayed_work(system_power_efficient_wq,
>  			   &info->wq_detcable, 0);
>  

cheers,
-roger

答复: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[陈建 (Bruce Chen)]

+orson

Hi Aswath & Vignesh:
Did you help confirm what Roger said? Thanks.

-----邮件原件-----
发件人: Roger Quadros <rogerq@kernel.org>
发送时间: 2022年3月3日 17:21
收件人: Bruce Chen <brucechen251@gmail.com>; 陈建 (Bruce Chen) <bruce.chen@unisoc.com>; myungjoo.ham@samsung.com; cw00.choi@samsung.com; linux-kernel@vger.kernel.org; Aswath Govindraju <a-govindraju@ti.com>; Vignesh Raghavendra <vigneshr@ti.com>
抄送: orsonzhai@gmail.com; gengcixi@gmail.com; baolin.wang7@gmail.com; zhang.lyra@gmail.com
主题: Re: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.


注意: 这封邮件来自于外部。除非你确定邮件内容安全，否则不要点击任何链接和附件。
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.



+TI folks Aswath & Vignesh.

Hi Bruce,

On 03/03/2022 09:36, Bruce Chen wrote:
> From: Bruce Chen <bruce.chen@unisoc.com>
>
> If disable vbus/id irq, it will lead to wakeup system fail in unisoc
> platform. In unisoc platform, Irq enable and irq wakeup are the same
> interrupt line. So remove disable vbus/id irq operation is a way to
> solve the issue>
> Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>
> ---
>  drivers/extcon/extcon-usb-gpio.c | 15 ---------------
>  1 file changed, 15 deletions(-)
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c
> b/drivers/extcon/extcon-usb-gpio.c
> index f2b65d9..40d967a 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
>               }
>       }
>
> -     /*
> -      * We don't want to process any IRQs after this point
> -      * as GPIOs used behind I2C subsystem might not be
> -      * accessible until resume completes. So disable IRQ.
> -      */

I think the reason this was added was that if USB cable is plugged/unplugged while system was suspending it was causing some faults on TI DRA7 EVM at usb_irq_handler()/usb_extcon_detect_cable() when it tries to read GPIO status when I2C based GPIO controller was used but already suspended.
But looks like disabling the IRQ is not the right approach either so your patch is right.

However I'm not sure if the above issue will still exist on DRA7 and other TI EVMS using ID/VBUS GPIO over I2C device.

Let's wait for Aswath/Vignesh to confirm. If no issues I'm OK with this patch.

> -     if (info->id_gpiod)
> -             disable_irq(info->id_irq);
> -     if (info->vbus_gpiod)
> -             disable_irq(info->vbus_irq);
> -
>       if (!device_may_wakeup(dev))
>               pinctrl_pm_select_sleep_state(dev);
>
> @@ -267,11 +257,6 @@ static int usb_extcon_resume(struct device *dev)
>               }
>       }
>
> -     if (info->id_gpiod)
> -             enable_irq(info->id_irq);
> -     if (info->vbus_gpiod)
> -             enable_irq(info->vbus_irq);
> -
>       queue_delayed_work(system_power_efficient_wq,
>                          &info->wq_detcable, 0);
>

cheers,
-roger
________________________________
 This email (including its attachments) is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. Unauthorized use, dissemination, distribution or copying of this email or the information herein or taking any action in reliance on the contents of this email or the information herein, by anyone other than the intended recipient, or an employee or agent responsible for delivering the message to the intended recipient, is strictly prohibited. If you are not the intended recipient, please do not read, copy, use or disclose any part of this e-mail to others. Please notify the sender immediately and permanently delete this e-mail and any attachments if you received it in error. Internet communications cannot be guaranteed to be timely, secure, error-free or virus-free. The sender does not accept liability for any errors or omissions.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。

Re: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[Vignesh Raghavendra]

Hi,

On 03/03/22 2:50 pm, Roger Quadros wrote:
> +TI folks Aswath & Vignesh.
> 

Thanks for looping in.

> Hi Bruce,
> 
> On 03/03/2022 09:36, Bruce Chen wrote:
>> From: Bruce Chen <bruce.chen@unisoc.com>
>>
>> If disable vbus/id irq, it will lead to wakeup system fail
>> in unisoc platform. In unisoc platform, Irq enable and irq
>> wakeup are the same interrupt line. So remove disable vbus/id
>> irq operation is a way to solve the issue> 
>> Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>
>> ---
>>  drivers/extcon/extcon-usb-gpio.c | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index f2b65d9..40d967a 100644
>> --- a/drivers/extcon/extcon-usb-gpio.c
>> +++ b/drivers/extcon/extcon-usb-gpio.c
>> @@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
>>  		}
>>  	}
>>  
>> -	/*
>> -	 * We don't want to process any IRQs after this point
>> -	 * as GPIOs used behind I2C subsystem might not be
>> -	 * accessible until resume completes. So disable IRQ.
>> -	 */
> 
> I think the reason this was added was that if USB cable is plugged/unplugged
> while system was suspending it was causing some faults on TI DRA7 EVM
> at usb_irq_handler()/usb_extcon_detect_cable() when it tries to read GPIO status
> when I2C based GPIO controller was used but already suspended.
> But looks like disabling the IRQ is not the right approach either so your patch
> is right.
> 
> However I'm not sure if the above issue will still exist on DRA7 and other TI EVMS
> using ID/VBUS GPIO over I2C device.
> 
> Let's wait for Aswath/Vignesh to confirm. If no issues I'm OK with this patch.


Support for suspend/resume on DRA7x isn't really maintained actively.
Hence, cannot really test the patch. But, no objections for this patch
either.

[...]

Regards
Vignesh

Re: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[Roger Quadros]

On 03/03/2022 09:36, Bruce Chen wrote:
> From: Bruce Chen <bruce.chen@unisoc.com>
> 
> If disable vbus/id irq, it will lead to wakeup system fail
> in unisoc platform. In unisoc platform, Irq enable and irq
> wakeup are the same interrupt line. So remove disable vbus/id
> irq operation is a way to solve the issue.
> 
> Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>

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

> ---
>  drivers/extcon/extcon-usb-gpio.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index f2b65d9..40d967a 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
>  		}
>  	}
>  
> -	/*
> -	 * We don't want to process any IRQs after this point
> -	 * as GPIOs used behind I2C subsystem might not be
> -	 * accessible until resume completes. So disable IRQ.
> -	 */
> -	if (info->id_gpiod)
> -		disable_irq(info->id_irq);
> -	if (info->vbus_gpiod)
> -		disable_irq(info->vbus_irq);
> -
>  	if (!device_may_wakeup(dev))
>  		pinctrl_pm_select_sleep_state(dev);
>  
> @@ -267,11 +257,6 @@ static int usb_extcon_resume(struct device *dev)
>  		}
>  	}
>  
> -	if (info->id_gpiod)
> -		enable_irq(info->id_irq);
> -	if (info->vbus_gpiod)
> -		enable_irq(info->vbus_irq);
> -
>  	queue_delayed_work(system_power_efficient_wq,
>  			   &info->wq_detcable, 0);
>  

--
cheers,
-roger

Re: [PATCH V1] extcon/usb: Remove disable irq operation in system sleep.

[Chanwoo Choi]

On 3/3/22 6:20 PM, Roger Quadros wrote:
> +TI folks Aswath & Vignesh.
> 
> Hi Bruce,
> 
> On 03/03/2022 09:36, Bruce Chen wrote:
>> From: Bruce Chen <bruce.chen@unisoc.com>
>>
>> If disable vbus/id irq, it will lead to wakeup system fail
>> in unisoc platform. In unisoc platform, Irq enable and irq
>> wakeup are the same interrupt line. So remove disable vbus/id
>> irq operation is a way to solve the issue> 
>> Signed-off-by: Bruce Chen <bruce.chen@unisoc.com>
>> ---
>>  drivers/extcon/extcon-usb-gpio.c | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index f2b65d9..40d967a 100644
>> --- a/drivers/extcon/extcon-usb-gpio.c
>> +++ b/drivers/extcon/extcon-usb-gpio.c
>> @@ -226,16 +226,6 @@ static int usb_extcon_suspend(struct device *dev)
>>  		}
>>  	}
>>  
>> -	/*
>> -	 * We don't want to process any IRQs after this point
>> -	 * as GPIOs used behind I2C subsystem might not be
>> -	 * accessible until resume completes. So disable IRQ.
>> -	 */
> 
> I think the reason this was added was that if USB cable is plugged/unplugged
> while system was suspending it was causing some faults on TI DRA7 EVM
> at usb_irq_handler()/usb_extcon_detect_cable() when it tries to read GPIO status
> when I2C based GPIO controller was used but already suspended.
> But looks like disabling the IRQ is not the right approach either so your patch
> is right.
> 
> However I'm not sure if the above issue will still exist on DRA7 and other TI EVMS
> using ID/VBUS GPIO over I2C device.
> 
> Let's wait for Aswath/Vignesh to confirm. If no issues I'm OK with this patch.
> 
>> -	if (info->id_gpiod)
>> -		disable_irq(info->id_irq);
>> -	if (info->vbus_gpiod)
>> -		disable_irq(info->vbus_irq);
>> -
>>  	if (!device_may_wakeup(dev))
>>  		pinctrl_pm_select_sleep_state(dev);
>>  
>> @@ -267,11 +257,6 @@ static int usb_extcon_resume(struct device *dev)
>>  		}
>>  	}
>>  
>> -	if (info->id_gpiod)
>> -		enable_irq(info->id_irq);
>> -	if (info->vbus_gpiod)
>> -		enable_irq(info->vbus_irq);
>> -
>>  	queue_delayed_work(system_power_efficient_wq,
>>  			   &info->wq_detcable, 0);
>>  
> 
> cheers,
> -roger
> 
> 

Applied it after editing the patch title as following: Thanks.

- extcon: usb-gpio: Remove disable irq operation in system sleep


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics