[PATCH v2 0/2] USB SS PHY for Qualcomm's QCS404

[PATCH v2 0/2] USB SS PHY for Qualcomm's QCS404

[Jorge Ramirez-Ortiz]

This set adds USB SS PHY support to Qualcomm's QCS404 SoC
The PHY is implemented using Synopsys SS PHY 1.0.0 IP

The code is losely based on Sriharsha Allenki's
<sallenki@codeaurora.org> original implementation.

v2:
  enable OTG mode detection
  move vdd voltage levels to driver
  use bulk_ control interfaces
  ss-phy-bindings [1]

[1] ss-phy-binding discussion:
 - qcom,dwc3-ss-usb-phy exist for a generic usb2/usb3 phy driver that
 was never merged. Rather than trying to re-use these bindings (or
 delete them) I  propose that we go ahead with the new separate
 bindings for HS and SS: if  not now - investigation  in progress- in
 the  future  it might be  possible to have again a common phy driver
 for  which these old  bindings would be the binding agreement. 
    
Jorge Ramirez-Ortiz (2):
  dt-bindings: Add Qualcomm USB Super-Speed PHY bindings
  phy: qualcomm: usb: Add Super-Speed PHY driver

 .../devicetree/bindings/usb/qcom,usb-ssphy.txt     |  73 +++++
 drivers/phy/qualcomm/Kconfig                       |  11 +
 drivers/phy/qualcomm/Makefile                      |   1 +
 drivers/phy/qualcomm/phy-qcom-usb-ss.c             | 347 +++++++++++++++++++++
 4 files changed, 432 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
 create mode 100644 drivers/phy/qualcomm/phy-qcom-usb-ss.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez-Ortiz]

Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
controller embedded in QCS404.

Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
definitions.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt

diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
new file mode 100644
index 0000000..8ef6e39
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
@@ -0,0 +1,73 @@
+Qualcomm Synopsys 1.0.0 SS phy controller
+===========================================
+
+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
+chipsets
+
+Required properties:
+
+- compatible:
+    Value type: <string>
+    Definition: Should contain "qcom,usb-ssphy".
+
+- reg:
+    Value type: <prop-encoded-array>
+    Definition: USB PHY base address and length of the register map.
+
+- #phy-cells:
+    Value type: <u32>
+    Definition: Should be 0. See phy/phy-bindings.txt for details.
+
+- clocks:
+    Value type: <prop-encoded-array>
+    Definition: See clock-bindings.txt section "consumers". List of
+		 three clock specifiers for reference, phy core and
+		 pipe clocks.
+
+- clock-names:
+    Value type: <string>
+    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
+		 property. Must contain "ref", "phy" and "pipe".
+
+- vdd-supply:
+    Value type: <phandle>
+    Definition: phandle to the regulator VDD supply node.
+
+- vdda1p8-supply:
+    Value type: <phandle>
+    Definition: phandle to the regulator 1.8V supply node.
+
+
+Optional child nodes:
+
+- vbus-supply:
+    Value type: <phandle>
+    Definition: phandle to the VBUS supply node.
+
+- resets:
+    Value type: <prop-encoded-array>
+    Definition: See reset.txt section "consumers". PHY reset specifiers
+		 for phy core and COR resets.
+
+- reset-names:
+    Value type: <string>
+    Definition: Names of the resets in 1-1 correspondence with the "resets"
+		 property. Must contain "com" and "phy".
+
+Example:
+
+usb3_phy: phy@78000 {
+	compatible = "qcom,usb-ssphy";
+	reg = <0x78000 0x400>;
+	#phy-cells = <0>;
+	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
+		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
+		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
+	clock-names = "ref", "phy", "pipe";
+	resets = <&gcc GCC_USB3_PHY_BCR>,
+		 <&gcc GCC_USB3PHY_PHY_BCR>;
+	reset-names = "com", "phy";
+	vdd-supply = <&vreg_l3_1p05>;
+	vdda1p8-supply = <&vreg_l5_1p8>;
+	vbus-supply = <&usb3_vbus_reg>;
+};
-- 
2.7.4

[PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Jorge Ramirez-Ortiz]

Driver to control the Synopsys SS PHY 1.0.0 implemeneted in QCS404
Based on Sriharsha Allenki's <sallenki@codeaurora.org> original code.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/phy/qualcomm/Kconfig           |  11 ++
 drivers/phy/qualcomm/Makefile          |   1 +
 drivers/phy/qualcomm/phy-qcom-usb-ss.c | 347 +++++++++++++++++++++++++++++++++
 3 files changed, 359 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-usb-ss.c

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index c7b5ee8..35a5a67 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -92,3 +92,14 @@ config PHY_QCOM_USB_HS_SNPS_28NM
 	  Enable this to support the Synopsys 28nm Femto USB PHY on Qualcomm
 	  chips. This driver supports the high-speed PHY which is usually
 	  paired with either the ChipIdea or Synopsys DWC3 USB IPs on MSM SOCs.
+
+config PHY_QCOM_USB_SS
+	tristate "Qualcomm USB SS PHY driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on EXTCON || !EXTCON # if EXTCON=m, this cannot be built-in
+	select GENERIC_PHY
+	help
+	  Enable this to support the Super-Speed USB transceiver on Qualcomm
+	  chips. This driver supports the PHY which uses the QSCRATCH-based
+	  register set for its control sequences, normally paired with newer
+	  DWC3-based Super-Speed controllers on Qualcomm SoCs.
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index dc238d9..7149261 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_PHY_QCOM_UFS_20NM)		+= phy-qcom-ufs-qmp-20nm.o
 obj-$(CONFIG_PHY_QCOM_USB_HS) 		+= phy-qcom-usb-hs.o
 obj-$(CONFIG_PHY_QCOM_USB_HSIC) 	+= phy-qcom-usb-hsic.o
 obj-$(CONFIG_PHY_QCOM_USB_HS_SNPS_28NM)	+= phy-qcom-usb-hs-snsp-28nm.o
