[PATCH 0/5] mmc: omap_hsmmc: Few patches for omap_hsmmc

[PATCH 0/5] mmc: omap_hsmmc: Few patches for omap_hsmmc

[Venkatraman S]

Hi Chris,
  Please review and merge the below patches.
The first one is a bug fix that would be required to be sent
for 3.7-rcX, others are intended for 3.8.

Thanks,
Venkat.
=============

Felipe Balbi (1):
  mmc: omap_hsmmc: introduce omap_hsmmc_prepare/complete

Hebbar, Gururaja (1):
  mmc: omap_hsmmc: Enable HSPE bit for high speed cards

Venkatraman S (3):
  mmc: omap_hsmmc: Avoid host->cmd dereference during data transfer    
    failures
  mmc: omap_hsmmc: cleanup the bitmap definitions of Interrupt Register
  mmc: omap_hsmmc: convert critical failure reports to dev_err

 .../devicetree/bindings/mmc/ti-omap-hsmmc.txt      |   1 +
 arch/arm/plat-omap/include/plat/mmc.h              |   1 +
 drivers/mmc/host/omap_hsmmc.c                      | 143 +++++++++++++--------
 3 files changed, 94 insertions(+), 51 deletions(-)

-- 
1.8.0

[PATCH 1/5] mmc: omap_hsmmc: Avoid host->cmd dereference during data transfer failures

[Venkatraman S]

Sometimes, a error occurs just after the Command has been reported
to be successful (CC=1) but before data transfer completes (TC=1).
Setting end_cmd=1 here leads to a NULL pointer dereference of
host->cmd as the command complete has previously been handled.

Set end_cmd only when command complete has not been handled
before, else a NULL pointer dereference occurs.

CC: stable@vger.kernel.org
Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 5434fd8..06d2e03 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -996,7 +996,8 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
 		else if (status & (CMD_CRC | DATA_CRC))
 			hsmmc_command_incomplete(host, -EILSEQ);
 
