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

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

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.

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

Thanks,
Xu Yang

> 
> will below logic be simple?
> 
> ci_hdrc_imx_suspend()
> {
> 	writel(WAKEUP_EN, ...);
> 	....
> }
> 
> ci_wakeup_irq_handler()
> {
> 	...
> 	pm_runtime_resume(&imx_data->ci_pdev->dev);
> 	writel(~WAKEUP_EN, ...);
> 	...
> }
> 
> Frank
> 
> >  	pinctrl_pm_select_default_state(dev);
> >  	ret = imx_controller_resume(dev, PMSG_RESUME);
> >  	if (!ret && data->supports_runtime_pm) {
> > --
> > 2.34.1
> >