+obj-$(CONFIG_PHY_QCOM_USB_SS)		+= phy-qcom-usb-ss.o
diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
new file mode 100644
index 0000000..e6ae96e
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define PHY_CTRL0			0x6C
+#define PHY_CTRL1			0x70
+#define PHY_CTRL2			0x74
+#define PHY_CTRL4			0x7C
+
+/* PHY_CTRL bits */
+#define REF_PHY_EN			BIT(0)
+#define LANE0_PWR_ON			BIT(2)
+#define SWI_PCS_CLK_SEL			BIT(4)
+#define TST_PWR_DOWN			BIT(4)
+#define PHY_RESET			BIT(7)
+
+enum phy_vdd_ctrl { ENABLE, DISABLE, };
+enum phy_regulator { VDD, VDDA1P8, };
+
+struct ssphy_priv {
+	void __iomem *base;
+	struct device *dev;
+	struct reset_control *reset_com;
+	struct reset_control *reset_phy;
+	struct regulator *vbus;
+	struct regulator_bulk_data *regs;
+	int num_regs;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	enum phy_mode mode;
+};
+
+static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val)
+{
+	writel((readl(addr) & ~mask) | val, addr);
+}
+
+static inline int qcom_ssphy_vbus_enable(struct regulator *vbus)
+{
+	return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0;
+}
+
+static inline int qcom_ssphy_vbus_disable(struct regulator *vbus)
+{
+	return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0;
+}
+
+static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl ctrl)
+{
+	const int vdd_min = ctrl == ENABLE ? 1050000 : 0;
+	const int vdd_max = 1050000;
+	int ret;
+
+	ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max);
+	if (ret)
+		dev_err(priv->dev, "Failed to set regulator vdd to %d\n",
+			vdd_min);
+
+	return ret;
+}
+
+static int qcom_ssphy_vbus_ctrl(struct regulator *vbus, enum phy_mode mode)
+{
+	if (!vbus)
+		return 0;
+
+	if (mode == PHY_MODE_INVALID)
+		return 0;
+
+	/* gadget attached */
+	if (mode == PHY_MODE_USB_HOST)
+		return qcom_ssphy_vbus_enable(vbus);
+
+	/* USB_DEVICE: gadget removed: enable detection */
+	return qcom_ssphy_vbus_disable(vbus);
+}
+
+static int qcom_ssphy_do_reset(struct ssphy_priv *priv)
+{
+	int ret;
+
+	if (!priv->reset_com) {
+		qcom_ssphy_updatel(priv->base + PHY_CTRL1, PHY_RESET,
+				   PHY_RESET);
+		usleep_range(10, 20);
+		qcom_ssphy_updatel(priv->base + PHY_CTRL1, PHY_RESET, 0);
+	} else {
+		ret = reset_control_assert(priv->reset_com);
+		if (ret) {
+			dev_err(priv->dev, "Failed to assert reset com\n");
+			return ret;
+		}
+
+		ret = reset_control_assert(priv->reset_phy);
+		if (ret) {
+			dev_err(priv->dev, "Failed to assert reset phy\n");
+			return ret;
+		}
+
+		usleep_range(10, 20);
+
+		ret = reset_control_deassert(priv->reset_com);
+		if (ret) {
+			dev_err(priv->dev, "Failed to deassert reset com\n");
+			return ret;
+		}
+
+		ret = reset_control_deassert(priv->reset_phy);
+		if (ret) {
+			dev_err(priv->dev, "Failed to deassert reset phy\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_ssphy_power_on(struct phy *phy)
+{
+	struct ssphy_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = qcom_ssphy_vdd_ctrl(priv, ENABLE);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(priv->num_regs, priv->regs);
+	if (ret)
+		goto err1;
+
+	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+	if (ret)
+		goto err2;
+
+	ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode);
+	if (ret)
+		goto err3;
+
+	ret = qcom_ssphy_do_reset(priv);
+	if (ret)
+		goto err4;
+
+	writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0);
+
+	return 0;
+err4:
+	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
+		qcom_ssphy_vbus_disable(priv->vbus);
+err3:
+	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+err2:
+	regulator_bulk_disable(priv->num_regs, priv->regs);
+err1:
+	qcom_ssphy_vdd_ctrl(priv, DISABLE);
+
+	return ret;
+}
+
+static int qcom_ssphy_power_off(struct phy *phy)
+{
+	struct ssphy_priv *priv = phy_get_drvdata(phy);
+
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN);
+
+	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+	regulator_bulk_disable(priv->num_regs, priv->regs);
+
+	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
+		qcom_ssphy_vbus_disable(priv->vbus);
+
+	qcom_ssphy_vdd_ctrl(priv, DISABLE);
+
+	return 0;
+}
+
+static int qcom_ssphy_init_clock(struct ssphy_priv *priv)
+{
+	const char * const clk_id[] = { "ref", "phy", "pipe", };
+	int i;
+
+	priv->num_clks = ARRAY_SIZE(clk_id);
+	priv->clks = devm_kcalloc(priv->dev, priv->num_clks,
+				  sizeof(*priv->clks), GFP_KERNEL);
+	if (!priv->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->num_clks; i++)
+		priv->clks[i].id = clk_id[i];
+
+	return devm_clk_bulk_get(priv->dev, priv->num_clks, priv->clks);
+}
+
+static int qcom_ssphy_init_regulator(struct ssphy_priv *priv)
+{
+	const char * const reg_supplies[] = {
+		[VDD] = "vdd",
+		[VDDA1P8] = "vdda1p8",
+	};
+	int ret, i;
+
+	priv->num_regs = ARRAY_SIZE(reg_supplies);
+	priv->regs = devm_kcalloc(priv->dev, priv->num_regs,
+				  sizeof(*priv->regs), GFP_KERNEL);
+	if (!priv->regs)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->num_regs; i++)
+		priv->regs[i].supply = reg_supplies[i];
+
+	ret = devm_regulator_bulk_get(priv->dev, priv->num_regs, priv->regs);
+	if (ret)
+		return ret;
+
+	priv->vbus = devm_regulator_get_optional(priv->dev, "vbus");
+	if (IS_ERR(priv->vbus))
+		return PTR_ERR(priv->vbus);
+
+	return 0;
+}
+
+static int qcom_ssphy_init_reset(struct ssphy_priv *priv)
+{
+	priv->reset_com = devm_reset_control_get_optional(priv->dev, "com");
+	if (IS_ERR(priv->reset_com)) {
+		dev_err(priv->dev, "Failed to get reset control com\n");
+		return PTR_ERR(priv->reset_com);
+	}
+
+	if (priv->reset_com) {
+		/* if reset_com is present, reset_phy is no longer optional */
+		priv->reset_phy = devm_reset_control_get(priv->dev, "phy");
+		if (IS_ERR(priv->reset_phy)) {
+			dev_err(priv->dev, "Failed to get reset control phy\n");
+			return PTR_ERR(priv->reset_phy);
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_ssphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct ssphy_priv *priv = phy_get_drvdata(phy);
+
+	if (!priv->vbus)
+		return 0;
+
+	if (mode != PHY_MODE_USB_HOST && mode != PHY_MODE_USB_DEVICE)
+		return -EINVAL;
+
+	priv->mode = mode;
+
+	dev_dbg(priv->dev, "mode %d", mode);
+
+	return qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode);
+}
+
+static const struct phy_ops qcom_ssphy_ops = {
+	.set_mode = qcom_ssphy_set_mode,
+	.power_off = qcom_ssphy_power_off,
+	.power_on = qcom_ssphy_power_on,
+	.owner = THIS_MODULE,
+};
+
+static int qcom_ssphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *provider;
+	struct ssphy_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct ssphy_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->mode = PHY_MODE_INVALID;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	ret = qcom_ssphy_init_clock(priv);
+	if (ret)
+		return ret;
+
+	ret = qcom_ssphy_init_reset(priv);
+	if (ret)
+		return ret;
+
+	ret = qcom_ssphy_init_regulator(priv);
+	if (ret)
+		return ret;
+
+	phy = devm_phy_create(dev, dev->of_node, &qcom_ssphy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create the SS phy\n");
+		return PTR_ERR(phy);
+	}
+
+	phy_set_drvdata(phy, priv);
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(provider);
+}
+
+static const struct of_device_id qcom_ssphy_match[] = {
+	{ .compatible = "qcom,usb-ssphy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_ssphy_match);
+
+static struct platform_driver qcom_ssphy_driver = {
+	.probe		= qcom_ssphy_probe,
+	.driver = {
+		.name	= "qcom_usb_ssphy",
+		.of_match_table = qcom_ssphy_match,
+	},
+};
+module_platform_driver(qcom_ssphy_driver);
+
+MODULE_DESCRIPTION("Qualcomm Super-Speed USB PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Bjorn Andersson]

On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote:
> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
> new file mode 100644
> index 0000000..e6ae96e
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2018, Linaro Limited
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define PHY_CTRL0			0x6C
> +#define PHY_CTRL1			0x70
> +#define PHY_CTRL2			0x74
> +#define PHY_CTRL4			0x7C
> +
> +/* PHY_CTRL bits */
> +#define REF_PHY_EN			BIT(0)
> +#define LANE0_PWR_ON			BIT(2)
> +#define SWI_PCS_CLK_SEL			BIT(4)
> +#define TST_PWR_DOWN			BIT(4)
> +#define PHY_RESET			BIT(7)
> +
> +enum phy_vdd_ctrl { ENABLE, DISABLE, };

Use bool to describe boolean values.

> +enum phy_regulator { VDD, VDDA1P8, };

It doesn't look like you need either of these if you remove the
set_voltage calls.

> +
> +struct ssphy_priv {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct reset_control *reset_com;
> +	struct reset_control *reset_phy;
> +	struct regulator *vbus;
> +	struct regulator_bulk_data *regs;
> +	int num_regs;
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +	enum phy_mode mode;
> +};
> +
> +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val)
> +{
> +	writel((readl(addr) & ~mask) | val, addr);
> +}
> +
> +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus)
> +{
> +	return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0;

regulator_is_enabled() will check if the actual regulator is on, not if
you already voted for it.

So if something else is enabling the vbus regulator you will skip your
enable and be in the mercy of them not releasing it. Presumably there's
only one consumer of this particular regulator, but it's a bad habit.

Please keep track of this drivers requested state in this driver, if
qcom_ssphy_vbus_ctrl() is called not only for the state changes.

> +}
> +
> +static inline int qcom_ssphy_vbus_disable(struct regulator *vbus)
> +{
> +	return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0;
> +}
> +
> +static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl ctrl)

