linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/2] phy: intel: Add Keem Bay eMMC PHY support
@ 2020-07-02  0:09 Wan Ahmad Zainie
  2020-07-02  0:09 ` [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings Wan Ahmad Zainie
  2020-07-02  0:09 ` [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
  0 siblings, 2 replies; 6+ messages in thread
From: Wan Ahmad Zainie @ 2020-07-02  0:09 UTC (permalink / raw)
  To: kishon, vkoul, robh+dt, andriy.shevchenko
  Cc: linux-kernel, devicetree, wan.ahmad.zainie.wan.mohamad,
	sureshkumar.mp, lakshmi.bai.raja.subramanian

Hi.

The first part is to document DT bindings for Keem Bay eMMC PHY.

The second 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 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 (2):
  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                     |  12 +
 drivers/phy/intel/Makefile                    |   1 +
 drivers/phy/intel/phy-keembay-emmc.c          | 314 ++++++++++++++++++
 4 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
 create mode 100644 drivers/phy/intel/phy-keembay-emmc.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
  2020-07-02  0:09 [PATCH v6 0/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
@ 2020-07-02  0:09 ` Wan Ahmad Zainie
  2020-07-09 20:50   ` Rob Herring
  2020-07-02  0:09 ` [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
  1 sibling, 1 reply; 6+ messages in thread
From: Wan Ahmad Zainie @ 2020-07-02  0:09 UTC (permalink / raw)
  To: kishon, vkoul, robh+dt, andriy.shevchenko
  Cc: linux-kernel, devicetree, wan.ahmad.zainie.wan.mohamad,
	sureshkumar.mp, lakshmi.bai.raja.subramanian

Binding description for Intel Keem Bay eMMC PHY.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 .../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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
  2020-07-02  0:09 [PATCH v6 0/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
  2020-07-02  0:09 ` [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings Wan Ahmad Zainie
@ 2020-07-02  0:09 ` Wan Ahmad Zainie
  2020-07-13  5:40   ` Vinod Koul
  1 sibling, 1 reply; 6+ messages in thread
From: Wan Ahmad Zainie @ 2020-07-02  0:09 UTC (permalink / raw)
  To: kishon, vkoul, robh+dt, andriy.shevchenko
  Cc: linux-kernel, devicetree, wan.ahmad.zainie.wan.mohamad,
	sureshkumar.mp, lakshmi.bai.raja.subramanian

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-keembay-emmc.c | 314 +++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/phy/intel/phy-keembay-emmc.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 7b47682a4e0e..8ddda4fb95d2 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
 	select GENERIC_PHY
 	help
 	  Enable this to support the Intel EMMC PHY
+
+config PHY_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.
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 233d530dadde..6566334e7b77 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_COMBO)		+= phy-intel-combo.o
 obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
+obj-$(CONFIG_PHY_KEEMBAY_EMMC)		+= phy-keembay-emmc.o
diff --git a/drivers/phy/intel/phy-keembay-emmc.c b/drivers/phy/intel/phy-keembay-emmc.c
new file mode 100644
index 000000000000..66b5931a749a
--- /dev/null
+++ b/drivers/phy/intel/phy-keembay-emmc.c
@@ -0,0 +1,314 @@
+// 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  SEL_DLY_TXCLK(x)	(((x) << 29) & SEL_DLY_TXCLK_MASK)
+#define  OTAP_DLY_ENA_MASK	BIT(27)
+#define  OTAP_DLY_ENA(x)	(((x) << 27) & OTAP_DLY_ENA_MASK)
+#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
+#define  OTAP_DLY_SEL(x)	(((x) << 23) & OTAP_DLY_SEL_MASK)
+#define  DLL_EN_MASK		BIT(10)
+#define  DLL_EN(x)		(((x) << 10) & DLL_EN_MASK)
+#define  PWR_DOWN_MASK		BIT(0)
+#define  PWR_DOWN(x)		(((x) << 0) & PWR_DOWN_MASK)
+
+#define PHY_CFG_2		0x2c
+#define  SEL_FREQ_MASK		GENMASK(12, 10)
+#define  SEL_FREQ(x)		(((x) << 10) & SEL_FREQ_MASK)
+
+#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
+
+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,
+				 PWR_DOWN(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,
+				 DLL_EN(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,
+				 PWR_DOWN(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,
+				 SEL_FREQ(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,
+				 DLL_EN(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,
+				 SEL_DLY_TXCLK(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,
+				 OTAP_DLY_ENA(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,
+				 OTAP_DLY_SEL(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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
  2020-07-02  0:09 ` [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings Wan Ahmad Zainie
@ 2020-07-09 20:50   ` Rob Herring
  0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2020-07-09 20:50 UTC (permalink / raw)
  To: Wan Ahmad Zainie
  Cc: andriy.shevchenko, sureshkumar.mp, lakshmi.bai.raja.subramanian,
	devicetree, vkoul, kishon, linux-kernel, robh+dt

On Thu, 02 Jul 2020 08:09:33 +0800, 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>
> ---
>  .../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
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
  2020-07-02  0:09 ` [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
@ 2020-07-13  5:40   ` Vinod Koul
  2020-07-16  7:58     ` Wan Mohamad, Wan Ahmad Zainie
  0 siblings, 1 reply; 6+ messages in thread
From: Vinod Koul @ 2020-07-13  5:40 UTC (permalink / raw)
  To: Wan Ahmad Zainie
  Cc: kishon, robh+dt, andriy.shevchenko, linux-kernel, devicetree,
	sureshkumar.mp, lakshmi.bai.raja.subramanian

On 02-07-20, 08:09, Wan Ahmad Zainie wrote:
> 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-keembay-emmc.c | 314 +++++++++++++++++++++++++++
>  3 files changed, 327 insertions(+)
>  create mode 100644 drivers/phy/intel/phy-keembay-emmc.c
> 
> diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> index 7b47682a4e0e..8ddda4fb95d2 100644
> --- a/drivers/phy/intel/Kconfig
> +++ b/drivers/phy/intel/Kconfig
> @@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
>  	select GENERIC_PHY
>  	help
>  	  Enable this to support the Intel EMMC PHY
> +
> +config PHY_KEEMBAY_EMMC

Pls keep this in alphabetical sort

> +	tristate "Intel Keem Bay EMMC PHY driver"
> +	depends on ARM64 || COMPILE_TEST

Intel and ARM64, aha, fun times!

> +	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.

phy-keembay-emmc.ko ?

> diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> index 233d530dadde..6566334e7b77 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_COMBO)		+= phy-intel-combo.o
>  obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
> +obj-$(CONFIG_PHY_KEEMBAY_EMMC)		+= phy-keembay-emmc.o

here as well

> +/* eMMC/SD/SDIO core/phy configuration registers */
> +#define PHY_CFG_0		0x24
> +#define  SEL_DLY_TXCLK_MASK	BIT(29)
> +#define  SEL_DLY_TXCLK(x)	(((x) << 29) & SEL_DLY_TXCLK_MASK)
> +#define  OTAP_DLY_ENA_MASK	BIT(27)
> +#define  OTAP_DLY_ENA(x)	(((x) << 27) & OTAP_DLY_ENA_MASK)
> +#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
> +#define  OTAP_DLY_SEL(x)	(((x) << 23) & OTAP_DLY_SEL_MASK)

why not a generic helper to do (x) << ffs(reg - 1) & reg ?
You can skip defining for each register that way!

-- 
~Vinod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
  2020-07-13  5:40   ` Vinod Koul
@ 2020-07-16  7:58     ` Wan Mohamad, Wan Ahmad Zainie
  0 siblings, 0 replies; 6+ messages in thread
From: Wan Mohamad, Wan Ahmad Zainie @ 2020-07-16  7:58 UTC (permalink / raw)
  To: Vinod Koul
  Cc: kishon, robh+dt, Shevchenko, Andriy, linux-kernel, devicetree,
	MP, Sureshkumar, Raja Subramanian, Lakshmi Bai

Hi Vinod.

Thanks for the review.

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, July 13, 2020 1:40 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>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; MP, Sureshkumar
> <sureshkumar.mp@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
> 
> On 02-07-20, 08:09, Wan Ahmad Zainie wrote:
> > 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-keembay-emmc.c | 314
> > +++++++++++++++++++++++++++
> >  3 files changed, 327 insertions(+)
> >  create mode 100644 drivers/phy/intel/phy-keembay-emmc.c
> >
> > diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> > index 7b47682a4e0e..8ddda4fb95d2 100644
> > --- a/drivers/phy/intel/Kconfig
> > +++ b/drivers/phy/intel/Kconfig
> > @@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
> >  	select GENERIC_PHY
> >  	help
> >  	  Enable this to support the Intel EMMC PHY
> > +
> > +config PHY_KEEMBAY_EMMC
> 
> Pls keep this in alphabetical sort

PHY_INTEL_ followed by PHY_KEEMBAY_ is alphabetically sorted.
Could you please help to clarify?

> 
> > +	tristate "Intel Keem Bay EMMC PHY driver"
> > +	depends on ARM64 || COMPILE_TEST
> 
> Intel and ARM64, aha, fun times!

😊

> 
> > +	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.
> 
> phy-keembay-emmc.ko ?

Is it a must? I saw few Kconfig files omit .ko.
I can fix this in next version.

> 
> > diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> > index 233d530dadde..6566334e7b77 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_COMBO)		+= phy-intel-combo.o
> >  obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
> > +obj-$(CONFIG_PHY_KEEMBAY_EMMC)		+= phy-keembay-
> emmc.o
> 
> here as well
> 
> > +/* eMMC/SD/SDIO core/phy configuration registers */
> > +#define PHY_CFG_0		0x24
> > +#define  SEL_DLY_TXCLK_MASK	BIT(29)
> > +#define  SEL_DLY_TXCLK(x)	(((x) << 29) & SEL_DLY_TXCLK_MASK)
> > +#define  OTAP_DLY_ENA_MASK	BIT(27)
> > +#define  OTAP_DLY_ENA(x)	(((x) << 27) & OTAP_DLY_ENA_MASK)
> > +#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
> > +#define  OTAP_DLY_SEL(x)	(((x) << 23) & OTAP_DLY_SEL_MASK)
> 
> why not a generic helper to do (x) << ffs(reg - 1) & reg ?
> You can skip defining for each register that way!

Is it something like this following?
#define maskval(mask, val)     (((val) << (ffs(mask) - 1)) & mask)

> 
> --
> ~Vinod

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-16  7:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02  0:09 [PATCH v6 0/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
2020-07-02  0:09 ` [PATCH v6 1/2] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings Wan Ahmad Zainie
2020-07-09 20:50   ` Rob Herring
2020-07-02  0:09 ` [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support Wan Ahmad Zainie
2020-07-13  5:40   ` Vinod Koul
2020-07-16  7:58     ` Wan Mohamad, Wan Ahmad Zainie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).