[PATCH v2 0/4] mmc: sdhci-msm: Corrections to implementation of power irq

[PATCH v2 0/4] mmc: sdhci-msm: Corrections to implementation of power irq

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus on/off
require sdhc controller to be ready before progressing further. Once a
register write which affects IO lines is done, the driver should wait for
power irq from controller. Once the irq comes, the driver should acknowledge
the irq by writing to power control register. If the acknowledgement is not
given to controller, the controller may not complete the corresponding
register write action and this can mess up the controller if drivers proceeds
without power irq completing.

Changes since v1:
	Patch enabling MMC_IO_ACCESSORS in Kconfig moved up the patch list.
	Also corrected a mistake in the patch.
	Removed all ifdef CONFIG_MMC_IO_ACCESSORS since the patches using it
	will come after MMC_IO_ACCESSORS are enabled.
	Merged the patches 3 & 4 of earlier series into 1 patch (patch 4 in
	current series).
	Corrected a mistake in a comment text in patch 3 of previous series. 

Changes since RFC:
	wait_for_completion_timeout replaced with wait_event_timeout when
	waiting for power irq.
	Removed the spinlock within power irq handler and API which waits
	for power irq.
	Added comments to sdhci msm register write functions, warning that they
	can sleep.
	Sdhci msm register write functions will do a memory barrier before
	writing to the register if the particular register can trigger
	power irq.
	Instead of enabling SDHCI IO ACCESSORS config in arm64/defconfig, it
	will be selected in mmc/host/Kconfig if the platform is MMC_SDHCI_MSM.

Sahitya Tummala (1):
  mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

Subhash Jadavani (1):
  mmc: sdhci-msm: fix issue with power irq

Vijay Viswanath (2):
  mmc: Kconfig: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS
  mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr
    irq

 drivers/mmc/host/Kconfig     |   1 +
 drivers/mmc/host/sdhci-msm.c | 235 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 231 insertions(+), 5 deletions(-)

-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/4] mmc: sdhci-msm: fix issue with power irq

[Vijay Viswanath]

From: Subhash Jadavani <subhashj@codeaurora.org>

SDCC controller reset (SW_RST) during probe may trigger power irq if
previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
enable the power irq interrupt in GIC (by registering the interrupt
handler), we need to ensure that any pending power irq interrupt status
is acknowledged otherwise power irq interrupt handler would be fired
prematurely.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 9d601dc..d636251 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1250,6 +1250,21 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 			       CORE_VENDOR_SPEC_CAPABILITIES0);
 	}
 
+	/*
+	 * Power on reset state may trigger power irq if previous status of
+	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
+	 * interrupt in GIC, any pending power irq interrupt should be
+	 * acknowledged. Otherwise power irq interrupt handler would be
+	 * fired prematurely.
+	 */
+	sdhci_msm_voltage_switch(host);
+
+	/*
+	 * Ensure that above writes are propogated before interrupt enablement
+	 * in GIC.
+	 */
+	mb();
+
 	/* Setup IRQ for handling power/voltage tasks with PMIC */
 	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
 	if (msm_host->pwr_irq < 0) {
@@ -1259,6 +1274,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	/* Enable pwr irq interrupts */
+	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
+
 	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
 					sdhci_msm_pwr_irq, IRQF_ONESHOT,
 					dev_name(&pdev->dev), host);
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/4] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Vijay Viswanath]

From: Sahitya Tummala <stummala@codeaurora.org>

There is a rare scenario in HW, where the first clear pulse could
be lost when the actual reset and clear/read of status register
are happening at the same time. Fix this by retrying upto 10 times
to ensure the status register gets cleared. Otherwise, this will
lead to a spurious power IRQ which results in system instability.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 46 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index d636251..42a65ab 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -995,17 +995,52 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
-static void sdhci_msm_voltage_switch(struct sdhci_host *host)
+static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
+			mmc_hostname(host->mmc),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
+}
+
+static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	u32 irq_status, irq_ack = 0;
+	int retry = 10;
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
 
 	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
 
+	/*
+	 * There is a rare HW scenario where the first clear pulse could be
+	 * lost when actual reset and clear/read of status register is
+	 * happening at a time. Hence, retry for at least 10 times to make
+	 * sure status register is cleared. Otherwise, this will result in
+	 * a spurious power IRQ resulting in system instability.
+	 */
+	while (irq_status & readl_relaxed(msm_host->core_mem +
+				CORE_PWRCTL_STATUS)) {
+		if (retry == 0) {
+			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
+					mmc_hostname(host->mmc), irq_status);
+			sdhci_msm_dump_pwr_ctrl_regs(host);
+			WARN_ON(1);
+			break;
+		}
+		writel_relaxed(irq_status,
+				msm_host->core_mem + CORE_PWRCTL_CLEAR);
+		retry--;
+		udelay(10);
+	}
+
 	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
 	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
@@ -1017,13 +1052,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
 	 * switches are handled by the sdhci core, so just report success.
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
+
+	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
+		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
+		irq_ack);
 }
 
 static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
 {
 	struct sdhci_host *host = (struct sdhci_host *)data;
 
-	sdhci_msm_voltage_switch(host);
+	sdhci_msm_handle_pwr_irq(host, irq);
 
 	return IRQ_HANDLED;
 }
@@ -1106,7 +1145,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
-	.voltage_switch = sdhci_msm_voltage_switch,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -1257,7 +1295,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	 * acknowledged. Otherwise power irq interrupt handler would be
 	 * fired prematurely.
 	 */
-	sdhci_msm_voltage_switch(host);
+	sdhci_msm_handle_pwr_irq(host, 0);
 
 	/*
 	 * Ensure that above writes are propogated before interrupt enablement
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 3/4] mmc: Kconfig: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
register read and write APIs, if registered, can be used.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2db84dd..11bfb82 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -420,6 +420,7 @@ config MMC_SDHCI_MSM
 	tristate "Qualcomm SDHCI Controller Support"
 	depends on ARCH_QCOM || (ARM && COMPILE_TEST)
 	depends on MMC_SDHCI_PLTFM
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the Secure Digital Host Controller Interface (SDHCI)
 	  support present in Qualcomm SOCs. The controller supports
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus
on/off require controller to be ready before progressing further. When
the controller is ready, it will generate a power irq which needs to be
handled. The thread which initiated the register write should wait for
power irq to complete. This will be done through the new sdhc msm write
APIs which will check whether the particular write can trigger a power
irq and wait for it with a timeout if it is expected.
The SDHC core power control IRQ gets triggered when -
* There is a state change in power control bit (bit 0)
  of SDHCI_POWER_CONTROL register.
* There is a state change in 1.8V enable bit (bit 3) of
  SDHCI_HOST_CONTROL2 register.
* Bit 1 of SDHCI_SOFTWARE_RESET is set.

Also add support APIs which are used by sdhc msm write APIs to check
if power irq is expected to be generated and wait for the power irq
to come and complete if the irq is expected.

This patch requires CONFIG_MMC_SDHCI_IO_ACCESSORS to be enabled.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 173 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 42a65ab..b72a487 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -123,6 +123,10 @@
 #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
 
 #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
+
+/* Timeout value to avoid infinite waiting for pwr_irq */
+#define MSM_PWR_IRQ_TIMEOUT_MS 5000
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -138,6 +142,10 @@ struct sdhci_msm_host {
 	bool calibration_done;
 	u8 saved_tuning_phase;
 	bool use_cdclp533;
+	u32 curr_pwr_state;
+	u32 curr_io_level;
+	wait_queue_head_t pwr_irq_wait;
+	bool pwr_irq_flag;
 };
 
 static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
