Re: [PATCH 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/24/2017 3:35 PM, Adrian Hunter wrote:
On 18/08/17 08:19, Vijay Viswanath wrote:
From: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>

Add support API which will check if power irq is expected to be
generated and wait for the power irq to come and complete if the irq is
expected.

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

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index f3e0489..6d3b1fd 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,11 @@ struct sdhci_msm_host {
  	bool calibration_done;
  	u8 saved_tuning_phase;
  	bool use_cdclp533;
+	u32 curr_pwr_state;
+	u32 curr_io_level;
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+	struct completion pwr_irq_completion;
+#endif
  };
static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
@@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
  		sdhci_msm_hs400(host, &mmc->ios);
  }
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	init_completion(&msm_host->pwr_irq_completion);
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	complete(&msm_host->pwr_irq_completion);
+}
+
+/*
+ * 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);
+	unsigned long flags;
+	bool done = false;
+
+	spin_lock_irqsave(&host->lock, flags);
+	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\n",
+				mmc_hostname(host->mmc));
+		spin_unlock_irqrestore(&host->lock, flags);
+		return;
+	}
+	if ((req_type & msm_host->curr_pwr_state) ||
+			(req_type & msm_host->curr_io_level))
+		done = true;
+	spin_unlock_irqrestore(&host->lock, flags);
+	/*
+	 * This is needed here to hanlde a case where IRQ gets
+	 * triggered even before this function is called so that
+	 * x->done counter of completion gets reset. Otherwise,
+	 * next call to wait_for_completion returns immediately
+	 * without actually waiting for the IRQ to be handled.
+	 */
+	if (done)
+		init_completion(&msm_host->pwr_irq_completion);
+	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
+				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))

This all looks a bit more complicated and fragile than it needs to be.  You
are waiting for an event so you really want to be using
wait_event_timeout().  Reset the event condition before (will need a memory
barrier) writing the register and then just wait_event_timeout() to wait i.e.

Waiter:
	clear flag
	memory barrier
	write register
	wait_event_timeout(wq,flag is set,timeout)

Interrupt:
	set flag
	wake_up(&wq);

AFAICS you shouldn't need the spin lock at all.


Will do it this way. It looks cleaner and neat. Thanks!

+		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
+				mmc_hostname(host->mmc), req_type);
+	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
+			__func__, req_type);
+}
+#else
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+#endif
+
  static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
  {
  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1013,6 +1106,9 @@ 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;
+	unsigned long flags;
+
irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
  	irq_status &= INT_MASK;
@@ -1040,10 +1136,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
@@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
  	 */
  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
+ spin_lock_irqsave(&host->lock, flags);
+	if (pwr_state)
+		msm_host->curr_pwr_state = pwr_state;
+	if (io_level)
+		msm_host->curr_io_level = io_level;

Why separate curr_pwr_state and curr_io_level - the bits are separate
anyway.  Looks like this could just be:

	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))
		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
   	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);

	msm_host->pwr_irq_status = irq_status;

And as mentioned above, I don't think you need the spin lock.

I will remove the spinlock. Regarding why there are separate variable for BUS state and IO level, During initialization, we get CORE_PWRCTL_BUS_ON interrupt from PWRCTL register. But the bit CORE_PWRCTL_IO_HIGH will not be set in the CORE_PWRCTL_STATUS register even though the io level will be high. So after the BUS is set on, if we do a register write to set IO as high, in the sdhci_msm_check_power_status register, we will think that there is a change in IO level and will wait for power irq . But the controller will not trigger any power irq as the IO level was already high. So whenever we get the BUS_ON interrupt, we should store somewhere that the IO level is also HIGH.

We can do the above with a single variable instead of 2 variables used now, but it will make the code more complex. Whenever we have to change the pwr_irq_status in msm_host, we will have to clear either the 2 IO bits or the whole variable when we get power irq for IO level change or BUS on/off respectively.

+	spin_unlock_irqrestore(&host->lock, flags);
+	sdhci_msm_complete_pwr_irq_completion(msm_host);
+
  	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);
@@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
  		goto clk_disable;
  	}
+ sdhci_msm_init_pwr_irq_completion(msm_host);
  	/* Enable pwr irq interrupts */
  	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux