On Thu, Jul 1, 2021 at 6:34 AM Yoshihiro Shimoda
[off-list ref] wrote:

Hi Adam, Geert-san,

quoted

From: Geert Uytterhoeven, Sent: Thursday, July 1, 2021 6:07 PM

<snip>

quoted

To: Adam Ford <redacted>
Cc: USB list <redacted>; Adam Ford-BE <redacted>; Greg Kroah-Hartman
[off-list ref]; Rob Herring [off-list ref]; Magnus Damm [off-list ref]; Yoshihiro Shimoda
[off-list ref]; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
[off-list ref]; Linux Kernel Mailing List [off-list ref]; Linux-Renesas
[off-list ref]
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>

quoted

quoted

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>

quoted

quoted

@@ -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]/

quoted

quoted

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

quoted

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