diff mbox series

[v5,1/5] dt-bindings: phy-qcom-qmp: Fix register underspecification

Message ID 20181026173544.136037-2-evgreen@chromium.org
State Not Applicable, archived
Headers show
Series arm64: dts: qcom: sdm845: Add UFS DT nodes | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Evan Green Oct. 26, 2018, 5:35 p.m. UTC 
Add register regions for the second lane of dual-lane nodes.
This additional specification is needed so that the driver can stop
reaching beyond the tx and rx register allocations to get at the
second lane registers in a dual-lane PHY.

While in there, document #clock-cells as optional for PHYs that don't
provide a pipe clock. Also, document the pcs_misc register region, which
was being quietly supplied and used.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changes in v5:
- Fix incorrect register value in example, copied from real life!

Changes in v4:
- Remove "status" from DT binding example (Rob)

Changes in v3: None
Changes in v2:
- Added dt bindings change, corresponding driver fixup, and USB PHY fixup

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 70 +++++++++++++++++++---
 1 file changed, 62 insertions(+), 8 deletions(-)

Comments

Stephen Boyd Nov. 4, 2018, 2:40 a.m. UTC | #1 
Quoting Evan Green (2018-10-26 10:35:40)
>                         (or)
> @@ -150,3 +153,54 @@ Example:
>                 ...
>                 ...
>         };
> +
> +       phy@88eb000 {
> +               compatible = "qcom,sdm845-qmp-usb3-uni-phy";
> +               reg = <0x88eb000 0x18c>;
> +               #clock-cells = <1>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
> +                        <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +                        <&gcc GCC_USB3_SEC_CLKREF_CLK>,
> +                        <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
> +               clock-names = "aux", "cfg_ahb", "ref", "com_aux";
> +
> +               resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
> +                        <&gcc GCC_USB3_PHY_SEC_BCR>;
> +               reset-names = "phy", "common";
> +
> +               lane@88eb200 {
> +                       reg = <0x88eb200 0x128>,
> +                             <0x88eb400 0x1fc>,
> +                             <0x88eb800 0x218>,
> +                             <0x88eb600 0x70>;
> +                       #phy-cells = <0>;
> +                       clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
> +                       clock-names = "pipe0";
> +                       clock-output-names = "usb3_uni_phy_pipe_clk_src";

If this has clock-output-names then I would expect to see a #clock-cells
property, but that isn't here in this node. Are we relying on the same
property in the parent node?

> +               };
> +       };
Doug Anderson Nov. 5, 2018, 4:52 p.m. UTC | #2 
Hi,

