[PATCH 0/7] Arasan sdhci driver updates

[PATCH 0/7] Arasan sdhci driver updates

[Ashok Reddy Soma]

This patch series updates/fixes below things:
 - Handle errors from tapdelay functions and return to set_delay()
 - Add ZynqMP firmware related enums which are used in sdhci driver
 - Replace mmio_write() with firmware call xilinx_pm_request()
 - Move tapdelay setting code from tap_delays.c to driver and remove
   tap_dealy.c and zynqmp_tap_delay.h
 - Change variable name from deviceid to node_id in couple of functions
   for consistancy
 - Add a workaround for sd card detect stable issue for Versal platforms
 - Use set_control_reg from sdhci.c


Ashok Reddy Soma (4):
  mmc: zynq_sdhci: Use xilinx pm request instead of mmio_write
  mmc: zynq_sdhci: Move setting tapdelay code to driver
  mmc: zynq_sdhci: Change variable deviceid to node_id
  mmc: zynq_sdhci: Use set_control_reg from sdhci.c

T Karthik Reddy (3):
  mmc: sdhci: Return error in case of failure
  zynqmp_firmware: Add zynqmp firmware related enums
  mmc: zynq_sdhci: Wait till sd card detect state is stable

 board/xilinx/zynqmp/Makefile     |   2 -
 board/xilinx/zynqmp/tap_delays.c | 101 --------------------
 drivers/mmc/sdhci.c              |   8 +-
 drivers/mmc/zynq_sdhci.c         | 152 +++++++++++++++++++++++--------
 include/sdhci.h                  |   2 +-
 include/zynqmp_firmware.h        | 127 ++++++++++++++++++++++++++
 include/zynqmp_tap_delay.h       |  21 -----
 7 files changed, 246 insertions(+), 167 deletions(-)
 delete mode 100644 board/xilinx/zynqmp/tap_delays.c
 delete mode 100644 include/zynqmp_tap_delay.h

-- 
2.17.1

[PATCH 1/7] mmc: sdhci: Return error in case of failure

[Ashok Reddy Soma]

From: T Karthik Reddy <t.karthik.reddy@xilinx.com>

set_delay() function is from sdhci host ops, which does not return
any error due to void return type. Get return values from input and
output set clock phase functions inside arasan_sdhci_set_tapdelay()
and return the errors.

Change return type to int for arasan_sdhci_set_tapdelay() and also for
set_delay() in sdhci_ops structure.

Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 drivers/mmc/sdhci.c      |  8 ++++++--
 drivers/mmc/zynq_sdhci.c | 21 ++++++++++++++++-----
 include/sdhci.h          |  2 +-
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index d9ab6a0a83..f144602eec 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -366,6 +366,7 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 {
 	struct sdhci_host *host = mmc->priv;
 	unsigned int div, clk = 0, timeout;
+	int ret;
 
 	/* Wait max 20 ms */
 	timeout = 200;
@@ -386,8 +387,11 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	if (clock == 0)
 		return 0;
 
-	if (host->ops && host->ops->set_delay)
-		host->ops->set_delay(host);
+	if (host->ops && host->ops->set_delay) {
+		ret = host->ops->set_delay(host);
+		if (ret)
+			return ret;
+	}
 
 	if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
 		/*
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index ba87ee8dd5..9fb3603c7e 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -422,7 +422,7 @@ static int sdhci_versal_sampleclk_set_phase(struct sdhci_host *host,
 	return 0;
 }
 
-static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
+static int arasan_sdhci_set_tapdelay(struct sdhci_host *host)
 {
 	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
 	struct arasan_sdhci_clk_data *clk_data = &priv->clk_data;
@@ -431,18 +431,29 @@ static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
 	u8 timing = mode2timing[mmc->selected_mode];
 	u32 iclk_phase = clk_data->clk_phase_in[timing];
 	u32 oclk_phase = clk_data->clk_phase_out[timing];
+	int ret;
 
 	dev_dbg(dev, "%s, host:%s, mode:%d\n", __func__, host->name, timing);
 
 	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
 	    device_is_compatible(dev, "xlnx,zynqmp-8.9a")) {
-		sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
-		sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
+		ret = sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
+		if (ret)
+			return ret;
+		ret = sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
+		if (ret)
+			return ret;
 	} else if (IS_ENABLED(CONFIG_ARCH_VERSAL) &&
 		   device_is_compatible(dev, "xlnx,versal-8.9a")) {
-		sdhci_versal_sampleclk_set_phase(host, iclk_phase);
-		sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
+		ret = sdhci_versal_sampleclk_set_phase(host, iclk_phase);
+		if (ret)
+			return ret;
+		ret = sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
+		if (ret)
+			return ret;
 	}
+
+	return 0;
 }
 
 static void arasan_dt_read_clk_phase(struct udevice *dev, unsigned char timing,
diff --git a/include/sdhci.h b/include/sdhci.h
index 0ae9471ad7..44a0d84e5a 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -268,7 +268,7 @@ struct sdhci_ops {
 	int	(*set_ios_post)(struct sdhci_host *host);
 	void	(*set_clock)(struct sdhci_host *host, u32 div);
 	int (*platform_execute_tuning)(struct mmc *host, u8 opcode);
-	void (*set_delay)(struct sdhci_host *host);
+	int (*set_delay)(struct sdhci_host *host);
 	int	(*deferred_probe)(struct sdhci_host *host);
 };
 
-- 
2.17.1

[PATCH 2/7] zynqmp_firmware: Add zynqmp firmware related enums

[Ashok Reddy Soma]

From: T Karthik Reddy <t.karthik.reddy@xilinx.com>

Add enums for pm node id's, pm ioctl id's, tapdelay types, dll reset types

Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 include/zynqmp_firmware.h | 127 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/include/zynqmp_firmware.h b/include/zynqmp_firmware.h
index f6f82bf882..c559ee7d3c 100644
--- a/include/zynqmp_firmware.h
+++ b/include/zynqmp_firmware.h
@@ -62,6 +62,99 @@ enum pm_api_id {
 	PM_API_MAX,
 };
 
+enum pm_node_id {
+	NODE_UNKNOWN = 0,
+	NODE_APU = 1,
+	NODE_APU_0 = 2,
+	NODE_APU_1 = 3,
+	NODE_APU_2 = 4,
+	NODE_APU_3 = 5,
+	NODE_RPU = 6,
+	NODE_RPU_0 = 7,
+	NODE_RPU_1 = 8,
+	NODE_PLD = 9,
+	NODE_FPD = 10,
+	NODE_OCM_BANK_0 = 11,
+	NODE_OCM_BANK_1 = 12,
+	NODE_OCM_BANK_2 = 13,
+	NODE_OCM_BANK_3 = 14,
+	NODE_TCM_0_A = 15,
+	NODE_TCM_0_B = 16,
+	NODE_TCM_1_A = 17,
+	NODE_TCM_1_B = 18,
+	NODE_L2 = 19,
+	NODE_GPU_PP_0 = 20,
+	NODE_GPU_PP_1 = 21,
+	NODE_USB_0 = 22,
+	NODE_USB_1 = 23,
+	NODE_TTC_0 = 24,
+	NODE_TTC_1 = 25,
+	NODE_TTC_2 = 26,
+	NODE_TTC_3 = 27,
+	NODE_SATA = 28,
+	NODE_ETH_0 = 29,
+	NODE_ETH_1 = 30,
+	NODE_ETH_2 = 31,
+	NODE_ETH_3 = 32,
+	NODE_UART_0 = 33,
+	NODE_UART_1 = 34,
+	NODE_SPI_0 = 35,
+	NODE_SPI_1 = 36,
+	NODE_I2C_0 = 37,
+	NODE_I2C_1 = 38,
+	NODE_SD_0 = 39,
+	NODE_SD_1 = 40,
+	NODE_DP = 41,
+	NODE_GDMA = 42,
+	NODE_ADMA = 43,
+	NODE_NAND = 44,
+	NODE_QSPI = 45,
+	NODE_GPIO = 46,
+	NODE_CAN_0 = 47,
+	NODE_CAN_1 = 48,
+	NODE_EXTERN = 49,
+	NODE_APLL = 50,
+	NODE_VPLL = 51,
+	NODE_DPLL = 52,
+	NODE_RPLL = 53,
+	NODE_IOPLL = 54,
+	NODE_DDR = 55,
+	NODE_IPI_APU = 56,
+	NODE_IPI_RPU_0 = 57,
+	NODE_GPU = 58,
+	NODE_PCIE = 59,
+	NODE_PCAP = 60,
+	NODE_RTC = 61,
+	NODE_LPD = 62,
+	NODE_VCU = 63,
+	NODE_IPI_RPU_1 = 64,
+	NODE_IPI_PL_0 = 65,
+	NODE_IPI_PL_1 = 66,
+	NODE_IPI_PL_2 = 67,
+	NODE_IPI_PL_3 = 68,
+	NODE_PL = 69,
+	NODE_GEM_TSU = 70,
+	NODE_SWDT_0 = 71,
+	NODE_SWDT_1 = 72,
+	NODE_CSU = 73,
+	NODE_PJTAG = 74,
+	NODE_TRACE = 75,
+	NODE_TESTSCAN = 76,
+	NODE_PMU = 77,
+	NODE_MAX = 78,
+};
+
+enum tap_delay_type {
+	PM_TAPDELAY_INPUT = 0,
+	PM_TAPDELAY_OUTPUT = 1,
+};
+
+enum dll_reset_type {
+	PM_DLL_RESET_ASSERT = 0,
+	PM_DLL_RESET_RELEASE = 1,
+	PM_DLL_RESET_PULSE = 2,
+};
+
 enum pm_query_id {
 	PM_QID_INVALID = 0,
 	PM_QID_CLOCK_GET_NAME = 1,
@@ -79,6 +172,40 @@ enum pm_query_id {
 	PM_QID_CLOCK_GET_MAX_DIVISOR = 13,
 };
 
+enum pm_ioctl_id {
+	IOCTL_GET_RPU_OPER_MODE,
+	IOCTL_SET_RPU_OPER_MODE,
+	IOCTL_RPU_BOOT_ADDR_CONFIG,
+	IOCTL_TCM_COMB_CONFIG,
+	IOCTL_SET_TAPDELAY_BYPASS,
+	IOCTL_SET_SGMII_MODE,
+	IOCTL_SD_DLL_RESET,
+	IOCTL_SET_SD_TAPDELAY,
+	IOCTL_SET_PLL_FRAC_MODE,
+	IOCTL_GET_PLL_FRAC_MODE,
+	IOCTL_SET_PLL_FRAC_DATA,
+	IOCTL_GET_PLL_FRAC_DATA,
+	IOCTL_WRITE_GGS,
+	IOCTL_READ_GGS,
+	IOCTL_WRITE_PGGS,
+	IOCTL_READ_PGGS,
+	/* IOCTL for ULPI reset */
+	IOCTL_ULPI_RESET,
+	/* Set healthy bit value*/
+	IOCTL_SET_BOOT_HEALTH_STATUS,
+	IOCTL_AFI,
+	/* Probe counter read/write */
+	IOCTL_PROBE_COUNTER_READ,
+	IOCTL_PROBE_COUNTER_WRITE,
+	IOCTL_OSPI_MUX_SELECT,
+	/* IOCTL for USB power request */
+	IOCTL_USB_SET_STATE,
+	/* IOCTL to get last reset reason */
+	IOCTL_GET_LAST_RESET_REASON,
+	/* AIE ISR Clear */
+	IOCTL_AIE_ISR_CLEAR,
+};
+
 #define PM_SIP_SVC      0xc2000000
 
 #define ZYNQMP_PM_VERSION_MAJOR         1
-- 
2.17.1

[PATCH 3/7] mmc: zynq_sdhci: Use xilinx pm request instead of mmio_write

[Ashok Reddy Soma]

Currently xilinx sdhci driver is using zynqmp_mmio_write() to set
tapdelay values. Use xilinx_pm_request() using appropriate arguments
to set input/output tapdelays for zynqmp. Where tapdelay setting is
done by firmware. Host driver should explicitly request DLL reset
before ITAP (assert DLL) and after OTAP (release DLL) to avoid issues
in some cases. Also handle error return where possible.

Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 board/xilinx/zynqmp/tap_delays.c | 89 +++-----------------------------
 drivers/mmc/zynq_sdhci.c         | 72 +++++++++++++++++++++-----
 include/zynqmp_tap_delay.h       | 25 ++++++---
 3 files changed, 84 insertions(+), 102 deletions(-)

diff --git a/board/xilinx/zynqmp/tap_delays.c b/board/xilinx/zynqmp/tap_delays.c
index d16bbb8eff..4ce2244060 100644
--- a/board/xilinx/zynqmp/tap_delays.c
+++ b/board/xilinx/zynqmp/tap_delays.c
@@ -10,92 +10,17 @@
 #include <asm/arch/sys_proto.h>
 #include <linux/delay.h>
 #include <mmc.h>
+#include <zynqmp_firmware.h>
 
-#define SD_DLL_CTRL			0xFF180358
-#define SD_ITAP_DLY			0xFF180314
-#define SD_OTAP_DLY			0xFF180318
-#define SD0_DLL_RST_MASK		0x00000004
-#define SD0_DLL_RST			0x00000004
-#define SD1_DLL_RST_MASK		0x00040000
-#define SD1_DLL_RST			0x00040000
-#define SD0_ITAPCHGWIN_MASK		0x00000200
-#define SD0_ITAPCHGWIN			0x00000200
-#define SD1_ITAPCHGWIN_MASK		0x02000000
-#define SD1_ITAPCHGWIN			0x02000000
-#define SD0_ITAPDLYENA_MASK		0x00000100
-#define SD0_ITAPDLYENA			0x00000100
-#define SD1_ITAPDLYENA_MASK		0x01000000
-#define SD1_ITAPDLYENA			0x01000000
-#define SD0_ITAPDLYSEL_MASK		0x000000FF
-#define SD1_ITAPDLYSEL_MASK		0x00FF0000
-#define SD0_OTAPDLYSEL_MASK		0x0000003F
-#define SD1_OTAPDLYSEL_MASK		0x003F0000
-
-void zynqmp_dll_reset(u8 deviceid)
+int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type, u32 itap_delay)
 {
-	/* Issue DLL Reset */
-	if (deviceid == 0)
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK,
-				  SD0_DLL_RST);
-	else
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK,
-				  SD1_DLL_RST);
-
-	mdelay(1);
 
-	/* Release DLL Reset */
-	if (deviceid == 0)
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
-	else
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
+	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+			  type, itap_delay, NULL);
 }
 
-void arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 itap_delay)
+int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type, u32 otap_delay)
 {
-	if (deviceid == 0) {
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
-
-		/* Program ITAP delay */
-		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK,
-				  SD0_ITAPCHGWIN);
-		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYENA_MASK,
-				  SD0_ITAPDLYENA);
-		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYSEL_MASK, itap_delay);
-		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK, 0x0);
-
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
-	} else {
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
-
-		/* Program ITAP delay */
-		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK,
-				  SD1_ITAPCHGWIN);
-		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYENA_MASK,
-				  SD1_ITAPDLYENA);
-		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYSEL_MASK,
-				  (itap_delay << 16));
-		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK, 0x0);
-
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
-	}
-}
-
-void arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 otap_delay)
-{
-	if (deviceid == 0) {
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
-
-		/* Program OTAP delay */
-		zynqmp_mmio_write(SD_OTAP_DLY, SD0_OTAPDLYSEL_MASK, otap_delay);
-
-		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
-	} else {
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
-
-		/* Program OTAP delay */
-		zynqmp_mmio_write(SD_OTAP_DLY, SD1_OTAPDLYSEL_MASK,
-				  (otap_delay << 16));
-
-		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
-	}
+	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+			  type, otap_delay, NULL);
 }
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index 9fb3603c7e..4dafd47b57 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -18,6 +18,7 @@
 #include <malloc.h>
 #include <sdhci.h>
 #include <zynqmp_tap_delay.h>
+#include <zynqmp_firmware.h>
 
 #define SDHCI_ARASAN_ITAPDLY_REGISTER	0xF0F8
 #define SDHCI_ARASAN_ITAPDLY_SEL_MASK	GENMASK(7, 0)
@@ -75,26 +76,39 @@ static const u8 mode2timing[] = {
 	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
 };
 
