[PATCH v2 00/21] Add support for MMC higher speed modes for TI's am65x, j721e and j7200 platforms

[PATCH v2 00/21] Add support for MMC higher speed modes for TI's am65x, j721e and j7200 platforms

[Faiz Abbas]

The following patches add support for higher speeds in the SD card and
eMMC for TI's am65x, j721e, j7200 platforms.

With these patches, the following max speeds are supported:
j721e: DDR50, HS200
j7200: SDR104, HS200
am65x: SDR104*, HS200

v2:
1. Added patches to support UHS modes for the SD card even in am654x platforms.
2. Fixed an issue with patch 1 that was breaking builds on some platforms.

* There's an issue with the am65x base board such that the power cycle
circuit to the card takes way longer than the wait time in mmc core.
Until this is fixed, am654x-evm and -idk will only support High speed mode
at 3.3V (see patch 20) but this shouldn't block us from adding UHS modes in the
dtsi as well as in the configs so other boards can still take advantage of the
higher speed. UHS modes have been tested by adding the appropriate delay in the
power cycle circuit.

Link to v1:
https://patchwork.ozlabs.org/project/uboot/list/?series=206622

Faiz Abbas (21):
  mmc: sdhci: Add helper functions for UHS modes
  mmc: am654_sdhci: Unconditionally switch off DLL in the beginning of
    ios_post()
  mmc: am654_sdhci: Convert flag fields to BIT macro
  mmc: am654_sdhci: Add flag for PHY calibration
  mmc: am654_sdhci: Add support for AM65x SR2.0
  mmc: am654_sdhci: Add support for input tap delay
  mmc: am654_sdhci: Add support for writing to clkbuf_sel
  mmc: am654_sdhci: Add support for software tuning
  mmc: am654_sdhci: Fix HISPD bit configuration in some lower speed
    modes
  mmc: am654_sdhci: Use sdhci_set_control_reg()
  arm: dts: k3-am65: Fix mmc nodes
  arm: dts: k3-j721e-main: Update otap-delay values
  arm: dts: k3-j721e-common-proc-board: Add support for UHS modes for SD
    card
  arm: dts: k3-j7200-main: Add support for gpio0
  arm: dts: k3-j7200-common-proc-board: Enable support for UHS modes
  configs: j721e_evm: Add support for UHS modes
  configs: j7200_evm: Add support for UHS modes
  i2c: Makefile: Add SPL_DM_I2C_GPIO
  arm: dts: k3-am65-main: Add itapdly and clkbuf-sel values
  arm: dts: k3-am654-base-board: Limit Sd card to High speed modes
  configs: am65x_evm: Add configs for UHS modes

 arch/arm/dts/k3-am65-main.dtsi                |  31 ++
 arch/arm/dts/k3-am654-base-board-u-boot.dtsi  |  67 +--
 arch/arm/dts/k3-am654-base-board.dts          |  26 ++
 arch/arm/dts/k3-am654-r5-base-board.dts       |  20 +-
 arch/arm/dts/k3-j7200-common-proc-board.dts   |  49 ++-
 arch/arm/dts/k3-j7200-main.dtsi               |  23 ++
 .../arm/dts/k3-j7200-r5-common-proc-board.dts |  15 +
 arch/arm/dts/k3-j721e-common-proc-board.dts   |  32 ++
 arch/arm/dts/k3-j721e-main.dtsi               |   8 +-
 configs/am65x_evm_a53_defconfig               |   8 +
 configs/am65x_evm_r5_defconfig                |   2 +
 configs/j7200_evm_a72_defconfig               |   8 +
 configs/j7200_evm_r5_defconfig                |   1 +
 configs/j721e_evm_a72_defconfig               |   8 +
 configs/j721e_evm_r5_defconfig                |   1 +
 drivers/i2c/Makefile                          |   2 +-
 drivers/mmc/Kconfig                           |   1 +
 drivers/mmc/am654_sdhci.c                     | 384 +++++++++++++-----
 drivers/mmc/sdhci.c                           |  51 +++
 include/sdhci.h                               |   1 +
 20 files changed, 564 insertions(+), 174 deletions(-)

-- 
2.17.1

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Faiz Abbas]

Add a set_voltage() function which handles the switch from 3.3V to 1.8V
for SD card UHS modes.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
 include/sdhci.h     |  1 +
 2 files changed, 52 insertions(+)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 7673219fb3..a69f058191 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -20,6 +20,7 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <phys2bus.h>
+#include <power/regulator.h>
 
 static void sdhci_reset(struct sdhci_host *host, u8 mask)
 {
@@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
 	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
 }
 
+#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
+static void sdhci_set_voltage(struct sdhci_host *host)
+{
+	struct mmc *mmc = (struct mmc *)host->mmc;
+	u32 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+
+	switch (mmc->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+#if CONFIG_IS_ENABLED(DM_REGULATOR)
+		if (mmc->vqmmc_supply) {
+			regulator_set_enable(mmc->vqmmc_supply, false);
+			regulator_set_value(mmc->vqmmc_supply, 3300000);
+			regulator_set_enable(mmc->vqmmc_supply, true);
+		}
+#endif
+		mdelay(5);
+		if (IS_SD(mmc)) {
+			ctrl &= ~SDHCI_CTRL_VDD_180;
+			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		}
+		break;
+	case MMC_SIGNAL_VOLTAGE_180:
+#if CONFIG_IS_ENABLED(DM_REGULATOR)
+		if (mmc->vqmmc_supply) {
+			regulator_set_enable(mmc->vqmmc_supply, false);
+			regulator_set_value(mmc->vqmmc_supply, 1800000);
+			regulator_set_enable(mmc->vqmmc_supply, true);
+		}
+#endif
+		if (IS_SD(mmc)) {
+			ctrl |= SDHCI_CTRL_VDD_180;
+			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		}
+		break;
+	default:
+		/* No signal voltage switch required */
+		return;
+	}
+}
+#else
+static void sdhci_set_voltage(struct sdhci_host *host) { }
+#endif
+void sdhci_set_control_reg(struct sdhci_host *host)
+{
+	sdhci_set_voltage(host);
+	sdhci_set_uhs_timing(host);
+}
+
 #ifdef CONFIG_DM_MMC
 static int sdhci_set_ios(struct udevice *dev)
 {
diff --git a/include/sdhci.h b/include/sdhci.h
index 94fc3ed56a..d3f8741042 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -492,6 +492,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host);
 /* Export the operations to drivers */
 int sdhci_probe(struct udevice *dev);
 int sdhci_set_clock(struct mmc *mmc, unsigned int clock);
+void sdhci_set_control_reg(struct sdhci_host *host);
 extern const struct dm_mmc_ops sdhci_ops;
 #else
 #endif
-- 
2.17.1

[PATCH v2 02/21] mmc: am654_sdhci: Unconditionally switch off DLL in the beginning of ios_post()

[Faiz Abbas]

There are some speed modes that work without switching the dll on.
Unconditionally switch off the DLL before setting clock frequency to
support this case. The software will automatically enable DLL for speed
modes that require it. This also means the dll_on priv data member is no
longer required.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 82abf484e4..15a47f1e71 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -84,7 +84,6 @@ struct am654_sdhci_plat {
 #define IOMUX_PRESENT	(1 << 1)
 #define FREQSEL_2_BIT	(1 << 2)
 #define STRBSEL_4_BIT	(1 << 3)
-	bool dll_on;
 };
 
 struct timing_data {
@@ -141,12 +140,7 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 	val &= ~SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, val, SDHCI_CLOCK_CONTROL);
 
-	/* power off phy */
-	if (plat->dll_on) {
-		regmap_update_bits(plat->base, PHY_CTRL1, ENDLL_MASK, 0);
-
-		plat->dll_on = false;
-	}
+	regmap_update_bits(plat->base, PHY_CTRL1, ENDLL_MASK, 0);
 
 	/* restart clock */
 	sdhci_set_clock(host->mmc, speed);
@@ -212,8 +206,6 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 					 val & DLLRDY_MASK, 1000, 1000000);
 		if (ret)
 			return ret;
-
-		plat->dll_on = true;
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v2 03/21] mmc: am654_sdhci: Convert flag fields to BIT macro

[Faiz Abbas]

Convert the flags field defines to use the BIT() macro.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 15a47f1e71..10350d8d61 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -80,10 +80,10 @@ struct am654_sdhci_plat {
 	u32 drv_strength;
 	u32 strb_sel;
 	u32 flags;
-#define DLL_PRESENT	(1 << 0)
-#define IOMUX_PRESENT	(1 << 1)
-#define FREQSEL_2_BIT	(1 << 2)
-#define STRBSEL_4_BIT	(1 << 3)
+#define DLL_PRESENT	BIT(0)
+#define IOMUX_PRESENT	BIT(1)
+#define FREQSEL_2_BIT	BIT(2)
+#define STRBSEL_4_BIT	BIT(3)
 };
 
 struct timing_data {
-- 
2.17.1

[PATCH v2 04/21] mmc: am654_sdhci: Add flag for PHY calibration

[Faiz Abbas]

Not all controllers need calibration for the PHY DLL. Add a DLL_CALIB
flag to indicate the same.

Also move the write of trm_icp and driver strength to the set_clock()
function to match the kernel configuration flow.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 10350d8d61..145373ad46 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -84,6 +84,7 @@ struct am654_sdhci_plat {
 #define IOMUX_PRESENT	BIT(1)
 #define FREQSEL_2_BIT	BIT(2)
 #define STRBSEL_4_BIT	BIT(3)
+#define DLL_CALIB	BIT(4)
 };
 
 struct timing_data {
@@ -195,6 +196,15 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 					   freqsel << FREQSEL_SHIFT);
 		}
 
+		/* Configure DLL TRIM */
+		mask = DLL_TRIM_ICP_MASK;
+		val = plat->trm_icp << DLL_TRIM_ICP_SHIFT;
+
+		/* Configure DLL driver strength */
+		mask |= DR_TY_MASK;
+		val |= plat->drv_strength << DR_TY_SHIFT;
+		regmap_update_bits(plat->base, PHY_CTRL1, mask, val);
+
 		/* Enable DLL */
 		regmap_update_bits(plat->base, PHY_CTRL1, ENDLL_MASK,
 				   0x1 << ENDLL_SHIFT);
@@ -221,7 +231,7 @@ int am654_sdhci_init(struct am654_sdhci_plat *plat)
 	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
 	regmap_update_bits(plat->base, PHY_CTRL4, mask, 0x0);
 