@@ -995,6 +1003,73 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
+static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
+{
+	init_waitqueue_head(&msm_host->pwr_irq_wait);
+}
+
+static inline void sdhci_msm_complete_pwr_irq_wait(
+		struct sdhci_msm_host *msm_host)
+{
+	wake_up(&msm_host->pwr_irq_wait);
+}
+
+/*
+ * sdhci_msm_check_power_status API should be called when registers writes
+ * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
+ * To what state the register writes will change the IO lines should be passed
+ * as the argument req_type. This API will check whether the IO line's state
+ * is already the expected state and will wait for power irq only if
+ * power irq is expected to be trigerred based on the current IO line state
+ * and expected IO line state.
+ */
+static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	bool done = false;
+
+	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
+			mmc_hostname(host->mmc), __func__, req_type,
+			msm_host->curr_pwr_state, msm_host->curr_io_level);
+
+	/*
+	 * The IRQ for request type IO High/LOW will be generated when -
+	 * there is a state change in 1.8V enable bit (bit 3) of
+	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
+	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
+	 * to set it to 3.3V before card detection happens, the
+	 * IRQ doesn't get triggered as there is no state change in this bit.
+	 * The driver already handles this case by changing the IO voltage
+	 * level to high as part of controller power up sequence. Hence, check
+	 * for host->pwr to handle a case where IO voltage high request is
+	 * issued even before controller power up.
+	 */
+	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
+		pr_debug("%s: do not wait for power IRQ that never comes, req_type: %d\n",
+				mmc_hostname(host->mmc), req_type);
+		return;
+	}
+	if ((req_type & msm_host->curr_pwr_state) ||
+			(req_type & msm_host->curr_io_level))
+		done = true;
+	/*
+	 * This is needed here to handle cases where register writes will
+	 * not change the current bus state or io level of the controller.
+	 * In this case, no power irq will be triggerred and we should
+	 * not wait.
+	 */
+	if (!done) {
+		if (!wait_event_timeout(msm_host->pwr_irq_wait,
+				msm_host->pwr_irq_flag,
+				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
+			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
+					mmc_hostname(host->mmc), req_type);
+	}
+	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
+			__func__, req_type);
+}
+
 static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1013,6 +1088,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	u32 irq_status, irq_ack = 0;
 	int retry = 10;
+	int pwr_state = 0, io_level = 0;
+
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
@@ -1041,10 +1118,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		udelay(10);
 	}
 
-	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+	/* Handle BUS ON/OFF*/
+	if (irq_status & CORE_PWRCTL_BUS_ON) {
+		pwr_state = REQ_BUS_ON;
+		io_level = REQ_IO_HIGH;
+		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+	}
+	if (irq_status & CORE_PWRCTL_BUS_OFF) {
+		pwr_state = REQ_BUS_OFF;
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
-	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
+	}
+	/* Handle IO LOW/HIGH */
+	if (irq_status & CORE_PWRCTL_IO_LOW) {
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
+	if (irq_status & CORE_PWRCTL_IO_HIGH) {
+		io_level = REQ_IO_HIGH;
+		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
 
 	/*
 	 * The driver has to acknowledge the interrupt, switch voltages and
@@ -1053,6 +1146,11 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
 
+	if (pwr_state)
+		msm_host->curr_pwr_state = pwr_state;
+	if (io_level)
+		msm_host->curr_io_level = io_level;
+
 	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
 		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
 		irq_ack);
@@ -1061,8 +1159,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
 {
 	struct sdhci_host *host = (struct sdhci_host *)data;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 
 	sdhci_msm_handle_pwr_irq(host, irq);
+	msm_host->pwr_irq_flag = 1;
+	sdhci_msm_complete_pwr_irq_wait(msm_host);
+
 
 	return IRQ_HANDLED;
 }
@@ -1131,6 +1234,69 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	__sdhci_msm_set_clock(host, clock);
 }
 
+/*
+ * Platform specific register write functions. This is so that, if any
+ * register write needs to be followed up by platform specific actions,
+ * they can be added here. These functions can go to sleep when writes
+ * to certain registers are done.
+ * These functions are relying on sdhci_set_ios not using spinlock.
+ */
+static int __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	u32 req_type = 0;
+
+	switch (reg) {
+	case SDHCI_HOST_CONTROL2:
+		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
+			REQ_IO_HIGH;
+		break;
+	case SDHCI_SOFTWARE_RESET:
+		if (host->pwr && (val & SDHCI_RESET_ALL))
+			req_type = REQ_BUS_OFF;
+		break;
+	case SDHCI_POWER_CONTROL:
+		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
+		break;
+	}
+
+	if (req_type) {
+		msm_host->pwr_irq_flag = 0;
+		/*
+		 * Since this register write may trigger a power irq, ensure
+		 * all previous register writes are complete by this point.
+		 */
+		mb();
+	}
+	return req_type;
+}
+
+/* This function may sleep*/
+static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	u32 req_type = 0;
+
+	req_type = __sdhci_msm_check_write(host, val, reg);
+	writew_relaxed(val, host->ioaddr + reg);
+
+	if (req_type)
+		sdhci_msm_check_power_status(host, req_type);
+}
+
+/* This function may sleep*/
+static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	u32 req_type = 0;
+
+	req_type = __sdhci_msm_check_write(host, val, reg);
+
+	writeb_relaxed(val, host->ioaddr + reg);
+
+	if (req_type)
+		sdhci_msm_check_power_status(host, req_type);
+}
+
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -1145,6 +1311,8 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
+	.write_w = sdhci_msm_writew,
+	.write_b = sdhci_msm_writeb,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -1312,6 +1480,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	sdhci_msm_init_pwr_irq_wait(msm_host);
 	/* Enable pwr irq interrupts */
 	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
 
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/4] mmc: sdhci-msm: fix issue with power irq

