[PATCH v7 0/3] phy: intel: Add Keem Bay eMMC PHY support

[PATCH v7 0/3] phy: intel: Add Keem Bay eMMC PHY support

[Wan Ahmad Zainie]

Hi.

The first patch is new and added to rename rename
phy-intel-{combo,emmc}.c to phy-intel-lgm-{combo,emmc}.c..

The second patch is to document DT bindings for Keem Bay eMMC PHY.

The the third is the driver file, loosely based on phy-rockchip-emmc.c
and phy-intel-emmc.c. The latter is not being reused as there are
quite a number of differences i.e. registers offset, supported clock
rates, bitfield to set.

The patch was tested with Keem Bay evaluation module board.

Thank you.

Best regards,
Zainie

Changes since v6:
- Add new patch to rename phy-intel-{combo,emmc}.c to
  phy-intel-lgm-{combo,emmc}.c.
- Add Reviewed-by: Rob Herring <robh@kernel.org> tag to 2nd patch.
- In 3rd patch,
  - add and use generic helper to update register bits,
  - end module name with .ko in Kconfig help.

Changes since resend v5:
- In keembay_emmc_phy_power(), remove redundant return ret;.

Resend v5:
- Include missing changes done for phy-keembay-emmc.c (See Changes since
  v3)

Changes since v4:
- In Kconfig file, add depends on ARM64 || COMPILE_TEST and
  depends on OF && HAS_IOMEM.
- In Kconfig file, reword the help section to be more verbose.

Changes since v3:
- Exit keembay_emmc_phy_power() with return ret;.
- In keembay_emmc_phy_init(), use PTR_ERR_OR_ZERO(...).
- In keembay_emmc_phy_probe(), devm_regmap_init_mmio(...) in single
  line.

Changes since v2:
- Modify DT example to use single cell for address and size.

Changes since v1:
- Rework phy-keembay-emmc.c to make it similar to phy-intel-emmc.c.
- Use regmap_mmio, and remove reference to intel,syscon.
- Use node name phy@....
- Update license i.e. use dual license.


Wan Ahmad Zainie (3):
  phy: intel: Rename phy-intel to phy-intel-lgm
  dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
  phy: intel: Add Keem Bay eMMC PHY support

 .../bindings/phy/intel,keembay-emmc-phy.yaml  |  44 +++
 drivers/phy/intel/Kconfig                     |  22 +-
 drivers/phy/intel/Makefile                    |   5 +-
 drivers/phy/intel/phy-intel-keembay-emmc.c    | 310 ++++++++++++++++++
 ...hy-intel-combo.c => phy-intel-lgm-combo.c} |   0
 ...{phy-intel-emmc.c => phy-intel-lgm-emmc.c} |   0
 6 files changed, 374 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
 create mode 100644 drivers/phy/intel/phy-intel-keembay-emmc.c
 rename drivers/phy/intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} (100%)
 rename drivers/phy/intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c} (100%)

-- 
2.17.1

[PATCH v7 1/3] phy: intel: Rename phy-intel to phy-intel-lgm

[Wan Ahmad Zainie]

Rename phy-intel-{combo,emmc}.c to phy-intel-lgm-{combo,emmc}.c
to make drivers/phy/intel directory more generic for future use.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 drivers/phy/intel/Kconfig                              | 10 +++++-----
 drivers/phy/intel/Makefile                             |  4 ++--
 .../intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} |  0
 .../intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c}   |  0
 4 files changed, 7 insertions(+), 7 deletions(-)
 rename drivers/phy/intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} (100%)
 rename drivers/phy/intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c} (100%)

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 7b47682a4e0e..db8586c3eed8 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -1,9 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# Phy drivers for Intel Lightning Mountain(LGM) platform
+# Phy drivers for Intel platforms
 #
-config PHY_INTEL_COMBO
-	bool "Intel ComboPHY driver"
+config PHY_INTEL_LGM_COMBO
+	bool "Intel Lightning Mountain ComboPHY driver"
 	depends on X86 || COMPILE_TEST
 	depends on OF && HAS_IOMEM
 	select MFD_SYSCON
@@ -16,8 +16,8 @@ config PHY_INTEL_COMBO
 	  chipsets which provides PHYs for various controllers, EMAC,
 	  SATA and PCIe.
 