-static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
+static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
 {
-	u16 clk;
+	struct mmc *mmc = (struct mmc *)host->mmc;
+	struct udevice *dev = mmc->dev;
 	unsigned long timeout;
+	int ret;
+	u16 clk;
 
 	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 	clk &= ~(SDHCI_CLOCK_CARD_EN);
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Issue DLL Reset */
-	zynqmp_dll_reset(deviceid);
+	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_ASSERT);
+	if (ret) {
+		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
+		return ret;
+	}
+
+	mdelay(1);
+	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_RELEASE);
+	if (ret) {
+		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
+		return ret;
+	}
 
 	/* Wait max 20 ms */
 	timeout = 100;
 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 				& SDHCI_CLOCK_INT_STABLE)) {
 		if (timeout == 0) {
-			dev_err(mmc_dev(host->mmc),
-				": Internal clock never stabilised.\n");
-			return;
+			dev_err(dev, ": Internal clock never stabilised.\n");
+			return -EBUSY;
 		}
 		timeout--;
 		udelay(1000);
@@ -102,6 +116,8 @@ static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
 
 	clk |= SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	return 0;
 }
 
 static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
@@ -109,15 +125,14 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	struct mmc_cmd cmd;
 	struct mmc_data data;
 	u32 ctrl;
+	u8 node_id = mmc->dev->seq_ ? NODE_SD_0 : NODE_SD_1;
 	struct sdhci_host *host;
 	struct arasan_sdhci_priv *priv = dev_get_priv(mmc->dev);
 	char tuning_loop_counter = SDHCI_TUNING_LOOP_COUNT;
-	u8 deviceid;
 
 	debug("%s\n", __func__);
 
 	host = priv->host;
-	deviceid = priv->deviceid;
 
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 	ctrl |= SDHCI_CTRL_EXEC_TUNING;
@@ -125,7 +140,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 
 	mdelay(1);
 
-	arasan_zynqmp_dll_reset(host, deviceid);
+	arasan_zynqmp_dll_reset(host, node_id);
 
 	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
 	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
@@ -171,7 +186,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	}
 
 	udelay(1);
-	arasan_zynqmp_dll_reset(host, deviceid);
+	arasan_zynqmp_dll_reset(host, node_id);
 
 	/* Enable only interrupts served by the SD controller */
 	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
@@ -194,10 +209,12 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
 					    int degrees)
 {
-	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
 	struct mmc *mmc = (struct mmc *)host->mmc;
+	struct udevice *dev = mmc->dev;
+	u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
 	u8 tap_delay, tap_max = 0;
 	int timing = mode2timing[mmc->selected_mode];
+	int ret;
 
 	/*
 	 * This is applicable for SDHCI_SPEC_300 and above
@@ -233,7 +250,20 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
 	/* Limit output tap_delay value to 6 bits */
 	tap_delay &= SDHCI_ARASAN_OTAPDLY_SEL_MASK;
 
-	arasan_zynqmp_set_out_tapdelay(priv->deviceid, tap_delay);
+	/* Set the Clock Phase */
+	ret = arasan_zynqmp_set_out_tapdelay(node_id,
+					     PM_TAPDELAY_OUTPUT, tap_delay);
+	if (ret) {
+		dev_err(dev, "Error setting output Tap Delay\n");
+		return ret;
+	}
+
+	/* Release DLL Reset */
+	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
+	if (ret) {
+		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
+		return ret;
+	}
 
 	return 0;
 }
@@ -250,10 +280,12 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
 static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
 					    int degrees)
 {
-	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
 	struct mmc *mmc = (struct mmc *)host->mmc;
+	struct udevice *dev = mmc->dev;
+	u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
 	u8 tap_delay, tap_max = 0;
 	int timing = mode2timing[mmc->selected_mode];
+	int ret;
 
 	/*
 	 * This is applicable for SDHCI_SPEC_300 and above
@@ -263,6 +295,13 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
 	if (SDHCI_GET_VERSION(host) < SDHCI_SPEC_300)
 		return 0;
 
+	/* Assert DLL Reset */
+	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
+	if (ret) {
+		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
+		return ret;
+	}
+
 	switch (timing) {
 	case MMC_TIMING_MMC_HS:
 	case MMC_TIMING_SD_HS:
@@ -289,7 +328,12 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
 	/* Limit input tap_delay value to 8 bits */
 	tap_delay &= SDHCI_ARASAN_ITAPDLY_SEL_MASK;
 
-	arasan_zynqmp_set_in_tapdelay(priv->deviceid, tap_delay);
+	ret = arasan_zynqmp_set_in_tapdelay(node_id,
+					    PM_TAPDELAY_INPUT, tap_delay);
+	if (ret) {
+		dev_err(dev, "Error setting Input Tap Delay\n");
+		return ret;
+	}
 
 	return 0;
 }
diff --git a/include/zynqmp_tap_delay.h b/include/zynqmp_tap_delay.h
index 1c1e3e7dee..7e4d4e4a9a 100644
--- a/include/zynqmp_tap_delay.h
+++ b/include/zynqmp_tap_delay.h
@@ -8,14 +8,27 @@
 #ifndef __ZYNQMP_TAP_DELAY_H__
 #define __ZYNQMP_TAP_DELAY_H__
 
+#include <zynqmp_firmware.h>
+
 #ifdef CONFIG_ARCH_ZYNQMP
-void zynqmp_dll_reset(u8 deviceid);
-void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay);
-void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay);
+int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay);
+int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay);
 #else
-inline void zynqmp_dll_reset(u8 deviceid) {}
-inline void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay) {}
-inline void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay) {}
+inline int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay)
+{
+	return 0;
+}
+
+int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay)
+{
+	return 0;
+}
 #endif
 
+static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
+{
+	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
+				 type, 0, NULL);
+}
+
 #endif
-- 
2.17.1

[PATCH 4/7] mmc: zynq_sdhci: Move setting tapdelay code to driver

[Ashok Reddy Soma]

tap_delays.c just has calls to xilinx_pm_request() for setting tapdelays.
Simply move these calls to zynq_sdhci.c and make them static inline.
Similarly zynqmp_tap_delay.h also has call to xilinx_pm_request() for
dll reset. Do the same for this file as well.

Remove tap_delays.c and zynqmp_tap_delay.h files.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 board/xilinx/zynqmp/Makefile     |  2 --
 board/xilinx/zynqmp/tap_delays.c | 26 ------------------------
 drivers/mmc/zynq_sdhci.c         | 21 +++++++++++++++++++-
 include/zynqmp_tap_delay.h       | 34 --------------------------------
 4 files changed, 20 insertions(+), 63 deletions(-)
 delete mode 100644 board/xilinx/zynqmp/tap_delays.c
 delete mode 100644 include/zynqmp_tap_delay.h

diff --git a/board/xilinx/zynqmp/Makefile b/board/xilinx/zynqmp/Makefile
index 7d8277ca40..a914028753 100644
--- a/board/xilinx/zynqmp/Makefile
+++ b/board/xilinx/zynqmp/Makefile
@@ -44,8 +44,6 @@ $(obj)/pm_cfg_obj.o: $(shell cd $(srctree); readlink -f $(CONFIG_ZYNQMP_SPL_PM_C
 endif
 endif
 
-obj-$(CONFIG_MMC_SDHCI_ZYNQ) += tap_delays.o
-
 ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_CMD_ZYNQMP) += cmds.o
 endif
diff --git a/board/xilinx/zynqmp/tap_delays.c b/board/xilinx/zynqmp/tap_delays.c
deleted file mode 100644
index 4ce2244060..0000000000
--- a/board/xilinx/zynqmp/tap_delays.c
+++ /dev/null
@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Xilinx ZynqMP SoC Tap Delay Programming
- *
- * Copyright (C) 2018 Xilinx, Inc.
- */
-
-#include <common.h>
-#include <zynqmp_tap_delay.h>
-#include <asm/arch/sys_proto.h>
-#include <linux/delay.h>
-#include <mmc.h>
-#include <zynqmp_firmware.h>
-
-int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type, u32 itap_delay)
-{
-
-	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
-			  type, itap_delay, NULL);
-}
-
-int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type, u32 otap_delay)
-{
-	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
-			  type, otap_delay, NULL);
-}
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index 4dafd47b57..97caca51e2 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -17,7 +17,6 @@
 #include <linux/libfdt.h>
 #include <malloc.h>
 #include <sdhci.h>
-#include <zynqmp_tap_delay.h>
 #include <zynqmp_firmware.h>
 
 #define SDHCI_ARASAN_ITAPDLY_REGISTER	0xF0F8
