[PATCH 0/4] Add a struct mci_cmd::busy_timeout member

[PATCH 0/4] Add a struct mci_cmd::busy_timeout member

[Sebastien Bourdelin]

From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

This patch series introduce the busy_timeout member as part of the
struct mci_cmd and fix the SDHCI timeout value for the BCM2711 SoC.

It comes as a first step to [1] and allow sdhci drivers to define their
timeout value.

[1] https://github.com/barebox/barebox/blob/master/drivers/mci/mci-core.c#L2109

Sebastien Bourdelin (4):
  mci: add a busy_timeout member to the struct mci_cmd
  mci: zeroed all structs mci_cmd instances
  mci: sdhci: use the busy_timeout value in the sdhci_wait_idle
    functions
  mci: bcm2835: set timeout value to 100ms

 arch/arm/mach-socfpga/arria10-xload-emmc.c |  2 +-
 drivers/mci/dwcmshc-sdhci.c                |  2 +-
 drivers/mci/imx-esdhc-pbl.c                |  4 +--
 drivers/mci/mci-bcm2835.c                  |  4 +++
 drivers/mci/mci-core.c                     | 34 +++++++++++-----------
 drivers/mci/sdhci.c                        | 14 +++++++--
 drivers/mci/sdhci.h                        |  2 ++
 drivers/mci/stm32_sdmmc2.c                 |  2 +-
 include/mci.h                              |  1 +
 9 files changed, 41 insertions(+), 24 deletions(-)

-- 
2.46.2

[PATCH 1/4] mci: add a busy_timeout member to the struct mci_cmd

[Sebastien Bourdelin]

From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

This commit extends the struct mci_cmd to add a new busy_timeout member.

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
---
 include/mci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/mci.h b/include/mci.h
index cd01e2f992..f607ef1b74 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -471,6 +471,7 @@ struct mci_cmd {
 	unsigned cmdidx;	/**< Command to be sent to the SD/MMC card */
 	unsigned resp_type;	/**< Type of expected response, refer MMC_RSP_* macros */
 	unsigned cmdarg;	/**< Command's arguments */
+	unsigned busy_timeout;	/**< Busy timeout in ms */
 	unsigned response[4];	/**< card's response */
 };
 
-- 
2.46.2

[PATCH 2/4] mci: zeroed all structs mci_cmd instances

[Sebastien Bourdelin]

From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

All structs mci_cmd should be init to zero to avoid error while using
the busy_timeout value.

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
---
 arch/arm/mach-socfpga/arria10-xload-emmc.c |  2 +-
 drivers/mci/dwcmshc-sdhci.c                |  2 +-
 drivers/mci/imx-esdhc-pbl.c                |  4 +--
 drivers/mci/mci-core.c                     | 34 +++++++++++-----------
 drivers/mci/stm32_sdmmc2.c                 |  2 +-
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-socfpga/arria10-xload-emmc.c b/arch/arm/mach-socfpga/arria10-xload-emmc.c
index ed24faf9bf..61774c6174 100644
--- a/arch/arm/mach-socfpga/arria10-xload-emmc.c
+++ b/arch/arm/mach-socfpga/arria10-xload-emmc.c
@@ -178,7 +178,7 @@ static int dwmci_cmd(struct mci_cmd *cmd, struct mci_data *data)
 
 int arria10_read_blocks(void *dst, int blocknum, size_t len)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	int ret;
 	int blocks;
diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
index 010d376421..a7306fdb44 100644
--- a/drivers/mci/dwcmshc-sdhci.c
+++ b/drivers/mci/dwcmshc-sdhci.c
@@ -52,7 +52,7 @@ static int do_abort_sequence(struct mci_host *mci, struct mci_cmd *current_cmd)
 {
 	int ret = 0;
 	struct dwcmshc_host *host = priv_from_mci_host(mci);
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	u64 start;
 
 	mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
diff --git a/drivers/mci/imx-esdhc-pbl.c b/drivers/mci/imx-esdhc-pbl.c
index 5b1d9a3cf4..0e4f96d2de 100644
--- a/drivers/mci/imx-esdhc-pbl.c
+++ b/drivers/mci/imx-esdhc-pbl.c
@@ -35,7 +35,7 @@ static u8 ext_csd[512] __aligned(64);
 
 static int esdhc_send_ext_csd(struct fsl_esdhc_host *host)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 
 	cmd.cmdidx = MMC_CMD_SEND_EXT_CSD;
@@ -67,7 +67,7 @@ static bool __maybe_unused esdhc_bootpart_active(struct fsl_esdhc_host *host)
 
 static int esdhc_read_blocks(struct fsl_esdhc_host *host, void *dst, size_t len)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	u32 val;
 	int ret;
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 48a3df9ec9..a6c304c1bd 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -119,7 +119,7 @@ static void mci_setup_cmd(struct mci_cmd *p, unsigned cmd, unsigned arg, unsigne
  */
 static int mci_set_dsr(struct mci *mci)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 
 	mci_setup_cmd(&cmd, MMC_CMD_SET_DSR,
 			(mci->host->dsr_val >> 16) | 0xffff, MMC_RSP_NONE);
@@ -134,7 +134,7 @@ static int mci_set_dsr(struct mci *mci)
  */
 static int mci_set_blocklen(struct mci *mci, unsigned len)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 
 	if (mci->host->timing == MMC_TIMING_MMC_DDR52)
 		return 0;
@@ -148,7 +148,7 @@ static void *sector_buf;
 static int mci_send_status(struct mci *mci, unsigned int *status)
 {
 	struct mci_host *host = mci->host;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int ret;
 
 	/*
@@ -172,7 +172,7 @@ static int mci_send_status(struct mci *mci, unsigned int *status)
 static int mci_app_sd_status(struct mci *mci, __be32 *ssr)
 {
 	int err;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 
 	cmd.cmdidx = MMC_CMD_APP_CMD;
@@ -272,7 +272,7 @@ static int mci_poll_until_ready(struct mci *mci, int timeout_ms)
 static int mci_block_write(struct mci *mci, const void *src, int blocknum,
 	int blocks)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	unsigned mmccmd;
 	int ret;
@@ -374,7 +374,7 @@ static int mci_block_erase(struct mci *card, unsigned int from,
 static int mci_read_block(struct mci *mci, void *dst, int blocknum,
 		int blocks)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	int ret;
 	unsigned mmccmd;
@@ -411,7 +411,7 @@ static int mci_read_block(struct mci *mci, void *dst, int blocknum,
  */
 static int mci_go_idle(struct mci *mci)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int err;
 
 	udelay(1000);
@@ -431,7 +431,7 @@ static int mci_go_idle(struct mci *mci)
 
 static int sdio_send_op_cond(struct mci *mci)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 
 	mci_setup_cmd(&cmd, SD_IO_SEND_OP_COND, 0, MMC_RSP_SPI_R4 | MMC_RSP_R4 | MMC_CMD_BCR);
 
@@ -446,7 +446,7 @@ static int sdio_send_op_cond(struct mci *mci)
 static int sd_send_op_cond(struct mci *mci)
 {
 	struct mci_host *host = mci->host;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int timeout = 1000;
 	int err;
 	unsigned voltages;
@@ -521,7 +521,7 @@ static int sd_send_op_cond(struct mci *mci)
 static int mmc_send_op_cond(struct mci *mci)
 {
 	struct mci_host *host = mci->host;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int timeout = 1000;
 	int err;
 
@@ -566,7 +566,7 @@ static int mmc_send_op_cond(struct mci *mci)
  */
 int mci_send_ext_csd(struct mci *mci, char *ext_csd)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 
 	/* Get the Card Status Register */
@@ -595,7 +595,7 @@ int mci_send_ext_csd(struct mci *mci, char *ext_csd)
 int mci_switch(struct mci *mci, unsigned index, unsigned value)
 {
 	unsigned int status;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int ret;
 
 	mci_setup_cmd(&cmd, MMC_CMD_SWITCH,
@@ -840,7 +840,7 @@ static int mmc_change_freq(struct mci *mci)
 static int sd_switch(struct mci *mci, unsigned mode, unsigned group,
 			unsigned value, uint8_t *resp)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	unsigned arg;
 
@@ -909,7 +909,7 @@ static int sd_read_ssr(struct mci *mci)
  */
 static int sd_change_freq(struct mci *mci)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	struct mci_data data;
 	struct mci_host *host = mci->host;
 	uint32_t *switch_status = sector_buf;
@@ -1387,7 +1387,7 @@ static char *mci_version_string(struct mci *mci)
 
 static int mci_startup_sd(struct mci *mci)
 {
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int err;
 
 	if (mci_caps(mci) & MMC_CAP_4_BIT_DATA) {
@@ -1782,7 +1782,7 @@ static int mci_startup_mmc(struct mci *mci)
 static int mci_startup(struct mci *mci)
 {
 	struct mci_host *host = mci->host;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int err;
 
 	if (IS_ENABLED(CONFIG_MMC_SPI_CRC_ON) && mmc_host_is_spi(host)) { /* enable CRC check for spi */
@@ -1924,7 +1924,7 @@ static int mci_startup(struct mci *mci)
 static int sd_send_if_cond(struct mci *mci)
 {
 	struct mci_host *host = mci->host;
-	struct mci_cmd cmd;
+	struct mci_cmd cmd = {};
 	int err;
 
 	mci_setup_cmd(&cmd, SD_CMD_SEND_IF_COND,
diff --git a/drivers/mci/stm32_sdmmc2.c b/drivers/mci/stm32_sdmmc2.c
index 822416c457..64a7171d2c 100644
--- a/drivers/mci/stm32_sdmmc2.c
+++ b/drivers/mci/stm32_sdmmc2.c
@@ -512,7 +512,7 @@ static int stm32_sdmmc2_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
 	 * shall be send on cmd or data errors.
 	 */
 	if (ret && cmd->cmdidx != MMC_CMD_STOP_TRANSMISSION) {
-		struct mci_cmd stop_cmd;
+		struct mci_cmd stop_cmd = {};
 
 		stop_cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
 		stop_cmd.cmdarg = 0;
-- 
2.46.2

[PATCH 3/4] mci: sdhci: use the busy_timeout value in the sdhci_wait_idle functions

[Sebastien Bourdelin]

From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

The busy_timeout value is used by the sdhci_wait_idle and
sdhci_wait_idle_data functions to define the timeout to wait on when
send a command to the controller.

The default value remains unchanged and is set to 10ms.

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
---
 drivers/mci/sdhci.c | 14 ++++++++++++--
 drivers/mci/sdhci.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index fe033583ad..1425c72ef5 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -796,9 +796,11 @@ void sdhci_enable_clk(struct sdhci *host, u16 clk)
 int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data)
 {
 	u32 mask;
+	unsigned timeout_ms;
 	int ret;
 
 	mask = SDHCI_CMD_INHIBIT_CMD;
+	timeout_ms = SDHCI_CMD_DEFAULT_BUSY_TIMEOUT;
 
 	if (data || (cmd && (cmd->resp_type & MMC_RSP_BUSY)))
 		mask |= SDHCI_CMD_INHIBIT_DATA;
@@ -806,7 +808,10 @@ int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *da
 	if (cmd && cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
 		mask &= ~SDHCI_CMD_INHIBIT_DATA;
 
-	ret = wait_on_timeout(10 * MSECOND,
+	if (cmd && cmd->busy_timeout != 0)
+		timeout_ms = cmd->busy_timeout;
+
+	ret = wait_on_timeout(timeout_ms * MSECOND,
 			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
 
 	if (ret) {
@@ -821,14 +826,19 @@ int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *da
 int sdhci_wait_idle_data(struct sdhci *host, struct mci_cmd *cmd)
 {
 	u32 mask;
+	unsigned timeout_ms;
 	int ret;
 
 	mask = SDHCI_CMD_INHIBIT_CMD | SDHCI_CMD_INHIBIT_DATA;
+	timeout_ms = SDHCI_CMD_DEFAULT_BUSY_TIMEOUT;
 
 	if (cmd && cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
 		mask &= ~SDHCI_CMD_INHIBIT_DATA;
 
-	ret = wait_on_timeout(10 * MSECOND,
+	if (cmd && cmd->busy_timeout != 0)
+		timeout_ms = cmd->busy_timeout;
+
+	ret = wait_on_timeout(timeout_ms * MSECOND,
 			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
 
 	if (ret) {
diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
index 5de85239b1..757840acc7 100644
--- a/drivers/mci/sdhci.h
+++ b/drivers/mci/sdhci.h
@@ -200,6 +200,8 @@
 #define SDHCI_MAX_DIV_SPEC_200	256
 #define SDHCI_MAX_DIV_SPEC_300	2046
 
+#define SDHCI_CMD_DEFAULT_BUSY_TIMEOUT 10
+
 struct sdhci {
 	u32 (*read32)(struct sdhci *host, int reg);
 	u16 (*read16)(struct sdhci *host, int reg);
-- 
2.46.2

[PATCH 4/4] mci: bcm2835: set timeout value to 100ms

[Sebastien Bourdelin]

From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

As mentionned in the Raspberry Pi4 bootrom Changelog [1]:
"Increase timeout of early SD/EMMC commands to 100ms".
The BCM2711 SDHCI can take up to 100ms to complete a command.

Without this change, we had noticed that Barebox sometime stay stucked
on a timeout error while trying to boot Linux from the eMMC.

[1] https://github.com/raspberrypi/rpi-eeprom/blob/master/firmware-2711/release-notes.md#2022-03-10---promote-the-2022-03-10-beta-release-to-lateststable

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
---
 drivers/mci/mci-bcm2835.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mci/mci-bcm2835.c b/drivers/mci/mci-bcm2835.c
index cbf99ee7ca..3b086ed22c 100644
--- a/drivers/mci/mci-bcm2835.c
+++ b/drivers/mci/mci-bcm2835.c
@@ -128,6 +128,10 @@ static int bcm2835_mci_request(struct mci_host *mci, struct mci_cmd *cmd,
 		block_data |= data->blocksize;
 	}
 
+	/* BCM2xxx SDHCI might take up to 100ms to complete a command */
+	if (cmd != NULL)
+		cmd->busy_timeout = 100;
+
 	ret = sdhci_wait_idle_data(&host->sdhci, cmd);
 	if (ret)
 		return ret;
-- 
2.46.2

Re: [PATCH 4/4] mci: bcm2835: set timeout value to 100ms

[Sascha Hauer]

On Thu, Oct 31, 2024 at 10:27:21AM +0100, Sebastien Bourdelin wrote:
> From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> 
> As mentionned in the Raspberry Pi4 bootrom Changelog [1]:
> "Increase timeout of early SD/EMMC commands to 100ms".
> The BCM2711 SDHCI can take up to 100ms to complete a command.
> 
> Without this change, we had noticed that Barebox sometime stay stucked
> on a timeout error while trying to boot Linux from the eMMC.
> 
> [1] https://github.com/raspberrypi/rpi-eeprom/blob/master/firmware-2711/release-notes.md#2022-03-10---promote-the-2022-03-10-beta-release-to-lateststable
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> ---
>  drivers/mci/mci-bcm2835.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mci/mci-bcm2835.c b/drivers/mci/mci-bcm2835.c
> index cbf99ee7ca..3b086ed22c 100644
> --- a/drivers/mci/mci-bcm2835.c
> +++ b/drivers/mci/mci-bcm2835.c
> @@ -128,6 +128,10 @@ static int bcm2835_mci_request(struct mci_host *mci, struct mci_cmd *cmd,
>  		block_data |= data->blocksize;
>  	}
>  
> +	/* BCM2xxx SDHCI might take up to 100ms to complete a command */
> +	if (cmd != NULL)
> +		cmd->busy_timeout = 100;

Do we need this for all commands or just some specific ones?

cmd is always non NULL, no need to check.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mci: bcm2835: set timeout value to 100ms

[Sebastien Bourdelin]

On Mon Nov 4, 2024 at 1:27 PM CET, Sascha Hauer wrote:
> On Thu, Oct 31, 2024 at 10:27:21AM +0100, Sebastien Bourdelin wrote:
> > From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> > 
> > As mentionned in the Raspberry Pi4 bootrom Changelog [1]:
> > "Increase timeout of early SD/EMMC commands to 100ms".
> > The BCM2711 SDHCI can take up to 100ms to complete a command.
> > 
> > Without this change, we had noticed that Barebox sometime stay stucked
> > on a timeout error while trying to boot Linux from the eMMC.
> > 
> > [1] https://github.com/raspberrypi/rpi-eeprom/blob/master/firmware-2711/release-notes.md#2022-03-10---promote-the-2022-03-10-beta-release-to-lateststable
> > 
> > Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> > ---
> >  drivers/mci/mci-bcm2835.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/mci/mci-bcm2835.c b/drivers/mci/mci-bcm2835.c
> > index cbf99ee7ca..3b086ed22c 100644
> > --- a/drivers/mci/mci-bcm2835.c
> > +++ b/drivers/mci/mci-bcm2835.c
> > @@ -128,6 +128,10 @@ static int bcm2835_mci_request(struct mci_host *mci, struct mci_cmd *cmd,
> >  		block_data |= data->blocksize;
> >  	}
> >  
> > +	/* BCM2xxx SDHCI might take up to 100ms to complete a command */
> > +	if (cmd != NULL)
> > +		cmd->busy_timeout = 100;
>
> Do we need this for all commands or just some specific ones?
I actually have no idea, the CM4s datasheet doesn't give much
informations regarding the eMMC controller and we just found that
information in the rpi-eeprom changelog, and saw it was also the defaut
timeout value used in U-boot [1].
After this change, we haven't been able to reproduce the issue.

We know for sure this issue happens at least on a read
command.

[1] https://github.com/u-boot/u-boot/blob/master/drivers/mmc/sdhci.c#L195
>
> cmd is always non NULL, no need to check.
Ok, i can send a v2 for that, just let me know if my explanation for
your first question is enough. 
>
> Sascha

Re: [PATCH 1/4] mci: add a busy_timeout member to the struct mci_cmd

[Ahmad Fatoum]

On 31.10.24 10:27, Sebastien Bourdelin wrote:
> From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> 
> This commit extends the struct mci_cmd to add a new busy_timeout member.
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  include/mci.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/mci.h b/include/mci.h
> index cd01e2f992..f607ef1b74 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -471,6 +471,7 @@ struct mci_cmd {
>  	unsigned cmdidx;	/**< Command to be sent to the SD/MMC card */
>  	unsigned resp_type;	/**< Type of expected response, refer MMC_RSP_* macros */
>  	unsigned cmdarg;	/**< Command's arguments */
> +	unsigned busy_timeout;	/**< Busy timeout in ms */
>  	unsigned response[4];	/**< card's response */
>  };
>  


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] mci: zeroed all structs mci_cmd instances

[Ahmad Fatoum]

On 31.10.24 10:27, Sebastien Bourdelin wrote:
> From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> 
> All structs mci_cmd should be init to zero to avoid error while using
> the busy_timeout value.

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> ---
>  arch/arm/mach-socfpga/arria10-xload-emmc.c |  2 +-
>  drivers/mci/dwcmshc-sdhci.c                |  2 +-
>  drivers/mci/imx-esdhc-pbl.c                |  4 +--
>  drivers/mci/mci-core.c                     | 34 +++++++++++-----------
>  drivers/mci/stm32_sdmmc2.c                 |  2 +-
>  5 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/arria10-xload-emmc.c b/arch/arm/mach-socfpga/arria10-xload-emmc.c
> index ed24faf9bf..61774c6174 100644
> --- a/arch/arm/mach-socfpga/arria10-xload-emmc.c
> +++ b/arch/arm/mach-socfpga/arria10-xload-emmc.c
> @@ -178,7 +178,7 @@ static int dwmci_cmd(struct mci_cmd *cmd, struct mci_data *data)
>  
>  int arria10_read_blocks(void *dst, int blocknum, size_t len)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	int ret;
>  	int blocks;
> diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
> index 010d376421..a7306fdb44 100644
> --- a/drivers/mci/dwcmshc-sdhci.c
> +++ b/drivers/mci/dwcmshc-sdhci.c
> @@ -52,7 +52,7 @@ static int do_abort_sequence(struct mci_host *mci, struct mci_cmd *current_cmd)
>  {
>  	int ret = 0;
>  	struct dwcmshc_host *host = priv_from_mci_host(mci);
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	u64 start;
>  
>  	mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
> diff --git a/drivers/mci/imx-esdhc-pbl.c b/drivers/mci/imx-esdhc-pbl.c
> index 5b1d9a3cf4..0e4f96d2de 100644
> --- a/drivers/mci/imx-esdhc-pbl.c
> +++ b/drivers/mci/imx-esdhc-pbl.c
> @@ -35,7 +35,7 @@ static u8 ext_csd[512] __aligned(64);
>  
>  static int esdhc_send_ext_csd(struct fsl_esdhc_host *host)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  
>  	cmd.cmdidx = MMC_CMD_SEND_EXT_CSD;
> @@ -67,7 +67,7 @@ static bool __maybe_unused esdhc_bootpart_active(struct fsl_esdhc_host *host)
>  
>  static int esdhc_read_blocks(struct fsl_esdhc_host *host, void *dst, size_t len)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	u32 val;
>  	int ret;
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 48a3df9ec9..a6c304c1bd 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -119,7 +119,7 @@ static void mci_setup_cmd(struct mci_cmd *p, unsigned cmd, unsigned arg, unsigne
>   */
>  static int mci_set_dsr(struct mci *mci)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  
>  	mci_setup_cmd(&cmd, MMC_CMD_SET_DSR,
>  			(mci->host->dsr_val >> 16) | 0xffff, MMC_RSP_NONE);
> @@ -134,7 +134,7 @@ static int mci_set_dsr(struct mci *mci)
>   */
>  static int mci_set_blocklen(struct mci *mci, unsigned len)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  
>  	if (mci->host->timing == MMC_TIMING_MMC_DDR52)
>  		return 0;
> @@ -148,7 +148,7 @@ static void *sector_buf;
>  static int mci_send_status(struct mci *mci, unsigned int *status)
>  {
>  	struct mci_host *host = mci->host;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int ret;
>  
>  	/*
> @@ -172,7 +172,7 @@ static int mci_send_status(struct mci *mci, unsigned int *status)
>  static int mci_app_sd_status(struct mci *mci, __be32 *ssr)
>  {
>  	int err;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  
>  	cmd.cmdidx = MMC_CMD_APP_CMD;
> @@ -272,7 +272,7 @@ static int mci_poll_until_ready(struct mci *mci, int timeout_ms)
>  static int mci_block_write(struct mci *mci, const void *src, int blocknum,
>  	int blocks)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	unsigned mmccmd;
>  	int ret;
> @@ -374,7 +374,7 @@ static int mci_block_erase(struct mci *card, unsigned int from,
>  static int mci_read_block(struct mci *mci, void *dst, int blocknum,
>  		int blocks)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	int ret;
>  	unsigned mmccmd;
> @@ -411,7 +411,7 @@ static int mci_read_block(struct mci *mci, void *dst, int blocknum,
>   */
>  static int mci_go_idle(struct mci *mci)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int err;
>  
>  	udelay(1000);
> @@ -431,7 +431,7 @@ static int mci_go_idle(struct mci *mci)
>  
>  static int sdio_send_op_cond(struct mci *mci)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  
>  	mci_setup_cmd(&cmd, SD_IO_SEND_OP_COND, 0, MMC_RSP_SPI_R4 | MMC_RSP_R4 | MMC_CMD_BCR);
>  
> @@ -446,7 +446,7 @@ static int sdio_send_op_cond(struct mci *mci)
>  static int sd_send_op_cond(struct mci *mci)
>  {
>  	struct mci_host *host = mci->host;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int timeout = 1000;
>  	int err;
>  	unsigned voltages;
> @@ -521,7 +521,7 @@ static int sd_send_op_cond(struct mci *mci)
>  static int mmc_send_op_cond(struct mci *mci)
>  {
>  	struct mci_host *host = mci->host;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int timeout = 1000;
>  	int err;
>  
> @@ -566,7 +566,7 @@ static int mmc_send_op_cond(struct mci *mci)
>   */
>  int mci_send_ext_csd(struct mci *mci, char *ext_csd)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  
>  	/* Get the Card Status Register */
> @@ -595,7 +595,7 @@ int mci_send_ext_csd(struct mci *mci, char *ext_csd)
>  int mci_switch(struct mci *mci, unsigned index, unsigned value)
>  {
>  	unsigned int status;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int ret;
>  
>  	mci_setup_cmd(&cmd, MMC_CMD_SWITCH,
> @@ -840,7 +840,7 @@ static int mmc_change_freq(struct mci *mci)
>  static int sd_switch(struct mci *mci, unsigned mode, unsigned group,
>  			unsigned value, uint8_t *resp)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	unsigned arg;
>  
> @@ -909,7 +909,7 @@ static int sd_read_ssr(struct mci *mci)
>   */
>  static int sd_change_freq(struct mci *mci)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	struct mci_data data;
>  	struct mci_host *host = mci->host;
>  	uint32_t *switch_status = sector_buf;
> @@ -1387,7 +1387,7 @@ static char *mci_version_string(struct mci *mci)
>  
>  static int mci_startup_sd(struct mci *mci)
>  {
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int err;
>  
>  	if (mci_caps(mci) & MMC_CAP_4_BIT_DATA) {
> @@ -1782,7 +1782,7 @@ static int mci_startup_mmc(struct mci *mci)
>  static int mci_startup(struct mci *mci)
>  {
>  	struct mci_host *host = mci->host;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int err;
>  
>  	if (IS_ENABLED(CONFIG_MMC_SPI_CRC_ON) && mmc_host_is_spi(host)) { /* enable CRC check for spi */
> @@ -1924,7 +1924,7 @@ static int mci_startup(struct mci *mci)
>  static int sd_send_if_cond(struct mci *mci)
>  {
>  	struct mci_host *host = mci->host;
> -	struct mci_cmd cmd;
> +	struct mci_cmd cmd = {};
>  	int err;
>  
>  	mci_setup_cmd(&cmd, SD_CMD_SEND_IF_COND,
> diff --git a/drivers/mci/stm32_sdmmc2.c b/drivers/mci/stm32_sdmmc2.c
> index 822416c457..64a7171d2c 100644
> --- a/drivers/mci/stm32_sdmmc2.c
> +++ b/drivers/mci/stm32_sdmmc2.c
> @@ -512,7 +512,7 @@ static int stm32_sdmmc2_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
>  	 * shall be send on cmd or data errors.
>  	 */
>  	if (ret && cmd->cmdidx != MMC_CMD_STOP_TRANSMISSION) {
> -		struct mci_cmd stop_cmd;
> +		struct mci_cmd stop_cmd = {};
>  
>  		stop_cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
>  		stop_cmd.cmdarg = 0;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] mci: sdhci: use the busy_timeout value in the sdhci_wait_idle functions

[Ahmad Fatoum]

On 31.10.24 10:27, Sebastien Bourdelin wrote:
> From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> 
> The busy_timeout value is used by the sdhci_wait_idle and
> sdhci_wait_idle_data functions to define the timeout to wait on when
> send a command to the controller.
> 
> The default value remains unchanged and is set to 10ms.
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

With below point addressed:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
> index 5de85239b1..757840acc7 100644
> --- a/drivers/mci/sdhci.h
> +++ b/drivers/mci/sdhci.h
> @@ -200,6 +200,8 @@
>  #define SDHCI_MAX_DIV_SPEC_200	256
>  #define SDHCI_MAX_DIV_SPEC_300	2046
>  
> +#define SDHCI_CMD_DEFAULT_BUSY_TIMEOUT 10

Please rename to SDHCI_CMD_DEFAULT_BUSY_TIMEOUT_MS

> +
>  struct sdhci {
>  	u32 (*read32)(struct sdhci *host, int reg);
>  	u16 (*read16)(struct sdhci *host, int reg);


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mci: bcm2835: set timeout value to 100ms

[Ahmad Fatoum]

Hello Sebastien,

Thanks for your fix.

On 31.10.24 10:27, Sebastien Bourdelin wrote:
> From: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>
> 
> As mentionned in the Raspberry Pi4 bootrom Changelog [1]:
> "Increase timeout of early SD/EMMC commands to 100ms".
> The BCM2711 SDHCI can take up to 100ms to complete a command.
> 
> Without this change, we had noticed that Barebox sometime stay stucked
> on a timeout error while trying to boot Linux from the eMMC.
> 
> [1] https://github.com/raspberrypi/rpi-eeprom/blob/master/firmware-2711/release-notes.md#2022-03-10---promote-the-2022-03-10-beta-release-to-lateststable
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@rtone.fr>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Cheers,
Ahmad

> ---
>  drivers/mci/mci-bcm2835.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mci/mci-bcm2835.c b/drivers/mci/mci-bcm2835.c
> index cbf99ee7ca..3b086ed22c 100644
> --- a/drivers/mci/mci-bcm2835.c
> +++ b/drivers/mci/mci-bcm2835.c
> @@ -128,6 +128,10 @@ static int bcm2835_mci_request(struct mci_host *mci, struct mci_cmd *cmd,
>  		block_data |= data->blocksize;
>  	}
>  
> +	/* BCM2xxx SDHCI might take up to 100ms to complete a command */
> +	if (cmd != NULL)
> +		cmd->busy_timeout = 100;
> +
>  	ret = sdhci_wait_idle_data(&host->sdhci, cmd);
>  	if (ret)
>  		return ret;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |