[PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

[PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

[Venkatraman S]

omap hsmmc controller IP has a built in timer that can be programmed to
guard against unresponsive operations. But its range is very narrow,
and the maximum countable time is a few seconds.

Card maintenance operations like BKOPS and MMC_ERASE and long
stream writes like packed command require timers of order of
several minutes, much beyond the capability of the IP timer.
So get rid of using the IP timer entirely and use kernel's hrtimer
functionality for guarding the device operations.
As part of this change, a workaround that disabled timeouts for
MMC_ERASE command is removed, and the arbitary timing of 100ms
is used only when the timeout is not explicitly specified by core.

A trivial change to get rid of unnecessary dealiasing of host->data
in omap_hsmmc_do_irq is also included.

Signed-off-by: Venkatraman S <svenkatr@ti.com>
---

v1->v2:
   Fix typos in commit message.
   Add checks to handle subtle races between MMC IRQ and HRTIMER IRQ
   Mark set_guard_timer function as static
   (Felipe's comment to use macros for INT_EN_MASK is done as a separate patch )
 drivers/mmc/host/omap_hsmmc.c | 96 ++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 9afdd20..57e86a4 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -79,7 +79,7 @@
 #define CLKD_SHIFT		6
 #define DTO_MASK		0x000F0000
 #define DTO_SHIFT		16
-#define INT_EN_MASK		0x307F0033
+#define INT_EN_MASK		0x306E0033
 #define BWR_ENABLE		(1 << 4)
 #define BRR_ENABLE		(1 << 5)
 #define DTO_ENABLE		(1 << 20)
@@ -160,6 +160,7 @@ struct omap_hsmmc_host {
 	unsigned int		dma_sg_idx;
 	unsigned char		bus_mode;
 	unsigned char		power_mode;
+	unsigned int		ns_per_clk_cycle;
 	int			suspended;
 	int			irq;
 	int			use_dma, dma_ch;
@@ -172,6 +173,7 @@ struct omap_hsmmc_host {
 	int			reqs_blocked;
 	int			use_reg;
 	int			req_in_progress;
+	struct hrtimer	guard_timer;
 	struct omap_hsmmc_next	next_data;
 
 	struct	omap_mmc_platform_data	*pdata;
@@ -455,10 +457,6 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
 	else
 		irq_mask = INT_EN_MASK;
 
-	/* Disable timeout for erases */
-	if (cmd->opcode == MMC_ERASE)
-		irq_mask &= ~DTO_ENABLE;
-
 	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
 	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
 	OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
@@ -508,6 +506,9 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
 		&& time_before(jiffies, timeout))
 		cpu_relax();
 
+	if (ios->clock)
+		host->ns_per_clk_cycle = DIV_ROUND_UP(NSEC_PER_SEC, ios->clock);
+
 	omap_hsmmc_start_clock(host);
 }
 
@@ -824,7 +825,7 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
 		omap_hsmmc_request_done(host, mrq);
 		return;
 	}
-
+	hrtimer_cancel(&host->guard_timer);
 	host->data = NULL;
 
 	if (!data->error)
@@ -859,8 +860,11 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct mmc_command *cmd)
 			cmd->resp[0] = OMAP_HSMMC_READ(host->base, RSP10);
 		}
 	}
