Re: [PATCH v2 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY

[Ramuthevar, Vadivel MuruganX]

On 20/8/2019 9:56 PM, Andy Shevchenko wrote:

On Tue, Aug 20, 2019 at 06:31:33PM +0800, Ramuthevar,Vadivel MuruganX wrote:

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthe...@linux.intel.com>

Add support for eMMC PHY on Intel's Lightning Mountain SoC.

Thanks for an update.
Looks better though several minor comments below.

Thanks a lot! Andy,  for the review comments.

+/* eMMC phy register definitions */
+#define EMMC_PHYCTRL0_REG      0xa8
+#define DR_TY_MASK             GENMASK(30, 28)
+#define DR_TY_50OHM(x)         ((~(x) << 28) & DR_TY_MASK)

For consistency it should be

#define DR_TY_SHIFT(x)          (((x) << 28) & DR_TY_MASK)

with explanation about 50 Ohm in the code below.

+#define OTAPDLYENA             BIT(14)
+#define OTAPDLYSEL_MASK                GENMASK(13, 10)
+#define OTAPDLYSEL_SHIFT(x)    (((x) << 10) & OTAPDLYSEL_MASK)
+
+#define EMMC_PHYCTRL1_REG      0xac
+#define PDB_MASK               BIT(0)
+#define ENDLL_MASK             BIT(7)
+#define ENDLL_VAL              BIT(7)

Again, inconsistency here,

#define ENDLL_SHIFT(x)          (((x) << 7) & ENDLL_MASK)

Agreed

+#define EMMC_PHYCTRL2_REG      0xb0
+#define FRQSEL_25M             0
+#define FRQSEL_150M            3
+#define FRQSEL_MASK            GENMASK(24, 22)
+#define FRQSEL_SHIFT(x)                ((x) << 22)

And here

#define FRQSEL_SHIFT(x)         (((x) << 22) & FRQSEL_MASK)

Agreed

+       /*
+        * According to the user manual, calpad calibration
+        * cycle takes more than 2us without the minimal recommended
+        * value, so we may need a little margin here
+        */
+       usleep_range(3, 6);

Actually for this low values it's recommended to use udelay() disregard to
context.

        udelay(5);

Agreed

+       regmap_update_bits(priv->syscfg, EMMC_PHYCTRL1_REG, PDB_MASK, 1);

1 looks like a magic that has to be changed in the same way as for the rest, 
i.e.

#define PDB_SHIFT(x)    (((x) << 0) & PDB_MASK)

        ..., PDB_MASK, PDB_SHIFT(1)...

Agreed

+static int intel_emmc_phy_power_on(struct phy *phy)
+{
+       struct intel_emmc_phy *priv = phy_get_drvdata(phy);
+       int ret;
+
+       /* Drive impedance: 50 Ohm */

Nice, you have already a comment here. Just use DR_TY_SHIFT(1)

+       ret = regmap_update_bits(priv->syscfg, EMMC_PHYCTRL0_REG, DR_TY_MASK,
+                                DR_TY_50OHM(1));
+       ret = regmap_update_bits(priv->syscfg, EMMC_PHYCTRL0_REG, OTAPDLYENA,
+                                0x0);

0x0 -> 0

Noted

+static int intel_emmc_phy_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct intel_emmc_phy *priv;
+       struct phy *generic_phy;
+       struct phy_provider *phy_provider;
+
+       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+
+       /* Get eMMC phy (accessed via chiptop) regmap */
+       priv->syscfg = syscon_regmap_lookup_by_phandle(dev->of_node,
+                                                      "intel,syscon");

Perhaps

        struct device_node *np = dev->of_node;
        ...
        priv->syscfg = syscon_regmap_lookup_by_phandle(np, "intel,syscon");

+       generic_phy = devm_phy_create(dev, dev->of_node, &ops);

And here.

Noted, will update

With Best Regards
Vadivel