[Adrian Hunter]

On 27/09/17 08:34, Vijay Viswanath wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> SDCC controller reset (SW_RST) during probe may trigger power irq if
> previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
> enable the power irq interrupt in GIC (by registering the interrupt
> handler), we need to ensure that any pending power irq interrupt status
> is acknowledged otherwise power irq interrupt handler would be fired
> prematurely.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>

I already acked v1, nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-msm.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 9d601dc..d636251 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1250,6 +1250,21 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  			       CORE_VENDOR_SPEC_CAPABILITIES0);
>  	}
>  
> +	/*
> +	 * Power on reset state may trigger power irq if previous status of
> +	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
> +	 * interrupt in GIC, any pending power irq interrupt should be
> +	 * acknowledged. Otherwise power irq interrupt handler would be
> +	 * fired prematurely.
> +	 */
> +	sdhci_msm_voltage_switch(host);
> +
> +	/*
> +	 * Ensure that above writes are propogated before interrupt enablement
> +	 * in GIC.
> +	 */
> +	mb();
> +
>  	/* Setup IRQ for handling power/voltage tasks with PMIC */
>  	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>  	if (msm_host->pwr_irq < 0) {
> @@ -1259,6 +1274,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	/* Enable pwr irq interrupts */
> +	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>  					sdhci_msm_pwr_irq, IRQF_ONESHOT,
>  					dev_name(&pdev->dev), host);
> 

Re: [PATCH v2 2/4] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Adrian Hunter]

On 27/09/17 08:34, Vijay Viswanath wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> There is a rare scenario in HW, where the first clear pulse could
> be lost when the actual reset and clear/read of status register
> are happening at the same time. Fix this by retrying upto 10 times
> to ensure the status register gets cleared. Otherwise, this will
> lead to a spurious power IRQ which results in system instability.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>

