mmc: sdhci-omap: Add UHS/HS200 mode support

sdhci_omap_config_iodelay_pinctrl_state() requires caps and caps2 to be initialized (speed mode capabilities like UHS/HS200) before it is invoked. While mmc_of_parse() initializes caps/caps2 if capabilities is populated in device tree, it will remain uninitialized for capabilities obtained from SDHCI_CAPABILITIES register.
Fix sdhci_omap_config_iodelay_pinctrl_state() to be used even while getting the capabilities from SDHCI_CAPABILITIES register by invoking sdhci_setup_host() before sdhci_omap_config_iodelay_pinctrl_state().

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci-omap.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

drivers/mmc/host/sdhci-omap.c +1 -5

index 1456abd5eeb9..3cce30584d2f 100644
@@ -916,10 +916,6 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 		goto err_put_sync;
 	}
 
-	ret = sdhci_omap_config_iodelay_pinctrl_state(omap_host);
-	if (ret)
-		goto err_put_sync;
-
 	host->mmc_host_ops.get_ro = mmc_gpio_get_ro;
 	host->mmc_host_ops.start_signal_voltage_switch =
 					sdhci_omap_start_signal_voltage_switch;

@@ -930,7 +926,15 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	sdhci_read_caps(host);
 	host->caps |= SDHCI_CAN_DO_ADMA2;
 
-	ret = sdhci_add_host(host);
+	ret = sdhci_setup_host(host);

sdhci can directly get ADMA capability from MMCHS_CAPA register. Remove explicitly setting ADMA here as some instances might not have ADMA enabled. (sdhci_read_caps() is also removed from here since sdhci_setup_host() invokes it).

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci-omap.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/mmc/host/sdhci-omap.c +0 -3

index 3cce30584d2f..0c40b13fb67d 100644
@@ -923,9 +923,6 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	host->mmc_host_ops.card_busy = sdhci_omap_card_busy;
 	host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
 
-	sdhci_read_caps(host);
-	host->caps |= SDHCI_CAN_DO_ADMA2;
-
 	ret = sdhci_setup_host(host);
 	if (ret)
 		goto err_put_sync;

Errata i843 in AM572x Sitara Processors Silicon Revision 2.0, 1.1 (SPRZ429K July 2014?Revised March 2017 [1]) mentions
PG 1.0/1.1 silicon has limitations w.r.t frequencies at which MMC1/2/3 can operate.

Use soc_device_match() to identify rev 1.0/1.1 silicon and override mmc->f_max according to the errata workaround. "max-frequency" dt property cannot be used since the device tree is added for rev 2.0 silicon.

soc_device_match() is also used in order to get the IODelay values for rev 1.0/1.1 silicon.

[1] -> http://www.ti.com/lit/er/sprz429k/sprz429k.pdf

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci-omap.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

drivers/mmc/host/sdhci-omap.c +34 -1

index 0c40b13fb67d..fbc20a4fbb23 100644
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/sys_soc.h>
 
 #include "sdhci-pltfm.h"
 