@@ -76,6 +75,26 @@ static const u8 mode2timing[] = {
 	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
 };
 
+static inline int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type,
+						u32 itap_delay)
+{
+	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+				 type, itap_delay, NULL);
+}
+
+static inline int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type,
+						 u32 otap_delay)
+{
+	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+				 type, otap_delay, NULL);
+}
+
+static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
+{
+	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
+				 type, 0, NULL);
+}
+
 static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
 {
 	struct mmc *mmc = (struct mmc *)host->mmc;
diff --git a/include/zynqmp_tap_delay.h b/include/zynqmp_tap_delay.h
deleted file mode 100644
index 7e4d4e4a9a..0000000000
--- a/include/zynqmp_tap_delay.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Xilinx ZynqMP SoC Tap Delay Programming
- *
- * Copyright (C) 2018 Xilinx, Inc.
- */
-
-#ifndef __ZYNQMP_TAP_DELAY_H__
-#define __ZYNQMP_TAP_DELAY_H__
-
-#include <zynqmp_firmware.h>
-
-#ifdef CONFIG_ARCH_ZYNQMP
-int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay);
-int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay);
-#else
-inline int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay)
-{
-	return 0;
-}
-
-int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay)
-{
-	return 0;
-}
-#endif
-
-static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
-{
-	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
-				 type, 0, NULL);
-}
-
-#endif
-- 
2.17.1

[PATCH 5/7] mmc: zynq_sdhci: Change variable deviceid to node_id

[Ashok Reddy Soma]

Change deviceid to node_id in arasan_zynqmp_dll_reset() and also in
tapdelay related static inline functions to reflect proper name and
for consistency.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 drivers/mmc/zynq_sdhci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index 97caca51e2..8ffa8c1269 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -75,17 +75,17 @@ static const u8 mode2timing[] = {
 	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
 };
 
-static inline int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type,
+static inline int arasan_zynqmp_set_in_tapdelay(u8 node_id, u32 type,
 						u32 itap_delay)
 {
-	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SET_SD_TAPDELAY,
 				 type, itap_delay, NULL);
 }
 
-static inline int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type,
+static inline int arasan_zynqmp_set_out_tapdelay(u8 node_id, u32 type,
 						 u32 otap_delay)
 {
-	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
+	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SET_SD_TAPDELAY,
 				 type, otap_delay, NULL);
 }
 
@@ -95,7 +95,7 @@ static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
 				 type, 0, NULL);
 }
 
-static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
+static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 node_id)
 {
 	struct mmc *mmc = (struct mmc *)host->mmc;
 	struct udevice *dev = mmc->dev;
@@ -108,14 +108,14 @@ static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Issue DLL Reset */
-	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_ASSERT);
+	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
 	if (ret) {
 		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
 		return ret;
 	}
 
 	mdelay(1);
-	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_RELEASE);
+	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
 	if (ret) {
 		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
 		return ret;
-- 
2.17.1

[PATCH 6/7] mmc: zynq_sdhci: Wait till sd card detect state is stable

[Ashok Reddy Soma]

From: T Karthik Reddy <t.karthik.reddy@xilinx.com>

As per SD spec when SD host controller is reset, it takes 1000msec
to detect the card state. In case, if we enable the sd bus voltage &
card detect state is not stable, then host controller will disable
the sd bus voltage.

In case of warm/subsystem reboot, due to unstable card detect state
host controller is disabling the sd bus voltage to sd card causing
sd card timeout error. So we wait for a maximum of 1000msec to get
the card detect state stable before we enable the sd bus voltage.

This current fix is workaround for now, this needs to be analysed
further. Zynqmp platform should behave the same as Versal, but we
did not encounter this issue as of now. So we are fixing it for
Versal only.

Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 drivers/mmc/zynq_sdhci.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index 8ffa8c1269..a192f60320 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -686,6 +686,23 @@ static int arasan_sdhci_probe(struct udevice *dev)
 		return ret;
 	upriv->mmc = host->mmc;
 
+	/*
+	 * Wait for 1000msec till the card detect state gets stable
+	 * else host controller will set sd power bus voltage to 0.
+	 */
+	if (IS_ENABLED(CONFIG_ARCH_VERSAL)) {
+		u32 timeout = 1000;
+
+		while (((sdhci_readl(host, SDHCI_PRESENT_STATE) &
+			 SDHCI_CARD_STATE_STABLE) == 0) && timeout--) {
+			mdelay(1);
+		}
+		if (!timeout) {
+			dev_err(dev, "Sdhci card detect state not stable\n");
+			return -EIO;
+		}
+	}
+
 	return sdhci_probe(dev);
 }
 
-- 
2.17.1

[PATCH 7/7] mmc: zynq_sdhci: Use set_control_reg from sdhci.c

[Ashok Reddy Soma]

Since set_control_reg is available in sdhci.c, use it and remove
arasan_sdhci_set_control_reg().

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
---

 drivers/mmc/zynq_sdhci.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index a192f60320..93d453a284 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -600,29 +600,10 @@ static void arasan_dt_parse_clk_phases(struct udevice *dev)
 				 "clk-phase-mmc-hs400");
 }
 
-static void arasan_sdhci_set_control_reg(struct sdhci_host *host)
-{
-	struct mmc *mmc = (struct mmc *)host->mmc;
-	u32 reg;
-
-	if (!IS_SD(mmc))
-		return;
-
-	if (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);
-	}
-
-	if (mmc->selected_mode > SD_HS &&
-	    mmc->selected_mode <= MMC_HS_200)
-		sdhci_set_uhs_timing(host);
-}
-
 static const struct sdhci_ops arasan_ops = {
 	.platform_execute_tuning	= &arasan_sdhci_execute_tuning,
 	.set_delay = &arasan_sdhci_set_tapdelay,
-	.set_control_reg = &arasan_sdhci_set_control_reg,
+	.set_control_reg = &sdhci_set_control_reg,
 };
 #endif
 
-- 
2.17.1

Re: [PATCH 1/7] mmc: sdhci: Return error in case of failure

[Jaehoon Chung]

Hi Ashok,

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> 
> set_delay() function is from sdhci host ops, which does not return
> any error due to void return type. Get return values from input and
> output set clock phase functions inside arasan_sdhci_set_tapdelay()
> and return the errors.
> 
> Change return type to int for arasan_sdhci_set_tapdelay() and also for
> set_delay() in sdhci_ops structure.

Could you separate the patch to sdhci and zync_sdhci part?

