| Message ID | 20210630173042.186394-2-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock | expand |
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
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
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 >
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 --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; }
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>