-	if (plat->flags & DLL_PRESENT) {
+	if (plat->flags & DLL_CALIB) {
 		regmap_read(plat->base, PHY_STAT1, &val);
 		if (~val & CALDONE_MASK) {
 			/* Calibrate IO lines */
@@ -233,15 +243,6 @@ int am654_sdhci_init(struct am654_sdhci_plat *plat)
 			if (ret)
 				return ret;
 		}
-
-		/* Configure DLL TRIM */
-		mask = DLL_TRIM_ICP_MASK;
-		val = plat->trm_icp << DLL_TRIM_ICP_SHIFT;
-
-		/* Configure DLL driver strength */
-		mask |= DR_TY_MASK;
-		val |= plat->drv_strength << DR_TY_SHIFT;
-		regmap_update_bits(plat->base, PHY_CTRL1, mask, val);
 	}
 
 	/* Enable pins by setting IO mux to 0 */
@@ -292,12 +293,13 @@ const struct sdhci_ops am654_sdhci_ops = {
 
 const struct am654_driver_data am654_drv_data = {
 	.ops = &am654_sdhci_ops,
-	.flags = IOMUX_PRESENT | FREQSEL_2_BIT | DLL_PRESENT | STRBSEL_4_BIT,
+	.flags = IOMUX_PRESENT | FREQSEL_2_BIT | DLL_PRESENT | DLL_CALIB |
+		 STRBSEL_4_BIT,
 };
 
 const struct am654_driver_data j721e_8bit_drv_data = {
 	.ops = &am654_sdhci_ops,
-	.flags = DLL_PRESENT,
+	.flags = DLL_PRESENT | DLL_CALIB,
 };
 
 static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
-- 
2.17.1

[PATCH v2 05/21] mmc: am654_sdhci: Add support for AM65x SR2.0

[Faiz Abbas]

Add Support for AM65x PG2.0. Use the SoC bus framework to fixup
the platform data and do DLL calibration if the revision is 1.0

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 145373ad46..71798be765 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -12,6 +12,7 @@
 #include <power-domain.h>
 #include <regmap.h>
 #include <sdhci.h>
+#include <soc.h>
 #include <dm/device_compat.h>
 #include <linux/bitops.h>
 #include <linux/err.h>
@@ -292,6 +293,11 @@ const struct sdhci_ops am654_sdhci_ops = {
 };
 
 const struct am654_driver_data am654_drv_data = {
+	.ops = &am654_sdhci_ops,
+	.flags = DLL_PRESENT | IOMUX_PRESENT | FREQSEL_2_BIT | STRBSEL_4_BIT,
+};
+
+const struct am654_driver_data am654_sr1_drv_data = {
 	.ops = &am654_sdhci_ops,
 	.flags = IOMUX_PRESENT | FREQSEL_2_BIT | DLL_PRESENT | DLL_CALIB |
 		 STRBSEL_4_BIT,
@@ -326,6 +332,11 @@ const struct am654_driver_data j721e_4bit_drv_data = {
 	.flags = IOMUX_PRESENT,
 };
 
+const struct soc_attr am654_sdhci_soc_attr[] = {
+	{ .family = "AM65X", .revision = "SR1.0", .data = &am654_sr1_drv_data},
+	{/* sentinel */}
+};
+
 static int sdhci_am654_get_otap_delay(struct udevice *dev,
 				      struct mmc_config *cfg)
 {
@@ -365,6 +376,8 @@ static int am654_sdhci_probe(struct udevice *dev)
 	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
 	struct sdhci_host *host = dev_get_priv(dev);
 	struct mmc_config *cfg = &plat->cfg;
+	const struct soc_attr *soc;
+	const struct am654_driver_data *soc_drv_data;
 	struct clk clk;
 	unsigned long clock;
 	int ret;
@@ -394,6 +407,14 @@ static int am654_sdhci_probe(struct udevice *dev)
 		return ret;
 
 	host->ops = drv_data->ops;
+
+	/* Update ops based on SoC revision */
+	soc = soc_device_match(am654_sdhci_soc_attr);
+	if (soc && soc->data) {
+		soc_drv_data = soc->data;
+		host->ops = soc_drv_data->ops;
+	}
+
 	host->mmc->priv = host;
 	upriv->mmc = host->mmc;
 
@@ -458,9 +479,18 @@ static int am654_sdhci_bind(struct udevice *dev)
 	struct am654_driver_data *drv_data =
 			(struct am654_driver_data *)dev_get_driver_data(dev);
 	struct am654_sdhci_plat *plat = dev_get_platdata(dev);
+	const struct soc_attr *soc;
+	const struct am654_driver_data *soc_drv_data;
 
 	plat->flags = drv_data->flags;
 
+	/* Update flags based on SoC revision */
+	soc = soc_device_match(am654_sdhci_soc_attr);
+	if (soc && soc->data) {
+		soc_drv_data = soc->data;
+		plat->flags = soc_drv_data->flags;
+	}
+
 	return sdhci_bind(dev, &plat->mmc, &plat->cfg);
 }
 
-- 
2.17.1

[PATCH v2 06/21] mmc: am654_sdhci: Add support for input tap delay

[Faiz Abbas]

DLL need only be enabled for speed modes and clock frequencies at or
above 50 MHz. For speed modes that don't enable the DLL, we need to
configure a static input delay value. This involves reading an optional
itap-del-sel-* value from the device tree and configuring it for the
appropriate speed mode.

Therefore, move all dll configurations to their own functions and gate it
with 50 MHz speed and a minimum mode. If both these conditions are not
satisfied then configure delay chain modes.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 241 +++++++++++++++++++++++++-------------
 1 file changed, 161 insertions(+), 80 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 71798be765..f472672152 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -62,6 +62,16 @@
 #define CALDONE_MASK		BIT(CALDONE_SHIFT)
 #define RETRIM_SHIFT		17
 #define RETRIM_MASK		BIT(RETRIM_SHIFT)
+#define SELDLYTXCLK_SHIFT	17
+#define SELDLYTXCLK_MASK	BIT(SELDLYTXCLK_SHIFT)
+#define SELDLYRXCLK_SHIFT	16
+#define SELDLYRXCLK_MASK	BIT(SELDLYRXCLK_SHIFT)
+#define ITAPDLYSEL_SHIFT	0
+#define ITAPDLYSEL_MASK		GENMASK(4, 0)
+#define ITAPDLYENA_SHIFT	8
+#define ITAPDLYENA_MASK		BIT(ITAPDLYENA_SHIFT)
+#define ITAPCHGWIN_SHIFT	9
+#define ITAPCHGWIN_MASK		BIT(ITAPCHGWIN_SHIFT)
 
 #define DRIVER_STRENGTH_50_OHM	0x0
 #define DRIVER_STRENGTH_33_OHM	0x1
@@ -70,6 +80,7 @@
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define AM654_SDHCI_MIN_FREQ	400000
+#define CLOCK_TOO_SLOW_HZ	50000000
 
 struct am654_sdhci_plat {
 	struct mmc_config cfg;
@@ -77,6 +88,7 @@ struct am654_sdhci_plat {
 	struct regmap *base;
 	bool non_removable;
 	u32 otap_del_sel[MMC_MODES_END];
+	u32 itap_del_sel[MMC_MODES_END];
 	u32 trm_icp;
 	u32 drv_strength;
 	u32 strb_sel;
@@ -89,22 +101,45 @@ struct am654_sdhci_plat {
 };
 
 struct timing_data {
-	const char *binding;
+	const char *otap_binding;
+	const char *itap_binding;
 	u32 capability;
 };
 
 static const struct timing_data td[] = {
-	[MMC_LEGACY] = {"ti,otap-del-sel-legacy", 0},
-	[MMC_HS] = {"ti,otap-del-sel-mmc-hs", MMC_CAP(MMC_HS)},
-	[SD_HS]  = {"ti,otap-del-sel-sd-hs", MMC_CAP(SD_HS)},
-	[UHS_SDR12] = {"ti,otap-del-sel-sdr12", MMC_CAP(UHS_SDR12)},
-	[UHS_SDR25] = {"ti,otap-del-sel-sdr25", MMC_CAP(UHS_SDR25)},
-	[UHS_SDR50] = {"ti,otap-del-sel-sdr50", MMC_CAP(UHS_SDR50)},
-	[UHS_SDR104] = {"ti,otap-del-sel-sdr104", MMC_CAP(UHS_SDR104)},
-	[UHS_DDR50] = {"ti,otap-del-sel-ddr50", MMC_CAP(UHS_DDR50)},
-	[MMC_DDR_52] = {"ti,otap-del-sel-ddr52", MMC_CAP(MMC_DDR_52)},
-	[MMC_HS_200] = {"ti,otap-del-sel-hs200", MMC_CAP(MMC_HS_200)},
-	[MMC_HS_400] = {"ti,otap-del-sel-hs400", MMC_CAP(MMC_HS_400)},
+	[MMC_LEGACY]	= {"ti,otap-del-sel-legacy",
+			   "ti,itap-del-sel-legacy",
+			   0},
+	[MMC_HS]	= {"ti,otap-del-sel-mmc-hs",
+			   "ti,itap-del-sel-mms-hs",
+			   MMC_CAP(MMC_HS)},
+	[SD_HS]		= {"ti,otap-del-sel-sd-hs",
+			   "ti,itap-del-sel-sd-hs",
+			   MMC_CAP(SD_HS)},
+	[UHS_SDR12]	= {"ti,otap-del-sel-sdr12",
+			   "ti,itap-del-sel-sdr12",
+			   MMC_CAP(UHS_SDR12)},
+	[UHS_SDR25]	= {"ti,otap-del-sel-sdr25",
+			   "ti,itap-del-sel-sdr25",
+			   MMC_CAP(UHS_SDR25)},
+	[UHS_SDR50]	= {"ti,otap-del-sel-sdr50",
+			   NULL,
+			   MMC_CAP(UHS_SDR50)},
+	[UHS_SDR104]	= {"ti,otap-del-sel-sdr104",
+			   NULL,
+			   MMC_CAP(UHS_SDR104)},
+	[UHS_DDR50]	= {"ti,otap-del-sel-ddr50",
+			   NULL,
+			   MMC_CAP(UHS_DDR50)},
+	[MMC_DDR_52]	= {"ti,otap-del-sel-ddr52",
+			   "ti,itap-del-sel-ddr52",
+			   MMC_CAP(MMC_DDR_52)},
+	[MMC_HS_200]	= {"ti,otap-del-sel-hs200",
+			   NULL,
+			   MMC_CAP(MMC_HS_200)},
+	[MMC_HS_400]	= {"ti,otap-del-sel-hs400",
+			   NULL,
+			   MMC_CAP(MMC_HS_400)},
 };
 
 struct am654_driver_data {
@@ -127,12 +162,99 @@ static void am654_sdhci_set_control_reg(struct sdhci_host *host)
 	sdhci_set_uhs_timing(host);
 }
 
+static int am654_sdhci_setup_dll(struct am654_sdhci_plat *plat,
+				 unsigned int speed)
+{
+	int sel50, sel100, freqsel;
+	u32 mask, val;
+	int ret;
+
+	/* Disable delay chain mode */
+	regmap_update_bits(plat->base, PHY_CTRL5,
+			   SELDLYTXCLK_MASK | SELDLYRXCLK_MASK, 0);
+
+	if (plat->flags & FREQSEL_2_BIT) {
+		switch (speed) {
+		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(plat->base, PHY_CTRL5, mask, val);
+	} else {
+		switch (speed) {
+		case 200000000:
+			freqsel = 0x0;
+			break;
+		default:
+			freqsel = 0x4;
+		}
+		regmap_update_bits(plat->base, PHY_CTRL5, FREQSEL_MASK,
+				   freqsel << FREQSEL_SHIFT);
+	}
+
+	/* Configure DLL TRIM */
+	mask = DLL_TRIM_ICP_MASK;
+	val = plat->trm_icp << DLL_TRIM_ICP_SHIFT;
+
+	/* Configure DLL driver strength */
+	mask |= DR_TY_MASK;
+	val |= plat->drv_strength << DR_TY_SHIFT;
+	regmap_update_bits(plat->base, PHY_CTRL1, mask, val);
+
+	/* Enable DLL */
+	regmap_update_bits(plat->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(plat->base, PHY_STAT1, val,
+				       val & DLLRDY_MASK, 1000, 1000000);
+
+	return ret;
+}
+
+static void am654_sdhci_write_itapdly(struct am654_sdhci_plat *plat,
+				      u32 itapdly)
+{
+	/* Set ITAPCHGWIN before writing to ITAPDLY */
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
+			   1 << ITAPCHGWIN_SHIFT);
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYSEL_MASK,
+			   itapdly << ITAPDLYSEL_SHIFT);
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
+}
+
+static void am654_sdhci_setup_delay_chain(struct am654_sdhci_plat *plat,
+					  int mode)
+{
+	u32 mask, val;
+
+	val = 1 << SELDLYTXCLK_SHIFT | 1 << SELDLYRXCLK_SHIFT;
+	mask = SELDLYTXCLK_MASK | SELDLYRXCLK_MASK;
+	regmap_update_bits(plat->base, PHY_CTRL5, mask, val);
+
+	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode]);
+}
+
 static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 {
 	struct udevice *dev = host->mmc->dev;
 	struct am654_sdhci_plat *plat = dev_get_platdata(dev);
 	unsigned int speed = host->mmc->clock;
-	int sel50, sel100, freqsel;
+	int mode = host->mmc->selected_mode;
 	u32 otap_del_sel;
 	u32 mask, val;
 	int ret;
@@ -148,75 +270,29 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 	sdhci_set_clock(host->mmc, speed);
 
 	/* switch phy back on */
-	if (speed > AM654_SDHCI_MIN_FREQ) {
-		otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
-		mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
-		val = (1 << OTAPDLYENA_SHIFT) |
-		      (otap_del_sel << OTAPDLYSEL_SHIFT);
-
-		/* Write to STRBSEL for HS400 speed mode */
-		if (host->mmc->selected_mode == MMC_HS_400) {
-			if (plat->flags & STRBSEL_4_BIT)
-				mask |= STRBSEL_4BIT_MASK;
-			else
-				mask |= STRBSEL_8BIT_MASK;
-
-			val |= plat->strb_sel << STRBSEL_SHIFT;
-		}
+	otap_del_sel = plat->otap_del_sel[mode];
+	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
+	val = (1 << OTAPDLYENA_SHIFT) |
+	      (otap_del_sel << OTAPDLYSEL_SHIFT);
 
-		regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
-
-		if (plat->flags & FREQSEL_2_BIT) {
-			switch (speed) {
-			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(plat->base, PHY_CTRL5, mask, val);
-		} else {
-			switch (speed) {
-			case 200000000:
-				freqsel = 0x0;
-				break;
-			default:
-				freqsel = 0x4;
-			}
-			regmap_update_bits(plat->base, PHY_CTRL5, FREQSEL_MASK,
-					   freqsel << FREQSEL_SHIFT);
-		}
+	/* Write to STRBSEL for HS400 speed mode */
+	if (host->mmc->selected_mode == MMC_HS_400) {
+		if (plat->flags & STRBSEL_4_BIT)
+			mask |= STRBSEL_4BIT_MASK;
+		else
+			mask |= STRBSEL_8BIT_MASK;
 
-		/* Configure DLL TRIM */
-		mask = DLL_TRIM_ICP_MASK;
-		val = plat->trm_icp << DLL_TRIM_ICP_SHIFT;
-
-		/* Configure DLL driver strength */
-		mask |= DR_TY_MASK;
-		val |= plat->drv_strength << DR_TY_SHIFT;
-		regmap_update_bits(plat->base, PHY_CTRL1, mask, val);
-
-		/* Enable DLL */
-		regmap_update_bits(plat->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(plat->base, PHY_STAT1, val,
-					 val & DLLRDY_MASK, 1000, 1000000);
+		val |= plat->strb_sel << STRBSEL_SHIFT;
+	}
+
+	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
+
+	if (mode > UHS_SDR25 && speed >= CLOCK_TOO_SLOW_HZ) {
+		ret = am654_sdhci_setup_dll(plat, speed);
 		if (ret)
 			return ret;
+	} else {
+		am654_sdhci_setup_delay_chain(plat, mode);
 	}
 
 	return 0;
@@ -354,15 +430,20 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
 	 * value is not found
 	 */
 	for (i = MMC_HS; i <= MMC_HS_400; i++) {
-		ret = dev_read_u32(dev, td[i].binding, &plat->otap_del_sel[i]);
+		ret = dev_read_u32(dev, td[i].otap_binding,
+				   &plat->otap_del_sel[i]);
 		if (ret) {
-			dev_dbg(dev, "Couldn't find %s\n", td[i].binding);
+			dev_dbg(dev, "Couldn't find %s\n", td[i].otap_binding);
 			/*
 			 * Remove the corresponding capability
 			 * if an otap-del-sel value is not found
 			 */
 			cfg->host_caps &= ~td[i].capability;
 		}
+
+		if (td[i].itap_binding)
+			dev_read_u32(dev, td[i].itap_binding,
+				     &plat->itap_del_sel[i]);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v2 07/21] mmc: am654_sdhci: Add support for writing to clkbuf_sel

[Faiz Abbas]

Add support for writing new clock buffer select property for both
the am654x and j721e 4 bit IPs

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index f472672152..fa118fc56c 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -48,6 +48,8 @@
 #define SEL100_MASK		BIT(SEL100_SHIFT)
 #define FREQSEL_SHIFT		8
 #define FREQSEL_MASK		GENMASK(10, 8)
+#define CLKBUFSEL_SHIFT		0
+#define CLKBUFSEL_MASK		GENMASK(2, 0)
 #define DLL_TRIM_ICP_SHIFT	4
 #define DLL_TRIM_ICP_MASK	GENMASK(7, 4)
 #define DR_TY_SHIFT		20
@@ -92,6 +94,7 @@ struct am654_sdhci_plat {
 	u32 trm_icp;
 	u32 drv_strength;
 	u32 strb_sel;
+	u32 clkbuf_sel;
 	u32 flags;
 #define DLL_PRESENT	BIT(0)
 #define IOMUX_PRESENT	BIT(1)
@@ -295,6 +298,9 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 		am654_sdhci_setup_delay_chain(plat, mode);
 	}
 
+	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
+			   plat->clkbuf_sel);
+
 	return 0;
 }
 
@@ -395,6 +401,9 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
 	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
 
+	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
+			   plat->clkbuf_sel);
+
 	return 0;
 }
 
@@ -548,6 +557,8 @@ static int am654_sdhci_ofdata_to_platdata(struct udevice *dev)
 		}
 	}
 
+	dev_read_u32(dev, "ti,clkbuf-sel", &plat->clkbuf_sel);
+
 	ret = mmc_of_parse(dev, cfg);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH v2 08/21] mmc: am654_sdhci: Add support for software tuning