> 
> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> ---
> 
>  drivers/mmc/sdhci.c      |  8 ++++++--
>  drivers/mmc/zynq_sdhci.c | 21 ++++++++++++++++-----
>  include/sdhci.h          |  2 +-
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index d9ab6a0a83..f144602eec 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -366,6 +366,7 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
>  {
>  	struct sdhci_host *host = mmc->priv;
>  	unsigned int div, clk = 0, timeout;
> +	int ret;
>  
>  	/* Wait max 20 ms */
>  	timeout = 200;
> @@ -386,8 +387,11 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
>  	if (clock == 0)
>  		return 0;
>  
> -	if (host->ops && host->ops->set_delay)
> -		host->ops->set_delay(host);
> +	if (host->ops && host->ops->set_delay) {
> +		ret = host->ops->set_delay(host);
> +		if (ret)
> +			return ret;

how about adding debug(). It's helpful to debug when it's failed. 

Best Regards,
Jaehoon Chung

> +	}
>  
>  	if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
>  		/*
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index ba87ee8dd5..9fb3603c7e 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -422,7 +422,7 @@ static int sdhci_versal_sampleclk_set_phase(struct sdhci_host *host,
>  	return 0;
>  }
>  
> -static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
> +static int arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>  {
>  	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
>  	struct arasan_sdhci_clk_data *clk_data = &priv->clk_data;
> @@ -431,18 +431,29 @@ static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>  	u8 timing = mode2timing[mmc->selected_mode];
>  	u32 iclk_phase = clk_data->clk_phase_in[timing];
>  	u32 oclk_phase = clk_data->clk_phase_out[timing];
> +	int ret;
>  
>  	dev_dbg(dev, "%s, host:%s, mode:%d\n", __func__, host->name, timing);
>  
>  	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
>  	    device_is_compatible(dev, "xlnx,zynqmp-8.9a")) {
> -		sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
> -		sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
> +		ret = sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
> +		if (ret)
> +			return ret;
> +		ret = sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
> +		if (ret)
> +			return ret;
>  	} else if (IS_ENABLED(CONFIG_ARCH_VERSAL) &&
>  		   device_is_compatible(dev, "xlnx,versal-8.9a")) {
> -		sdhci_versal_sampleclk_set_phase(host, iclk_phase);
> -		sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
> +		ret = sdhci_versal_sampleclk_set_phase(host, iclk_phase);
> +		if (ret)
> +			return ret;
> +		ret = sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
> +		if (ret)
> +			return ret;
>  	}
> +
> +	return 0;
>  }
>  
>  static void arasan_dt_read_clk_phase(struct udevice *dev, unsigned char timing,
> diff --git a/include/sdhci.h b/include/sdhci.h
> index 0ae9471ad7..44a0d84e5a 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -268,7 +268,7 @@ struct sdhci_ops {
>  	int	(*set_ios_post)(struct sdhci_host *host);
>  	void	(*set_clock)(struct sdhci_host *host, u32 div);
>  	int (*platform_execute_tuning)(struct mmc *host, u8 opcode);
> -	void (*set_delay)(struct sdhci_host *host);
> +	int (*set_delay)(struct sdhci_host *host);
>  	int	(*deferred_probe)(struct sdhci_host *host);
>  };
>  
> 

RE: [PATCH 1/7] mmc: sdhci: Return error in case of failure

[Ashok Reddy Soma]

Hi Jaehoon,

Thanks for the review.

> -----Original Message-----
> From: Jaehoon Chung <jh80.chung@samsung.com>
> Sent: Monday, July 26, 2021 3:18 AM
> To: Ashok Reddy Soma <ashokred@xilinx.com>; u-boot@lists.denx.de
> Cc: peng.fan@nxp.com; faiz_abbas@ti.com; sjg@chromium.org;
> michael@walle.cc; git <git@xilinx.com>; monstr@monstr.eu;
> somaashokreddy@gmail.com; T Karthik Reddy <tkarthik@xilinx.com>
> Subject: Re: [PATCH 1/7] mmc: sdhci: Return error in case of failure
> 
> Hi Ashok,
> 
> On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> > From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> >
> > set_delay() function is from sdhci host ops, which does not return any
> > error due to void return type. Get return values from input and output
> > set clock phase functions inside arasan_sdhci_set_tapdelay() and
> > return the errors.
> >
> > Change return type to int for arasan_sdhci_set_tapdelay() and also for
> > set_delay() in sdhci_ops structure.
> 
> Could you separate the patch to sdhci and zync_sdhci part?
Sure, i will split in to two patches.
> 
> >
> > Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> > ---
> >
> >  drivers/mmc/sdhci.c      |  8 ++++++--
> >  drivers/mmc/zynq_sdhci.c | 21 ++++++++++++++++-----
> >  include/sdhci.h          |  2 +-
> >  3 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index
> > d9ab6a0a83..f144602eec 100644
> > --- a/drivers/mmc/sdhci.c
> > +++ b/drivers/mmc/sdhci.c
> > @@ -366,6 +366,7 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
> > clock)  {
> >  	struct sdhci_host *host = mmc->priv;
> >  	unsigned int div, clk = 0, timeout;
> > +	int ret;
> >
> >  	/* Wait max 20 ms */
> >  	timeout = 200;
> > @@ -386,8 +387,11 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
> clock)
> >  	if (clock == 0)
> >  		return 0;
> >
> > -	if (host->ops && host->ops->set_delay)
> > -		host->ops->set_delay(host);
> > +	if (host->ops && host->ops->set_delay) {
> > +		ret = host->ops->set_delay(host);
> > +		if (ret)
> > +			return ret;
> 
> how about adding debug(). It's helpful to debug when it's failed.

Ok, I will add a debug print here.

Any comments for other patches in this series or shall I send V2 with these changes ?

Thanks,
Ashok
> 
> Best Regards,
> Jaehoon Chung
> 
> > +	}
> >
> >  	if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
> >  		/*
> > diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index
> > ba87ee8dd5..9fb3603c7e 100644
> > --- a/drivers/mmc/zynq_sdhci.c
> > +++ b/drivers/mmc/zynq_sdhci.c
> > @@ -422,7 +422,7 @@ static int sdhci_versal_sampleclk_set_phase(struct
> sdhci_host *host,
> >  	return 0;
> >  }
> >
> > -static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
> > +static int arasan_sdhci_set_tapdelay(struct sdhci_host *host)
> >  {
> >  	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
> >  	struct arasan_sdhci_clk_data *clk_data = &priv->clk_data; @@ -431,18
> > +431,29 @@ static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
> >  	u8 timing = mode2timing[mmc->selected_mode];
> >  	u32 iclk_phase = clk_data->clk_phase_in[timing];
> >  	u32 oclk_phase = clk_data->clk_phase_out[timing];
> > +	int ret;
> >
> >  	dev_dbg(dev, "%s, host:%s, mode:%d\n", __func__, host->name,
> > timing);
> >
> >  	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
> >  	    device_is_compatible(dev, "xlnx,zynqmp-8.9a")) {
> > -		sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
> > -		sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
> > +		ret = sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
> > +		if (ret)
> > +			return ret;
> > +		ret = sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
> > +		if (ret)
> > +			return ret;
> >  	} else if (IS_ENABLED(CONFIG_ARCH_VERSAL) &&
> >  		   device_is_compatible(dev, "xlnx,versal-8.9a")) {
> > -		sdhci_versal_sampleclk_set_phase(host, iclk_phase);
> > -		sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
> > +		ret = sdhci_versal_sampleclk_set_phase(host, iclk_phase);
> > +		if (ret)
> > +			return ret;
> > +		ret = sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
> > +		if (ret)
> > +			return ret;
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  static void arasan_dt_read_clk_phase(struct udevice *dev, unsigned
> > char timing, diff --git a/include/sdhci.h b/include/sdhci.h index
> > 0ae9471ad7..44a0d84e5a 100644
> > --- a/include/sdhci.h
> > +++ b/include/sdhci.h
> > @@ -268,7 +268,7 @@ struct sdhci_ops {
> >  	int	(*set_ios_post)(struct sdhci_host *host);
> >  	void	(*set_clock)(struct sdhci_host *host, u32 div);
> >  	int (*platform_execute_tuning)(struct mmc *host, u8 opcode);
> > -	void (*set_delay)(struct sdhci_host *host);
> > +	int (*set_delay)(struct sdhci_host *host);
> >  	int	(*deferred_probe)(struct sdhci_host *host);
> >  };
> >
> >

Re: [PATCH 1/7] mmc: sdhci: Return error in case of failure

[Jaehoon Chung]

Hi Ashok,

On 7/26/21 2:33 PM, Ashok Reddy Soma wrote:
> Hi Jaehoon,
> 
> Thanks for the review.
> 
>> -----Original Message-----
>> From: Jaehoon Chung <jh80.chung@samsung.com>
>> Sent: Monday, July 26, 2021 3:18 AM
>> To: Ashok Reddy Soma <ashokred@xilinx.com>; u-boot@lists.denx.de
>> Cc: peng.fan@nxp.com; faiz_abbas@ti.com; sjg@chromium.org;
>> michael@walle.cc; git <git@xilinx.com>; monstr@monstr.eu;
>> somaashokreddy@gmail.com; T Karthik Reddy <tkarthik@xilinx.com>
>> Subject: Re: [PATCH 1/7] mmc: sdhci: Return error in case of failure
>>
>> Hi Ashok,
>>
>> On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
>>> From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
>>>
>>> set_delay() function is from sdhci host ops, which does not return any
>>> error due to void return type. Get return values from input and output
>>> set clock phase functions inside arasan_sdhci_set_tapdelay() and
>>> return the errors.
>>>
>>> Change return type to int for arasan_sdhci_set_tapdelay() and also for
>>> set_delay() in sdhci_ops structure.
>>
>> Could you separate the patch to sdhci and zync_sdhci part?
> Sure, i will split in to two patches.
>>
>>>
>>> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>> ---
>>>
>>>  drivers/mmc/sdhci.c      |  8 ++++++--
>>>  drivers/mmc/zynq_sdhci.c | 21 ++++++++++++++++-----
>>>  include/sdhci.h          |  2 +-
>>>  3 files changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index
>>> d9ab6a0a83..f144602eec 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -366,6 +366,7 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
>>> clock)  {
>>>  	struct sdhci_host *host = mmc->priv;
>>>  	unsigned int div, clk = 0, timeout;
>>> +	int ret;
>>>
>>>  	/* Wait max 20 ms */
>>>  	timeout = 200;
>>> @@ -386,8 +387,11 @@ int sdhci_set_clock(struct mmc *mmc, unsigned int
>> clock)
>>>  	if (clock == 0)
>>>  		return 0;
>>>
>>> -	if (host->ops && host->ops->set_delay)
>>> -		host->ops->set_delay(host);
>>> +	if (host->ops && host->ops->set_delay) {
>>> +		ret = host->ops->set_delay(host);
>>> +		if (ret)
>>> +			return ret;
>>
>> how about adding debug(). It's helpful to debug when it's failed.
> 
> Ok, I will add a debug print here.
> 
> Any comments for other patches in this series or shall I send V2 with these changes ?

I didn't see other patch yet. Sorry.
After checked other patch, I will reply ASAP.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Ashok
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> +	}
>>>
>>>  	if (SDHCI_GET_VERSION(host) >= SDHCI_SPEC_300) {
>>>  		/*
>>> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index
>>> ba87ee8dd5..9fb3603c7e 100644
>>> --- a/drivers/mmc/zynq_sdhci.c
>>> +++ b/drivers/mmc/zynq_sdhci.c
>>> @@ -422,7 +422,7 @@ static int sdhci_versal_sampleclk_set_phase(struct
>> sdhci_host *host,
>>>  	return 0;
>>>  }
>>>
>>> -static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>> +static int arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>>  {
>>>  	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
>>>  	struct arasan_sdhci_clk_data *clk_data = &priv->clk_data; @@ -431,18
>>> +431,29 @@ static void arasan_sdhci_set_tapdelay(struct sdhci_host *host)
>>>  	u8 timing = mode2timing[mmc->selected_mode];
>>>  	u32 iclk_phase = clk_data->clk_phase_in[timing];
>>>  	u32 oclk_phase = clk_data->clk_phase_out[timing];
>>> +	int ret;
>>>
>>>  	dev_dbg(dev, "%s, host:%s, mode:%d\n", __func__, host->name,
>>> timing);
>>>
>>>  	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
>>>  	    device_is_compatible(dev, "xlnx,zynqmp-8.9a")) {
>>> -		sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
>>> -		sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
>>> +		ret = sdhci_zynqmp_sampleclk_set_phase(host, iclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = sdhci_zynqmp_sdcardclk_set_phase(host, oclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>>  	} else if (IS_ENABLED(CONFIG_ARCH_VERSAL) &&
>>>  		   device_is_compatible(dev, "xlnx,versal-8.9a")) {
>>> -		sdhci_versal_sampleclk_set_phase(host, iclk_phase);
>>> -		sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
>>> +		ret = sdhci_versal_sampleclk_set_phase(host, iclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = sdhci_versal_sdcardclk_set_phase(host, oclk_phase);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>> +
>>> +	return 0;
>>>  }
>>>
>>>  static void arasan_dt_read_clk_phase(struct udevice *dev, unsigned
>>> char timing, diff --git a/include/sdhci.h b/include/sdhci.h index
>>> 0ae9471ad7..44a0d84e5a 100644
>>> --- a/include/sdhci.h
>>> +++ b/include/sdhci.h
>>> @@ -268,7 +268,7 @@ struct sdhci_ops {
>>>  	int	(*set_ios_post)(struct sdhci_host *host);
>>>  	void	(*set_clock)(struct sdhci_host *host, u32 div);
>>>  	int (*platform_execute_tuning)(struct mmc *host, u8 opcode);
>>> -	void (*set_delay)(struct sdhci_host *host);
>>> +	int (*set_delay)(struct sdhci_host *host);
>>>  	int	(*deferred_probe)(struct sdhci_host *host);
>>>  };
>>>
>>>
> 

Re: [PATCH 3/7] mmc: zynq_sdhci: Use xilinx pm request instead of mmio_write

[Jaehoon Chung]

Hi,

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> Currently xilinx sdhci driver is using zynqmp_mmio_write() to set
> tapdelay values. Use xilinx_pm_request() using appropriate arguments
> to set input/output tapdelays for zynqmp. Where tapdelay setting is
> done by firmware. Host driver should explicitly request DLL reset
> before ITAP (assert DLL) and after OTAP (release DLL) to avoid issues
> in some cases. Also handle error return where possible.
> 
> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> ---
> 
>  board/xilinx/zynqmp/tap_delays.c | 89 +++-----------------------------
>  drivers/mmc/zynq_sdhci.c         | 72 +++++++++++++++++++++-----
>  include/zynqmp_tap_delay.h       | 25 ++++++---
>  3 files changed, 84 insertions(+), 102 deletions(-)

Doen it impossible to separate the board specific patch and mmc patch?

> 
> diff --git a/board/xilinx/zynqmp/tap_delays.c b/board/xilinx/zynqmp/tap_delays.c
> index d16bbb8eff..4ce2244060 100644
> --- a/board/xilinx/zynqmp/tap_delays.c
> +++ b/board/xilinx/zynqmp/tap_delays.c
> @@ -10,92 +10,17 @@
>  #include <asm/arch/sys_proto.h>
>  #include <linux/delay.h>
>  #include <mmc.h>
> +#include <zynqmp_firmware.h>
>  
> -#define SD_DLL_CTRL			0xFF180358
> -#define SD_ITAP_DLY			0xFF180314
> -#define SD_OTAP_DLY			0xFF180318
> -#define SD0_DLL_RST_MASK		0x00000004
> -#define SD0_DLL_RST			0x00000004
> -#define SD1_DLL_RST_MASK		0x00040000
> -#define SD1_DLL_RST			0x00040000
> -#define SD0_ITAPCHGWIN_MASK		0x00000200
> -#define SD0_ITAPCHGWIN			0x00000200
> -#define SD1_ITAPCHGWIN_MASK		0x02000000
> -#define SD1_ITAPCHGWIN			0x02000000
> -#define SD0_ITAPDLYENA_MASK		0x00000100
> -#define SD0_ITAPDLYENA			0x00000100
> -#define SD1_ITAPDLYENA_MASK		0x01000000
> -#define SD1_ITAPDLYENA			0x01000000
> -#define SD0_ITAPDLYSEL_MASK		0x000000FF
> -#define SD1_ITAPDLYSEL_MASK		0x00FF0000
> -#define SD0_OTAPDLYSEL_MASK		0x0000003F
> -#define SD1_OTAPDLYSEL_MASK		0x003F0000
> -
> -void zynqmp_dll_reset(u8 deviceid)
> +int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type, u32 itap_delay)
>  {
> -	/* Issue DLL Reset */
> -	if (deviceid == 0)
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK,
> -				  SD0_DLL_RST);
> -	else
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK,
> -				  SD1_DLL_RST);
> -
> -	mdelay(1);
>  
> -	/* Release DLL Reset */
> -	if (deviceid == 0)
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> -	else
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> +	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +			  type, itap_delay, NULL);
>  }
>  
> -void arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 itap_delay)
> +int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type, u32 otap_delay)
>  {
> -	if (deviceid == 0) {
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
> -
> -		/* Program ITAP delay */
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK,
> -				  SD0_ITAPCHGWIN);
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYENA_MASK,
> -				  SD0_ITAPDLYENA);
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYSEL_MASK, itap_delay);
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK, 0x0);
> -
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> -	} else {
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
> -
> -		/* Program ITAP delay */
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK,
> -				  SD1_ITAPCHGWIN);
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYENA_MASK,
> -				  SD1_ITAPDLYENA);
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYSEL_MASK,
> -				  (itap_delay << 16));
> -		zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK, 0x0);
> -
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> -	}
> -}
> -
> -void arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 otap_delay)
> -{
> -	if (deviceid == 0) {
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
> -
> -		/* Program OTAP delay */
> -		zynqmp_mmio_write(SD_OTAP_DLY, SD0_OTAPDLYSEL_MASK, otap_delay);
> -
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> -	} else {
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
> -
> -		/* Program OTAP delay */
> -		zynqmp_mmio_write(SD_OTAP_DLY, SD1_OTAPDLYSEL_MASK,
> -				  (otap_delay << 16));
> -
> -		zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> -	}
> +	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +			  type, otap_delay, NULL);
>  }
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 9fb3603c7e..4dafd47b57 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -18,6 +18,7 @@
>  #include <malloc.h>
>  #include <sdhci.h>
>  #include <zynqmp_tap_delay.h>
> +#include <zynqmp_firmware.h>
>  
>  #define SDHCI_ARASAN_ITAPDLY_REGISTER	0xF0F8
>  #define SDHCI_ARASAN_ITAPDLY_SEL_MASK	GENMASK(7, 0)
> @@ -75,26 +76,39 @@ static const u8 mode2timing[] = {
>  	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
>  };
>  
> -static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
> +static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
>  {
> -	u16 clk;
> +	struct mmc *mmc = (struct mmc *)host->mmc;
> +	struct udevice *dev = mmc->dev;
>  	unsigned long timeout;
> +	int ret;
> +	u16 clk;
>  
>  	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>  	clk &= ~(SDHCI_CLOCK_CARD_EN);
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  
>  	/* Issue DLL Reset */
> -	zynqmp_dll_reset(deviceid);
> +	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_ASSERT);
> +	if (ret) {
> +		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mdelay(1);

If add some comments about this, it's more helpful.

> +	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_RELEASE);
> +	if (ret) {
> +		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
> +		return ret;
> +	}
>  
>  	/* Wait max 20 ms */
>  	timeout = 100;
>  	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>  				& SDHCI_CLOCK_INT_STABLE)) {
>  		if (timeout == 0) {
> -			dev_err(mmc_dev(host->mmc),
> -				": Internal clock never stabilised.\n");
> -			return;
> +			dev_err(dev, ": Internal clock never stabilised.\n");
> +			return -EBUSY;
>  		}
>  		timeout--;
>  		udelay(1000);
> @@ -102,6 +116,8 @@ static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
>  
>  	clk |= SDHCI_CLOCK_CARD_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +	return 0;
>  }
>  
>  static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
> @@ -109,15 +125,14 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	struct mmc_cmd cmd;
>  	struct mmc_data data;
>  	u32 ctrl;
> +	u8 node_id = mmc->dev->seq_ ? NODE_SD_0 : NODE_SD_1;

