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

From: Adam Ford <aford173@gmail.com>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
	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>,
	"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
Date: Fri, 27 Aug 2021 13:57:48 -0500	[thread overview]
Message-ID: <CAHCN7xJrP9Aug6jQzWqdSZ3Tr1_A9qhssc6SYW0rS-VE9Yg9PA@mail.gmail.com> (raw)
In-Reply-To: <TY2PR01MB3692DA5F92178F1ECC31BACDD8009@TY2PR01MB3692.jpnprd01.prod.outlook.com>

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
>