As discussed on IRC, I think you should just leave the voltage
constraints to DeviceTree.

> +{
> +	const int vdd_min = ctrl == ENABLE ? 1050000 : 0;
> +	const int vdd_max = 1050000;
> +	int ret;
> +
> +	ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max);
> +	if (ret)
> +		dev_err(priv->dev, "Failed to set regulator vdd to %d\n",
> +			vdd_min);
> +
> +	return ret;
> +}
[..]
> +static int qcom_ssphy_power_on(struct phy *phy)
> +{
> +	struct ssphy_priv *priv = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = qcom_ssphy_vdd_ctrl(priv, ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_bulk_enable(priv->num_regs, priv->regs);
> +	if (ret)
> +		goto err1;
> +
> +	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> +	if (ret)
> +		goto err2;
> +
> +	ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode);
> +	if (ret)
> +		goto err3;
> +
> +	ret = qcom_ssphy_do_reset(priv);
> +	if (ret)
> +		goto err4;
> +
> +	writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0);
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON);
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN);
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0);
> +
> +	return 0;
> +err4:

Name your labels based on what they do, e.g. err_disable_vbus.

> +	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
> +		qcom_ssphy_vbus_disable(priv->vbus);

qcom_ssphy_vbus_ctrl() above was either enabling or disabling vbus, but
here you're directly calling disable to unroll that. It think the result
is correct (in host this reverts to disabled and in gadget it's a
no-op), but I'm not sure I like the design of sometimes calling straight
to the vbus enable/disable and sometimes to the wrapper function.

> +err3:

err_clk_disable

