| Message ID | 1571915821-1620-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: renesas: rcar-usb2-clock-sel: Fix clks/resets handling | expand |
Hi Shimoda-san, On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > This hardware needs to enable clocks of both host and peripheral. > So, this patch adds multiple clocks management. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > @@ -53,14 +60,32 @@ static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv *priv) > > static int usb2_clock_sel_enable(struct clk_hw *hw) > { > - usb2_clock_sel_enable_extal_only(to_priv(hw)); > + struct usb2_clock_sel_priv *priv = to_priv(hw); > + int i, ret; > + > + for (i = 0; i < CLK_NUM; i++) { > + ret = clk_prepare_enable(priv->clks[i]); > + if (ret) { > + while (--i >= 0) > + clk_disable_unprepare(priv->clks[i]); > + return ret; > + } > + } You can use the clk_bulk_* APIs, instead of open-coding the same operation. > @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); pm_runtime_get_sync() will have already enabled the first module clock listed in the DT "clocks" property. If you want the driver to manage all clocks itself, perhaps the PM Runtime calls should be dropped? > + priv->clks[CLK_INDEX_EHCI_OHCI] = devm_clk_get(dev, "ehci_ohci"); > + if (IS_ERR(priv->clks[CLK_INDEX_EHCI_OHCI])) > + return PTR_ERR(priv->clks[CLK_INDEX_EHCI_OHCI]); > + > + priv->clks[CLK_INDEX_HS_USB] = devm_clk_get(dev, "hs-usb-if"); > + if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) > + return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); > + > clk = devm_clk_get(dev, "usb_extal"); > if (!IS_ERR(clk) && !clk_prepare_enable(clk)) { > priv->extal = !!clk_get_rate(clk); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert-san, Thank you for your comments! > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:35 PM <snip> > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > @@ -53,14 +60,32 @@ static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv *priv) > > > > static int usb2_clock_sel_enable(struct clk_hw *hw) > > { > > - usb2_clock_sel_enable_extal_only(to_priv(hw)); > > + struct usb2_clock_sel_priv *priv = to_priv(hw); > > + int i, ret; > > + > > + for (i = 0; i < CLK_NUM; i++) { > > + ret = clk_prepare_enable(priv->clks[i]); > > + if (ret) { > > + while (--i >= 0) > > + clk_disable_unprepare(priv->clks[i]); > > + return ret; > > + } > > + } > > You can use the clk_bulk_* APIs, instead of open-coding the same > operation. I didn't know such APIs. I'll fix it. > > @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > pm_runtime_enable(dev); > > pm_runtime_get_sync(dev); > > pm_runtime_get_sync() will have already enabled the first module clock listed in > the DT "clocks" property. > > If you want the driver to manage all clocks itself, perhaps the PM Runtime > calls should be dropped? I'm thinking PM Runtime calls are related to power domain control so that we cannot drop it. Or, since the hardware is the Always-on domain, can we drop it anyway? Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Fri, Oct 25, 2019 at 3:36 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:35 PM > <snip> > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > > pm_runtime_enable(dev); > > > pm_runtime_get_sync(dev); > > > > pm_runtime_get_sync() will have already enabled the first module clock listed in > > the DT "clocks" property. > > > > If you want the driver to manage all clocks itself, perhaps the PM Runtime > > calls should be dropped? > > I'm thinking PM Runtime calls are related to power domain control so that we cannot > drop it. Or, since the hardware is the Always-on domain, can we drop it anyway? That's right: if the hardware block ever ends up in a non-always-on power domain, you won't have a choice but to use PM Runtime. Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, October 25, 2019 4:47 PM > > Hi Shimoda-san, > > On Fri, Oct 25, 2019 at 3:36 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:35 PM > > <snip> > > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > > @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > > > pm_runtime_enable(dev); > > > > pm_runtime_get_sync(dev); > > > > > > pm_runtime_get_sync() will have already enabled the first module clock listed in > > > the DT "clocks" property. > > > > > > If you want the driver to manage all clocks itself, perhaps the PM Runtime > > > calls should be dropped? > > > > I'm thinking PM Runtime calls are related to power domain control so that we cannot > > drop it. Or, since the hardware is the Always-on domain, can we drop it anyway? > > That's right: if the hardware block ever ends up in a non-always-on > power domain, > you won't have a choice but to use PM Runtime. So, should I keep the PM Runtime calls? # In such the case, I should add to describe power-domains property into # the dt-binding doc though :) Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Fri, Oct 25, 2019 at 10:44 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Friday, October 25, 2019 4:47 PM > > On Fri, Oct 25, 2019 at 3:36 AM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:35 PM > > > <snip> > > > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > > > > @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > > > > pm_runtime_enable(dev); > > > > > pm_runtime_get_sync(dev); > > > > > > > > pm_runtime_get_sync() will have already enabled the first module clock listed in > > > > the DT "clocks" property. > > > > > > > > If you want the driver to manage all clocks itself, perhaps the PM Runtime > > > > calls should be dropped? > > > > > > I'm thinking PM Runtime calls are related to power domain control so that we cannot > > > drop it. Or, since the hardware is the Always-on domain, can we drop it anyway? > > > > That's right: if the hardware block ever ends up in a non-always-on > > power domain, > > you won't have a choice but to use PM Runtime. > > So, should I keep the PM Runtime calls? I think they're good to have. Just make sure the PM Runtime status matches the state of the other clocks. > # In such the case, I should add to describe power-domains property into > # the dt-binding doc though :) Indeed. Gr{oetje,eeting}s, Geert
diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c b/drivers/clk/renesas/rcar-usb2-clock-sel.c index b97f5f9..570fbd0 100644 --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c @@ -26,9 +26,16 @@ #define CLKSET0_PRIVATE BIT(0) #define CLKSET0_EXTAL_ONLY (CLKSET0_INTCLK_EN | CLKSET0_PRIVATE) +enum { + CLK_INDEX_EHCI_OHCI, + CLK_INDEX_HS_USB, + CLK_NUM +}; + struct usb2_clock_sel_priv { void __iomem *base; struct clk_hw hw; + struct clk *clks[CLK_NUM]; bool extal; bool xtal; }; @@ -53,14 +60,32 @@ static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv *priv) static int usb2_clock_sel_enable(struct clk_hw *hw) { - usb2_clock_sel_enable_extal_only(to_priv(hw)); + struct usb2_clock_sel_priv *priv = to_priv(hw); + int i, ret; + + for (i = 0; i < CLK_NUM; i++) { + ret = clk_prepare_enable(priv->clks[i]); + if (ret) { + while (--i >= 0) + clk_disable_unprepare(priv->clks[i]); + return ret; + } + } + + usb2_clock_sel_enable_extal_only(priv); return 0; } static void usb2_clock_sel_disable(struct clk_hw *hw) { - usb2_clock_sel_disable_extal_only(to_priv(hw)); + struct usb2_clock_sel_priv *priv = to_priv(hw); + int i; + + usb2_clock_sel_disable_extal_only(priv); + + for (i = 0; i < CLK_NUM; i++) + clk_disable_unprepare(priv->clks[i]); } /* @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) pm_runtime_enable(dev); pm_runtime_get_sync(dev); + priv->clks[CLK_INDEX_EHCI_OHCI] = devm_clk_get(dev, "ehci_ohci"); + if (IS_ERR(priv->clks[CLK_INDEX_EHCI_OHCI])) + return PTR_ERR(priv->clks[CLK_INDEX_EHCI_OHCI]); + + priv->clks[CLK_INDEX_HS_USB] = devm_clk_get(dev, "hs-usb-if"); + if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) + return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); + clk = devm_clk_get(dev, "usb_extal"); if (!IS_ERR(clk) && !clk_prepare_enable(clk)) { priv->extal = !!clk_get_rate(clk);
This hardware needs to enable clocks of both host and peripheral. So, this patch adds multiple clocks management. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/clk/renesas/rcar-usb2-clock-sel.c | 37 +++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)