| Message ID | 20210630173042.186394-1-aford173@gmail.com |
|---|---|
| State | New |
| 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 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 > >
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/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
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>