-	if ((host->data == NULL && !host->response_busy) || cmd->error)
+	if ((host->data == NULL && !host->response_busy) || cmd->error) {
+		if (cmd->error != -ETIMEDOUT)
+			hrtimer_cancel(&host->guard_timer);
 		omap_hsmmc_request_done(host, cmd->mrq);
+	}
 }
 
 /*
@@ -992,7 +996,7 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
 			hsmmc_command_incomplete(host, -EILSEQ);
 
 		end_cmd = 1;
-		if (host->data || host->response_busy) {
+		if (data || host->response_busy) {
 			end_trans = 1;
 			host->response_busy = 0;
 		}
@@ -1292,41 +1296,35 @@ static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
 	return 0;
 }
 
-static void set_data_timeout(struct omap_hsmmc_host *host,
-			     unsigned int timeout_ns,
-			     unsigned int timeout_clks)
+static void set_guard_timer(struct omap_hsmmc_host *host,
+		unsigned long timeout_ms, unsigned long timeout_ns,
+		unsigned int timeout_clks)
 {
-	unsigned int timeout, cycle_ns;
-	uint32_t reg, clkd, dto = 0;
+	ktime_t gtime;
+	unsigned int sec, nsec;
 
-	reg = OMAP_HSMMC_READ(host->base, SYSCTL);
-	clkd = (reg & CLKD_MASK) >> CLKD_SHIFT;
-	if (clkd == 0)
-		clkd = 1;
+	sec = timeout_ms / MSEC_PER_SEC;
+	nsec = (timeout_ms % MSEC_PER_SEC) * NSEC_PER_MSEC + timeout_ns;
 
-	cycle_ns = 1000000000 / (clk_get_rate(host->fclk) / clkd);
-	timeout = timeout_ns / cycle_ns;
-	timeout += timeout_clks;
-	if (timeout) {
-		while ((timeout & 0x80000000) == 0) {
-			dto += 1;
-			timeout <<= 1;
-		}
-		dto = 31 - dto;
-		timeout <<= 1;
-		if (timeout && dto)
-			dto += 1;
-		if (dto >= 13)
-			dto -= 13;
-		else
-			dto = 0;
-		if (dto > 14)
-			dto = 14;
-	}
+	nsec += timeout_clks * host->ns_per_clk_cycle;
+	gtime = ktime_set(sec, nsec);
 
-	reg &= ~DTO_MASK;
-	reg |= dto << DTO_SHIFT;
-	OMAP_HSMMC_WRITE(host->base, SYSCTL, reg);
+	hrtimer_start(&host->guard_timer, gtime, HRTIMER_MODE_REL);
+}
+
+static enum hrtimer_restart omap_hsmmc_timedout(struct hrtimer *gtimer)
+{
+	struct omap_hsmmc_host *host;
+	host = container_of(gtimer, struct omap_hsmmc_host, guard_timer);
+	if (host->req_in_progress) {
+		omap_hsmmc_disable_irq(host);
+		hsmmc_command_incomplete(host, -ETIMEDOUT);
+		host->response_busy = 0;
+		omap_hsmmc_cmd_done(host, host->cmd);
+		if (host->data)
+			omap_hsmmc_xfer_done(host, host->data);
+	}
+	return HRTIMER_NORESTART;
 }
 
 /*
@@ -1340,18 +1338,22 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
 
 	if (req->data == NULL) {
 		OMAP_HSMMC_WRITE(host->base, BLK, 0);
-		/*
-		 * Set an arbitrary 100ms data timeout for commands with
-		 * busy signal.
-		 */
-		if (req->cmd->flags & MMC_RSP_BUSY)
-			set_data_timeout(host, 100000000U, 0);
+		if (req->cmd->cmd_timeout_ms == 0) {
+			/*
+			 * Set an arbitrary 100ms data timeout for commands with
+			 * busy signal.
+			 */
+			set_guard_timer(host, 100, 0, 0);
+		} else {
+			set_guard_timer(host, req->cmd->cmd_timeout_ms, 0, 0);
+		}
 		return 0;
 	}
 
 	OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
 					| (req->data->blocks << 16));
-	set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
+	set_guard_timer(host, 0, req->data->timeout_ns,
+			req->data->timeout_clks);
 
 	if (host->use_dma) {
 		ret = omap_hsmmc_start_dma_transfer(host, req);
@@ -1921,6 +1923,8 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 		pdata->suspend = omap_hsmmc_suspend_cdirq;
 		pdata->resume = omap_hsmmc_resume_cdirq;
 	}
+	hrtimer_init(&host->guard_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	host->guard_timer.function = omap_hsmmc_timedout;
 
 	omap_hsmmc_disable_irq(host);
 
-- 
1.7.11.1.25.g0e18bef

[PATCH 2/2] mmc: omap_hsmmc: cleanup the bitmap definitions of Interrupt Register

[Venkatraman S]

Define the most frequently used bitmasks of the Interrupt Enable /
Interrupt Status register with consistent naming ( with _EN suffix).

Use meaningful concatenation of bitfields for INT_EN_MASK, which shows
which interrupts are enabled by default.
No functional changes.

Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 51 ++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 57e86a4..03c2362 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -79,28 +79,16 @@
 #define CLKD_SHIFT		6
 #define DTO_MASK		0x000F0000
 #define DTO_SHIFT		16
-#define INT_EN_MASK		0x306E0033
-#define BWR_ENABLE		(1 << 4)
-#define BRR_ENABLE		(1 << 5)
-#define DTO_ENABLE		(1 << 20)
 #define INIT_STREAM		(1 << 1)
 #define DP_SELECT		(1 << 21)
 #define DDIR			(1 << 4)
-#define DMA_EN			0x1
+#define DMAE			0x1
 #define MSBS			(1 << 5)
 #define BCE			(1 << 1)
 #define FOUR_BIT		(1 << 1)
 #define DDR			(1 << 19)
 #define DW8			(1 << 5)
-#define CC			0x1
-#define TC			0x02
 #define OD			0x1
-#define ERR			(1 << 15)
-#define CMD_TIMEOUT		(1 << 16)
-#define DATA_TIMEOUT		(1 << 20)
-#define CMD_CRC			(1 << 17)
-#define DATA_CRC		(1 << 21)
-#define CARD_ERR		(1 << 28)
 #define STAT_CLEAR		0xFFFFFFFF
 #define INIT_STREAM_CMD		0x00000000
 #define DUAL_VOLT_OCR_BIT	7
@@ -109,6 +97,25 @@
 #define SOFTRESET		(1 << 1)
 #define RESETDONE		(1 << 0)
 
+/* Interrupt masks for IE and ISE register */
+#define CC_EN			(1 << 0)
+#define TC_EN			(1 << 1)
+#define BWR_EN			(1 << 4)
+#define BRR_EN			(1 << 5)
+#define ERR_EN			(1 << 15)
+#define CTO_EN			(1 << 16)
+#define CCRC_EN		(1 << 17)
+#define CEB_EN			(1 << 18)
+#define CIE_EN			(1 << 19)
+#define DTO_EN			(1 << 20)
+#define DCRC_EN		(1 << 21)
+#define DEB_EN			(1 << 22)
+#define CERR_EN		(1 << 28)
+#define BADA_EN		(1 << 29)
+
+#define INT_EN_MASK		(BADA_EN | CERR_EN | DEB_EN | DCRC_EN | \
+		CIE_EN | CEB_EN | CCRC_EN | BRR_EN | BWR_EN | TC_EN | CC_EN)
+
 #define MMC_AUTOSUSPEND_DELAY	100
 #define MMC_TIMEOUT_MS		20
 #define OMAP_MMC_MIN_CLOCK	400000
@@ -453,7 +460,7 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
 	unsigned int irq_mask;
 
 	if (host->use_dma)
-		irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
+		irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
 	else
 		irq_mask = INT_EN_MASK;
 
@@ -673,8 +680,8 @@ static void send_init_stream(struct omap_hsmmc_host *host)
 	OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD);
 
 	timeout = jiffies + msecs_to_jiffies(MMC_TIMEOUT_MS);
-	while ((reg != CC) && time_before(jiffies, timeout))
-		reg = OMAP_HSMMC_READ(host->base, STAT) & CC;
+	while ((reg != CC_EN) && time_before(jiffies, timeout))
+		reg = OMAP_HSMMC_READ(host->base, STAT) & CC_EN;
 
 	OMAP_HSMMC_WRITE(host->base, CON,
 		OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
@@ -765,7 +772,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
 	}
 
 	if (host->use_dma)
-		cmdreg |= DMA_EN;
+		cmdreg |= DMAE;
 
 	host->req_in_progress = 1;
 
@@ -988,11 +995,11 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
 	data = host->data;
 	dev_vdbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
 
-	if (status & ERR) {
+	if (status & ERR_EN) {
 		omap_hsmmc_dbg_report_irq(host, status);
-		if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
+		if (status & (CTO_EN | DTO_EN))
 			hsmmc_command_incomplete(host, -ETIMEDOUT);
-		else if (status & (CMD_CRC | DATA_CRC))
+		else if (status & (CCRC_EN | DCRC_EN))
 			hsmmc_command_incomplete(host, -EILSEQ);
 
 		end_cmd = 1;
@@ -1002,9 +1009,9 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
 		}
 	}
 
-	if (end_cmd || ((status & CC) && host->cmd))
+	if (end_cmd || ((status & CC_EN) && host->cmd))
 		omap_hsmmc_cmd_done(host, host->cmd);
-	if ((end_trans || (status & TC)) && host->mrq)
+	if ((end_trans || (status & TC_EN)) && host->mrq)
 		omap_hsmmc_xfer_done(host, data);
 }
 
-- 
1.7.11.1.25.g0e18bef

Re: [PATCH 2/2] mmc: omap_hsmmc: cleanup the bitmap definitions of Interrupt Register

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]

Hi,

On Tue, Aug 28, 2012 at 06:49:07PM +0530, Venkatraman S wrote:
> Define the most frequently used bitmasks of the Interrupt Enable /
> Interrupt Status register with consistent naming ( with _EN suffix).
> 
> Use meaningful concatenation of bitfields for INT_EN_MASK, which shows
> which interrupts are enabled by default.
> No functional changes.
> 
> Signed-off-by: Venkatraman S <svenkatr@ti.com>