I already acked v1, nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-msm.c | 46 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index d636251..42a65ab 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -995,17 +995,52 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> -static void sdhci_msm_voltage_switch(struct sdhci_host *host)
> +static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
> +			mmc_hostname(host->mmc),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
> +}
> +
> +static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>  	u32 irq_status, irq_ack = 0;
> +	int retry = 10;
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
>  
>  	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>  
> +	/*
> +	 * There is a rare HW scenario where the first clear pulse could be
> +	 * lost when actual reset and clear/read of status register is
> +	 * happening at a time. Hence, retry for at least 10 times to make
> +	 * sure status register is cleared. Otherwise, this will result in
> +	 * a spurious power IRQ resulting in system instability.
> +	 */
> +	while (irq_status & readl_relaxed(msm_host->core_mem +
> +				CORE_PWRCTL_STATUS)) {
> +		if (retry == 0) {
> +			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
> +					mmc_hostname(host->mmc), irq_status);
> +			sdhci_msm_dump_pwr_ctrl_regs(host);
> +			WARN_ON(1);
> +			break;
> +		}
> +		writel_relaxed(irq_status,
> +				msm_host->core_mem + CORE_PWRCTL_CLEAR);
> +		retry--;
> +		udelay(10);
> +	}
> +
>  	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>  	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> @@ -1017,13 +1052,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>  	 * switches are handled by the sdhci core, so just report success.
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
> +
> +	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
> +		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
> +		irq_ack);
>  }
>  
>  static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>  {
>  	struct sdhci_host *host = (struct sdhci_host *)data;
>  
> -	sdhci_msm_voltage_switch(host);
> +	sdhci_msm_handle_pwr_irq(host, irq);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1106,7 +1145,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> -	.voltage_switch = sdhci_msm_voltage_switch,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> @@ -1257,7 +1295,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	 * acknowledged. Otherwise power irq interrupt handler would be
>  	 * fired prematurely.
>  	 */
> -	sdhci_msm_voltage_switch(host);
> +	sdhci_msm_handle_pwr_irq(host, 0);
>  
>  	/*
>  	 * Ensure that above writes are propogated before interrupt enablement
> 

Re: [PATCH v2 3/4] mmc: Kconfig: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Adrian Hunter]

On 27/09/17 08:34, Vijay Viswanath wrote:
> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
> register read and write APIs, if registered, can be used.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 2db84dd..11bfb82 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -420,6 +420,7 @@ config MMC_SDHCI_MSM
>  	tristate "Qualcomm SDHCI Controller Support"
>  	depends on ARCH_QCOM || (ARM && COMPILE_TEST)
>  	depends on MMC_SDHCI_PLTFM
> +	select MMC_SDHCI_IO_ACCESSORS
>  	help
>  	  This selects the Secure Digital Host Controller Interface (SDHCI)
>  	  support present in Qualcomm SOCs. The controller supports
> 

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Adrian Hunter]

On 27/09/17 08:34, Vijay Viswanath wrote:
> Register writes which change voltage of IO lines or turn the IO bus
> on/off require controller to be ready before progressing further. When
> the controller is ready, it will generate a power irq which needs to be
> handled. The thread which initiated the register write should wait for
> power irq to complete. This will be done through the new sdhc msm write
> APIs which will check whether the particular write can trigger a power
> irq and wait for it with a timeout if it is expected.
> The SDHC core power control IRQ gets triggered when -
> * There is a state change in power control bit (bit 0)
>   of SDHCI_POWER_CONTROL register.
> * There is a state change in 1.8V enable bit (bit 3) of
>   SDHCI_HOST_CONTROL2 register.
> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
> 
> Also add support APIs which are used by sdhc msm write APIs to check
> if power irq is expected to be generated and wait for the power irq
> to come and complete if the irq is expected.
> 
> This patch requires CONFIG_MMC_SDHCI_IO_ACCESSORS to be enabled.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-msm.c | 173 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 171 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 42a65ab..b72a487 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -123,6 +123,10 @@
>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>  
>  #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
> +
> +/* Timeout value to avoid infinite waiting for pwr_irq */
> +#define MSM_PWR_IRQ_TIMEOUT_MS 5000
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -138,6 +142,10 @@ struct sdhci_msm_host {
>  	bool calibration_done;
>  	u8 saved_tuning_phase;
>  	bool use_cdclp533;
> +	u32 curr_pwr_state;
> +	u32 curr_io_level;
> +	wait_queue_head_t pwr_irq_wait;
> +	bool pwr_irq_flag;
>  };
>  
>  static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -995,6 +1003,73 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> +static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
> +{
> +	init_waitqueue_head(&msm_host->pwr_irq_wait);
> +}
> +
> +static inline void sdhci_msm_complete_pwr_irq_wait(
> +		struct sdhci_msm_host *msm_host)
> +{
> +	wake_up(&msm_host->pwr_irq_wait);
> +}
> +
> +/*
> + * sdhci_msm_check_power_status API should be called when registers writes
> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> + * To what state the register writes will change the IO lines should be passed
> + * as the argument req_type. This API will check whether the IO line's state
> + * is already the expected state and will wait for power irq only if
> + * power irq is expected to be trigerred based on the current IO line state
> + * and expected IO line state.
> + */
> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	bool done = false;
> +
> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
> +			mmc_hostname(host->mmc), __func__, req_type,
> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
> +
> +	/*
> +	 * The IRQ for request type IO High/LOW will be generated when -
> +	 * there is a state change in 1.8V enable bit (bit 3) of
> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
> +	 * to set it to 3.3V before card detection happens, the
> +	 * IRQ doesn't get triggered as there is no state change in this bit.
> +	 * The driver already handles this case by changing the IO voltage
> +	 * level to high as part of controller power up sequence. Hence, check
> +	 * for host->pwr to handle a case where IO voltage high request is
> +	 * issued even before controller power up.
> +	 */
> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
> +		pr_debug("%s: do not wait for power IRQ that never comes, req_type: %d\n",
> +				mmc_hostname(host->mmc), req_type);
> +		return;
> +	}
> +	if ((req_type & msm_host->curr_pwr_state) ||
> +			(req_type & msm_host->curr_io_level))
> +		done = true;
> +	/*
> +	 * This is needed here to handle cases where register writes will
> +	 * not change the current bus state or io level of the controller.
> +	 * In this case, no power irq will be triggerred and we should
> +	 * not wait.
> +	 */
> +	if (!done) {
> +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
> +				msm_host->pwr_irq_flag,
> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
> +					mmc_hostname(host->mmc), req_type);
> +	}
> +	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
> +			__func__, req_type);
> +}
> +
>  static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1013,6 +1088,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>  	u32 irq_status, irq_ack = 0;
>  	int retry = 10;
> +	int pwr_state = 0, io_level = 0;
> +
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
> @@ -1041,10 +1118,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> -	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +	/* Handle BUS ON/OFF*/
> +	if (irq_status & CORE_PWRCTL_BUS_ON) {
> +		pwr_state = REQ_BUS_ON;
> +		io_level = REQ_IO_HIGH;
> +		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +	}
> +	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> +		pwr_state = REQ_BUS_OFF;
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> -	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> +	}
> +	/* Handle IO LOW/HIGH */
> +	if (irq_status & CORE_PWRCTL_IO_LOW) {
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
> +	if (irq_status & CORE_PWRCTL_IO_HIGH) {
> +		io_level = REQ_IO_HIGH;
> +		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
>  
>  	/*
>  	 * The driver has to acknowledge the interrupt, switch voltages and
> @@ -1053,6 +1146,11 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>  
> +	if (pwr_state)
> +		msm_host->curr_pwr_state = pwr_state;
> +	if (io_level)
> +		msm_host->curr_io_level = io_level;
> +
>  	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>  		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>  		irq_ack);
> @@ -1061,8 +1159,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>  {
>  	struct sdhci_host *host = (struct sdhci_host *)data;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>  
>  	sdhci_msm_handle_pwr_irq(host, irq);
> +	msm_host->pwr_irq_flag = 1;
> +	sdhci_msm_complete_pwr_irq_wait(msm_host);
> +
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1131,6 +1234,69 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> +/*
> + * Platform specific register write functions. This is so that, if any
> + * register write needs to be followed up by platform specific actions,
> + * they can be added here. These functions can go to sleep when writes
> + * to certain registers are done.
> + * These functions are relying on sdhci_set_ios not using spinlock.
> + */
> +static int __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	u32 req_type = 0;
> +
> +	switch (reg) {
> +	case SDHCI_HOST_CONTROL2:
> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
> +			REQ_IO_HIGH;
> +		break;
> +	case SDHCI_SOFTWARE_RESET:
> +		if (host->pwr && (val & SDHCI_RESET_ALL))
> +			req_type = REQ_BUS_OFF;
> +		break;
> +	case SDHCI_POWER_CONTROL:
> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
> +		break;
> +	}
> +
> +	if (req_type) {
> +		msm_host->pwr_irq_flag = 0;
> +		/*
> +		 * Since this register write may trigger a power irq, ensure
> +		 * all previous register writes are complete by this point.
> +		 */
> +		mb();
> +	}
> +	return req_type;
> +}
> +
> +/* This function may sleep*/
> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
> +{
> +	u32 req_type = 0;
> +
> +	req_type = __sdhci_msm_check_write(host, val, reg);
> +	writew_relaxed(val, host->ioaddr + reg);
> +
> +	if (req_type)
> +		sdhci_msm_check_power_status(host, req_type);
> +}
> +
> +/* This function may sleep*/
> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
> +{
> +	u32 req_type = 0;
> +
> +	req_type = __sdhci_msm_check_write(host, val, reg);
> +
> +	writeb_relaxed(val, host->ioaddr + reg);
> +
> +	if (req_type)
> +		sdhci_msm_check_power_status(host, req_type);
> +}
> +
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>  	{ .compatible = "qcom,sdhci-msm-v4" },
>  	{},
> @@ -1145,6 +1311,8 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> +	.write_w = sdhci_msm_writew,
> +	.write_b = sdhci_msm_writeb,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> @@ -1312,6 +1480,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	sdhci_msm_init_pwr_irq_wait(msm_host);
>  	/* Enable pwr irq interrupts */
>  	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>  
> 

Re: [PATCH v2 0/4] mmc: sdhci-msm: Corrections to implementation of power irq

[Ulf Hansson]

On 27 September 2017 at 07:34, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Register writes which change voltage of IO lines or turn the IO bus on/off
> require sdhc controller to be ready before progressing further. Once a
> register write which affects IO lines is done, the driver should wait for
> power irq from controller. Once the irq comes, the driver should acknowledge
> the irq by writing to power control register. If the acknowledgement is not
> given to controller, the controller may not complete the corresponding
> register write action and this can mess up the controller if drivers proceeds
> without power irq completing.
>
> Changes since v1:
>         Patch enabling MMC_IO_ACCESSORS in Kconfig moved up the patch list.
>         Also corrected a mistake in the patch.
>         Removed all ifdef CONFIG_MMC_IO_ACCESSORS since the patches using it
>         will come after MMC_IO_ACCESSORS are enabled.
>         Merged the patches 3 & 4 of earlier series into 1 patch (patch 4 in
>         current series).
>         Corrected a mistake in a comment text in patch 3 of previous series.
>
> Changes since RFC:
>         wait_for_completion_timeout replaced with wait_event_timeout when
>         waiting for power irq.
>         Removed the spinlock within power irq handler and API which waits
>         for power irq.
>         Added comments to sdhci msm register write functions, warning that they
>         can sleep.
>         Sdhci msm register write functions will do a memory barrier before
>         writing to the register if the particular register can trigger
>         power irq.
>         Instead of enabling SDHCI IO ACCESSORS config in arm64/defconfig, it
>         will be selected in mmc/host/Kconfig if the platform is MMC_SDHCI_MSM.
>
> Sahitya Tummala (1):
>   mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
>
> Subhash Jadavani (1):
>   mmc: sdhci-msm: fix issue with power irq
>
> Vijay Viswanath (2):
>   mmc: Kconfig: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS
>   mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr
>     irq
>
>  drivers/mmc/host/Kconfig     |   1 +
>  drivers/mmc/host/sdhci-msm.c | 235 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 231 insertions(+), 5 deletions(-)
>

Thanks, applied for next!

Kind regards
Uffe

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Bjorn Andersson]

