Re: [PATCH v6 2/2] phy: qualcomm: Add Synopsys High-Speed USB PHY driver
Hi,
On 12/20/2018 4:39 PM, Shawn Guo wrote:
> Hi Manu,
>
> On Thu, Dec 20, 2018 at 09:33:43AM +0530, mgau...@codeaurora.org wrote:
>> Hi Shawn,
>>
>> On 2018-12-20 06:31, Shawn Guo wrote:
>>> It adds Synopsys 28nm Femto High-Speed USB PHY driver support, which
>>> is usually paired with Synopsys DWC3 USB controllers on Qualcomm SoCs.
>>>
>>> Signed-off-by: Shawn Guo
>>> ---
>>
...
>>> +struct hsphy_priv {
>> nit-pick - indentation for following structure members?
> Hmm, my personal taste says no, because I found that it's hard to keep
> the indentation when adding new members later.
ok
>
>>> + void __iomem *base;
>>> + struct clk_bulk_data *clks;
>>> + int num_clks;
>>> + struct reset_control *phy_reset;
>>> + struct reset_control *por_reset;
>>> + struct regulator_bulk_data vregs[VREG_NUM];
>>> + unsigned int voltages[VREG_NUM][VOL_NUM];
>>> + const struct hsphy_data *data;
>>> + bool cable_connected;
>> You can get cable-connected state from "enum phy_mode mode" which
>> is present in this driver.
>> E.g. cable_connected is false if mode is neither HOST nor DEVICE.
>>
>>
>>> + struct extcon_dev *vbus_edev;
>>> + struct notifier_block vbus_notify;
>> extcons not needed if you use "mode" for the same purpose.
> The extcon is there for indicating cable connection status. I'm not
> sure that phy_mode can be used for that purpose. For example, what
> value would phy core set phy_mode to, if we disconnect the cable from
> phy_mode being HOST or DEVICE?
it depends how it is used. Looks like it is used to decide whether to turn-off
regulators or not. Unless you plan to add low power support as part of
runtime-suspend of PHY during host mode, there is no reason to not turn-off
regulators on pm_suspend(). Please refer to my comments below.
>
>>
>>> + enum phy_mode mode;
>>> +};
>>> +
>>
>>> +
>>> +static int qcom_snps_hsphy_vbus_notifier(struct notifier_block *nb,
>>> +unsigned long event, void *ptr)
>>> +{
>>> + struct hsphy_priv *priv = container_of(nb, struct hsphy_priv,
>>> + vbus_notify);
>>> + priv->cable_connected = !!event;
>>> + return 0;
>>> +}
>>> +
>>> +static int qcom_snps_hsphy_power_on(struct phy *phy)
>> Can you instead merge this power_on function with phy_init?
> I can do that, but what's the gain/advantage from doing that?
dwc3 core calls phy_init() before power_on(). AFAIK, PHY regulators
need to be ON before initializing it.
>
>>> +{
>>> + struct hsphy_priv *priv = phy_get_drvdata(phy);
>>> + int ret;
>>> +
>>> + if (priv->cable_connected) {
>> Why distinguish between cable connected vs not-connected?
> This is based on the vendor driver implementation. It does a more
> aggressive low power management in case that cable is not connected,
> i.e. turning off regulator and entering retention mode.
I believe 'aggressive low power management' will be triggered on
pm_suspend?
And dwc3 core will in any case perform phy_exit()->phy_init() across
pm_suspend/resume which will reset PHYs. Hence, there is no need to check
for cable connected state here.
>
>>> + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
>>> + if (ret)
>>> + return ret;
>>> + qcom_snps_hsphy_disable_hv_interrupts(priv);
>>> + } else {
>>> + ret = qcom_snps_hsphy_enable_regulators(priv);
>>> + if (ret)
>>> + return ret;
>>> + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
>>> + if (ret)
>>> + return ret;
>>> + qcom_snps_hsphy_exit_retention(priv);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int qcom_snps_hsphy_power_off(struct phy *phy)
>>> +{
>>> + struct hsphy_priv *priv = phy_get_drvdata(phy);
>>> +
>>> + if (priv->cable_connected) {
>>> + qcom_snps_hsphy_enable_hv_interrupts(priv);
>>> + clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
>>> + } else {
>>> + qcom_snps_hsphy_enter_retention(priv);
>>> + clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
>>> + qcom_snps_hsphy_disable_regulators(priv);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>>
>> ..
>>> +static const struct phy_ops qcom_snps_hsphy_ops = {
>>> + .init = qcom_snps_hsphy_init,
>>> + .power_on = qcom_snps_hsphy_power_on,
>>> + .power_off = qcom_snps_hsphy_power_off,
>>> + .set_mode = qcom_snps_hsphy_set_mode,
>> .phy_exit()?
>> I believe that is needed as dwc3 core driver performs
>> phy_exit/phy_init across pm_suspend/resume.
> I just do not see anything that we should be doing in .exit hook right
> now.
After you merge power_on() with phy_init() as per my previous comment,
you can rely on phy_exit() to take care of putting PHY in low power state
and turn off regulators as well.
>
> Shawn
>
>>
>>> + .owner = THIS_MODULE,
>>> +};
>>> +
--
The Qualcomm Innovation Center, Inc. is a
Re: [PATCH v6 2/2] phy: qualcomm: Add Synopsys High-Speed USB PHY driver
Hi Manu,
On Thu, Dec 20, 2018 at 09:33:43AM +0530, mgau...@codeaurora.org wrote:
> Hi Shawn,
>
> On 2018-12-20 06:31, Shawn Guo wrote:
> >It adds Synopsys 28nm Femto High-Speed USB PHY driver support, which
> >is usually paired with Synopsys DWC3 USB controllers on Qualcomm SoCs.
> >
> >Signed-off-by: Shawn Guo
> >---
>
>
> >+
> >+/* PHY register and bit definitions */
> >+#define PHY_CTRL_COMMON00x078
> >+#define SIDDQ BIT(2)
> >+#define PHY_IRQ_CMD 0x0d0
> >+#define PHY_INTR_MASK0 0x0d4
> >+#define PHY_INTR_CLEAR0 0x0dc
> >+#define DPDM_MASK 0x1e
> >+#define DP_1_0 BIT(4)
> >+#define DP_0_1 BIT(3)
> >+#define DM_1_0 BIT(2)
> >+#define DM_0_1 BIT(1)
>
> Can we rename these to something more readable? e.g.:
> #define DP_FALL_INT_ENBIT(4)
> #define DP_RISE_INT_ENBIT(3)
> ...
Good suggestion. Will do.
>
> >+
> >+enum hsphy_voltage {
> >+VOL_NONE,
> >+VOL_MIN,
> >+VOL_MAX,
> >+VOL_NUM,
> >+};
> >+
> >+enum hsphy_vreg {
> >+VDD,
> >+VDDA_1P8,
> >+VDDA_3P3,
> >+VREG_NUM,
> >+};
> >+
> >+struct hsphy_init_seq {
> >+int offset;
> >+int val;
> >+int delay;
> >+};
> >+
> >+struct hsphy_data {
> >+const struct hsphy_init_seq *init_seq;
> >+unsigned int init_seq_num;
> >+};
> >+
> >+struct hsphy_priv {
> nit-pick - indentation for following structure members?
Hmm, my personal taste says no, because I found that it's hard to keep
the indentation when adding new members later.
>
> >+void __iomem *base;
> >+struct clk_bulk_data *clks;
> >+int num_clks;
> >+struct reset_control *phy_reset;
> >+struct reset_control *por_reset;
> >+struct regulator_bulk_data vregs[VREG_NUM];
> >+unsigned int voltages[VREG_NUM][VOL_NUM];
> >+const struct hsphy_data *data;
> >+bool cable_connected;
>
> You can get cable-connected state from "enum phy_mode mode" which
> is present in this driver.
> E.g. cable_connected is false if mode is neither HOST nor DEVICE.
>
>
> >+struct extcon_dev *vbus_edev;
> >+struct notifier_block vbus_notify;
>
> extcons not needed if you use "mode" for the same purpose.
The extcon is there for indicating cable connection status. I'm not
sure that phy_mode can be used for that purpose. For example, what
value would phy core set phy_mode to, if we disconnect the cable from
phy_mode being HOST or DEVICE?
>
>
> >+enum phy_mode mode;
> >+};
> >+
>
>
> >+
> >+static int qcom_snps_hsphy_vbus_notifier(struct notifier_block *nb,
> >+ unsigned long event, void *ptr)
> >+{
> >+struct hsphy_priv *priv = container_of(nb, struct hsphy_priv,
> >+vbus_notify);
> >+priv->cable_connected = !!event;
> >+return 0;
> >+}
> >+
> >+static int qcom_snps_hsphy_power_on(struct phy *phy)
>
> Can you instead merge this power_on function with phy_init?
I can do that, but what's the gain/advantage from doing that?
>
> >+{
> >+struct hsphy_priv *priv = phy_get_drvdata(phy);
> >+int ret;
> >+
> >+if (priv->cable_connected) {
>
> Why distinguish between cable connected vs not-connected?
This is based on the vendor driver implementation. It does a more
aggressive low power management in case that cable is not connected,
i.e. turning off regulator and entering retention mode.
>
> >+ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> >+if (ret)
> >+return ret;
> >+qcom_snps_hsphy_disable_hv_interrupts(priv);
> >+} else {
> >+ret = qcom_snps_hsphy_enable_regulators(priv);
> >+if (ret)
> >+return ret;
> >+ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> >+if (ret)
> >+return ret;
> >+qcom_snps_hsphy_exit_retention(priv);
> >+}
> >+
> >+return 0;
> >+}
> >+
> >+static int qcom_snps_hsphy_power_off(struct phy *phy)
> >+{
> >+struct hsphy_priv *priv = phy_get_drvdata(phy);
> >+
> >+if (priv->cable_connected) {
> >+qcom_snps_hsphy_enable_hv_interrupts(priv);
> >+clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> >+} else {
> >+qcom_snps_hsphy_enter_retention(priv);
> >+clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> >+qcom_snps_hsphy_disable_regulators(priv);
> >+}
> >+
> >+return 0;
> >+}
> >+
>
>
>
> ..
> >+static const struct phy_ops qcom_snps_hsphy_ops = {
> >+.init = qcom_snps_hsphy_init,
> >+.power_on = qcom_snps_hsphy_power_on,
> >+.power_off = qcom_snps_hsphy_power_off,
> >+.set_mode = qcom_snps_hsphy_set_mode,
>
> .phy_exit()?
> I believe that is needed as dwc3 core
Re: [PATCH v6 2/2] phy: qualcomm: Add Synopsys High-Speed USB PHY driver
Hi Shawn,
On 2018-12-20 06:31, Shawn Guo wrote:
It adds Synopsys 28nm Femto High-Speed USB PHY driver support, which
is usually paired with Synopsys DWC3 USB controllers on Qualcomm SoCs.
Signed-off-by: Shawn Guo
---
+
+/* PHY register and bit definitions */
+#define PHY_CTRL_COMMON0 0x078
+#define SIDDQ BIT(2)
+#define PHY_IRQ_CMD0x0d0
+#define PHY_INTR_MASK0 0x0d4
+#define PHY_INTR_CLEAR00x0dc
+#define DPDM_MASK 0x1e
+#define DP_1_0 BIT(4)
+#define DP_0_1 BIT(3)
+#define DM_1_0 BIT(2)
+#define DM_0_1 BIT(1)
Can we rename these to something more readable? e.g.:
#define DP_FALL_INT_ENBIT(4)
#define DP_RISE_INT_ENBIT(3)
...
+
+enum hsphy_voltage {
+ VOL_NONE,
+ VOL_MIN,
+ VOL_MAX,
+ VOL_NUM,
+};
+
+enum hsphy_vreg {
+ VDD,
+ VDDA_1P8,
+ VDDA_3P3,
+ VREG_NUM,
+};
+
+struct hsphy_init_seq {
+ int offset;
+ int val;
+ int delay;
+};
+
+struct hsphy_data {
+ const struct hsphy_init_seq *init_seq;
+ unsigned int init_seq_num;
+};
+
+struct hsphy_priv {
nit-pick - indentation for following structure members?
+ void __iomem *base;
+ struct clk_bulk_data *clks;
+ int num_clks;
+ struct reset_control *phy_reset;
+ struct reset_control *por_reset;
+ struct regulator_bulk_data vregs[VREG_NUM];
+ unsigned int voltages[VREG_NUM][VOL_NUM];
+ const struct hsphy_data *data;
+ bool cable_connected;
You can get cable-connected state from "enum phy_mode mode" which
is present in this driver.
E.g. cable_connected is false if mode is neither HOST nor DEVICE.
+ struct extcon_dev *vbus_edev;
+ struct notifier_block vbus_notify;
extcons not needed if you use "mode" for the same purpose.
+ enum phy_mode mode;
+};
+
+
+static int qcom_snps_hsphy_vbus_notifier(struct notifier_block *nb,
+unsigned long event, void *ptr)
+{
+ struct hsphy_priv *priv = container_of(nb, struct hsphy_priv,
+ vbus_notify);
+ priv->cable_connected = !!event;
+ return 0;
+}
+
+static int qcom_snps_hsphy_power_on(struct phy *phy)
Can you instead merge this power_on function with phy_init?
+{
+ struct hsphy_priv *priv = phy_get_drvdata(phy);
+ int ret;
+
+ if (priv->cable_connected) {
Why distinguish between cable connected vs not-connected?
+ ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+ if (ret)
+ return ret;
+ qcom_snps_hsphy_disable_hv_interrupts(priv);
+ } else {
+ ret = qcom_snps_hsphy_enable_regulators(priv);
+ if (ret)
+ return ret;
+ ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+ if (ret)
+ return ret;
+ qcom_snps_hsphy_exit_retention(priv);
+ }
+
+ return 0;
+}
+
+static int qcom_snps_hsphy_power_off(struct phy *phy)
+{
+ struct hsphy_priv *priv = phy_get_drvdata(phy);
+
+ if (priv->cable_connected) {
+ qcom_snps_hsphy_enable_hv_interrupts(priv);
+ clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+ } else {
+ qcom_snps_hsphy_enter_retention(priv);
+ clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+ qcom_snps_hsphy_disable_regulators(priv);
+ }
+
+ return 0;
+}
+
..
+static const struct phy_ops qcom_snps_hsphy_ops = {
+ .init = qcom_snps_hsphy_init,
+ .power_on = qcom_snps_hsphy_power_on,
+ .power_off = qcom_snps_hsphy_power_off,
+ .set_mode = qcom_snps_hsphy_set_mode,
.phy_exit()?
I believe that is needed as dwc3 core driver performs
phy_exit/phy_init across pm_suspend/resume.
+ .owner = THIS_MODULE,
+};
+
[PATCH v6 2/2] phy: qualcomm: Add Synopsys High-Speed USB PHY driver
It adds Synopsys 28nm Femto High-Speed USB PHY driver support, which
is usually paired with Synopsys DWC3 USB controllers on Qualcomm SoCs.
Signed-off-by: Shawn Guo
---
drivers/phy/qualcomm/Kconfig | 10 +
drivers/phy/qualcomm/Makefile | 1 +
.../phy/qualcomm/phy-qcom-usb-hs-snps-28nm.c | 529 ++
3 files changed, 540 insertions(+)
create mode 100644 drivers/phy/qualcomm/phy-qcom-usb-hs-snps-28nm.c
diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 32f7d34eb784..c7b5ee82895d 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -82,3 +82,13 @@ config PHY_QCOM_USB_HSIC
select GENERIC_PHY
help
Support for the USB HSIC ULPI compliant PHY on QCOM chipsets.
+
+config PHY_QCOM_USB_HS_SNPS_28NM
+ tristate "Qualcomm Synopsys 28nm USB HS 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 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.
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index c56efd3af205..6d220dba6b4f 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_PHY_QCOM_UFS_14NM) +=
phy-qcom-ufs-qmp-14nm.o
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-snps-28nm.o
diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs-snps-28nm.c
b/drivers/phy/qualcomm/phy-qcom-usb-hs-snps-28nm.c
new file mode 100644
index ..1fa364417237
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-usb-hs-snps-28nm.c
@@ -0,0 +1,529 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2009-2018, Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* PHY register and bit definitions */
+#define PHY_CTRL_COMMON0 0x078
+#define SIDDQ BIT(2)
+#define PHY_IRQ_CMD0x0d0
+#define PHY_INTR_MASK0 0x0d4
+#define PHY_INTR_CLEAR00x0dc
+#define DPDM_MASK 0x1e
+#define DP_1_0 BIT(4)
+#define DP_0_1 BIT(3)
+#define DM_1_0 BIT(2)
+#define DM_0_1 BIT(1)
+
+enum hsphy_voltage {
+ VOL_NONE,
+ VOL_MIN,
+ VOL_MAX,
+ VOL_NUM,
+};
+
+enum hsphy_vreg {
+ VDD,
+ VDDA_1P8,
+ VDDA_3P3,
+ VREG_NUM,
+};
+
+struct hsphy_init_seq {
+ int offset;
+ int val;
+ int delay;
+};
+
+struct hsphy_data {
+ const struct hsphy_init_seq *init_seq;
+ unsigned int init_seq_num;
+};
+
+struct hsphy_priv {
+ void __iomem *base;
+ struct clk_bulk_data *clks;
+ int num_clks;
+ struct reset_control *phy_reset;
+ struct reset_control *por_reset;
+ struct regulator_bulk_data vregs[VREG_NUM];
+ unsigned int voltages[VREG_NUM][VOL_NUM];
+ const struct hsphy_data *data;
+ bool cable_connected;
+ struct extcon_dev *vbus_edev;
+ struct notifier_block vbus_notify;
+ enum phy_mode mode;
+};
+
+static int qcom_snps_hsphy_config_regulators(struct hsphy_priv *priv, int high)
+{
+ int old_uV[VREG_NUM];
+ int min, ret, i;
+
+ min = high ? 1 : 0; /* low or none? */
+
+ for (i = 0; i < VREG_NUM; i++) {
+ old_uV[i] = regulator_get_voltage(priv->vregs[i].consumer);
+ ret = regulator_set_voltage(priv->vregs[i].consumer,
+ priv->voltages[i][min],
+ priv->voltages[i][VOL_MAX]);
+ if (ret)
+ goto roll_back;
+ }
+
+ return 0;
+
+roll_back:
+ for (; i >= 0; i--)
+ regulator_set_voltage(priv->vregs[i].consumer,
+ old_uV[i], old_uV[i]);
+ return ret;
+}
+
+static int qcom_snps_hsphy_enable_regulators(struct hsphy_priv *priv)
+{
+ int ret;
+
+ ret = qcom_snps_hsphy_config_regulators(priv, 1);
+ if (ret)
+ return ret;
+
+ ret = regulator_set_load(priv->vregs[VDDA_1P8].consumer, 19000);
+ if (ret < 0)
+ goto unconfig_regulators;
+
+ ret = regulator_set_load(priv->vregs[VDDA_3P3].consumer, 16000);
+ if
