Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Xu Yang <xu.yang_2@nxp.com>]

On Fri, Feb 21, 2025 at 10:23:26AM -0500, Frank Li wrote:
> On Fri, Feb 21, 2025 at 11:23:48AM +0800, Xu Yang wrote:
> > On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> > > On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > > > In previous imx platform, normal USB controller interrupt and wakeup
> > > > interrupt are bound to one irq line. However, it changes on latest
> > > > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > > > This will add wakup interrupt handling for i.MX95 to support various
> > > > wakeup events.
> > > >
> > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > ---
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 1a7fc638213e..5779568ebcf6 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> > > >  	struct clk *clk;
> > > >  	struct clk *clk_wakeup;
> > > >  	struct imx_usbmisc_data *usbmisc_data;
> > > > +	/* wakeup interrupt*/
> > > > +	int irq;
> > >
> > > use "wakeup_irq" to avoid confuse with normal controller irq?
> >
> > It doesn't matter. It can't be confused since the driver is different. The
> > controller driver is core.c. This glue driver is ci_hdrc_imx.c.
> >
> > >
> > > >  	bool supports_runtime_pm;
> > > >  	bool override_phy_control;
> > > >  	bool in_lpm;
> > > > +	bool wakeup_pending;
> > > >  	struct pinctrl *pinctrl;
> > > >  	struct pinctrl_state *pinctrl_hsic_active;
> > > >  	struct regulator *hsic_pad_regulator;
> > > > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > > > +{
> > > > +	struct ci_hdrc_imx_data *imx_data = data;
> > > > +
> > > > +	if (imx_data->in_lpm) {
> > > > +		if (imx_data->wakeup_pending)
> > > > +			return IRQ_HANDLED;
> > > > +
> > > > +		disable_irq_nosync(irq);
> > > > +		imx_data->wakeup_pending = true;
> > > > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> > >
> > > Not sure why need pm_runtime_resume here? There are not access register
> > > here.
> >
> > It's needed for runtime resume case.
> > When wakeup event happens, wakeup irq will be triggered. To let controller
> > resume back, we need enable USB controller clock to trigger controller
> > irq. So we call pm_runtime_resume() to resume controller's parent back
> > and the parent device will enable clocks.
> 
> Please add comment here. why need in_lpm if we can make sure irq only
> enable during suspend/resume pharse?

I have checked again, in_lpm checking is not needed. I will remove the
if condition later.

> 
> >
> > >
> > > > +
> > > > +		return IRQ_HANDLED;
> > > > +	}
> > > > +
> > > > +	return IRQ_NONE;
> > > > +}
> > > > +
> > > >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct ci_hdrc_imx_data *data;
> > > > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > > >  		data->supports_runtime_pm = true;
> > > >
> > > > +	data->irq = platform_get_irq_optional(pdev, 1);
> > > > +	if (data->irq > 0) {
> > > > +		ret = devm_request_threaded_irq(dev, data->irq,
> > > > +				NULL, ci_wakeup_irq_handler,
> > > > +				IRQF_ONESHOT, pdata.name, data);
> > > > +		if (ret)
> > > > +			goto err_clk;
> > > > +	}
> > > > +
> > > >  	ret = imx_usbmisc_init(data->usbmisc_data);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > > > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> > > >  		goto clk_disable;
> > > >  	}
> > > >
> > > > +	if (data->wakeup_pending) {
> > > > +		data->wakeup_pending = false;
> > > > +		enable_irq(data->irq);
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >
> > > >  clk_disable:
> > > > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> > > >  		return ret;
> > > >
> > > >  	pinctrl_pm_select_sleep_state(dev);
> > > > +
> > > > +	if (device_may_wakeup(dev))
> > > > +		enable_irq_wake(data->irq);
> > > > +
> > > >  	return ret;
> > > >  }
> > > >
> > > > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> > > >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > > >  	int ret;
> > > >
> > > > +	if (device_may_wakeup(dev))
> > > > +		disable_irq_wake(data->irq);
> > > > +
> > >
> > > Look like only want enable wakeup irq between suspend and resume. There are
> > > some questions to understand hehavor.
> > >
> > > 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> > >
> > > 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> > > disable issue wakeup irq, otherwise flood irq will happen because you check
> > > lpm in irq handler.
> >
> > It's not true.
> >
> > We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
> > enabled when do suspend() and disabled when do resume(). So before suspend()
> > called, the wakeup irq can't be triggered. No flood irq too.
> >
> > >
> > > after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> > > ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> > >
> > > But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> > > guaranteed.
> > >
> > > If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> > > itself. imx_controller_resume() will enable wakeup irq, which will be same
> > > state 1--2. but if imx_controller_resume() run firstly,
> >
> > It's not true too. Because WAKE_ENABLE is disabled after resume().
> >
> > > ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> > > with 1--2.
> > >
> > > If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> > > enable/disable,
> >
> > Yes, WAKE_ENABLE bit. It's already used:
> >
> > ci_hdrc_imx_suspend()
> >   imx_controller_suspend() -> enable WAKE_ENABLE
> >
> > ci_hdrc_imx_resume()
> >   imx_controller_resume() -> disable WAKE_ENABLE
> 
> Okay,
> 
> I think wakeup_pending is not neccesary. ci_wakeup_irq_handler() can
> simple disable WAKE_ENABLE.

Right, wakeup_pending can be removed. However, it's not that simple to
control WAKE_ENABLE in ci_hdrc_imx.c due to imx_controller_susepnd/resume()
do more things expect enable/disable WAKE_ENABLE bit. And usbmisc.c doesn't
export a function to directly control WAKE_ENABLE. Therefore, I still need
to use enable/disable_irq() for simplicity.

Due to this wakeup irq is only used in suspend case, I would like to add
IRQF_NO_AUTOEN flag to request_threaded_irq(), then enable irq in
ci_hdrc_imx_suspend() and disable irq in ci_hdrc_imx_resume(). 

For example:

ci_wakeup_irq_handler()
{
	disable_irq_nosync(data->irq);
	...
}

ci_hdrc_imx_suspend()
{
	...
	enable_irq(data->irq);	
}

ci_hdrc_imx_resume()
{
	if (irq disabled)
		disable_irq_nosync(data->irq);
	...
}

Do you think if it's feasible and better than current patch?

Thanks,
Xu Yang