[PATCH 0/3] clk: renesas: rcar-usb2-clock-sel: Fix clks/resets handling

[PATCH 0/3] clk: renesas: rcar-usb2-clock-sel: Fix clks/resets handling

[Yoshihiro Shimoda]

This patch series is based on the latest renesas-drivers.git /
clk-renesas-for-v5.5 branch.
The hardware also needs multiple clocks/resets management like
renesas_usbhs driver [1], so this patch series fixes it.

[1]
3df0e24 usb: renesas_usbhs: Add multiple clocks management
f181dbb usb: renesas_usbhs: Add reset_control

Yoshihiro Shimoda (3):
  dt-bindings: clock: renesas: rcar-usb2-clock-sel: Fix clock[-name]s
    properties
  clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management
  clk: renesas: rcar-usb2-clock-sel: Add reset_control

 .../bindings/clock/renesas,rcar-usb2-clock-sel.txt |  8 ++--
 drivers/clk/renesas/rcar-usb2-clock-sel.c          | 49 +++++++++++++++++++++-
 2 files changed, 52 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/3] dt-bindings: clock: renesas: rcar-usb2-clock-sel: Fix clock[-name]s properties

[Yoshihiro Shimoda]

Since the hardware requires to enable both USB 2.0 host and peripheral
functional clock, this patch fixes the documentation.
Fortunately, no one has this device node for now, so that we don't
need to think of backward compatibility.

Fixes: 311accb64570 ("clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt     | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
index 83f6c6a..5c1903f 100644
--- a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
+++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
@@ -38,7 +38,8 @@ Required properties:
 - reg: offset and length of the USB 2.0 clock selector register block.
 - clocks: A list of phandles and specifier pairs.
 - clock-names: Name of the clocks.
- - The functional clock must be "ehci_ohci"
+ - The functional clock of USB 2.0 host side must be "ehci_ohci"
+ - The functional clock of HS-USB side must be "hs-usb-if"
  - The USB_EXTAL clock pin must be "usb_extal"
  - The USB_XTAL clock pin must be "usb_xtal"
 - #clock-cells: Must be 0
@@ -49,7 +50,8 @@ Example (R-Car H3):
 		compatible = "renesas,r8a7795-rcar-usb2-clock-sel",
 			     "renesas,rcar-gen3-usb2-clock-sel";
 		reg = <0 0xe6590630 0 0x02>;
-		clocks = <&cpg CPG_MOD 703>, <&usb_extal>, <&usb_xtal>;
-		clock-names = "ehci_ohci", "usb_extal", "usb_xtal";
+		clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>,
+			 <&usb_extal>, <&usb_xtal>;
+		clock-names = "ehci_ohci", "hs-usb-if", "usb_extal", "usb_xtal";
 		#clock-cells = <0>;
 	};
-- 
2.7.4

[PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Yoshihiro Shimoda]

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(-)

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);
-- 
2.7.4

[PATCH 3/3] clk: renesas: rcar-usb2-clock-sel: Add reset_control

[Yoshihiro Shimoda]

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(+)

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);
-- 
2.7.4

Re: [PATCH 3/3] clk: renesas: rcar-usb2-clock-sel: Add reset_control

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Geert Uytterhoeven]

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

Re: [PATCH 1/3] dt-bindings: clock: renesas: rcar-usb2-clock-sel: Fix clock[-name]s properties

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Since the hardware requires to enable both USB 2.0 host and peripheral
> functional clock, this patch fixes the documentation.
> Fortunately, no one has this device node for now, so that we don't
> need to think of backward compatibility.
>
> Fixes: 311accb64570 ("clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks four your patch!

Looks good to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'm just wondering about the inconsistent use of "_" and "-" in clock
names, but I don't have a better suggestion ("hs-usb-if", "usb_extal",
and "usb_xtal" do match the datasheet), so let's ignore my OCD ;-)

> --- a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> +++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> @@ -38,7 +38,8 @@ Required properties:
>  - reg: offset and length of the USB 2.0 clock selector register block.
>  - clocks: A list of phandles and specifier pairs.
>  - clock-names: Name of the clocks.
> - - The functional clock must be "ehci_ohci"
> + - The functional clock of USB 2.0 host side must be "ehci_ohci"

"_" means "and".

> + - The functional clock of HS-USB side must be "hs-usb-if"

"-" means concatenation of terms.

>   - The USB_EXTAL clock pin must be "usb_extal"
>   - The USB_XTAL clock pin must be "usb_xtal"

"_" means concatenation of terms.

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