[Faiz Abbas]

With the new SW tuning App note[1], a custom tuning algorithm is
required for eMMC HS200, HS400 and SD card UHS modes. The algorithm
involves running through the 32 possible input tap delay values and
sending the appropriate tuning command (CMD19/21) for each of them
to get a fail or pass result for each of the values. Typically, the
range will have a small contiguous failing window. Considering the
tuning range as a circular buffer, the algorithm then sets a final
tuned value directly opposite to the failing window.

[1] https://www.ti.com/lit/pdf/spract9

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 45 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index fa118fc56c..79b4331c3c 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -9,6 +9,7 @@
 #include <common.h>
 #include <dm.h>
 #include <malloc.h>
+#include <mmc.h>
 #include <power-domain.h>
 #include <regmap.h>
 #include <sdhci.h>
@@ -368,7 +369,48 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
 	return sdhci_probe(dev);
 }
 
+#ifdef MMC_SUPPORTS_TUNING
+#define ITAP_MAX	32
+static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
+{
+	struct udevice *dev = mmc->dev;
+	struct am654_sdhci_plat *plat = dev_get_platdata(dev);
+	int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len;
+	u32 itap;
+
+	/* Enable ITAPDLY */
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
+			   1 << ITAPDLYENA_SHIFT);
+
+	for (itap = 0; itap < ITAP_MAX; itap++) {
+		am654_sdhci_write_itapdly(plat, itap);
+
+		cur_val = !mmc_send_tuning(mmc, opcode, NULL);
+		if (cur_val && !prev_val)
+			pass_window = itap;
+
+		if (!cur_val)
+			fail_len++;
+
+		prev_val = cur_val;
+	}
+	/*
+	 * Having determined the length of the failing window and start of
+	 * the passing window calculate the length of the passing window and
+	 * set the final value halfway through it considering the range as a
+	 * circular buffer
+	 */
+	pass_len = ITAP_MAX - fail_len;
+	itap = (pass_window + (pass_len >> 1)) % ITAP_MAX;
+	am654_sdhci_write_itapdly(plat, itap);
+
+	return 0;
+}
+#endif
 const struct sdhci_ops am654_sdhci_ops = {
+#ifdef MMC_SUPPORTS_TUNING
+	.platform_execute_tuning = am654_sdhci_execute_tuning,
+#endif
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &am654_sdhci_set_ios_post,
 	.set_control_reg	= &am654_sdhci_set_control_reg,
@@ -408,6 +450,9 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 }
 
 const struct sdhci_ops j721e_4bit_sdhci_ops = {
+#ifdef MMC_SUPPORTS_TUNING
+	.platform_execute_tuning = am654_sdhci_execute_tuning,
+#endif
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &j721e_4bit_sdhci_set_ios_post,
 };
-- 
2.17.1

[PATCH v2 09/21] mmc: am654_sdhci: Fix HISPD bit configuration in some lower speed modes

[Faiz Abbas]

According to the AM654x Data Manual[1], the setup timing in lower speed
modes can only be met if the controller uses a falling edge data launch.

To ensure this, the HIGH_SPEED_ENA (HOST_CONTROL[2]) bit should be
cleared in default speed, SD high speed, MMC high speed, SDR12 and SDR25
speed modes.

Use the sdhci writeb callback to implement this condition.

[1] http://www.ti.com/lit/gpn/am6546 Section 5.10.5.16.1

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/Kconfig       |  1 +
 drivers/mmc/am654_sdhci.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 0c252e34c7..525445949d 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -521,6 +521,7 @@ config MMC_SDHCI_AM654
 	depends on MMC_SDHCI
 	depends on DM_MMC && OF_CONTROL && BLK
 	depends on REGMAP
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  Support for Secure Digital Host Controller Interface (SDHCI)
 	  controllers present on TI's AM654 SOCs.
diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 79b4331c3c..410517398a 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -369,6 +369,26 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
 	return sdhci_probe(dev);
 }
 
+static void am654_sdhci_write_b(struct sdhci_host *host, u8 val, int reg)
+{
+	if (reg == SDHCI_HOST_CONTROL) {
+		switch (host->mmc->selected_mode) {
+		/*
+		 * According to the data manual, HISPD bit
+		 * should not be set in these speed modes.
+		 */
+		case SD_HS:
+		case MMC_HS:
+		case UHS_SDR12:
+		case UHS_SDR25:
+			val &= ~SDHCI_CTRL_HISPD;
+		default:
+			break;
+		}
+	}
+
+	writeb(val, host->ioaddr + reg);
+}
 #ifdef MMC_SUPPORTS_TUNING
 #define ITAP_MAX	32
 static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
@@ -414,6 +434,7 @@ const struct sdhci_ops am654_sdhci_ops = {
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &am654_sdhci_set_ios_post,
 	.set_control_reg	= &am654_sdhci_set_control_reg,
+	.write_b		= am654_sdhci_write_b,
 };
 
 const struct am654_driver_data am654_drv_data = {
@@ -455,6 +476,7 @@ const struct sdhci_ops j721e_4bit_sdhci_ops = {
 #endif
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &j721e_4bit_sdhci_set_ios_post,
+	.write_b		= am654_sdhci_write_b,
 };
 
 const struct am654_driver_data j721e_4bit_drv_data = {
@@ -532,6 +554,7 @@ static int am654_sdhci_probe(struct udevice *dev)
 	host->max_clk = clock;
 	host->mmc = &plat->mmc;
 	host->mmc->dev = dev;
+	host->ops = drv_data->ops;
 	ret = sdhci_setup_cfg(cfg, host, cfg->f_max,
 			      AM654_SDHCI_MIN_FREQ);
 	if (ret)
@@ -541,8 +564,6 @@ static int am654_sdhci_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
-	host->ops = drv_data->ops;
-
 	/* Update ops based on SoC revision */
 	soc = soc_device_match(am654_sdhci_soc_attr);
 	if (soc && soc->data) {
-- 
2.17.1

[PATCH v2 10/21] mmc: am654_sdhci: Use sdhci_set_control_reg()

[Faiz Abbas]

Use the generic sdhci_set_control_reg() instead of duplicating
in platform driver.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/am654_sdhci.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 410517398a..92aac8c0e7 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -151,21 +151,6 @@ struct am654_driver_data {
 	u32 flags;
 };
 
-static void am654_sdhci_set_control_reg(struct sdhci_host *host)
-{
-	struct mmc *mmc = (struct mmc *)host->mmc;
-	u32 reg;
-
-	if (IS_SD(host->mmc) &&
-	    mmc->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
-		reg = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-		reg |= SDHCI_CTRL_VDD_180;
-		sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
-	}
-
-	sdhci_set_uhs_timing(host);
-}
-
 static int am654_sdhci_setup_dll(struct am654_sdhci_plat *plat,
 				 unsigned int speed)
 {
@@ -433,7 +418,7 @@ const struct sdhci_ops am654_sdhci_ops = {
 #endif
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &am654_sdhci_set_ios_post,
-	.set_control_reg	= &am654_sdhci_set_control_reg,
+	.set_control_reg	= sdhci_set_control_reg,
 	.write_b		= am654_sdhci_write_b,
 };
 
@@ -476,6 +461,7 @@ const struct sdhci_ops j721e_4bit_sdhci_ops = {
 #endif
 	.deferred_probe		= am654_sdhci_deferred_probe,
 	.set_ios_post		= &j721e_4bit_sdhci_set_ios_post,
+	.set_control_reg	= sdhci_set_control_reg,
 	.write_b		= am654_sdhci_write_b,
 };
 
-- 
2.17.1

[PATCH v2 11/21] arm: dts: k3-am65: Fix mmc nodes

[Faiz Abbas]

Because of fundamental interface issues in am65x pg1, only the
initial sdhci1 node at 25 MHz was added in the u-boot.dtsi
from which both the base-board.dts and r5-base-board.dts
inherit the node. Move the node out to k3-am65-main.dtsi
where it belongs and add the board specific properties
in base-board.dts and r5-base-board.dts

This ensures dts compatibility with the kernel dts in the
base-board.dts and enables the SD card interface at 50 MHz
and High Speed mode

While we are here, also fix the main_mmc0_pins_default
property to be included and inherit from the base-board.dts
instead of the u-boot.dtsi

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-am65-main.dtsi               | 22 +++++++
 arch/arm/dts/k3-am654-base-board-u-boot.dtsi | 67 +++-----------------
 arch/arm/dts/k3-am654-base-board.dts         | 25 ++++++++
 arch/arm/dts/k3-am654-r5-base-board.dts      | 20 +++++-
 4 files changed, 74 insertions(+), 60 deletions(-)

diff --git a/arch/arm/dts/k3-am65-main.dtsi b/arch/arm/dts/k3-am65-main.dtsi
index 028f57379b..d151e27028 100644
--- a/arch/arm/dts/k3-am65-main.dtsi
+++ b/arch/arm/dts/k3-am65-main.dtsi
@@ -113,6 +113,28 @@
 		dma-coherent;
 	};
 
+	sdhci1: sdhci at 4fa0000 {
+		compatible = "ti,am654-sdhci-5.1";
+		reg = <0x0 0x4fa0000 0x0 0x260>, <0x0 0x4fb0000 0x0 0x134>;
+		power-domains = <&k3_pds 48 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 48 0>, <&k3_clks 48 1>;
+		clock-names = "clk_ahb", "clk_xin";
+		interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
+		ti,otap-del-sel-legacy = <0x0>;
+		ti,otap-del-sel-mmc-hs = <0x0>;
+		ti,otap-del-sel-sd-hs = <0x0>;
+		ti,otap-del-sel-sdr12 = <0x0>;
+		ti,otap-del-sel-sdr25 = <0x0>;
+		ti,otap-del-sel-sdr50 = <0x8>;
+		ti,otap-del-sel-sdr104 = <0x7>;
+		ti,otap-del-sel-ddr50 = <0x4>;
+		ti,otap-del-sel-ddr52 = <0x4>;
+		ti,otap-del-sel-hs200 = <0x7>;
+		ti,clkbuf-sel = <0x7>;
+		ti,trm-icp = <0x8>;
+		dma-coherent;
+	};
+
 	main_i2c0: i2c at 2000000 {
 		compatible = "ti,am654-i2c", "ti,omap4-i2c";
 		reg = <0x0 0x2000000 0x0 0x100>;
diff --git a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
index d75d1b1c28..88fab99698 100644
--- a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
+++ b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
@@ -19,28 +19,6 @@
 
 &cbass_main{
 	u-boot,dm-spl;
-
-	sdhci1: sdhci at 04FA0000 {
-		compatible = "ti,am654-sdhci-5.1";
-		reg = <0x0 0x4FA0000 0x0 0x1000>,
-		      <0x0 0x4FB0000 0x0 0x400>;
-		clocks =<&k3_clks 48 0>, <&k3_clks 48 1>;
-		clock-names = "clk_ahb", "clk_xin";
-		power-domains = <&k3_pds 48 TI_SCI_PD_EXCLUSIVE>;
-		max-frequency = <25000000>;
-		ti,otap-del-sel-legacy = <0x0>;
-		ti,otap-del-sel-mmc-hs = <0x0>;
-		ti,otap-del-sel-sd-hs = <0x0>;
-		ti,otap-del-sel-sdr12 = <0x0>;
-		ti,otap-del-sel-sdr25 = <0x0>;
-		ti,otap-del-sel-sdr50 = <0x8>;
-		ti,otap-del-sel-sdr104 = <0x7>;
-		ti,otap-del-sel-ddr50 = <0x4>;
-		ti,otap-del-sel-ddr52 = <0x4>;
-		ti,otap-del-sel-hs200 = <0x7>;
-		ti,trm-icp = <0x8>;
-	};
-
 };
 
 &cbass_mcu {
@@ -107,38 +85,6 @@
 		u-boot,dm-spl;
 	};
 
-	main_mmc0_pins_default: main_mmc0_pins_default {
-		pinctrl-single,pins = <
-			AM65X_IOPAD(0x01a8, PIN_INPUT_PULLDOWN, 0)	/* (B25) MMC0_CLK */
-			AM65X_IOPAD(0x01aC, PIN_INPUT_PULLUP, 0)	/* (B27) MMC0_CMD */
-			AM65X_IOPAD(0x01a4, PIN_INPUT_PULLUP, 0)	/* (A26) MMC0_DAT0 */
-			AM65X_IOPAD(0x01a0, PIN_INPUT_PULLUP, 0)	/* (E25) MMC0_DAT1 */
-			AM65X_IOPAD(0x019c, PIN_INPUT_PULLUP, 0)	/* (C26) MMC0_DAT2 */
-			AM65X_IOPAD(0x0198, PIN_INPUT_PULLUP, 0)	/* (A25) MMC0_DAT3 */
-			AM65X_IOPAD(0x0194, PIN_INPUT_PULLUP, 0)	/* (E24) MMC0_DAT4 */
-			AM65X_IOPAD(0x0190, PIN_INPUT_PULLUP, 0)	/* (A24) MMC0_DAT5 */
-			AM65X_IOPAD(0x018c, PIN_INPUT_PULLUP, 0)	/* (B26) MMC0_DAT6 */
-			AM65X_IOPAD(0x0188, PIN_INPUT_PULLUP, 0)	/* (D25) MMC0_DAT7 */
-			AM65X_IOPAD(0x01b4, PIN_INPUT_PULLUP, 0)	/* (A23) MMC0_SDCD */
-			AM65X_IOPAD(0x01b0, PIN_INPUT, 0)		/* (C25) MMC0_DS */
-		>;
-		u-boot,dm-spl;
-	};
-
-	main_mmc1_pins_default: main_mmc1_pins_default {
-		pinctrl-single,pins = <
-			AM65X_IOPAD(0x02d4, PIN_INPUT_PULLDOWN, 0)	/* (C27) MMC1_CLK */
-			AM65X_IOPAD(0x02d8, PIN_INPUT_PULLUP, 0)	/* (C28) MMC1_CMD */
-			AM65X_IOPAD(0x02d0, PIN_INPUT_PULLUP, 0)	/* (D28) MMC1_DAT0 */
-			AM65X_IOPAD(0x02cc, PIN_INPUT_PULLUP, 0)	/* (E27) MMC1_DAT1 */
-			AM65X_IOPAD(0x02c8, PIN_INPUT_PULLUP, 0)	/* (D26) MMC1_DAT2 */
-			AM65X_IOPAD(0x02c4, PIN_INPUT_PULLUP, 0)	/* (D27) MMC1_DAT3 */
-			AM65X_IOPAD(0x02dc, PIN_INPUT_PULLUP, 0)	/* (B24) MMC1_SDCD */
-			AM65X_IOPAD(0x02e0, PIN_INPUT, 0)			/* (C24) MMC1_SDWP */
-		>;
-		u-boot,dm-spl;
-	};
-
 	usb0_pins_default: usb0_pins_default {
 		pinctrl-single,pins = <
 			AM65X_IOPAD(0x02bc, PIN_OUTPUT, 0) /* (AD9) USB0_DRVVBUS */
@@ -188,17 +134,20 @@
 	status = "okay";
 };
 
+&main_mmc0_pins_default {
+	u-boot,dm-spl;
+};
+
+&main_mmc1_pins_default {
+	u-boot,dm-spl;
+};
+
 &sdhci0 {
 	u-boot,dm-spl;
 };
 
 &sdhci1 {
 	u-boot,dm-spl;
-	status = "okay";
-	pinctrl-names = "default";
-	pinctrl-0 = <&main_mmc1_pins_default>;
-	sdhci-caps-mask = <0x7 0x0>;
-	ti,driver-strength-ohm = <50>;
 };
 
 &mcu_cpsw {
diff --git a/arch/arm/dts/k3-am654-base-board.dts b/arch/arm/dts/k3-am654-base-board.dts
index 3ebf4af5e4..33a1b9fdc4 100644
--- a/arch/arm/dts/k3-am654-base-board.dts
+++ b/arch/arm/dts/k3-am654-base-board.dts
@@ -59,6 +59,19 @@
 		>;
 	};
 
+	main_mmc1_pins_default: main_mmc1_pins_default {
+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x02d4, PIN_INPUT_PULLDOWN, 0)	/* (C27) MMC1_CLK */
+			AM65X_IOPAD(0x02d8, PIN_INPUT_PULLUP, 0)	/* (C28) MMC1_CMD */
+			AM65X_IOPAD(0x02d0, PIN_INPUT_PULLUP, 0)	/* (D28) MMC1_DAT0 */
+			AM65X_IOPAD(0x02cc, PIN_INPUT_PULLUP, 0)	/* (E27) MMC1_DAT1 */
+			AM65X_IOPAD(0x02c8, PIN_INPUT_PULLUP, 0)	/* (D26) MMC1_DAT2 */
+			AM65X_IOPAD(0x02c4, PIN_INPUT_PULLUP, 0)	/* (D27) MMC1_DAT3 */
+			AM65X_IOPAD(0x02dc, PIN_INPUT_PULLUP, 0)	/* (B24) MMC1_SDCD */
+			AM65X_IOPAD(0x02e0, PIN_INPUT, 0)		/* (C24) MMC1_SDWP */
+		>;
+	};
+
 	usb1_pins_default: usb1_pins_default {
 		pinctrl-single,pins = <
 			AM65X_IOPAD(0x02c0, PIN_OUTPUT, 0) /* (AC8) USB1_DRVVBUS */
@@ -122,6 +135,18 @@
 	ti,driver-strength-ohm = <50>;
 };
 
+/*
+ * Because of erratas i2025 and i2026 for silicon revision 1.0, the
+ * SD card interface might fail. Boards with sr1.0 are recommended to
+ * disable sdhci1
+ */
+&sdhci1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&main_mmc1_pins_default>;
+	ti,driver-strength-ohm = <50>;
+	disable-wp;
+};
+
 &wkup_i2c0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&wkup_i2c0_pins_default>;
diff --git a/arch/arm/dts/k3-am654-r5-base-board.dts b/arch/arm/dts/k3-am654-r5-base-board.dts
index d43a4edc71..a214fb8247 100644
--- a/arch/arm/dts/k3-am654-r5-base-board.dts
+++ b/arch/arm/dts/k3-am654-r5-base-board.dts
@@ -6,7 +6,6 @@
 /dts-v1/;
 
 #include "k3-am654.dtsi"
-#include "k3-am654-base-board-u-boot.dtsi"
 #include "k3-am654-base-board-ddr4-1600MTs.dtsi"
 #include "k3-am654-ddr.dtsi"
 
@@ -213,6 +212,21 @@
 			AM65X_IOPAD(0x0188, PIN_INPUT_PULLUP, 0)	/* (D25) MMC0_DAT7 */
 			AM65X_IOPAD(0x01b0, PIN_INPUT, 0)		/* (C25) MMC0_DS */
 		>;
+		u-boot,dm-spl;
+	};
+
+	main_mmc1_pins_default: main_mmc1_pins_default {
+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x02d4, PIN_INPUT_PULLDOWN, 0)	/* (C27) MMC1_CLK */
+			AM65X_IOPAD(0x02d8, PIN_INPUT_PULLUP, 0)	/* (C28) MMC1_CMD */
+			AM65X_IOPAD(0x02d0, PIN_INPUT_PULLUP, 0)	/* (D28) MMC1_DAT0 */
+			AM65X_IOPAD(0x02cc, PIN_INPUT_PULLUP, 0)	/* (E27) MMC1_DAT1 */
+			AM65X_IOPAD(0x02c8, PIN_INPUT_PULLUP, 0)	/* (D26) MMC1_DAT2 */
+			AM65X_IOPAD(0x02c4, PIN_INPUT_PULLUP, 0)	/* (D27) MMC1_DAT3 */
+			AM65X_IOPAD(0x02dc, PIN_INPUT_PULLUP, 0)	/* (B24) MMC1_SDCD */
+			AM65X_IOPAD(0x02e0, PIN_INPUT, 0)		/* (C24) MMC1_SDWP */
+		>;
+		u-boot,dm-spl;
 	};
 };
 