-		end_cmd = 1;
+		if (host->cmd)
+			end_cmd = 1;
 		if (host->data || host->response_busy) {
 			end_trans = 1;
 			host->response_busy = 0;
-- 
1.8.0

Re: [PATCH 1/5] mmc: omap_hsmmc: Avoid host->cmd dereference during data transfer failures

[Balaji T K]

On Tuesday 06 November 2012 10:22 PM, Venkatraman S wrote:
> Sometimes, a error occurs just after the Command has been reported
> to be successful (CC=1) but before data transfer completes (TC=1).
> Setting end_cmd=1 here leads to a NULL pointer dereference of
> host->cmd as the command complete has previously been handled.
>
> Set end_cmd only when command complete has not been handled
> before, else a NULL pointer dereference occurs.
>
> CC: stable@vger.kernel.org

Removing stable list as this is not applicable for previous major
release. Sent a patch to fix the same in different way.

> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> ---
>   drivers/mmc/host/omap_hsmmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 5434fd8..06d2e03 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -996,7 +996,8 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
>   		else if (status & (CMD_CRC | DATA_CRC))
>   			hsmmc_command_incomplete(host, -EILSEQ);
>
> -		end_cmd = 1;
> +		if (host->cmd)
> +			end_cmd = 1;
>   		if (host->data || host->response_busy) {
>   			end_trans = 1;
>   			host->response_busy = 0;
>

[PATCH 2/5] mmc: omap_hsmmc: Enable HSPE bit for high speed cards

[Venkatraman S]

From: "Hebbar, Gururaja" <gururaja.hebbar@ti.com>

HSMMC IP on AM33xx need a special setting to handle High-speed cards.
Other platforms like TI81xx, OMAP4 may need this as-well. This depends
on the HSMMC IP timing closure done for the high speed cards.

>From AM335x TRM (SPRUH73F - 18.3.12 Output Signals Generation)

The MMC/SD/SDIO output signals can be driven on either falling edge or
rising edge depending on the SD_HCTL[2] HSPE bit. This feature allows
to reach better timing performance, and thus to increase data transfer
frequency.

There are few pre-requisites for enabling the HSPE bit
- Controller should support High-Speed-Enable Bit and
- Controller should not be using DDR Mode and
- Controller should advertise that it supports High Speed in
  capabilities register and
- MMC/SD clock coming out of controller > 25MHz

Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 .../devicetree/bindings/mmc/ti-omap-hsmmc.txt      |  1 +
 arch/arm/plat-omap/include/plat/mmc.h              |  1 +
 drivers/mmc/host/omap_hsmmc.c                      | 30 +++++++++++++++++++++-
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index be76a23..ed271fc 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -19,6 +19,7 @@ ti,dual-volt: boolean, supports dual voltage cards
 "supply-name" examples are "vmmc", "vmmc_aux" etc
 ti,non-removable: non-removable slot (like eMMC)
 ti,needs-special-reset: Requires a special softreset sequence
+ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed
 
 Example:
 	mmc1: mmc@0x4809c000 {
diff --git a/arch/arm/plat-omap/include/plat/mmc.h b/arch/arm/plat-omap/include/plat/mmc.h
index 8b4e4f2..346af5b 100644
--- a/arch/arm/plat-omap/include/plat/mmc.h
+++ b/arch/arm/plat-omap/include/plat/mmc.h
@@ -126,6 +126,7 @@ struct omap_mmc_platform_data {
 		/* we can put the features above into this variable */
 #define HSMMC_HAS_PBIAS		(1 << 0)
 #define HSMMC_HAS_UPDATED_RESET	(1 << 1)
+#define HSMMC_HAS_HSPE_SUPPORT	(1 << 2)
 		unsigned features;
 
 		int switch_pin;			/* gpio (card detect) */
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 06d2e03..c277da4 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -63,6 +63,7 @@
 
 #define VS18			(1 << 26)
 #define VS30			(1 << 25)
+#define HSS			(1 << 21)
 #define SDVS18			(0x5 << 9)
 #define SDVS30			(0x6 << 9)
 #define SDVS33			(0x7 << 9)
@@ -90,6 +91,7 @@
 #define MSBS			(1 << 5)
 #define BCE			(1 << 1)
 #define FOUR_BIT		(1 << 1)
+#define HSPE			(1 << 2)
 #define DDR			(1 << 19)
 #define DW8			(1 << 5)
 #define CC			0x1
@@ -495,6 +497,7 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
 	struct mmc_ios *ios = &host->mmc->ios;
 	unsigned long regval;
 	unsigned long timeout;
+	unsigned long clkdiv;
 
 	dev_vdbg(mmc_dev(host->mmc), "Set clock to %uHz\n", ios->clock);
 
@@ -502,7 +505,8 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
 
 	regval = OMAP_HSMMC_READ(host->base, SYSCTL);
 	regval = regval & ~(CLKD_MASK | DTO_MASK);
-	regval = regval | (calc_divisor(host, ios) << 6) | (DTO << 16);
+	clkdiv = calc_divisor(host, ios);
+	regval = regval | (clkdiv << 6) | (DTO << 16);
 	OMAP_HSMMC_WRITE(host->base, SYSCTL, regval);
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 		OMAP_HSMMC_READ(host->base, SYSCTL) | ICE);
@@ -513,6 +517,27 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
 		&& time_before(jiffies, timeout))
 		cpu_relax();
 
+	/*
+	 * Enable High-Speed Support
+	 * Pre-Requisites
+	 *	- Controller should support High-Speed-Enable Bit
+	 *	- Controller should not be using DDR Mode
+	 *	- Controller should advertise that it supports High Speed
+	 *	  in capabilities register
+	 *	- MMC/SD clock coming out of controller > 25MHz
+	 */
+	if ((mmc_slot(host).features & HSMMC_HAS_HSPE_SUPPORT) &&
+	    (ios->timing != MMC_TIMING_UHS_DDR50) &&
+	    ((OMAP_HSMMC_READ(host->base, CAPA) & HSS) == HSS)) {
+		regval = OMAP_HSMMC_READ(host->base, HCTL);
+		if (clkdiv && (clk_get_rate(host->fclk)/clkdiv) > 25000000)
+			regval |= HSPE;
+		else
+			regval &= ~HSPE;
+
+		OMAP_HSMMC_WRITE(host->base, HCTL, regval);
+	}
+
 	omap_hsmmc_start_clock(host);
 }
 
@@ -1710,6 +1735,9 @@ static struct omap_mmc_platform_data *of_get_hsmmc_pdata(struct device *dev)
 	if (!of_property_read_u32(np, "max-frequency", &max_freq))
 		pdata->max_freq = max_freq;
 
+	if (of_find_property(np, "ti,needs-special-hs-handling", NULL))
+		pdata->slots[0].features |= HSMMC_HAS_HSPE_SUPPORT;
+
 	return pdata;
 }
 #else
-- 
1.8.0

[PATCH 3/5] mmc: omap_hsmmc: introduce omap_hsmmc_prepare/complete

[Venkatraman S]

From: Felipe Balbi <balbi@ti.com>

prepare() is supposed to prevent new children from
being registered. On the MMC subsystem, children
(new cards) registration starts with the card
detect IRQ.

Move card detect IRQ disabling to prepare() so that
no new cards will be registered while we're trying
to suspend.

Likewise, move card detect IRQ enabling to complete()
so we only try to register new children after our MMC
IP is back up.

Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 44 +++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index c277da4..e91e85a 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -2065,6 +2065,25 @@ static int __devexit omap_hsmmc_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
+static int omap_hsmmc_prepare(struct device *dev)
+{
+	struct omap_hsmmc_host *host = dev_get_drvdata(dev);
+
+	if (host->pdata->suspend)
+		return host->pdata->suspend(dev, host->slot_id);
+
+	return 0;
+}
+
+static void omap_hsmmc_complete(struct device *dev)
+{
+	struct omap_hsmmc_host *host = dev_get_drvdata(dev);
+
+	if (host->pdata->resume)
+		host->pdata->resume(dev, host->slot_id);
+
+}
+
 static int omap_hsmmc_suspend(struct device *dev)
 {
 	int ret = 0;
@@ -2078,23 +2097,10 @@ static int omap_hsmmc_suspend(struct device *dev)
 
 	pm_runtime_get_sync(host->dev);
 	host->suspended = 1;
-	if (host->pdata->suspend) {
-		ret = host->pdata->suspend(dev, host->slot_id);
-		if (ret) {
-			dev_dbg(dev, "Unable to handle MMC board"
-					" level suspend\n");
-			host->suspended = 0;
-			return ret;
-		}
-	}
 	ret = mmc_suspend_host(host->mmc);
 
 	if (ret) {
 		host->suspended = 0;
-		if (host->pdata->resume) {
-			if (host->pdata->resume(dev, host->slot_id))
-				dev_dbg(dev, "Unmask interrupt failed\n");
-		}
 		goto err;
 	}
 
@@ -2131,12 +2137,6 @@ static int omap_hsmmc_resume(struct device *dev)
 	if (!(host->mmc->pm_flags & MMC_PM_KEEP_POWER))
 		omap_hsmmc_conf_bus_power(host);
 
-	if (host->pdata->resume) {
-		ret = host->pdata->resume(dev, host->slot_id);
-		if (ret)
-			dev_dbg(dev, "Unmask interrupt failed\n");
-	}
-
 	omap_hsmmc_protect_card(host);
 
 	/* Notify the core to resume the host */
@@ -2152,8 +2152,10 @@ static int omap_hsmmc_resume(struct device *dev)
 }
 
 #else
+#define omap_hsmmc_prepare	NULL
+#define omap_hsmmc_complete	NULL
 #define omap_hsmmc_suspend	NULL
-#define omap_hsmmc_resume		NULL
+#define omap_hsmmc_resume	NULL
 #endif
 
 static int omap_hsmmc_runtime_suspend(struct device *dev)
@@ -2181,6 +2183,8 @@ static int omap_hsmmc_runtime_resume(struct device *dev)
 static struct dev_pm_ops omap_hsmmc_dev_pm_ops = {
 	.suspend	= omap_hsmmc_suspend,
 	.resume		= omap_hsmmc_resume,
+	.prepare	= omap_hsmmc_prepare,
+	.complete	= omap_hsmmc_complete,
 	.runtime_suspend = omap_hsmmc_runtime_suspend,
 	.runtime_resume = omap_hsmmc_runtime_resume,
 };
-- 
1.8.0