Does it need to get a node_id value from mmc->dev->seq_ at everytime?
And priv->deviceid doesn't use anywhere. Can't it reuse?

Best Regards,
Jaehoon Chung

>  	struct sdhci_host *host;
>  	struct arasan_sdhci_priv *priv = dev_get_priv(mmc->dev);
>  	char tuning_loop_counter = SDHCI_TUNING_LOOP_COUNT;
> -	u8 deviceid;
>  
>  	debug("%s\n", __func__);
>  
>  	host = priv->host;
> -	deviceid = priv->deviceid;
>  
>  	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>  	ctrl |= SDHCI_CTRL_EXEC_TUNING;
> @@ -125,7 +140,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  
>  	mdelay(1);
>  
> -	arasan_zynqmp_dll_reset(host, deviceid);
> +	arasan_zynqmp_dll_reset(host, node_id);
>  
>  	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
>  	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
> @@ -171,7 +186,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	}
>  
>  	udelay(1);
> -	arasan_zynqmp_dll_reset(host, deviceid);
> +	arasan_zynqmp_dll_reset(host, node_id);
>  
>  	/* Enable only interrupts served by the SD controller */
>  	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
> @@ -194,10 +209,12 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
>  					    int degrees)
>  {
> -	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
>  	struct mmc *mmc = (struct mmc *)host->mmc;
> +	struct udevice *dev = mmc->dev;
> +	u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
>  	u8 tap_delay, tap_max = 0;
>  	int timing = mode2timing[mmc->selected_mode];
> +	int ret;
>  
>  	/*
>  	 * This is applicable for SDHCI_SPEC_300 and above
> @@ -233,7 +250,20 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
>  	/* Limit output tap_delay value to 6 bits */
>  	tap_delay &= SDHCI_ARASAN_OTAPDLY_SEL_MASK;
>  
> -	arasan_zynqmp_set_out_tapdelay(priv->deviceid, tap_delay);
> +	/* Set the Clock Phase */
> +	ret = arasan_zynqmp_set_out_tapdelay(node_id,
> +					     PM_TAPDELAY_OUTPUT, tap_delay);
> +	if (ret) {
> +		dev_err(dev, "Error setting output Tap Delay\n");
> +		return ret;
> +	}
> +
> +	/* Release DLL Reset */
> +	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
> +	if (ret) {
> +		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
> +		return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -250,10 +280,12 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
>  static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
>  					    int degrees)
>  {
> -	struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
>  	struct mmc *mmc = (struct mmc *)host->mmc;
> +	struct udevice *dev = mmc->dev;
> +	u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
>  	u8 tap_delay, tap_max = 0;
>  	int timing = mode2timing[mmc->selected_mode];
> +	int ret;
>  
>  	/*
>  	 * This is applicable for SDHCI_SPEC_300 and above
> @@ -263,6 +295,13 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
>  	if (SDHCI_GET_VERSION(host) < SDHCI_SPEC_300)
>  		return 0;
>  
> +	/* Assert DLL Reset */
> +	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
> +	if (ret) {
> +		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
> +		return ret;
> +	}
> +
>  	switch (timing) {
>  	case MMC_TIMING_MMC_HS:
>  	case MMC_TIMING_SD_HS:
> @@ -289,7 +328,12 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
>  	/* Limit input tap_delay value to 8 bits */
>  	tap_delay &= SDHCI_ARASAN_ITAPDLY_SEL_MASK;
>  
> -	arasan_zynqmp_set_in_tapdelay(priv->deviceid, tap_delay);
> +	ret = arasan_zynqmp_set_in_tapdelay(node_id,
> +					    PM_TAPDELAY_INPUT, tap_delay);
> +	if (ret) {
> +		dev_err(dev, "Error setting Input Tap Delay\n");
> +		return ret;
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/zynqmp_tap_delay.h b/include/zynqmp_tap_delay.h
> index 1c1e3e7dee..7e4d4e4a9a 100644
> --- a/include/zynqmp_tap_delay.h
> +++ b/include/zynqmp_tap_delay.h
> @@ -8,14 +8,27 @@
>  #ifndef __ZYNQMP_TAP_DELAY_H__
>  #define __ZYNQMP_TAP_DELAY_H__
>  
> +#include <zynqmp_firmware.h>
> +
>  #ifdef CONFIG_ARCH_ZYNQMP
> -void zynqmp_dll_reset(u8 deviceid);
> -void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay);
> -void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay);
> +int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay);
> +int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay);
>  #else
> -inline void zynqmp_dll_reset(u8 deviceid) {}
> -inline void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay) {}
> -inline void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay) {}
> +inline int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay)
> +{
> +	return 0;
> +}
> +
> +int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay)
> +{
> +	return 0;
> +}
>  #endif
>  
> +static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
> +{
> +	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
> +				 type, 0, NULL);
> +}
> +
>  #endif
> 

