Re: [PATCH 3/5] mci: sdhci: add sdhci_send_cmd

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Dec 15, 2025 at 03:21:25PM +0100, Steffen Trumtrar wrote:
> Linux uses a generic sdhci_send_cmd function for most sdhci host
> drivers. Port the v6.18-rc1 version and mix and match with the
> arasan_sdhci_send_cmd function already in barebox.
> 
> Also add sdhci_dumpregs for debugging errors.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/mci/sdhci.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mci/sdhci.h |  21 +++++++++
>  include/mci.h       |   1 +
>  3 files changed, 152 insertions(+)
> 
> diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
> index 2d32a8b311..cfdfdbba8d 100644
> --- a/drivers/mci/sdhci.c
> +++ b/drivers/mci/sdhci.c
> @@ -11,6 +11,8 @@
>  
>  #define MAX_TUNING_LOOP 40
>  
> +#define DRIVER_NAME "sdhci"
> +
>  enum sdhci_reset_reason {
>  	SDHCI_RESET_FOR_INIT,
>  	SDHCI_RESET_FOR_REQUEST_ERROR,
> @@ -25,6 +27,71 @@ static inline struct device *sdhci_dev(struct sdhci *host)
>  	return host->mci ? host->mci->hw_dev : NULL;
>  }
>  
> +#define SDHCI_DUMP(f, x...) pr_err("%s: " DRIVER_NAME ": " f, host->mci->devname, ## x)

A plain dev_err would be in line with other messages.

>  	if (host->quirks2 & SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
> @@ -51,6 +118,69 @@ static void sdhci_reset_for_reason(struct sdhci *host, enum sdhci_reset_reason r
>  
>  #define sdhci_reset_for(h, r) sdhci_reset_for_reason((h), SDHCI_RESET_FOR_##r)
>  
> +bool sdhci_send_command(struct sdhci *host, struct mci_cmd *cmd)

The Linux function returns bool because the return value only signals if
the command was sent or not. The command error is propagated in struct
mmc_command. We don't have this asynchronous approach in barebox and we
also do not have an error code in struct mci_cmd. So either you have to
return the error code directly from this function or add an error code
to struct mci_cmd.

>  #define sdhci_read8_poll_timeout(sdhci, reg, val, cond, timeout_us) \
> diff --git a/include/mci.h b/include/mci.h
> index 3db33f9142..e54f6eba8d 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -489,6 +489,7 @@ struct mci_cmd {
>  	unsigned cmdarg;	/**< Command's arguments */
>  	unsigned busy_timeout;	/**< Busy timeout in ms */
>  	unsigned response[4];	/**< card's response */
> +	struct mci_data		*data;		/* data segment associated with cmd */

We can add the data to struct mci_cmd, but

- the core should set this up, not some individual driver
- when the data is in the command, the then redundant data argument to
  the various send_cmd functions should be removed
- this should be a separate patch

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 |