On Tue 26 Sep 22:34 PDT 2017, Vijay Viswanath wrote:

> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> +	if (!done) {
> +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
> +				msm_host->pwr_irq_flag,
> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
> +					mmc_hostname(host->mmc), req_type);

Bumped my MSM8974AB device to latest linux-next and found this patch; I
see this print every five seconds during boot and the eMMC never comes
up.

Regards,
Bjorn

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Vijay Viswanath]

On 10/14/2017 1:01 PM, Bjorn Andersson wrote:
> On Tue 26 Sep 22:34 PDT 2017, Vijay Viswanath wrote:
> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> [..]
>> +	if (!done) {
>> +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
>> +				msm_host->pwr_irq_flag,
>> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
>> +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
>> +					mmc_hostname(host->mmc), req_type);
> 
> Bumped my MSM8974AB device to latest linux-next and found this patch; I
> see this print every five seconds during boot and the eMMC never comes
> up.
> 
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Hi Bjorn,

I couldn't get one 8974 device. I tested the latest linux-next on db410c 
and its not showing any issue. 8974 is an older msm and has some 
differences in the controller like your "mmc: sdhci-msm: Enable delay 
circuit calibration clocks" patch.

I am currently going through the programming guide to see what could be 
the reason. Can you please share the full logs from the device with me 
so that I can focus my search?

Thanks,
Vijay

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Bjorn Andersson]

On Fri 20 Oct 03:54 PDT 2017, Vijay Viswanath wrote:

> On 10/14/2017 1:01 PM, Bjorn Andersson wrote:
> > On Tue 26 Sep 22:34 PDT 2017, Vijay Viswanath wrote:
> > 
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > [..]
> > > +	if (!done) {
> > > +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
> > > +				msm_host->pwr_irq_flag,
> > > +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> > > +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
> > > +					mmc_hostname(host->mmc), req_type);
> > 
> > Bumped my MSM8974AB device to latest linux-next and found this patch; I
> > see this print every five seconds during boot and the eMMC never comes
> > up.
> > 
> > Regards,
> > Bjorn
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> Hi Bjorn,
> 

Hi Vijay,

> I couldn't get one 8974 device. I tested the latest linux-next on db410c and
> its not showing any issue. 8974 is an older msm and has some differences in
> the controller like your "mmc: sdhci-msm: Enable delay circuit calibration
> clocks" patch.
> 

In particular there seems to be some quirks that appeared in 8974pro
(which this is).

> I am currently going through the programming guide to see what could be the
> reason. Can you please share the full logs from the device with me so that I
> can focus my search?
> 

Of course, sorry for the drive-by report without much to go on. I did
some more testing and you can find an extract of the kernel log below.
I was apparently not patient enough before, after 124 seconds we're
through all the timeouts and my eMMC is "functional" - so things are
working, but we're getting stuck waiting for the timeout every time
we're waiting for the pwr irq.


As you can see below we get sdhci_msm_handle_pwr_irq() with status
BUS_ON, so we set io-level HIGH. Then we don't get any more pwr
interrupts, so the io_level remains "high" - although I believe we're
actually low (vqmmc is always 1.8V on this board).

Worth noting is the large comment in sdhci_msm_check_power_status()
related to !host->pwr, host->pwr is always 0 on this board.

