[PATCH v2 2/8] mmc: bcm2835: Add new driver for the sdhost controller.

Stefan Wahren stefan.wahren at i2se.com
Mon Feb 6 12:34:00 PST 2017


Hi Gerd,

first of all here my wishlist for the next round of this driver:
* compile test the series with ARM and ARM64
* add me in CC for all patches of the series
* run checkpatch.pl before submission
* apply the patch series to your cgit repo

> Gerd Hoffmann <kraxel at redhat.com> hat am 3. Februar 2017 um 13:11 geschrieben:
> 
> 
> From: Eric Anholt <eric at anholt.net>
> 
> The 2835 has two SD controllers: The Arasan sdhci controller (supported
> by the iproc driver) and a custom sdhost controller.  This patch adds a
> driver for the latter.
> 
> The sdhci controller supports both sdcard and sdio.  The sdhost
> controller supports the sdcard only, but has better performance.  Also

Sorry, for the confusion. I was wrong. According to the registers the SDHOST should also support SDIO. It's a feature we could implement later.

> note that the rpi3 has sdio wifi, so driving the sdcard with the sdhost
> controller allows to use the sdhci controller for wifi support.
> 
> The configuration is done by devicetree via pin muxing.  Both SD
> controller are available on the same pins (2 pin groups = pin 22 to 27 +
> pin 48 to 53).  So it's possible to use both SD controllers at the same
> time with different pin groups.
> 
> The code was originally written by Phil Elwell in the downstream
> Rasbperry Pi tree, and I did a major cleanup on it (+319, -707 lines
> out of the original 2055) for inclusion.

I think it would be helpful to known the downstream commit, because this commit [1]
doesn't seem to be included.

[1] - https://github.com/raspberrypi/linux/commit/ea4b1c5c2ddbb6caba43ab9b0103542a4ca7e1f0

I've found and fixed a lot of issues in this version and i think it would be better if i send you the patches for squashing them all together. Here is the preview for my patch series:

Stefan Wahren (15):
  mmc: bcm2835: Add missing include for threaded irq
  mmc: bcm2835: Remove CMD_DALLY_US
  mmc: bcm2835: Fix pio_timeout handling
  mmc: bcm2835: Remove unnecessary return in bcm2835_data_irq
  mmc: bcm2835: Handle error cases during probe
  mmc: bcm2835: Print clk_max as decimal
  mmc: bcm2835: Downrate message in case of PIO fallback
  mmc: bcm2835: Don't unveil the data pointer
  mmc: bcm2835: remove unused host members
  mmc: bcm2835: Avoid unnecessary linebreaks
  mmc: bcm2835: Add leading zero to register dumps
  mmc: bcm2835: Align struct members with tabs
  mmc: bcm2835: Rearrange bcm2835_finish_request()
  mmc: bcm2835: Rearrange bcm2835_dma_complete_work()
  mmc: bcm2835: Rename Kconfig switch

I'll send it after my tests. In the following review i will mention only the issues which aren't fixed in my patch series.

