| Message ID | 20250219093104.2589449-1-xu.yang_2@nxp.com |
|---|---|
| Headers | show |
| Series | add USB2.0 support for i.MX95-19x19 EVK board | expand |
On Wed, Feb 19, 2025 at 05:30:59PM +0800, Xu Yang wrote: > The i.MX95 USB2.0 controller is basically compatible with i.MX7d, except > it needs a second interrupt for wakeup handling. This will add compatible > for i.MX95 platform and restriciton on interrupt property. Besides, this ^ typo, run check-patch --code-spell > will also add iommus property. dt-bindings: usb: Add i.MX95 compatible string 'fsl,imx95-usb' The i.MX95 USB2.0 controller is mostly compatible with i.MX7D, except it requires a second interrupt for wakeup handling. Add the compatible string for the i.MX95 platform, add the iommus property, and enforce the interrupt property restriction. Keep the same restriction for existing compatible strings. Fix below nit. Reviewed-by: Frank Li <Frank.Li@nxp.com> Frank > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > --- > .../bindings/usb/chipidea,usb2-common.yaml | 3 +++ > .../bindings/usb/chipidea,usb2-imx.yaml | 26 ++++++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml > index d2a7d2ecf48a..10020af15afc 100644 > --- a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml > +++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml > @@ -42,6 +42,9 @@ properties: > > phy_type: true > > + iommus: > + maxItems: 1 > + > itc-setting: > description: > interrupt threshold control register control, the setting should be > diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml > index 8f6136f5d72e..3678804a3743 100644 > --- a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml > +++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml > @@ -41,6 +41,7 @@ properties: > - fsl,imx8mm-usb > - fsl,imx8mn-usb > - fsl,imx93-usb > + - fsl,imx95-usb > - const: fsl,imx7d-usb > - const: fsl,imx27-usb > - items: > @@ -54,7 +55,13 @@ properties: > maxItems: 1 > > interrupts: > - maxItems: 1 > + minItems: 1 > + items: > + - description: USB controller interrupt > + - description: Wakeup interrupt > + description: > + One interrupt for USB controller and wakeup interrupt combined > + case or two interrupts for individual USB controller and wakeup Needn't descrition here. Items: descrption already provide enough informaiton. Or you can change first description: USB controller interrupt Or combine USB controller and wakeup interrupts. > > clocks: > minItems: 1 > @@ -191,6 +198,7 @@ allOf: > contains: > enum: > - fsl,imx93-usb > + - fsl,imx95-usb > then: > properties: > clocks: > @@ -238,6 +246,22 @@ allOf: > maxItems: 1 > clock-names: false > > + # imx95 soc use two interrupts > + - if: > + properties: > + compatible: > + contains: > + enum: > + - fsl,imx95-usb > + then: > + properties: > + interrupts: > + minItems: 2 > + else: > + properties: > + interrupts: > + maxItems: 1 > + > unevaluatedProperties: false > > examples: > -- > 2.34.1 >
On Wed, Feb 19, 2025 at 12:58:10PM -0500, Frank Li wrote: > On Wed, Feb 19, 2025 at 05:30:59PM +0800, Xu Yang wrote: > > The i.MX95 USB2.0 controller is basically compatible with i.MX7d, except > > it needs a second interrupt for wakeup handling. This will add compatible > > for i.MX95 platform and restriciton on interrupt property. Besides, this > ^ typo, > > run check-patch --code-spell > > > will also add iommus property. > > dt-bindings: usb: Add i.MX95 compatible string 'fsl,imx95-usb' > > The i.MX95 USB2.0 controller is mostly compatible with i.MX7D, except it > requires a second interrupt for wakeup handling. Add the compatible string > for the i.MX95 platform, add the iommus property, and enforce the > interrupt property restriction. Keep the same restriction for existing > compatible strings. Okay. > > Fix below nit. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Frank > > > > [...] > > + description: > > + One interrupt for USB controller and wakeup interrupt combined > > + case or two interrupts for individual USB controller and wakeup > > Needn't descrition here. Items: descrption already provide enough > informaiton. Or you can change first > > description: USB controller interrupt Or combine USB controller and wakeup > interrupts. Okay. will do. Thanks, Xu Yang
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 > >
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? > > > > > > + > > > + 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 > > >