[PATCH 0/4] mci: clean up MCI idle delay handling a bit

[PATCH 0/4] mci: clean up MCI idle delay handling a bit

[Ahmad Fatoum]

Stuff I noticed while looking into actual issues that have been
submitted separately as fixes for master.

Nothing critical in this series, so can go into next. 

Ahmad Fatoum (4):
  include: linux/iopoll.h: evaluate timeout_us argument only once
  mci: bcm2835: allow core to request timeouts longer than 100ms
  mci: sdhci: use sdhci_compute_timeout in sdhci_wait_idle[_data]
  mci: sdhci: reduce duplication in sdhci_wait_idle[_data]

 drivers/mci/mci-bcm2835.c |  2 +-
 drivers/mci/sdhci.c       | 44 +++++++++++++--------------------------
 include/linux/iopoll.h    |  9 ++++----
 3 files changed, 20 insertions(+), 35 deletions(-)

-- 
2.39.5

[PATCH 1/4] include: linux/iopoll.h: evaluate timeout_us argument only once

[Ahmad Fatoum]

We currently evaluate timeout_us three times, which is unexpected and
suboptimal when having complexer timeout calculations.

Define a local variable to fix this.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/linux/iopoll.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index c38190272606..a6fade2a11fc 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -20,7 +20,7 @@
 #  define read_poll_get_time_ns()	0
 # endif
 # ifndef read_poll_is_timeout
-#  define read_poll_is_timeout(s, t)	((void)s, (void)t, 0)
+#  define read_poll_is_timeout(s, t)	((void)(s), (void)(t), 0)
 # endif
 #endif
 