@@ -100,6 +101,7 @@ struct sdhci_omap_data {
 };
 
 struct sdhci_omap_host {
+	char			*version;
 	void __iomem		*base;
 	struct device		*dev;
 	struct	regulator	*pbias;

@@ -733,12 +735,21 @@ static struct pinctrl_state
 				  u32 *caps, u32 capmask)
 {
 	struct device *dev = omap_host->dev;
+	char *version = omap_host->version;
 	struct pinctrl_state *pinctrl_state = ERR_PTR(-ENODEV);
+	char str[20];
 
 	if (!(*caps & capmask))
 		goto ret;
 
-	pinctrl_state = pinctrl_lookup_state(omap_host->pinctrl, mode);
+	if (version) {
+		snprintf(str, 20, "%s-%s", mode, version);
+		pinctrl_state = pinctrl_lookup_state(omap_host->pinctrl, str);
+	}
+
+	if (IS_ERR(pinctrl_state))
+		pinctrl_state = pinctrl_lookup_state(omap_host->pinctrl, mode);
+
 	if (IS_ERR(pinctrl_state)) {
 		dev_err(dev, "no pinctrl state for %s mode", mode);
 		*caps &= ~capmask;

@@ -830,6 +841,16 @@ static int sdhci_omap_config_iodelay_pinctrl_state(struct sdhci_omap_host
 	return 0;
 }
 
+static const struct soc_device_attribute sdhci_omap_soc_devices[] = {
+	{
+		.machine = "DRA7[45]*",
+		.revision = "ES1.[01]",
+	},
+	{
+		/* sentinel */
+	}
+};
+
 static int sdhci_omap_probe(struct platform_device *pdev)
 {
 	int ret;

@@ -841,6 +862,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	struct mmc_host *mmc;
 	const struct of_device_id *match;
 	struct sdhci_omap_data *data;
+	const struct soc_device_attribute *soc;
 
 	match = of_match_device(omap_sdhci_match, dev);
 	if (!match)

@@ -875,6 +897,17 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_pltfm_free;
 
+	soc = soc_device_match(sdhci_omap_soc_devices);
+	if (soc) {
+		omap_host->version = "rev11";
+		if (!strcmp(dev_name(dev), "4809c000.mmc"))
+			mmc->f_max = 96000000;
+		if (!strcmp(dev_name(dev), "480b4000.mmc"))
+			mmc->f_max = 48000000;
+		if (!strcmp(dev_name(dev), "480ad000.mmc"))
+			mmc->f_max = 48000000;
+	}
+
 	pltfm_host->clk = devm_clk_get(dev, "fck");
 	if (IS_ERR(pltfm_host->clk)) {
 		ret = PTR_ERR(pltfm_host->clk);

Invoke sdhci_get_of_property defined in sdhci-pltfm.c to read sdhci specific properties from dt node.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci-omap.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mmc/host/sdhci-omap.c +1 -0

index fbc20a4fbb23..314dbe4d7412 100644
@@ -893,6 +893,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	host->ioaddr += offset;
 
 	mmc = host->mmc;
+	sdhci_get_of_property(pdev);
 	ret = mmc_of_parse(mmc);
 	if (ret)
 		goto err_pltfm_free;

Though MMC controller can indicate HS200 mode capability (by using "mmc-hs200-1_8v" dt property), if the IO lines in the board is connected to 3.3v supply, HS200 mode cannot be supported. Such boards have "no-1-8-v" property in their dts file. Disable HS200 mode for boards which have "no-1-8-v" set.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mmc/host/sdhci.c +0 -0

index 2ededa7f43df..aa8b5ca0d1b0 100644

drivers/mmc/host/sdhci.c +0 -0

 	}
 
 	/* Any UHS-I mode in caps implies SDR12 and SDR25 support. */

Add quirk to disable HW timeout if the requested timeout is more than the maximum obtainable timeout.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 27 +++++++++++++++++++++------
 drivers/mmc/host/sdhci.h |  5 +++++
 2 files changed, 26 insertions(+), 6 deletions(-)

drivers/mmc/host/sdhci.c +14 -6

index aa8b5ca0d1b0..1dd117cbeb6e 100644
@@ -767,12 +767,6 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 			break;
 	}
 
-	if (count >= 0xF) {
-		DBG("Too large timeout 0x%x requested for CMD%d!\n",
-		    count, cmd->opcode);
-		count = 0xE;
-	}
-
 	return count;
 }
 

@@ -790,6 +784,16 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
 }
 
+static void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable)
+{
+	if (enable)
+		host->ier |= SDHCI_INT_DATA_TIMEOUT;
+	else
+		host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
+	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
+	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+}
+
 static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -798,6 +802,17 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		host->ops->set_timeout(host, cmd);
 	} else {
 		count = sdhci_calc_timeout(host, cmd);
+		if (count >= 0xF &&
+		    host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT)
+			sdhci_set_data_timeout_irq(host, false);
+		else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT))

drivers/mmc/host/sdhci.h +5 -0

