Re: [PATCH v6 2/2] phy: qualcomm: Add Synopsys High-Speed USB PHY driver

[Shawn Guo]

Hi Manu,

On Thu, Dec 20, 2018 at 09:33:43AM +0530, mgautam@xxxxxxxxxxxxxx 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 <shawn.guo@xxxxxxxxxx>
> >---
> ....
> 
> >+
> >+/* 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.

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

> 
> 
> >+	.owner = THIS_MODULE,
> >+};
> >+
>