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 <kra...@redhat.com> hat am 3. Februar 2017 um 13:11 geschrieben: > > > From: Eric Anholt <e...@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 <p...@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
