mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: barebox@lists.infradead.org
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Subject: [PATCH master] Revert "mmc: merge block read/write functions"
Date: Fri, 11 Apr 2025 07:28:41 +0200	[thread overview]
Message-ID: <20250411052841.1745388-1-a.fatoum@pengutronix.de> (raw)

The premise of the code merger that they share so much code already that
it makes sense to combine them. This resulted in the introduction
of at least three bugs:

  - Writes were broken intermittently, until they were fixed in
    f646ed68434f ("mci: fix data write")

  - eMMC writes are broken at least on STM32MP1, because eMMC
    expects R1b for STOP_TRANSMISSION, but R1 was used

  - The merged code assumes read_bl_len and write_bl_len are
    identical, which is not necessarily the case on eMMCs
    at lower speed mode

Instead of fixing these by adding even more conditionals into the code,
just revert the offending commit.

Fixes: 38543119546f ("mmc: merge block read/write functions")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/mci-core.c | 156 +++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 75 deletions(-)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 2c82379b0d73..68464f1cf95e 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -260,6 +260,59 @@ static int mci_poll_until_ready(struct mci *mci, int timeout_ms)
 	return 0;
 }
 
+
+/**
+ * Write one or several blocks of data to the card
+ * @param mci_dev MCI instance
+ * @param src Where to read from to write to the card
+ * @param blocknum Block number to write
+ * @param blocks Block count to write
+ * @return Transaction status (0 on success)
+ */
+static int mci_block_write(struct mci *mci, const void *src, int blocknum,
+			   int blocks)
+{
+	struct mci_cmd cmd = {};
+	struct mci_data data;
+	unsigned mmccmd;
+	int ret;
+
+	/*
+	 * Quoting eMMC Spec v5.1 (JEDEC Standard No. 84-B51):
+	 * Due to legacy reasons, a Device may still treat CMD24/25 during
+	 * prg-state (while busy is active) as a legal or illegal command.
+	 * A host should not send CMD24/25 while the Device is in the prg
+	 * state and busy is active.
+	 */
+	ret = mci_poll_until_ready(mci, 1000 /* ms */);
+	if (ret && ret != -ENOSYS)
+		return ret;
+
+	if (blocks > 1)
+		mmccmd = MMC_CMD_WRITE_MULTIPLE_BLOCK;
+	else
+		mmccmd = MMC_CMD_WRITE_SINGLE_BLOCK;
+
+	mci_setup_cmd(&cmd,
+		mmccmd,
+		mci->high_capacity != 0 ? blocknum : blocknum * mci->write_bl_len,
+		MMC_RSP_R1);
+
+	data.src = src;
+	data.blocks = blocks;
+	data.blocksize = mci->write_bl_len;
+	data.flags = MMC_DATA_WRITE;
+
+	ret = mci_send_cmd(mci, &cmd, &data);
+
+	if (ret || blocks > 1) {
+		mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
+		mci_send_cmd(mci, &cmd, NULL);
+        }
+
+	return ret;
+}
+
 /**
  * Erase one or several blocks of data to the card
  * @param mci_dev MCI instance
@@ -320,66 +373,6 @@ int mci_set_blockcount(struct mci *mci, unsigned int cmdarg)
 	return mci_send_cmd(mci, &cmd, NULL);
 }
 
-static int mci_do_block_op(struct mci *mci, const void *src, void *dst, int blocknum,
-		int blocks)
-{
-	struct mci_cmd cmd = {};
-	struct mci_data data;
-	int ret;
-	unsigned mmccmd_multi_block, mmccmd_single_block, mmccmd;
-	unsigned int flags;
-
-	if (dst) {
-		mmccmd_multi_block = MMC_CMD_READ_MULTIPLE_BLOCK;
-		mmccmd_single_block = MMC_CMD_READ_SINGLE_BLOCK;
-		flags = MMC_DATA_READ;
-	} else {
-		/*
-		 * Quoting eMMC Spec v5.1 (JEDEC Standard No. 84-B51):
-		 * Due to legacy reasons, a Device may still treat CMD24/25 during
-		 * prg-state (while busy is active) as a legal or illegal command.
-		 * A host should not send CMD24/25 while the Device is in the prg
-		 * state and busy is active.
-		 */
-		ret = mci_poll_until_ready(mci, 1000 /* ms */);
-		if (ret && ret != -ENOSYS)
-			return ret;
-
-		mmccmd_multi_block = MMC_CMD_WRITE_MULTIPLE_BLOCK;
-		mmccmd_single_block = MMC_CMD_WRITE_SINGLE_BLOCK;
-		flags = MMC_DATA_WRITE;
-	}
-
-	if (blocks > 1)
-		mmccmd = mmccmd_multi_block;
-	else
-		mmccmd = mmccmd_single_block;
-
-	mci_setup_cmd(&cmd,
-		mmccmd,
-		mci->high_capacity != 0 ? blocknum : blocknum * mci->read_bl_len,
-		MMC_RSP_R1);
-
-	if (dst)
-		data.dest = dst;
-	else
-		data.src = src;
-
-	data.blocks = blocks;
-	data.blocksize = mci->read_bl_len;
-	data.flags = flags;
-
-	ret = mci_send_cmd(mci, &cmd, &data);
-
-	if (ret || blocks > 1) {
-		mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0,
-			      IS_SD(mci) ? MMC_RSP_R1b : MMC_RSP_R1);
-		mci_send_cmd(mci, &cmd, NULL);
-	}
-
-	return ret;
-}
-
 /**
  * Read one or several block(s) of data from the card
  * @param mci MCI instance
@@ -388,23 +381,36 @@ static int mci_do_block_op(struct mci *mci, const void *src, void *dst, int bloc
  * @param blocks number of blocks to read
  */
 static int mci_block_read(struct mci *mci, void *dst, int blocknum,
-		int blocks)
+			  int blocks)
 {
-	return mci_do_block_op(mci, NULL, dst, blocknum, blocks);
-}
+	struct mci_cmd cmd = {};
+	struct mci_data data;
+	int ret;
+	unsigned mmccmd;
 
-/**
- * Write one or several blocks of data to the card
- * @param mci_dev MCI instance
- * @param src Where to read from to write to the card
- * @param blocknum Block number to write
- * @param blocks Block count to write
- * @return Transaction status (0 on success)
- */
-static int mci_block_write(struct mci *mci, const void *src, int blocknum,
-	int blocks)
-{
-	return mci_do_block_op(mci, src, NULL, blocknum, blocks);
+	if (blocks > 1)
+		mmccmd = MMC_CMD_READ_MULTIPLE_BLOCK;
+	else
+		mmccmd = MMC_CMD_READ_SINGLE_BLOCK;
+
+	mci_setup_cmd(&cmd,
+		mmccmd,
+		mci->high_capacity != 0 ? blocknum : blocknum * mci->read_bl_len,
+		MMC_RSP_R1);
+
+	data.dest = dst;
+	data.blocks = blocks;
+	data.blocksize = mci->read_bl_len;
+	data.flags = MMC_DATA_READ;
+
+	ret = mci_send_cmd(mci, &cmd, &data);
+
+	if (ret || blocks > 1) {
+		mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0,
+			      IS_SD(mci) ? MMC_RSP_R1b : MMC_RSP_R1);
+		mci_send_cmd(mci, &cmd, NULL);
+	}
+	return ret;
 }
 
 /**
-- 
2.39.5




             reply	other threads:[~2025-04-11  5:29 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-11  5:28 Ahmad Fatoum [this message]
2025-04-11  7:06 ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250411052841.1745388-1-a.fatoum@pengutronix.de \
    --to=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox