Re: [PATCH v2 7/7] mci: Add dwcmshc-sdhci driver

[Sascha Hauer <sha@pengutronix.de>]

Hi Jules,

Some small things inline.


On Wed, Aug 16, 2023 at 11:39:45AM +0200, Jules Maselbas wrote:
> +#include "sdhci.h"
> +
> +#define CARD_STATUS_MASK (0x1e00)
> +#define CARD_STATUS_TRAN (4 << 9)
> +static int dwcmshc_mci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
> +				struct mci_data *data);
> +
> +struct dwcmshc_host {
> +	struct mci_host mci;
> +	struct sdhci sdhci;
> +	unsigned int in_abort_sequence;
> +};
> +
> +#define priv_from_mci_host(h) container_of(h, struct dwcmshc_host, mci)

Writing this as static inline function immediately makes the type of 'h'
clear.

> +
> +static void mci_setup_cmd(struct mci_cmd *p, unsigned int cmd, unsigned int arg,
> +			  unsigned int response)
> +{
> +	p->cmdidx = cmd;
> +	p->cmdarg = arg;
> +	p->resp_type = response;
> +}
> +
> +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;
> +	u64 start;
> +
> +	host->in_abort_sequence = 1;
> +
> +	mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
> +	ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> +	if (ret) {
> +		dev_err(host->mci.hw_dev, "Abort failed at first cmd12!\n");
> +		goto out;
> +	}
> +	mci_setup_cmd(&cmd, MMC_CMD_SEND_STATUS, mci->mci->rca << 16,
> +		      MMC_RSP_R1);
> +	ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> +	if (ret) {
> +		dev_err(host->mci.hw_dev, "Abort failed at first cmd13!\n");
> +		goto out;
> +	}
> +
> +	if ((cmd.response[0] & CARD_STATUS_MASK) == CARD_STATUS_TRAN)
> +		goto out; /* All is OK! */
> +
> +	mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
> +	ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> +	if (ret) {
> +		dev_err(host->mci.hw_dev, "Abort failed at second cmd12!\n");
> +		goto out;
> +	}
> +
> +	mci_setup_cmd(&cmd, MMC_CMD_SEND_STATUS, mci->mci->rca << 16,
> +		      MMC_RSP_R1);
> +	ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> +	if (ret) {
> +		dev_err(host->mci.hw_dev, "Abort failed at second cmd13!\n");
> +		goto out;
> +	}
> +
> +	if ((cmd.response[0] & CARD_STATUS_MASK) == CARD_STATUS_TRAN) {
> +		goto out; /* All is OK! */
> +	} else {
> +		dev_err(host->mci.hw_dev,
> +			"Abort sequence failed to put card in TRAN state!\n");
> +		ret = 1;

This should be an error code.

> +		goto out;
> +	}
> +
> +out:
> +	/* Perform SW reset if in abort sequence */
> +	sdhci_write8(&host->sdhci, SDHCI_SOFTWARE_RESET,
> +		       SDHCI_RESET_DATA | SDHCI_RESET_CMD);
> +	start = get_time_ns();
> +	while (sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET) != 0) {
> +		if (is_timeout(start, 50 * MSECOND)) {
> +			dev_err(host->mci.hw_dev,
> +				"SDHCI data reset timeout\n");
> +			break;
> +		}
> +	}
> +	host->in_abort_sequence = 0;
> +	return ret;
> +}
> +
> +static int dwcmshc_mci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
> +				struct mci_data *data)
> +{
> +	struct dwcmshc_host *host = priv_from_mci_host(mci);
> +	dma_addr_t dma = SDHCI_NO_DMA;
> +	u32 mask, command, xfer;
> +	int ret;
> +
> +	if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION
> +	    && host->in_abort_sequence == 0)
> +		return do_abort_sequence(mci, cmd);

You could factor the rest of this function out to some dwcmshc_mci_do_send_cmd()
and call it from here and do_abort_sequence().

