[PATCH 1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock

[PATCH 1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock

[Adam Ford]

The usbhs driver expects a fixed external reference clock, but it
could be sourced from a programmable clock instead of a fixed clock.
Add support for an optional 4th reference clock.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
index e67223d90bb7..2372d8c42979 100644
--- a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
+++ b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
@@ -53,11 +53,12 @@ properties:
 
   clocks:
     minItems: 1
-    maxItems: 3
+    maxItems: 4
     items:
       - description: USB 2.0 host
       - description: USB 2.0 peripheral
       - description: USB 2.0 clock selector
+      - description: Optional external reference clock
 
   interrupts:
     maxItems: 1
-- 
2.25.1

[PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks

[Adam Ford]

The RZ/G2 boards expect there to be an external clock reference for
USBHS controller, but this could be set by a programmable clock.
For those devices using a programmable clock, there need to be two
additional clocks beyond the internal reference clocks:

rcar-usb2-clock-sel to specify we using an external clock, and
the external reference clock itself.

Make this driver dynamically enable all the clocks assigned to it
instead of only enabling the first one or two clocks.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 3af91b2b8f76..255e4bd68ed3 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -297,6 +297,8 @@ static bool usbhsc_is_multi_clks(struct usbhs_priv *priv)
 
 static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
 {
+	unsigned int i;
+
 	if (!usbhsc_is_multi_clks(priv))
 		return 0;
 
@@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
 	 * To backward compatibility with old DT, this driver checks the return
 	 * value if it's -ENOENT or not.
 	 */
-	priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
-	if (PTR_ERR(priv->clks[1]) == -ENOENT)
-		priv->clks[1] = NULL;
-	else if (IS_ERR(priv->clks[1]))
-		return PTR_ERR(priv->clks[1]);
+	for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
+		priv->clks[1] = of_clk_get(dev->of_node, i);
+		if (PTR_ERR(priv->clks[i]) == -ENOENT)
+			priv->clks[i] = NULL;
+		else if (IS_ERR(priv->clks[i]))
+			return PTR_ERR(priv->clks[i]);
+	}
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks

[Geert Uytterhoeven]

Hi Adam,

Thanks for your patch!

On Wed, Jun 30, 2021 at 7:30 PM Adam Ford <aford173@gmail.com> wrote:
> The RZ/G2 boards expect there to be an external clock reference for
> USBHS controller, but this could be set by a programmable clock.
> For those devices using a programmable clock, there need to be two
> additional clocks beyond the internal reference clocks:
>
> rcar-usb2-clock-sel to specify we using an external clock, and
> the external reference clock itself.

Something is missing in the above sentence?

>
> Make this driver dynamically enable all the clocks assigned to it
> instead of only enabling the first one or two clocks.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> index 3af91b2b8f76..255e4bd68ed3 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -297,6 +297,8 @@ static bool usbhsc_is_multi_clks(struct usbhs_priv *priv)
>
>  static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
>  {
> +       unsigned int i;
> +
>         if (!usbhsc_is_multi_clks(priv))
>                 return 0;
>
> @@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
>          * To backward compatibility with old DT, this driver checks the return
>          * value if it's -ENOENT or not.
>          */
> -       priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
> -       if (PTR_ERR(priv->clks[1]) == -ENOENT)
> -               priv->clks[1] = NULL;
> -       else if (IS_ERR(priv->clks[1]))
> -               return PTR_ERR(priv->clks[1]);
> +       for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
> +               priv->clks[1] = of_clk_get(dev->of_node, i);
> +               if (PTR_ERR(priv->clks[i]) == -ENOENT)
> +                       priv->clks[i] = NULL;
> +               else if (IS_ERR(priv->clks[i]))
> +                       return PTR_ERR(priv->clks[i]);
> +       }

This is identical to the current code, as ARRAY_SIZE(priv->clks) == 2.
Probably you wanted to increase usbhs_priv.clks[], too?

Does it make sense to start using the clk_bulk*() API?

>
>         return 0;
>  }

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] usb: renesas_usbhs: Enable support for more than two clks

[Yoshihiro Shimoda]

Hi Adam, Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, July 1, 2021 6:07 PM
<snip>
> To: Adam Ford <aford173@gmail.com>
> Cc: USB list <linux-usb@vger.kernel.org>; Adam Ford-BE <aford@beaconembedded.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Magnus Damm <magnus.damm@gmail.com>; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Linux-Renesas
> <linux-renesas-soc@vger.kernel.org>
> Subject: Re: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks
> 
> Hi Adam,
> 
> Thanks for your patch!
> 
> On Wed, Jun 30, 2021 at 7:30 PM Adam Ford wrote:
<snip>
> > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > index 3af91b2b8f76..255e4bd68ed3 100644
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
<snip>
> > @@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
> >          * To backward compatibility with old DT, this driver checks the return
> >          * value if it's -ENOENT or not.
> >          */
> > -       priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
> > -       if (PTR_ERR(priv->clks[1]) == -ENOENT)
> > -               priv->clks[1] = NULL;
> > -       else if (IS_ERR(priv->clks[1]))
> > -               return PTR_ERR(priv->clks[1]);
> > +       for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
> > +               priv->clks[1] = of_clk_get(dev->of_node, i);

s/clks[1]/[i]/

> > +               if (PTR_ERR(priv->clks[i]) == -ENOENT)
> > +                       priv->clks[i] = NULL;
> > +               else if (IS_ERR(priv->clks[i]))
> > +                       return PTR_ERR(priv->clks[i]);
> > +       }
> 
> This is identical to the current code, as ARRAY_SIZE(priv->clks) == 2.
> Probably you wanted to increase usbhs_priv.clks[], too?