> diff --git a/drivers/mmc/host/bcm2835.c b/drivers/mmc/host/bcm2835.c
> new file mode 100644
> index 0000000..5ea8bd6
> --- /dev/null
> +++ b/drivers/mmc/host/bcm2835.c
> @@ -0,0 +1,1521 @@
> +/*
> + * bcm2835 sdhost driver.
> + *
> + * The 2835 has two SD controllers: The Arasan sdhci controller
> + * (supported by the iproc driver) and a custom sdhost controller
> + * (supported by this driver).
> + *
> + * The sdhci controller supports both sdcard and sdio.  The sdhost
> + * controller supports the sdcard only, but has better performance.
> + * Also note that the rpi3 has sdio wifi, so driving the sdcard with
> + * the sdhost controller allows to use the sdhci controller for wifi
> + * support.
> + *
> + * The configuration is done by devicetree via pin muxing.  Both
> + * SD controller are available on the same pins (2 pin groups = pin 22
> + * to 27 + pin 48 to 53).  So it's possible to use both SD controllers
> + * at the same time with different pin groups.
> + *
> + * Author:      Phil Elwell <phil at raspberrypi.org>
> + *              Copyright (C) 2015-2016 Raspberry Pi (Trading) Ltd.
> + *
> + * Based on
> + *  mmc-bcm2835.c by Gellert Weisz
> + * which is, in turn, based on
> + *  sdhci-bcm2708.c by Broadcom
> + *  sdhci-bcm2835.c by Stephen Warren and Oleksandr Tymoshenko
> + *  sdhci.c and sdhci-pci.c by Pierre Ossman
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/highmem.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/scatterlist.h>
> +#include <linux/time.h>
> +#include <linux/workqueue.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +
> +#define SDCMD  0x00 /* Command to SD card              - 16 R/W */
> +#define SDARG  0x04 /* Argument to SD card             - 32 R/W */
> +#define SDTOUT 0x08 /* Start value for timeout counter - 32 R/W */
> +#define SDCDIV 0x0c /* Start value for clock divider   - 11 R/W */
> +#define SDRSP0 0x10 /* SD card response (31:0)         - 32 R   */
> +#define SDRSP1 0x14 /* SD card response (63:32)        - 32 R   */
> +#define SDRSP2 0x18 /* SD card response (95:64)        - 32 R   */
> +#define SDRSP3 0x1c /* SD card response (127:96)       - 32 R   */
> +#define SDHSTS 0x20 /* SD host status                  - 11 R   */
> +#define SDVDD  0x30 /* SD card power control           -  1 R/W */
> +#define SDEDM  0x34 /* Emergency Debug Mode            - 13 R/W */
> +#define SDHCFG 0x38 /* Host configuration              -  2 R/W */
> +#define SDHBCT 0x3c /* Host byte count (debug)         - 32 R/W */
> +#define SDDATA 0x40 /* Data to/from SD card            - 32 R/W */
> +#define SDHBLC 0x50 /* Host block count (SDIO/SDHC)    -  9 R/W */
> +
> +#define SDCMD_NEW_FLAG			0x8000
> +#define SDCMD_FAIL_FLAG			0x4000
> +#define SDCMD_BUSYWAIT			0x800
> +#define SDCMD_NO_RESPONSE		0x400
> +#define SDCMD_LONG_RESPONSE		0x200
> +#define SDCMD_WRITE_CMD			0x80
> +#define SDCMD_READ_CMD			0x40
> +#define SDCMD_CMD_MASK			0x3f
> +
> +#define SDCDIV_MAX_CDIV			0x7ff
> +
> +#define SDHSTS_BUSY_IRPT		0x400
> +#define SDHSTS_BLOCK_IRPT		0x200
> +#define SDHSTS_SDIO_IRPT		0x100
> +#define SDHSTS_REW_TIME_OUT		0x80
> +#define SDHSTS_CMD_TIME_OUT		0x40
> +#define SDHSTS_CRC16_ERROR		0x20
> +#define SDHSTS_CRC7_ERROR		0x10
> +#define SDHSTS_FIFO_ERROR		0x08
> +/* Reserved */
> +/* Reserved */
> +#define SDHSTS_DATA_FLAG		0x01
> +
> +#define SDHSTS_TRANSFER_ERROR_MASK	(SDHSTS_CRC7_ERROR | \
> +					 SDHSTS_CRC16_ERROR | \
> +					 SDHSTS_REW_TIME_OUT | \
> +					 SDHSTS_FIFO_ERROR)
> +
> +#define SDHSTS_ERROR_MASK		(SDHSTS_CMD_TIME_OUT | \
> +					 SDHSTS_TRANSFER_ERROR_MASK)
> +
> +#define SDHCFG_BUSY_IRPT_EN	BIT(10)
> +#define SDHCFG_BLOCK_IRPT_EN	BIT(8)
> +#define SDHCFG_SDIO_IRPT_EN	BIT(5)
> +#define SDHCFG_DATA_IRPT_EN	BIT(4)
> +#define SDHCFG_SLOW_CARD	BIT(3)
> +#define SDHCFG_WIDE_EXT_BUS	BIT(2)
> +#define SDHCFG_WIDE_INT_BUS	BIT(1)
> +#define SDHCFG_REL_CMD_LINE	BIT(0)
> +
> +#define SDVDD_POWER_OFF		0
> +#define SDVDD_POWER_ON		1
> +
> +#define SDEDM_FORCE_DATA_MODE	BIT(19)
> +#define SDEDM_CLOCK_PULSE	BIT(20)
> +#define SDEDM_BYPASS		BIT(21)
> +
> +#define SDEDM_WRITE_THRESHOLD_SHIFT	9
> +#define SDEDM_READ_THRESHOLD_SHIFT	14
> +#define SDEDM_THRESHOLD_MASK		0x1f
> +
> +#define SDEDM_FSM_MASK		0xf
> +#define SDEDM_FSM_IDENTMODE	0x0
> +#define SDEDM_FSM_DATAMODE	0x1
> +#define SDEDM_FSM_READDATA	0x2
> +#define SDEDM_FSM_WRITEDATA	0x3
> +#define SDEDM_FSM_READWAIT	0x4
> +#define SDEDM_FSM_READCRC	0x5
> +#define SDEDM_FSM_WRITECRC	0x6
> +#define SDEDM_FSM_WRITEWAIT1	0x7
> +#define SDEDM_FSM_POWERDOWN	0x8
> +#define SDEDM_FSM_POWERUP	0x9
> +#define SDEDM_FSM_WRITESTART1	0xa
> +#define SDEDM_FSM_WRITESTART2	0xb
> +#define SDEDM_FSM_GENPULSES	0xc
> +#define SDEDM_FSM_WRITEWAIT2	0xd
> +#define SDEDM_FSM_STARTPOWDOWN	0xf
> +
> +#define SDDATA_FIFO_WORDS	16
> +
> +#define FIFO_READ_THRESHOLD	4
> +#define FIFO_WRITE_THRESHOLD	4
> +#define SDDATA_FIFO_PIO_BURST	8
> +#define CMD_DALLY_US		1
> +
> +struct bcm2835_host {
> +	spinlock_t		lock;
> +	struct mutex            mutex;

Would be nice to have a comment for the lock and mutex.

> +
> +	void __iomem		*ioaddr;
> +	u32			phys_addr;
> +
> +	struct mmc_host		*mmc;
> +	struct platform_device	*pdev;
> +
> +	u32			pio_timeout;	/* In jiffies */
> +	int			clock;		/* Current clock speed */
> +	unsigned int		max_clk;	/* Max possible freq */
> +	struct work_struct      dma_work;
> +	struct delayed_work     timeout_work;   /* Timer for timeouts */
> +	struct sg_mapping_iter	sg_miter;	/* SG state for PIO */
> +	unsigned int		blocks;		/* remaining PIO blocks */
> +	int			irq;		/* Device IRQ */
> +
> +	u32			ns_per_fifo_word;
> +
> +	/* cached registers */
> +	u32			hcfg;
> +	u32			cdiv;
> +
> +	struct mmc_request	*mrq;		/* Current request */
> +	struct mmc_command	*cmd;		/* Current command */
> +	struct mmc_data		*data;		/* Current data request */
> +	bool			data_complete:1;/* Data finished before cmd */
> +	bool			flush_fifo:1;	/* Drain the fifo when finish */
> +	bool			use_busy:1;	/* Wait for busy interrupt */
> +	bool			use_sbc:1;	/* Send CMD23 */

It would be nice to get the rid off use_busy and use_sbc. Do you see any chance for use_busy?

> +
> +	/* for threaded irq handler */
> +	bool                    irq_block;
> +	bool                    irq_busy;
> +	bool                    irq_data;
> +
> +	/* DMA part */
> +	struct dma_chan		*dma_chan_rx;
> +	struct dma_chan		*dma_chan_tx;
> +	struct dma_chan		*dma_chan;
> +	struct dma_async_tx_descriptor	*dma_desc;
> +	u32			dma_dir;
> +	u32			drain_words;
> +	struct page		*drain_page;
> +	u32			drain_offset;
> +	bool			use_dma;
> +
> +	int	max_delay;	/* maximum length of time spent waiting */
> +	u32	pio_limit;	/* Maximum block count for PIO (0 = DMA) */
> +};
> +
> +static void bcm2835_dumpcmd(struct bcm2835_host *host,
> +			    struct mmc_command *cmd,
> +			    const char *label)
> +{
> +	struct device *dev = &host->pdev->dev;
> +
> +	if (!cmd)
> +		return;
> +
> +	dev_dbg(dev, "%c%s op %d arg 0x%x flags 0x%x - resp %08x %08x %08x %08x, err %d\n",
> +		(cmd == host->cmd) ? '>' : ' ',
> +		label, cmd->opcode, cmd->arg, cmd->flags,
> +		cmd->resp[0], cmd->resp[1], cmd->resp[2], cmd->resp[3],
> +		cmd->error);
> +}
> +
> +static void bcm2835_dumpregs(struct bcm2835_host *host)
> +{
> +	struct mmc_request *mrq = host->mrq;
> +	struct device *dev = &host->pdev->dev;
> +
> +	if (mrq) {
> +		bcm2835_dumpcmd(host, mrq->sbc, "sbc");
> +		bcm2835_dumpcmd(host, mrq->cmd, "cmd");
> +		if (mrq->data) {
> +			dev_dbg(dev, "data blocks %x blksz %x - err %d\n",
> +				mrq->data->blocks,
> +				mrq->data->blksz,
> +				mrq->data->error);
> +		}
> +		bcm2835_dumpcmd(host, mrq->stop, "stop");
> +	}
> +
> +	dev_dbg(dev, "=========== REGISTER DUMP ===========\n");
> +	dev_dbg(dev, "SDCMD  0x%08x\n", readl(host->ioaddr + SDCMD));
> +	dev_dbg(dev, "SDARG  0x%08x\n", readl(host->ioaddr + SDARG));
> +	dev_dbg(dev, "SDTOUT 0x%08x\n", readl(host->ioaddr + SDTOUT));
> +	dev_dbg(dev, "SDCDIV 0x%08x\n", readl(host->ioaddr + SDCDIV));
> +	dev_dbg(dev, "SDRSP0 0x%08x\n", readl(host->ioaddr + SDRSP0));
> +	dev_dbg(dev, "SDRSP1 0x%08x\n", readl(host->ioaddr + SDRSP1));
> +	dev_dbg(dev, "SDRSP2 0x%08x\n", readl(host->ioaddr + SDRSP2));
> +	dev_dbg(dev, "SDRSP3 0x%08x\n", readl(host->ioaddr + SDRSP3));
> +	dev_dbg(dev, "SDHSTS 0x%08x\n", readl(host->ioaddr + SDHSTS));
> +	dev_dbg(dev, "SDVDD  0x%08x\n", readl(host->ioaddr + SDVDD));
> +	dev_dbg(dev, "SDEDM  0x%08x\n", readl(host->ioaddr + SDEDM));
> +	dev_dbg(dev, "SDHCFG 0x%08x\n", readl(host->ioaddr + SDHCFG));
> +	dev_dbg(dev, "SDHBCT 0x%08x\n", readl(host->ioaddr + SDHBCT));
> +	dev_dbg(dev, "SDHBLC 0x%08x\n", readl(host->ioaddr + SDHBLC));
> +	dev_dbg(dev, "===========================================\n");
> +}
> +
> +static void bcm2835_reset_internal(struct bcm2835_host *host)
> +{
> +	u32 temp;
> +
> +	writel(SDVDD_POWER_OFF, host->ioaddr + SDVDD);
> +	writel(0, host->ioaddr + SDCMD);
> +	writel(0, host->ioaddr + SDARG);
> +	writel(0xf00000, host->ioaddr + SDTOUT);
> +	writel(0, host->ioaddr + SDCDIV);
> +	writel(0x7f8, host->ioaddr + SDHSTS); /* Write 1s to clear */

Any chance to eliminate the bit magic for SDTOUT and SDHSTS?

> +	writel(0, host->ioaddr + SDHCFG);
> +	writel(0, host->ioaddr + SDHBCT);
> +	writel(0, host->ioaddr + SDHBLC);
> +
> +	/* Limit fifo usage due to silicon bug */
> +	temp = readl(host->ioaddr + SDEDM);
> +	temp &= ~((SDEDM_THRESHOLD_MASK << SDEDM_READ_THRESHOLD_SHIFT) |
> +		  (SDEDM_THRESHOLD_MASK << SDEDM_WRITE_THRESHOLD_SHIFT));
> +	temp |= (FIFO_READ_THRESHOLD << SDEDM_READ_THRESHOLD_SHIFT) |
> +		(FIFO_WRITE_THRESHOLD << SDEDM_WRITE_THRESHOLD_SHIFT);
> +	writel(temp, host->ioaddr + SDEDM);
> +	msleep(20);
> +	writel(SDVDD_POWER_ON, host->ioaddr + SDVDD);
> +	msleep(20);
> +	host->clock = 0;
> +	writel(host->hcfg, host->ioaddr + SDHCFG);
> +	writel(host->cdiv, host->ioaddr + SDCDIV);
> +}
> +
> ...
> +
> +static void bcm2835_finish_data(struct bcm2835_host *host)
> +{
> +	struct device *dev = &host->pdev->dev;
> +	struct mmc_data *data;
> +
> +	data = host->data;
> +
> +	dev_dbg(dev, "finish_data(error %d, stop %d, sbc %d)\n",
> +		data->error, data->stop ? 1 : 0,
> +		host->mrq->sbc ? 1 : 0);
> +
> +	host->hcfg &= ~(SDHCFG_DATA_IRPT_EN | SDHCFG_BLOCK_IRPT_EN);
> +	writel(host->hcfg, host->ioaddr + SDHCFG);
> +
> +	data->bytes_xfered = data->error ? 0 : (data->blksz * data->blocks);
> +
> +	host->data_complete = true;
> +
> +	if (host->cmd) {
> +		/* Data managed to finish before the
> +		 * command completed. Make sure we do
> +		 * things in the proper order.
> +		 */
> +		dev_dbg(dev, "Finished early - HSTS %x\n",
> +			readl(host->ioaddr + SDHSTS));
> +	} else {
> +		bcm2835_transfer_complete(host);
> +	}
> +}
> +
> +static void bcm2835_finish_command(struct bcm2835_host *host)
> +{
> +	struct device *dev = &host->pdev->dev;
> +	struct mmc_command *cmd = host->cmd;
> +	u32 sdcmd;
> +
> +	dev_dbg(dev, "finish_command(%x)\n", readl(host->ioaddr + SDCMD));
> +
> +	sdcmd = bcm2835_read_wait_sdcmd(host, 100, true);
> +
> +	/* Check for errors */
> +	if (sdcmd & SDCMD_NEW_FLAG) {
> +		dev_err(dev, "command never completed.\n");
> +		bcm2835_dumpregs(host);
> +		host->cmd->error = -EIO;
> +		bcm2835_finish_request(host);
> +		return;
> +	} else if (sdcmd & SDCMD_FAIL_FLAG) {
> +		u32 sdhsts = readl(host->ioaddr + SDHSTS);
> +
> +		/* Clear the errors */
> +		writel(SDHSTS_ERROR_MASK, host->ioaddr + SDHSTS);
> +
> +		if (!(sdhsts & SDHSTS_CRC7_ERROR) ||
> +		    (host->cmd->opcode != MMC_SEND_OP_COND)) {
> +			if (sdhsts & SDHSTS_CMD_TIME_OUT) {
> +				host->cmd->error = -ETIMEDOUT;
> +			} else {
> +				dev_err(dev, "unexpected command %d error\n",
> +					host->cmd->opcode);
> +				bcm2835_dumpregs(host);
> +				host->cmd->error = -EILSEQ;
> +			}
> +			bcm2835_finish_request(host);
> +			return;
> +		}
> +	}
> +
> +	if (cmd->flags & MMC_RSP_PRESENT) {
> +		if (cmd->flags & MMC_RSP_136) {
> +			int i;
> +
> +			for (i = 0; i < 4; i++) {
> +				cmd->resp[3 - i] =
> +					readl(host->ioaddr + SDRSP0 + i * 4);
> +			}
> +
> +			dev_dbg(dev, "finish_command %08x %08x %08x %08x\n",
> +				cmd->resp[0], cmd->resp[1],
> +				cmd->resp[2], cmd->resp[3]);
> +		} else {
> +			cmd->resp[0] = readl(host->ioaddr + SDRSP0);
> +			dev_dbg(dev, "finish_command %08x\n", cmd->resp[0]);
> +		}
> +	}
> +
> +	if (cmd == host->mrq->sbc) {
> +		/* Finished CMD23, now send actual command. */
> +		host->cmd = NULL;
> +		if (bcm2835_send_command(host, host->mrq->cmd)) {
> +			if (host->data && host->dma_desc)
> +				/* DMA transfer starts now, PIO starts
> +				 * after irq
> +				 */
> +				bcm2835_start_dma(host);
> +
> +			if (!host->use_busy)
> +				bcm2835_finish_command(host);

Recursion?

> +		}
> +	} else if (cmd == host->mrq->stop) {
> +		/* Finished CMD12 */
> +		bcm2835_finish_request(host);
> +	} else {
> +		/* Processed actual command. */
> +		host->cmd = NULL;
> +		if (!host->data)
> +			bcm2835_finish_request(host);
> +		else if (host->data_complete)
> +			bcm2835_transfer_complete(host);

bcm2835_finish_command() calls bcm2835_transfer_complete()
bcm2835_transfer_complete() calls bcm2835_finish_command()

This should be avoided.

> ...
> +int bcm2835_add_host(struct bcm2835_host *host)
> +{
> +	struct mmc_host *mmc = host->mmc;
> +	struct device *dev = &host->pdev->dev;
> +	struct dma_slave_config cfg;
> +	char pio_limit_string[20];
> +	int ret;
> +
> +	bcm2835_reset_internal(host);
> +
> +	mmc->f_max = host->max_clk;
> +	mmc->f_min = host->max_clk / SDCDIV_MAX_CDIV;
> +
> +	mmc->max_busy_timeout = ~0 / (mmc->f_max / 1000);
> +
> +	dev_dbg(dev, "f_max %d, f_min %d, max_busy_timeout %d\n",
> +		mmc->f_max, mmc->f_min, mmc->max_busy_timeout);
> +
> +	/* host controller capabilities */
> +	mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
> +		     MMC_CAP_NEEDS_POLL | MMC_CAP_HW_RESET | MMC_CAP_ERASE |
> +		     MMC_CAP_CMD23;
> +
> +	spin_lock_init(&host->lock);
> +	mutex_init(&host->mutex);
> +
> +	if (IS_ERR_OR_NULL(host->dma_chan_tx) ||
> +	    IS_ERR_OR_NULL(host->dma_chan_rx)) {
> +		dev_err(dev, "unable to initialise DMA channels. Falling back to PIO\n");
> +		host->use_dma = false;
> +	} else {
> +		host->use_dma = true;
> +
> +		cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		cfg.slave_id = 13;		/* DREQ channel */

Is this the same value which is also defined in the DT?

Thanks
Stefan



More information about the linux-arm-kernel mailing list