index c95b0a4a7594..ff283ee08854 100644
@@ -437,6 +437,11 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
 /* Controller has CRC in 136 bit Command Response */
 #define SDHCI_QUIRK2_RSP_136_HAS_CRC			(1<<16)
+/*
+ * Disable HW timeout if the requested timeout is more than the maximum
+ * obtainable timeout
+ */
+#define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT			(1<<17)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		timeout += 10 * HZ;
+	}
+	sdhci_mod_timer(host, cmd->mrq, timeout);
+
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);

drivers/mmc/host/sdhci.h +10 -0

index ff283ee08854..29b242fd17de 100644
@@ -332,6 +332,14 @@ struct sdhci_adma2_64_desc {
 /* Allow for a a command request and a data request at the same time */
 #define SDHCI_MAX_MRQS		2
 
+/*
+ * 48bit command and 136 bit response in 100KHz clock could take upto 2.48ms.
+ * However since the start time of the command, the time between
+ * command and response, and the time between response and start of data is
+ * not known, set the command transfer time to 10ms.
+ */
+#define MMC_CMD_TRANSFER_TIME	(10 * NSEC_PER_MSEC) /* max 10 ms */
+
 enum sdhci_cookie {
 	COOKIE_UNMAPPED,
 	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */

@@ -555,6 +563,8 @@ struct sdhci_host {
 	/* Host SDMA buffer boundary. */
 	u32			sdma_boundary;
 
+	u64			data_timeout;
+
 	unsigned long private[0] ____cacheline_aligned;
 };
 

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

Hi,

Quoted 139 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Quoted 4 lines

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Quoted 142 lines · Kishon Vijay Abraham I

Hi,

Quoted 139 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Quoted 6 lines

Quoted 4 lines

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Yes, but I was thinking you would only calculate when it was needed.

Hi Adrian,

Quoted 148 lines · Adrian Hunter

Quoted 147 lines · Kishon Vijay Abraham I

Hi,

Quoted 143 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

Quoted 3 lines

Quoted 1 line

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Yes, but I was thinking you would only calculate when it was needed.

I feel since we would have anyways calculated data_timeout, we should use that instead unless you see a problem with that.

Quoted 153 lines · Kishon Vijay Abraham I

Hi Adrian,

Quoted 150 lines · Adrian Hunter

Quoted 148 lines · Kishon Vijay Abraham I

Hi,

Quoted 139 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Quoted 4 lines

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Quoted 4 lines

Quoted 1 line

Yes, but I was thinking you would only calculate when it was needed.

I feel since we would have anyways calculated data_timeout, we should use that instead unless you see a problem with that.

I would prefer not to calculate data_timeout when a hardware timeout is being used.

Hi Adrian,

Quoted 159 lines · Adrian Hunter

Quoted 157 lines · Kishon Vijay Abraham I

Hi Adrian,

Quoted 143 lines · Adrian Hunter

Quoted 142 lines · Kishon Vijay Abraham I

Hi,

Quoted 139 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Yes, but I was thinking you would only calculate when it was needed.

I feel since we would have anyways calculated data_timeout, we should use that instead unless you see a problem with that.

Quoted 3 lines

I would prefer not to calculate data_timeout when a hardware timeout is being used.

That differs from what I had thought. This patch tries to program a relatively accurate SW timeout value (for data_timer) irrespective of whether hardware timeout is used or not. This only tries to change the 10 Sec SW timeout value programmed for all data transfer commands.

Hi Adrian,

Quoted 166 lines · Kishon Vijay Abraham I

Hi Adrian,

Quoted 162 lines · Adrian Hunter

Quoted 151 lines · Kishon Vijay Abraham I

Hi Adrian,

Quoted 148 lines · Adrian Hunter

Quoted 147 lines · Kishon Vijay Abraham I

Hi,

Quoted 143 lines · Adrian Hunter

Quoted 32 lines · Kishon Vijay Abraham I

sdhci has a 10 second timeout to catch devices that stop responding. Instead of programming 10 second arbitrary value, calculate the total time it would take for the entire transfer to happen and program the timeout value accordingly.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h | 10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +13 -0

index 1dd117cbeb6e..baab67bfa39b 100644
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
 		return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+				  struct mmc_command *cmd,
+				  unsigned int target_timeout)
+{
+	struct mmc_data *data = cmd->data;
+	struct mmc_host *mmc = host->mmc;
+	u64 transfer_time;
+	struct mmc_ios *ios = &mmc->ios;
+	unsigned char bus_width = 1 << ios->bus_width;
+	unsigned int blksz;
+	unsigned int freq;
+
+	if (data) {

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 734

Quoted 10 lines

drivers/mmc/host/sdhci.c +10 -0

+		blksz = data->blksz;
+		freq = host->mmc->actual_clock ? : host->clock;
+		transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+		do_div(transfer_time, freq);
+		/* multiply by '2' to account for any unknowns */
+		transfer_time = transfer_time * 2;
+		/* calculate timeout for the entire data */
+		host->data_timeout = (data->blocks * ((target_timeout *
+						       NSEC_PER_USEC) +
+						       transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds.

drivers/mmc/host/sdhci.c static u32 sdhci_sdma_address(struct sdhci_host *host) · line 739

Quoted 5 lines

drivers/mmc/host/sdhci.c +5 -0

+	} else {
+		host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+	}
+
+	host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

	if (host->data_timeout)
		host->data_timeout += MMC_CMD_TRANSFER_TIME;

drivers/mmc/host/sdhci.c static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) · line 799

Quoted 10 lines

drivers/mmc/host/sdhci.c +3 -0

+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;

@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		if (count >= 0xF)
 			break;
 	}
+	sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
struct mmc_command *cmd, struct mmc_data *data)

{
	unsigned int target_timeout;

	/* timeout in us */
	if (!data)
		target_timeout = cmd->busy_timeout * 1000;
	else {
		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
		if (host->clock && data->timeout_clks) {
			unsigned long long val;

			/*
			 * data->timeout_clks is in units of clock cycles.
			 * host->clock is in Hz.  target_timeout is in us.
			 * Hence, us = 1000000 * cycles / Hz.  Round up.
			 */
			val = 1000000ULL * data->timeout_clks;
			if (do_div(val, host->clock))
				target_timeout++;
			target_timeout += val;
		}
	}

	return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

Quoted 18 lines

drivers/mmc/host/sdhci.c +0 -7

 	return count;
 }

@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	timeout = jiffies;
-	if (!cmd->data && cmd->busy_timeout > 9000)
-		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-	else
-		timeout += 10 * HZ;
-	sdhci_mod_timer(host, cmd->mrq, timeout);
-
 	host->cmd = cmd;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1249

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
 		flags |= SDHCI_CMD_DATA;
 
+	timeout = jiffies;
+	if (host->data_timeout > 0) {

This can be just:

	if (host->data_timeout) {

drivers/mmc/host/sdhci.c void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) · line 1251

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+		timeout += nsecs_to_jiffies(host->data_timeout);
+		host->data_timeout = 0;

It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data().

Also still need:

	else if (!cmd->data && cmd->busy_timeout > 9000) {
		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

sdhci_calc_sw_timeout should have calculated the timeout for this case too no?

Yes, but I was thinking you would only calculate when it was needed.

I feel since we would have anyways calculated data_timeout, we should use that instead unless you see a problem with that.

I would prefer not to calculate data_timeout when a hardware timeout is being used.

Quoted 4 lines

That differs from what I had thought. This patch tries to program a relatively accurate SW timeout value (for data_timer) irrespective of whether hardware timeout is used or not. This only tries to change the 10 Sec SW timeout value programmed for all data transfer commands.

IMHO since we calculate the worst case timeout value we should be using that for all cases where we are able to calculate the timeout value so that we don't give a too high or too low timeout value. Let me know If this sounds okay to you.

Add sdhci_omap_enable_sdio_irq to set CTPL and CLKEXTFREE bits in MMCHS_CON register required to detect asynchronous card interrupt on DAT[1].

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/mmc/host/sdhci-omap.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/mmc/host/sdhci-omap.c +19 -0

index 14dd51b51b41..8ceb3956b211 100644
@@ -36,6 +36,7 @@
 #define CON_DDR			BIT(19)
 #define CON_CLKEXTFREE		BIT(16)
 #define CON_PADEN		BIT(15)
+#define CON_CTPL		BIT(11)
 #define CON_INIT		BIT(1)
 #define CON_OD			BIT(0)
 

@@ -226,6 +227,23 @@ static void sdhci_omap_conf_bus_power(struct sdhci_omap_host *omap_host,
 	}
 }
 
+static void sdhci_omap_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+	u32 reg;
+
+	reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON);
+	if (enable)
+		reg |= (CON_CTPL | CON_CLKEXTFREE);
+	else
+		reg &= ~(CON_CTPL | CON_CLKEXTFREE);
+	sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg);
+
+	sdhci_enable_sdio_irq(mmc, enable);
+}
+
 static inline void sdhci_omap_set_dll(struct sdhci_omap_host *omap_host,
 				      int count)
 {

@@ -962,6 +980,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
 	host->mmc_host_ops.card_busy = sdhci_omap_card_busy;
 	host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
+	host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;
 
 	ret = sdhci_setup_host(host);
 	if (ret)

Changes from v2:
*) Used soc_device_match() instead of pdata-quirks as per Tony's
suggestions.