# dmesg |grep mmc0
[    1.734573] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    1.857611] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    1.889400] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    2.141634] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    2.163023] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    2.238875] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(0), irq_status=0x0, ack=0x0
[    2.509617] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 0 curr_io_level 0
[    2.509710] mmc0: sdhci_msm_pwr_irq()
[    2.509723] mmc0: sdhci_msm_handle_pwr_irq: Handled IRQ(30), irq_status=0x2, ack=0x1
[    2.536468] mmc0: sdhci_msm_check_power_status: request 2 done
[    2.546363] mmc0: sdhci_msm_check_power_status: request 8 curr_pwr_state 2 curr_io_level 8
[    2.551220] mmc0: do not wait for power IRQ that never comes, req_type: 8
[    2.559502] sdhci_msm f9824900.sdhci: mmc0: clock=0 uhs=0 ctrl_2=0x0
[    2.572777] mmc0: sdhci_msm_check_power_status: request 8 curr_pwr_state 2 curr_io_level 8
[    2.577353] mmc0: do not wait for power IRQ that never comes, req_type: 8
[    2.592564] mmc0: sdhci_msm_check_power_status: request 8 curr_pwr_state 2 curr_io_level 8
[    2.597230] mmc0: do not wait for power IRQ that never comes, req_type: 8
[    2.605589] mmc0: Switching to 3.3V signalling voltage failed
[    2.618172] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[    7.687764] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[    7.687795] mmc0: sdhci_msm_check_power_status: request 4 done
[    7.717799] mmc0: Setting clock at rate 400000 at timing 0
[    7.717887] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[    7.722170] mmc0: sdhci_msm_check_power_status: request 2 done
[    7.736229] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   12.807757] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   12.807785] mmc0: sdhci_msm_check_power_status: request 4 done
[   12.813531] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   12.826189] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   17.847757] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   17.847784] mmc0: sdhci_msm_check_power_status: request 4 done
[   17.853520] mmc0: Setting clock at rate 400000 at timing 0
[   17.887757] mmc0: SDHCI controller on f9824900.sdhci [f9824900.sdhci] using ADMA 64-bit
[   17.897018] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   17.902821] mmc0: sdhci_msm_check_power_status: request 2 done
[   17.916694] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   22.967759] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   22.967787] mmc0: sdhci_msm_check_power_status: request 4 done
[   22.973533] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   22.986192] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   28.007753] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   28.007777] mmc0: sdhci_msm_check_power_status: request 4 done
[   28.013515] mmc0: Setting clock at rate 400000 at timing 0
[   28.021738] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   28.024798] mmc0: sdhci_msm_check_power_status: request 2 done
[   28.038879] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   33.047769] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   33.047793] mmc0: sdhci_msm_check_power_status: request 4 done
[   33.053536] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   33.066199] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   38.087755] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   38.087779] mmc0: sdhci_msm_check_power_status: request 4 done
[   38.093514] mmc0: Setting clock at rate 400000 at timing 0
[   38.105722] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   38.105748] mmc0: sdhci_msm_check_power_status: request 2 done
[   38.118881] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   43.127756] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   43.127780] mmc0: sdhci_msm_check_power_status: request 4 done
[   43.133523] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   43.146184] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   48.167756] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   48.167780] mmc0: sdhci_msm_check_power_status: request 4 done
[   48.173515] mmc0: Setting clock at rate 400000 at timing 0
[   48.179810] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   48.184800] mmc0: sdhci_msm_check_power_status: request 2 done
[   48.198880] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   53.207753] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   53.207777] mmc0: sdhci_msm_check_power_status: request 4 done
[   53.213518] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   53.226181] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   58.247754] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   58.247777] mmc0: sdhci_msm_check_power_status: request 4 done
[   58.253513] mmc0: Setting clock at rate 400000 at timing 0
[   58.259414] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   58.264797] mmc0: sdhci_msm_check_power_status: request 2 done
[   58.278877] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   63.287754] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   63.287779] mmc0: sdhci_msm_check_power_status: request 4 done
[   63.293521] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   63.306184] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   68.327764] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   68.327792] mmc0: sdhci_msm_check_power_status: request 4 done
[   68.333530] mmc0: Setting clock at rate 400000 at timing 0
[   68.341746] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   68.344812] mmc0: sdhci_msm_check_power_status: request 2 done
[   68.358891] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   73.367756] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   73.367783] mmc0: sdhci_msm_check_power_status: request 4 done
[   73.373525] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   73.386186] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   78.407759] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   78.407786] mmc0: sdhci_msm_check_power_status: request 4 done
[   78.413521] mmc0: Setting clock at rate 400000 at timing 0
[   78.449337] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   78.449363] mmc0: sdhci_msm_check_power_status: request 2 done
[   78.462325] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   83.527756] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   83.527781] mmc0: sdhci_msm_check_power_status: request 4 done
[   83.533524] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   83.546187] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   88.567758] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   88.567786] mmc0: sdhci_msm_check_power_status: request 4 done
[   88.573522] mmc0: Setting clock at rate 400000 at timing 0
[   88.611069] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   93.687759] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   93.687788] mmc0: sdhci_msm_check_power_status: request 4 done
[   93.694461] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[   93.699369] mmc0: sdhci_msm_check_power_status: request 2 done
[   93.713422] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[   98.727752] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   98.727779] mmc0: sdhci_msm_check_power_status: request 4 done
[   98.733523] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=0 ctrl_2=0x8
[   98.746184] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[  103.767770] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[  103.767795] mmc0: sdhci_msm_check_power_status: request 4 done
[  103.773531] mmc0: Setting clock at rate 400000 at timing 0
[  103.799563] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[  103.799589] mmc0: sdhci_msm_check_power_status: request 2 done
[  103.812552] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[  108.887755] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[  108.887779] mmc0: sdhci_msm_check_power_status: request 4 done
[  108.893522] sdhci_msm f9824900.sdhci: mmc0: clock=400000 uhs=9 ctrl_2=0x8
[  108.906185] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[  113.927755] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[  113.927778] mmc0: sdhci_msm_check_power_status: request 4 done
[  113.933513] mmc0: Setting clock at rate 400000 at timing 9
[  113.939901] mmc0: Setting clock at rate 200000000 at timing 9
[  113.944801] mmc0: sdhci_msm_check_power_status: request 2 curr_pwr_state 2 curr_io_level 8
[  113.950642] mmc0: sdhci_msm_check_power_status: request 2 done
[  113.964587] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[  119.047754] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[  119.047778] mmc0: sdhci_msm_check_power_status: request 4 done
[  119.053513] sdhci_msm f9824900.sdhci: mmc0: clock=200000000 uhs=9 ctrl_2=0xb
[  119.066531] mmc0: sdhci_msm_check_power_status: request 4 curr_pwr_state 2 curr_io_level 8
[  124.087755] sdhci_msm f9824900.sdhci: mmc0: pwr_irq for req: (4) timed out
[  124.087779] mmc0: sdhci_msm_check_power_status: request 4 done
[  124.093514] mmc0: Setting clock at rate 200000000 at timing 9
[  124.099657] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 0
[  124.105439] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 1
[  124.111537] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 2
[  124.117501] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 3
[  124.123514] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 4
[  124.129501] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 5
[  124.135411] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 6
[  124.141423] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 7
[  124.147389] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 8
[  124.153403] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 9
[  124.159448] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 10
[  124.165417] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 11
[  124.171433] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 12
[  124.177483] sdhci_msm f9824900.sdhci: mmc0: Found good phase = 13
[  124.183709] sdhci_msm f9824900.sdhci: mmc0: Setting the tuning phase to 9
[  124.189943] mmc0: new HS200 MMC card at address 0001
[  124.198091] mmcblk0: mmc0:0001 MAG2GC 14.6 GiB 
[  124.201538] mmcblk0boot0: mmc0:0001 MAG2GC partition 1 4.00 MiB
[  124.205695] mmcblk0boot1: mmc0:0001 MAG2GC partition 2 4.00 MiB
[  124.211637] mmcblk0rpmb: mmc0:0001 MAG2GC partition 3 4.00 MiB, chardev (247:0)


Please let me know if I can assist you with anything additional!

Regards,
Bjorn

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Georgi Djakov]

Hello Vijay,