> +
> +	/* Do not wait for CMD_INHIBIT_DAT on stop commands */
> +	mask = SDHCI_CMD_INHIBIT_CMD;
> +	if (cmd->cmdidx != MMC_CMD_STOP_TRANSMISSION)
> +		mask |= SDHCI_CMD_INHIBIT_DATA;
> +
> +	/* Wait for bus idle */
> +	ret = wait_on_timeout(50 * MSECOND,
> +			      !(sdhci_read16(&host->sdhci, SDHCI_PRESENT_STATE) & mask));
> +	if (ret) {
> +		dev_err(host->mci.hw_dev, "SDHCI timeout while waiting for idle\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (data)
> +		dev_dbg(host->mci.hw_dev, "cmd %d arg %d bcnt %d bsize %d\n",
> +			cmd->cmdidx, cmd->cmdarg, data->blocks, data->blocksize);
> +	else
> +		dev_dbg(host->mci.hw_dev, "cmd %d arg %d\n", cmd->cmdidx, cmd->cmdarg);
> +
> +	sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> +
> +	sdhci_setup_data_dma(&host->sdhci, data, &dma);
> +
> +	sdhci_write8(&host->sdhci, SDHCI_TIMEOUT_CONTROL, 0xe);
> +
> +	sdhci_set_cmd_xfer_mode(&host->sdhci, cmd, data,
> +				dma == SDHCI_NO_DMA ? false : true,
> +				&command, &xfer);
> +
> +	sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
> +
> +	sdhci_write32(&host->sdhci, SDHCI_ARGUMENT, cmd->cmdarg);
> +	sdhci_write16(&host->sdhci, SDHCI_COMMAND, command);
> +
> +	ret = sdhci_wait_for_done(&host->sdhci, SDHCI_INT_CMD_COMPLETE);
> +	if (ret)
> +		goto error;
> +
> +	sdhci_read_response(&host->sdhci, cmd);
> +
> +	ret = sdhci_transfer_data(&host->sdhci, data, dma);
> +error:
> +	if (ret) {
> +		sdhci_reset(&host->sdhci, SDHCI_RESET_CMD);
> +		sdhci_reset(&host->sdhci, SDHCI_RESET_DATA);
> +	}
> +
> +	sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> +	return ret;
> +}
> +
> +static void dwcmshc_mci_set_ios(struct mci_host *mci, struct mci_ios *ios)
> +{
> +	struct dwcmshc_host *host = priv_from_mci_host(mci);
> +	u16 val;
> +
> +	debug("%s: clock = %u, bus-width = %d, timing = %02x\n", __func__,
> +	      ios->clock, ios->bus_width, ios->timing);

dev_dbg()

> +static int dwcmshc_mci_init(struct mci_host *mci, struct device *dev)
> +{
> +	struct dwcmshc_host *host = priv_from_mci_host(mci);
> +	u16 ctrl2;
> +
> +	/* reset mshci controller */
> +	sdhci_write8(&host->sdhci, SDHCI_SOFTWARE_RESET, SDHCI_RESET_ALL);
> +
> +	/* wait for reset completion */
> +	if (wait_on_timeout(100 * MSECOND,
> +			    (sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET)
> +			     & SDHCI_RESET_ALL) == 0)) {
> +		dev_err(dev, "SDHCI reset timeout\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	sdhci_write16(&host->sdhci, SDHCI_INT_ERROR_ENABLE, ~0);
> +	sdhci_write16(&host->sdhci, SDHCI_INT_ENABLE, ~0);
> +	sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> +	sdhci_write32(&host->sdhci, SDHCI_SIGNAL_ENABLE, ~0);
> +
> +	sdhci_enable_v4_mode(&host->sdhci);
> +
> +	dev_dbg(host->mci.hw_dev, "host version4: %s\n",
> +		ctrl2 & SDHCI_CTRL_V4_MODE ? "enabled" : "disabled");
> +
> +	return 0;
> +}
> +

[...]

> +static int dwcmshc_probe(struct device *dev)
> +{
> +	struct dwcmshc_host *host;
> +	struct resource *iores;
> +	struct mci_host *mci;
> +	struct clk *clk;
> +	int ret;
> +
> +	host = xzalloc(sizeof(*host));
> +	mci = &host->mci;
> +
> +	iores = dev_request_mem_resource(dev, 0);
> +	if (IS_ERR(iores)) {
> +		ret = PTR_ERR(iores);
> +		goto err_mem_req;
> +	}
> +
> +	clk = clk_get(dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		goto err_clk_get;
> +	}
> +	clk_enable(clk);
> +
> +	host->sdhci.base = IOMEM(iores->start);
> +	host->sdhci.mci = mci;
> +	host->sdhci.max_clk = clk_get_rate(clk);
> +
> +	mci->hw_dev = dev;
> +	mci->init = dwcmshc_mci_init;
> +	mci->set_ios = dwcmshc_mci_set_ios;
> +	mci->send_cmd = dwcmshc_mci_send_cmd;
> +	mci->card_present = dwcmshc_mci_card_present;
> +
> +	mci_of_parse(&host->mci);
> +
> +	/* Enable host_version4 */
> +	sdhci_enable_v4_mode(&host->sdhci);

This is also called from dwcmshc_mci_init(). Do we need both?

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 |