Add UHS/HS200 mode support in sdhci-omap. The programming sequence for voltage switching, tuning is followed from AM572x TRM
http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf
(Similar to all AM57x/DRA7x SoCs). The patch series also implements workaround for errata published in
http://www.ti.com/lit/er/sprz429k/sprz429k.pdf

patches are created on top of
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next

Patches in v2 already applied to mmc -next are dropped from the series.

Changes from v2:
*) Changed SW timeout logic as per Adrians's suggestion *) Validated SDIO with the current patches (added a couple of fixes
found while adding SDIO support).
*) Used soc_device_match() instead of pdata-quirks as per Tony's
suggestions.

Changes from v1:
*) Only poll on DAT0 and DATI for card_busy status
*) Cleanup iodelay patch as suggested by Tony.
*) Added quirk to disable HW timeout
*) Use the existing data timer but program a relatively accurate
SW timeout value (Impacts all platforms)
*) Fix a bug in sdhci which was using data_timer for non data line
commands

Kishon Vijay Abraham I (11):
mmc: sdhci-omap: Fix when capabilities are obtained from
SDHCI_CAPABILITIES reg
mmc: sdhci-omap: Remove setting ADMA capability in driver
mmc: sdhci-omap: Workaround for Errata i843
mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt
properties
mmc: sdhci: Disable HS200 mode if controller can't support 1.8v
mmc: sdhci: Add quirk to disable HW timeout
mmc: sdhci: Program a relatively accurate SW timeout value
mmc: sdhci-omap: Workaround for Errata i834
dt-bindings: sdhci-omap: Add K2G specific binding
mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC
mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq

 .../devicetree/bindings/mmc/sdhci-omap.txt         |  2 +
 drivers/mmc/host/sdhci-omap.c                      | 78 +++++++++++++++++++---
 drivers/mmc/host/sdhci.c                           | 75 +++++++++++++++++----
 drivers/mmc/host/sdhci.h                           | 15 +++++
 4 files changed, 148 insertions(+), 22 deletions(-)