RE: [PATCH 1/3] dt-bindings: clock: renesas: rcar-usb2-clock-sel: Fix clock[-name]s properties

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:46 PM
> 
> Hi Shimoda-san,
> 
> On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Since the hardware requires to enable both USB 2.0 host and peripheral
> > functional clock, this patch fixes the documentation.
> > Fortunately, no one has this device node for now, so that we don't
> > need to think of backward compatibility.
> >
> > Fixes: 311accb64570 ("clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks four your patch!
> 
> Looks good to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your review!

> I'm just wondering about the inconsistent use of "_" and "-" in clock
> names, but I don't have a better suggestion ("hs-usb-if", "usb_extal",
> and "usb_xtal" do match the datasheet), so let's ignore my OCD ;-)

I intended to match the names with the datasheet, but "ehci_ohci" doesn't match though...
So, should I change the "ehci_ohci" to "ehci/ohci"?

Best regards,
Yoshihiro Shimoda

> > --- a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > +++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > @@ -38,7 +38,8 @@ Required properties:
> >  - reg: offset and length of the USB 2.0 clock selector register block.
> >  - clocks: A list of phandles and specifier pairs.
> >  - clock-names: Name of the clocks.
> > - - The functional clock must be "ehci_ohci"
> > + - The functional clock of USB 2.0 host side must be "ehci_ohci"
> 
> "_" means "and".
> 
> > + - The functional clock of HS-USB side must be "hs-usb-if"
> 
> "-" means concatenation of terms.
> 
> >   - The USB_EXTAL clock pin must be "usb_extal"
> >   - The USB_XTAL clock pin must be "usb_xtal"
> 
> "_" means concatenation of terms.
> 
> 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

RE: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Yoshihiro Shimoda]

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

RE: [PATCH 3/3] clk: renesas: rcar-usb2-clock-sel: Add reset_control

[Yoshihiro Shimoda]

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

Re: [PATCH 1/3] dt-bindings: clock: renesas: rcar-usb2-clock-sel: Fix clock[-name]s properties

[Geert Uytterhoeven]

Hi Shimoda-san,

On Fri, Oct 25, 2019 at 3:29 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Thursday, October 24, 2019 8:46 PM
> > On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > Since the hardware requires to enable both USB 2.0 host and peripheral
> > > functional clock, this patch fixes the documentation.
> > > Fortunately, no one has this device node for now, so that we don't
> > > need to think of backward compatibility.
> > >
> > > Fixes: 311accb64570 ("clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY")
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Thanks four your patch!
> >
> > Looks good to me, so
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Thank you for your review!
>
> > I'm just wondering about the inconsistent use of "_" and "-" in clock
> > names, but I don't have a better suggestion ("hs-usb-if", "usb_extal",
> > and "usb_xtal" do match the datasheet), so let's ignore my OCD ;-)
>
> I intended to match the names with the datasheet, but "ehci_ohci" doesn't match though...
> So, should I change the "ehci_ohci" to "ehci/ohci"?

I think that's up to you.  Both are fine to me.
Given this is USB2, using "ehci" only may be fine, too.

Note that
  - the only other clock with a slash in its name in the datasheet is
    "ths/thc", which we call "thermal" in the clock drivers,
  - the "ehci/ohci" clocks are called "ehciN" or "usb-ehci" in the clock
    drivers.
Nothing relies on those names, though, and they're not part of any ABI,
unlike the ehci/ohci clock input in this binding.

> > > --- a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > > +++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > > @@ -38,7 +38,8 @@ Required properties:
> > >  - reg: offset and length of the USB 2.0 clock selector register block.
> > >  - clocks: A list of phandles and specifier pairs.
> > >  - clock-names: Name of the clocks.
> > > - - The functional clock must be "ehci_ohci"
> > > + - The functional clock of USB 2.0 host side must be "ehci_ohci"
> >
> > "_" means "and".
> >
> > > + - The functional clock of HS-USB side must be "hs-usb-if"
> >
> > "-" means concatenation of terms.
> >
> > >   - The USB_EXTAL clock pin must be "usb_extal"
> > >   - The USB_XTAL clock pin must be "usb_xtal"
> >
> > "_" means concatenation of terms.

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

Re: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH 3/3] clk: renesas: rcar-usb2-clock-sel: Add reset_control

[Geert Uytterhoeven]

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

RE: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Yoshihiro Shimoda]

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

RE: [PATCH 3/3] clk: renesas: rcar-usb2-clock-sel: Add reset_control

[Yoshihiro Shimoda]

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

Re: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Geert Uytterhoeven]

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

-- 
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