Re: [PATCH v2 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
On 20/8/2019 9:59 PM, Andy Shevchenko wrote: On Tue, Aug 20, 2019 at 04:56:02PM +0300, Andy Shevchenko wrote: On Tue, Aug 20, 2019 at 06:31:33PM +0800, Ramuthevar,Vadivel MuruganX wrote: +#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. + /* Drive impedance: 50 Ohm */ Nice, you have already a comment here. Just use DR_TY_SHIFT(1) It should be DR_TY_SHIFT(6) now since I dropped the negation. Thanks Andy, will update the review comments. Best Regards Vadivel
Re: [PATCH v2 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
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
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_MASKGENMASK(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_150M3
+#define FRQSEL_MASKGENMASK(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 = >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, );
And here.
Noted, will update
With Best Regards
Vadivel
Re: [PATCH v2 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
On Tue, Aug 20, 2019 at 04:56:02PM +0300, Andy Shevchenko wrote: > On Tue, Aug 20, 2019 at 06:31:33PM +0800, Ramuthevar,Vadivel MuruganX wrote: > > +#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. > > + /* Drive impedance: 50 Ohm */ > > Nice, you have already a comment here. Just use DR_TY_SHIFT(1) It should be DR_TY_SHIFT(6) now since I dropped the negation. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
On Tue, Aug 20, 2019 at 06:31:33PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan
>
> Add support for eMMC PHY on Intel's Lightning Mountain SoC.
Thanks for an update.
Looks better though several minor comments below.
> +/* eMMC phy register definitions */
> +#define EMMC_PHYCTRL0_REG0xa8
> +#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_REG0xac
> +#define PDB_MASK BIT(0)
> +#define ENDLL_MASK BIT(7)
> +#define ENDLL_VALBIT(7)
Again, inconsistency here,
#define ENDLL_SHIFT(x) (((x) << 7) & ENDLL_MASK)
> +#define EMMC_PHYCTRL2_REG0xb0
> +#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)
> + /*
> + * 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);
> + 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)...
> +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
> +static int intel_emmc_phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >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, );
And here.
--
With Best Regards,
Andy Shevchenko