[PATCH 4/5] 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>
Acked-by: Felipe Balbi <balbi@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 54 +++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index e91e85a..d16ef0f 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -80,29 +80,17 @@
 #define CLKD_SHIFT		6
 #define DTO_MASK		0x000F0000
 #define DTO_SHIFT		16
-#define INT_EN_MASK		0x307F0033
-#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 HSPE			(1 << 2)
 #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
@@ -111,6 +99,26 @@
 #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 |\
+		DTO_EN | CIE_EN | CEB_EN | CCRC_EN | CTO_EN | ERR_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
@@ -458,13 +466,13 @@ 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;
 
 	/* Disable timeout for erases */
 	if (cmd->opcode == MMC_ERASE)
-		irq_mask &= ~DTO_ENABLE;
+		irq_mask &= ~DTO_EN;
 
 	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
 	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
@@ -702,8 +710,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);
@@ -794,7 +802,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;
 
@@ -1014,11 +1022,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);
 
 		if (host->cmd)
@@ -1029,9 +1037,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.8.0

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

[Balaji T K]

On Tuesday 06 November 2012 10:22 PM, 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>

Hi Venkat,
Not sure if you had chance to look into my comments on Version 2 of this 
patch

> Acked-by: Felipe Balbi <balbi@ti.com>
> ---
>   drivers/mmc/host/omap_hsmmc.c | 54 +++++++++++++++++++++++++------------------
>   1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index e91e85a..d16ef0f 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -80,29 +80,17 @@
>   #define CLKD_SHIFT		6
>   #define DTO_MASK		0x000F0000
>   #define DTO_SHIFT		16
> -#define INT_EN_MASK		0x307F0033
> -#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 HSPE			(1 << 2)
>   #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
> @@ -111,6 +99,26 @@
>   #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)

You might want to retain CC, TC ... which has been defined and already
used for in many places for MMCHS_STAT instead of CC_EN. CC_EN is not 
mentioned in TRM however CC is defined as bit field in TRM Register spec.
So It would be better to reuse the previously defined CC, TC (and other 
bits fields) 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 |\
> +		DTO_EN | CIE_EN | CEB_EN | CCRC_EN | CTO_EN | ERR_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
> @@ -458,13 +466,13 @@ 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;
>
>   	/* Disable timeout for erases */
>   	if (cmd->opcode == MMC_ERASE)
> -		irq_mask &= ~DTO_ENABLE;
> +		irq_mask &= ~DTO_EN;
>
>   	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>   	OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
> @@ -702,8 +710,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;

By reusing already defined bit fields documented in TRM,
this change and other changes below will not be needed thereby keeping
the patch smaller with less changes and yet removing the magic bit mask
numbers

>
>   	OMAP_HSMMC_WRITE(host->base, CON,
>   		OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
> @@ -794,7 +802,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
>   	}
>
>   	if (host->use_dma)
> -		cmdreg |= DMA_EN;
> +		cmdreg |= DMAE;

DMA_EN for DMA enable is much more readable than DMAE.

>   	host->req_in_progress = 1;
>
> @@ -1014,11 +1022,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) {

ERR can be retained.
This change will not be needed.

>   		omap_hsmmc_dbg_report_irq(host, status);
> -		if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
> +		if (status & (CTO_EN | DTO_EN))

If you really want to use uniform naming you can rename CMD_TIMEOUT,
DATA_TIMEOUT to CTO, DTO ....

>   			hsmmc_command_incomplete(host, -ETIMEDOUT);
> -		else if (status & (CMD_CRC | DATA_CRC))
> +		else if (status & (CCRC_EN | DCRC_EN))
>   			hsmmc_command_incomplete(host, -EILSEQ);
>
>   		if (host->cmd)
> @@ -1029,9 +1037,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))

CC can be retained.
This change will not be needed.

>   		omap_hsmmc_cmd_done(host, host->cmd);
> -	if ((end_trans || (status & TC)) && host->mrq)
> +	if ((end_trans || (status & TC_EN)) && host->mrq)

TC can be retained.
This change will not be needed.

>   		omap_hsmmc_xfer_done(host, data);
>   }
>
>

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

[Venkatraman S]

