[PATCH 0/3] Add driver Support for MMCSD on AM654x.

[PATCH 0/3] Add driver Support for MMCSD on AM654x.

[Faiz Abbas]

The following patches add driver support for MMCSD on TI's AM654
platforms.

Previously I had added the support to sdhci-of-arasan driver with
a separate phy driver[1]. Since then it has turned out that tuning
operation (for HS200, HS400 and SDR104 speed modes) will require
configuration of phy registers. This completely breaks the model of
the sdhci-of-arasan driver which relies on a separate driver to
configure the phy register space.

Because of this, I am creating a new driver with both the sdhci and
phy register spaces. This helps me use the phy registers in a future
patch that adds tuning support.

DT patches will be posted in a separate series.

[1] driver and phy patches posted before
    https://patchwork.kernel.org/project/linux-mmc/list/?series=26623

Faiz Abbas (3):
  dt-bindings: mmc: sdhci-am654: Document bindings for the host
    controllers on TI's AM654 SOCs
  dt-bindings: mmc: sdhci-of-arasan: Add deprecated message for am654
  mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

 .../devicetree/bindings/mmc/arasan,sdhci.txt  |   4 +
 .../devicetree/bindings/mmc/sdhci-am654.txt   |  23 ++
 drivers/mmc/host/Kconfig                      |  12 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/sdhci-of-arasan.c            |  46 ---
 drivers/mmc/host/sdhci_am654.c                | 366 ++++++++++++++++++
 6 files changed, 406 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-am654.txt
 create mode 100644 drivers/mmc/host/sdhci_am654.c

-- 
2.19.2

[PATCH 1/3] dt-bindings: mmc: sdhci-am654: Document bindings for the host controllers on TI's AM654 SOCs

[Faiz Abbas]

Add binding documentation for mmc host controllers present on TI's AM654
SOCs

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 .../devicetree/bindings/mmc/sdhci-am654.txt   | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-am654.txt

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-am654.txt b/Documentation/devicetree/bindings/mmc/sdhci-am654.txt
new file mode 100644
index 000000000000..862b93051bfe
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-am654.txt
@@ -0,0 +1,37 @@
+Device Tree Bindings for the SDHCI Controllers present on TI's AM654 SOCs
+
+The bindings follow the mmc[1], clock[2] and interrupt[3] bindings.
+Only deviations are documented here.
+
+  [1] Documentation/devicetree/bindings/mmc/mmc.txt
+  [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
+  [3] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+Required Properties:
+	- compatible: should be "ti,am654-sdhci-5.1"
+	- reg: Must be two entries.
+		- The first should be the sdhci register space
+		- The second should the subsystem/phy register space
+	- clocks: Handles to the clock inputs.
+	- clock-names: Tuple including "clk_xin" and "clk_ahb"
+	- interrupts: Interrupt specifiers
+	- ti,otap-del-sel: Output Tap Delay select
+	- ti,trm-icp: DLL trim select
+	- ti,driver-strength-ohm: driver strength in ohms.
+				  Valid values are 33, 40, 50, 66 and 100 ohms.
+
+Example:
+
+	sdhci0: sdhci@4f80000 {
+		compatible = "ti,am654-sdhci-5.1";
+		reg = <0x0 0x4f80000 0x0 0x260>, <0x0 0x4f90000 0x0 0x134>;
+		power-domains = <&k3_pds 47>;
+		clocks = <&k3_clks 47 0>, <&k3_clks 47 1>;
+		clock-names = "clk_ahb", "clk_xin";
+		interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>;
+		sdhci-caps-mask = <0x80000007 0x0>;
+		mmc-ddr-1_8v;
+		ti,otap-del-sel = <0x2>;
+		ti,trm-icp = <0x8>;
+		status = "disabled";
+	};
-- 
2.19.2

[PATCH 2/3] dt-bindings: mmc: sdhci-of-arasan: Add deprecated message for am654

[Faiz Abbas]

The "ti,am654-sdhci-5.1" binding has been moved to a new driver. Indicate
this by a deprecated message.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index e2effe17f05e..1edbb049cccb 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -16,6 +16,10 @@ Required Properties:
     - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC PHY
       For this device it is strongly suggested to include arasan,soc-ctl-syscon.
     - "ti,am654-sdhci-5.1", "arasan,sdhci-5.1": TI AM654 MMC PHY
+	Note: This binding has been deprecated and moved to [5].
+
+  [5] Documentation/devicetree/bindings/mmc/sdhci-am654.txt
+
   - reg: From mmc bindings: Register location and length.
   - clocks: From clock bindings: Handles to clock inputs.
   - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
-- 
2.19.2

[PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Faiz Abbas]

The host controllers on TI's AM654 SOCs are not compatible with
the phy and consumer model of the sdhci-of-arasan driver. It turns out
that for optimal operation at higher speeds, a special tuning procedure
needs to be implemented which involves configuration of platform
specific phy registers.

Therefore, branch out to a new sdhci_am654 driver and add the phy
register space with all phy configurations to it. Populate AM654
specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
applicable.

Only add support for upto High Speed for SD card and upto DDR52 speed
mode for eMMC. Higher speeds will be added in subsequent patches.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/Kconfig           |  12 +
 drivers/mmc/host/Makefile          |   1 +
 drivers/mmc/host/sdhci-of-arasan.c |  46 ----
 drivers/mmc/host/sdhci_am654.c     | 376 +++++++++++++++++++++++++++++
 4 files changed, 389 insertions(+), 46 deletions(-)
 create mode 100644 drivers/mmc/host/sdhci_am654.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1b58739d9744..cfb2eb1a2c32 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -977,3 +977,15 @@ config MMC_SDHCI_OMAP
 	  If you have a controller with this interface, say Y or M here.
 
 	  If unsure, say N.
+
+config MMC_SDHCI_AM654
+	tristate "Support for the SDHCI Controller in TI's AM654 SOCs"
+	depends on MMC_SDHCI_PLTFM && OF
+	help
+	  This selects the Secure Digital Host Controller Interface (SDHCI)
+	  support present in TI's AM654 SOCs. The controller supports
+	  SD/MMC/SDIO devices.
+
+	  If you have a controller with this interface, say Y or M here.
+
+	  If unsure, say N.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 720d37777098..5c7770edc431 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
 obj-$(CONFIG_MMC_SDHCI_SIRF)   	+= sdhci-sirf.o
 obj-$(CONFIG_MMC_SDHCI_F_SDH30)	+= sdhci_f_sdh30.o
 obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
+obj-$(CONFIG_MMC_SDHCI_AM654)	+= sdhci_am654.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
 obj-$(CONFIG_MMC_MTK)		+= mtk-sd.o
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 142c4b802f31..c9e3e050ccc8 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -231,25 +231,6 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 }
 