On 09/27/2017 08:34 AM, Vijay Viswanath wrote:
> Register writes which change voltage of IO lines or turn the IO bus
> on/off require controller to be ready before progressing further. When
> the controller is ready, it will generate a power irq which needs to be
> handled. The thread which initiated the register write should wait for
> power irq to complete. This will be done through the new sdhc msm write
> APIs which will check whether the particular write can trigger a power
> irq and wait for it with a timeout if it is expected.
> The SDHC core power control IRQ gets triggered when -
> * There is a state change in power control bit (bit 0)
>   of SDHCI_POWER_CONTROL register.
> * There is a state change in 1.8V enable bit (bit 3) of
>   SDHCI_HOST_CONTROL2 register.
> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
> 
> Also add support APIs which are used by sdhc msm write APIs to check
> if power irq is expected to be generated and wait for the power irq
> to come and complete if the irq is expected.
> 
> This patch requires CONFIG_MMC_SDHCI_IO_ACCESSORS to be enabled.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 173 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 171 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c

[..]

> +/*
> + * sdhci_msm_check_power_status API should be called when registers writes
> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> + * To what state the register writes will change the IO lines should be passed
> + * as the argument req_type. This API will check whether the IO line's state
> + * is already the expected state and will wait for power irq only if
> + * power irq is expected to be trigerred based on the current IO line state
> + * and expected IO line state.
> + */
> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	bool done = false;
> +
> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
> +			mmc_hostname(host->mmc), __func__, req_type,
> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
> +
> +	/*
> +	 * The IRQ for request type IO High/LOW will be generated when -
> +	 * there is a state change in 1.8V enable bit (bit 3) of
> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
> +	 * to set it to 3.3V before card detection happens, the
> +	 * IRQ doesn't get triggered as there is no state change in this bit.
> +	 * The driver already handles this case by changing the IO voltage
> +	 * level to high as part of controller power up sequence. Hence, check
> +	 * for host->pwr to handle a case where IO voltage high request is
> +	 * issued even before controller power up.
> +	 */
> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
> +		pr_debug("%s: do not wait for power IRQ that never comes, req_type: %d\n",
> +				mmc_hostname(host->mmc), req_type);
> +		return;
> +	}
> +	if ((req_type & msm_host->curr_pwr_state) ||
> +			(req_type & msm_host->curr_io_level))
> +		done = true;
> +	/*
> +	 * This is needed here to handle cases where register writes will
> +	 * not change the current bus state or io level of the controller.
> +	 * In this case, no power irq will be triggerred and we should
> +	 * not wait.
> +	 */
> +	if (!done) {
> +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
> +				msm_host->pwr_irq_flag,
> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
> +					mmc_hostname(host->mmc), req_type);

I am seeing the above error message on a db820c device (apq8096). When i
unplug the SD card and then plug it back in, there is a 5 sec card
detection delay and a timeout. Here is a log:

[   50.253997] mmc0: card 59b4 removed
[   50.381874] mmc0: sdhci_msm_check_power_status: request 1
curr_pwr_state 2 curr_io_level 4 sdhci_host_ctrl2 b
[   50.382656] mmc0: sdhci_msm_check_power_status: request 1 done
[   50.392493] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 b
[   50.398258] mmc0: sdhci_msm_check_power_status: request 4 done
[   50.408728] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8
[   50.413865] mmc0: sdhci_msm_check_power_status: request 4 done
[   54.966316] mmc0: sdhci_msm_check_power_status: request 2
curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8  <-- card is plugged
[   54.967756] mmc0: sdhci_msm_check_power_status: request 2 done
[   54.976822] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
[   60.132253] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out
[   60.132782] mmc0: sdhci_msm_check_power_status: request 4 done
[   60.139888] mmc0: sdhci_msm_check_power_status: request 4
curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
[   65.252497] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out

The following patch seem to "fix" it.

-- >8 --
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index b2d875afae5f..ca8ad4edcf80 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1049,6 +1049,11 @@ static void sdhci_msm_check_power_status(struct
sdhci_host *host, u32 req_type)
                return;
        }