On Wed, Nov 7, 2012 at 6:54 PM, Balaji T K <balajitk@ti.com> wrote:
> On Tuesday 06 November 2012 10:22 PM, 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>
>
>
> Hi Venkat,
> Not sure if you had chance to look into my comments on Version 2 of this
> patch
>
>
>> Acked-by: Felipe Balbi <balbi@ti.com>
>> ---
>>   drivers/mmc/host/omap_hsmmc.c | 54
>> +++++++++++++++++++++++++------------------
>>   1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index e91e85a..d16ef0f 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -80,29 +80,17 @@
>>   #define CLKD_SHIFT            6
>>   #define DTO_MASK              0x000F0000
>>   #define DTO_SHIFT             16
>> -#define INT_EN_MASK            0x307F0033
>> -#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.
>
Actually its about consistency in naming convention. As part of this
patch, the IE, ISE and STAT family of registers all use the _EN suffix
for the bitfields, so this one had to have something different.

>
>>   #define MSBS                  (1 << 5)
>>   #define BCE                   (1 << 1)
>>   #define FOUR_BIT              (1 << 1)
>>   #define HSPE                  (1 << 2)
>>   #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
>> @@ -111,6 +99,26 @@
>>   #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)
>
>
> You might want to retain CC, TC ... which has been defined and already
> used for in many places for MMCHS_STAT instead of CC_EN. CC_EN is not
> mentioned in TRM however CC is defined as bit field in TRM Register spec.
> So It would be better to reuse the previously defined CC, TC (and other bits
> fields) for MMCHS_IE, MMCHS_ISE inorder to reduce the number of #define's.
>
>
Probably yes - One way would be to use TRM fields as is, but sometimes
it leads to conflicts
like the DMAE/DMA_EN case.

Do you think it would be better to switch to _IE suffix instead of _EN ?

>> +#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
>
Ok. The define would still be needed, but probably I shouldn't include
it in INT_EN_MASK then ?

>
>> +#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 |\
>> +               DTO_EN | CIE_EN | CEB_EN | CCRC_EN | CTO_EN | ERR_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
>> @@ -458,13 +466,13 @@ 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;
>>
>>         /* Disable timeout for erases */
>>         if (cmd->opcode == MMC_ERASE)
>> -               irq_mask &= ~DTO_ENABLE;
>> +               irq_mask &= ~DTO_EN;
>>
>>         OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>>         OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>> @@ -702,8 +710,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;
>
>
> By reusing already defined bit fields documented in TRM,
> this change and other changes below will not be needed thereby keeping
> the patch smaller with less changes and yet removing the magic bit mask
> numbers
>
I don't think the size of the patch is a constraint or a limitation. The overall
intent it to move towards a consistent naming that leads to lesser mistakes
when making patches (using the DMAE bit in the wrong register, for example).

>
>>
>>         OMAP_HSMMC_WRITE(host->base, CON,
>>                 OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
>> @@ -794,7 +802,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host,
>> struct mmc_command *cmd,
>>         }
>>
>>         if (host->use_dma)
>> -               cmdreg |= DMA_EN;
>> +               cmdreg |= DMAE;
>
>
> DMA_EN for DMA enable is much more readable than DMAE.
>
I can't challenge or accept this "readability" claim, but as I said above,
consistency wins. _EN is for IE/ISE register, so this one has to change.
If I apply your 'TRM'  rule, then this should be 'DE' then, which is a lot less
readable.

>
>>         host->req_in_progress = 1;
>>
>> @@ -1014,11 +1022,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) {
>
>
> ERR can be retained.
> This change will not be needed.
>
The individual changes are needed if it's agreed that consistent
naming is better,
and if the _EN suffix makes sense.

>
>>                 omap_hsmmc_dbg_report_irq(host, status);
>> -               if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
>> +               if (status & (CTO_EN | DTO_EN))
>
>
> If you really want to use uniform naming you can rename CMD_TIMEOUT,
> DATA_TIMEOUT to CTO, DTO ....
>
>
>>                         hsmmc_command_incomplete(host, -ETIMEDOUT);
>> -               else if (status & (CMD_CRC | DATA_CRC))
>> +               else if (status & (CCRC_EN | DCRC_EN))
>>                         hsmmc_command_incomplete(host, -EILSEQ);
>>
>>                 if (host->cmd)
>> @@ -1029,9 +1037,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))
>
>
> CC can be retained.
> This change will not be needed.
>
>
>>                 omap_hsmmc_cmd_done(host, host->cmd);
>> -       if ((end_trans || (status & TC)) && host->mrq)
>> +       if ((end_trans || (status & TC_EN)) && host->mrq)
>
>
> TC can be retained.
> This change will not be needed.
>
>>                 omap_hsmmc_xfer_done(host, data);
>>   }
>>
>>

