[v2,04/15] dt-bindings: rcar-gen3-phy-usb2: Document use of usb_x1
diff mbox series

Message ID 20190509201142.10543-5-chris.brandt@renesas.com
State Superseded
Headers show
Series
  • usb: Add host and device support for RZ/A2
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Chris Brandt May 9, 2019, 8:11 p.m. UTC
Document the optional renesas,uses_usb_x1 property.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
 * removed 'use_usb_x1' option
 * document that 'usb_x1' clock node will be detected to determine if
   48MHz clock exists
---
 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Yoshihiro Shimoda May 10, 2019, 4:38 a.m. UTC | #1
Hi Chris-san,

Thank you for the patch!

> From: Chris Brandt, Sent: Friday, May 10, 2019 5:12 AM
> 
> Document the optional renesas,uses_usb_x1 property.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v2:
>  * removed 'use_usb_x1' option
>  * document that 'usb_x1' clock node will be detected to determine if
>    48MHz clock exists
> ---
>  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> index d46188f450bf..79d8360d92e5 100644
> --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> @@ -28,7 +28,9 @@ Required properties:
>  	      followed by the generic version.
> 
>  - reg: offset and length of the partial USB 2.0 Host register block.
> -- clocks: clock phandle and specifier pair(s).
> +- clocks: clock phandle and specifier pair(s). For SoCs that have a separate
> +          dedicated 48MHz USB_X1 input, if a 'usb_x1' clock node exists and is
> +          set to non-zero, the PHY will use the 48MHZ input for the PLL.

I think we need to add clock-names property for usb_x1 at least.
I checked the other doc "renesas,du.txt".
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/renesas,du.txt#n31

I think we can reuse it like below:

- clock-names: Name of the clocks. This property is model-dependent.
  - R-Car Gen3 SoCs use a single functional clock. The clock doesn't need to be
    named.
  - RZ/A2 uses a single functional clock as a separate dedicated 48MHz
    USB_X1 input. So, the functional clock must be named "???" and
    the USB_X1 input must be named as "usb_x1".

What do you think? I'm not sure how to be named the functional clock so that
the sample is named as "???".

Best regards,
Yoshihiro Shimoda

>  - #phy-cells: see phy-bindings.txt in the same directory, must be <1> (and
>  	      using <0> is deprecated).
> 
> --
> 2.16.1
Geert Uytterhoeven May 10, 2019, 6:55 a.m. UTC | #2
Hi Shimoda-san, Chris,

On Fri, May 10, 2019 at 6:38 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Chris Brandt, Sent: Friday, May 10, 2019 5:12 AM
> >
> > Document the optional renesas,uses_usb_x1 property.
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> > v2:
> >  * removed 'use_usb_x1' option
> >  * document that 'usb_x1' clock node will be detected to determine if
> >    48MHz clock exists
> > ---
> >  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > index d46188f450bf..79d8360d92e5 100644
> > --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > @@ -28,7 +28,9 @@ Required properties:
> >             followed by the generic version.
> >
> >  - reg: offset and length of the partial USB 2.0 Host register block.
> > -- clocks: clock phandle and specifier pair(s).
> > +- clocks: clock phandle and specifier pair(s). For SoCs that have a separate
> > +          dedicated 48MHz USB_X1 input, if a 'usb_x1' clock node exists and is
> > +          set to non-zero, the PHY will use the 48MHZ input for the PLL.
>
> I think we need to add clock-names property for usb_x1 at least.

Indeed. "if a 'usb_x1' clock node exists" is too fragile; better reference
the clock from "clocks".

> I checked the other doc "renesas,du.txt".
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/renesas,du.txt#n31
>
> I think we can reuse it like below:
>
> - clock-names: Name of the clocks. This property is model-dependent.
>   - R-Car Gen3 SoCs use a single functional clock. The clock doesn't need to be
>     named.
>   - RZ/A2 uses a single functional clock as a separate dedicated 48MHz

and a separate?

>     USB_X1 input. So, the functional clock must be named "???" and
>     the USB_X1 input must be named as "usb_x1".
>
> What do you think? I'm not sure how to be named the functional clock so that
> the sample is named as "???".

We typically use "fclk" for the functional clock's name.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt May 13, 2019, 9:07 p.m. UTC | #3
Hi Geert and Shimoda-san,

On Fri, May 10, 2019, Geert Uytterhoeven wrote:
> > I think we can reuse it like below:
> >
> > - clock-names: Name of the clocks. This property is model-dependent.
> >   - R-Car Gen3 SoCs use a single functional clock. The clock doesn't
> need to be
> >     named.
> >   - RZ/A2 uses a single functional clock as a separate dedicated 48MHz
> 
> and a separate?
> 
> >     USB_X1 input. So, the functional clock must be named "???" and
> >     the USB_X1 input must be named as "usb_x1".
> >
> > What do you think? I'm not sure how to be named the functional clock so
> that
> > the sample is named as "???".
> 
> We typically use "fclk" for the functional clock's name.