Acked-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/mmc/host/omap_hsmmc.c | 51 ++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 57e86a4..03c2362 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -79,28 +79,16 @@
>  #define CLKD_SHIFT		6
>  #define DTO_MASK		0x000F0000
>  #define DTO_SHIFT		16
> -#define INT_EN_MASK		0x306E0033
> -#define BWR_ENABLE		(1 << 4)
> -#define BRR_ENABLE		(1 << 5)
> -#define DTO_ENABLE		(1 << 20)
>  #define INIT_STREAM		(1 << 1)
>  #define DP_SELECT		(1 << 21)
>  #define DDIR			(1 << 4)
> -#define DMA_EN			0x1
> +#define DMAE			0x1
>  #define MSBS			(1 << 5)
>  #define BCE			(1 << 1)
>  #define FOUR_BIT		(1 << 1)
>  #define DDR			(1 << 19)
>  #define DW8			(1 << 5)
> -#define CC			0x1
> -#define TC			0x02
>  #define OD			0x1
> -#define ERR			(1 << 15)
> -#define CMD_TIMEOUT		(1 << 16)
> -#define DATA_TIMEOUT		(1 << 20)
> -#define CMD_CRC			(1 << 17)
> -#define DATA_CRC		(1 << 21)
> -#define CARD_ERR		(1 << 28)
>  #define STAT_CLEAR		0xFFFFFFFF
>  #define INIT_STREAM_CMD		0x00000000
>  #define DUAL_VOLT_OCR_BIT	7
> @@ -109,6 +97,25 @@
>  #define SOFTRESET		(1 << 1)
>  #define RESETDONE		(1 << 0)
>  
> +/* Interrupt masks for IE and ISE register */
> +#define CC_EN			(1 << 0)
> +#define TC_EN			(1 << 1)
> +#define BWR_EN			(1 << 4)
> +#define BRR_EN			(1 << 5)
> +#define ERR_EN			(1 << 15)
> +#define CTO_EN			(1 << 16)
> +#define CCRC_EN		(1 << 17)
> +#define CEB_EN			(1 << 18)
> +#define CIE_EN			(1 << 19)
> +#define DTO_EN			(1 << 20)
> +#define DCRC_EN		(1 << 21)
> +#define DEB_EN			(1 << 22)
> +#define CERR_EN		(1 << 28)
> +#define BADA_EN		(1 << 29)
> +
> +#define INT_EN_MASK		(BADA_EN | CERR_EN | DEB_EN | DCRC_EN | \
> +		CIE_EN | CEB_EN | CCRC_EN | BRR_EN | BWR_EN | TC_EN | CC_EN)
> +
>  #define MMC_AUTOSUSPEND_DELAY	100
>  #define MMC_TIMEOUT_MS		20
>  #define OMAP_MMC_MIN_CLOCK	400000
> @@ -453,7 +460,7 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
>  	unsigned int irq_mask;
>  
>  	if (host->use_dma)
> -		irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
> +		irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
>  	else
>  		irq_mask = INT_EN_MASK;
>  
> @@ -673,8 +680,8 @@ static void send_init_stream(struct omap_hsmmc_host *host)
>  	OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD);
>  
>  	timeout = jiffies + msecs_to_jiffies(MMC_TIMEOUT_MS);
> -	while ((reg != CC) && time_before(jiffies, timeout))
> -		reg = OMAP_HSMMC_READ(host->base, STAT) & CC;
> +	while ((reg != CC_EN) && time_before(jiffies, timeout))
> +		reg = OMAP_HSMMC_READ(host->base, STAT) & CC_EN;
>  
>  	OMAP_HSMMC_WRITE(host->base, CON,
>  		OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
> @@ -765,7 +772,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
>  	}
>  
>  	if (host->use_dma)
> -		cmdreg |= DMA_EN;
> +		cmdreg |= DMAE;
>  
>  	host->req_in_progress = 1;
>  
> @@ -988,11 +995,11 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>  	data = host->data;
>  	dev_vdbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
>  
> -	if (status & ERR) {
> +	if (status & ERR_EN) {
>  		omap_hsmmc_dbg_report_irq(host, status);
> -		if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
> +		if (status & (CTO_EN | DTO_EN))
>  			hsmmc_command_incomplete(host, -ETIMEDOUT);
> -		else if (status & (CMD_CRC | DATA_CRC))
> +		else if (status & (CCRC_EN | DCRC_EN))
>  			hsmmc_command_incomplete(host, -EILSEQ);
>  
>  		end_cmd = 1;
> @@ -1002,9 +1009,9 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>  		}
>  	}
>  
> -	if (end_cmd || ((status & CC) && host->cmd))
> +	if (end_cmd || ((status & CC_EN) && host->cmd))
>  		omap_hsmmc_cmd_done(host, host->cmd);
> -	if ((end_trans || (status & TC)) && host->mrq)
> +	if ((end_trans || (status & TC_EN)) && host->mrq)
>  		omap_hsmmc_xfer_done(host, data);
>  }
>  
> -- 
> 1.7.11.1.25.g0e18bef
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] mmc: omap_hsmmc: cleanup the bitmap definitions of Interrupt Register