--
2.11.0

Quoted 50 lines · Kishon Vijay Abraham I

Add UHS/HS200 mode support in sdhci-omap. The programming sequence for voltage switching, tuning is followed from AM572x TRM
http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf
(Similar to all AM57x/DRA7x SoCs). The patch series also implements workaround for errata published in
http://www.ti.com/lit/er/sprz429k/sprz429k.pdf

patches are created on top of
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next

Patches in v2 already applied to mmc -next are dropped from the series.

Changes from v2:
*) Changed SW timeout logic as per Adrians's suggestion *) Validated SDIO with the current patches (added a couple of fixes
found while adding SDIO support).
*) Used soc_device_match() instead of pdata-quirks as per Tony's
suggestions.

Changes from v1:
*) Only poll on DAT0 and DATI for card_busy status
*) Cleanup iodelay patch as suggested by Tony.
*) Added quirk to disable HW timeout
*) Use the existing data timer but program a relatively accurate
SW timeout value (Impacts all platforms)
*) Fix a bug in sdhci which was using data_timer for non data line
commands

Kishon Vijay Abraham I (11):
mmc: sdhci-omap: Fix when capabilities are obtained from
SDHCI_CAPABILITIES reg
mmc: sdhci-omap: Remove setting ADMA capability in driver
mmc: sdhci-omap: Workaround for Errata i843
mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt
properties
mmc: sdhci: Disable HS200 mode if controller can't support 1.8v
mmc: sdhci: Add quirk to disable HW timeout
mmc: sdhci: Program a relatively accurate SW timeout value
mmc: sdhci-omap: Workaround for Errata i834
dt-bindings: sdhci-omap: Add K2G specific binding
mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC
mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq

 .../devicetree/bindings/mmc/sdhci-omap.txt         |  2 +
 drivers/mmc/host/sdhci-omap.c                      | 78 +++++++++++++++++++---
 drivers/mmc/host/sdhci.c                           | 75 +++++++++++++++++----
 drivers/mmc/host/sdhci.h                           | 15 +++++
 4 files changed, 148 insertions(+), 22 deletions(-)