@@ -225,6 +239,7 @@
 &sdhci0 {
 	clock-names = "clk_xin";
 	clocks = <&clk_200mhz>;
+	pinctrl-0 = <&main_mmc0_pins_default>;
 	/delete-property/ power-domains;
 	ti,driver-strength-ohm = <50>;
 };
@@ -232,6 +247,7 @@
 &sdhci1 {
 	clock-names = "clk_xin";
 	clocks = <&clk_200mhz>;
+	pinctrl-0 = <&main_mmc1_pins_default>;
 	/delete-property/ power-domains;
 	ti,driver-strength-ohm = <50>;
 };
@@ -313,3 +329,5 @@
 &scm_conf {
 	u-boot,dm-spl;
 };
+
+#include "k3-am654-base-board-u-boot.dtsi"
-- 
2.17.1

[PATCH v2 12/21] arm: dts: k3-j721e-main: Update otap-delay values

[Faiz Abbas]

Update otap delay values to match with the latest Data Manual[1].

[1] https://www.ti.com/lit/gpn/dra829v

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-j721e-main.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/dts/k3-j721e-main.dtsi b/arch/arm/dts/k3-j721e-main.dtsi
index 33db74a267..e08e743706 100644
--- a/arch/arm/dts/k3-j721e-main.dtsi
+++ b/arch/arm/dts/k3-j721e-main.dtsi
@@ -235,11 +235,14 @@
 		ti,trm-icp = <0x8>;
 		dma-coherent;
 		mmc-ddr-1_8v;
-		ti,otap-del-sel-legacy = <0x0>;
-		ti,otap-del-sel-mmc-hs = <0x0>;
+		ti,otap-del-sel-legacy = <0xf>;
+		ti,otap-del-sel-mmc-hs = <0xf>;
 		ti,otap-del-sel-ddr52 = <0x5>;
 		ti,otap-del-sel-hs200 = <0x6>;
 		ti,otap-del-sel-hs400 = <0x0>;
+		ti,itap-del-sel-legacy = <0x10>;
+		ti,itap-del-sel-mmc-hs = <0xa>;
+		ti,itap-del-sel-ddr52 = <0x3>;
 	};
 
 	main_sdhci1: sdhci at 4fb0000 {
@@ -256,7 +259,6 @@
 		ti,otap-del-sel-sdr12 = <0xf>;
 		ti,otap-del-sel-sdr25 = <0xf>;
 		ti,otap-del-sel-sdr50 = <0xc>;
-		ti,otap-del-sel-sdr104 = <0x5>;
 		ti,otap-del-sel-ddr50 = <0xc>;
 		ti,trm-icp = <0x8>;
 		dma-coherent;
-- 
2.17.1

[PATCH v2 13/21] arm: dts: k3-j721e-common-proc-board: Add support for UHS modes for SD card

[Faiz Abbas]

Add support for regulators to power cycle and switch IO voltage to the
SD card. This enables support for UHS modes.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-j721e-common-proc-board.dts | 32 +++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/dts/k3-j721e-common-proc-board.dts b/arch/arm/dts/k3-j721e-common-proc-board.dts
index 496a15e1d1..6b29200bba 100644
--- a/arch/arm/dts/k3-j721e-common-proc-board.dts
+++ b/arch/arm/dts/k3-j721e-common-proc-board.dts
@@ -6,6 +6,7 @@
 /dts-v1/;
 
 #include "k3-j721e-som-p0.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	chosen {
@@ -24,6 +25,29 @@
 		remoteproc7 = &c66_1;
 		remoteproc8 = &c71_0;
 	};
+
+	vdd_mmc1: fixedregulator-sd {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd_mmc1";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		enable-active-high;
+		gpio = <&exp2 2 GPIO_ACTIVE_HIGH>;
+	};
+
+	vdd_sd_dv_alt: gpio-regulator-TLV71033 {
+		compatible = "regulator-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&vdd_sd_dv_alt_pins_default>;
+		regulator-name = "tlv71033";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		gpios = <&main_gpio0 117 GPIO_ACTIVE_HIGH>;
+		states = <1800000 0x0
+			  3300000 0x1>;
+	};
 };
 
 &wkup_uart0 {
@@ -79,6 +103,12 @@
 			J721E_IOPAD(0x25c, PIN_INPUT, 0) /* (R28) MMC1_SDWP */
 		>;
 	};
+
+	vdd_sd_dv_alt_pins_default: vdd_sd_dv_alt_pins_default {
+		pinctrl-single,pins = <
+			J721E_IOPAD(0x1d8, PIN_INPUT, 7) /* (W4) SPI1_CS1.GPIO0_117 */
+		>;
+	};
 };
 
 &main_sdhci0 {
@@ -92,6 +122,8 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&main_mmc1_pins_default>;
 	ti,driver-strength-ohm = <50>;
+	vmmc-supply = <&vdd_mmc1>;
+	vqmmc-supply = <&vdd_sd_dv_alt>;
 };
 
 &main_pmx0 {
-- 
2.17.1

[PATCH v2 14/21] arm: dts: k3-j7200-main: Add support for gpio0

[Faiz Abbas]

Add support for the main_gpio0 node

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-j7200-main.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/dts/k3-j7200-main.dtsi b/arch/arm/dts/k3-j7200-main.dtsi
index c25f03cf23..b722204c44 100644
--- a/arch/arm/dts/k3-j7200-main.dtsi
+++ b/arch/arm/dts/k3-j7200-main.dtsi
@@ -197,6 +197,28 @@
 		clock-names = "fclk";
 	};
 
+	main_gpio0: gpio at 600000 {
+		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
+		reg = <0x0 0x00600000 0x0 0x100>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupts = <105 0 IRQ_TYPE_EDGE_RISING>,
+			     <105 1 IRQ_TYPE_EDGE_RISING>,
+			     <105 2 IRQ_TYPE_EDGE_RISING>,
+			     <105 3 IRQ_TYPE_EDGE_RISING>,
+			     <105 4 IRQ_TYPE_EDGE_RISING>,
+			     <105 5 IRQ_TYPE_EDGE_RISING>,
+			     <105 6 IRQ_TYPE_EDGE_RISING>,
+			     <105 7 IRQ_TYPE_EDGE_RISING>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		ti,ngpio = <69>;
+		ti,davinci-gpio-unbanked = <0>;
+		power-domains = <&k3_pds 105 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 105 0>;
+		clock-names = "gpio";
+	};
+
 	main_sdhci0: sdhci at 4f80000 {
 		compatible = "ti,j721e-sdhci-8bit";
 		reg = <0x0 0x04f80000 0x0 0x260>, <0x0 0x4f88000 0x0 0x134>;
-- 
2.17.1

[PATCH v2 15/21] arm: dts: k3-j7200-common-proc-board: Enable support for UHS modes

[Faiz Abbas]

Add support for UHS modes by adding the regulators to power cycle
and voltage switch the card. Also add pinmuxes required for each
node

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-j7200-common-proc-board.dts   | 49 ++++++++++++++++++-
 arch/arm/dts/k3-j7200-main.dtsi               |  1 +
 .../arm/dts/k3-j7200-r5-common-proc-board.dts | 15 ++++++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/k3-j7200-common-proc-board.dts b/arch/arm/dts/k3-j7200-common-proc-board.dts
index cc3d933cbb..20974aff59 100644
--- a/arch/arm/dts/k3-j7200-common-proc-board.dts
+++ b/arch/arm/dts/k3-j7200-common-proc-board.dts
@@ -7,6 +7,7 @@
 
 #include <dt-bindings/net/ti-dp83867.h>
 #include "k3-j7200-som-p0.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	chosen {
@@ -20,6 +21,29 @@
 		remoteproc2 = &main_r5fss0_core0;
 		remoteproc3 = &main_r5fss0_core1;
 	};