> +	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> +err2:
> +	regulator_bulk_disable(priv->num_regs, priv->regs);
> +err1:
> +	qcom_ssphy_vdd_ctrl(priv, DISABLE);
> +
> +	return ret;
> +}
> +
> +static int qcom_ssphy_power_off(struct phy *phy)
> +{
> +	struct ssphy_priv *priv = phy_get_drvdata(phy);
> +
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0);
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0);
> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN);
> +
> +	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> +	regulator_bulk_disable(priv->num_regs, priv->regs);
> +
> +	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
> +		qcom_ssphy_vbus_disable(priv->vbus);
> +
> +	qcom_ssphy_vdd_ctrl(priv, DISABLE);
> +
> +	return 0;
> +}
> +
> +static int qcom_ssphy_init_clock(struct ssphy_priv *priv)
> +{
> +	const char * const clk_id[] = { "ref", "phy", "pipe", };
> +	int i;
> +
> +	priv->num_clks = ARRAY_SIZE(clk_id);
> +	priv->clks = devm_kcalloc(priv->dev, priv->num_clks,
> +				  sizeof(*priv->clks), GFP_KERNEL);

You know num_clks is 3, so I would suggest that you just change the
sshphy_priv to clks[3] and skip the dynamic allocation.


Also, as num_clks always is ARRAY_SIZE(priv->clks) I would suggest using
the latter, to make that clear throughout the driver.

> +	if (!priv->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->num_clks; i++)
> +		priv->clks[i].id = clk_id[i];

There's no harm in just writing this on three lines:

	priv->clks[0].id = "ref";
	priv->clks[1].id = "phy";
	priv->clks[2].id = "pipe";

> +
> +	return devm_clk_bulk_get(priv->dev, priv->num_clks, priv->clks);
> +}
> +
> +static int qcom_ssphy_init_regulator(struct ssphy_priv *priv)
> +{
> +	const char * const reg_supplies[] = {
> +		[VDD] = "vdd",
> +		[VDDA1P8] = "vdda1p8",
> +	};
> +	int ret, i;
> +
> +	priv->num_regs = ARRAY_SIZE(reg_supplies);
> +	priv->regs = devm_kcalloc(priv->dev, priv->num_regs,
> +				  sizeof(*priv->regs), GFP_KERNEL);

As with clocks, you know there will only be 2 of these, make it fixed
size in ssphy_priv.

And as with clocks, I would suggest using ARRAY_SIZE(priv->regs)
throughout the driver to make it obvious that's it's a static number. 

> +	if (!priv->regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->num_regs; i++)
> +		priv->regs[i].supply = reg_supplies[i];

As with clocks, just unroll this and fill in the 2 supplies directly.

> +
> +	ret = devm_regulator_bulk_get(priv->dev, priv->num_regs, priv->regs);
> +	if (ret)
> +		return ret;
> +
> +	priv->vbus = devm_regulator_get_optional(priv->dev, "vbus");

get_optional means that if vbus-supply is not found, rather than
returning a dummy regulator object this will fail with -ENODEV.

Given the rest of the logic related to vbus you should set vbus to NULL
if the returned PTR_ERR(value) is -ENODEV, and fail for other errors.


Or just drop the _optional, and let your vbus controls operate on the
dummy regulator you get back.

(Right now vbus can't be NULL, so these checks are not very useful)

> +	if (IS_ERR(priv->vbus))
> +		return PTR_ERR(priv->vbus);
> +
> +	return 0;

return PTR_ERR_OR_ZERO(priv->vbus)

(Although that might change based on above comment)

> +}

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Bjorn Andersson]

On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote:

> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY

SuperSpeed

> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> new file mode 100644
> index 0000000..8ef6e39
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> @@ -0,0 +1,73 @@
> +Qualcomm Synopsys 1.0.0 SS phy controller
> +===========================================
> +
> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> +chipsets

It's based on Synopsys IP, but it's Qualcomm's version, and isn't the
1.0.0 Qualcomm's version number for this block?

Also I think it "provides SuperSpeed USB connectivity on some Qualcomm
platforms".

> +
> +Required properties:
> +
> +- compatible:
> +    Value type: <string>
> +    Definition: Should contain "qcom,usb-ssphy".
> +
> +- reg:
> +    Value type: <prop-encoded-array>
> +    Definition: USB PHY base address and length of the register map.
> +
> +- #phy-cells:
> +    Value type: <u32>
> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
> +
> +- clocks:
> +    Value type: <prop-encoded-array>
> +    Definition: See clock-bindings.txt section "consumers". List of
> +		 three clock specifiers for reference, phy core and
> +		 pipe clocks.
> +
> +- clock-names:
> +    Value type: <string>
> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
> +		 property. Must contain "ref", "phy" and "pipe".
> +
> +- vdd-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator VDD supply node.
> +
> +- vdda1p8-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator 1.8V supply node.
> +
> +
> +Optional child nodes:
> +
> +- vbus-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the VBUS supply node.
> +
> +- resets:
> +    Value type: <prop-encoded-array>
> +    Definition: See reset.txt section "consumers". PHY reset specifiers
> +		 for phy core and COR resets.
> +
> +- reset-names:
> +    Value type: <string>
> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
> +		 property. Must contain "com" and "phy".

Perhaps "Must contain both com and phy, if property is specified", to
clarify that it's all or nothing.


Looks good otherwise.

Regards,
Bjorn

Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Jorge Ramirez]

On 1/29/19 21:27, Bjorn Andersson wrote:
> On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote:
>> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
>> new file mode 100644
>> index 0000000..e6ae96e
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
>> @@ -0,0 +1,347 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2018, Linaro Limited
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#define PHY_CTRL0			0x6C
>> +#define PHY_CTRL1			0x70
>> +#define PHY_CTRL2			0x74
>> +#define PHY_CTRL4			0x7C
>> +
>> +/* PHY_CTRL bits */
>> +#define REF_PHY_EN			BIT(0)
>> +#define LANE0_PWR_ON			BIT(2)
>> +#define SWI_PCS_CLK_SEL			BIT(4)
>> +#define TST_PWR_DOWN			BIT(4)
>> +#define PHY_RESET			BIT(7)
>> +
>> +enum phy_vdd_ctrl { ENABLE, DISABLE, };
> 
> Use bool to describe boolean values.

um, ok, but these are not booleans, they are actions (ie ctrl = action
not true or false).

anyway will change it to something else

> 
>> +enum phy_regulator { VDD, VDDA1P8, };
> 
> It doesn't look like you need either of these if you remove the
> set_voltage calls.

we need to point to the regulator in the array so we need an index to it
somehow.

