[PATCH v5 0/5] arm64: dts: qcom: sdm845: Add UFS DT nodes

[PATCH v5 0/5] arm64: dts: qcom: sdm845: Add UFS DT nodes

[Evan Green]

Update the device tree bindings for the QMP PHY to properly
specify the registers for dual-lane PHYs. Update the driver to use
those new registers. Add the DT nodes for UFS on SDM845 and MTP.
Finally, fix up the USB3 PHY on SDM845, which also has a dual-lane phy

Andy/Kishon,
I believe these changes are ready to go.
Just a heads up that these changes stack on top of each other,
and if taken through separate trees might break things a little until
they come back together.

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:
 - Removed erroneous fixup for USB UniPro PHY, which is not dual lane (Doug)

Changes in v2:
- Added dt bindings change, corresponding driver fixup, and USB PHY fixup
- Renamed ufsphy to phy (Vivek)
- Removed #clock-cells (Vivek)

Can Guo (1):
  arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

Evan Green (4):
  dt-bindings: phy-qcom-qmp: Fix register underspecification
  phy: qcom-qmp: Utilize fully-specified DT registers
  arm64: dts: qcom: sdm845: add UFS controller
  arm64: dts: qcom: sdm845: Add USB PHY lane two

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 70 ++++++++++++++++++---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts            | 14 +++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               | 71 +++++++++++++++++++++-
 drivers/phy/qualcomm/phy-qcom-qmp.c                | 51 ++++++++++++----
 4 files changed, 184 insertions(+), 22 deletions(-)

-- 
2.16.4

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

[Evan Green]

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(-)

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>;
+		};
+	};
-- 
2.16.4

[PATCH v5 2/5] phy: qcom-qmp: Utilize fully-specified DT registers

[Evan Green]

Utilize the newly fixed up DT bindings to get the tx2 and rx2 register
regions for the second lane of dual-lane PHYs. Before this change,
the driver was simply using lane one's register region and adding
0x400, which reached well beyond the DT-specified register
allocation. This would have been a crash were it not for the page size
on ARM64. Fix the driver not to rely on the magic of virtual memory by
using the newly specified DT register regions for tx2 and rx2.

In order to support existing device trees, this change also contains a
fallback mode for when those new register regions don't exist, which
reverts to the original behavior of overreaching and prints a complaint.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
As Doug mentioned, this should land before the dts patches land, otherwise
the old driver code will use the tx2 register region as pcs_misc.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/phy/qualcomm/phy-qcom-qmp.c | 51 +++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index a83332411026..61d5fe115d9d 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -72,6 +72,9 @@
 
 #define MAX_PROP_NAME				32
 
