mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Jules Maselbas <jmaselbas@zdiv.net>
To: barebox@lists.infradead.org, Sascha Hauer <sha@pengutronix.de>,
	Jules Maselbas <jmaselbas@kalray.eu>
Subject: Re: [PATCH v2 7/7] mci: Add dwcmshc-sdhci driver
Date: Fri, 18 Aug 2023 13:37:27 +0200	[thread overview]
Message-ID: <FC4EFE3F-1EA7-4977-B63D-AEE785F68878@zdiv.net> (raw)
In-Reply-To: <20230818062748.GM5650@pengutronix.de>



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
> 



      reply	other threads:[~2023-08-18 11:39 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-16  9:39 [PATCH v2 1/7] mci: sdhci: Set 8-bit host caps Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 2/7] mci: sdhci: Add registers defines Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 3/7] mci: sdhci: Actually return the error code instead of 0 Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 4/7] mci: sdhci: Add sd host v4 mode Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 5/7] mci: sdhci: Add 64-bit DMA addressing suport for V4 mode Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 6/7] mci: sdhci: Force DMA update to the next block boundary Jules Maselbas
2023-08-16  9:39 ` [PATCH v2 7/7] mci: Add dwcmshc-sdhci driver Jules Maselbas
2023-08-18  6:27   ` Sascha Hauer
2023-08-18 11:37     ` Jules Maselbas [this message]

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=FC4EFE3F-1EA7-4977-B63D-AEE785F68878@zdiv.net \
    --to=jmaselbas@zdiv.net \
    --cc=barebox@lists.infradead.org \
    --cc=jmaselbas@kalray.eu \
    --cc=sha@pengutronix.de \
    /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