> 
>> +
>> +struct ssphy_priv {
>> +	void __iomem *base;
>> +	struct device *dev;
>> +	struct reset_control *reset_com;
>> +	struct reset_control *reset_phy;
>> +	struct regulator *vbus;
>> +	struct regulator_bulk_data *regs;
>> +	int num_regs;
>> +	struct clk_bulk_data *clks;
>> +	int num_clks;
>> +	enum phy_mode mode;
>> +};
>> +
>> +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val)
>> +{
>> +	writel((readl(addr) & ~mask) | val, addr);
>> +}
>> +
>> +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus)
>> +{
>> +	return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0;
> 
> regulator_is_enabled() will check if the actual regulator is on, not if
> you already voted for it.
> 
> So if something else is enabling the vbus regulator you will skip your
> enable and be in the mercy of them not releasing it. Presumably there's
> only one consumer of this particular regulator, but it's a bad habit.

yep

> 
> Please keep track of this drivers requested state in this driver, if
> qcom_ssphy_vbus_ctrl() is called not only for the state changes.

um, there is not such a function: the ctrl function is not for vbus but
for vdd

> 
>> +}
>> +
>> +static inline int qcom_ssphy_vbus_disable(struct regulator *vbus)
>> +{
>> +	return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0;
>> +}
>> +
>> +static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl ctrl)
> 
> As discussed on IRC, I think you should just leave the voltage
> constraints to DeviceTree.

yes

> 
>> +{
>> +	const int vdd_min = ctrl == ENABLE ? 1050000 : 0;
>> +	const int vdd_max = 1050000;
>> +	int ret;
>> +
>> +	ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max);
>> +	if (ret)
>> +		dev_err(priv->dev, "Failed to set regulator vdd to %d\n",
>> +			vdd_min);
>> +
>> +	return ret;
>> +}
> [..]
>> +static int qcom_ssphy_power_on(struct phy *phy)
>> +{
>> +	struct ssphy_priv *priv = phy_get_drvdata(phy);
>> +	int ret;
>> +
>> +	ret = qcom_ssphy_vdd_ctrl(priv, ENABLE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_bulk_enable(priv->num_regs, priv->regs);
>> +	if (ret)
>> +		goto err1;
>> +
>> +	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
>> +	if (ret)
>> +		goto err2;
>> +
>> +	ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode);
>> +	if (ret)
>> +		goto err3;
>> +
>> +	ret = qcom_ssphy_do_reset(priv);
>> +	if (ret)
>> +		goto err4;
>> +
>> +	writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0);
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON);
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN);
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0);
>> +
>> +	return 0;
>> +err4:
> 
> Name your labels based on what they do, e.g. err_disable_vbus.

ok

> 
>> +	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
>> +		qcom_ssphy_vbus_disable(priv->vbus);
> 
> qcom_ssphy_vbus_ctrl() above was either enabling or disabling vbus, but
> here you're directly calling disable to unroll that. It think the result
> is correct (in host this reverts to disabled and in gadget it's a
> no-op), but I'm not sure I like the design of sometimes calling straight
> to the vbus enable/disable and sometimes to the wrapper function.

I think you misread: we have vbus enable/disable and vdd control
(different regulators)


I have to admit that the only reason I had separate functions for vbus
enable/disable was cosmetic (and "if" on the control variable + another
"if" to check that the regulator was already voted was taking me beyond
the 80 lines character and I hate when that happens on simple
operations). anyway will redo

> 
>> +err3:
> 
> err_clk_disable
> 
>> +	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
>> +err2:
>> +	regulator_bulk_disable(priv->num_regs, priv->regs);
>> +err1:
>> +	qcom_ssphy_vdd_ctrl(priv, DISABLE);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_ssphy_power_off(struct phy *phy)
>> +{
>> +	struct ssphy_priv *priv = phy_get_drvdata(phy);
>> +
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0);
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0);
>> +	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN);
>> +
>> +	clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
>> +	regulator_bulk_disable(priv->num_regs, priv->regs);
>> +
>> +	if (priv->vbus && priv->mode != PHY_MODE_INVALID)
>> +		qcom_ssphy_vbus_disable(priv->vbus);
>> +
>> +	qcom_ssphy_vdd_ctrl(priv, DISABLE);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_ssphy_init_clock(struct ssphy_priv *priv)
>> +{
>> +	const char * const clk_id[] = { "ref", "phy", "pipe", };
>> +	int i;
>> +
>> +	priv->num_clks = ARRAY_SIZE(clk_id);
>> +	priv->clks = devm_kcalloc(priv->dev, priv->num_clks,
>> +				  sizeof(*priv->clks), GFP_KERNEL);
> 
> You know num_clks is 3, so I would suggest that you just change the
> sshphy_priv to clks[3] and skip the dynamic allocation.


ok

> 
> 
> Also, as num_clks always is ARRAY_SIZE(priv->clks) I would suggest using
> the latter, to make that clear throughout the driver.

maybe then just define NBR_CLKS (I find long lines a pain to read)

> 
>> +	if (!priv->clks)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < priv->num_clks; i++)
>> +		priv->clks[i].id = clk_id[i];
> 
> There's no harm in just writing this on three lines:
> 
> 	priv->clks[0].id = "ref";
> 	priv->clks[1].id = "phy";
> 	priv->clks[2].id = "pipe";
> 
>> +
>> +	return devm_clk_bulk_get(priv->dev, priv->num_clks, priv->clks);
>> +}
>> +
>> +static int qcom_ssphy_init_regulator(struct ssphy_priv *priv)
>> +{
>> +	const char * const reg_supplies[] = {
>> +		[VDD] = "vdd",
>> +		[VDDA1P8] = "vdda1p8",
>> +	};
>> +	int ret, i;
>> +
>> +	priv->num_regs = ARRAY_SIZE(reg_supplies);
>> +	priv->regs = devm_kcalloc(priv->dev, priv->num_regs,
>> +				  sizeof(*priv->regs), GFP_KERNEL);
> 
> As with clocks, you know there will only be 2 of these, make it fixed
> size in ssphy_priv.

well ok

> 
> And as with clocks, I would suggest using ARRAY_SIZE(priv->regs)
> throughout the driver to make it obvious that's it's a static number. 
> 
>> +	if (!priv->regs)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < priv->num_regs; i++)
>> +		priv->regs[i].supply = reg_supplies[i];
> 
> As with clocks, just unroll this and fill in the 2 supplies directly.

um, ok, I find the above more readable but I see where you are going
with these sort of recomendations...will just follow them