Just to make sure I'm following this, here is what you are asking for:

[r7s9210.dtsi]

	usb2_phy1: usb-phy@e821a200 {
		compatible = "renesas,usb2-phy-r7s9210", "renesas,rcar-gen3-usb2-phy";
		reg = <0xe821a200 0x10>;
		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 60>, <&usb_x1_clk>;
+		clock-names = "fclk", "usb_x1";
		power-domains = <&cpg>;
		#phy-cells = <0>;
		status = "disabled";


[phy-rcar-gen3-usb2.c]
	usb_x1_clk = devm_clk_get(dev, "usb_x1");
	if (!IS_ERR(usb_x1_clk))
		if (clk_get_rate(usb_x1_clk))
			channel->uses_usb_x1 = true;


And then document this in the bindings, saying that clock-names is 
option if there is only 1 clock (to be backward compatible with existing 
Device Trees.

Is this correct?

Thanks,
Chris
Geert Uytterhoeven May 13, 2019, 9:12 p.m. UTC | #4
Hi Chris,

On Mon, May 13, 2019 at 11:07 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Fri, May 10, 2019, Geert Uytterhoeven wrote:
> > > I think we can reuse it like below:
> > >
> > > - clock-names: Name of the clocks. This property is model-dependent.
> > >   - R-Car Gen3 SoCs use a single functional clock. The clock doesn't
> > need to be
> > >     named.
> > >   - RZ/A2 uses a single functional clock as a separate dedicated 48MHz
> >
> > and a separate?
> >
> > >     USB_X1 input. So, the functional clock must be named "???" and
> > >     the USB_X1 input must be named as "usb_x1".
> > >
> > > What do you think? I'm not sure how to be named the functional clock so
> > that
> > > the sample is named as "???".
> >
> > We typically use "fclk" for the functional clock's name.
>
>
> Just to make sure I'm following this, here is what you are asking for:
>
> [r7s9210.dtsi]
>
>         usb2_phy1: usb-phy@e821a200 {
>                 compatible = "renesas,usb2-phy-r7s9210", "renesas,rcar-gen3-usb2-phy";
>                 reg = <0xe821a200 0x10>;
>                 interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +               clocks = <&cpg CPG_MOD 60>, <&usb_x1_clk>;
> +               clock-names = "fclk", "usb_x1";
>                 power-domains = <&cpg>;
>                 #phy-cells = <0>;
>                 status = "disabled";
>
>
> [phy-rcar-gen3-usb2.c]
>         usb_x1_clk = devm_clk_get(dev, "usb_x1");
>         if (!IS_ERR(usb_x1_clk)))
>                 if (clk_get_rate(usb_x1_clk))

if (!IS_ERR(usb_x1_clk) && clk_get_rate(usb_x1_clk))

>                         channel->uses_usb_x1 = true;
>
>
> And then document this in the bindings, saying that clock-names is
> option if there is only 1 clock (to be backward compatible with existing

optional

> Device Trees.
>
> Is this correct?

Exactly!

Gr{oetje,eeting}s,

                        Geert
Chris Brandt May 13, 2019, 9:24 p.m. UTC | #5
Hi Geert,

Thank you for the quick reply.


On Mon, May 13, 2019, Geert Uytterhoeven wrote:
> > [phy-rcar-gen3-usb2.c]
> >         usb_x1_clk = devm_clk_get(dev, "usb_x1");
> >         if (!IS_ERR(usb_x1_clk)))
> >                 if (clk_get_rate(usb_x1_clk))
> 
> if (!IS_ERR(usb_x1_clk) && clk_get_rate(usb_x1_clk))

:)


> > Is this correct?
> 
> Exactly!

Thank you!

#I'm trying to avoid a v4


Chris

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
index d46188f450bf..79d8360d92e5 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
@@ -28,7 +28,9 @@  Required properties:
 	      followed by the generic version.
 
 - reg: offset and length of the partial USB 2.0 Host register block.
-- clocks: clock phandle and specifier pair(s).
+- clocks: clock phandle and specifier pair(s). For SoCs that have a separate
+          dedicated 48MHz USB_X1 input, if a 'usb_x1' clock node exists and is
+          set to non-zero, the PHY will use the 48MHZ input for the PLL.
 - #phy-cells: see phy-bindings.txt in the same directory, must be <1> (and
 	      using <0> is deprecated).