Quoted 3 lines

--
2.11.0

Overall this looks good to me, however it seems like we need to give Adrian a little more time to comment.

Quoted 55 lines · Ulf Hansson

Quoted 53 lines · Kishon Vijay Abraham I

Add UHS/HS200 mode support in sdhci-omap. The programming sequence for voltage switching, tuning is followed from AM572x TRM
http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf
(Similar to all AM57x/DRA7x SoCs). The patch series also implements workaround for errata published in
http://www.ti.com/lit/er/sprz429k/sprz429k.pdf

patches are created on top of
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next

Patches in v2 already applied to mmc -next are dropped from the series.

Changes from v2:
*) Changed SW timeout logic as per Adrians's suggestion *) Validated SDIO with the current patches (added a couple of fixes
found while adding SDIO support).
*) Used soc_device_match() instead of pdata-quirks as per Tony's
suggestions.

Changes from v1:
*) Only poll on DAT0 and DATI for card_busy status
*) Cleanup iodelay patch as suggested by Tony.
*) Added quirk to disable HW timeout
*) Use the existing data timer but program a relatively accurate
SW timeout value (Impacts all platforms)
*) Fix a bug in sdhci which was using data_timer for non data line
commands

Kishon Vijay Abraham I (11):
mmc: sdhci-omap: Fix when capabilities are obtained from
SDHCI_CAPABILITIES reg
mmc: sdhci-omap: Remove setting ADMA capability in driver
mmc: sdhci-omap: Workaround for Errata i843
mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt
properties
mmc: sdhci: Disable HS200 mode if controller can't support 1.8v
mmc: sdhci: Add quirk to disable HW timeout
mmc: sdhci: Program a relatively accurate SW timeout value
mmc: sdhci-omap: Workaround for Errata i834
dt-bindings: sdhci-omap: Add K2G specific binding
mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC
mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq

 .../devicetree/bindings/mmc/sdhci-omap.txt         |  2 +
 drivers/mmc/host/sdhci-omap.c                      | 78 +++++++++++++++++++---
 drivers/mmc/host/sdhci.c                           | 75 +++++++++++++++++----
 drivers/mmc/host/sdhci.h                           | 15 +++++
 4 files changed, 148 insertions(+), 22 deletions(-)

--
2.11.0

Quoted 3 lines

Overall this looks good to me, however it seems like we need to give Adrian a little more time to comment.

I will try to look at it today.