Re: [PATCH 4/7] mmc: zynq_sdhci: Move setting tapdelay code to driver

[Jaehoon Chung]

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> tap_delays.c just has calls to xilinx_pm_request() for setting tapdelays.
> Simply move these calls to zynq_sdhci.c and make them static inline.
> Similarly zynqmp_tap_delay.h also has call to xilinx_pm_request() for
> dll reset. Do the same for this file as well.
> 
> Remove tap_delays.c and zynqmp_tap_delay.h files.
> 
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  board/xilinx/zynqmp/Makefile     |  2 --
>  board/xilinx/zynqmp/tap_delays.c | 26 ------------------------
>  drivers/mmc/zynq_sdhci.c         | 21 +++++++++++++++++++-
>  include/zynqmp_tap_delay.h       | 34 --------------------------------
>  4 files changed, 20 insertions(+), 63 deletions(-)
>  delete mode 100644 board/xilinx/zynqmp/tap_delays.c
>  delete mode 100644 include/zynqmp_tap_delay.h
> 
> diff --git a/board/xilinx/zynqmp/Makefile b/board/xilinx/zynqmp/Makefile
> index 7d8277ca40..a914028753 100644
> --- a/board/xilinx/zynqmp/Makefile
> +++ b/board/xilinx/zynqmp/Makefile
> @@ -44,8 +44,6 @@ $(obj)/pm_cfg_obj.o: $(shell cd $(srctree); readlink -f $(CONFIG_ZYNQMP_SPL_PM_C
>  endif
>  endif
>  
> -obj-$(CONFIG_MMC_SDHCI_ZYNQ) += tap_delays.o
> -
>  ifndef CONFIG_SPL_BUILD
>  obj-$(CONFIG_CMD_ZYNQMP) += cmds.o
>  endif
> diff --git a/board/xilinx/zynqmp/tap_delays.c b/board/xilinx/zynqmp/tap_delays.c
> deleted file mode 100644
> index 4ce2244060..0000000000
> --- a/board/xilinx/zynqmp/tap_delays.c
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Xilinx ZynqMP SoC Tap Delay Programming
> - *
> - * Copyright (C) 2018 Xilinx, Inc.
> - */
> -
> -#include <common.h>
> -#include <zynqmp_tap_delay.h>
> -#include <asm/arch/sys_proto.h>
> -#include <linux/delay.h>
> -#include <mmc.h>
> -#include <zynqmp_firmware.h>
> -
> -int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type, u32 itap_delay)
> -{
> -
> -	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> -			  type, itap_delay, NULL);
> -}
> -
> -int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type, u32 otap_delay)
> -{
> -	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> -			  type, otap_delay, NULL);
> -}
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 4dafd47b57..97caca51e2 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -17,7 +17,6 @@
>  #include <linux/libfdt.h>
>  #include <malloc.h>
>  #include <sdhci.h>
> -#include <zynqmp_tap_delay.h>
>  #include <zynqmp_firmware.h>
>  
>  #define SDHCI_ARASAN_ITAPDLY_REGISTER	0xF0F8
> @@ -76,6 +75,26 @@ static const u8 mode2timing[] = {
>  	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
>  };
>  
> +static inline int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type,
> +						u32 itap_delay)
> +{
> +	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +				 type, itap_delay, NULL);
> +}
> +
> +static inline int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type,
> +						 u32 otap_delay)
> +{
> +	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +				 type, otap_delay, NULL);
> +}
> +
> +static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
> +{
> +	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
> +				 type, 0, NULL);
> +}
> +
>  static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
>  {
>  	struct mmc *mmc = (struct mmc *)host->mmc;
> diff --git a/include/zynqmp_tap_delay.h b/include/zynqmp_tap_delay.h
> deleted file mode 100644
> index 7e4d4e4a9a..0000000000
> --- a/include/zynqmp_tap_delay.h
> +++ /dev/null
> @@ -1,34 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * Xilinx ZynqMP SoC Tap Delay Programming
> - *
> - * Copyright (C) 2018 Xilinx, Inc.
> - */
> -
> -#ifndef __ZYNQMP_TAP_DELAY_H__
> -#define __ZYNQMP_TAP_DELAY_H__
> -
> -#include <zynqmp_firmware.h>
> -
> -#ifdef CONFIG_ARCH_ZYNQMP
> -int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay);
> -int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay);
> -#else
> -inline int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay)
> -{
> -	return 0;
> -}
> -
> -int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay)
> -{
> -	return 0;
> -}
> -#endif
> -
> -static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
> -{
> -	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
> -				 type, 0, NULL);
> -}
> -
> -#endif
> 

