Re: [PATCH v2 2/3] usb: chipidea: core: allow ci_irq_handler() handle both ID and VBUS change

[Peter Chen (CIX)]

On 26-04-02 15:14:56, Xu Yang wrote:
> For USB role switch-triggered IRQ, ID and VBUS change come together, for
> example when switching from host to device mode. ID indicate a role switch
> and VBUS is required to determine whether the device controller can start
> operating. Currently, ci_irq_handler() handles only a single event per
> invocation. This can cause an issue where switching to device mode results
> in the device controller not working at all. Allowing ci_irq_handler() to
> handle both ID and VBUS change in one call resolves this issue.
> 
> Meanwhile, this change also affects the VBUS event handling logic.
> Previously, if an ID event indicated host mode the VBUS IRQ will be
> ignored as the device disable BSE when stop() is called. With the new
> behavior, if ID and VBUS IRQ occur together and the target mode is host,
> the VBUS event is queued and ci_handle_vbus_change() will call
> usb_gadget_vbus_connect(), after which USBMODE is switched to device mode,
> causing host mode to stop working. To prevent this, an additional check is
> added to skip handling VBUS event when current role is not device mode.
> 
> Suggested-by: Peter Chen <peter.chen@xxxxxxxxxx>
> Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> 
> ---
> Changes in v2:
>  - change ci_irq_handler() instead of assign id_event/b_sess_valid_event
>    as true and queue otg work directly
> ---
>  drivers/usb/chipidea/core.c | 45 +++++++++++++++++++------------------
>  drivers/usb/chipidea/otg.c  |  3 +++
>  2 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 87be716dff3e..7cfabb04a4fb 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -544,30 +544,31 @@ static irqreturn_t ci_irq_handler(int irq, void *data)
>  			if (ret == IRQ_HANDLED)
>  				return ret;
>  		}
> -	}
>  
> -	/*
> -	 * Handle id change interrupt, it indicates device/host function
> -	 * switch.
> -	 */
> -	if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> -		ci->id_event = true;
> -		/* Clear ID change irq status */
> -		hw_write_otgsc(ci, OTGSC_IDIS, OTGSC_IDIS);
> -		ci_otg_queue_work(ci);
> -		return IRQ_HANDLED;
> -	}
> +		/*
> +		 * Handle id change interrupt, it indicates device/host function
> +		 * switch.
> +		 */
> +		if ((otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> +			ci->id_event = true;
> +			/* Clear ID change irq status */
> +			hw_write_otgsc(ci, OTGSC_IDIS, OTGSC_IDIS);
> +		}
>  
> -	/*
> -	 * Handle vbus change interrupt, it indicates device connection
> -	 * and disconnection events.
> -	 */
> -	if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> -		ci->b_sess_valid_event = true;
> -		/* Clear BSV irq */
> -		hw_write_otgsc(ci, OTGSC_BSVIS, OTGSC_BSVIS);
> -		ci_otg_queue_work(ci);
> -		return IRQ_HANDLED;
> +		/*
> +		 * Handle vbus change interrupt, it indicates device connection
> +		 * and disconnection events.
> +		 */
> +		if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> +			ci->b_sess_valid_event = true;
> +			/* Clear BSV irq */
> +			hw_write_otgsc(ci, OTGSC_BSVIS, OTGSC_BSVIS);
> +		}
> +
> +		if (ci->id_event || ci->b_sess_valid_event) {
> +			ci_otg_queue_work(ci);
> +			return IRQ_HANDLED;
> +		}
>  	}
>  
>  	/* Handle device/host interrupt */
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 647e98f4e351..def933b73a90 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -130,6 +130,9 @@ enum ci_role ci_otg_role(struct ci_hdrc *ci)
>  
>  void ci_handle_vbus_change(struct ci_hdrc *ci)
>  {
> +	if (ci->role != CI_ROLE_GADGET)
> +		return;
> +

Are there the situations that the ci->role is not CI_ROLE_GADGET, but
it needs to handle VBUS? Eg, ci->role is CI_ROLE_NONE, and VBUS event
occurs?

-- 

Best regards,
Peter