@@ -43,13 +43,14 @@
  */
 #define read_poll_timeout(op, val, cond, timeout_us, args...)	\
 ({ \
-	uint64_t start = (timeout_us) != 0 ? read_poll_get_time_ns() : 0; \
+	uint64_t __timeout_us = (timeout_us); \
+	uint64_t start = __timeout_us ? read_poll_get_time_ns() : 0; \
 	for (;;) { \
 		(val) = op(args); \
 		if (cond) \
 			break; \
-		if ((timeout_us) != 0 && \
-		    read_poll_is_timeout(start, ((timeout_us) * USECOND))) { \
+		if (__timeout_us && \
+		    read_poll_is_timeout(start, __timeout_us * USECOND)) { \
 			(val) = op(args); \
 			break; \
 		} \
-- 
2.39.5

[PATCH 2/4] mci: bcm2835: allow core to request timeouts longer than 100ms

[Ahmad Fatoum]

For longer erase operations in future, the core may want to increase
the busy_timeout value beyond 100ms. Drivers should respect that and
only enforce a minimum timeout duration, but not a maximum one.

This is not a full solution, because some drivers/hardwares indeed have
a maximum duration they can wait, but that should probably be solved via
a max_busy_timeout attribute for MMC hosts like Linux does.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/mci-bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mci/mci-bcm2835.c b/drivers/mci/mci-bcm2835.c
index 0099e4e35753..aad5ae0d9e1d 100644
--- a/drivers/mci/mci-bcm2835.c
+++ b/drivers/mci/mci-bcm2835.c
@@ -129,7 +129,7 @@ static int bcm2835_mci_request(struct mci_host *mci, struct mci_cmd *cmd,
 	}
 
 	/* BCM2xxx SDHCI might take up to 100ms to complete a command */
-	cmd->busy_timeout = 100;
+	cmd->busy_timeout = max(cmd->busy_timeout, 100U);
 
 	ret = sdhci_wait_idle_data(&host->sdhci, cmd);
 	if (ret)
-- 
2.39.5

[PATCH 3/4] mci: sdhci: use sdhci_compute_timeout in sdhci_wait_idle[_data]

[Ahmad Fatoum]

sdhci_compute_timeout looks at the timeout values in both cmd and data
and if both are NULL falls back to SDHCI_CMD_DEFAULT_BUSY_TIMEOUT_NS.

It's thus can be used as-is in sdhci_wait_idle_data to reduce the
difference to sdhci_wait_idle in preparation for their merger.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/sdhci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index 38a108adb1a8..f0c95f2dbfa8 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -856,14 +856,12 @@ int sdhci_wait_idle_data(struct sdhci *host, struct mci_cmd *cmd)
 	int ret;
 
 	mask = SDHCI_CMD_INHIBIT_CMD | SDHCI_CMD_INHIBIT_DATA;
-	timeout_ns = SDHCI_CMD_DEFAULT_BUSY_TIMEOUT_NS;
 
 	if (cmd && (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION ||
 		    mmc_op_tuning(cmd->cmdidx)))
 		mask &= ~SDHCI_CMD_INHIBIT_DATA;
 
-	if (cmd && cmd->busy_timeout != 0)
-		timeout_ns = ms_to_ktime(cmd->busy_timeout);
+	timeout_ns = sdhci_compute_timeout(cmd, NULL);
 
 	ret = wait_on_timeout(timeout_ns,
 			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
-- 
2.39.5

[PATCH 4/4] mci: sdhci: reduce duplication in sdhci_wait_idle[_data]

[Ahmad Fatoum]

Now that the two functions look nearly identical, let's factor out the
common parts into a helper function and keep only the differences.

In future, we may export only a single function once the implications
are better understood, but even now, it's already a win for readability.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/sdhci.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index f0c95f2dbfa8..2d32a8b31188 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -821,17 +821,12 @@ void sdhci_enable_clk(struct sdhci *host, u16 clk)
 	sdhci_write16(host, SDHCI_CLOCK_CONTROL, clk);
 }
 
-int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data)
+static int __sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd,
+			     struct mci_data *data, u32 mask)
 {
-	u32 mask;
 	ktime_t timeout_ns;
 	int ret;
 
-	mask = SDHCI_CMD_INHIBIT_CMD;
-
-	if (data || (cmd && (cmd->resp_type & MMC_RSP_BUSY)))
-		mask |= SDHCI_CMD_INHIBIT_DATA;
-
 	if (cmd && (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION ||
 		    mmc_op_tuning(cmd->cmdidx)))
 		mask &= ~SDHCI_CMD_INHIBIT_DATA;
@@ -849,30 +844,21 @@ int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *da
 	return 0;
 }
 
+int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data)
+{
+	u32 mask = SDHCI_CMD_INHIBIT_CMD;
+
+	if (data || (cmd && (cmd->resp_type & MMC_RSP_BUSY)))
+		mask |= SDHCI_CMD_INHIBIT_DATA;
+
+	return __sdhci_wait_idle(host, cmd, data, mask);
+}
+
 int sdhci_wait_idle_data(struct sdhci *host, struct mci_cmd *cmd)
 {
-	u32 mask;
-	ktime_t timeout_ns;
-	int ret;
+	u32 mask = SDHCI_CMD_INHIBIT_CMD | SDHCI_CMD_INHIBIT_DATA;
 
-	mask = SDHCI_CMD_INHIBIT_CMD | SDHCI_CMD_INHIBIT_DATA;
-
-	if (cmd && (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION ||
-		    mmc_op_tuning(cmd->cmdidx)))
-		mask &= ~SDHCI_CMD_INHIBIT_DATA;
-
-	timeout_ns = sdhci_compute_timeout(cmd, NULL);
-
-	ret = wait_on_timeout(timeout_ns,
-			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
-
-	if (ret) {
-		dev_err(sdhci_dev(host),
-				"SDHCI timeout while waiting for idle\n");
-		return -EBUSY;
-	}
-
-	return 0;
+	return __sdhci_wait_idle(host, cmd, NULL, mask);
 }
 
 void sdhci_set_clock(struct sdhci *host, unsigned int clock, unsigned int input_clock)
-- 
2.39.5