[T Krishnamoorthy, Balaji]

On Tue, Aug 28, 2012 at 6:49 PM, Venkatraman S <svenkatr@ti.com> wrote:
> Define the most frequently used bitmasks of the Interrupt Enable /
> Interrupt Status register with consistent naming ( with _EN suffix).
>
> Use meaningful concatenation of bitfields for INT_EN_MASK, which shows
> which interrupts are enabled by default.
> No functional changes.
>
> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> ---
>  drivers/mmc/host/omap_hsmmc.c | 51 ++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 57e86a4..03c2362 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -79,28 +79,16 @@
>  #define CLKD_SHIFT             6
>  #define DTO_MASK               0x000F0000
>  #define DTO_SHIFT              16
> -#define INT_EN_MASK            0x306E0033
> -#define BWR_ENABLE             (1 << 4)
> -#define BRR_ENABLE             (1 << 5)
> -#define DTO_ENABLE             (1 << 20)
>  #define INIT_STREAM            (1 << 1)
>  #define DP_SELECT              (1 << 21)
>  #define DDIR                   (1 << 4)
> -#define DMA_EN                 0x1
> +#define DMAE                   0x1

This change is not needed or may not be part of this patch

>  #define MSBS                   (1 << 5)
>  #define BCE                    (1 << 1)
>  #define FOUR_BIT               (1 << 1)
>  #define DDR                    (1 << 19)
>  #define DW8                    (1 << 5)
> -#define CC                     0x1
> -#define TC                     0x02
>  #define OD                     0x1
> -#define ERR                    (1 << 15)
> -#define CMD_TIMEOUT            (1 << 16)
> -#define DATA_TIMEOUT           (1 << 20)
> -#define CMD_CRC                        (1 << 17)
> -#define DATA_CRC               (1 << 21)
> -#define CARD_ERR               (1 << 28)
>  #define STAT_CLEAR             0xFFFFFFFF
>  #define INIT_STREAM_CMD                0x00000000
>  #define DUAL_VOLT_OCR_BIT      7
> @@ -109,6 +97,25 @@
>  #define SOFTRESET              (1 << 1)
>  #define RESETDONE              (1 << 0)
>
> +/* Interrupt masks for IE and ISE register */
> +#define CC_EN                  (1 << 0)
> +#define TC_EN                  (1 << 1)

Minor comment:
You might want to retain CC, TC...  instead of CC_EN as before.
CC_EN is not mentioned in TRM. It would be better to reuse CC
(similarly for other bits fields too) for MMCHS_IE, MMCHS_ISE
inorder to reduce the number of #define's

> +#define BWR_EN                 (1 << 4)
> +#define BRR_EN                 (1 << 5)
> +#define ERR_EN                 (1 << 15)

ERR_EN is not applicable for Interrupt masks for IE and ISE register

> +#define CTO_EN                 (1 << 16)
> +#define CCRC_EN                (1 << 17)
> +#define CEB_EN                 (1 << 18)
> +#define CIE_EN                 (1 << 19)
> +#define DTO_EN                 (1 << 20)
> +#define DCRC_EN                (1 << 21)
> +#define DEB_EN                 (1 << 22)
> +#define CERR_EN                (1 << 28)
> +#define BADA_EN                (1 << 29)
> +
> +#define INT_EN_MASK            (BADA_EN | CERR_EN | DEB_EN | DCRC_EN | \
> +               CIE_EN | CEB_EN | CCRC_EN | BRR_EN | BWR_EN | TC_EN | CC_EN)
> +
>  #define MMC_AUTOSUSPEND_DELAY  100
>  #define MMC_TIMEOUT_MS         20
>  #define OMAP_MMC_MIN_CLOCK     400000
> @@ -453,7 +460,7 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
>         unsigned int irq_mask;
>
>         if (host->use_dma)
> -               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
> +               irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
>         else
>                 irq_mask = INT_EN_MASK;
>
> @@ -673,8 +680,8 @@ static void send_init_stream(struct omap_hsmmc_host *host)
>         OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD);
>
>         timeout = jiffies + msecs_to_jiffies(MMC_TIMEOUT_MS);
> -       while ((reg != CC) && time_before(jiffies, timeout))
> -               reg = OMAP_HSMMC_READ(host->base, STAT) & CC;
> +       while ((reg != CC_EN) && time_before(jiffies, timeout))
> +               reg = OMAP_HSMMC_READ(host->base, STAT) & CC_EN;
>
>         OMAP_HSMMC_WRITE(host->base, CON,
>                 OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
> @@ -765,7 +772,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
>         }
>
>         if (host->use_dma)
> -               cmdreg |= DMA_EN;
> +               cmdreg |= DMAE;

