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

[Peter Chen (CIX)]

On 26-03-27 18:54:45, Xu Yang wrote:
> 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? :)

My main concerns are below code, and it may confuse the reader.

ci->id_event = true;
ci->b_sess_valid_event = true;

I may prefer the first option, it is easy to understand.

Peter

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

-- 

Best regards,
Peter