mbox series

[v3,0/3] usb: renesas_usbhs: add reset_control and multiple clocks management

Message ID 1536213014-1325-1-git-send-email-yoshihiro.shimoda.uh@renesas.com
Headers show
Series usb: renesas_usbhs: add reset_control and multiple clocks management | expand

Message

Yoshihiro Shimoda Sept. 6, 2018, 5:50 a.m. UTC 
This patch set is based on Felipe's usb.git / testing/next branch
(the commit id is 5b394b2ddf0347bef56e50c69a58773c94343ff3) with
the following patch:
  https://patchwork.kernel.org/patch/10574875/

Changes from v2:
 - Use clk_bulk_enable_prepare() instead of two functions on patch 3/3.

Changes from v1:
 - Fix error path on patch 3/3.
 - Use clk_bulk_disable_unprepare() instead of two functions on patch 3/3.
 - Use staic array of struct clk_bulk_data instead of a pointer on patch 3/3.

Yoshihiro Shimoda (3):
  usb: renesas_usbhs: Add reset_control
  dt-bindings: usb: renesas_usbhs: add clock-names property
  usb: renesas_usbhs: Add multiple clocks management

 .../devicetree/bindings/usb/renesas_usbhs.txt      |  2 ++
 drivers/usb/renesas_usbhs/common.c                 | 37 ++++++++++++++++++++++
 drivers/usb/renesas_usbhs/common.h                 |  5 +++
 3 files changed, 44 insertions(+)

Comments

Kuninori Morimoto Sept. 6, 2018, 6:49 a.m. UTC | #1 
Hi Shimoda-san

> R-Car Gen3 needs to enable clocks of both host and peripheral.
> Since [eo]hci-platform disables the reset(s) when the drivers are
> removed, renesas_usbhs driver doesn't work correctly. To fix this
> issue, this patch adds multiple clocks management on this
> renesas_usbhs driver.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
(snip)
> +		/* enable clks if exist */
> +		if (priv->num_clks &&
> +		    clk_bulk_prepare_enable(priv->num_clks, priv->clks))
> +			return;
(snip)
> +		/* disable clks if exist */
> +		if (priv->num_clks)
> +			clk_bulk_disable_unprepare(priv->num_clks, priv->clks);

I think clk_bulk_xxx() will do nothing if priv->num_clks was 0.
priv->num_clks check is not neede, I think.

Best regards
---
Kuninori Morimoto
Geert Uytterhoeven Sept. 6, 2018, 7:28 a.m. UTC | #2 
Hi Shimoda-san,

On Thu, Sep 6, 2018 at 7:52 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> R-Car Gen3 needs to enable clocks of both host and peripheral.
> Since [eo]hci-platform disables the reset(s) when the drivers are
> removed, renesas_usbhs driver doesn't work correctly. To fix this
> issue, this patch adds multiple clocks management on this
> renesas_usbhs driver.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c

> @@ -667,6 +684,12 @@ static int usbhs_probe(struct platform_device *pdev)
>         if (ret)
>                 goto probe_fail_rst;
>
> +       if (priv->num_clks) {
> +               ret = clk_bulk_get(&pdev->dev, priv->num_clks, priv->clks);

(inspired by Morimoto-san) clk_bulk_get() is a no-op if num_clks is zero.


> +               if (ret == -EPROBE_DEFER)

Why this special handling for -EPROBE_DEFER only?
Shouldn't all errors be considered fatal?

Perhaps this is needed for backwards compatibility with old DT?
In that case, you should do more thorough checking (first clock should
exist, second should return -ENOENT).

> +                       goto probe_fail_clks;
> +       }
> +
>         /*
>          * deviece reset here because
>          * USB device might be used in boot loader.

Gr{oetje,eeting}s,

                        Geert
Yoshihiro Shimoda Sept. 6, 2018, 12:09 p.m. UTC | #3 
Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, September 6, 2018 4:28 PM
> 
> Hi Shimoda-san,
> 
> On Thu, Sep 6, 2018 at 7:52 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > R-Car Gen3 needs to enable clocks of both host and peripheral.
> > Since [eo]hci-platform disables the reset(s) when the drivers are
> > removed, renesas_usbhs driver doesn't work correctly. To fix this
> > issue, this patch adds multiple clocks management on this
> > renesas_usbhs driver.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
> 
> > @@ -667,6 +684,12 @@ static int usbhs_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto probe_fail_rst;
> >
> > +       if (priv->num_clks) {
> > +               ret = clk_bulk_get(&pdev->dev, priv->num_clks, priv->clks);
> 
> (inspired by Morimoto-san) clk_bulk_get() is a no-op if num_clks is zero.

That's right. Thank you for your comment.

> > +               if (ret == -EPROBE_DEFER)
> 
> Why this special handling for -EPROBE_DEFER only?
> Shouldn't all errors be considered fatal?
> 
> Perhaps this is needed for backwards compatibility with old DT?

Yes, this is needed for backward s compatibility with old DT.

> In that case, you should do more thorough checking (first clock should
> exist, second should return -ENOENT).

I understood it. (For backwards compatibility with old DT, if second clock
returns -ENOENT, the driver should do a special handling.)
I'll fix this patch.

Best regards,
Yoshihiro Shimoda

> > +                       goto probe_fail_clks;
> > +       }
> > +
> >         /*
> >          * deviece reset here because
> >          * USB device might be used in boot loader.
> 
> 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