On Sat, Nov 3, 2018 at 7:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2018-10-26 10:35:40)
> >                         (or)
> > @@ -150,3 +153,54 @@ Example:
> >                 ...
> >                 ...
> >         };
> > +
> > +       phy@88eb000 {
> > +               compatible = "qcom,sdm845-qmp-usb3-uni-phy";
> > +               reg = <0x88eb000 0x18c>;
> > +               #clock-cells = <1>;
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               ranges;
> > +
> > +               clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
> > +                        <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> > +                        <&gcc GCC_USB3_SEC_CLKREF_CLK>,
> > +                        <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
> > +               clock-names = "aux", "cfg_ahb", "ref", "com_aux";
> > +
> > +               resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
> > +                        <&gcc GCC_USB3_PHY_SEC_BCR>;
> > +               reset-names = "phy", "common";
> > +
> > +               lane@88eb200 {
> > +                       reg = <0x88eb200 0x128>,
> > +                             <0x88eb400 0x1fc>,
> > +                             <0x88eb800 0x218>,
> > +                             <0x88eb600 0x70>;
> > +                       #phy-cells = <0>;
> > +                       clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
> > +                       clock-names = "pipe0";
> > +                       clock-output-names = "usb3_uni_phy_pipe_clk_src";
>
> If this has clock-output-names then I would expect to see a #clock-cells
> property, but that isn't here in this node. Are we relying on the same
> property in the parent node?

If I had to guess, I believe it's yet more confusing than that.  I
believe you actually point to the parent phandle if you want to use
the clock.  I notice that the parent has #clock-cells as 1 so
presumably this is how you point to one child or the other?  ...but I
don't think it's documented how this works.  The lane nodes don't have
any sort of ID as far as I can tell.  ...and in any case having
#clock-cells of 1 makes no sense for USB 3 PHYs which are supposed to
only have one child?

Let's look at the code, maybe?  Hrm, phy_pipe_clk_register() takes no
ID or anything.  Huh?  OK, so as far as I can tell
of_clk_add_provider() is never called on this clock...

So I think the answer is that #clock-cells should be <0> and should
move to the child node to match with clock-output-names.  Then I guess
(if anyone references this clock from the device tree rather than
relying on the global clock-output-names) we should add the
of_clk_add_provider() into the code?

Maybe we can add that as a patch to the end of this series?  There are
so many crazy / random things wrong with these bindings that it makes
sense to make smaller / incremental changes?


-Doug
Stephen Boyd Nov. 10, 2018, 12:53 a.m. UTC | #3 
Quoting Doug Anderson (2018-11-05 08:52:39)
> On Sat, Nov 3, 2018 at 7:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > +                       clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
> > > +                       clock-names = "pipe0";
> > > +                       clock-output-names = "usb3_uni_phy_pipe_clk_src";
> >
> > If this has clock-output-names then I would expect to see a #clock-cells
> > property, but that isn't here in this node. Are we relying on the same
> > property in the parent node?
> 
> If I had to guess, I believe it's yet more confusing than that.  I
> believe you actually point to the parent phandle if you want to use
> the clock.  I notice that the parent has #clock-cells as 1 so
> presumably this is how you point to one child or the other?  ...but I
> don't think it's documented how this works.

There are 'clock-ranges', that almost nobody uses. It might be usable
for this purpose.

> The lane nodes don't have
> any sort of ID as far as I can tell.  ...and in any case having
> #clock-cells of 1 makes no sense for USB 3 PHYs which are supposed to
> only have one child?
> 
> Let's look at the code, maybe?  Hrm, phy_pipe_clk_register() takes no
> ID or anything.  Huh?  OK, so as far as I can tell
> of_clk_add_provider() is never called on this clock...
> 
> So I think the answer is that #clock-cells should be <0> and should
> move to the child node to match with clock-output-names.  Then I guess
> (if anyone references this clock from the device tree rather than
> relying on the global clock-output-names) we should add the
> of_clk_add_provider() into the code?
> 
> Maybe we can add that as a patch to the end of this series?  There are
> so many crazy / random things wrong with these bindings that it makes
> sense to make smaller / incremental changes?
> 

Sure that sounds fine. It would be another case where a driver would
want to call the proposed devm_of_clk_add_parent_provider() API.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index fbc198d5dd39..f7b532125a4d 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -25,7 +25,7 @@  Required properties:
   - For all others:
     - The reg-names property shouldn't be defined.
 
- - #clock-cells: must be 1
+ - #clock-cells: must be 1 (PCIe and USB3 PHYs only)
     - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
       interface (for pipe based PHYs). These clock are then gate-controlled
       by gcc.
@@ -82,23 +82,26 @@  Required nodes:
  - Each device node of QMP phy is required to have as many child nodes as
    the number of lanes the PHY has.
 
-Required properties for child node:
+Required properties for child nodes of PCIe PHYs (one child per lane):
  - reg: list of offset and length pairs of register sets for PHY blocks -
-	- index 0: tx
-	- index 1: rx
-	- index 2: pcs
-	- index 3: pcs_misc (optional)
+	tx, rx, pcs, and pcs_misc (optional).
+ - #phy-cells: must be 0
 
+Required properties for a single "lanes" child node of non-PCIe PHYs:
+ - reg: list of offset and length pairs of register sets for PHY blocks
+	For 1-lane devices:
+		tx, rx, pcs, and (optionally) pcs_misc
+	For 2-lane devices:
+		tx0, rx0, pcs, tx1, rx1, and (optionally) pcs_misc
  - #phy-cells: must be 0
 
-Required properties child node of pcie and usb3 qmp phys:
+Required properties for child node of PCIe and USB3 qmp phys:
  - clocks: a list of phandles and clock-specifier pairs,
 	   one for each entry in clock-names.
  - clock-names: Must contain following:
 		 "pipe<lane-number>" for pipe clock specific to each lane.
  - clock-output-names: Name of the PHY clock that will be the parent for
 		       the above pipe clock.
-
 	For "qcom,ipq8074-qmp-pcie-phy":
 		- "pcie20_phy0_pipe_clk"	Pipe Clock parent
 			(or)
@@ -150,3 +153,54 @@  Example:
 		...
 		...
 	};
+
+	phy@88eb000 {
+		compatible = "qcom,sdm845-qmp-usb3-uni-phy";
+		reg = <0x88eb000 0x18c>;
+		#clock-cells = <1>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
+			 <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
+			 <&gcc GCC_USB3_SEC_CLKREF_CLK>,
+			 <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
+		clock-names = "aux", "cfg_ahb", "ref", "com_aux";
+
+		resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
+			 <&gcc GCC_USB3_PHY_SEC_BCR>;
+		reset-names = "phy", "common";
+
+		lane@88eb200 {
+			reg = <0x88eb200 0x128>,
+			      <0x88eb400 0x1fc>,
+			      <0x88eb800 0x218>,
+			      <0x88eb600 0x70>;
+			#phy-cells = <0>;
+			clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
+			clock-names = "pipe0";
+			clock-output-names = "usb3_uni_phy_pipe_clk_src";
+		};
+	};
+
+	phy@1d87000 {
+		compatible = "qcom,sdm845-qmp-ufs-phy";
+		reg = <0x1d87000 0x18c>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		clock-names = "ref",
+			      "ref_aux";
+		clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
+			 <&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
+
+		lanes@1d87400 {
+			reg = <0x1d87400 0x108>,
+			      <0x1d87600 0x1e0>,
+			      <0x1d87c00 0x1dc>,
+			      <0x1d87800 0x108>,
+			      <0x1d87a00 0x1e0>;
+			#phy-cells = <0>;
+		};
+	};