In that case, this change can be avoided and in couple of other places too ...

>
>         host->req_in_progress = 1;
>
> @@ -988,11 +995,11 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>         data = host->data;
>         dev_vdbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
>
> -       if (status & ERR) {
> +       if (status & ERR_EN) {
>                 omap_hsmmc_dbg_report_irq(host, status);
> -               if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
> +               if (status & (CTO_EN | DTO_EN))
>                         hsmmc_command_incomplete(host, -ETIMEDOUT);
> -               else if (status & (CMD_CRC | DATA_CRC))
> +               else if (status & (CCRC_EN | DCRC_EN))
>                         hsmmc_command_incomplete(host, -EILSEQ);
>
>                 end_cmd = 1;
> @@ -1002,9 +1009,9 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>                 }
>         }
>
> -       if (end_cmd || ((status & CC) && host->cmd))
> +       if (end_cmd || ((status & CC_EN) && host->cmd))
>                 omap_hsmmc_cmd_done(host, host->cmd);
> -       if ((end_trans || (status & TC)) && host->mrq)
> +       if ((end_trans || (status & TC_EN)) && host->mrq)
>                 omap_hsmmc_xfer_done(host, data);
>  }
>
> --
> 1.7.11.1.25.g0e18bef
>
> --
> 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: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7553 bytes --]

On Tue, Aug 28, 2012 at 06:49:06PM +0530, Venkatraman S wrote:
> omap hsmmc controller IP has a built in timer that can be programmed to
> guard against unresponsive operations. But its range is very narrow,
> and the maximum countable time is a few seconds.
> 
> Card maintenance operations like BKOPS and MMC_ERASE and long
> stream writes like packed command require timers of order of
> several minutes, much beyond the capability of the IP timer.
> So get rid of using the IP timer entirely and use kernel's hrtimer
> functionality for guarding the device operations.
> As part of this change, a workaround that disabled timeouts for
> MMC_ERASE command is removed, and the arbitary timing of 100ms
> is used only when the timeout is not explicitly specified by core.
> 
> A trivial change to get rid of unnecessary dealiasing of host->data
> in omap_hsmmc_do_irq is also included.
> 
> Signed-off-by: Venkatraman S <svenkatr@ti.com>

