Hi Manu,

On Thu, Dec 20, 2018 at 09:33:43AM +0530, mgautam@codeaurora.org wrote:

Hi Shawn,

On 2018-12-20 06:31, Shawn Guo wrote:

quoted

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 <redacted>
---

....

quoted

+
+/* PHY register and bit definitions */
+#define PHY_CTRL_COMMON0		0x078
+#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_EN    BIT(4)
#define DP_RISE_INT_EN    BIT(3)
...

Good suggestion.  Will do.

quoted

+
+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.

quoted

+	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.

quoted

+	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?

quoted

+	enum phy_mode mode;
+};
+

quoted

+
+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?

quoted

+{
+	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.

quoted

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

..

quoted

+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.

Shawn

quoted

+	.owner = THIS_MODULE,
+};
+