+       if (req_type & REQ_IO_LOW &&
+           msm_host->curr_pwr_state & REQ_BUS_ON &&
+           msm_host->curr_io_level & REQ_IO_HIGH &&
+           !host->pwr)
+               done = true;
        /*
         * The IRQ for request type IO High/LOW will be generated when -
         * there is a state change in 1.8V enable bit (bit 3) of
-- >8 --

Reverting this patch series or applying the above patch makes the issue
go away. According to the log, there is no change in the
sdhci_host_ctrl2 register. What do you think?

Thanks,
Georgi

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Vijay Viswanath]

Hi Georgi,

Thanks for testing the patch on 8096 and pointing out this issue.
The issue is coming because, when card is removed, the HOST_CONTROL2 
register is retaining the 1.8V Signalling enable bit till SDHCI reset 
happens after a new card is inserted.

Adding the change you suggested can avoid this wait, but I feel a better 
solution is to clear the 1.8V signalling bit when the card is removed. 
When a new card is inserted, we shouldn't be keeping the 1.8V bit set 
until we send cmd11 to the SD card. A new SD card should start with 3V.

One solution is to explicitly clear the HOST_CONTROL2 register when card 
is removed.

Other way is to revert the commit: 
9718f84b85396e090ca42fafa730410d286d61e3 "mmc: sdhci-msm: Do not reset 
the controller if no card in the slot"

The sdhci-msm doesn't require "SDHCI_QUIRK_NO_CARD_NO_RESET". The issue 
which above commit is trying to avoid is fixed by the pwr-irq patches. 
Resetting the controller will clear the HOST_CONTROL2 register and avoid 
this issue.

Can you please try this ? I tested reverting the QUIRK on two platforms: 
db410c(8916) and sdm845. SD card insert/remove worked fine after that 
and I didn't get any "Reset 0x1 never completed" error during card 
insert/remove or shutdown.

Thanks,
Vijay

On 5/29/2018 5:49 PM, Georgi Djakov wrote:
> Hello Vijay,
> 
> On 09/27/2017 08:34 AM, Vijay Viswanath wrote:
>> Register writes which change voltage of IO lines or turn the IO bus
>> on/off require controller to be ready before progressing further. When
>> the controller is ready, it will generate a power irq which needs to be
>> handled. The thread which initiated the register write should wait for
>> power irq to complete. This will be done through the new sdhc msm write
>> APIs which will check whether the particular write can trigger a power
>> irq and wait for it with a timeout if it is expected.
>> The SDHC core power control IRQ gets triggered when -
>> * There is a state change in power control bit (bit 0)
>>    of SDHCI_POWER_CONTROL register.
>> * There is a state change in 1.8V enable bit (bit 3) of
>>    SDHCI_HOST_CONTROL2 register.
>> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
>>
>> Also add support APIs which are used by sdhc msm write APIs to check
>> if power irq is expected to be generated and wait for the power irq
>> to come and complete if the irq is expected.
>>
>> This patch requires CONFIG_MMC_SDHCI_IO_ACCESSORS to be enabled.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 173 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 171 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> 
> [..]
> 
>> +/*
>> + * sdhci_msm_check_power_status API should be called when registers writes
>> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
>> + * To what state the register writes will change the IO lines should be passed
>> + * as the argument req_type. This API will check whether the IO line's state
>> + * is already the expected state and will wait for power irq only if
>> + * power irq is expected to be trigerred based on the current IO line state
>> + * and expected IO line state.
>> + */
>> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	bool done = false;
>> +
>> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
>> +			mmc_hostname(host->mmc), __func__, req_type,
>> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
>> +
>> +	/*
>> +	 * The IRQ for request type IO High/LOW will be generated when -
>> +	 * there is a state change in 1.8V enable bit (bit 3) of
>> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
>> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
>> +	 * to set it to 3.3V before card detection happens, the
>> +	 * IRQ doesn't get triggered as there is no state change in this bit.
>> +	 * The driver already handles this case by changing the IO voltage
>> +	 * level to high as part of controller power up sequence. Hence, check
>> +	 * for host->pwr to handle a case where IO voltage high request is
>> +	 * issued even before controller power up.
>> +	 */
>> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
>> +		pr_debug("%s: do not wait for power IRQ that never comes, req_type: %d\n",
>> +				mmc_hostname(host->mmc), req_type);
>> +		return;
>> +	}
>> +	if ((req_type & msm_host->curr_pwr_state) ||
>> +			(req_type & msm_host->curr_io_level))
>> +		done = true;
>> +	/*
>> +	 * This is needed here to handle cases where register writes will
>> +	 * not change the current bus state or io level of the controller.
>> +	 * In this case, no power irq will be triggerred and we should
>> +	 * not wait.
>> +	 */
>> +	if (!done) {
>> +		if (!wait_event_timeout(msm_host->pwr_irq_wait,
>> +				msm_host->pwr_irq_flag,
>> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
>> +			__WARN_printf("%s: pwr_irq for req: (%d) timed out\n",
>> +					mmc_hostname(host->mmc), req_type);
> 
> I am seeing the above error message on a db820c device (apq8096). When i
> unplug the SD card and then plug it back in, there is a 5 sec card
> detection delay and a timeout. Here is a log:
> 
> [   50.253997] mmc0: card 59b4 removed
> [   50.381874] mmc0: sdhci_msm_check_power_status: request 1
> curr_pwr_state 2 curr_io_level 4 sdhci_host_ctrl2 b
> [   50.382656] mmc0: sdhci_msm_check_power_status: request 1 done
> [   50.392493] mmc0: sdhci_msm_check_power_status: request 4
> curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 b
> [   50.398258] mmc0: sdhci_msm_check_power_status: request 4 done
> [   50.408728] mmc0: sdhci_msm_check_power_status: request 4
> curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8
> [   50.413865] mmc0: sdhci_msm_check_power_status: request 4 done
> [   54.966316] mmc0: sdhci_msm_check_power_status: request 2
> curr_pwr_state 1 curr_io_level 4 sdhci_host_ctrl2 8  <-- card is plugged
> [   54.967756] mmc0: sdhci_msm_check_power_status: request 2 done
> [   54.976822] mmc0: sdhci_msm_check_power_status: request 4
> curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
> [   60.132253] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out
> [   60.132782] mmc0: sdhci_msm_check_power_status: request 4 done
> [   60.139888] mmc0: sdhci_msm_check_power_status: request 4
> curr_pwr_state 2 curr_io_level 8 sdhci_host_ctrl2 8
> [   65.252497] sdhci_msm 74a4900.sdhci: mmc0: pwr_irq for req: (4) timed out
> 
> The following patch seem to "fix" it.
> 
> -- >8 --
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b2d875afae5f..ca8ad4edcf80 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1049,6 +1049,11 @@ static void sdhci_msm_check_power_status(struct
> sdhci_host *host, u32 req_type)
>                  return;
>          }
> 
> +       if (req_type & REQ_IO_LOW &&
> +           msm_host->curr_pwr_state & REQ_BUS_ON &&
> +           msm_host->curr_io_level & REQ_IO_HIGH &&
> +           !host->pwr)
> +               done = true;
>          /*
>           * The IRQ for request type IO High/LOW will be generated when -
>           * there is a state change in 1.8V enable bit (bit 3) of
> -- >8 --
> 
> Reverting this patch series or applying the above patch makes the issue
> go away. According to the log, there is no change in the
> sdhci_host_ctrl2 register. What do you think?
> 
> Thanks,
> Georgi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 4/4] mmc: sdhci-msm: Add sdhci msm register write APIs which wait for pwr irq

[Georgi Djakov]

Hi Vijay,

On 05/30/2018 10:11 AM, Vijay Viswanath wrote:
> Hi Georgi,
> 
> Thanks for testing the patch on 8096 and pointing out this issue.
> The issue is coming because, when card is removed, the HOST_CONTROL2
> register is retaining the 1.8V Signalling enable bit till SDHCI reset
> happens after a new card is inserted.
> 
> Adding the change you suggested can avoid this wait, but I feel a better
> solution is to clear the 1.8V signalling bit when the card is removed.
> When a new card is inserted, we shouldn't be keeping the 1.8V bit set
> until we send cmd11 to the SD card. A new SD card should start with 3V.
> 
> One solution is to explicitly clear the HOST_CONTROL2 register when card
> is removed.
> 
> Other way is to revert the commit:
> 9718f84b85396e090ca42fafa730410d286d61e3 "mmc: sdhci-msm: Do not reset
> the controller if no card in the slot"
> 
> The sdhci-msm doesn't require "SDHCI_QUIRK_NO_CARD_NO_RESET". The issue
> which above commit is trying to avoid is fixed by the pwr-irq patches.
> Resetting the controller will clear the HOST_CONTROL2 register and avoid
> this issue.
> 
> Can you please try this ? I tested reverting the QUIRK on two platforms:
> db410c(8916) and sdm845. SD card insert/remove worked fine after that
> and I didn't get any "Reset 0x1 never completed" error during card
> insert/remove or shutdown.

Thank you! I have submitted a patch to remove the quirk and tested it on
db410c and db820c.

BR,
Georgi