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@xxxxxxx>
> > > > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > > > ---
> > > > 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