I think so.
# I realized the clks array is only 2, so that this driver cannot
# enable usb2_clksel...

> Does it make sense to start using the clk_bulk*() API?

clk_bulk*() API seems to need clock-names property.
Is my understanding correct? However, the hsusb nodes doesn't
have the property for now...

Best regards,
Yoshihiro Shimoda

Re: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks

[Adam Ford]

On Thu, Jul 1, 2021 at 6:34 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> Hi Adam, Geert-san,
>
> > From: Geert Uytterhoeven, Sent: Thursday, July 1, 2021 6:07 PM
> <snip>
> > To: Adam Ford <aford173@gmail.com>
> > Cc: USB list <linux-usb@vger.kernel.org>; Adam Ford-BE <aford@beaconembedded.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Magnus Damm <magnus.damm@gmail.com>; Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Linux-Renesas
> > <linux-renesas-soc@vger.kernel.org>
> > Subject: Re: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks
> >
> > Hi Adam,
> >
> > Thanks for your patch!
> >
> > On Wed, Jun 30, 2021 at 7:30 PM Adam Ford wrote:
> <snip>
> > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > > index 3af91b2b8f76..255e4bd68ed3 100644
> > > --- a/drivers/usb/renesas_usbhs/common.c
> > > +++ b/drivers/usb/renesas_usbhs/common.c
> <snip>
> > > @@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
> > >          * To backward compatibility with old DT, this driver checks the return
> > >          * value if it's -ENOENT or not.
> > >          */
> > > -       priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
> > > -       if (PTR_ERR(priv->clks[1]) == -ENOENT)
> > > -               priv->clks[1] = NULL;
> > > -       else if (IS_ERR(priv->clks[1]))
> > > -               return PTR_ERR(priv->clks[1]);
> > > +       for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
> > > +               priv->clks[1] = of_clk_get(dev->of_node, i);
>
> s/clks[1]/[i]/
>
> > > +               if (PTR_ERR(priv->clks[i]) == -ENOENT)
> > > +                       priv->clks[i] = NULL;
> > > +               else if (IS_ERR(priv->clks[i]))
> > > +                       return PTR_ERR(priv->clks[i]);
> > > +       }
> >
> > This is identical to the current code, as ARRAY_SIZE(priv->clks) == 2.
> > Probably you wanted to increase usbhs_priv.clks[], too?

Thanks for the review.  Sorry for my delayed response.  I broke my
wrist on the evening on June 30, and I was on medical leave for 6
weeks.  I am now trying to get caught up to where I was.
>
> I think so.
> # I realized the clks array is only 2, so that this driver cannot
> # enable usb2_clksel...
>
> > Does it make sense to start using the clk_bulk*() API?
>
> clk_bulk*() API seems to need clock-names property.
> Is my understanding correct? However, the hsusb nodes doesn't
> have the property for now...

The usb2_clksel has the names "ehci_ohci" and "hs-usb-if" for <&cpg
CPG_MOD 703> and <&cpg CPG_MOD 704>, respectively.

What if I add the clock-names property, then switch to the clk_bulk
IO?  I think we can just turn them all on together if that's
acceptable

adam


>
> Best regards,
> Yoshihiro Shimoda
>

RE: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks

[Yoshihiro Shimoda]

Hi Adam,