+/* Define the assumed distance between lanes for underspecified device trees. */
+#define QMP_PHY_LEGACY_LANE_STRIDE		0x400
+
 struct qmp_phy_init_tbl {
 	unsigned int offset;
 	unsigned int val;
@@ -733,9 +736,6 @@ struct qmp_phy_cfg {
 	bool has_phy_dp_com_ctrl;
 	/* true, if PHY has secondary tx/rx lanes to be configured */
 	bool is_dual_lane_phy;
-	/* Register offset of secondary tx/rx lanes for USB DP combo PHY */
-	unsigned int tx_b_lane_offset;
-	unsigned int rx_b_lane_offset;
 
 	/* true, if PCS block has no separate SW_RESET register */
 	bool no_pcs_sw_reset;
@@ -748,6 +748,8 @@ struct qmp_phy_cfg {
  * @tx: iomapped memory space for lane's tx
  * @rx: iomapped memory space for lane's rx
  * @pcs: iomapped memory space for lane's pcs
+ * @tx2: iomapped memory space for second lane's tx (in dual lane PHYs)
+ * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
  * @pcs_misc: iomapped memory space for lane's pcs_misc
  * @pipe_clk: pipe lock
  * @index: lane index
@@ -759,6 +761,8 @@ struct qmp_phy {
 	void __iomem *tx;
 	void __iomem *rx;
 	void __iomem *pcs;
+	void __iomem *tx2;
+	void __iomem *rx2;
 	void __iomem *pcs_misc;
 	struct clk *pipe_clk;
 	unsigned int index;
@@ -975,8 +979,6 @@ static const struct qmp_phy_cfg qmp_v3_usb3phy_cfg = {
 
 	.has_phy_dp_com_ctrl	= true,
 	.is_dual_lane_phy	= true,
-	.tx_b_lane_offset	= 0x400,
-	.rx_b_lane_offset	= 0x400,
 };
 
 static const struct qmp_phy_cfg qmp_v3_usb3_uniphy_cfg = {
@@ -1031,9 +1033,6 @@ static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
 	.mask_pcs_ready		= PCS_READY,
 
 	.is_dual_lane_phy	= true,
-	.tx_b_lane_offset	= 0x400,
-	.rx_b_lane_offset	= 0x400,
-
 	.no_pcs_sw_reset	= true,
 };
 
@@ -1238,12 +1237,12 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	qcom_qmp_phy_configure(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num);
 	/* Configuration for other LANE for USB-DP combo PHY */
 	if (cfg->is_dual_lane_phy)
-		qcom_qmp_phy_configure(tx + cfg->tx_b_lane_offset, cfg->regs,
+		qcom_qmp_phy_configure(qphy->tx2, cfg->regs,
 				       cfg->tx_tbl, cfg->tx_tbl_num);
 
 	qcom_qmp_phy_configure(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num);
 	if (cfg->is_dual_lane_phy)
-		qcom_qmp_phy_configure(rx + cfg->rx_b_lane_offset, cfg->regs,
+		qcom_qmp_phy_configure(qphy->rx2, cfg->regs,
 				       cfg->rx_tbl, cfg->rx_tbl_num);
 
 	qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
@@ -1614,8 +1613,9 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 
 	/*
 	 * Get memory resources for each phy lane:
-	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2; and
-	 * pcs_misc (optional) -> 3.
+	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
+	 * For dual lane PHYs: tx2 -> 3, rx2 -> 4, pcs_misc (optional) -> 5
+	 * For single lane PHYs: pcs_misc (optional) -> 3.
 	 */
 	qphy->tx = of_iomap(np, 0);
 	if (!qphy->tx)
@@ -1629,7 +1629,32 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 	if (!qphy->pcs)
 		return -ENOMEM;
 
-	qphy->pcs_misc = of_iomap(np, 3);
+	/*
+	 * If this is a dual-lane PHY, then there should be registers for the
+	 * second lane. Some old device trees did not specify this, so fall
+	 * back to old legacy behavior of assuming they can be reached at an
+	 * offset from the first lane.
+	 */
+	if (qmp->cfg->is_dual_lane_phy) {
+		qphy->tx2 = of_iomap(np, 3);
+		qphy->rx2 = of_iomap(np, 4);
+		if (!qphy->tx2 || !qphy->rx2) {
+			dev_warn(dev,
+				 "Underspecified device tree, falling back to legacy register regions\n");
+
+			/* In the old version, pcs_misc is at index 3. */
+			qphy->pcs_misc = qphy->tx2;
+			qphy->tx2 = qphy->tx + QMP_PHY_LEGACY_LANE_STRIDE;
+			qphy->rx2 = qphy->rx + QMP_PHY_LEGACY_LANE_STRIDE;
+
+		} else {
+			qphy->pcs_misc = of_iomap(np, 5);
+		}
+
+	} else {
+		qphy->pcs_misc = of_iomap(np, 3);
+	}
+
 	if (!qphy->pcs_misc)
 		dev_vdbg(dev, "PHY pcs_misc-reg not used\n");
 
-- 
2.16.4

[PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller

[Evan Green]

Add the UFS controller and PHY to SDM845.

Signed-off-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>

---
As Doug mentioned in v2, this should land after (or with) the driver fix
in this series.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Renamed ufsphy to phy (Vivek)
- Removed #clock-cells (Vivek)

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 67 ++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b72bdb0a31a5..9c72edb678ec 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -808,6 +808,73 @@
 			};
 		};
 
+		ufshc1: ufshc@1d84000 {
+			compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
+				     "jedec,ufs-2.0";
+			reg = <0x1d84000 0x2500>;
+			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
+			phys = <&ufsphy1_lanes>;
+			phy-names = "ufsphy";
+			lanes-per-direction = <2>;
+			power-domains = <&gcc UFS_PHY_GDSC>;
+
+			clock-names =
+				"core_clk",
+				"bus_aggr_clk",
+				"iface_clk",
+				"core_clk_unipro",
+				"ref_clk",
+				"tx_lane0_sync_clk",
+				"rx_lane0_sync_clk",
+				"rx_lane1_sync_clk";
+			clocks =
+				<&gcc GCC_UFS_PHY_AXI_CLK>,
+				<&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
+				<&gcc GCC_UFS_PHY_AHB_CLK>,
+				<&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>,
+				<&rpmhcc RPMH_CXO_CLK>,
+				<&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
+				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
+				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
+			freq-table-hz =
+				<50000000 200000000>,
+				<0 0>,
+				<0 0>,
+				<37500000 150000000>,
+				<0 0>,
+				<0 0>,
+				<0 0>,
+				<0 0>;
+
+			resets = <&gcc GCC_UFS_PHY_BCR>;
+			reset-names = "rst";
+
+			status = "disabled";
+		};
+
+		ufsphy1: 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>;
+
+			status = "disabled";
+
+			ufsphy1_lanes: lanes@1d87400 {
+				reg = <0x1d87400 0x108>,
+				      <0x1d87600 0x1e0>,
+				      <0x1d87c00 0x1dc>,
+				      <0x1d87800 0x108>,
+				      <0x1d87a00 0x1e0>;
+				#phy-cells = <0>;
+			};
+		};
+
 		tcsr_mutex_regs: syscon@1f40000 {
 			compatible = "syscon";
 			reg = <0x1f40000 0x40000>;
-- 
2.16.4

[PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Evan Green]

From: Can Guo <cang@codeaurora.org>

Enable the UFS host controller and PHY on sdm845-mtp.

Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>

---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index eedfaf8922e2..d5fddea71a85 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -356,6 +356,20 @@
 	status = "okay";
 };
 
+&ufshc1 {
+	status = "okay";
+
+	vcc-supply = <&vreg_l20a_2p95>;
+	vcc-max-microamp = <600000>;
+};
+
+&ufsphy1 {
+	status = "okay";
+
+	vdda-phy-supply = <&vdda_ufs1_core>;
+	vdda-pll-supply = <&vdda_ufs1_1p2>;
+};
+
 &usb_1 {
 	status = "okay";
 };
-- 
2.16.4

[PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two

[Evan Green]

Add the second lane registers for the USB PHY, now that the
QMP phy bindings have been updated. This way the driver can stop
reaching beyond its register region to get at the second lane.

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

Changes in v5: None
Changes in v4: None
Changes in v3:
 - Removed erroneous fixup for USB UniPro PHY, which is not dual lane (Doug)

Changes in v2: None

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 9c72edb678ec..ff2db36ec4fa 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1188,10 +1188,12 @@
 				 <&gcc GCC_USB3_PHY_PRIM_BCR>;
 			reset-names = "phy", "common";
 
-			usb_1_ssphy: lane@88e9200 {
+			usb_1_ssphy: lanes@88e9200 {
 				reg = <0x88e9200 0x128>,
 				      <0x88e9400 0x200>,
 				      <0x88e9c00 0x218>,
+				      <0x88e9600 0x128>,
+				      <0x88e9800 0x200>,
 				      <0x88e9a00 0x100>;
 				#phy-cells = <0>;
 				clocks = <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
-- 
2.16.4

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

[Stephen Boyd]

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?

> +               };
> +       };

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

[Doug Anderson]

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

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

[Stephen Boyd]

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.

Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Stephen Boyd]

Quoting Evan Green (2018-10-26 10:35:43)
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index eedfaf8922e2..d5fddea71a85 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -356,6 +356,20 @@
>         status = "okay";
>  };
>  
> +&ufshc1 {
> +       status = "okay";
> +
> +       vcc-supply = <&vreg_l20a_2p95>;
> +       vcc-max-microamp = <600000>;

Is this board dependent? I would guess this is SoC specific and not
board specific.

> +};
> +
> +&ufsphy1 {
> +       status = "okay";
> +
> +       vdda-phy-supply = <&vdda_ufs1_core>;
> +       vdda-pll-supply = <&vdda_ufs1_1p2>;

These two properties can be specified in the SoC dtsi file instead of
each board variant file. This way we don't have to specify the things
that are SoC independent in each board file. The board integrator just
has to attach the labels to the right regulator nodes, in this case
vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be
matched up with the right regulator automatically. It's also nice so
that board integrators don't have to know anything besides what
regulator goes to what pin on the SoC.

The status property has to stay of course.

Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Doug Anderson]

Hi,

On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2018-10-26 10:35:43)
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > index eedfaf8922e2..d5fddea71a85 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > @@ -356,6 +356,20 @@
> >         status = "okay";
> >  };
> >
> > +&ufshc1 {
> > +       status = "okay";
> > +
> > +       vcc-supply = <&vreg_l20a_2p95>;
> > +       vcc-max-microamp = <600000>;
>
> Is this board dependent? I would guess this is SoC specific and not
> board specific.
>
> > +};
> > +
> > +&ufsphy1 {
> > +       status = "okay";
> > +
> > +       vdda-phy-supply = <&vdda_ufs1_core>;
> > +       vdda-pll-supply = <&vdda_ufs1_1p2>;
>
> These two properties can be specified in the SoC dtsi file instead of
> each board variant file. This way we don't have to specify the things
> that are SoC independent in each board file. The board integrator just
> has to attach the labels to the right regulator nodes, in this case
> vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be
> matched up with the right regulator automatically. It's also nice so
> that board integrators don't have to know anything besides what
> regulator goes to what pin on the SoC.

This is an interesting proposal and it feels like we have to consider
the tradeoffs.

I agree that it would be nice not to have to specify this in every
single board .dts file, but at the same time what if you've got a
board that doesn't use UFS?  Such a board would bother adding the
labels "vdda_ufs1_core" and "vdda_ufs1_1p2".  This would lead to a
compile error in the device tree bindings.  That seems pretty
undesirable.

+Bjorn since I think Bjorn wasn't a huge fan of the labels like
"vdda_ufs1_core" to start with.


-Doug

Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Stephen Boyd]

Quoting Doug Anderson (2018-11-19 11:25:08)
> On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Evan Green (2018-10-26 10:35:43)
> >
> > > +};
> > > +
> > > +&ufsphy1 {
> > > +       status = "okay";
> > > +
> > > +       vdda-phy-supply = <&vdda_ufs1_core>;
> > > +       vdda-pll-supply = <&vdda_ufs1_1p2>;
> >
> > These two properties can be specified in the SoC dtsi file instead of
> > each board variant file. This way we don't have to specify the things
> > that are SoC independent in each board file. The board integrator just
> > has to attach the labels to the right regulator nodes, in this case
> > vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be
> > matched up with the right regulator automatically. It's also nice so
> > that board integrators don't have to know anything besides what
> > regulator goes to what pin on the SoC.
> 
> This is an interesting proposal and it feels like we have to consider
> the tradeoffs.
> 
> I agree that it would be nice not to have to specify this in every
> single board .dts file, but at the same time what if you've got a
> board that doesn't use UFS?  Such a board would bother adding the
> labels "vdda_ufs1_core" and "vdda_ufs1_1p2".  This would lead to a
> compile error in the device tree bindings.  That seems pretty
> undesirable.
> 

A workaround for this somewhat rare case would be to specify
/delete-property/ on those nodes that aren't used. Unless that can't
even work because the phandle is parsed before properties are deleted? I
haven't tried. Or we could try to have dtc ignore broken phandles in
status = "disabled" nodes or omit them entirely from the dtb so this
isn't a problem.

Either way, I would push for making it easier for the users of the SoC
to not need to know the SoC internal details too much and rely on the
SoC dtsi file to get it right. From the user perspective it's just a
bunch of pins connected to something. We could also have a 0 volt
"ground" regulator for any grounded/unconnected pins if that helps. It
could be marked as status = "disabled" so that no runtime code is used
while dtc is still happy to have the disabled node with a label.

Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Bjorn Andersson]

