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