diff mbox series

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

Message ID 20210630173042.186394-2-aford173@gmail.com
State New
Headers show
Series None | expand

Commit Message

Adam Ford June 30, 2021, 5:30 p.m. UTC 
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>

Comments

Geert Uytterhoeven July 1, 2021, 9:07 a.m. UTC | #1 
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
Yoshihiro Shimoda July 1, 2021, 11:34 a.m. UTC | #2 
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
Adam Ford Aug. 27, 2021, 6:57 p.m. UTC | #3 
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

>
Yoshihiro Shimoda Aug. 30, 2021, 5:32 a.m. UTC | #4 
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
diff mbox series

Patch

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