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@xxxxxxxxxxxxxxx
> > Reviewed-by: Jun Li <jun.li@xxxxxxx>
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > ---
> >  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