-static void sdhci_arasan_am654_set_clock(struct sdhci_host *host,
-					 unsigned int clock)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
-
-	if (sdhci_arasan->is_phy_on) {
-		phy_power_off(sdhci_arasan->phy);
-		sdhci_arasan->is_phy_on = false;
-	}
-
-	sdhci_set_clock(host, clock);
-
-	if (clock > PHY_CLK_TOO_SLOW_HZ) {
-		phy_power_on(sdhci_arasan->phy);
-		sdhci_arasan->is_phy_on = true;
-	}
-}
-
 static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
 					struct mmc_ios *ios)
 {
@@ -335,29 +316,6 @@ static struct sdhci_arasan_of_data sdhci_arasan_data = {
 	.pdata = &sdhci_arasan_pdata,
 };
 
-static const struct sdhci_ops sdhci_arasan_am654_ops = {
-	.set_clock = sdhci_arasan_am654_set_clock,
-	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
-	.set_bus_width = sdhci_set_bus_width,
-	.reset = sdhci_arasan_reset,
-	.set_uhs_signaling = sdhci_set_uhs_signaling,
-};
-
-static const struct sdhci_pltfm_data sdhci_arasan_am654_pdata = {
-	.ops = &sdhci_arasan_am654_ops,
-	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN  |
-		  SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
-		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
-		   SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400,
-};
-
-static const struct sdhci_arasan_of_data sdhci_arasan_am654_data = {
-	.pdata = &sdhci_arasan_am654_pdata,
-};
-
 static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32 intmask)
 {
 	int cmd_error = 0;
@@ -520,10 +478,6 @@ static const struct of_device_id sdhci_arasan_of_match[] = {
 		.compatible = "rockchip,rk3399-sdhci-5.1",
 		.data = &sdhci_arasan_rk3399_data,
 	},
-	{
-		.compatible = "ti,am654-sdhci-5.1",
-		.data = &sdhci_arasan_am654_data,
-	},
 	/* Generic compatible below here */
 	{
 		.compatible = "arasan,sdhci-8.9a",
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
new file mode 100644
index 000000000000..6e5be12d5946
--- /dev/null
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sdhci_am654.c - SDHCI driver for TI's AM654 SOCs
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
+ *
+ */
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#include "sdhci-pltfm.h"
+
+/* CTL_CFG Registers */
+#define CTL_CFG_2		0x14
+
+#define SLOTTYPE_MASK		GENMASK(31, 30)
+#define SLOTTYPE_EMBEDDED	BIT(30)
+
+/* PHY Registers */
+#define PHY_CTRL1	0x100
+#define PHY_CTRL2	0x104
+#define PHY_CTRL3	0x108
+#define PHY_CTRL4	0x10C
+#define PHY_CTRL5	0x110
+#define PHY_CTRL6	0x114
+#define PHY_STAT1	0x130
+#define PHY_STAT2	0x134
+
+#define IOMUX_ENABLE_SHIFT	31
+#define IOMUX_ENABLE_MASK	BIT(IOMUX_ENABLE_SHIFT)
+#define OTAPDLYENA_SHIFT	20
+#define OTAPDLYENA_MASK		BIT(OTAPDLYENA_SHIFT)
+#define OTAPDLYSEL_SHIFT	12
+#define OTAPDLYSEL_MASK		GENMASK(15, 12)
+#define STRBSEL_SHIFT		24
+#define STRBSEL_MASK		GENMASK(27, 24)
+#define SEL50_SHIFT		8
+#define SEL50_MASK		BIT(SEL50_SHIFT)
+#define SEL100_SHIFT		9
+#define SEL100_MASK		BIT(SEL100_SHIFT)
+#define DLL_TRIM_ICP_SHIFT	4
+#define DLL_TRIM_ICP_MASK	GENMASK(7, 4)
+#define DR_TY_SHIFT		20
+#define DR_TY_MASK		GENMASK(22, 20)
+#define ENDLL_SHIFT		1
+#define ENDLL_MASK		BIT(ENDLL_SHIFT)
+#define DLLRDY_SHIFT		0
+#define DLLRDY_MASK		BIT(DLLRDY_SHIFT)
+#define PDB_SHIFT		0
+#define PDB_MASK		BIT(PDB_SHIFT)
+#define CALDONE_SHIFT		1
+#define CALDONE_MASK		BIT(CALDONE_SHIFT)
+#define RETRIM_SHIFT		17
+#define RETRIM_MASK		BIT(RETRIM_SHIFT)
+
+#define DRIVER_STRENGTH_50_OHM	0x0
+#define DRIVER_STRENGTH_33_OHM	0x1
+#define DRIVER_STRENGTH_66_OHM	0x2
+#define DRIVER_STRENGTH_100_OHM	0x3
+#define DRIVER_STRENGTH_40_OHM	0x4
+
+#define CLOCK_TOO_SLOW_HZ	400000
+
+static struct regmap_config sdhci_am654_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+struct sdhci_am654_data {
+	struct regmap *base;
+	struct clk *clk_ahb;
+	int otap_del_sel;
+	int trm_icp;
+	int drv_strength;
+	bool dll_on;
+};
+
+static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+	int sel50, sel100;
+	u32 mask, val;
+	int ret;
+
+	if (sdhci_am654->dll_on) {
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
+				   ENDLL_MASK, 0);
+
+		sdhci_am654->dll_on = false;
+	}
+
+	sdhci_set_clock(host, clock);
+
+	if (clock > CLOCK_TOO_SLOW_HZ) {
+		/* Setup DLL Output TAP delay */
+		mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
+		val = (1 << OTAPDLYENA_SHIFT) |
+		      (sdhci_am654->otap_del_sel << OTAPDLYSEL_SHIFT);
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
+				   mask, val);
+		switch (clock) {
+		case 200000000:
+			sel50 = 0;
+			sel100 = 0;
+			break;
+		case 100000000:
+			sel50 = 0;
+			sel100 = 1;
+			break;
+		default:
+			sel50 = 1;
+			sel100 = 0;
+		}
+
+		/* Configure PHY DLL frequency */
+		mask = SEL50_MASK | SEL100_MASK;
+		val = (sel50 << SEL50_SHIFT) | (sel100 << SEL100_SHIFT);
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL5,
+				   mask, val);
+		/* Configure DLL TRIM */
+		mask = DLL_TRIM_ICP_MASK;
+		val = sdhci_am654->trm_icp << DLL_TRIM_ICP_SHIFT;
+
+		/* Configure DLL driver strength */
+		mask |= DR_TY_MASK;
+		val |= sdhci_am654->drv_strength << DR_TY_SHIFT;
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
+				   mask, val);
+		/* Enable DLL */
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
+				   ENDLL_MASK, 0x1 << ENDLL_SHIFT);
+		/*
+		 * Poll for DLL ready. Use a one second timeout.
+		 * Works in all experiments done so far
+		 */
+		ret = regmap_read_poll_timeout(sdhci_am654->base,
+					 PHY_STAT1, val,
+					 val & DLLRDY_MASK,
+					 1000, 1000000);
+
+		sdhci_am654->dll_on = true;
+	}
+}
+
+
+static void sdhci_am654_set_power(struct sdhci_host *host, unsigned char mode,
+				  unsigned short vdd)
+{
+	if (!IS_ERR(host->mmc->supply.vmmc)) {
+		struct mmc_host *mmc = host->mmc;
+
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+	}
+	sdhci_set_power_noreg(host, mode, vdd);
+}
+
+struct sdhci_ops sdhci_am654_ops = {
+	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
+	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.set_bus_width = sdhci_set_bus_width,
+	.set_power = sdhci_am654_set_power,
+	.set_clock = sdhci_am654_set_clock,
+	.reset = sdhci_reset,
+};
+
+static const struct sdhci_pltfm_data sdhci_am654_pdata = {
+	.ops = &sdhci_am654_ops,
+	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
+		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+};
+
+static int sdhci_am654_init(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+	u32 ctl_cfg_2 = 0;
+	u32 val;
+	int ret;
+
+	regmap_read(sdhci_am654->base, PHY_STAT1, &val);
+	if (~val & CALDONE_MASK) {
+		/* Calibrate IO lines */
+		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
+				   PDB_MASK, PDB_MASK);
+		ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
+					       val, val & CALDONE_MASK, 1, 20);
+		if (ret)
+			return ret;
+	}
+
+	/* Enable pins by setting IO mux to 0 */
+	regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
+			   IOMUX_ENABLE_MASK, 0);
+
+	/* Set slot type based on SD or eMMC */
+	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+		ctl_cfg_2 = SLOTTYPE_EMBEDDED;
+
+	regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
+			   ctl_cfg_2, SLOTTYPE_MASK);
+
+	return sdhci_add_host(host);
+}
+
+static int sdhci_am654_get_of_property(struct platform_device *pdev,
+					struct sdhci_am654_data *sdhci_am654)
+{
+	struct device *dev = &pdev->dev;
+	int drv_strength;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ti,trm-icp",
+				       &sdhci_am654->trm_icp);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev, "ti,otap-del-sel",
+				       &sdhci_am654->otap_del_sel);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev, "ti,driver-strength-ohm",
+				       &drv_strength);
+	if (ret)
+		return ret;
+
+	switch (drv_strength) {
+	case 50:
+		sdhci_am654->drv_strength = DRIVER_STRENGTH_50_OHM;
+		break;
+	case 33:
+		sdhci_am654->drv_strength = DRIVER_STRENGTH_33_OHM;
+		break;
+	case 66:
+		sdhci_am654->drv_strength = DRIVER_STRENGTH_66_OHM;
+		break;
+	case 100:
+		sdhci_am654->drv_strength = DRIVER_STRENGTH_100_OHM;
+		break;
+	case 40:
+		sdhci_am654->drv_strength = DRIVER_STRENGTH_40_OHM;
+		break;
+	default:
+		dev_err(dev, "Invalid driver strength\n");
+		return -EINVAL;
+	}
+
+	sdhci_get_of_property(pdev);
+
+	return 0;
+}
+
+static int sdhci_am654_probe(struct platform_device *pdev)
+{
+	struct sdhci_pltfm_host *pltfm_host;
+	struct sdhci_am654_data *sdhci_am654;
+	struct sdhci_host *host;
+	struct resource *res;
+	struct clk *clk_xin;
+	struct device *dev = &pdev->dev;
+	void __iomem *base;
+	int ret;
+
+	host = sdhci_pltfm_init(pdev, &sdhci_am654_pdata, sizeof(*sdhci_am654));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	pltfm_host = sdhci_priv(host);
+	sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+
+	clk_xin = devm_clk_get(dev, "clk_xin");
+	if (IS_ERR(clk_xin)) {
+		dev_err(dev, "clk_xin clock not found.\n");
+		ret = PTR_ERR(clk_xin);
+		goto err_pltfm_free;
+	}
+
+	sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
+	if (IS_ERR(sdhci_am654->clk_ahb)) {
+		dev_err(dev, "clk_ahb clock not found.\n");
+		ret = PTR_ERR(sdhci_am654->clk_ahb);
+		goto err_pltfm_free;
+	}
+
+	pltfm_host->clk = clk_xin;
+
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret > 0) {
+		pm_runtime_put_noidle(dev);
+		goto pm_runtime_disable;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base)) {
+		ret = PTR_ERR(base);
+		goto pm_runtime_put;
+	}
+
+	sdhci_am654->base = devm_regmap_init_mmio(dev, base,
+						  &sdhci_am654_regmap_config);
+	if (IS_ERR(sdhci_am654->base)) {
+		dev_err(dev, "Failed to initialize regmap\n");
+		ret = PTR_ERR(sdhci_am654->base);
+		goto pm_runtime_put;
+	}
+
+	ret = sdhci_am654_get_of_property(pdev, sdhci_am654);
+	if (ret)
+		goto pm_runtime_put;
+
+	ret = mmc_of_parse(host->mmc);
+	if (ret) {
+		dev_err(dev, "parsing dt failed (%d)\n", ret);
+		goto pm_runtime_put;
+	}
+
+	ret = sdhci_am654_init(host);
+	if (ret)
+		goto pm_runtime_put;
+
+	return 0;
+
+pm_runtime_put:
+	pm_runtime_put_sync(dev);
+pm_runtime_disable:
+	pm_runtime_disable(dev);
+err_pltfm_free:
+	sdhci_pltfm_free(pdev);
+	return ret;
+}
+
+static int sdhci_am654_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	int ret;
+
+	sdhci_remove_host(host, true);
+	ret = pm_runtime_put_sync(&pdev->dev);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_disable(&pdev->dev);
+	sdhci_pltfm_free(pdev);
+
+	return 0;
+}
+
+static const struct of_device_id sdhci_am654_of_match[] = {
+	{ .compatible = "ti,am654-sdhci-5.1" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver sdhci_am654_driver = {
+	.driver = {
+		.name = "sdhci-am654",
+		.of_match_table = sdhci_am654_of_match,
+	},
+	.probe = sdhci_am654_probe,
+	.remove = sdhci_am654_remove,
+};
+
+module_platform_driver(sdhci_am654_driver);
+
+MODULE_DESCRIPTION("Driver for SDHCI Controller on TI's AM654 devices");
+MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
+MODULE_LICENSE("GPL");
-- 
2.19.2

Re: [PATCH 2/3] dt-bindings: mmc: sdhci-of-arasan: Add deprecated message for am654

[Michal Simek]

On 29. 11. 18 17:15, Faiz Abbas wrote:
> The "ti,am654-sdhci-5.1" binding has been moved to a new driver. Indicate
> this by a deprecated message.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index e2effe17f05e..1edbb049cccb 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -16,6 +16,10 @@ Required Properties:
>      - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC PHY
>        For this device it is strongly suggested to include arasan,soc-ctl-syscon.
>      - "ti,am654-sdhci-5.1", "arasan,sdhci-5.1": TI AM654 MMC PHY
> +	Note: This binding has been deprecated and moved to [5].
> +
> +  [5] Documentation/devicetree/bindings/mmc/sdhci-am654.txt
> +
>    - reg: From mmc bindings: Register location and length.
>    - clocks: From clock bindings: Handles to clock inputs.
>    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
> 

I would prefer if you can extend commit message by description you have
in cover letter which explains reasons. When this is applied and someone
look at git history none will understand from first read why you have
done that.

Thanks,
Michal

Re: [PATCH 2/3] dt-bindings: mmc: sdhci-of-arasan: Add deprecated message for am654

[Faiz Abbas]

Hi Michal,

On 30/11/18 12:48 PM, Michal Simek wrote:
> On 29. 11. 18 17:15, Faiz Abbas wrote:
>> The "ti,am654-sdhci-5.1" binding has been moved to a new driver. Indicate
>> this by a deprecated message.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index e2effe17f05e..1edbb049cccb 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -16,6 +16,10 @@ Required Properties:
>>      - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC PHY
>>        For this device it is strongly suggested to include arasan,soc-ctl-syscon.
>>      - "ti,am654-sdhci-5.1", "arasan,sdhci-5.1": TI AM654 MMC PHY
>> +	Note: This binding has been deprecated and moved to [5].
>> +
>> +  [5] Documentation/devicetree/bindings/mmc/sdhci-am654.txt
>> +
>>    - reg: From mmc bindings: Register location and length.
>>    - clocks: From clock bindings: Handles to clock inputs.
>>    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
>>
> 
> I would prefer if you can extend commit message by description you have
> in cover letter which explains reasons. When this is applied and someone
> look at git history none will understand from first read why you have
> done that.
> 

Ok. Will add full explanations to both documentation patches.

Thanks,
Faiz

Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Adrian Hunter]

On 29/11/18 6:15 PM, Faiz Abbas wrote:
> The host controllers on TI's AM654 SOCs are not compatible with
> the phy and consumer model of the sdhci-of-arasan driver. It turns out
> that for optimal operation at higher speeds, a special tuning procedure
> needs to be implemented which involves configuration of platform
> specific phy registers.
> 
> Therefore, branch out to a new sdhci_am654 driver and add the phy
> register space with all phy configurations to it. Populate AM654
> specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
> applicable.
> 
> Only add support for upto High Speed for SD card and upto DDR52 speed
> mode for eMMC. Higher speeds will be added in subsequent patches.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/Kconfig           |  12 +
>  drivers/mmc/host/Makefile          |   1 +
>  drivers/mmc/host/sdhci-of-arasan.c |  46 ----
>  drivers/mmc/host/sdhci_am654.c     | 376 +++++++++++++++++++++++++++++
>  4 files changed, 389 insertions(+), 46 deletions(-)
>  create mode 100644 drivers/mmc/host/sdhci_am654.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1b58739d9744..cfb2eb1a2c32 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -977,3 +977,15 @@ config MMC_SDHCI_OMAP
>  	  If you have a controller with this interface, say Y or M here.
>  
>  	  If unsure, say N.
> +
> +config MMC_SDHCI_AM654
> +	tristate "Support for the SDHCI Controller in TI's AM654 SOCs"
> +	depends on MMC_SDHCI_PLTFM && OF
> +	help
> +	  This selects the Secure Digital Host Controller Interface (SDHCI)
> +	  support present in TI's AM654 SOCs. The controller supports
> +	  SD/MMC/SDIO devices.
> +
> +	  If you have a controller with this interface, say Y or M here.
> +
> +	  If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 720d37777098..5c7770edc431 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
>  obj-$(CONFIG_MMC_SDHCI_SIRF)   	+= sdhci-sirf.o
>  obj-$(CONFIG_MMC_SDHCI_F_SDH30)	+= sdhci_f_sdh30.o
>  obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
> +obj-$(CONFIG_MMC_SDHCI_AM654)	+= sdhci_am654.o
>  obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
>  obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
>  obj-$(CONFIG_MMC_MTK)		+= mtk-sd.o
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 142c4b802f31..c9e3e050ccc8 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -231,25 +231,6 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>  	}
>  }
>  
> -static void sdhci_arasan_am654_set_clock(struct sdhci_host *host,
> -					 unsigned int clock)
> -{
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> -
> -	if (sdhci_arasan->is_phy_on) {
> -		phy_power_off(sdhci_arasan->phy);
> -		sdhci_arasan->is_phy_on = false;
> -	}
> -
> -	sdhci_set_clock(host, clock);
> -
> -	if (clock > PHY_CLK_TOO_SLOW_HZ) {
> -		phy_power_on(sdhci_arasan->phy);
> -		sdhci_arasan->is_phy_on = true;
> -	}
> -}
> -
>  static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
>  					struct mmc_ios *ios)
>  {
> @@ -335,29 +316,6 @@ static struct sdhci_arasan_of_data sdhci_arasan_data = {
>  	.pdata = &sdhci_arasan_pdata,
>  };
>  
> -static const struct sdhci_ops sdhci_arasan_am654_ops = {
> -	.set_clock = sdhci_arasan_am654_set_clock,
> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> -	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> -	.set_bus_width = sdhci_set_bus_width,
> -	.reset = sdhci_arasan_reset,
> -	.set_uhs_signaling = sdhci_set_uhs_signaling,
> -};
> -
> -static const struct sdhci_pltfm_data sdhci_arasan_am654_pdata = {
> -	.ops = &sdhci_arasan_am654_ops,
> -	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN  |
> -		  SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
> -		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> -		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> -		   SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400,
> -};
> -
> -static const struct sdhci_arasan_of_data sdhci_arasan_am654_data = {
> -	.pdata = &sdhci_arasan_am654_pdata,
> -};
> -
>  static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32 intmask)
>  {
>  	int cmd_error = 0;
> @@ -520,10 +478,6 @@ static const struct of_device_id sdhci_arasan_of_match[] = {
>  		.compatible = "rockchip,rk3399-sdhci-5.1",
>  		.data = &sdhci_arasan_rk3399_data,
>  	},
> -	{
> -		.compatible = "ti,am654-sdhci-5.1",
> -		.data = &sdhci_arasan_am654_data,
> -	},
>  	/* Generic compatible below here */
>  	{
>  		.compatible = "arasan,sdhci-8.9a",
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> new file mode 100644
> index 000000000000..6e5be12d5946
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * sdhci_am654.c - SDHCI driver for TI's AM654 SOCs
> + *
> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
> + *
> + */
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +/* CTL_CFG Registers */
> +#define CTL_CFG_2		0x14
> +
> +#define SLOTTYPE_MASK		GENMASK(31, 30)
> +#define SLOTTYPE_EMBEDDED	BIT(30)
> +
> +/* PHY Registers */
> +#define PHY_CTRL1	0x100
> +#define PHY_CTRL2	0x104
> +#define PHY_CTRL3	0x108
> +#define PHY_CTRL4	0x10C
> +#define PHY_CTRL5	0x110
> +#define PHY_CTRL6	0x114
> +#define PHY_STAT1	0x130
> +#define PHY_STAT2	0x134
> +
> +#define IOMUX_ENABLE_SHIFT	31
> +#define IOMUX_ENABLE_MASK	BIT(IOMUX_ENABLE_SHIFT)
> +#define OTAPDLYENA_SHIFT	20
> +#define OTAPDLYENA_MASK		BIT(OTAPDLYENA_SHIFT)
> +#define OTAPDLYSEL_SHIFT	12
> +#define OTAPDLYSEL_MASK		GENMASK(15, 12)
> +#define STRBSEL_SHIFT		24
> +#define STRBSEL_MASK		GENMASK(27, 24)
> +#define SEL50_SHIFT		8
> +#define SEL50_MASK		BIT(SEL50_SHIFT)
> +#define SEL100_SHIFT		9
> +#define SEL100_MASK		BIT(SEL100_SHIFT)
> +#define DLL_TRIM_ICP_SHIFT	4
> +#define DLL_TRIM_ICP_MASK	GENMASK(7, 4)
> +#define DR_TY_SHIFT		20
> +#define DR_TY_MASK		GENMASK(22, 20)
> +#define ENDLL_SHIFT		1
> +#define ENDLL_MASK		BIT(ENDLL_SHIFT)
> +#define DLLRDY_SHIFT		0
> +#define DLLRDY_MASK		BIT(DLLRDY_SHIFT)
> +#define PDB_SHIFT		0
> +#define PDB_MASK		BIT(PDB_SHIFT)
> +#define CALDONE_SHIFT		1
> +#define CALDONE_MASK		BIT(CALDONE_SHIFT)
> +#define RETRIM_SHIFT		17
> +#define RETRIM_MASK		BIT(RETRIM_SHIFT)
> +
> +#define DRIVER_STRENGTH_50_OHM	0x0
> +#define DRIVER_STRENGTH_33_OHM	0x1
> +#define DRIVER_STRENGTH_66_OHM	0x2
> +#define DRIVER_STRENGTH_100_OHM	0x3
> +#define DRIVER_STRENGTH_40_OHM	0x4
> +
> +#define CLOCK_TOO_SLOW_HZ	400000
> +
> +static struct regmap_config sdhci_am654_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +struct sdhci_am654_data {
> +	struct regmap *base;
> +	struct clk *clk_ahb;
> +	int otap_del_sel;
> +	int trm_icp;
> +	int drv_strength;
> +	bool dll_on;
> +};
> +
> +static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +	int sel50, sel100;
> +	u32 mask, val;
> +	int ret;
> +
> +	if (sdhci_am654->dll_on) {
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +				   ENDLL_MASK, 0);
> +
> +		sdhci_am654->dll_on = false;
> +	}
> +
> +	sdhci_set_clock(host, clock);
> +
> +	if (clock > CLOCK_TOO_SLOW_HZ) {
> +		/* Setup DLL Output TAP delay */
> +		mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> +		val = (1 << OTAPDLYENA_SHIFT) |
> +		      (sdhci_am654->otap_del_sel << OTAPDLYSEL_SHIFT);
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
> +				   mask, val);
> +		switch (clock) {
> +		case 200000000:
> +			sel50 = 0;
> +			sel100 = 0;
> +			break;
> +		case 100000000:
> +			sel50 = 0;
> +			sel100 = 1;
> +			break;
> +		default:
> +			sel50 = 1;
> +			sel100 = 0;
> +		}
> +
> +		/* Configure PHY DLL frequency */
> +		mask = SEL50_MASK | SEL100_MASK;
> +		val = (sel50 << SEL50_SHIFT) | (sel100 << SEL100_SHIFT);
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL5,
> +				   mask, val);
> +		/* Configure DLL TRIM */
> +		mask = DLL_TRIM_ICP_MASK;
> +		val = sdhci_am654->trm_icp << DLL_TRIM_ICP_SHIFT;
> +
> +		/* Configure DLL driver strength */
> +		mask |= DR_TY_MASK;
> +		val |= sdhci_am654->drv_strength << DR_TY_SHIFT;
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +				   mask, val);
> +		/* Enable DLL */
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +				   ENDLL_MASK, 0x1 << ENDLL_SHIFT);
> +		/*
> +		 * Poll for DLL ready. Use a one second timeout.
> +		 * Works in all experiments done so far
> +		 */
> +		ret = regmap_read_poll_timeout(sdhci_am654->base,
> +					 PHY_STAT1, val,
> +					 val & DLLRDY_MASK,
> +					 1000, 1000000);
> +
> +		sdhci_am654->dll_on = true;
> +	}
> +}
> +
> +