> 
>> +
>> +	ret = devm_regulator_bulk_get(priv->dev, priv->num_regs, priv->regs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->vbus = devm_regulator_get_optional(priv->dev, "vbus");
> 
> get_optional means that if vbus-supply is not found, rather than
> returning a dummy regulator object this will fail with -ENODEV.

yes I messed this up.

> 
> Given the rest of the logic related to vbus you should set vbus to NULL
> if the returned PTR_ERR(value) is -ENODEV, and fail for other errors.
> 
> 
> Or just drop the _optional, and let your vbus controls operate on the
> dummy regulator you get back.

yes will do this. thanks for the suggestion and the review!

> 
> (Right now vbus can't be NULL, so these checks are not very useful)
> 
>> +	if (IS_ERR(priv->vbus))
>> +		return PTR_ERR(priv->vbus);
>> +
>> +	return 0;
> 
> return PTR_ERR_OR_ZERO(priv->vbus)
> 
> (Although that might change based on above comment)
> 
>> +}
> 
> Regards,
> Bjorn
> 

Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Jorge Ramirez]

On 1/30/19 10:53, Jorge Ramirez wrote:
> On 1/29/19 21:27, Bjorn Andersson wrote:
>> On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote:
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
>>> new file mode 100644
>>> index 0000000..e6ae96e
>>> --- /dev/null
>>> +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c
>>> @@ -0,0 +1,347 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2018, Linaro Limited
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/reset.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#define PHY_CTRL0			0x6C
>>> +#define PHY_CTRL1			0x70
>>> +#define PHY_CTRL2			0x74
>>> +#define PHY_CTRL4			0x7C
>>> +
>>> +/* PHY_CTRL bits */
>>> +#define REF_PHY_EN			BIT(0)
>>> +#define LANE0_PWR_ON			BIT(2)
>>> +#define SWI_PCS_CLK_SEL			BIT(4)
>>> +#define TST_PWR_DOWN			BIT(4)
>>> +#define PHY_RESET			BIT(7)
>>> +
>>> +enum phy_vdd_ctrl { ENABLE, DISABLE, };
>>
>> Use bool to describe boolean values.
> 
> um, ok, but these are not booleans, they are actions (ie ctrl = action
> not true or false).
> 
> anyway will change it to something else
> 
>>
>>> +enum phy_regulator { VDD, VDDA1P8, };
>>
>> It doesn't look like you need either of these if you remove the
>> set_voltage calls.
> 
> we need to point to the regulator in the array so we need an index to it
> somehow.

you are right, we dont

> 
>>
>>> +
>>> +struct ssphy_priv {
>>> +	void __iomem *base;
>>> +	struct device *dev;
>>> +	struct reset_control *reset_com;
>>> +	struct reset_control *reset_phy;
>>> +	struct regulator *vbus;
>>> +	struct regulator_bulk_data *regs;
>>> +	int num_regs;
>>> +	struct clk_bulk_data *clks;
>>> +	int num_clks;
>>> +	enum phy_mode mode;
>>> +};
>>> +
>>> +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val)
>>> +{
>>> +	writel((readl(addr) & ~mask) | val, addr);
>>> +}
>>> +
>>> +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus)
>>> +{
>>> +	return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0;
>>
>> regulator_is_enabled() will check if the actual regulator is on, not if
>> you already voted for it.
>>
>> So if something else is enabling the vbus regulator you will skip your
>> enable and be in the mercy of them not releasing it. Presumably there's
>> only one consumer of this particular regulator, but it's a bad habit.
> 
> yep
> 
>>
>> Please keep track of this drivers requested state in this driver, if
>> qcom_ssphy_vbus_ctrl() is called not only for the state changes.
> 
> um, there is not such a function: the ctrl function is not for vbus but
> for vdd

argh, sorry, it is me who misread my own driver. ok I know what you
mean. will send the updated driver shortly.

Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Jorge Ramirez]

On 1/30/19 10:53, Jorge Ramirez wrote:

>> +	priv->vbus = devm_regulator_get_optional(priv->dev, "vbus");
> get_optional means that if vbus-supply is not found, rather than
> returning a dummy regulator object this will fail with -ENODEV.

on this subject, is it intentional that devm_reset_control_get_optional
behaves differently (ie, it returns a NULL if it cant be found)

IMO all *_get_optional should be semantically as well as functionally
equivalent to avoid confusions like the one I had.

Re: [PATCH v2 0/2] USB SS PHY for Qualcomm's QCS404

[Rob Herring]

On Tue, Jan 29, 2019 at 12:35:13PM +0100, Jorge Ramirez-Ortiz wrote:
> This set adds USB SS PHY support to Qualcomm's QCS404 SoC
> The PHY is implemented using Synopsys SS PHY 1.0.0 IP
> 
> The code is losely based on Sriharsha Allenki's
> <sallenki@codeaurora.org> original implementation.
> 
> v2:
>   enable OTG mode detection
>   move vdd voltage levels to driver
>   use bulk_ control interfaces
>   ss-phy-bindings [1]
> 
> [1] ss-phy-binding discussion:
>  - qcom,dwc3-ss-usb-phy exist for a generic usb2/usb3 phy driver that
>  was never merged. Rather than trying to re-use these bindings (or
>  delete them) I  propose that we go ahead with the new separate
>  bindings for HS and SS: if  not now - investigation  in progress- in
>  the  future  it might be  possible to have again a common phy driver
>  for  which these old  bindings would be the binding agreement. 

Either use/extend the old binding or remove it if it is not being used.

Rob

Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Rob Herring]

On Tue, Jan 29, 2019 at 12:35:14PM +0100, Jorge Ramirez-Ortiz wrote:
> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> new file mode 100644
> index 0000000..8ef6e39
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> @@ -0,0 +1,73 @@
> +Qualcomm Synopsys 1.0.0 SS phy controller
> +===========================================
> +
> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> +chipsets
> +
> +Required properties:
> +
> +- compatible:
> +    Value type: <string>
> +    Definition: Should contain "qcom,usb-ssphy".

This is in no way specific enough.

