| Message ID | 1571915821-1620-4-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, Thanks for your patch! On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > This hardware needs to deassert resets of both host and peripheral. > So, this patch adds reset control. If the hardware needs it, probably you want to make CLK_RCAR_USB2_CLOCK_SEL select RESET_CONTROLLER? > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > @@ -164,6 +172,10 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) > return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); > > + priv->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev); If the reset is really needed, you should not use the optional API. > + if (IS_ERR(priv->rsts)) > + return PTR_ERR(priv->rsts); > + > 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
Hi Geert-san, Thank you for your comments! > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:26 PM <snip> > > This hardware needs to deassert resets of both host and peripheral. > > So, this patch adds reset control. > > If the hardware needs it, probably you want to make CLK_RCAR_USB2_CLOCK_SEL > select RESET_CONTROLLER? You're correct. I'll fix it. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > @@ -164,6 +172,10 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) > > return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); > > > > + priv->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev); > > If the reset is really needed, you should not use the optional API. That's true. So, I'll use devm_reset_control_array_get(&pdev->dev, true, false) Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Fri, Oct 25, 2019 at 3:42 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:26 PM > <snip> > > > This hardware needs to deassert resets of both host and peripheral. > > > So, this patch adds reset control. > > > > If the hardware needs it, probably you want to make CLK_RCAR_USB2_CLOCK_SEL > > select RESET_CONTROLLER? > > You're correct. I'll fix it. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > > @@ -164,6 +172,10 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > > if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) > > > return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); > > > > > > + priv->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev); > > > > If the reset is really needed, you should not use the optional API. > > That's true. So, I'll use devm_reset_control_array_get(&pdev->dev, true, false) Any reason you need the array version? Are there multiple resets to be specified? No longer shared? Which brings to my attention you forgot to document the resets in the DT bindings ;-) 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, > From: Geert Uytterhoeven, Sent: Friday, October 25, 2019 4:54 PM > > Hi Shimoda-san, > > On Fri, Oct 25, 2019 at 3:42 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:26 PM > > <snip> > > > > This hardware needs to deassert resets of both host and peripheral. > > > > So, this patch adds reset control. > > > > > > If the hardware needs it, probably you want to make CLK_RCAR_USB2_CLOCK_SEL > > > select RESET_CONTROLLER? > > > > You're correct. I'll fix it. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c > > > > > > > @@ -164,6 +172,10 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) > > > > if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) > > > > return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); > > > > > > > > + priv->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev); > > > > > > If the reset is really needed, you should not use the optional API. > > > > That's true. So, I'll use devm_reset_control_array_get(&pdev->dev, true, false) > > Any reason you need the array version? Are there multiple resets to be > specified? Yes, there are multiple resets (<&cpg 703> and <&cpg 704>) to be specified. > No longer shared? This hardware shares the resets with USB host (EHCI and OHCI), USB Function (HS-USB) and USB2-PHY... > Which brings to my attention you forgot to document the resets in the > DT bindings ;-) That's right. I'll add the resets property. Best regards, Yoshihiro Shimoda > > 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
diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c b/drivers/clk/renesas/rcar-usb2-clock-sel.c index 570fbd0..dfe5510 100644 --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c @@ -19,6 +19,7 @@ #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include <linux/slab.h> #define USB20_CLKSET0 0x00 @@ -36,6 +37,7 @@ struct usb2_clock_sel_priv { void __iomem *base; struct clk_hw hw; struct clk *clks[CLK_NUM]; + struct reset_control *rsts; bool extal; bool xtal; }; @@ -63,11 +65,16 @@ static int usb2_clock_sel_enable(struct clk_hw *hw) struct usb2_clock_sel_priv *priv = to_priv(hw); int i, ret; + ret = reset_control_deassert(priv->rsts); + if (ret) + return 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]); + reset_control_assert(priv->rsts); return ret; } } @@ -86,6 +93,7 @@ static void usb2_clock_sel_disable(struct clk_hw *hw) for (i = 0; i < CLK_NUM; i++) clk_disable_unprepare(priv->clks[i]); + reset_control_assert(priv->rsts); } /* @@ -164,6 +172,10 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev) if (IS_ERR(priv->clks[CLK_INDEX_HS_USB])) return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]); + priv->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev); + if (IS_ERR(priv->rsts)) + return PTR_ERR(priv->rsts); + 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 deassert resets of both host and peripheral. So, this patch adds reset control. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/clk/renesas/rcar-usb2-clock-sel.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)