+
+	vdd_mmc1: fixedregulator-sd {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd_mmc1";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		enable-active-high;
+		gpio = <&exp2 2 GPIO_ACTIVE_HIGH>;
+	};
+
+	vdd_sd_dv: gpio-regulator-vdd-sd-dv {
+		compatible = "regulator-gpio";
+		regulator-name = "vdd_sd_dv";
+		pinctrl-names = "default";
+		pinctrl-0 = <&vdd_sd_dv_pins_default>;
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		gpios = <&main_gpio0 55 GPIO_ACTIVE_HIGH>;
+		states = <1800000 0x0
+			  3300000 0x1>;
+	};
 };
 
 &wkup_pmx0 {
@@ -69,6 +93,25 @@
 		>;
 	};
 
+	main_mmc1_pins_default: main_mmc1_pins_default {
+		pinctrl-single,pins = <
+			J721E_IOPAD(0x104, PIN_INPUT, 0) /* (M20) MMC1_CMD */
+			J721E_IOPAD(0x100, PIN_INPUT, 0) /* (P21) MMC1_CLK */
+			J721E_IOPAD(0xfc, PIN_INPUT, 0) /* (P25) MMC1_CLKLB */
+			J721E_IOPAD(0xf8, PIN_INPUT, 0) /* (M19) MMC1_DAT0 */
+			J721E_IOPAD(0xf4, PIN_INPUT, 0) /* (N21) MMC1_DAT1 */
+			J721E_IOPAD(0xf0, PIN_INPUT, 0) /* (N20) MMC1_DAT2 */
+			J721E_IOPAD(0xec, PIN_INPUT, 0) /* (N19) MMC1_DAT3 */
+			J721E_IOPAD(0xe4, PIN_INPUT, 8) /* (V1) TIMER_IO0.MMC1_SDCD */
+		>;
+	};
+
+	vdd_sd_dv_pins_default: vdd_sd_dv_pins_default {
+		pinctrl-single,pins = <
+			J721E_IOPAD(0xd0, PIN_INPUT, 7) /* (T5) SPI0_D1.GPIO0_55 */
+		>;
+	};
+
 	main_usbss0_pins_default: main_usbss0_pins_default {
 		pinctrl-single,pins = <
 			J721E_IOPAD(0x120, PIN_OUTPUT, 0) /* (T4) USB0_DRVVBUS */
@@ -140,10 +183,12 @@
 
 &main_sdhci1 {
 	/* SD card */
+	pinctrl-0 = <&main_mmc1_pins_default>;
+	pinctrl-names = "default";
+	vmmc-supply = <&vdd_mmc1>;
+	vqmmc-supply = <&vdd_sd_dv>;
 	ti,driver-strength-ohm = <50>;
 	disable-wp;
-	no-1-8-v;
-	sdhci-caps-mask = <0x8000000F 0x0>;
 };
 
 &main_i2c0 {
diff --git a/arch/arm/dts/k3-j7200-main.dtsi b/arch/arm/dts/k3-j7200-main.dtsi
index b722204c44..8150ff6332 100644
--- a/arch/arm/dts/k3-j7200-main.dtsi
+++ b/arch/arm/dts/k3-j7200-main.dtsi
@@ -253,6 +253,7 @@
 		ti,otap-del-sel-sdr50 = <0xc>;
 		ti,otap-del-sel-sdr104 = <0x5>;
 		ti,otap-del-sel-ddr50 = <0xc>;
+		ti,clkbuf-sel = <0x7>;
 		dma-coherent;
 	};
 
diff --git a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
index db63d93777..288f4bf565 100644
--- a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
+++ b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
@@ -161,6 +161,19 @@
 		>;
 	};
 
+	main_mmc1_pins_default: main_mmc1_pins_default {
+		pinctrl-single,pins = <
+			J721E_IOPAD(0x104, PIN_INPUT, 0) /* (M20) MMC1_CMD */
+			J721E_IOPAD(0x100, PIN_INPUT, 0) /* (P21) MMC1_CLK */
+			J721E_IOPAD(0xfc, PIN_INPUT, 0) /* (P25) MMC1_CLKLB */
+			J721E_IOPAD(0xf8, PIN_INPUT, 0) /* (M19) MMC1_DAT0 */
+			J721E_IOPAD(0xf4, PIN_INPUT, 0) /* (N21) MMC1_DAT1 */
+			J721E_IOPAD(0xf0, PIN_INPUT, 0) /* (N20) MMC1_DAT2 */
+			J721E_IOPAD(0xec, PIN_INPUT, 0) /* (N19) MMC1_DAT3 */
+			J721E_IOPAD(0xe4, PIN_INPUT, 8) /* (V1) TIMER_IO0.MMC1_SDCD */
+		>;
+	};
+
 	main_usbss0_pins_default: main_usbss0_pins_default {
 		pinctrl-single,pins = <
 			J721E_IOPAD(0x120, PIN_OUTPUT, 0) /* (T4) USB0_DRVVBUS */
@@ -197,6 +210,8 @@
 	/delete-property/ power-domains;
 	/delete-property/ assigned-clocks;
 	/delete-property/ assigned-clock-parents;
+	pinctrl-0 = <&main_mmc1_pins_default>;
+	pinctrl-names = "default";
 	clock-names = "clk_xin";
 	clocks = <&clk_200mhz>;
 	ti,driver-strength-ohm = <50>;
-- 
2.17.1

[PATCH v2 16/21] configs: j721e_evm: Add support for UHS modes

[Faiz Abbas]

Add configs to support UHS modes for the SD card and HS200 for the eMMC.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 configs/j721e_evm_a72_defconfig | 8 ++++++++
 configs/j721e_evm_r5_defconfig  | 1 +
 2 files changed, 9 insertions(+)

diff --git a/configs/j721e_evm_a72_defconfig b/configs/j721e_evm_a72_defconfig
index 6de8666956..3991100832 100644
--- a/configs/j721e_evm_a72_defconfig
+++ b/configs/j721e_evm_a72_defconfig
@@ -107,11 +107,16 @@ CONFIG_TI_SCI_PROTOCOL=y
 CONFIG_DA8XX_GPIO=y
 CONFIG_DM_PCA953X=y
 CONFIG_DM_I2C=y
+CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_MMC_IO_VOLTAGE=y
+CONFIG_MMC_UHS_SUPPORT=y
+CONFIG_MMC_HS200_SUPPORT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_ADMA=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
@@ -140,6 +145,9 @@ CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_SINGLE=y
 CONFIG_POWER_DOMAIN=y
 CONFIG_TI_SCI_POWER_DOMAIN=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_FIXED=y
+CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_RAM=y
 CONFIG_SPL_RAM=y
 CONFIG_REMOTEPROC_TI_K3_DSP=y
diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
index 4128548100..7b4f0afce6 100644
--- a/configs/j721e_evm_r5_defconfig
+++ b/configs/j721e_evm_r5_defconfig
@@ -89,6 +89,7 @@ CONFIG_K3_AVS0=y
 CONFIG_ESM_PMIC=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
 CONFIG_MMC_SDHCI_AM654=y
-- 
2.17.1

[PATCH v2 17/21] configs: j7200_evm: Add support for UHS modes

[Faiz Abbas]

Add configs to support UHS modes for the SD card and HS200 for the eMMC.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 configs/j7200_evm_a72_defconfig | 8 ++++++++
 configs/j7200_evm_r5_defconfig  | 1 +
 2 files changed, 9 insertions(+)

diff --git a/configs/j7200_evm_a72_defconfig b/configs/j7200_evm_a72_defconfig
index 7c900b1d2e..c932500dc4 100644
--- a/configs/j7200_evm_a72_defconfig
+++ b/configs/j7200_evm_a72_defconfig
@@ -110,11 +110,16 @@ CONFIG_TI_SCI_PROTOCOL=y
 CONFIG_DA8XX_GPIO=y
 CONFIG_DM_PCA953X=y
 CONFIG_DM_I2C=y
+CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_MMC_IO_VOLTAGE=y
+CONFIG_MMC_UHS_SUPPORT=y
+CONFIG_MMC_HS200_SUPPORT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_ADMA=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
@@ -142,6 +147,9 @@ CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_SINGLE=y
 CONFIG_POWER_DOMAIN=y
 CONFIG_TI_SCI_POWER_DOMAIN=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_FIXED=y
+CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_RAM=y
 CONFIG_SPL_RAM=y
 CONFIG_REMOTEPROC_TI_K3_R5F=y
diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
index 3820fc508b..b20698fd0e 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -89,6 +89,7 @@ CONFIG_K3_SEC_PROXY=y
 CONFIG_FS_LOADER=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
 CONFIG_MMC_SDHCI_AM654=y
-- 
2.17.1

[PATCH v2 18/21] i2c: Makefile: Add SPL_DM_I2C_GPIO

[Faiz Abbas]

Add an SPL flag to DM_I2C_GPIO to prevent it building
automatically in SPL.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/i2c/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index bd248cbf52..b37198036c 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_DM_I2C) += i2c-uclass.o
 ifdef CONFIG_ACPIGEN
 obj-$(CONFIG_DM_I2C) += acpi_i2c.o
 endif
-obj-$(CONFIG_DM_I2C_GPIO) += i2c-gpio.o
+obj-$(CONFIG_$(SPL_)DM_I2C_GPIO) += i2c-gpio.o
 obj-$(CONFIG_$(SPL_)I2C_CROS_EC_TUNNEL) += cros_ec_tunnel.o
 obj-$(CONFIG_$(SPL_)I2C_CROS_EC_LDO) += cros_ec_ldo.o
 
-- 
2.17.1

[PATCH v2 19/21] arm: dts: k3-am65-main: Add itapdly and clkbuf-sel values

[Faiz Abbas]

Add the appropriate itapdly and clkbuf-sel values required for
some lower speed modes.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-am65-main.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/dts/k3-am65-main.dtsi b/arch/arm/dts/k3-am65-main.dtsi
index d151e27028..a7b03dc669 100644
--- a/arch/arm/dts/k3-am65-main.dtsi
+++ b/arch/arm/dts/k3-am65-main.dtsi
@@ -109,6 +109,11 @@
 		ti,otap-del-sel-ddr52 = <0x5>;
 		ti,otap-del-sel-hs200 = <0x5>;
 		ti,otap-del-sel-hs400 = <0x0>;
+		ti,itap-del-sel-legacy = <0xa>;
+		ti,itap-del-sel-mmc-hs = <0x1>;
+		ti,itap-del-sel-sdr12 = <0xa>;
+		ti,itap-del-sel-sdr25 = <0x1>;
+		ti,clkbuf-sel = <0x7>;
 		ti,trm-icp = <0x8>;
 		dma-coherent;
 	};
@@ -130,6 +135,10 @@
 		ti,otap-del-sel-ddr50 = <0x4>;
 		ti,otap-del-sel-ddr52 = <0x4>;
 		ti,otap-del-sel-hs200 = <0x7>;
+		ti,itap-del-sel-legacy = <0xa>;
+		ti,itap-del-sel-mmc-hs = <0x1>;
+		ti,itap-del-sel-sdr12 = <0xa>;
+		ti,itap-del-sel-sdr25 = <0x1>;
 		ti,clkbuf-sel = <0x7>;
 		ti,trm-icp = <0x8>;
 		dma-coherent;
-- 
2.17.1

[PATCH v2 20/21] arm: dts: k3-am654-base-board: Limit Sd card to High speed modes

[Faiz Abbas]

There's an issue with the base board in which the power cycle
circuit takes way longer to power down than expected by mmc core.
code. This prevents the card from enumerating in UHS modes.

Disable UHS modes for this board until a new board revision fixes
the issue.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm/dts/k3-am654-base-board.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/k3-am654-base-board.dts b/arch/arm/dts/k3-am654-base-board.dts
index 33a1b9fdc4..830526a1e4 100644
--- a/arch/arm/dts/k3-am654-base-board.dts
+++ b/arch/arm/dts/k3-am654-base-board.dts
@@ -144,6 +144,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&main_mmc1_pins_default>;
 	ti,driver-strength-ohm = <50>;
+	sdhci-caps-mask = <0x7 0x0>;
 	disable-wp;
 };
 
-- 
2.17.1

[PATCH v2 21/21] configs: am65x_evm: Add configs for UHS modes

[Faiz Abbas]

Add configs for voltage switching and UHS modes for the SD card
and HS200 for the eMMC.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 configs/am65x_evm_a53_defconfig | 8 ++++++++
 configs/am65x_evm_r5_defconfig  | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
index 8a94ad1530..fd7eaef58b 100644
--- a/configs/am65x_evm_a53_defconfig
+++ b/configs/am65x_evm_a53_defconfig
@@ -95,12 +95,17 @@ CONFIG_TI_SCI_PROTOCOL=y
 CONFIG_DM_PCA953X=y
 CONFIG_DM_I2C=y
 CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
+CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_KEYBOARD=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_MMC_IO_VOLTAGE=y
+CONFIG_MMC_UHS_SUPPORT=y
+CONFIG_MMC_HS200_SUPPORT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_ADMA=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
@@ -132,6 +137,9 @@ CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_SINGLE=y
 CONFIG_POWER_DOMAIN=y
 CONFIG_TI_SCI_POWER_DOMAIN=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_FIXED=y
+CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_REMOTEPROC_TI_K3_R5F=y
 CONFIG_DM_RESET=y
 CONFIG_RESET_TI_SCI=y
diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index 96c4351196..b9b11010a1 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -85,6 +85,8 @@ CONFIG_K3_SEC_PROXY=y
 CONFIG_K3_AVS0=y
 CONFIG_DM_MMC=y
 CONFIG_SUPPORT_EMMC_BOOT=y
+CONFIG_MMC_HS200_SUPPORT=y
+CONFIG_SPL_MMC_HS200_SUPPORT=y
 CONFIG_MMC_SDHCI=y
 CONFIG_SPL_MMC_SDHCI_ADMA=y
 CONFIG_MMC_SDHCI_AM654=y
-- 
2.17.1

[PATCH v2 00/21] Add support for MMC higher speed modes for TI's am65x, j721e and j7200 platforms

[Peng Fan]

> Subject: [PATCH v2 00/21] Add support for MMC higher speed modes for TI's
> am65x, j721e and j7200 platforms