> +
> +- reg:
> +    Value type: <prop-encoded-array>
> +    Definition: USB PHY base address and length of the register map.
> +
> +- #phy-cells:
> +    Value type: <u32>
> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
> +
> +- clocks:
> +    Value type: <prop-encoded-array>
> +    Definition: See clock-bindings.txt section "consumers". List of
> +		 three clock specifiers for reference, phy core and
> +		 pipe clocks.
> +
> +- clock-names:
> +    Value type: <string>
> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
> +		 property. Must contain "ref", "phy" and "pipe".
> +
> +- vdd-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator VDD supply node.
> +
> +- vdda1p8-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator 1.8V supply node.
> +
> +
> +Optional child nodes:
> +
> +- vbus-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the VBUS supply node.

Does the phy actually get supplied by Vbus? If not, then Vbus supply 
should be defined in a USB connector node.

> +
> +- resets:
> +    Value type: <prop-encoded-array>
> +    Definition: See reset.txt section "consumers". PHY reset specifiers
> +		 for phy core and COR resets.

COR or COM?

Looks to me the order is reversed.

> +
> +- reset-names:
> +    Value type: <string>
> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
> +		 property. Must contain "com" and "phy".
> +
> +Example:
> +
> +usb3_phy: phy@78000 {

usb3-phy@...

> +	compatible = "qcom,usb-ssphy";
> +	reg = <0x78000 0x400>;
> +	#phy-cells = <0>;
> +	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
> +		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
> +		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
> +	clock-names = "ref", "phy", "pipe";
> +	resets = <&gcc GCC_USB3_PHY_BCR>,
> +		 <&gcc GCC_USB3PHY_PHY_BCR>;
> +	reset-names = "com", "phy";
> +	vdd-supply = <&vreg_l3_1p05>;
> +	vdda1p8-supply = <&vreg_l5_1p8>;
> +	vbus-supply = <&usb3_vbus_reg>;
> +};
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 1/30/19 21:02, Rob Herring wrote:
> On Tue, Jan 29, 2019 at 12:35:14PM +0100, Jorge Ramirez-Ortiz wrote:
>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
>> controller embedded in QCS404.
>>
>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
>> definitions.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
>>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>> new file mode 100644
>> index 0000000..8ef6e39
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>> @@ -0,0 +1,73 @@
>> +Qualcomm Synopsys 1.0.0 SS phy controller
>> +===========================================
>> +
>> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
>> +chipsets
>> +
>> +Required properties:
>> +
>> +- compatible:
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,usb-ssphy".
> 
> This is in no way specific enough.

ok. will remove the old unused bindings and reuse qcom,dwc3-ss-usb-phy

> 
>> +
>> +- reg:
>> +    Value type: <prop-encoded-array>
>> +    Definition: USB PHY base address and length of the register map.
>> +
>> +- #phy-cells:
>> +    Value type: <u32>
>> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
>> +
>> +- clocks:
>> +    Value type: <prop-encoded-array>
>> +    Definition: See clock-bindings.txt section "consumers". List of
>> +		 three clock specifiers for reference, phy core and
>> +		 pipe clocks.
>> +
>> +- clock-names:
>> +    Value type: <string>
>> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
>> +		 property. Must contain "ref", "phy" and "pipe".
>> +
>> +- vdd-supply:
>> +    Value type: <phandle>
>> +    Definition: phandle to the regulator VDD supply node.
>> +
>> +- vdda1p8-supply:
>> +    Value type: <phandle>
>> +    Definition: phandle to the regulator 1.8V supply node.
>> +
>> +
>> +Optional child nodes:
>> +
>> +- vbus-supply:
>> +    Value type: <phandle>
>> +    Definition: phandle to the VBUS supply node.
> 
> Does the phy actually get supplied by Vbus? If not, then Vbus supply 
> should be defined in a USB connector node.

yes per the documentation vbus can optionally be routed to the phy to
drive a signal to the controller.


> 
>> +
>> +- resets:
>> +    Value type: <prop-encoded-array>
>> +    Definition: See reset.txt section "consumers". PHY reset specifiers
>> +		 for phy core and COR resets.
> 
> COR or COM?

com
> 
> Looks to me the order is reversed.

yes

> 
>> +
>> +- reset-names:
>> +    Value type: <string>
>> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
>> +		 property. Must contain "com" and "phy".
>> +
>> +Example:
>> +
>> +usb3_phy: phy@78000 {
> 
> usb3-phy@...

ok

> 
>> +	compatible = "qcom,usb-ssphy";
>> +	reg = <0x78000 0x400>;
>> +	#phy-cells = <0>;
>> +	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
>> +		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
>> +		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
>> +	clock-names = "ref", "phy", "pipe";
>> +	resets = <&gcc GCC_USB3_PHY_BCR>,
>> +		 <&gcc GCC_USB3PHY_PHY_BCR>;
>> +	reset-names = "com", "phy";
>> +	vdd-supply = <&vreg_l3_1p05>;
>> +	vdda1p8-supply = <&vreg_l5_1p8>;
>> +	vbus-supply = <&usb3_vbus_reg>;
>> +};
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 2/5/19 12:02, Jorge Ramirez wrote:
> On 1/30/19 21:02, Rob Herring wrote:
>> On Tue, Jan 29, 2019 at 12:35:14PM +0100, Jorge Ramirez-Ortiz wrote:
>>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
>>> controller embedded in QCS404.
>>>
>>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
>>> definitions.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> ---
>>>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
>>>  1 file changed, 73 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> new file mode 100644
>>> index 0000000..8ef6e39
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> @@ -0,0 +1,73 @@
>>> +Qualcomm Synopsys 1.0.0 SS phy controller
>>> +===========================================
>>> +
>>> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
>>> +chipsets
>>> +
>>> +Required properties:
>>> +
>>> +- compatible:
>>> +    Value type: <string>
>>> +    Definition: Should contain "qcom,usb-ssphy".
>>
>> This is in no way specific enough.
> 
> ok. will remove the old unused bindings and reuse qcom,dwc3-ss-usb-phy
> 
>>
>>> +
>>> +- reg:
>>> +    Value type: <prop-encoded-array>
>>> +    Definition: USB PHY base address and length of the register map.
>>> +
>>> +- #phy-cells:
>>> +    Value type: <u32>
>>> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
>>> +
>>> +- clocks:
>>> +    Value type: <prop-encoded-array>
>>> +    Definition: See clock-bindings.txt section "consumers". List of
>>> +		 three clock specifiers for reference, phy core and
>>> +		 pipe clocks.
>>> +
>>> +- clock-names:
>>> +    Value type: <string>
>>> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
>>> +		 property. Must contain "ref", "phy" and "pipe".
>>> +
>>> +- vdd-supply:
>>> +    Value type: <phandle>
>>> +    Definition: phandle to the regulator VDD supply node.
>>> +
>>> +- vdda1p8-supply:
>>> +    Value type: <phandle>
>>> +    Definition: phandle to the regulator 1.8V supply node.
>>> +
>>> +
>>> +Optional child nodes:
>>> +
>>> +- vbus-supply:
>>> +    Value type: <phandle>
>>> +    Definition: phandle to the VBUS supply node.
>>
>> Does the phy actually get supplied by Vbus? If not, then Vbus supply 
>> should be defined in a USB connector node.
> 
> yes per the documentation vbus can optionally be routed to the phy to
> drive a signal to the controller.