Double blank line

> +static void sdhci_am654_set_power(struct sdhci_host *host, unsigned char mode,
> +				  unsigned short vdd)
> +{
> +	if (!IS_ERR(host->mmc->supply.vmmc)) {
> +		struct mmc_host *mmc = host->mmc;
> +
> +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> +	}
> +	sdhci_set_power_noreg(host, mode, vdd);
> +}
> +
> +struct sdhci_ops sdhci_am654_ops = {
> +	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
> +	.set_bus_width = sdhci_set_bus_width,
> +	.set_power = sdhci_am654_set_power,
> +	.set_clock = sdhci_am654_set_clock,
> +	.reset = sdhci_reset,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_am654_pdata = {
> +	.ops = &sdhci_am654_ops,
> +	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
> +		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> +};
> +
> +static int sdhci_am654_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +	u32 ctl_cfg_2 = 0;
> +	u32 val;
> +	int ret;
> +
> +	regmap_read(sdhci_am654->base, PHY_STAT1, &val);
> +	if (~val & CALDONE_MASK) {
> +		/* Calibrate IO lines */
> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +				   PDB_MASK, PDB_MASK);
> +		ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
> +					       val, val & CALDONE_MASK, 1, 20);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Enable pins by setting IO mux to 0 */
> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +			   IOMUX_ENABLE_MASK, 0);
> +
> +	/* Set slot type based on SD or eMMC */
> +	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
> +		ctl_cfg_2 = SLOTTYPE_EMBEDDED;
> +
> +	regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
> +			   ctl_cfg_2, SLOTTYPE_MASK);
> +
> +	return sdhci_add_host(host);
> +}
> +
> +static int sdhci_am654_get_of_property(struct platform_device *pdev,
> +					struct sdhci_am654_data *sdhci_am654)
> +{
> +	struct device *dev = &pdev->dev;
> +	int drv_strength;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "ti,trm-icp",
> +				       &sdhci_am654->trm_icp);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "ti,otap-del-sel",
> +				       &sdhci_am654->otap_del_sel);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "ti,driver-strength-ohm",
> +				       &drv_strength);
> +	if (ret)
> +		return ret;
> +
> +	switch (drv_strength) {
> +	case 50:
> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_50_OHM;
> +		break;
> +	case 33:
> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_33_OHM;
> +		break;
> +	case 66:
> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_66_OHM;
> +		break;
> +	case 100:
> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_100_OHM;
> +		break;
> +	case 40:
> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_40_OHM;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid driver strength\n");
> +		return -EINVAL;
> +	}
> +
> +	sdhci_get_of_property(pdev);
> +
> +	return 0;
> +}
> +
> +static int sdhci_am654_probe(struct platform_device *pdev)
> +{
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct sdhci_am654_data *sdhci_am654;
> +	struct sdhci_host *host;
> +	struct resource *res;
> +	struct clk *clk_xin;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	int ret;
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_am654_pdata, sizeof(*sdhci_am654));
> +	if (IS_ERR(host))
> +		return PTR_ERR(host);
> +
> +	pltfm_host = sdhci_priv(host);
> +	sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +
> +	clk_xin = devm_clk_get(dev, "clk_xin");
> +	if (IS_ERR(clk_xin)) {
> +		dev_err(dev, "clk_xin clock not found.\n");
> +		ret = PTR_ERR(clk_xin);
> +		goto err_pltfm_free;
> +	}
> +
> +	sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
> +	if (IS_ERR(sdhci_am654->clk_ahb)) {
> +		dev_err(dev, "clk_ahb clock not found.\n");
> +		ret = PTR_ERR(sdhci_am654->clk_ahb);
> +		goto err_pltfm_free;
> +	}