Now it looks good:

Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
> 
> v1->v2:
>    Fix typos in commit message.
>    Add checks to handle subtle races between MMC IRQ and HRTIMER IRQ
>    Mark set_guard_timer function as static
>    (Felipe's comment to use macros for INT_EN_MASK is done as a separate patch )
>  drivers/mmc/host/omap_hsmmc.c | 96 ++++++++++++++++++++++---------------------
>  1 file changed, 50 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 9afdd20..57e86a4 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -79,7 +79,7 @@
>  #define CLKD_SHIFT		6
>  #define DTO_MASK		0x000F0000
>  #define DTO_SHIFT		16
> -#define INT_EN_MASK		0x307F0033
> +#define INT_EN_MASK		0x306E0033
>  #define BWR_ENABLE		(1 << 4)
>  #define BRR_ENABLE		(1 << 5)
>  #define DTO_ENABLE		(1 << 20)
> @@ -160,6 +160,7 @@ struct omap_hsmmc_host {
>  	unsigned int		dma_sg_idx;
>  	unsigned char		bus_mode;
>  	unsigned char		power_mode;
> +	unsigned int		ns_per_clk_cycle;
>  	int			suspended;
>  	int			irq;
>  	int			use_dma, dma_ch;
> @@ -172,6 +173,7 @@ struct omap_hsmmc_host {
>  	int			reqs_blocked;
>  	int			use_reg;
>  	int			req_in_progress;
> +	struct hrtimer	guard_timer;
>  	struct omap_hsmmc_next	next_data;
>  
>  	struct	omap_mmc_platform_data	*pdata;
> @@ -455,10 +457,6 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host,
>  	else
>  		irq_mask = INT_EN_MASK;
>  
> -	/* Disable timeout for erases */
> -	if (cmd->opcode == MMC_ERASE)
> -		irq_mask &= ~DTO_ENABLE;
> -
>  	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>  	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>  	OMAP_HSMMC_WRITE(host->base, IE, irq_mask);
> @@ -508,6 +506,9 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
>  		&& time_before(jiffies, timeout))
>  		cpu_relax();
>  
> +	if (ios->clock)
> +		host->ns_per_clk_cycle = DIV_ROUND_UP(NSEC_PER_SEC, ios->clock);
> +
>  	omap_hsmmc_start_clock(host);
>  }
>  
> @@ -824,7 +825,7 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
>  		omap_hsmmc_request_done(host, mrq);
>  		return;
>  	}
> -
> +	hrtimer_cancel(&host->guard_timer);
>  	host->data = NULL;
>  
>  	if (!data->error)
> @@ -859,8 +860,11 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct mmc_command *cmd)
>  			cmd->resp[0] = OMAP_HSMMC_READ(host->base, RSP10);
>  		}
>  	}
> -	if ((host->data == NULL && !host->response_busy) || cmd->error)
> +	if ((host->data == NULL && !host->response_busy) || cmd->error) {
> +		if (cmd->error != -ETIMEDOUT)
> +			hrtimer_cancel(&host->guard_timer);
>  		omap_hsmmc_request_done(host, cmd->mrq);
> +	}
>  }
>  
>  /*
> @@ -992,7 +996,7 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>  			hsmmc_command_incomplete(host, -EILSEQ);
>  
>  		end_cmd = 1;
> -		if (host->data || host->response_busy) {
> +		if (data || host->response_busy) {
>  			end_trans = 1;
>  			host->response_busy = 0;
>  		}
> @@ -1292,41 +1296,35 @@ static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
>  	return 0;
>  }
>  
> -static void set_data_timeout(struct omap_hsmmc_host *host,
> -			     unsigned int timeout_ns,
> -			     unsigned int timeout_clks)
> +static void set_guard_timer(struct omap_hsmmc_host *host,
> +		unsigned long timeout_ms, unsigned long timeout_ns,
> +		unsigned int timeout_clks)
>  {
> -	unsigned int timeout, cycle_ns;
> -	uint32_t reg, clkd, dto = 0;
> +	ktime_t gtime;
> +	unsigned int sec, nsec;
>  
> -	reg = OMAP_HSMMC_READ(host->base, SYSCTL);
> -	clkd = (reg & CLKD_MASK) >> CLKD_SHIFT;
> -	if (clkd == 0)
> -		clkd = 1;
> +	sec = timeout_ms / MSEC_PER_SEC;
> +	nsec = (timeout_ms % MSEC_PER_SEC) * NSEC_PER_MSEC + timeout_ns;
>  
> -	cycle_ns = 1000000000 / (clk_get_rate(host->fclk) / clkd);
> -	timeout = timeout_ns / cycle_ns;
> -	timeout += timeout_clks;
> -	if (timeout) {
> -		while ((timeout & 0x80000000) == 0) {
> -			dto += 1;
> -			timeout <<= 1;
> -		}
> -		dto = 31 - dto;
> -		timeout <<= 1;
> -		if (timeout && dto)
> -			dto += 1;
> -		if (dto >= 13)
> -			dto -= 13;
> -		else
> -			dto = 0;
> -		if (dto > 14)
> -			dto = 14;
> -	}
> +	nsec += timeout_clks * host->ns_per_clk_cycle;
> +	gtime = ktime_set(sec, nsec);
>  
> -	reg &= ~DTO_MASK;
> -	reg |= dto << DTO_SHIFT;
> -	OMAP_HSMMC_WRITE(host->base, SYSCTL, reg);
> +	hrtimer_start(&host->guard_timer, gtime, HRTIMER_MODE_REL);
> +}
> +
> +static enum hrtimer_restart omap_hsmmc_timedout(struct hrtimer *gtimer)
> +{
> +	struct omap_hsmmc_host *host;
> +	host = container_of(gtimer, struct omap_hsmmc_host, guard_timer);
> +	if (host->req_in_progress) {
> +		omap_hsmmc_disable_irq(host);
> +		hsmmc_command_incomplete(host, -ETIMEDOUT);
> +		host->response_busy = 0;
> +		omap_hsmmc_cmd_done(host, host->cmd);
> +		if (host->data)
> +			omap_hsmmc_xfer_done(host, host->data);
> +	}
> +	return HRTIMER_NORESTART;
>  }
>  
>  /*
> @@ -1340,18 +1338,22 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
>  
>  	if (req->data == NULL) {
>  		OMAP_HSMMC_WRITE(host->base, BLK, 0);
> -		/*
> -		 * Set an arbitrary 100ms data timeout for commands with
> -		 * busy signal.
> -		 */
> -		if (req->cmd->flags & MMC_RSP_BUSY)
> -			set_data_timeout(host, 100000000U, 0);
> +		if (req->cmd->cmd_timeout_ms == 0) {
> +			/*
> +			 * Set an arbitrary 100ms data timeout for commands with
> +			 * busy signal.
> +			 */
> +			set_guard_timer(host, 100, 0, 0);
> +		} else {
> +			set_guard_timer(host, req->cmd->cmd_timeout_ms, 0, 0);
> +		}
>  		return 0;
>  	}
>  
>  	OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
>  					| (req->data->blocks << 16));
> -	set_data_timeout(host, req->data->timeout_ns, req->data->timeout_clks);
> +	set_guard_timer(host, 0, req->data->timeout_ns,
> +			req->data->timeout_clks);
>  
>  	if (host->use_dma) {
>  		ret = omap_hsmmc_start_dma_transfer(host, req);
> @@ -1921,6 +1923,8 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
>  		pdata->suspend = omap_hsmmc_suspend_cdirq;
>  		pdata->resume = omap_hsmmc_resume_cdirq;
>  	}
> +	hrtimer_init(&host->guard_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	host->guard_timer.function = omap_hsmmc_timedout;
>  
>  	omap_hsmmc_disable_irq(host);
>  
> -- 
> 1.7.11.1.25.g0e18bef
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

[Kevin Hilman]

Venkatraman S <svenkatr@ti.com> writes:

> omap hsmmc controller IP has a built in timer that can be programmed to
> guard against unresponsive operations. But its range is very narrow,
> and the maximum countable time is a few seconds.
>
> Card maintenance operations like BKOPS and MMC_ERASE and long
> stream writes like packed command require timers of order of
> several minutes, much beyond the capability of the IP timer.
> So get rid of using the IP timer entirely and use kernel's hrtimer
> functionality for guarding the device operations.
> As part of this change, a workaround that disabled timeouts for
> MMC_ERASE command is removed, and the arbitary timing of 100ms
> is used only when the timeout is not explicitly specified by core.
>
> A trivial change to get rid of unnecessary dealiasing of host->data
> in omap_hsmmc_do_irq is also included.
>
> Signed-off-by: Venkatraman S <svenkatr@ti.com>

Dumb question: if the timers needed are on the order of minutes, why do
you need to use high-resolution timers?  I'm guessing the normal
kernel-internal timers should suffice here (see <linux/timer.h,
init_timer().)

Also, if they're on the order of minutes, I assume the actual firing
doesn't have to be precise, so deferrable timers can probably be used
(and thus help PM by coalescing wakeup events.) 

If my assumptions are true, it might be worth considering using the
normal timers, and use init_timer_deferrable().

Kevin

Re: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

[S, Venkatraman]

On Sat, Sep 8, 2012 at 3:29 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Venkatraman S <svenkatr@ti.com> writes:
>
>> omap hsmmc controller IP has a built in timer that can be programmed to
>> guard against unresponsive operations. But its range is very narrow,
>> and the maximum countable time is a few seconds.
>>
>> Card maintenance operations like BKOPS and MMC_ERASE and long
>> stream writes like packed command require timers of order of
>> several minutes, much beyond the capability of the IP timer.
>> So get rid of using the IP timer entirely and use kernel's hrtimer
>> functionality for guarding the device operations.
>> As part of this change, a workaround that disabled timeouts for
>> MMC_ERASE command is removed, and the arbitary timing of 100ms
>> is used only when the timeout is not explicitly specified by core.
>>
>> A trivial change to get rid of unnecessary dealiasing of host->data
>> in omap_hsmmc_do_irq is also included.
>>
>> Signed-off-by: Venkatraman S <svenkatr@ti.com>
>
> Dumb question: if the timers needed are on the order of minutes, why do
> you need to use high-resolution timers?  I'm guessing the normal
> kernel-internal timers should suffice here (see <linux/timer.h,
> init_timer().)
>
Both sub ms and long duration timers are needed, based on the type of operation.
The eMMC standard has evolved to define new commands which require
such long duration
operations. The short ("control") commands do need high precision.

> Also, if they're on the order of minutes, I assume the actual firing
> doesn't have to be precise, so deferrable timers can probably be used
> (and thus help PM by coalescing wakeup events.)
>
> If my assumptions are true, it might be worth considering using the
> normal timers, and use init_timer_deferrable().
>
I'll certainly look into this. One way is to use the IP timer for
short commands and
deferrable timers for long ones - but I reckon it would be messy.

I've actually put this patch on hold - as someone pointed out to me that
this violates some sections of eMMC spec.


> Kevin