funny enough when vbus is optionally routed to the phy is not to be
controlled like we do when the vbus-supply property is present.

So to all effects no, you are right, the phy does not get supplied by VBUS.

would defining the connector like this be enough?

usb3_phy: usb3-phy@78000 {
	compatible = "qcom,snps-usb-ssphy";
	[...]
	usb3_c_connector: usb3-c-connector {
		compatible = "usb-c-connector";
		label = "USB-C";
		type = "micro";
		vbus-supply = <&usb3_vbus_reg>;
	};
};


> 
> 
>>
>>> +
>>> +- resets:
>>> +    Value type: <prop-encoded-array>
>>> +    Definition: See reset.txt section "consumers". PHY reset specifiers
>>> +		 for phy core and COR resets.
>>
>> COR or COM?
> 
> com
>>
>> Looks to me the order is reversed.
> 
> yes
> 
>>
>>> +
>>> +- reset-names:
>>> +    Value type: <string>
>>> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
>>> +		 property. Must contain "com" and "phy".
>>> +
>>> +Example:
>>> +
>>> +usb3_phy: phy@78000 {
>>
>> usb3-phy@...
> 
> ok
> 
>>
>>> +	compatible = "qcom,usb-ssphy";
>>> +	reg = <0x78000 0x400>;
>>> +	#phy-cells = <0>;
>>> +	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
>>> +		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
>>> +		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
>>> +	clock-names = "ref", "phy", "pipe";
>>> +	resets = <&gcc GCC_USB3_PHY_BCR>,
>>> +		 <&gcc GCC_USB3PHY_PHY_BCR>;
>>> +	reset-names = "com", "phy";
>>> +	vdd-supply = <&vreg_l3_1p05>;
>>> +	vdda1p8-supply = <&vreg_l5_1p8>;
>>> +	vbus-supply = <&usb3_vbus_reg>;
>>> +};
>>> -- 
>>> 2.7.4
>>>
>>
> 

Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Rob Herring]

On Wed, Feb 6, 2019 at 8:11 AM Jorge Ramirez
<jorge.ramirez-ortiz@linaro.org> wrote:
>
> On 2/5/19 12:02, Jorge Ramirez wrote:
> > On 1/30/19 21:02, Rob Herring wrote:
> >> On Tue, Jan 29, 2019 at 12:35:14PM +0100, Jorge Ramirez-Ortiz wrote:
> >>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> >>> controller embedded in QCS404.
> >>>
> >>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> >>> definitions.
> >>>
> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >>> ---
> >>>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 73 ++++++++++++++++++++++
> >>>  1 file changed, 73 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>> new file mode 100644
> >>> index 0000000..8ef6e39
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>> @@ -0,0 +1,73 @@
> >>> +Qualcomm Synopsys 1.0.0 SS phy controller
> >>> +===========================================
> >>> +
> >>> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> >>> +chipsets
> >>> +
> >>> +Required properties:
> >>> +
> >>> +- compatible:
> >>> +    Value type: <string>
> >>> +    Definition: Should contain "qcom,usb-ssphy".
> >>
> >> This is in no way specific enough.
> >
> > ok. will remove the old unused bindings and reuse qcom,dwc3-ss-usb-phy
> >
> >>
> >>> +
> >>> +- reg:
> >>> +    Value type: <prop-encoded-array>
> >>> +    Definition: USB PHY base address and length of the register map.
> >>> +
> >>> +- #phy-cells:
> >>> +    Value type: <u32>
> >>> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
> >>> +
> >>> +- clocks:
> >>> +    Value type: <prop-encoded-array>
> >>> +    Definition: See clock-bindings.txt section "consumers". List of
> >>> +            three clock specifiers for reference, phy core and
> >>> +            pipe clocks.
> >>> +
> >>> +- clock-names:
> >>> +    Value type: <string>
> >>> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
> >>> +            property. Must contain "ref", "phy" and "pipe".
> >>> +
> >>> +- vdd-supply:
> >>> +    Value type: <phandle>
> >>> +    Definition: phandle to the regulator VDD supply node.
> >>> +
> >>> +- vdda1p8-supply:
> >>> +    Value type: <phandle>
> >>> +    Definition: phandle to the regulator 1.8V supply node.
> >>> +
> >>> +
> >>> +Optional child nodes:
> >>> +
> >>> +- vbus-supply:
> >>> +    Value type: <phandle>
> >>> +    Definition: phandle to the VBUS supply node.
> >>
> >> Does the phy actually get supplied by Vbus? If not, then Vbus supply
> >> should be defined in a USB connector node.
> >
> > yes per the documentation vbus can optionally be routed to the phy to
> > drive a signal to the controller.
>
>
> funny enough when vbus is optionally routed to the phy is not to be
> controlled like we do when the vbus-supply property is present.
>
> So to all effects no, you are right, the phy does not get supplied by VBUS.
>
> would defining the connector like this be enough?
>
> usb3_phy: usb3-phy@78000 {
>         compatible = "qcom,snps-usb-ssphy";
>         [...]
>         usb3_c_connector: usb3-c-connector {
>                 compatible = "usb-c-connector";
>                 label = "USB-C";
>                 type = "micro";
>                 vbus-supply = <&usb3_vbus_reg>;
>         };
> };

IIRC, the connector node is defined to go under either the USB
controller or a USB-C controller (if a separate chip controlling the
USB-PD and alternate modes). We generally don't put PHYs into a node
topology, but keep them as a phandle reference.

Rob