Did you intend not to enable clks?

> +
> +	pltfm_host->clk = clk_xin;
> +
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret > 0) {

Did you intend 'ret > 0'?

> +		pm_runtime_put_noidle(dev);
> +		goto pm_runtime_disable;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base)) {
> +		ret = PTR_ERR(base);
> +		goto pm_runtime_put;
> +	}
> +
> +	sdhci_am654->base = devm_regmap_init_mmio(dev, base,
> +						  &sdhci_am654_regmap_config);
> +	if (IS_ERR(sdhci_am654->base)) {
> +		dev_err(dev, "Failed to initialize regmap\n");
> +		ret = PTR_ERR(sdhci_am654->base);
> +		goto pm_runtime_put;
> +	}
> +
> +	ret = sdhci_am654_get_of_property(pdev, sdhci_am654);
> +	if (ret)
> +		goto pm_runtime_put;
> +
> +	ret = mmc_of_parse(host->mmc);
> +	if (ret) {
> +		dev_err(dev, "parsing dt failed (%d)\n", ret);
> +		goto pm_runtime_put;
> +	}
> +
> +	ret = sdhci_am654_init(host);
> +	if (ret)
> +		goto pm_runtime_put;
> +
> +	return 0;
> +
> +pm_runtime_put:
> +	pm_runtime_put_sync(dev);
> +pm_runtime_disable:
> +	pm_runtime_disable(dev);
> +err_pltfm_free:
> +	sdhci_pltfm_free(pdev);
> +	return ret;
> +}
> +
> +static int sdhci_am654_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	sdhci_remove_host(host, true);
> +	ret = pm_runtime_put_sync(&pdev->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	sdhci_pltfm_free(pdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sdhci_am654_of_match[] = {
> +	{ .compatible = "ti,am654-sdhci-5.1" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver sdhci_am654_driver = {
> +	.driver = {
> +		.name = "sdhci-am654",
> +		.of_match_table = sdhci_am654_of_match,
> +	},
> +	.probe = sdhci_am654_probe,
> +	.remove = sdhci_am654_remove,
> +};
> +
> +module_platform_driver(sdhci_am654_driver);
> +
> +MODULE_DESCRIPTION("Driver for SDHCI Controller on TI's AM654 devices");
> +MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Faiz Abbas]

Hi Adrian,

On 05/12/18 7:12 PM, Adrian Hunter wrote:
> On 29/11/18 6:15 PM, Faiz Abbas wrote:
>> The host controllers on TI's AM654 SOCs are not compatible with
>> the phy and consumer model of the sdhci-of-arasan driver. It turns out
>> that for optimal operation at higher speeds, a special tuning procedure
>> needs to be implemented which involves configuration of platform
>> specific phy registers.
>>
>> Therefore, branch out to a new sdhci_am654 driver and add the phy
>> register space with all phy configurations to it. Populate AM654
>> specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
>> applicable.
>>
>> Only add support for upto High Speed for SD card and upto DDR52 speed
>> mode for eMMC. Higher speeds will be added in subsequent patches.
>>
...
>> +		sdhci_am654->dll_on = true;
>> +	}
>> +}
>> +
>> +
> 
> Double blank line

Will fix.

> 
>> +static void sdhci_am654_set_power(struct sdhci_host *host, unsigned char mode,
>> +				  unsigned short vdd)
>> +{
>> +	if (!IS_ERR(host->mmc->supply.vmmc)) {
>> +		struct mmc_host *mmc = host->mmc;
>> +
>> +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>> +	}
>> +	sdhci_set_power_noreg(host, mode, vdd);
>> +}
>> +
>> +struct sdhci_ops sdhci_am654_ops = {
>> +	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>> +	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
>> +	.set_bus_width = sdhci_set_bus_width,
>> +	.set_power = sdhci_am654_set_power,
>> +	.set_clock = sdhci_am654_set_clock,
>> +	.reset = sdhci_reset,
>> +};
>> +
>> +static const struct sdhci_pltfm_data sdhci_am654_pdata = {
>> +	.ops = &sdhci_am654_ops,
>> +	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
>> +		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
>> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>> +};
>> +
>> +static int sdhci_am654_init(struct sdhci_host *host)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> +	u32 ctl_cfg_2 = 0;
>> +	u32 val;
>> +	int ret;
>> +
>> +	regmap_read(sdhci_am654->base, PHY_STAT1, &val);
>> +	if (~val & CALDONE_MASK) {
>> +		/* Calibrate IO lines */
>> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>> +				   PDB_MASK, PDB_MASK);
>> +		ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
>> +					       val, val & CALDONE_MASK, 1, 20);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/* Enable pins by setting IO mux to 0 */
>> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>> +			   IOMUX_ENABLE_MASK, 0);
>> +
>> +	/* Set slot type based on SD or eMMC */
>> +	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>> +		ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>> +
>> +	regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
>> +			   ctl_cfg_2, SLOTTYPE_MASK);
>> +
>> +	return sdhci_add_host(host);
>> +}
>> +
>> +static int sdhci_am654_get_of_property(struct platform_device *pdev,
>> +					struct sdhci_am654_data *sdhci_am654)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int drv_strength;
>> +	int ret;
>> +
>> +	ret = device_property_read_u32(dev, "ti,trm-icp",
>> +				       &sdhci_am654->trm_icp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = device_property_read_u32(dev, "ti,otap-del-sel",
>> +				       &sdhci_am654->otap_del_sel);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = device_property_read_u32(dev, "ti,driver-strength-ohm",
>> +				       &drv_strength);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (drv_strength) {
>> +	case 50:
>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_50_OHM;
>> +		break;
>> +	case 33:
>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_33_OHM;
>> +		break;
>> +	case 66:
>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_66_OHM;
>> +		break;
>> +	case 100:
>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_100_OHM;
>> +		break;
>> +	case 40:
>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_40_OHM;
>> +		break;
>> +	default:
>> +		dev_err(dev, "Invalid driver strength\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	sdhci_get_of_property(pdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sdhci_am654_probe(struct platform_device *pdev)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host;
>> +	struct sdhci_am654_data *sdhci_am654;
>> +	struct sdhci_host *host;
>> +	struct resource *res;
>> +	struct clk *clk_xin;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *base;
>> +	int ret;
>> +
>> +	host = sdhci_pltfm_init(pdev, &sdhci_am654_pdata, sizeof(*sdhci_am654));
>> +	if (IS_ERR(host))
>> +		return PTR_ERR(host);
>> +
>> +	pltfm_host = sdhci_priv(host);
>> +	sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	clk_xin = devm_clk_get(dev, "clk_xin");
>> +	if (IS_ERR(clk_xin)) {
>> +		dev_err(dev, "clk_xin clock not found.\n");
>> +		ret = PTR_ERR(clk_xin);
>> +		goto err_pltfm_free;
>> +	}
>> +
>> +	sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
>> +	if (IS_ERR(sdhci_am654->clk_ahb)) {
>> +		dev_err(dev, "clk_ahb clock not found.\n");
>> +		ret = PTR_ERR(sdhci_am654->clk_ahb);
>> +		goto err_pltfm_free;
>> +	}
> 
> Did you intend not to enable clks?

Yes. Clocks get enabled as a part of pm_runtime calls.
> 
>> +
>> +	pltfm_host->clk = clk_xin;
>> +
>> +	pm_runtime_enable(dev);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret > 0) {
> 
> Did you intend 'ret > 0'?

Sorry. That was intended to be < 0.

Thanks,
Faiz

Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Adrian Hunter]

On 5/12/18 5:07 PM, Faiz Abbas wrote:
> Hi Adrian,
> 
> On 05/12/18 7:12 PM, Adrian Hunter wrote:
>> On 29/11/18 6:15 PM, Faiz Abbas wrote:
>>> The host controllers on TI's AM654 SOCs are not compatible with
>>> the phy and consumer model of the sdhci-of-arasan driver. It turns out
>>> that for optimal operation at higher speeds, a special tuning procedure
>>> needs to be implemented which involves configuration of platform
>>> specific phy registers.
>>>
>>> Therefore, branch out to a new sdhci_am654 driver and add the phy
>>> register space with all phy configurations to it. Populate AM654
>>> specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
>>> applicable.
>>>
>>> Only add support for upto High Speed for SD card and upto DDR52 speed
>>> mode for eMMC. Higher speeds will be added in subsequent patches.
>>>
> ...
>>> +		sdhci_am654->dll_on = true;
>>> +	}
>>> +}
>>> +
>>> +
>>
>> Double blank line
> 
> Will fix.
> 
>>
>>> +static void sdhci_am654_set_power(struct sdhci_host *host, unsigned char mode,
>>> +				  unsigned short vdd)
>>> +{
>>> +	if (!IS_ERR(host->mmc->supply.vmmc)) {
>>> +		struct mmc_host *mmc = host->mmc;
>>> +
>>> +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>>> +	}
>>> +	sdhci_set_power_noreg(host, mode, vdd);
>>> +}
>>> +
>>> +struct sdhci_ops sdhci_am654_ops = {
>>> +	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>>> +	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
>>> +	.set_bus_width = sdhci_set_bus_width,
>>> +	.set_power = sdhci_am654_set_power,
>>> +	.set_clock = sdhci_am654_set_clock,
>>> +	.reset = sdhci_reset,
>>> +};
>>> +
>>> +static const struct sdhci_pltfm_data sdhci_am654_pdata = {
>>> +	.ops = &sdhci_am654_ops,
>>> +	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
>>> +		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
>>> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> +};
>>> +
>>> +static int sdhci_am654_init(struct sdhci_host *host)
>>> +{
>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>> +	u32 ctl_cfg_2 = 0;
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	regmap_read(sdhci_am654->base, PHY_STAT1, &val);
>>> +	if (~val & CALDONE_MASK) {
>>> +		/* Calibrate IO lines */
>>> +		regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>>> +				   PDB_MASK, PDB_MASK);
>>> +		ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
>>> +					       val, val & CALDONE_MASK, 1, 20);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	/* Enable pins by setting IO mux to 0 */
>>> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>>> +			   IOMUX_ENABLE_MASK, 0);
>>> +
>>> +	/* Set slot type based on SD or eMMC */
>>> +	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>>> +		ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>>> +
>>> +	regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
>>> +			   ctl_cfg_2, SLOTTYPE_MASK);
>>> +
>>> +	return sdhci_add_host(host);
>>> +}
>>> +
>>> +static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>> +					struct sdhci_am654_data *sdhci_am654)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	int drv_strength;
>>> +	int ret;
>>> +
>>> +	ret = device_property_read_u32(dev, "ti,trm-icp",
>>> +				       &sdhci_am654->trm_icp);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = device_property_read_u32(dev, "ti,otap-del-sel",
>>> +				       &sdhci_am654->otap_del_sel);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = device_property_read_u32(dev, "ti,driver-strength-ohm",
>>> +				       &drv_strength);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	switch (drv_strength) {
>>> +	case 50:
>>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_50_OHM;
>>> +		break;
>>> +	case 33:
>>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_33_OHM;
>>> +		break;
>>> +	case 66:
>>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_66_OHM;
>>> +		break;
>>> +	case 100:
>>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_100_OHM;
>>> +		break;
>>> +	case 40:
>>> +		sdhci_am654->drv_strength = DRIVER_STRENGTH_40_OHM;
>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "Invalid driver strength\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	sdhci_get_of_property(pdev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sdhci_am654_probe(struct platform_device *pdev)
>>> +{
>>> +	struct sdhci_pltfm_host *pltfm_host;
>>> +	struct sdhci_am654_data *sdhci_am654;
>>> +	struct sdhci_host *host;
>>> +	struct resource *res;
>>> +	struct clk *clk_xin;
>>> +	struct device *dev = &pdev->dev;
>>> +	void __iomem *base;
>>> +	int ret;
>>> +
>>> +	host = sdhci_pltfm_init(pdev, &sdhci_am654_pdata, sizeof(*sdhci_am654));
>>> +	if (IS_ERR(host))
>>> +		return PTR_ERR(host);
>>> +
>>> +	pltfm_host = sdhci_priv(host);
>>> +	sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> +	clk_xin = devm_clk_get(dev, "clk_xin");
>>> +	if (IS_ERR(clk_xin)) {
>>> +		dev_err(dev, "clk_xin clock not found.\n");
>>> +		ret = PTR_ERR(clk_xin);
>>> +		goto err_pltfm_free;
>>> +	}
>>> +
>>> +	sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
>>> +	if (IS_ERR(sdhci_am654->clk_ahb)) {
>>> +		dev_err(dev, "clk_ahb clock not found.\n");
>>> +		ret = PTR_ERR(sdhci_am654->clk_ahb);
>>> +		goto err_pltfm_free;
>>> +	}
>>
>> Did you intend not to enable clks?
> 
> Yes. Clocks get enabled as a part of pm_runtime calls.

Ok, but that could use an explanatory comment.  Also why get a reference to
clk_ahb if that reference is never used?

>>
>>> +
>>> +	pltfm_host->clk = clk_xin;
>>> +
>>> +	pm_runtime_enable(dev);
>>> +	ret = pm_runtime_get_sync(dev);
>>> +	if (ret > 0) {
>>
>> Did you intend 'ret > 0'?
> 
> Sorry. That was intended to be < 0.
> 
> Thanks,
> Faiz
> 

Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Faiz Abbas]

Hi Adrian,

On 07/12/18 7:02 PM, Adrian Hunter wrote:
> On 5/12/18 5:07 PM, Faiz Abbas wrote:
>> Hi Adrian,
>>
>> On 05/12/18 7:12 PM, Adrian Hunter wrote:
>>> On 29/11/18 6:15 PM, Faiz Abbas wrote:
>>>> The host controllers on TI's AM654 SOCs are not compatible with
>>>> the phy and consumer model of the sdhci-of-arasan driver. It turns out
>>>> that for optimal operation at higher speeds, a special tuning procedure
>>>> needs to be implemented which involves configuration of platform
>>>> specific phy registers.
>>>>
>>>> Therefore, branch out to a new sdhci_am654 driver and add the phy
>>>> register space with all phy configurations to it. Populate AM654
>>>> specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
>>>> applicable.
>>>>
>>>> Only add support for upto High Speed for SD card and upto DDR52 speed
>>>> mode for eMMC. Higher speeds will be added in subsequent patches.
>>>>
...
>>>> +
>>>> +	sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
>>>> +	if (IS_ERR(sdhci_am654->clk_ahb)) {
>>>> +		dev_err(dev, "clk_ahb clock not found.\n");
>>>> +		ret = PTR_ERR(sdhci_am654->clk_ahb);
>>>> +		goto err_pltfm_free;
>>>> +	}
>>>
>>> Did you intend not to enable clks?
>>
>> Yes. Clocks get enabled as a part of pm_runtime calls.
> 
> Ok, but that could use an explanatory comment.  Also why get a reference to
> clk_ahb if that reference is never used?
> 

You're right. It was being used in sdhci-of-arasan because other users
needed to call enable() and disable(). I missed out on removing it when
porting over. Will remove it and add the comment.

Thanks,
Faiz