[EXT] Re: [PATCH v2 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on

[Xu Yang]

Hi Peter,

> 
> On 23-12-18 14:14:18, Xu Yang wrote:
> > 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 at nxp.com>
> >
> > ---
> > Changes in v2:
> >  - modify commit msg to make it clear
> > ---
> >  drivers/usb/chipidea/ci_hdrc_imx.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index e28bb2f2612d..4330be8240ff 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,18 @@ 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 = PTR_ERR(data->clk_wakeup);
> > +                     dev_err(dev,
> > +                             "Failed to get wakeup clk, err=%ld\n",
> > +                             PTR_ERR(data->clk_wakeup));
> > +                     return ret;
> > +             }
> >               return ret;
> >       }
> >
> > @@ -423,6 +436,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;
> > +
> 
> Better add at function imx_prepare_enable_clks()
> 
> >       data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
> >       if (IS_ERR(data->phy)) {
> >               ret = PTR_ERR(data->phy);
> > @@ -504,6 +521,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:
> 
> 
> Better add clk_disable_unprepare for wakeup clk at function
> imx_disable_unprepare_clks();

According to IC degin's suggestion, 32K clock should be enabled
all the time. Since imx_prepare_enable_clks and imx_disable_unprepare_clks()
will be dynamiclly called in runtime/system PM cases,  I don't want
32K clock go though such path. Therefore, I didn't put clk_prepare_enable/
clk_disable_unprepare() in that functions.

Thanks,
Xu Yang

> 
> Peter
> 
> >       imx_disable_unprepare_clks(dev);
> >  disable_hsic_regulator:
> >       if (data->hsic_pad_regulator)
> > @@ -530,6 +549,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)
> > --
> > 2.34.1
> >
> >
> 
> --
> 
> Thanks,
> Peter Chen