> From: Adam Ford, Sent: Saturday, August 28, 2021 3:58 AM
> 
> On Thu, Jul 1, 2021 at 6:34 AM Yoshihiro Shimoda wrote:
> >
> > Hi Adam, Geert-san,
> >
> > > From: Geert Uytterhoeven, Sent: Thursday, July 1, 2021 6:07 PM
> > >
> > > Hi Adam,
> > >
> > > Thanks for your patch!
> > >
> > > On Wed, Jun 30, 2021 at 7:30 PM Adam Ford wrote:
> > <snip>
> > > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > > > index 3af91b2b8f76..255e4bd68ed3 100644
> > > > --- a/drivers/usb/renesas_usbhs/common.c
> > > > +++ b/drivers/usb/renesas_usbhs/common.c
> > <snip>
> > > > @@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
> > > >          * To backward compatibility with old DT, this driver checks the return
> > > >          * value if it's -ENOENT or not.
> > > >          */
> > > > -       priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
> > > > -       if (PTR_ERR(priv->clks[1]) == -ENOENT)
> > > > -               priv->clks[1] = NULL;
> > > > -       else if (IS_ERR(priv->clks[1]))
> > > > -               return PTR_ERR(priv->clks[1]);
> > > > +       for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
> > > > +               priv->clks[1] = of_clk_get(dev->of_node, i);
> >
> > s/clks[1]/[i]/
> >
> > > > +               if (PTR_ERR(priv->clks[i]) == -ENOENT)
> > > > +                       priv->clks[i] = NULL;
> > > > +               else if (IS_ERR(priv->clks[i]))
> > > > +                       return PTR_ERR(priv->clks[i]);
> > > > +       }
> > >
> > > This is identical to the current code, as ARRAY_SIZE(priv->clks) == 2.
> > > Probably you wanted to increase usbhs_priv.clks[], too?
> 
> Thanks for the review.  Sorry for my delayed response.  I broke my
> wrist on the evening on June 30, and I was on medical leave for 6
> weeks.  I am now trying to get caught up to where I was.

I'm sorry to hear that. I hope you recover soon.

> >
> > I think so.
> > # I realized the clks array is only 2, so that this driver cannot
> > # enable usb2_clksel...
> >
> > > Does it make sense to start using the clk_bulk*() API?
> >
> > clk_bulk*() API seems to need clock-names property.
> > Is my understanding correct? However, the hsusb nodes doesn't
> > have the property for now...
> 
> The usb2_clksel has the names "ehci_ohci" and "hs-usb-if" for <&cpg
> CPG_MOD 703> and <&cpg CPG_MOD 704>, respectively.
> 
> What if I add the clock-names property, then switch to the clk_bulk
> IO?  I think we can just turn them all on together if that's
> acceptable

According to an article [1],
=====================================================
forward and backward dts compatibility

Current practice is that
* new kernels work with old devicetrees
* old kernels may or may not work with new devicetrees
=====================================================
[1]
https://elinux.org/Device_Tree_Linux#forward_and_backward_dts_compatibility

So, IIUC, we have to modify binding doc, dts/dtsi files and the driver code step-by-step
to achieve the compatibility.
1) Modify binding doc to add clock-names.
2) Modify the renesas_usbhs driver to check clock-names.
3) Modify all dts/dtsi files to add clock-names.

We can make "1) and 2)" patches, and "3)" patch in parallel.
And, the 2) should take care of forward and backword dts compatibility,
IIUC.

Best regards,
Yoshihiro Shimoda

[PATCH 3/3] arm64: dts: renesas: beacon: Fix HSUSB ref clock references

[Adam Ford]

The RZ/G2 boards expect there to be an external clock reference for
USBHS controller.  For the Beacon boards, this reference clock
is controlled by a programmable versaclock.  Because the RZ/G2
family has a special clock driver when using an external clock,
the third clock reference in the USBHS node needs to point to this
special clock, called usb2_clksel, and the 4th clock is the versaclock.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
index e3c8b2fe143e..bcbf7eb5195b 100644
--- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
+++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
@@ -313,6 +313,7 @@ &hscif1 {
 };
 
 &hsusb {
+	clocks = <&cpg CPG_MOD 704>, <&cpg CPG_MOD 703>, <&usb2_clksel>, <&versaclock6_som 3>;
 	dr_mode = "otg";
 	status = "okay";
 };
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock

[Rob Herring]

On Wed, Jun 30, 2021 at 12:30:40PM -0500, Adam Ford wrote:
> The usbhs driver expects a fixed external reference clock, but it
> could be sourced from a programmable clock instead of a fixed clock.
> Add support for an optional 4th reference clock.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
> index e67223d90bb7..2372d8c42979 100644
> --- a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
> +++ b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
> @@ -53,11 +53,12 @@ properties:
>  
>    clocks:
>      minItems: 1
> -    maxItems: 3
> +    maxItems: 4

You won't need this in v5.14-rc1. These are implied by 'items' length if 
not specified.

>      items:
>        - description: USB 2.0 host
>        - description: USB 2.0 peripheral
>        - description: USB 2.0 clock selector
> +      - description: Optional external reference clock
>  
>    interrupts:
>      maxItems: 1
> -- 
> 2.25.1
> 
>