diff mbox series

[1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock

Message ID 20210630173042.186394-1-aford173@gmail.com
State Changes Requested, archived
Headers show
Series [1/3] dt-bindings: usb: renesas,usbhs: Support external ref clock | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

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

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
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
Rob Herring July 14, 2021, 9:27 p.m. UTC | #3 
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
> 
>
Adam Ford Aug. 27, 2021, 6:57 p.m. UTC | #4 
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 | #5 
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/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