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?
>
> >
> > > +
> > > + 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.
Frank
>
> 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
> > >
