Re: [PATCH v1 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
On 20/8/2019 12:06 AM, Andy Shevchenko wrote:
On Mon, Aug 19, 2019 at 11:44:16AM +0800, Ramuthevar,Vadivel MuruganX wrote:
From: Ramuthevar Vadivel Murugan
Adds support for eMMC PHY on Intel's Lightning Mountain SoC.
Adds -> Add.
Thanks Andy, agreed.
+/* 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)
+#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 1
BIT(0)
agreed.
+#define ENDLL_MASK BIT(7)
+#define ENDLL_VAL BIT(7)
+
+#define EMMC_PHYCTRL2_REG 0xb0
+#define FRQSEL_25M 0
+#define FRQSEL_150M3
+#define FRQSEL_MASKGENMASK(24, 22)
+#define FRQSEL_SHIFT(x)((x) << 22)
+
+#define EMMC_PHYSTAT_REG 0xbc
+#define CALDONE_MASK 1
+#define DLLRDY_MASK1
+#define IS_CALDONE(x) x) >> 9) & CALDONE_MASK) == 1)
+#define IS_DLLRDY(x) x) >> 8) & DLLRDY_MASK) == 1)
These are inconsistent with above:
#define CALDONE_MASKBIT(9)
...
#define IS_CALDONE ((x) & CALDONE_MASK)
Note redundant == part.
Agreed, will update.
+static int intel_emmc_phy_power(struct phy *phy, bool on_off)
+{
+* - PHY driver to probe
+* - SDHCI driver to start probe
+* - SDHCI driver to register it's clock
+* - SDHCI driver to get the PHY
+* - SDHCI driver to init the PHY
+*
+* The clock is optional, so upon any error we just set to NULL.
No, the clock framework will do it for you.
+*
+* NOTE: we don't do anything special for EPROBE_DEFER here. Given the
+* above expected use case, EPROBE_DEFER isn't sensible to expect, so
+* it's just like any other error.
This comment is not correct...
Agreed, re-structure the sentence.
+*/
+ priv->emmcclk = clk_get_optional(&phy->dev, "emmcclk");
+ if (IS_ERR(priv->emmcclk)) {
+ dev_warn(&phy->dev, "ERROR: getting emmcclk\n");
...because here you have to return an error...
Agreed.
+ priv->emmcclk = NULL;
...and here is redundant assignment.
Agreed.
+ }
+
+ return 0;
+}
When you send out patches, check that you do this for latest version you got
reviewed internally.
Thank you so much for the review comments, sure I will recheck and
recollect your review comments of different patches
for the same cases .
With Best Regards
vadivel
Re: [PATCH v1 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
On Mon, Aug 19, 2019 at 11:44:16AM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan
>
> Adds support for eMMC PHY on Intel's Lightning Mountain SoC.
Adds -> Add.
> +/* 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)
> +#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 1
BIT(0)
> +#define ENDLL_MASK BIT(7)
> +#define ENDLL_VALBIT(7)
> +
> +#define EMMC_PHYCTRL2_REG0xb0
> +#define FRQSEL_25M 0
> +#define FRQSEL_150M 3
> +#define FRQSEL_MASK GENMASK(24, 22)
> +#define FRQSEL_SHIFT(x) ((x) << 22)
> +
> +#define EMMC_PHYSTAT_REG 0xbc
> +#define CALDONE_MASK 1
> +#define DLLRDY_MASK 1
> +#define IS_CALDONE(x)x) >> 9) & CALDONE_MASK) == 1)
> +#define IS_DLLRDY(x) x) >> 8) & DLLRDY_MASK) == 1)
These are inconsistent with above:
#define CALDONE_MASKBIT(9)
...
#define IS_CALDONE ((x) & CALDONE_MASK)
Note redundant == part.
> +static int intel_emmc_phy_power(struct phy *phy, bool on_off)
> +{
> + * - PHY driver to probe
> + * - SDHCI driver to start probe
> + * - SDHCI driver to register it's clock
> + * - SDHCI driver to get the PHY
> + * - SDHCI driver to init the PHY
> + *
> + * The clock is optional, so upon any error we just set to NULL.
No, the clock framework will do it for you.
> + *
> + * NOTE: we don't do anything special for EPROBE_DEFER here. Given the
> + * above expected use case, EPROBE_DEFER isn't sensible to expect, so
> + * it's just like any other error.
This comment is not correct...
> + */
> + priv->emmcclk = clk_get_optional(&phy->dev, "emmcclk");
> + if (IS_ERR(priv->emmcclk)) {
> + dev_warn(&phy->dev, "ERROR: getting emmcclk\n");
...because here you have to return an error...
> + priv->emmcclk = NULL;
...and here is redundant assignment.
> + }
> +
> + return 0;
> +}
When you send out patches, check that you do this for latest version you got
reviewed internally.
--
With Best Regards,
Andy Shevchenko
[PATCH v1 2/2] phy: intel-lgm-emmc: Add support for eMMC PHY
From: Ramuthevar Vadivel Murugan
Adds support for eMMC PHY on Intel's Lightning Mountain SoC.
Signed-off-by: Ramuthevar Vadivel Murugan
---
drivers/phy/Kconfig| 1 +
drivers/phy/Makefile | 1 +
drivers/phy/intel/Kconfig | 8 ++
drivers/phy/intel/Makefile | 2 +
drivers/phy/intel/phy-intel-emmc.c | 276 +
5 files changed, 288 insertions(+)
create mode 100644 drivers/phy/intel/Kconfig
create mode 100644 drivers/phy/intel/Makefile
create mode 100644 drivers/phy/intel/phy-intel-emmc.c
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 0263db2ac874..b3ed94b98d9b 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -69,5 +69,6 @@ source "drivers/phy/socionext/Kconfig"
source "drivers/phy/st/Kconfig"
source "drivers/phy/tegra/Kconfig"
source "drivers/phy/ti/Kconfig"
+source "drivers/phy/intel/Kconfig"
endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 0d9fddc498a6..3f1fc9efbbed 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -19,6 +19,7 @@ obj-y += broadcom/\
cadence/ \
freescale/ \
hisilicon/ \
+ intel/ \
marvell/ \
motorola/\
mscc/\
diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
new file mode 100644
index ..aa34e0fa9824
--- /dev/null
+++ b/drivers/phy/intel/Kconfig
@@ -0,0 +1,8 @@
+#
+# Phy drivers for Intel X86 LGM platform
+#
+config PHY_INTEL_EMMC
+ tristate "Intel EMMC PHY driver"
+ select GENERIC_PHY
+ help
+ Enable this to support the Intel EMMC PHY
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
new file mode 100644
index ..6b876a75599d
--- /dev/null
+++ b/drivers/phy/intel/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o
diff --git a/drivers/phy/intel/phy-intel-emmc.c
b/drivers/phy/intel/phy-intel-emmc.c
new file mode 100644
index ..2fd4e42f18f8
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-emmc.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel eMMC PHY driver
+ * Copyright (C) 2019 Intel, Corp.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* 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)
+#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 1
+#define ENDLL_MASK BIT(7)
+#define ENDLL_VAL BIT(7)
+
+#define EMMC_PHYCTRL2_REG 0xb0
+#define FRQSEL_25M 0
+#define FRQSEL_150M3
+#define FRQSEL_MASKGENMASK(24, 22)
+#define FRQSEL_SHIFT(x)((x) << 22)
+
+#define EMMC_PHYSTAT_REG 0xbc
+#define CALDONE_MASK 1
+#define DLLRDY_MASK1
+#define IS_CALDONE(x) x) >> 9) & CALDONE_MASK) == 1)
+#define IS_DLLRDY(x) x) >> 8) & DLLRDY_MASK) == 1)
+
+struct intel_emmc_phy {
+ struct regmap *syscfg;
+ struct clk *emmcclk;
+};
+
+static int intel_emmc_phy_power(struct phy *phy, bool on_off)
+{
+ struct intel_emmc_phy *priv = phy_get_drvdata(phy);
+ unsigned int caldone;
+ unsigned int dllrdy;
+ unsigned int freqsel = 0;
+ unsigned long rate;
+ int ret, quot;
+
+ /*
+* Keep phyctrl_pdb and phyctrl_endll low to allow
+* initialization of CALIO state M/C DFFs
+*/
+ ret = regmap_update_bits(priv->syscfg, EMMC_PHYCTRL1_REG,
+PDB_MASK | ENDLL_MASK, 0);
+ if (ret) {
+ dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Already finish power_off above */
+ if (!on_off)
+ return 0;
+
+ rate = clk_get_rate(priv->emmcclk);
+ quot = DIV_ROUND_CLOSEST(rate, 5000);
+ if (quot > FRQSEL_150M)
+ dev_warn(&phy->dev, "Unsupported rate: %lu\n", rate);
+ freqsel = clamp_t(int, quot, FRQSEL_25M, FRQSEL_150M);
+
+ /*
+* 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);
+ regmap_update_bits(priv->syscfg, EMMC_PHYC