For the mmc driver part, 

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> The following patches add support for higher speeds in the SD card and eMMC
> for TI's am65x, j721e, j7200 platforms.
> 
> With these patches, the following max speeds are supported:
> j721e: DDR50, HS200
> j7200: SDR104, HS200
> am65x: SDR104*, HS200
> 
> v2:
> 1. Added patches to support UHS modes for the SD card even in am654x
> platforms.
> 2. Fixed an issue with patch 1 that was breaking builds on some platforms.
> 
> * There's an issue with the am65x base board such that the power cycle
> circuit to the card takes way longer than the wait time in mmc core.
> Until this is fixed, am654x-evm and -idk will only support High speed mode at
> 3.3V (see patch 20) but this shouldn't block us from adding UHS modes in the
> dtsi as well as in the configs so other boards can still take advantage of the
> higher speed. UHS modes have been tested by adding the appropriate delay in
> the power cycle circuit.
> 
> Link to v1:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fproject%2Fuboot%2Flist%2F%3Fseries%3D206622&amp;
> data=04%7C01%7Cpeng.fan%40nxp.com%7C60aff07a45894e36209308d871c
> 3e441%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6373844334
> 69526801%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bRq92%2
> FV4JzEhNipqsw5%2BqsgTOfJPqsmaSe3%2BkmnU3%2BU%3D&amp;reserved
> =0
> 
> Faiz Abbas (21):
>   mmc: sdhci: Add helper functions for UHS modes
>   mmc: am654_sdhci: Unconditionally switch off DLL in the beginning of
>     ios_post()
>   mmc: am654_sdhci: Convert flag fields to BIT macro
>   mmc: am654_sdhci: Add flag for PHY calibration
>   mmc: am654_sdhci: Add support for AM65x SR2.0
>   mmc: am654_sdhci: Add support for input tap delay
>   mmc: am654_sdhci: Add support for writing to clkbuf_sel
>   mmc: am654_sdhci: Add support for software tuning
>   mmc: am654_sdhci: Fix HISPD bit configuration in some lower speed
>     modes
>   mmc: am654_sdhci: Use sdhci_set_control_reg()
>   arm: dts: k3-am65: Fix mmc nodes
>   arm: dts: k3-j721e-main: Update otap-delay values
>   arm: dts: k3-j721e-common-proc-board: Add support for UHS modes for SD
>     card
>   arm: dts: k3-j7200-main: Add support for gpio0
>   arm: dts: k3-j7200-common-proc-board: Enable support for UHS modes
>   configs: j721e_evm: Add support for UHS modes
>   configs: j7200_evm: Add support for UHS modes
>   i2c: Makefile: Add SPL_DM_I2C_GPIO
>   arm: dts: k3-am65-main: Add itapdly and clkbuf-sel values
>   arm: dts: k3-am654-base-board: Limit Sd card to High speed modes
>   configs: am65x_evm: Add configs for UHS modes
> 
>  arch/arm/dts/k3-am65-main.dtsi                |  31 ++
>  arch/arm/dts/k3-am654-base-board-u-boot.dtsi  |  67 +--
>  arch/arm/dts/k3-am654-base-board.dts          |  26 ++
>  arch/arm/dts/k3-am654-r5-base-board.dts       |  20 +-
>  arch/arm/dts/k3-j7200-common-proc-board.dts   |  49 ++-
>  arch/arm/dts/k3-j7200-main.dtsi               |  23 ++
>  .../arm/dts/k3-j7200-r5-common-proc-board.dts |  15 +
>  arch/arm/dts/k3-j721e-common-proc-board.dts   |  32 ++
>  arch/arm/dts/k3-j721e-main.dtsi               |   8 +-
>  configs/am65x_evm_a53_defconfig               |   8 +
>  configs/am65x_evm_r5_defconfig                |   2 +
>  configs/j7200_evm_a72_defconfig               |   8 +
>  configs/j7200_evm_r5_defconfig                |   1 +
>  configs/j721e_evm_a72_defconfig               |   8 +
>  configs/j721e_evm_r5_defconfig                |   1 +
>  drivers/i2c/Makefile                          |   2 +-
>  drivers/mmc/Kconfig                           |   1 +
>  drivers/mmc/am654_sdhci.c                     | 384
> +++++++++++++-----
>  drivers/mmc/sdhci.c                           |  51 +++
>  include/sdhci.h                               |   1 +
>  20 files changed, 564 insertions(+), 174 deletions(-)
> 
> --
> 2.17.1

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Jaehoon Chung]

Hi Faiz,

On 10/16/20 8:08 PM, Faiz Abbas wrote:
> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
> for SD card UHS modes.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>  include/sdhci.h     |  1 +
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 7673219fb3..a69f058191 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -20,6 +20,7 @@
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <phys2bus.h>
> +#include <power/regulator.h>
>  
>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>  {
> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>  }
>  
> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
> +static void sdhci_set_voltage(struct sdhci_host *host)
> +{
> +	struct mmc *mmc = (struct mmc *)host->mmc;
> +	u32 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +
> +	switch (mmc->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_330:
> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
> +		if (mmc->vqmmc_supply) {
> +			regulator_set_enable(mmc->vqmmc_supply, false);
> +			regulator_set_value(mmc->vqmmc_supply, 3300000);

Doesn't need to consider about fail to set its value?

> +			regulator_set_enable(mmc->vqmmc_supply, true);
> +		}
> +#endif
> +		mdelay(5);

For what purpose about mdelay(5)?


> +		if (IS_SD(mmc)) {
> +			ctrl &= ~SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +		}
> +		break;
> +	case MMC_SIGNAL_VOLTAGE_180:
> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
> +		if (mmc->vqmmc_supply) {
> +			regulator_set_enable(mmc->vqmmc_supply, false);
> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
> +			regulator_set_enable(mmc->vqmmc_supply, true);
> +		}
> +#endif
> +		if (IS_SD(mmc)) {
> +			ctrl |= SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +		}
> +		break;
> +	default:
> +		/* No signal voltage switch required */
> +		return;
> +	}
> +}
> +#else
> +static void sdhci_set_voltage(struct sdhci_host *host) { }
> +#endif
> +void sdhci_set_control_reg(struct sdhci_host *host)

this function is called as callback function in sdhci_set_ios(). 
it's strange... set_control_reg callback is for host specific control register.

I think that it doesn't need to assign to callback.

Best Regards,
Jaehoon Chung

> +{
> +	sdhci_set_voltage(host);
> +	sdhci_set_uhs_timing(host);
> +}
> +
>  #ifdef CONFIG_DM_MMC
>  static int sdhci_set_ios(struct udevice *dev)
>  {
> diff --git a/include/sdhci.h b/include/sdhci.h
> index 94fc3ed56a..d3f8741042 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -492,6 +492,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host);
>  /* Export the operations to drivers */
>  int sdhci_probe(struct udevice *dev);
>  int sdhci_set_clock(struct mmc *mmc, unsigned int clock);
> +void sdhci_set_control_reg(struct sdhci_host *host);
>  extern const struct dm_mmc_ops sdhci_ops;
>  #else
>  #endif
> 

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Faiz Abbas]

Jaehoon,

On 21/10/20 5:08 pm, Jaehoon Chung wrote:
> Hi Faiz,
> 
> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>> for SD card UHS modes.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/sdhci.h     |  1 +
>>  2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index 7673219fb3..a69f058191 100644
>> --- a/drivers/mmc/sdhci.c
>> +++ b/drivers/mmc/sdhci.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/dma-mapping.h>
>>  #include <phys2bus.h>
>> +#include <power/regulator.h>
>>  
>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>  {
>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>  }
>>  
>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>> +static void sdhci_set_voltage(struct sdhci_host *host)
>> +{
>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>> +	u32 ctrl;
>> +
>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +
>> +	switch (mmc->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_330:
>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>> +		if (mmc->vqmmc_supply) {
>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
> 
> Doesn't need to consider about fail to set its value?

You're right. I'll handle the failure case in v3.

> 
>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>> +		}
>> +#endif
>> +		mdelay(5);
> 
> For what purpose about mdelay(5)?

I'm following this from the kernel implementation here:
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2547

Not sure if this a part of the spec or not though.

> 
> 
>> +		if (IS_SD(mmc)) {
>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +		}
>> +		break;
>> +	case MMC_SIGNAL_VOLTAGE_180:
>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>> +		if (mmc->vqmmc_supply) {
>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>> +		}
>> +#endif
>> +		if (IS_SD(mmc)) {
>> +			ctrl |= SDHCI_CTRL_VDD_180;
>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +		}
>> +		break;
>> +	default:
>> +		/* No signal voltage switch required */
>> +		return;
>> +	}
>> +}
>> +#else
>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>> +#endif
>> +void sdhci_set_control_reg(struct sdhci_host *host)
> 
> this function is called as callback function in sdhci_set_ios(). 
> it's strange... set_control_reg callback is for host specific control register.
> 
> I think that it doesn't need to assign to callback.

This is the default set_control_reg() implementation which is defined
in the sdhci spec. Any host that that wants default implementation
case assign this as their callback.

Hosts that have custom implementations can add in their own drivers.

Thanks,
Faiz

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Jaehoon Chung]

On 11/5/20 4:05 AM, Faiz Abbas wrote:
> Jaehoon,
> 
> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>> Hi Faiz,
>>
>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>> for SD card UHS modes.
>>>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> ---
>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>  include/sdhci.h     |  1 +
>>>  2 files changed, 52 insertions(+)
>>>
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>> index 7673219fb3..a69f058191 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -20,6 +20,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <phys2bus.h>
>>> +#include <power/regulator.h>
>>>  
>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>  {
>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>  }
>>>  
>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>> +{
>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>> +	u32 ctrl;
>>> +
>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> +
>>> +	switch (mmc->signal_voltage) {
>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>> +		if (mmc->vqmmc_supply) {
>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>
>> Doesn't need to consider about fail to set its value?
> 
> You're right. I'll handle the failure case in v3.
> 
>>
>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>> +		}
>>> +#endif
>>> +		mdelay(5);
>>
>> For what purpose about mdelay(5)?
> 
> I'm following this from the kernel implementation here:
> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
> 
> Not sure if this a part of the spec or not though.

Thanks for sharing info. :)

> 
>>
>>
>>> +		if (IS_SD(mmc)) {
>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +		}
>>> +		break;
>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>> +		if (mmc->vqmmc_supply) {
>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>> +		}
>>> +#endif
>>> +		if (IS_SD(mmc)) {
>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +		}
>>> +		break;
>>> +	default:
>>> +		/* No signal voltage switch required */
>>> +		return;
>>> +	}
>>> +}
>>> +#else
>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>> +#endif
>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>
>> this function is called as callback function in sdhci_set_ios(). 
>> it's strange... set_control_reg callback is for host specific control register.
>>
>> I think that it doesn't need to assign to callback.
> 
> This is the default set_control_reg() implementation which is defined
> in the sdhci spec. Any host that that wants default implementation
> case assign this as their callback.
> 
> Hosts that have custom implementations can add in their own drivers.

Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().

callback is for sdhci-xx specific progress.

In my opinion,

static int sdhci_set_ios() 
{
...
	sdhci_set_control_reg();
	if (host->ops && host->ops->set_control_reg)
		host->ops->set_control_reg();

}

if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.

Best Regards,
Jaehoon Chung


> 
> Thanks,
> Faiz
> 

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Aswath Govindraju]

Hi Jaehoon,

On 05/11/20 4:03 am, Jaehoon Chung wrote:
> On 11/5/20 4:05 AM, Faiz Abbas wrote:
>> Jaehoon,
>>
>> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>>> Hi Faiz,
>>>
>>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>>> for SD card UHS modes.
>>>>
>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>> ---
>>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/sdhci.h     |  1 +
>>>>  2 files changed, 52 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>> index 7673219fb3..a69f058191 100644
>>>> --- a/drivers/mmc/sdhci.c
>>>> +++ b/drivers/mmc/sdhci.c
>>>> @@ -20,6 +20,7 @@
>>>>  #include <linux/delay.h>
>>>>  #include <linux/dma-mapping.h>
>>>>  #include <phys2bus.h>
>>>> +#include <power/regulator.h>
>>>>  
>>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>>  {
>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>  }
>>>>  
>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>> +{
>>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>>> +	u32 ctrl;
>>>> +
>>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> +
>>>> +	switch (mmc->signal_voltage) {
>>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> +		if (mmc->vqmmc_supply) {
>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>>
>>> Doesn't need to consider about fail to set its value?
>>
>> You're right. I'll handle the failure case in v3.
>>
>>>
>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>> +		}
>>>> +#endif
>>>> +		mdelay(5);
>>>
>>> For what purpose about mdelay(5)?
>>
>> I'm following this from the kernel implementation here:
>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
>>
>> Not sure if this a part of the spec or not though.
> 
> Thanks for sharing info. :)
> 
>>
>>>
>>>
>>>> +		if (IS_SD(mmc)) {
>>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +		}
>>>> +		break;
>>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> +		if (mmc->vqmmc_supply) {
>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>> +		}
>>>> +#endif
>>>> +		if (IS_SD(mmc)) {
>>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +		}
>>>> +		break;
>>>> +	default:
>>>> +		/* No signal voltage switch required */
>>>> +		return;
>>>> +	}
>>>> +}
>>>> +#else
>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>>> +#endif
>>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>>
>>> this function is called as callback function in sdhci_set_ios(). 
>>> it's strange... set_control_reg callback is for host specific control register.
>>>
>>> I think that it doesn't need to assign to callback.
>>
>> This is the default set_control_reg() implementation which is defined
>> in the sdhci spec. Any host that that wants default implementation
>> case assign this as their callback.
>>
>> Hosts that have custom implementations can add in their own drivers.
> 
> Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
> If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().
> > callback is for sdhci-xx specific progress.
> 
> In my opinion,
> 
> static int sdhci_set_ios() 
> {
> ...
> 	sdhci_set_control_reg();
> 	if (host->ops && host->ops->set_control_reg)
> 		host->ops->set_control_reg();
> 
> }
> 
> if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.
> 


As Faiz has mentioned sdhci_set_control_reg() added here is the default
implementation according to the spec. One other driver apart from
drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
the above implementation is drivers/mmc/zynq_sdhci.c and there are some
which implement it differently for example drivers/mmc/s5p_sdhci.c and
drivers/mmc/sdhci-cadence.c.

So, if a host driver wants use the default implementation then it can
use the one implemented in the sdhci.c or it can implement its own
implementation the way in which all the drivers have done so far.


Thanks,
Aswath

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Jaehoon Chung]

Hi Aswath,