On Mon 19 Nov 11:42 PST 2018, Stephen Boyd wrote:

> Quoting Doug Anderson (2018-11-19 11:25:08)
> > On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Evan Green (2018-10-26 10:35:43)
> > >
> > > > +};
> > > > +
> > > > +&ufsphy1 {
> > > > +       status = "okay";
> > > > +
> > > > +       vdda-phy-supply = <&vdda_ufs1_core>;
> > > > +       vdda-pll-supply = <&vdda_ufs1_1p2>;
> > >
> > > These two properties can be specified in the SoC dtsi file instead of
> > > each board variant file. This way we don't have to specify the things
> > > that are SoC independent in each board file. The board integrator just
> > > has to attach the labels to the right regulator nodes, in this case
> > > vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be
> > > matched up with the right regulator automatically. It's also nice so
> > > that board integrators don't have to know anything besides what
> > > regulator goes to what pin on the SoC.
> > 
> > This is an interesting proposal and it feels like we have to consider
> > the tradeoffs.
> > 
> > I agree that it would be nice not to have to specify this in every
> > single board .dts file, but at the same time what if you've got a
> > board that doesn't use UFS?  Such a board would bother adding the
> > labels "vdda_ufs1_core" and "vdda_ufs1_1p2".  This would lead to a
> > compile error in the device tree bindings.  That seems pretty
> > undesirable.
> > 
> 
> A workaround for this somewhat rare case would be to specify
> /delete-property/ on those nodes that aren't used. Unless that can't
> even work because the phandle is parsed before properties are deleted? I
> haven't tried. Or we could try to have dtc ignore broken phandles in
> status = "disabled" nodes or omit them entirely from the dtb so this
> isn't a problem.
> 
> Either way, I would push for making it easier for the users of the SoC
> to not need to know the SoC internal details too much and rely on the
> SoC dtsi file to get it right. From the user perspective it's just a
> bunch of pins connected to something. We could also have a 0 volt
> "ground" regulator for any grounded/unconnected pins if that helps. It
> could be marked as status = "disabled" so that no runtime code is used
> while dtc is still happy to have the disabled node with a label.
> 

I dislike the use of the labels "vdda_ufs1_core" as that doesn't tell me
which regulator this is actually wired to, without having to jump around
in the board dts file. It's annoying, but it's pretty much write-once so
it's not a big issue.

But I really don't like the idea of having sdm845.dtsi depend on labels
specified in the board dtsi. This just means that in order to add a new
board I need to figure out the information and the way to specify it is
to change a label in the board file.


The static configuration here is that vdda-phy-supply is connected to
the vdda_ufs1_core pin on the SoC, which is connected to some board
specific regulator.  If anything I think we should model that
intermediate entity, in which case we could move the link between the
phy and the pin to the platform dtsi.


But I'm fine with us just merging Evan's patch as is.

Regards,
Bjorn

Re: [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two

[Bjorn Andersson]

On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote:

> Add the second lane registers for the USB PHY, now that the
> QMP phy bindings have been updated. This way the driver can stop
> reaching beyond its register region to get at the second lane.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3:
>  - Removed erroneous fixup for USB UniPro PHY, which is not dual lane (Doug)
> 
> Changes in v2: None
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 9c72edb678ec..ff2db36ec4fa 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1188,10 +1188,12 @@
>  				 <&gcc GCC_USB3_PHY_PRIM_BCR>;
>  			reset-names = "phy", "common";
>  
> -			usb_1_ssphy: lane@88e9200 {
> +			usb_1_ssphy: lanes@88e9200 {
>  				reg = <0x88e9200 0x128>,
>  				      <0x88e9400 0x200>,
>  				      <0x88e9c00 0x218>,
> +				      <0x88e9600 0x128>,
> +				      <0x88e9800 0x200>,
>  				      <0x88e9a00 0x100>;
>  				#phy-cells = <0>;
>  				clocks = <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> -- 
> 2.16.4
> 

Re: [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller

[Bjorn Andersson]

On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote:
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b72bdb0a31a5..9c72edb678ec 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -808,6 +808,73 @@
>  			};
>  		};
>  
> +		ufshc1: ufshc@1d84000 {

There's only one ufshc and one ufsphy, so no need to include the index.

[..]
> +			resets = <&gcc GCC_UFS_PHY_BCR>;
> +			reset-names = "rst";

I have this as well, but this is not used by the upstream driver nor is
it mentioned in the dt-binding.

> +
> +			status = "disabled";
> +		};
> +
> +		ufsphy1: phy@1d87000 {

With reservation for the "reset" issue:

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp

[Stephen Boyd]

Quoting Bjorn Andersson (2018-11-21 23:04:12)
> On Mon 19 Nov 11:42 PST 2018, Stephen Boyd wrote:
> > 
> > A workaround for this somewhat rare case would be to specify
> > /delete-property/ on those nodes that aren't used. Unless that can't
> > even work because the phandle is parsed before properties are deleted? I
> > haven't tried. Or we could try to have dtc ignore broken phandles in
> > status = "disabled" nodes or omit them entirely from the dtb so this
> > isn't a problem.
> > 
> > Either way, I would push for making it easier for the users of the SoC
> > to not need to know the SoC internal details too much and rely on the
> > SoC dtsi file to get it right. From the user perspective it's just a
> > bunch of pins connected to something. We could also have a 0 volt
> > "ground" regulator for any grounded/unconnected pins if that helps. It
> > could be marked as status = "disabled" so that no runtime code is used
> > while dtc is still happy to have the disabled node with a label.
> > 
> 
> I dislike the use of the labels "vdda_ufs1_core" as that doesn't tell me
> which regulator this is actually wired to, without having to jump around
> in the board dts file. It's annoying, but it's pretty much write-once so
> it's not a big issue.
> 
> But I really don't like the idea of having sdm845.dtsi depend on labels
> specified in the board dtsi. This just means that in order to add a new
> board I need to figure out the information and the way to specify it is
> to change a label in the board file.
> 
> 
> The static configuration here is that vdda-phy-supply is connected to
> the vdda_ufs1_core pin on the SoC, which is connected to some board
> specific regulator.  If anything I think we should model that
> intermediate entity, in which case we could move the link between the
> phy and the pin to the platform dtsi.
> 

Hmm alright. This is a similar problem that the DT connectors have with
phandle remapping through the connector to the actual phandle that is
desired. I can think of two solutions. One would be to make a
phandle-map binding to remap phandles within the soc node to actually be
another phandle. The downside is that we need to make nodes for
everything just to remap them. For example:


	soc {
		phandle-map {
			vdda_ufs1_core: vdda-ufs1-core {
			}
		};
	};

In the SoC dtsi file and then

	&vdda_ufs1_core {
		phandle-alias = <&pm8998_ldo19>
	};

Would be in the board specific dtsi file.

The regulator code to fix that up is rather simple, but not generic. It
just walks the chain of alias nodes until it doesn't find anything else.

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 210fc20f7de7..9c1346374351 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -432,6 +432,15 @@ static int of_node_match(struct device *dev, const void *data)
 struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
 {
 	struct device *dev;
+	struct device_node *aliased_np;
+
+	do {
+		aliased_np = of_parse_phandle(np, "phandle-alias", 0);
+
+		if (aliased_np)
+			np = aliased_np;
+
+	} while (aliased_np);
 
 	dev = class_find_device(&regulator_class, NULL, np, of_node_match);
 
Another idea would be to have a phandle nexus node that converts
phandles into some #cells property. This way, we can make the regulator
supply binding parse the cells of the nexus node and figure out that the
next specifier after it corresponds to something that needs to be set in
the nexus node by the board.

	soc {
		soc_regulator: regulator-nexus {
			#regulator-cells = <1>;
			#define VDDA_UFS1_CORE 0
		};

		ufs {
			vdda-supply = <&soc_regulator VDDA_UFS1_CORE>;

		};
	};

In the SoC dtsi file and then

	&soc_regulator {
		regulator-map = <VDDA_UFS1_CORE &pm8998_ldo19>
	}

And then some parsing code in regulator core to remap the cells on the
left side to the phandle on the right of the cells. I suppose that makes
things sort of awkward and makes the binding look different depending on
if the node has the #regulator-cells property or not for the supply
binding.

Another idea would be to have dts phandle fixups applied somehow when
the DT is loaded by the kernel or possibly bootloader or extra
efficiently with a compiler change. So then we can alias labels to
different labels in the board file.

So in the board dtsi file we would have something like

	soc {
		phandle-map {
			vdda-ufs1-core = <&pm8998_ldo19>;
		};
	};   

and the SoC dtsi file would have:

	soc {
		phandle-map {
			vdda_ufs1_core: vdda-ufs1-core = <0>;
		};
	};

except I can't get this to compile right now because syntax errors.

Re: [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller

[Evan Green]

On Wed, Nov 21, 2018 at 11:18 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index b72bdb0a31a5..9c72edb678ec 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -808,6 +808,73 @@
> >                       };
> >               };
> >
> > +             ufshc1: ufshc@1d84000 {
>
> There's only one ufshc and one ufsphy, so no need to include the index.

Aren't there two UFS controllers on SDM845, a "card" one and a "mem"
one? I'm only adding the "mem" one here since that's all I can test,
but I thought it made sense to leave the number there so someone could
add the "card" one later if needed.
>
> [..]
> > +                     resets = <&gcc GCC_UFS_PHY_BCR>;
> > +                     reset-names = "rst";
>
> I have this as well, but this is not used by the upstream driver nor is
> it mentioned in the dt-binding.

I see it in Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt,
but then the only place I see it being used is ufs-hisi.c. So you're
right, I think I should spin and remove this. Since I'm spinning, let
me know about the numbering thing above.


>
> > +
> > +                     status = "disabled";
> > +             };
> > +
> > +             ufsphy1: phy@1d87000 {
>
> With reservation for the "reset" issue:
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Regards,
> Bjorn