Re: [PATCH 5/7] mmc: zynq_sdhci: Change variable deviceid to node_id

[Jaehoon Chung]

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> Change deviceid to node_id in arasan_zynqmp_dll_reset() and also in
> tapdelay related static inline functions to reflect proper name and
> for consistency.
> 
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  drivers/mmc/zynq_sdhci.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 97caca51e2..8ffa8c1269 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -75,17 +75,17 @@ static const u8 mode2timing[] = {
>  	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
>  };
>  
> -static inline int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type,
> +static inline int arasan_zynqmp_set_in_tapdelay(u8 node_id, u32 type,
>  						u32 itap_delay)
>  {
> -	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SET_SD_TAPDELAY,
>  				 type, itap_delay, NULL);
>  }
>  
> -static inline int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type,
> +static inline int arasan_zynqmp_set_out_tapdelay(u8 node_id, u32 type,
>  						 u32 otap_delay)
>  {
> -	return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> +	return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SET_SD_TAPDELAY,
>  				 type, otap_delay, NULL);
>  }
>  
> @@ -95,7 +95,7 @@ static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
>  				 type, 0, NULL);
>  }
>  
> -static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
> +static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 node_id)
>  {
>  	struct mmc *mmc = (struct mmc *)host->mmc;
>  	struct udevice *dev = mmc->dev;
> @@ -108,14 +108,14 @@ static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  
>  	/* Issue DLL Reset */
> -	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_ASSERT);
> +	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
>  	if (ret) {
>  		dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
>  		return ret;
>  	}
>  
>  	mdelay(1);
> -	ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_RELEASE);
> +	ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
>  	if (ret) {
>  		dev_err(dev, "dll_reset release failed with err: %d\n", ret);
>  		return ret;
> 

Re: [PATCH 6/7] mmc: zynq_sdhci: Wait till sd card detect state is stable

[Jaehoon Chung]

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> From: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> 
> As per SD spec when SD host controller is reset, it takes 1000msec
> to detect the card state. In case, if we enable the sd bus voltage &
> card detect state is not stable, then host controller will disable
> the sd bus voltage.
> 
> In case of warm/subsystem reboot, due to unstable card detect state
> host controller is disabling the sd bus voltage to sd card causing
> sd card timeout error. So we wait for a maximum of 1000msec to get
> the card detect state stable before we enable the sd bus voltage.
> 
> This current fix is workaround for now, this needs to be analysed
> further. Zynqmp platform should behave the same as Versal, but we
> did not encounter this issue as of now. So we are fixing it for
> Versal only.
> 
> Signed-off-by: T Karthik Reddy <t.karthik.reddy@xilinx.com>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> ---
> 
>  drivers/mmc/zynq_sdhci.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 8ffa8c1269..a192f60320 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -686,6 +686,23 @@ static int arasan_sdhci_probe(struct udevice *dev)
>  		return ret;
>  	upriv->mmc = host->mmc;
>  
> +	/*
> +	 * Wait for 1000msec till the card detect state gets stable
> +	 * else host controller will set sd power bus voltage to 0.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARCH_VERSAL)) {
> +		u32 timeout = 1000;
> +
> +		while (((sdhci_readl(host, SDHCI_PRESENT_STATE) &
> +			 SDHCI_CARD_STATE_STABLE) == 0) && timeout--) {
> +			mdelay(1);
> +		}
> +		if (!timeout) {
> +			dev_err(dev, "Sdhci card detect state not stable\n");
> +			return -EIO;

-EIO is not correct, -ETIMEDOUT or -EBUSY?

Best Regards,
Jaehoon Chung

> +		}
> +	}
> +
>  	return sdhci_probe(dev);
>  }
>  
> 

Re: [PATCH 7/7] mmc: zynq_sdhci: Use set_control_reg from sdhci.c

[Jaehoon Chung]

On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> Since set_control_reg is available in sdhci.c, use it and remove
> arasan_sdhci_set_control_reg().
> 
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung


> ---
> 
>  drivers/mmc/zynq_sdhci.c | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index a192f60320..93d453a284 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -600,29 +600,10 @@ static void arasan_dt_parse_clk_phases(struct udevice *dev)
>  				 "clk-phase-mmc-hs400");
>  }
>  
> -static void arasan_sdhci_set_control_reg(struct sdhci_host *host)
> -{
> -	struct mmc *mmc = (struct mmc *)host->mmc;
> -	u32 reg;
> -
> -	if (!IS_SD(mmc))
> -		return;
> -
> -	if (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);
> -	}
> -
> -	if (mmc->selected_mode > SD_HS &&
> -	    mmc->selected_mode <= MMC_HS_200)
> -		sdhci_set_uhs_timing(host);
> -}
> -
>  static const struct sdhci_ops arasan_ops = {
>  	.platform_execute_tuning	= &arasan_sdhci_execute_tuning,
>  	.set_delay = &arasan_sdhci_set_tapdelay,
> -	.set_control_reg = &arasan_sdhci_set_control_reg,
> +	.set_control_reg = &sdhci_set_control_reg,
>  };
>  #endif
>  
>