[PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm

[PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm

[Xu Yang]

In current design, we expect 2 ci_irq() to handle ID and VBUS events in
usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
ci_irq() once. However, this won't bring any issues in low power mode,
because ci_irq() just take the device out of low power mode, and then
ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
suspend state, the device mode will not work properly because VBUS event
won'tbe handled (ID event has higher priority) at all.

Although 2 consecutive ci_irq() can work around the issue, do not do it
because ci_usb_role_switch_set() may or not be in low power context which
make the ci_irq() purpose not unique here. Because the final processing
is in ci_otg_work(), just directly queue an otg work. This also refine
the logic for more clarity and not set changed flag.

Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
Cc: stable@vger.kernel.org
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/core.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index fac11f20cf0a..1bd231a852a1 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -618,30 +618,22 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
 	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
 	struct ci_hdrc_cable *cable;
 
-	if (role == USB_ROLE_HOST) {
-		cable = &ci->platdata->id_extcon;
-		cable->changed = true;
+	cable = &ci->platdata->id_extcon;
+	if (role == USB_ROLE_HOST)
 		cable->connected = true;
-		cable = &ci->platdata->vbus_extcon;
-		cable->changed = true;
-		cable->connected = false;
-	} else if (role == USB_ROLE_DEVICE) {
-		cable = &ci->platdata->id_extcon;
-		cable->changed = true;
+	else
 		cable->connected = false;
-		cable = &ci->platdata->vbus_extcon;
-		cable->changed = true;
+
+	cable = &ci->platdata->vbus_extcon;
+	if (role == USB_ROLE_DEVICE)
 		cable->connected = true;
-	} else {
-		cable = &ci->platdata->id_extcon;
-		cable->changed = true;
-		cable->connected = false;
-		cable = &ci->platdata->vbus_extcon;
-		cable->changed = true;
+	else
 		cable->connected = false;
-	}
 
-	ci_irq(ci);
+	ci->id_event = true;
+	ci->b_sess_valid_event = true;
+	ci_otg_queue_work(ci);
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/2] usb: chipidea: otg: not wait vbus drop if use role_switch

[Xu Yang]

The usb role switch will update ID and VBUS states at the same time, and
vbus will not drop when execute data role swap in Type-C usecase. So lets
not wait vbus drop in usb role switch case too.

Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
Cc: stable@vger.kernel.org
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/otg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 647e98f4e351..2371789effa0 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -187,8 +187,8 @@ void ci_handle_id_switch(struct ci_hdrc *ci)
 
 		ci_role_stop(ci);
 
-		if (role == CI_ROLE_GADGET &&
-				IS_ERR(ci->platdata->vbus_extcon.edev))
+		if (role == CI_ROLE_GADGET && !ci->role_switch &&
+		    IS_ERR(ci->platdata->vbus_extcon.edev))
 			/*
 			 * Wait vbus lower than OTGSC_BSV before connecting
 			 * to host. If connecting status is from an external
-- 
2.34.1

Re: [PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm

[Peter Chen (CIX)]

On 26-03-19 17:57:15, Xu Yang wrote:
> In current design, we expect 2 ci_irq() to handle ID and VBUS events in
> usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
> ci_irq() once. However, this won't bring any issues in low power mode,
> because ci_irq() just take the device out of low power mode, and then
> ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
> suspend state, the device mode will not work properly because VBUS event
> won'tbe handled (ID event has higher priority) at all.

%s/won'tbe/won't be

Is it possible change ci_irq_handler and handle both events?

> 
> Although 2 consecutive ci_irq() can work around the issue, do not do it
> because ci_usb_role_switch_set() may or not be in low power context which
> make the ci_irq() purpose not unique here. Because the final processing
> is in ci_otg_work(), just directly queue an otg work. This also refine
> the logic for more clarity and not set changed flag.
> 
> Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> Cc: stable@vger.kernel.org
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/usb/chipidea/core.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index fac11f20cf0a..1bd231a852a1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -618,30 +618,22 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
>  	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
>  	struct ci_hdrc_cable *cable;
>  
> -	if (role == USB_ROLE_HOST) {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> +	cable = &ci->platdata->id_extcon;
> +	if (role == USB_ROLE_HOST)
>  		cable->connected = true;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> -		cable->connected = false;
> -	} else if (role == USB_ROLE_DEVICE) {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> +	else
>  		cable->connected = false;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> +
> +	cable = &ci->platdata->vbus_extcon;
> +	if (role == USB_ROLE_DEVICE)
>  		cable->connected = true;
> -	} else {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> -		cable->connected = false;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> +	else
>  		cable->connected = false;
> -	}
>  
> -	ci_irq(ci);
> +	ci->id_event = true;
> +	ci->b_sess_valid_event = true;

Why both ID and VBUS event are set as true unconditionally?

-- 

Best regards,
Peter

Re: [PATCH 2/2] usb: chipidea: otg: not wait vbus drop if use role_switch

[Peter Chen (CIX)]

On 26-03-19 17:57:16, Xu Yang wrote:
> The usb role switch will update ID and VBUS states at the same time, and
> vbus will not drop when execute data role swap in Type-C usecase. So lets
> not wait vbus drop in usb role switch case too.
> 
> Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> Cc: stable@vger.kernel.org
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter
> ---
>  drivers/usb/chipidea/otg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 647e98f4e351..2371789effa0 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -187,8 +187,8 @@ void ci_handle_id_switch(struct ci_hdrc *ci)
>  
>  		ci_role_stop(ci);
>  
> -		if (role == CI_ROLE_GADGET &&
> -				IS_ERR(ci->platdata->vbus_extcon.edev))
> +		if (role == CI_ROLE_GADGET && !ci->role_switch &&
> +		    IS_ERR(ci->platdata->vbus_extcon.edev))
>  			/*
>  			 * Wait vbus lower than OTGSC_BSV before connecting
>  			 * to host. If connecting status is from an external
> -- 
> 2.34.1
> 

-- 

Best regards,
Peter

Re: [PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm

[Xu Yang]

On Wed, Mar 25, 2026 at 03:08:49PM +0800, Peter Chen (CIX) wrote:
> On 26-03-19 17:57:15, Xu Yang wrote:
> > In current design, we expect 2 ci_irq() to handle ID and VBUS events in
> > usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
> > ci_irq() once. However, this won't bring any issues in low power mode,
> > because ci_irq() just take the device out of low power mode, and then
> > ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
> > suspend state, the device mode will not work properly because VBUS event
> > won'tbe handled (ID event has higher priority) at all.
> 
> %s/won'tbe/won't be

OK.

> 
> Is it possible change ci_irq_handler and handle both events?

Yes, we can change ci_irq_handler() and let it set both ci->id_event and
ci->b_sess_valid_event flags, then queue a ci_otg_work() to handle them
later. 

I think this just unnecessarily call ci_irq_handler() to handle lpm/non-lpm
case as the final path is ci_otg_work() and it will handle lpm/non-lpm case
by naturally calling pm_runtime_get/put_sync(), otherwise it relies on
ci_extcon_wakeup_int() to achieve the same purpose. 

Both methods work for me, may I know which one do you prefer? :)

> 
> > 
> > Although 2 consecutive ci_irq() can work around the issue, do not do it
> > because ci_usb_role_switch_set() may or not be in low power context which
> > make the ci_irq() purpose not unique here. Because the final processing
> > is in ci_otg_work(), just directly queue an otg work. This also refine
> > the logic for more clarity and not set changed flag.
> > 
> > Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/chipidea/core.c | 30 +++++++++++-------------------
> >  1 file changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index fac11f20cf0a..1bd231a852a1 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -618,30 +618,22 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> >  	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> >  	struct ci_hdrc_cable *cable;
> >  
> > -	if (role == USB_ROLE_HOST) {
> > -		cable = &ci->platdata->id_extcon;
> > -		cable->changed = true;
> > +	cable = &ci->platdata->id_extcon;
> > +	if (role == USB_ROLE_HOST)
> >  		cable->connected = true;
> > -		cable = &ci->platdata->vbus_extcon;
> > -		cable->changed = true;
> > -		cable->connected = false;
> > -	} else if (role == USB_ROLE_DEVICE) {
> > -		cable = &ci->platdata->id_extcon;
> > -		cable->changed = true;
> > +	else
> >  		cable->connected = false;
> > -		cable = &ci->platdata->vbus_extcon;
> > -		cable->changed = true;
> > +
> > +	cable = &ci->platdata->vbus_extcon;
> > +	if (role == USB_ROLE_DEVICE)
> >  		cable->connected = true;
> > -	} else {
> > -		cable = &ci->platdata->id_extcon;
> > -		cable->changed = true;
> > -		cable->connected = false;
> > -		cable = &ci->platdata->vbus_extcon;
> > -		cable->changed = true;
> > +	else
> >  		cable->connected = false;
> > -	}
> >  
> > -	ci_irq(ci);
> > +	ci->id_event = true;
> > +	ci->b_sess_valid_event = true;
> 
> Why both ID and VBUS event are set as true unconditionally?

The main purpose is to simplify the handling of the various situations.

The usb role include below types:
 - host
 - device
 - none.

 1. host <--> none
 Above change means ID change occur.

 2. device <--> none
 Above change means VBUS change occur.

 3. host <--> device
 Above change means ID and VBUS change occur.

By setting both of them as true, the logic can be simplified here and
ID and VBUS otg work will check if a real state change happen by comparing
old state and current OTGSC_ID/OTGSC_BSV bit.

Thanks,
Xu Yang











> 
> -- 
> 
> Best regards,
> Peter