On 1/19/21 9:35 PM, Aswath Govindraju wrote:
> Hi Jaehoon,
> 
> On 05/11/20 4:03 am, Jaehoon Chung wrote:
>> On 11/5/20 4:05 AM, Faiz Abbas wrote:
>>> Jaehoon,
>>>
>>> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>>>> Hi Faiz,
>>>>
>>>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>>>> for SD card UHS modes.
>>>>>
>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>> ---
>>>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/sdhci.h     |  1 +
>>>>>  2 files changed, 52 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>>> index 7673219fb3..a69f058191 100644
>>>>> --- a/drivers/mmc/sdhci.c
>>>>> +++ b/drivers/mmc/sdhci.c
>>>>> @@ -20,6 +20,7 @@
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/dma-mapping.h>
>>>>>  #include <phys2bus.h>
>>>>> +#include <power/regulator.h>
>>>>>  
>>>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>>>  {
>>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>>  }
>>>>>  
>>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>>> +{
>>>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>>>> +	u32 ctrl;
>>>>> +
>>>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>> +
>>>>> +	switch (mmc->signal_voltage) {
>>>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>> +		if (mmc->vqmmc_supply) {
>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>>>
>>>> Doesn't need to consider about fail to set its value?
>>>
>>> You're right. I'll handle the failure case in v3.
>>>
>>>>
>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>> +		}
>>>>> +#endif
>>>>> +		mdelay(5);
>>>>
>>>> For what purpose about mdelay(5)?
>>>
>>> I'm following this from the kernel implementation here:
>>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
>>>
>>> Not sure if this a part of the spec or not though.
>>
>> Thanks for sharing info. :)
>>
>>>
>>>>
>>>>
>>>>> +		if (IS_SD(mmc)) {
>>>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +		}
>>>>> +		break;
>>>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>> +		if (mmc->vqmmc_supply) {
>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>> +		}
>>>>> +#endif
>>>>> +		if (IS_SD(mmc)) {
>>>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +		}
>>>>> +		break;
>>>>> +	default:
>>>>> +		/* No signal voltage switch required */
>>>>> +		return;
>>>>> +	}
>>>>> +}
>>>>> +#else
>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>>>> +#endif
>>>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>>>
>>>> this function is called as callback function in sdhci_set_ios(). 
>>>> it's strange... set_control_reg callback is for host specific control register.
>>>>
>>>> I think that it doesn't need to assign to callback.
>>>
>>> This is the default set_control_reg() implementation which is defined
>>> in the sdhci spec. Any host that that wants default implementation
>>> case assign this as their callback.
>>>
>>> Hosts that have custom implementations can add in their own drivers.
>>
>> Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
>> If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().
>>> callback is for sdhci-xx specific progress.
>>
>> In my opinion,
>>
>> static int sdhci_set_ios() 
>> {
>> ...
>> 	sdhci_set_control_reg();
>> 	if (host->ops && host->ops->set_control_reg)
>> 		host->ops->set_control_reg();
>>
>> }
>>
>> if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
>> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.
>>
> 
> 
> As Faiz has mentioned sdhci_set_control_reg() added here is the default
> implementation according to the spec. One other driver apart from
> drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
> the above implementation is drivers/mmc/zynq_sdhci.c and there are some
> which implement it differently for example drivers/mmc/s5p_sdhci.c and
> drivers/mmc/sdhci-cadence.c.
> 
> So, if a host driver wants use the default implementation then it can
> use the one implemented in the sdhci.c or it can implement its own
> implementation the way in which all the drivers have done so far.

I understood what you said. :)
My mean is sdhci_set_control_reg() should be controlled a generic sdhci spec.
It doesn't need to assign to callback function. 

The default implementation doesn't need to use callback function.
It will be used just in sdhci.c by default. callback function will be additionally used for board specific.
(s5p_sdhci.c has some specific bits at control register. So i had added the callback ops to control them in board specific driver.)

When i have checked its patchset again, am654_sdhci can be used the sdhci_set_control_reg() without callback assigned.

So it's same sequence the below..

sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c
sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is assigned to callback.

doesn't it?

Maybe, it can be changed as likes belows

index baa935e0d5b0..b0e3ecc6314d 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
 const struct sdhci_ops am654_sdhci_ops = {
        .deferred_probe         = am654_sdhci_deferred_probe,
        .set_ios_post           = &am654_sdhci_set_ios_post,
-       .set_control_reg        = &am654_sdhci_set_control_reg,
 };

 const struct am654_driver_data am654_drv_data = {
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 06289343124e..9969a710755e 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
        sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
 }

+void sdhci_set_control_reg(struct sdhci_host *host)
+{
+       sdhci_set_voltage(host);
+       sdhci_set_uhs_timing(host);
+}
+
 #ifdef CONFIG_DM_MMC
 static int sdhci_set_ios(struct udevice *dev)
 {
@@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc)
        struct sdhci_host *host = mmc->priv;
        bool no_hispd_bit = false;

+       sdhci_set_control_reg(host);
+
+       /* If it needs to control the boards specific things, implement callback */
        if (host->ops && host->ops->set_control_reg)
                host->ops->set_control_reg(host);


Then other sdhci host drivers can also used the generic sdhci_set_control_reg() according to SDHCI spec.

Well, it's possible that i misunderstood something, if so that, let me know, plz.

Best Regards,
Jaehoon Chung

> 
> 
> Thanks,
> Aswath
> 

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Aswath Govindraju]

Hi Jaehoon,

On 21/01/21 4:26 am, Jaehoon Chung wrote:
> Hi Aswath,
> 
> On 1/19/21 9:35 PM, Aswath Govindraju wrote:
>> Hi Jaehoon,
>>
>> On 05/11/20 4:03 am, Jaehoon Chung wrote:
>>> On 11/5/20 4:05 AM, Faiz Abbas wrote:
>>>> Jaehoon,
>>>>
>>>> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>>>>> Hi Faiz,
>>>>>
>>>>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>>>>> for SD card UHS modes.
>>>>>>
>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>>> ---
>>>>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/sdhci.h     |  1 +
>>>>>>  2 files changed, 52 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>>>> index 7673219fb3..a69f058191 100644
>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>> @@ -20,6 +20,7 @@
>>>>>>  #include <linux/delay.h>
>>>>>>  #include <linux/dma-mapping.h>
>>>>>>  #include <phys2bus.h>
>>>>>> +#include <power/regulator.h>
>>>>>>  
>>>>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>>>>  {
>>>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>>>  }
>>>>>>  
>>>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>>>> +{
>>>>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>>>>> +	u32 ctrl;
>>>>>> +
>>>>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>> +
>>>>>> +	switch (mmc->signal_voltage) {
>>>>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>>>>
>>>>> Doesn't need to consider about fail to set its value?
>>>>
>>>> You're right. I'll handle the failure case in v3.
>>>>
>>>>>
>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>> +		}
>>>>>> +#endif
>>>>>> +		mdelay(5);
>>>>>
>>>>> For what purpose about mdelay(5)?
>>>>
>>>> I'm following this from the kernel implementation here:
>>>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
>>>>
>>>> Not sure if this a part of the spec or not though.
>>>
>>> Thanks for sharing info. :)
>>>
>>>>
>>>>>
>>>>>
>>>>>> +		if (IS_SD(mmc)) {
>>>>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +		}
>>>>>> +		break;
>>>>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>> +		}
>>>>>> +#endif
>>>>>> +		if (IS_SD(mmc)) {
>>>>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +		}
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		/* No signal voltage switch required */
>>>>>> +		return;
>>>>>> +	}
>>>>>> +}
>>>>>> +#else
>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>>>>> +#endif
>>>>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>>>>
>>>>> this function is called as callback function in sdhci_set_ios(). 
>>>>> it's strange... set_control_reg callback is for host specific control register.
>>>>>
>>>>> I think that it doesn't need to assign to callback.
>>>>
>>>> This is the default set_control_reg() implementation which is defined
>>>> in the sdhci spec. Any host that that wants default implementation
>>>> case assign this as their callback.
>>>>
>>>> Hosts that have custom implementations can add in their own drivers.
>>>
>>> Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
>>> If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().
>>>> callback is for sdhci-xx specific progress.
>>>
>>> In my opinion,
>>>
>>> static int sdhci_set_ios() 
>>> {
>>> ...
>>> 	sdhci_set_control_reg();
>>> 	if (host->ops && host->ops->set_control_reg)
>>> 		host->ops->set_control_reg();
>>>
>>> }
>>>
>>> if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
>>> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.
>>>
>>
>>
>> As Faiz has mentioned sdhci_set_control_reg() added here is the default
>> implementation according to the spec. One other driver apart from
>> drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
>> the above implementation is drivers/mmc/zynq_sdhci.c and there are some
>> which implement it differently for example drivers/mmc/s5p_sdhci.c and
>> drivers/mmc/sdhci-cadence.c.
>>
>> So, if a host driver wants use the default implementation then it can
>> use the one implemented in the sdhci.c or it can implement its own
>> implementation the way in which all the drivers have done so far.
> 
> I understood what you said. :)
> My mean is sdhci_set_control_reg() should be controlled a generic sdhci spec.
> It doesn't need to assign to callback function. 
> 
> The default implementation doesn't need to use callback function.
> It will be used just in sdhci.c by default. callback function will be additionally used for board specific.
> (s5p_sdhci.c has some specific bits at control register. So i had added the callback ops to control them in board specific driver.)
> 

[1]

> When i have checked its patchset again, am654_sdhci can be used the sdhci_set_control_reg() without callback assigned.
> 
> So it's same sequence the below..
> 
> sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c
> sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is assigned to callback.
> 
> doesn't it?
> 

Yes what you mentioned is correct assigning a callback is redundant in
the case of am654_sdhci. The reason behind doing this is so that this
patch would not break support for already existing drivers. If the
implementation that you suggested below is followed then it would
require follow up patches for all the existing drivers and some drivers
might not be able to follow this implementation (reason is explained below).

This way assigning a callback can also be seen in linux kernel mmc host
drivers,

For example,

drivers/mmc/host/sdhci-pxav2.c
drivers/mmc/host/sdhci-xenon.c
drivers/mmc/host/sdhci-omap.c

use function sdhci_pltfm_clk_get_max_clock() as a callback in their
custom drivers which is a part of generic driver
drivers/mmc/host/sdhci-pltfm.c.


> Maybe, it can be changed as likes belows
> 
> index baa935e0d5b0..b0e3ecc6314d 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
>  const struct sdhci_ops am654_sdhci_ops = {
>         .deferred_probe         = am654_sdhci_deferred_probe,
>         .set_ios_post           = &am654_sdhci_set_ios_post,
> -       .set_control_reg        = &am654_sdhci_set_control_reg,
>  };
> 
>  const struct am654_driver_data am654_drv_data = {
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 06289343124e..9969a710755e 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>         sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>  }
> 
> +void sdhci_set_control_reg(struct sdhci_host *host)
> +{
> +       sdhci_set_voltage(host);
> +       sdhci_set_uhs_timing(host);
> +}
> +
>  #ifdef CONFIG_DM_MMC
>  static int sdhci_set_ios(struct udevice *dev)
>  {
> @@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc)
>         struct sdhci_host *host = mmc->priv;
>         bool no_hispd_bit = false;
> 
> +       sdhci_set_control_reg(host);

[2]

> +
> +       /* If it needs to control the boards specific things, implement callback */
>         if (host->ops && host->ops->set_control_reg)
>                 host->ops->set_control_reg(host);
> 
> 
> Then other sdhci host drivers can also used the generic sdhci_set_control_reg() according to SDHCI spec.
> 

If [2] is added, then all the drivers should use it and not "can use it"
which would be a constraint while implementing.

> Well, it's possible that i misunderstood something, if so that, let me know, plz.
> 


As you have mentioned at [1] the s5p_sdhci.c driver does not implement
set_control_reg() in the generic way so it should not call
sdhci_set_control_reg(host) at [2] right?


So, the main reason for going with the implementation in the patch
rather than the one suggested by you is to not break support for the
existing drivers.


Thanks,
Aswath

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Jaehoon Chung]

Hi Aswath,