[PATCH 5/5] mmc: omap_hsmmc: convert critical failure reports to dev_err

[Venkatraman S]

Fatal errors for the driver are not reported when just error
debug is enabled. Convert selected dev_dbg to dev_err for
accurate error reporting.

Reported-by: Benoit Cousson <b-cousson@ti.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index d16ef0f..830c59b 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -313,7 +313,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 
 	reg = regulator_get(host->dev, "vmmc");
 	if (IS_ERR(reg)) {
-		dev_dbg(host->dev, "vmmc regulator missing\n");
+		dev_err(host->dev, "vmmc regulator missing\n");
 		return PTR_ERR(reg);
 	} else {
 		mmc_slot(host).set_power = omap_hsmmc_set_power;
@@ -1136,7 +1136,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd)
 
 	return 0;
 err:
-	dev_dbg(mmc_dev(host->mmc), "Unable to switch operating voltage\n");
+	dev_err(mmc_dev(host->mmc), "Unable to switch operating voltage\n");
 	return ret;
 }
 
@@ -1395,7 +1395,7 @@ omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
 	if (host->use_dma) {
 		ret = omap_hsmmc_start_dma_transfer(host, req);
 		if (ret != 0) {
-			dev_dbg(mmc_dev(host->mmc), "MMC start dma failure\n");
+			dev_err(mmc_dev(host->mmc), "MMC start dma failure\n");
 			return ret;
 		}
 	}
@@ -1930,13 +1930,13 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 	ret = request_irq(host->irq, omap_hsmmc_irq, 0,
 			mmc_hostname(mmc), host);
 	if (ret) {
-		dev_dbg(mmc_dev(host->mmc), "Unable to grab HSMMC IRQ\n");
+		dev_err(mmc_dev(host->mmc), "Unable to grab HSMMC IRQ\n");
 		goto err_irq;
 	}
 
 	if (pdata->init != NULL) {
 		if (pdata->init(&pdev->dev) != 0) {
-			dev_dbg(mmc_dev(host->mmc),
+			dev_err(mmc_dev(host->mmc),
 				"Unable to configure MMC IRQs\n");
 			goto err_irq_cd_init;
 		}
@@ -1959,7 +1959,7 @@ static int __devinit omap_hsmmc_probe(struct platform_device *pdev)
 					   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					   mmc_hostname(mmc), host);
 		if (ret) {
-			dev_dbg(mmc_dev(host->mmc),
+			dev_err(mmc_dev(host->mmc),
 				"Unable to grab MMC CD IRQ\n");
 			goto err_irq_cd;
 		}
-- 
1.8.0

Re: [PATCH 0/5] mmc: omap_hsmmc: Few patches for omap_hsmmc

[Chris Ball]

Hi Venkat,

On Tue, Nov 06 2012, Venkatraman S wrote:
> Hi Chris,
>   Please review and merge the below patches.
> The first one is a bug fix that would be required to be sent
> for 3.7-rcX, others are intended for 3.8.

There have been some changes requested to these -- could you resend your
3.8 queue to me, please?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/5] mmc: omap_hsmmc: Few patches for omap_hsmmc

[Venkatraman S]

On Sun, Nov 18, 2012 at 6:39 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi Venkat,
>
> On Tue, Nov 06 2012, Venkatraman S wrote:
>> Hi Chris,
>>   Please review and merge the below patches.
>> The first one is a bug fix that would be required to be sent
>> for 3.7-rcX, others are intended for 3.8.
>
> There have been some changes requested to these -- could you resend your
> 3.8 queue to me, please?
>

Yes - I am sending you now a series of 7 patches that has updated /
cleaned up revisions of
the previous patches. Also, it includes Balaji's mini bug fix series,
which is a better version of
my own fix.

Thanks,
Venkat.