From: Stefan Wahren <wahrenst@gmx.net> To: Xu Yang <xu.yang_2@nxp.com> Cc: peter.chen@kernel.org, gregkh@linuxfoundation.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, luca.ceresoli@bootlin.com, jun.li@nxp.com, linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on Date: Thu, 28 Dec 2023 14:29:58 +0100 [thread overview] Message-ID: <f7988879-4822-48dd-aab2-954fb9cd819e@gmx.net> (raw) In-Reply-To: <20231228110753.1755756-1-xu.yang_2@nxp.com> Hi Xu, Am 28.12.23 um 12:07 schrieb Xu Yang: > Some platform using ChipIdea IP may keep 32KHz wakeup clock always > on without usb driver intervention. And some may need driver to handle > this clock. For now only i.MX93 needs this wakeup clock. This patch will > get wakeup clock and keep it always on to make controller work properly. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > Acked-by: Peter Chen <peter.chen@kernel.org> i've tested successfully the whole series on a custom i.MX28 board (EVAcharge SE). Tested-by: Stefan Wahren <wahrenst@gmx.net> Looking deeper into this patch i'm a little bit confused. The commit message suggests that this patch introduce USB support for i.MX93, but ci-hdrc-usb2.yaml and imx93.dtsi doesn't contain the necessary parts. After a quick search i noticed a separate pending series which contains these parts. https://lore.kernel.org/linux-arm-kernel/DB7PR04MB5146920CF37DBF212199C1958C95A@DB7PR04MB5146.eurprd04.prod.outlook.com/T/#u Please keep in such a case: dt-bindings, driver and DTS changes in one series. This makes reviewing much easier. Especially for a series without any cover letter. Best regards > > --- > Changes in v2: > - modify commit msg to make it clear > Changes in v3: > - use dev_err_probe as suggested by Stefan Wahren > --- > drivers/usb/chipidea/ci_hdrc_imx.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c > index e28bb2f2612d..ae9a6a17ec6e 100644 > --- a/drivers/usb/chipidea/ci_hdrc_imx.c > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c > @@ -96,6 +96,7 @@ struct ci_hdrc_imx_data { > struct usb_phy *phy; > struct platform_device *ci_pdev; > struct clk *clk; > + struct clk *clk_wakeup; > struct imx_usbmisc_data *usbmisc_data; > bool supports_runtime_pm; > bool override_phy_control; > @@ -199,7 +200,7 @@ static int imx_get_clks(struct device *dev) > > data->clk_ipg = devm_clk_get(dev, "ipg"); > if (IS_ERR(data->clk_ipg)) { > - /* If the platform only needs one clocks */ > + /* If the platform only needs one primary clock */ > data->clk = devm_clk_get(dev, NULL); > if (IS_ERR(data->clk)) { > ret = PTR_ERR(data->clk); > @@ -208,6 +209,13 @@ static int imx_get_clks(struct device *dev) > PTR_ERR(data->clk), PTR_ERR(data->clk_ipg)); > return ret; > } > + /* Get wakeup clock. Not all of the platforms need to > + * handle this clock. So make it optional. > + */ > + data->clk_wakeup = devm_clk_get_optional(dev, "usb_wakeup_clk"); > + if (IS_ERR(data->clk_wakeup)) > + ret = dev_err_probe(dev, PTR_ERR(data->clk_wakeup), > + "Failed to get wakeup clk\n"); > return ret; > } > > @@ -423,6 +431,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev) > if (ret) > goto disable_hsic_regulator; > > + ret = clk_prepare_enable(data->clk_wakeup); > + if (ret) > + goto err_wakeup_clk; > + > data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0); > if (IS_ERR(data->phy)) { > ret = PTR_ERR(data->phy); > @@ -504,6 +516,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev) > disable_device: > ci_hdrc_remove_device(data->ci_pdev); > err_clk: > + clk_disable_unprepare(data->clk_wakeup); > +err_wakeup_clk: > imx_disable_unprepare_clks(dev); > disable_hsic_regulator: > if (data->hsic_pad_regulator) > @@ -530,6 +544,7 @@ static void ci_hdrc_imx_remove(struct platform_device *pdev) > usb_phy_shutdown(data->phy); > if (data->ci_pdev) { > imx_disable_unprepare_clks(&pdev->dev); > + clk_disable_unprepare(data->clk_wakeup); > if (data->plat_data->flags & CI_HDRC_PMQOS) > cpu_latency_qos_remove_request(&data->pm_qos_req); > if (data->hsic_pad_regulator) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Stefan Wahren <wahrenst@gmx.net> To: Xu Yang <xu.yang_2@nxp.com> Cc: peter.chen@kernel.org, gregkh@linuxfoundation.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, luca.ceresoli@bootlin.com, jun.li@nxp.com, linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on Date: Thu, 28 Dec 2023 14:29:58 +0100 [thread overview] Message-ID: <f7988879-4822-48dd-aab2-954fb9cd819e@gmx.net> (raw) In-Reply-To: <20231228110753.1755756-1-xu.yang_2@nxp.com> Hi Xu, Am 28.12.23 um 12:07 schrieb Xu Yang: > Some platform using ChipIdea IP may keep 32KHz wakeup clock always > on without usb driver intervention. And some may need driver to handle > this clock. For now only i.MX93 needs this wakeup clock. This patch will > get wakeup clock and keep it always on to make controller work properly. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > Acked-by: Peter Chen <peter.chen@kernel.org> i've tested successfully the whole series on a custom i.MX28 board (EVAcharge SE). Tested-by: Stefan Wahren <wahrenst@gmx.net> Looking deeper into this patch i'm a little bit confused. The commit message suggests that this patch introduce USB support for i.MX93, but ci-hdrc-usb2.yaml and imx93.dtsi doesn't contain the necessary parts. After a quick search i noticed a separate pending series which contains these parts. https://lore.kernel.org/linux-arm-kernel/DB7PR04MB5146920CF37DBF212199C1958C95A@DB7PR04MB5146.eurprd04.prod.outlook.com/T/#u Please keep in such a case: dt-bindings, driver and DTS changes in one series. This makes reviewing much easier. Especially for a series without any cover letter. Best regards > > --- > Changes in v2: > - modify commit msg to make it clear > Changes in v3: > - use dev_err_probe as suggested by Stefan Wahren > --- > drivers/usb/chipidea/ci_hdrc_imx.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c > index e28bb2f2612d..ae9a6a17ec6e 100644 > --- a/drivers/usb/chipidea/ci_hdrc_imx.c > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c > @@ -96,6 +96,7 @@ struct ci_hdrc_imx_data { > struct usb_phy *phy; > struct platform_device *ci_pdev; > struct clk *clk; > + struct clk *clk_wakeup; > struct imx_usbmisc_data *usbmisc_data; > bool supports_runtime_pm; > bool override_phy_control; > @@ -199,7 +200,7 @@ static int imx_get_clks(struct device *dev) > > data->clk_ipg = devm_clk_get(dev, "ipg"); > if (IS_ERR(data->clk_ipg)) { > - /* If the platform only needs one clocks */ > + /* If the platform only needs one primary clock */ > data->clk = devm_clk_get(dev, NULL); > if (IS_ERR(data->clk)) { > ret = PTR_ERR(data->clk); > @@ -208,6 +209,13 @@ static int imx_get_clks(struct device *dev) > PTR_ERR(data->clk), PTR_ERR(data->clk_ipg)); > return ret; > } > + /* Get wakeup clock. Not all of the platforms need to > + * handle this clock. So make it optional. > + */ > + data->clk_wakeup = devm_clk_get_optional(dev, "usb_wakeup_clk"); > + if (IS_ERR(data->clk_wakeup)) > + ret = dev_err_probe(dev, PTR_ERR(data->clk_wakeup), > + "Failed to get wakeup clk\n"); > return ret; > } > > @@ -423,6 +431,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev) > if (ret) > goto disable_hsic_regulator; > > + ret = clk_prepare_enable(data->clk_wakeup); > + if (ret) > + goto err_wakeup_clk; > + > data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0); > if (IS_ERR(data->phy)) { > ret = PTR_ERR(data->phy); > @@ -504,6 +516,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev) > disable_device: > ci_hdrc_remove_device(data->ci_pdev); > err_clk: > + clk_disable_unprepare(data->clk_wakeup); > +err_wakeup_clk: > imx_disable_unprepare_clks(dev); > disable_hsic_regulator: > if (data->hsic_pad_regulator) > @@ -530,6 +544,7 @@ static void ci_hdrc_imx_remove(struct platform_device *pdev) > usb_phy_shutdown(data->phy); > if (data->ci_pdev) { > imx_disable_unprepare_clks(&pdev->dev); > + clk_disable_unprepare(data->clk_wakeup); > if (data->plat_data->flags & CI_HDRC_PMQOS) > cpu_latency_qos_remove_request(&data->pm_qos_req); > if (data->hsic_pad_regulator)
next prev parent reply other threads:[~2023-12-28 13:30 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-12-28 11:07 [PATCH v3 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on Xu Yang 2023-12-28 11:07 ` Xu Yang 2023-12-28 11:07 ` [PATCH v3 2/3] usb: chipidea: wait controller resume finished for wakeup irq Xu Yang 2023-12-28 11:07 ` Xu Yang 2023-12-28 11:07 ` [PATCH v3 3/3] usb: phy: mxs: remove CONFIG_USB_OTG condition for mxs_phy_is_otg_host() Xu Yang 2023-12-28 11:07 ` Xu Yang 2023-12-28 13:29 ` Stefan Wahren [this message] 2023-12-28 13:29 ` [PATCH v3 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on Stefan Wahren
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f7988879-4822-48dd-aab2-954fb9cd819e@gmx.net \ --to=wahrenst@gmx.net \ --cc=festevam@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=jun.li@nxp.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-usb@vger.kernel.org \ --cc=luca.ceresoli@bootlin.com \ --cc=peter.chen@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ --cc=xu.yang_2@nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.