On 1/21/21 1:13 PM, Aswath Govindraju wrote:
> Hi Jaehoon,
> 
> On 21/01/21 4:26 am, Jaehoon Chung wrote:
>> Hi Aswath,
>>
>> On 1/19/21 9:35 PM, Aswath Govindraju wrote:
>>> Hi Jaehoon,
>>>
>>> On 05/11/20 4:03 am, Jaehoon Chung wrote:
>>>> On 11/5/20 4:05 AM, Faiz Abbas wrote:
>>>>> Jaehoon,
>>>>>
>>>>> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>>>>>> Hi Faiz,
>>>>>>
>>>>>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>>>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>>>>>> for SD card UHS modes.
>>>>>>>
>>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>>>> ---
>>>>>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/sdhci.h     |  1 +
>>>>>>>  2 files changed, 52 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>>>>> index 7673219fb3..a69f058191 100644
>>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>  #include <linux/delay.h>
>>>>>>>  #include <linux/dma-mapping.h>
>>>>>>>  #include <phys2bus.h>
>>>>>>> +#include <power/regulator.h>
>>>>>>>  
>>>>>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>>>>>  {
>>>>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>>>>  }
>>>>>>>  
>>>>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>>>>> +{
>>>>>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>>>>>> +	u32 ctrl;
>>>>>>> +
>>>>>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>>> +
>>>>>>> +	switch (mmc->signal_voltage) {
>>>>>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>>>>>
>>>>>> Doesn't need to consider about fail to set its value?
>>>>>
>>>>> You're right. I'll handle the failure case in v3.
>>>>>
>>>>>>
>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>>> +		}
>>>>>>> +#endif
>>>>>>> +		mdelay(5);
>>>>>>
>>>>>> For what purpose about mdelay(5)?
>>>>>
>>>>> I'm following this from the kernel implementation here:
>>>>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
>>>>>
>>>>> Not sure if this a part of the spec or not though.
>>>>
>>>> Thanks for sharing info. :)
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +		if (IS_SD(mmc)) {
>>>>>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>>> +		}
>>>>>>> +		break;
>>>>>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>>> +		}
>>>>>>> +#endif
>>>>>>> +		if (IS_SD(mmc)) {
>>>>>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>>> +		}
>>>>>>> +		break;
>>>>>>> +	default:
>>>>>>> +		/* No signal voltage switch required */
>>>>>>> +		return;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>>>>>> +#endif
>>>>>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>>>>>
>>>>>> this function is called as callback function in sdhci_set_ios(). 
>>>>>> it's strange... set_control_reg callback is for host specific control register.
>>>>>>
>>>>>> I think that it doesn't need to assign to callback.
>>>>>
>>>>> This is the default set_control_reg() implementation which is defined
>>>>> in the sdhci spec. Any host that that wants default implementation
>>>>> case assign this as their callback.
>>>>>
>>>>> Hosts that have custom implementations can add in their own drivers.
>>>>
>>>> Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
>>>> If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().
>>>>> callback is for sdhci-xx specific progress.
>>>>
>>>> In my opinion,
>>>>
>>>> static int sdhci_set_ios() 
>>>> {
>>>> ...
>>>> 	sdhci_set_control_reg();
>>>> 	if (host->ops && host->ops->set_control_reg)
>>>> 		host->ops->set_control_reg();
>>>>
>>>> }
>>>>
>>>> if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
>>>> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.
>>>>
>>>
>>>
>>> As Faiz has mentioned sdhci_set_control_reg() added here is the default
>>> implementation according to the spec. One other driver apart from
>>> drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
>>> the above implementation is drivers/mmc/zynq_sdhci.c and there are some
>>> which implement it differently for example drivers/mmc/s5p_sdhci.c and
>>> drivers/mmc/sdhci-cadence.c.
>>>
>>> So, if a host driver wants use the default implementation then it can
>>> use the one implemented in the sdhci.c or it can implement its own
>>> implementation the way in which all the drivers have done so far.
>>
>> I understood what you said. :)
>> My mean is sdhci_set_control_reg() should be controlled a generic sdhci spec.
>> It doesn't need to assign to callback function. 
>>
>> The default implementation doesn't need to use callback function.
>> It will be used just in sdhci.c by default. callback function will be additionally used for board specific.
>> (s5p_sdhci.c has some specific bits at control register. So i had added the callback ops to control them in board specific driver.)
>>
> 
> [1]
> 
>> When i have checked its patchset again, am654_sdhci can be used the sdhci_set_control_reg() without callback assigned.
>>
>> So it's same sequence the below..
>>
>> sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c
>> sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is assigned to callback.
>>
>> doesn't it?
>>
> 
> Yes what you mentioned is correct assigning a callback is redundant in
> the case of am654_sdhci. The reason behind doing this is so that this
> patch would not break support for already existing drivers. If the
> implementation that you suggested below is followed then it would
> require follow up patches for all the existing drivers and some drivers
> might not be able to follow this implementation (reason is explained below).
> 
> This way assigning a callback can also be seen in linux kernel mmc host
> drivers,
> 
> For example,
> 
> drivers/mmc/host/sdhci-pxav2.c
> drivers/mmc/host/sdhci-xenon.c
> drivers/mmc/host/sdhci-omap.c
> 
> use function sdhci_pltfm_clk_get_max_clock() as a callback in their
> custom drivers which is a part of generic driver
> drivers/mmc/host/sdhci-pltfm.c.

But i think that sdhci_pltfm_clk_get_max_clock() is not good example.
sdhci-pltfm is provided the generic code of platform side. 

> 
> 
>> Maybe, it can be changed as likes belows
>>
>> index baa935e0d5b0..b0e3ecc6314d 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
>>  const struct sdhci_ops am654_sdhci_ops = {
>>         .deferred_probe         = am654_sdhci_deferred_probe,
>>         .set_ios_post           = &am654_sdhci_set_ios_post,
>> -       .set_control_reg        = &am654_sdhci_set_control_reg,
>>  };
>>
>>  const struct am654_driver_data am654_drv_data = {
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index 06289343124e..9969a710755e 100644
>> --- a/drivers/mmc/sdhci.c
>> +++ b/drivers/mmc/sdhci.c
>> @@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>         sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>  }
>>
>> +void sdhci_set_control_reg(struct sdhci_host *host)
>> +{
>> +       sdhci_set_voltage(host);
>> +       sdhci_set_uhs_timing(host);
>> +}
>> +
>>  #ifdef CONFIG_DM_MMC
>>  static int sdhci_set_ios(struct udevice *dev)
>>  {
>> @@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc)
>>         struct sdhci_host *host = mmc->priv;
>>         bool no_hispd_bit = false;
>>
>> +       sdhci_set_control_reg(host);
> 
> [2]
> 
>> +
>> +       /* If it needs to control the boards specific things, implement callback */
>>         if (host->ops && host->ops->set_control_reg)
>>                 host->ops->set_control_reg(host);
>>
>>
>> Then other sdhci host drivers can also used the generic sdhci_set_control_reg() according to SDHCI spec.
>>
> 
> If [2] is added, then all the drivers should use it and not "can use it"
> which would be a constraint while implementing.

Then it can be avoid with if-else?
At this point, you're right. I didn't know exactly, but it makes sense.

> 
>> Well, it's possible that i misunderstood something, if so that, let me know, plz.
>>
> 
> 
> As you have mentioned at [1] the s5p_sdhci.c driver does not implement
> set_control_reg() in the generic way so it should not call
> sdhci_set_control_reg(host) at [2] right?

Right. When i had added set_control_reg callback, u-boot didn't follow mush rather than now about linux kernel.
And it didn't introduce some features at that time.

> 
> 
> So, the main reason for going with the implementation in the patch
> rather than the one suggested by you is to not break support for the
> existing drivers.

Thanks for kindly explanation in more detail!

If possible to resend the patchset based-on latest, i will test with them.
(There are some conflicts.)


Best Regards,
Jaehoon Chung

> 
> 
> Thanks,
> Aswath
> 

[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Aswath Govindraju]

Hi Jaehoon,

On 21/01/21 10:40 am, Jaehoon Chung wrote:
> Hi Aswath,
> 
> On 1/21/21 1:13 PM, Aswath Govindraju wrote:
>> Hi Jaehoon,
>>
>> On 21/01/21 4:26 am, Jaehoon Chung wrote:
>>> Hi Aswath,
>>>
>>> On 1/19/21 9:35 PM, Aswath Govindraju wrote:
>>>> Hi Jaehoon,
>>>>
>>>> On 05/11/20 4:03 am, Jaehoon Chung wrote:
>>>>> On 11/5/20 4:05 AM, Faiz Abbas wrote:
>>>>>> Jaehoon,
>>>>>>
>>>>>> On 21/10/20 5:08 pm, Jaehoon Chung wrote:
>>>>>>> Hi Faiz,
>>>>>>>
>>>>>>> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>>>>>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>>>>>>>> for SD card UHS modes.
>>>>>>>>
>>>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>>>>> ---
>>>>>>>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  include/sdhci.h     |  1 +
>>>>>>>>  2 files changed, 52 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>>>>>> index 7673219fb3..a69f058191 100644
>>>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>>  #include <linux/delay.h>
>>>>>>>>  #include <linux/dma-mapping.h>
>>>>>>>>  #include <phys2bus.h>
>>>>>>>> +#include <power/regulator.h>
>>>>>>>>  
>>>>>>>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>>>>>>>  {
>>>>>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>>>>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>>>>>> +{
>>>>>>>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>>>>>>>> +	u32 ctrl;
>>>>>>>> +
>>>>>>>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>>>> +
>>>>>>>> +	switch (mmc->signal_voltage) {
>>>>>>>> +	case MMC_SIGNAL_VOLTAGE_330:
>>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
>>>>>>>
>>>>>>> Doesn't need to consider about fail to set its value?
>>>>>>
>>>>>> You're right. I'll handle the failure case in v3.
>>>>>>
>>>>>>>
>>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>>>> +		}
>>>>>>>> +#endif
>>>>>>>> +		mdelay(5);
>>>>>>>
>>>>>>> For what purpose about mdelay(5)?
>>>>>>
>>>>>> I'm following this from the kernel implementation here:
>>>>>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
>>>>>>
>>>>>> Not sure if this a part of the spec or not though.
>>>>>
>>>>> Thanks for sharing info. :)
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> +		if (IS_SD(mmc)) {
>>>>>>>> +			ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>>>> +		}
>>>>>>>> +		break;
>>>>>>>> +	case MMC_SIGNAL_VOLTAGE_180:
>>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>>>>> +		if (mmc->vqmmc_supply) {
>>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>>>>>>>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>>>>>>>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>>>>>>>> +		}
>>>>>>>> +#endif
>>>>>>>> +		if (IS_SD(mmc)) {
>>>>>>>> +			ctrl |= SDHCI_CTRL_VDD_180;
>>>>>>>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>>>> +		}
>>>>>>>> +		break;
>>>>>>>> +	default:
>>>>>>>> +		/* No signal voltage switch required */
>>>>>>>> +		return;
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>>>>>>>> +#endif
>>>>>>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>>>>>>
>>>>>>> this function is called as callback function in sdhci_set_ios(). 
>>>>>>> it's strange... set_control_reg callback is for host specific control register.
>>>>>>>
>>>>>>> I think that it doesn't need to assign to callback.
>>>>>>
>>>>>> This is the default set_control_reg() implementation which is defined
>>>>>> in the sdhci spec. Any host that that wants default implementation
>>>>>> case assign this as their callback.
>>>>>>
>>>>>> Hosts that have custom implementations can add in their own drivers.
>>>>>
>>>>> Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
>>>>> If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().
>>>>>> callback is for sdhci-xx specific progress.
>>>>>
>>>>> In my opinion,
>>>>>
>>>>> static int sdhci_set_ios() 
>>>>> {
>>>>> ...
>>>>> 	sdhci_set_control_reg();
>>>>> 	if (host->ops && host->ops->set_control_reg)
>>>>> 		host->ops->set_control_reg();
>>>>>
>>>>> }
>>>>>
>>>>> if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
>>>>> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.
>>>>>
>>>>
>>>>
>>>> As Faiz has mentioned sdhci_set_control_reg() added here is the default
>>>> implementation according to the spec. One other driver apart from
>>>> drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
>>>> the above implementation is drivers/mmc/zynq_sdhci.c and there are some
>>>> which implement it differently for example drivers/mmc/s5p_sdhci.c and
>>>> drivers/mmc/sdhci-cadence.c.
>>>>
>>>> So, if a host driver wants use the default implementation then it can
>>>> use the one implemented in the sdhci.c or it can implement its own
>>>> implementation the way in which all the drivers have done so far.
>>>
>>> I understood what you said. :)
>>> My mean is sdhci_set_control_reg() should be controlled a generic sdhci spec.
>>> It doesn't need to assign to callback function. 
>>>
>>> The default implementation doesn't need to use callback function.
>>> It will be used just in sdhci.c by default. callback function will be additionally used for board specific.
>>> (s5p_sdhci.c has some specific bits at control register. So i had added the callback ops to control them in board specific driver.)
>>>
>>
>> [1]
>>
>>> When i have checked its patchset again, am654_sdhci can be used the sdhci_set_control_reg() without callback assigned.
>>>
>>> So it's same sequence the below..
>>>
>>> sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c
>>> sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is assigned to callback.
>>>
>>> doesn't it?
>>>
>>
>> Yes what you mentioned is correct assigning a callback is redundant in
>> the case of am654_sdhci. The reason behind doing this is so that this
>> patch would not break support for already existing drivers. If the
>> implementation that you suggested below is followed then it would
>> require follow up patches for all the existing drivers and some drivers
>> might not be able to follow this implementation (reason is explained below).
>>
>> This way assigning a callback can also be seen in linux kernel mmc host
>> drivers,
>>
>> For example,
>>
>> drivers/mmc/host/sdhci-pxav2.c
>> drivers/mmc/host/sdhci-xenon.c
>> drivers/mmc/host/sdhci-omap.c
>>
>> use function sdhci_pltfm_clk_get_max_clock() as a callback in their
>> custom drivers which is a part of generic driver
>> drivers/mmc/host/sdhci-pltfm.c.
> 
> But i think that sdhci_pltfm_clk_get_max_clock() is not good example.
> sdhci-pltfm is provided the generic code of platform side. 
> 
>>
>>
>>> Maybe, it can be changed as likes belows
>>>
>>> index baa935e0d5b0..b0e3ecc6314d 100644
>>> --- a/drivers/mmc/am654_sdhci.c
>>> +++ b/drivers/mmc/am654_sdhci.c
>>> @@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
>>>  const struct sdhci_ops am654_sdhci_ops = {
>>>         .deferred_probe         = am654_sdhci_deferred_probe,
>>>         .set_ios_post           = &am654_sdhci_set_ios_post,
>>> -       .set_control_reg        = &am654_sdhci_set_control_reg,
>>>  };
>>>
>>>  const struct am654_driver_data am654_drv_data = {
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>> index 06289343124e..9969a710755e 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>         sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>  }
>>>
>>> +void sdhci_set_control_reg(struct sdhci_host *host)
>>> +{
>>> +       sdhci_set_voltage(host);
>>> +       sdhci_set_uhs_timing(host);
>>> +}
>>> +
>>>  #ifdef CONFIG_DM_MMC
>>>  static int sdhci_set_ios(struct udevice *dev)
>>>  {
>>> @@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc)
>>>         struct sdhci_host *host = mmc->priv;
>>>         bool no_hispd_bit = false;
>>>
>>> +       sdhci_set_control_reg(host);
>>
>> [2]
>>
>>> +
>>> +       /* If it needs to control the boards specific things, implement callback */
>>>         if (host->ops && host->ops->set_control_reg)
>>>                 host->ops->set_control_reg(host);
>>>
>>>
>>> Then other sdhci host drivers can also used the generic sdhci_set_control_reg() according to SDHCI spec.
>>>
>>
>> If [2] is added, then all the drivers should use it and not "can use it"
>> which would be a constraint while implementing.
> 
> Then it can be avoid with if-else?
> At this point, you're right. I didn't know exactly, but it makes sense.
> 
>>
>>> Well, it's possible that i misunderstood something, if so that, let me know, plz.
>>>
>>
>>
>> As you have mentioned at [1] the s5p_sdhci.c driver does not implement
>> set_control_reg() in the generic way so it should not call
>> sdhci_set_control_reg(host) at [2] right?
> 
> Right. When i had added set_control_reg callback, u-boot didn't follow mush rather than now about linux kernel.
> And it didn't introduce some features at that time.
> 
>>
>>
>> So, the main reason for going with the implementation in the patch
>> rather than the one suggested by you is to not break support for the
>> existing drivers.
> 
> Thanks for kindly explanation in more detail!
> 
> If possible to resend the patchset based-on latest, i will test with them.
> (There are some conflicts.)
> 

I have sent a respin(v3) of this series.

Thanks,
Aswath