-config PHY_INTEL_EMMC
-	tristate "Intel EMMC PHY driver"
+config PHY_INTEL_LGM_EMMC
+	tristate "Intel Lightning Mountain EMMC PHY driver"
 	depends on X86 || COMPILE_TEST
 	select GENERIC_PHY
 	help
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 233d530dadde..662385d0a366 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.o
-obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
+obj-$(CONFIG_PHY_INTEL_LGM_COMBO)	+= phy-intel-lgm-combo.o
+obj-$(CONFIG_PHY_INTEL_LGM_EMMC)	+= phy-intel-lgm-emmc.o
diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-lgm-combo.c
similarity index 100%
rename from drivers/phy/intel/phy-intel-combo.c
rename to drivers/phy/intel/phy-intel-lgm-combo.c
diff --git a/drivers/phy/intel/phy-intel-emmc.c b/drivers/phy/intel/phy-intel-lgm-emmc.c
similarity index 100%
rename from drivers/phy/intel/phy-intel-emmc.c
rename to drivers/phy/intel/phy-intel-lgm-emmc.c
-- 
2.17.1

[PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings

[Wan Ahmad Zainie]

Binding description for Intel Keem Bay eMMC PHY.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/phy/intel,keembay-emmc-phy.yaml  | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
new file mode 100644
index 000000000000..4cbbd3887c13
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-phy.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel Keem Bay eMMC PHY bindings
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+
+properties:
+  compatible:
+    const: intel,keembay-emmc-phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: emmcclk
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    phy@20290000 {
+          compatible = "intel,keembay-emmc-phy";
+          reg = <0x20290000 0x54>;
+          clocks = <&emmc>;
+          clock-names = "emmcclk";
+          #phy-cells = <0>;
+    };
-- 
2.17.1

[PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support

[Wan Ahmad Zainie]

Add support for eMMC PHY on Intel Keem Bay SoC.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 drivers/phy/intel/Kconfig                  |  12 +
 drivers/phy/intel/Makefile                 |   1 +
 drivers/phy/intel/phy-intel-keembay-emmc.c | 310 +++++++++++++++++++++
 3 files changed, 323 insertions(+)
 create mode 100644 drivers/phy/intel/phy-intel-keembay-emmc.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index db8586c3eed8..dad72fc64871 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -2,6 +2,18 @@
 #
 # Phy drivers for Intel platforms
 #
+config PHY_INTEL_KEEMBAY_EMMC
+	tristate "Intel Keem Bay EMMC PHY driver"
+	depends on ARM64 || COMPILE_TEST
+	depends on OF && HAS_IOMEM
+	select GENERIC_PHY
+	select REGMAP_MMIO
+	help
+	  Choose this option if you have an Intel Keem Bay SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called phy-keembay-emmc.ko.
+
 config PHY_INTEL_LGM_COMBO
 	bool "Intel Lightning Mountain ComboPHY driver"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 662385d0a366..a5e0af5ccd75 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_INTEL_KEEMBAY_EMMC)	+= phy-intel-keembay-emmc.o
 obj-$(CONFIG_PHY_INTEL_LGM_COMBO)	+= phy-intel-lgm-combo.o
 obj-$(CONFIG_PHY_INTEL_LGM_EMMC)	+= phy-intel-lgm-emmc.o
diff --git a/drivers/phy/intel/phy-intel-keembay-emmc.c b/drivers/phy/intel/phy-intel-keembay-emmc.c
new file mode 100644
index 000000000000..c1a095c17165
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-keembay-emmc.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Keem Bay eMMC PHY driver
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* eMMC/SD/SDIO core/phy configuration registers */
+#define PHY_CFG_0		0x24
+#define  SEL_DLY_TXCLK_MASK	BIT(29)
+#define  OTAP_DLY_ENA_MASK	BIT(27)
+#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
+#define  DLL_EN_MASK		BIT(10)
+#define  PWR_DOWN_MASK		BIT(0)
+
+#define PHY_CFG_2		0x2c
+#define  SEL_FREQ_MASK		GENMASK(12, 10)
+
+#define PHY_STAT		0x40
+#define  CAL_DONE_MASK		BIT(6)
+#define  IS_CALDONE(x)		((x) & CAL_DONE_MASK)
+#define  DLL_RDY_MASK		BIT(5)
+#define  IS_DLLRDY(x)		((x) & DLL_RDY_MASK)
+
+/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */
+#define FREQSEL_200M_170M	0x0
+#define FREQSEL_170M_140M	0x1
+#define FREQSEL_140M_110M	0x2
+#define FREQSEL_110M_80M	0x3
+#define FREQSEL_80M_50M		0x4
+
+#define maskval(mask, val)	(((val) << (ffs(mask) - 1)) & mask)
+
+struct keembay_emmc_phy {
+	struct regmap *syscfg;
+	struct clk *emmcclk;
+};
+
+static const struct regmap_config keembay_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int keembay_emmc_phy_power(struct phy *phy, bool on_off)
+{
+	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
+	unsigned int caldone;
+	unsigned int dllrdy;
+	unsigned int freqsel;
+	unsigned int mhz;
+	int ret;
+
+	/*
+	 * Keep phyctrl_pdb and phyctrl_endll low to allow
+	 * initialization of CALIO state M/C DFFs
+	 */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, PWR_DOWN_MASK,
+				 maskval(PWR_DOWN_MASK, 0));
+	if (ret) {
+		dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, DLL_EN_MASK,
+				 maskval(DLL_EN_MASK, 0));
+	if (ret) {
+		dev_err(&phy->dev, "turn off the dll failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Already finish power off above */
+	if (!on_off)
+		return 0;
+
+	mhz = DIV_ROUND_CLOSEST(clk_get_rate(priv->emmcclk), 1000000);
+	if (mhz <= 200 && mhz >= 170)
+		freqsel = FREQSEL_200M_170M;
+	else if (mhz <= 170 && mhz >= 140)
+		freqsel = FREQSEL_170M_140M;
+	else if (mhz <= 140 && mhz >= 110)
+		freqsel = FREQSEL_140M_110M;
+	else if (mhz <= 110 && mhz >= 80)
+		freqsel = FREQSEL_110M_80M;
+	else if (mhz <= 80 && mhz >= 50)
+		freqsel = FREQSEL_80M_50M;
+	else
+		freqsel = 0x0;
+
+	if (mhz < 50 || mhz > 200)
+		dev_warn(&phy->dev, "Unsupported rate: %d MHz\n", mhz);
+
+	/*
+	 * 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
+	 */
+	udelay(5);
+
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, PWR_DOWN_MASK,
+				 maskval(PWR_DOWN_MASK, 1));
+	if (ret) {
+		dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * According to the user manual, it asks driver to wait 5us for
+	 * calpad busy trimming. However it is documented that this value is
+	 * PVT(A.K.A. process, voltage and temperature) relevant, so some
+	 * failure cases are found which indicates we should be more tolerant
+	 * to calpad busy trimming.
+	 */
+	ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
+				       caldone, IS_CALDONE(caldone),
+				       0, 50);
+	if (ret) {
+		dev_err(&phy->dev, "caldone failed, ret=%d\n", ret);
+		return ret;
+	}
+
+	/* Set the frequency of the DLL operation */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_2, SEL_FREQ_MASK,
+				 maskval(SEL_FREQ_MASK, freqsel));
+	if (ret) {
+		dev_err(&phy->dev, "set the frequency of dll failed:%d\n", ret);
+		return ret;
+	}
+
+	/* Turn on the DLL */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, DLL_EN_MASK,
+				 maskval(DLL_EN_MASK, 1));
+	if (ret) {
+		dev_err(&phy->dev, "turn on the dll failed: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * We turned on the DLL even though the rate was 0 because we the
+	 * clock might be turned on later.  ...but we can't wait for the DLL
+	 * to lock when the rate is 0 because it will never lock with no
+	 * input clock.
+	 *
+	 * Technically we should be checking the lock later when the clock
+	 * is turned on, but for now we won't.
+	 */
+	if (mhz == 0)
+		return 0;
+
+	/*
+	 * After enabling analog DLL circuits docs say that we need 10.2 us if
+	 * our source clock is at 50 MHz and that lock time scales linearly
+	 * with clock speed. If we are powering on the PHY and the card clock
+	 * is super slow (like 100kHz) this could take as long as 5.1 ms as
+	 * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms
+	 * hopefully we won't be running at 100 kHz, but we should still make
+	 * sure we wait long enough.
+	 *
+	 * NOTE: There appear to be corner cases where the DLL seems to take
+	 * extra long to lock for reasons that aren't understood. In some
+	 * extreme cases we've seen it take up to over 10ms (!). We'll be
+	 * generous and give it 50ms.
+	 */
+	ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
+				       dllrdy, IS_DLLRDY(dllrdy),
+				       0, 50 * USEC_PER_MSEC);
+	if (ret)
+		dev_err(&phy->dev, "dllrdy failed, ret=%d\n", ret);
+
+	return ret;
+}
+
+static int keembay_emmc_phy_init(struct phy *phy)
+{
+	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
+
+	/*
+	 * We purposely get the clock here and not in probe to avoid the
+	 * circular dependency problem. We expect:
+	 * - 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 just return it like
+	 * any other error to user.
+	 */
+	priv->emmcclk = clk_get_optional(&phy->dev, "emmcclk");
+
+	return PTR_ERR_OR_ZERO(priv->emmcclk);
+}
+
+static int keembay_emmc_phy_exit(struct phy *phy)
+{
+	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
+
+	clk_put(priv->emmcclk);
+
+	return 0;
+};
+
+static int keembay_emmc_phy_power_on(struct phy *phy)
+{
+	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
+	int ret;
+
+	/* Delay chain based txclk: enable */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, SEL_DLY_TXCLK_MASK,
+				 maskval(SEL_DLY_TXCLK_MASK, 1));
+	if (ret) {
+		dev_err(&phy->dev, "ERROR: delay chain txclk set: %d\n", ret);
+		return ret;
+	}
+
+	/* Output tap delay: enable */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, OTAP_DLY_ENA_MASK,
+				 maskval(OTAP_DLY_ENA_MASK, 1));
+	if (ret) {
+		dev_err(&phy->dev, "ERROR: output tap delay set: %d\n", ret);
+		return ret;
+	}
+
+	/* Output tap delay */
+	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, OTAP_DLY_SEL_MASK,
+				 maskval(OTAP_DLY_SEL_MASK, 2));
+	if (ret) {
+		dev_err(&phy->dev, "ERROR: output tap delay select: %d\n", ret);
+		return ret;
+	}
+
+	/* Power up eMMC phy analog blocks */
+	return keembay_emmc_phy_power(phy, true);
+}
+
+static int keembay_emmc_phy_power_off(struct phy *phy)
+{
+	/* Power down eMMC phy analog blocks */
+	return keembay_emmc_phy_power(phy, false);
+}
+
+static const struct phy_ops ops = {
+	.init		= keembay_emmc_phy_init,
+	.exit		= keembay_emmc_phy_exit,
+	.power_on	= keembay_emmc_phy_power_on,
+	.power_off	= keembay_emmc_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int keembay_emmc_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct keembay_emmc_phy *priv;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	void __iomem *base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->syscfg = devm_regmap_init_mmio(dev, base, &keembay_regmap_config);
+	if (IS_ERR(priv->syscfg))
+		return PTR_ERR(priv->syscfg);
+
+	generic_phy = devm_phy_create(dev, np, &ops);
+	if (IS_ERR(generic_phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		return PTR_ERR(generic_phy);
+	}
+
+	phy_set_drvdata(generic_phy, priv);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id keembay_emmc_phy_dt_ids[] = {
+	{ .compatible = "intel,keembay-emmc-phy" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, keembay_emmc_phy_dt_ids);
+
+static struct platform_driver keembay_emmc_phy_driver = {
+	.probe		= keembay_emmc_phy_probe,
+	.driver		= {
+		.name	= "keembay-emmc-phy",
+		.of_match_table = keembay_emmc_phy_dt_ids,
+	},
+};
+module_platform_driver(keembay_emmc_phy_driver);
+
+MODULE_AUTHOR("Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>");
+MODULE_DESCRIPTION("Intel Keem Bay eMMC PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support

[Andy Shevchenko]

On Fri, Aug 21, 2020 at 07:37:47PM +0800, Wan Ahmad Zainie wrote:
> Add support for eMMC PHY on Intel Keem Bay SoC.

Thanks for an update. I have couple of nit-picks below, otherwise, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  drivers/phy/intel/Kconfig                  |  12 +
>  drivers/phy/intel/Makefile                 |   1 +
>  drivers/phy/intel/phy-intel-keembay-emmc.c | 310 +++++++++++++++++++++
>  3 files changed, 323 insertions(+)
>  create mode 100644 drivers/phy/intel/phy-intel-keembay-emmc.c
> 
> diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> index db8586c3eed8..dad72fc64871 100644
> --- a/drivers/phy/intel/Kconfig
> +++ b/drivers/phy/intel/Kconfig
> @@ -2,6 +2,18 @@
>  #
>  # Phy drivers for Intel platforms
>  #
> +config PHY_INTEL_KEEMBAY_EMMC
> +	tristate "Intel Keem Bay EMMC PHY driver"

> +	depends on ARM64 || COMPILE_TEST
> +	depends on OF && HAS_IOMEM

It might be better from testing coverage perspective to have it like

	depends on HAS_IOMEM
	depends on (OF && ARM64) || COMPILE_TEST

> +	select GENERIC_PHY
> +	select REGMAP_MMIO
> +	help
> +	  Choose this option if you have an Intel Keem Bay SoC.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called phy-keembay-emmc.ko.
> +
>  config PHY_INTEL_LGM_COMBO
>  	bool "Intel Lightning Mountain ComboPHY driver"
>  	depends on X86 || COMPILE_TEST
> diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> index 662385d0a366..a5e0af5ccd75 100644
> --- a/drivers/phy/intel/Makefile
> +++ b/drivers/phy/intel/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_PHY_INTEL_KEEMBAY_EMMC)	+= phy-intel-keembay-emmc.o
>  obj-$(CONFIG_PHY_INTEL_LGM_COMBO)	+= phy-intel-lgm-combo.o
>  obj-$(CONFIG_PHY_INTEL_LGM_EMMC)	+= phy-intel-lgm-emmc.o
> diff --git a/drivers/phy/intel/phy-intel-keembay-emmc.c b/drivers/phy/intel/phy-intel-keembay-emmc.c
> new file mode 100644
> index 000000000000..c1a095c17165
> --- /dev/null
> +++ b/drivers/phy/intel/phy-intel-keembay-emmc.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Keem Bay eMMC PHY driver
> + * Copyright (C) 2020 Intel Corporation
> + */

Missed bitops.h

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +/* eMMC/SD/SDIO core/phy configuration registers */
> +#define PHY_CFG_0		0x24
> +#define  SEL_DLY_TXCLK_MASK	BIT(29)
> +#define  OTAP_DLY_ENA_MASK	BIT(27)
> +#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
> +#define  DLL_EN_MASK		BIT(10)
> +#define  PWR_DOWN_MASK		BIT(0)
> +
> +#define PHY_CFG_2		0x2c
> +#define  SEL_FREQ_MASK		GENMASK(12, 10)
> +
> +#define PHY_STAT		0x40
> +#define  CAL_DONE_MASK		BIT(6)
> +#define  IS_CALDONE(x)		((x) & CAL_DONE_MASK)
> +#define  DLL_RDY_MASK		BIT(5)
> +#define  IS_DLLRDY(x)		((x) & DLL_RDY_MASK)
> +
> +/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */
> +#define FREQSEL_200M_170M	0x0
> +#define FREQSEL_170M_140M	0x1
> +#define FREQSEL_140M_110M	0x2
> +#define FREQSEL_110M_80M	0x3
> +#define FREQSEL_80M_50M		0x4
> +
> +#define maskval(mask, val)	(((val) << (ffs(mask) - 1)) & mask)
> +
> +struct keembay_emmc_phy {
> +	struct regmap *syscfg;
> +	struct clk *emmcclk;
> +};
> +
> +static const struct regmap_config keembay_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int keembay_emmc_phy_power(struct phy *phy, bool on_off)
> +{
> +	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
> +	unsigned int caldone;
> +	unsigned int dllrdy;
> +	unsigned int freqsel;
> +	unsigned int mhz;
> +	int ret;
> +
> +	/*
> +	 * Keep phyctrl_pdb and phyctrl_endll low to allow
> +	 * initialization of CALIO state M/C DFFs
> +	 */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, PWR_DOWN_MASK,
> +				 maskval(PWR_DOWN_MASK, 0));
> +	if (ret) {
> +		dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, DLL_EN_MASK,
> +				 maskval(DLL_EN_MASK, 0));
> +	if (ret) {
> +		dev_err(&phy->dev, "turn off the dll failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Already finish power off above */
> +	if (!on_off)
> +		return 0;
> +
> +	mhz = DIV_ROUND_CLOSEST(clk_get_rate(priv->emmcclk), 1000000);
> +	if (mhz <= 200 && mhz >= 170)
> +		freqsel = FREQSEL_200M_170M;
> +	else if (mhz <= 170 && mhz >= 140)
> +		freqsel = FREQSEL_170M_140M;
> +	else if (mhz <= 140 && mhz >= 110)
> +		freqsel = FREQSEL_140M_110M;
> +	else if (mhz <= 110 && mhz >= 80)
> +		freqsel = FREQSEL_110M_80M;
> +	else if (mhz <= 80 && mhz >= 50)
> +		freqsel = FREQSEL_80M_50M;
> +	else
> +		freqsel = 0x0;
> +
> +	if (mhz < 50 || mhz > 200)
> +		dev_warn(&phy->dev, "Unsupported rate: %d MHz\n", mhz);
> +
> +	/*
> +	 * 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
> +	 */
> +	udelay(5);
> +
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, PWR_DOWN_MASK,
> +				 maskval(PWR_DOWN_MASK, 1));
> +	if (ret) {
> +		dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * According to the user manual, it asks driver to wait 5us for
> +	 * calpad busy trimming. However it is documented that this value is
> +	 * PVT(A.K.A. process, voltage and temperature) relevant, so some
> +	 * failure cases are found which indicates we should be more tolerant
> +	 * to calpad busy trimming.
> +	 */
> +	ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
> +				       caldone, IS_CALDONE(caldone),
> +				       0, 50);
> +	if (ret) {
> +		dev_err(&phy->dev, "caldone failed, ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Set the frequency of the DLL operation */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_2, SEL_FREQ_MASK,
> +				 maskval(SEL_FREQ_MASK, freqsel));
> +	if (ret) {
> +		dev_err(&phy->dev, "set the frequency of dll failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Turn on the DLL */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, DLL_EN_MASK,
> +				 maskval(DLL_EN_MASK, 1));
> +	if (ret) {
> +		dev_err(&phy->dev, "turn on the dll failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * We turned on the DLL even though the rate was 0 because we the
> +	 * clock might be turned on later.  ...but we can't wait for the DLL
> +	 * to lock when the rate is 0 because it will never lock with no
> +	 * input clock.
> +	 *
> +	 * Technically we should be checking the lock later when the clock
> +	 * is turned on, but for now we won't.
> +	 */
> +	if (mhz == 0)
> +		return 0;
> +
> +	/*
> +	 * After enabling analog DLL circuits docs say that we need 10.2 us if
> +	 * our source clock is at 50 MHz and that lock time scales linearly
> +	 * with clock speed. If we are powering on the PHY and the card clock
> +	 * is super slow (like 100kHz) this could take as long as 5.1 ms as
> +	 * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms
> +	 * hopefully we won't be running at 100 kHz, but we should still make
> +	 * sure we wait long enough.
> +	 *
> +	 * NOTE: There appear to be corner cases where the DLL seems to take
> +	 * extra long to lock for reasons that aren't understood. In some
> +	 * extreme cases we've seen it take up to over 10ms (!). We'll be
> +	 * generous and give it 50ms.
> +	 */
> +	ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
> +				       dllrdy, IS_DLLRDY(dllrdy),
> +				       0, 50 * USEC_PER_MSEC);
> +	if (ret)
> +		dev_err(&phy->dev, "dllrdy failed, ret=%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int keembay_emmc_phy_init(struct phy *phy)
> +{
> +	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
> +
> +	/*
> +	 * We purposely get the clock here and not in probe to avoid the
> +	 * circular dependency problem. We expect:
> +	 * - 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 just return it like
> +	 * any other error to user.
> +	 */
> +	priv->emmcclk = clk_get_optional(&phy->dev, "emmcclk");
> +
> +	return PTR_ERR_OR_ZERO(priv->emmcclk);
> +}
> +
> +static int keembay_emmc_phy_exit(struct phy *phy)
> +{
> +	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
> +
> +	clk_put(priv->emmcclk);
> +
> +	return 0;
> +};
> +
> +static int keembay_emmc_phy_power_on(struct phy *phy)
> +{
> +	struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
> +	int ret;
> +
> +	/* Delay chain based txclk: enable */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, SEL_DLY_TXCLK_MASK,
> +				 maskval(SEL_DLY_TXCLK_MASK, 1));
> +	if (ret) {
> +		dev_err(&phy->dev, "ERROR: delay chain txclk set: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Output tap delay: enable */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, OTAP_DLY_ENA_MASK,
> +				 maskval(OTAP_DLY_ENA_MASK, 1));
> +	if (ret) {
> +		dev_err(&phy->dev, "ERROR: output tap delay set: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Output tap delay */
> +	ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, OTAP_DLY_SEL_MASK,
> +				 maskval(OTAP_DLY_SEL_MASK, 2));
> +	if (ret) {
> +		dev_err(&phy->dev, "ERROR: output tap delay select: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Power up eMMC phy analog blocks */
> +	return keembay_emmc_phy_power(phy, true);
> +}
> +
> +static int keembay_emmc_phy_power_off(struct phy *phy)
> +{
> +	/* Power down eMMC phy analog blocks */
> +	return keembay_emmc_phy_power(phy, false);
> +}
> +
> +static const struct phy_ops ops = {
> +	.init		= keembay_emmc_phy_init,
> +	.exit		= keembay_emmc_phy_exit,
> +	.power_on	= keembay_emmc_phy_power_on,
> +	.power_off	= keembay_emmc_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int keembay_emmc_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct keembay_emmc_phy *priv;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	void __iomem *base;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->syscfg = devm_regmap_init_mmio(dev, base, &keembay_regmap_config);
> +	if (IS_ERR(priv->syscfg))
> +		return PTR_ERR(priv->syscfg);
> +
> +	generic_phy = devm_phy_create(dev, np, &ops);
> +	if (IS_ERR(generic_phy)) {

> +		dev_err(dev, "failed to create PHY\n");
> +		return PTR_ERR(generic_phy);

I guess now you may use

	return dev_err_probe(...);

> +	}
> +
> +	phy_set_drvdata(generic_phy, priv);
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id keembay_emmc_phy_dt_ids[] = {
> +	{ .compatible = "intel,keembay-emmc-phy" },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, keembay_emmc_phy_dt_ids);
> +
> +static struct platform_driver keembay_emmc_phy_driver = {
> +	.probe		= keembay_emmc_phy_probe,
> +	.driver		= {
> +		.name	= "keembay-emmc-phy",
> +		.of_match_table = keembay_emmc_phy_dt_ids,
> +	},
> +};
> +module_platform_driver(keembay_emmc_phy_driver);
> +
> +MODULE_AUTHOR("Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>");
> +MODULE_DESCRIPTION("Intel Keem Bay eMMC PHY driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/3] phy: intel: Rename phy-intel to phy-intel-lgm

[Ramuthevar, Vadivel MuruganX]

Hi,

On 21/8/2020 7:37 pm, Wan Ahmad Zainie wrote:
> Rename phy-intel-{combo,emmc}.c to phy-intel-lgm-{combo,emmc}.c
> to make drivers/phy/intel directory more generic for future use.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>   drivers/phy/intel/Kconfig                              | 10 +++++-----
>   drivers/phy/intel/Makefile                             |  4 ++--
>   .../intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} |  0
>   .../intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c}   |  0
>   4 files changed, 7 insertions(+), 7 deletions(-)
>   rename drivers/phy/intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} (100%)
>   rename drivers/phy/intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c} (100%)
> 
> diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> index 7b47682a4e0e..db8586c3eed8 100644
> --- a/drivers/phy/intel/Kconfig
> +++ b/drivers/phy/intel/Kconfig
> @@ -1,9 +1,9 @@
>   # SPDX-License-Identifier: GPL-2.0
>   #
> -# Phy drivers for Intel Lightning Mountain(LGM) platform
> +# Phy drivers for Intel platforms
>   #
> -config PHY_INTEL_COMBO
> -	bool "Intel ComboPHY driver"
> +config PHY_INTEL_LGM_COMBO
> +	bool "Intel Lightning Mountain ComboPHY driver"
>   	depends on X86 || COMPILE_TEST
>   	depends on OF && HAS_IOMEM
>   	select MFD_SYSCON
> @@ -16,8 +16,8 @@ config PHY_INTEL_COMBO
>   	  chipsets which provides PHYs for various controllers, EMAC,
>   	  SATA and PCIe.
>   
> -config PHY_INTEL_EMMC
> -	tristate "Intel EMMC PHY driver"
> +config PHY_INTEL_LGM_EMMC
> +	tristate "Intel Lightning Mountain EMMC PHY driver"
>   	depends on X86 || COMPILE_TEST
>   	select GENERIC_PHY
>   	help
> diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> index 233d530dadde..662385d0a366 100644
> --- a/drivers/phy/intel/Makefile
> +++ b/drivers/phy/intel/Makefile
> @@ -1,3 +1,3 @@
>   # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.o
> -obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
> +obj-$(CONFIG_PHY_INTEL_LGM_COMBO)	+= phy-intel-lgm-combo.o
> +obj-$(CONFIG_PHY_INTEL_LGM_EMMC)	+= phy-intel-lgm-emmc.o
> diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-lgm-combo.c
> similarity index 100%
> rename from drivers/phy/intel/phy-intel-combo.c
> rename to drivers/phy/intel/phy-intel-lgm-combo.c
> diff --git a/drivers/phy/intel/phy-intel-emmc.c b/drivers/phy/intel/phy-intel-lgm-emmc.c
> similarity index 100%
> rename from drivers/phy/intel/phy-intel-emmc.c
> rename to drivers/phy/intel/phy-intel-lgm-emmc.c
Reviewed-by:Ramuthevar Vadivel Murugan 
<vadivel.muruganx.ramuthevar@linux.intel.com>

Regards
Vadivel
> 

Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings

[Vinod Koul]

On 21-08-20, 19:37, Wan Ahmad Zainie wrote:
> Binding description for Intel Keem Bay eMMC PHY.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../bindings/phy/intel,keembay-emmc-phy.yaml  | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> new file mode 100644
> index 000000000000..4cbbd3887c13
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-phy.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel Keem Bay eMMC PHY bindings

This seems same as
Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why not
add a new compatible in lgm binding, or did I miss a difference?

> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,keembay-emmc-phy
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: emmcclk
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    phy@20290000 {
> +          compatible = "intel,keembay-emmc-phy";
> +          reg = <0x20290000 0x54>;
> +          clocks = <&emmc>;
> +          clock-names = "emmcclk";
> +          #phy-cells = <0>;
> +    };
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support

[Vinod Koul]

On 21-08-20, 19:37, Wan Ahmad Zainie wrote:

> +/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */
> +#define FREQSEL_200M_170M	0x0
> +#define FREQSEL_170M_140M	0x1
> +#define FREQSEL_140M_110M	0x2
> +#define FREQSEL_110M_80M	0x3
> +#define FREQSEL_80M_50M		0x4
> +
> +#define maskval(mask, val)	(((val) << (ffs(mask) - 1)) & mask)

Kernel has a macro do this for you, please use FIELD_PREP instead of

your own macro
-- 
~Vinod

RE: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support

[Wan Mohamad, Wan Ahmad Zainie]

Hi Vinod.

Thanks for the review.

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, August 31, 2020 5:20 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: kishon@ti.com; robh+dt@kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; eswara.kota@linux.intel.com;
> vadivel.muruganx.ramuthevar@linux.intel.com; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support
> 
> On 21-08-20, 19:37, Wan Ahmad Zainie wrote:
> 
> > +/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */
> > +#define FREQSEL_200M_170M	0x0
> > +#define FREQSEL_170M_140M	0x1
> > +#define FREQSEL_140M_110M	0x2
> > +#define FREQSEL_110M_80M	0x3
> > +#define FREQSEL_80M_50M		0x4
> > +
> > +#define maskval(mask, val)	(((val) << (ffs(mask) - 1)) & mask)
> 
> Kernel has a macro do this for you, please use FIELD_PREP instead of

I have updated to v8, to remove this macro and use FIELD_PREP.
I also add changes based on Andy's comments.

> 
> your own macro
> --
> ~Vinod

Best regards,
Zainie

RE: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings

[Wan Mohamad, Wan Ahmad Zainie]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, August 31, 2020 5:10 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: kishon@ti.com; robh+dt@kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; eswara.kota@linux.intel.com;
> vadivel.muruganx.ramuthevar@linux.intel.com; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC
> PHY bindings
> 
> On 21-08-20, 19:37, Wan Ahmad Zainie wrote:
> > Binding description for Intel Keem Bay eMMC PHY.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../bindings/phy/intel,keembay-emmc-phy.yaml  | 44
> > +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yaml
> > b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yaml
> > new file mode 100644
> > index 000000000000..4cbbd3887c13
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yam
> > +++ l
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-
> phy.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Intel Keem Bay eMMC PHY bindings
> 
> This seems same as
> Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why
> not add a new compatible in lgm binding, or did I miss a difference?

AFAIK, LGM make use of syscon node, whilst KMB does not.
And LGM and KMB belongs to different SoC family. So, I prefer them to
be in separate file.

Having said that, with few changes in wordings in title and description,
I think we can make it generic and can be used across few products.

> 
> > +
> > +maintainers:
> > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: intel,keembay-emmc-phy
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: emmcclk
> > +
> > +  "#phy-cells":
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#phy-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    phy@20290000 {
> > +          compatible = "intel,keembay-emmc-phy";
> > +          reg = <0x20290000 0x54>;
> > +          clocks = <&emmc>;
> > +          clock-names = "emmcclk";
> > +          #phy-cells = <0>;
> > +    };
> > --
> > 2.17.1
> 
> --
> ~Vinod

Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings

[Vinod Koul]

On 01-09-20, 04:58, Wan Mohamad, Wan Ahmad Zainie wrote:

> > > @@ -0,0 +1,44 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-
> > phy.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay eMMC PHY bindings
> > 
> > This seems same as
> > Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why
> > not add a new compatible in lgm binding, or did I miss a difference?
> 
> AFAIK, LGM make use of syscon node, whilst KMB does not.
> And LGM and KMB belongs to different SoC family. So, I prefer them to
> be in separate file.
> 
> Having said that, with few changes in wordings in title and description,
> I think we can make it generic and can be used across few products.

The bindings seems quite similar. We can have two drivers loaded using
two compatible but binding description can be made same

-- 
~Vinod

RE: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings

[Wan Mohamad, Wan Ahmad Zainie]

Resend the reply.

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Tuesday, September 1, 2020 1:52 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: kishon@ti.com; robh+dt@kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; eswara.kota@linux.intel.com;
> vadivel.muruganx.ramuthevar@linux.intel.com; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC
> PHY bindings
> 
> On 01-09-20, 04:58, Wan Mohamad, Wan Ahmad Zainie wrote:
> 
> > > > @@ -0,0 +1,44 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > > +---
> > > > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-
> > > phy.yaml#"
> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > +
> > > > +title: Intel Keem Bay eMMC PHY bindings
> > >
> > > This seems same as
> > > Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml,
> why
> > > not add a new compatible in lgm binding, or did I miss a difference?
> >
> > AFAIK, LGM make use of syscon node, whilst KMB does not.
> > And LGM and KMB belongs to different SoC family. So, I prefer them to
> > be in separate file.
> >
> > Having said that, with few changes in wordings in title and
> > description, I think we can make it generic and can be used across few
> products.
> 
> The bindings seems quite similar. We can have two drivers loaded using two
> compatible but binding description can be made same

Noted. I can make the change i.e. add Keem Bay compatible string in lgm
binding document and drop Keem Bay binding document.

Rob and Vadivel, is there any objection? If not, I will proceed with v9 in the
next one or two days.

> 
> --
> ~Vinod