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

[Jules Maselbas <jmaselbas@zdiv.net>]

On August 18, 2023 8:27:48 AM GMT+02:00, Sascha Hauer <sha@pengutronix.de> wrote:
> 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.
ack

> 
> > +
> > +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.
ack, not sure which error code is the most appropriated here, I am going to use EPROTO or EIO
> 
> > +		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().
ack

> 
> > +
> > +	/* 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()
ack

> 
> > +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?
IIRC sdhci_enable_v4_mode needs to be called before sdhci_read_caps which is called by sdhci_setup_host (called during probe),
and I think it also needs to be called after the controller is reset.

> 
> Sascha
>