| Message ID | 20190509201142.10543-5-chris.brandt@renesas.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | usb: Add host and device support for RZ/A2 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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
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
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
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
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
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).
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(-)