* [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
@ 2016-07-12 18:18 Steven J. Hill
2016-07-19 18:31 ` Steven J. Hill
2016-08-23 14:53 ` Ulf Hansson
0 siblings, 2 replies; 16+ messages in thread
From: Steven J. Hill @ 2016-07-12 18:18 UTC (permalink / raw)
To: linux-mmc, devicetree, linux-mips
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Ralf Baechle
The OCTEON MMC controller is currently found on cn61XX and cn71XX
devices. Device parameters are configured from device tree data.
eMMC, MMC and SD devices are supported. Tested on Rhino labs UTM8
and Cavium CN7130 board.
Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
Acked-by: David Daney <david.daney@cavium.com>
---
v8:
- Convert driver to use readq()/writeq() functions.
- Work around legacy u-boot device trees.
- Enable EMMC interrupts properly.
- Support DDR signalling. The timings are tighter, so
there may be failures with some FDT settings.
- Quiesce the device by calling mmc_remove_host() and
mmc_free_host() when unloading the driver.
- Set MIO_BOOT_CTL on cn70xx to select proper mmc mode
which is part of acquiring the bus.
- Use of_property_read_u32() helper for cleaner device
tree accesses.
- Properly implement multi-block DMA. The Octeon's DMA
enginer cannot do scatter-gather.
- Add driver parameters to limit multi-block transfers.
- Use octeon_bootbus_sem.
- Improve GPIO support and make actual requests to use the
GPIO lines before using and freeing them after.
v7:
- Revert to legacy device tree bindings handled in driver
- Tone down the warning printed when legacy bindings in use
v6:
- Split up patch
- Moved device tree fixup to platform code
Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com>
---
drivers/mmc/host/Kconfig | 10 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/octeon_mmc.c | 1331 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 1342 insertions(+)
create mode 100644 drivers/mmc/host/octeon_mmc.c
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 0aa484c..7ef7f8b 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -332,6 +332,16 @@ config MMC_SDHCI_IPROC
If unsure, say N.
+config MMC_OCTEON
+ tristate "Cavium OCTEON Multimedia Card Interface support"
+ depends on CAVIUM_OCTEON_SOC
+ help
+ This selects Cavium OCTEON Multimedia card Interface.
+ If you have an OCTEON board with a Multimedia Card slot,
+ say Y or M here.
+
+ If unsure, say N.
+
config MMC_MOXART
tristate "MOXART SD/MMC Host Controller support"
depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index af918d2..c43bd7d 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_MMC_SDHCI_SPEAR) += sdhci-spear.o
obj-$(CONFIG_MMC_WBSD) += wbsd.o
obj-$(CONFIG_MMC_AU1X) += au1xmmc.o
obj-$(CONFIG_MMC_MTK) += mtk-sd.o
+obj-$(CONFIG_MMC_OCTEON) += octeon_mmc.o
obj-$(CONFIG_MMC_OMAP) += omap.o
obj-$(CONFIG_MMC_OMAP_HS) += omap_hsmmc.o
obj-$(CONFIG_MMC_ATMELMCI) += atmel-mci.o
diff --git a/drivers/mmc/host/octeon_mmc.c b/drivers/mmc/host/octeon_mmc.c
new file mode 100644
index 0000000..d2ef434
--- /dev/null
+++ b/drivers/mmc/host/octeon_mmc.c
@@ -0,0 +1,1331 @@
+/*
+ * Driver for MMC and SSD cards for Cavium OCTEON SOCs.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2012-2016 Cavium Inc.
+ */
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/scatterlist.h>
+#include <linux/interrupt.h>
+#include <linux/blkdev.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/slot-gpio.h>
+#include <net/irda/parameters.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+#include <asm/byteorder.h>
+#include <asm/octeon/octeon.h>
+
+#define DRV_NAME "octeon_mmc"
+
+#define OCTEON_MAX_MMC 4
+
+#define OCT_MIO_NDF_DMA_CFG 0x00
+#define OCT_MIO_EMM_DMA_ADR 0x08
+
+#define OCT_MIO_EMM_CFG 0x00
+#define OCT_MIO_EMM_SWITCH 0x48
+#define OCT_MIO_EMM_DMA 0x50
+#define OCT_MIO_EMM_CMD 0x58
+#define OCT_MIO_EMM_RSP_STS 0x60
+#define OCT_MIO_EMM_RSP_LO 0x68
+#define OCT_MIO_EMM_RSP_HI 0x70
+#define OCT_MIO_EMM_INT 0x78
+#define OCT_MIO_EMM_INT_EN 0x80
+#define OCT_MIO_EMM_WDOG 0x88
+#define OCT_MIO_EMM_SAMPLE 0x90
+#define OCT_MIO_EMM_STS_MASK 0x98
+#define OCT_MIO_EMM_RCA 0xa0
+#define OCT_MIO_EMM_BUF_IDX 0xe0
+#define OCT_MIO_EMM_BUF_DAT 0xe8
+
+#define CVMX_MIO_BOOT_CTL CVMX_ADD_IO_SEG(0x00011800000000D0ull)
+
+struct octeon_mmc_host {
+ void __iomem *base;
+ void __iomem *ndf_base;
+ u64 emm_cfg;
+ u64 n_minus_one; /* OCTEON II workaround location */
+ int last_slot;
+
+ struct semaphore mmc_serializer;
+ struct mmc_request *current_req;
+ unsigned int linear_buf_size;
+ void *linear_buf;
+ struct sg_mapping_iter smi;
+ int sg_idx;
+ bool dma_active;
+
+ struct platform_device *pdev;
+ struct gpio_desc *global_pwr_gpiod;
+ bool dma_err_pending;
+ bool need_bootbus_lock;
+ bool big_dma_addr;
+ bool need_irq_handler_lock;
+ spinlock_t irq_handler_lock;
+ bool has_ciu3;
+
+ struct octeon_mmc_slot *slot[OCTEON_MAX_MMC];
+};
+
+struct octeon_mmc_slot {
+ struct mmc_host *mmc; /* slot-level mmc_core object */
+ struct octeon_mmc_host *host; /* common hw for all 4 slots */
+
+ unsigned int clock;
+ unsigned int sclock;
+
+ u64 cached_switch;
+ u64 cached_rca;
+
+ unsigned int cmd_cnt; /* sample delay */
+ unsigned int dat_cnt; /* sample delay */
+
+ int bus_width;
+ int bus_id;
+
+ /* Legacy property - in future mmc->supply.vmmc should be used */
+ struct gpio_desc *pwr_gpiod;
+};
+
+static int bb_size = 1 << 18;
+module_param(bb_size, int, S_IRUGO);
+MODULE_PARM_DESC(bb_size,
+ "Size of DMA linearizing buffer (max transfer size).");
+
+static int ddr = 2;
+module_param(ddr, int, S_IRUGO);
+MODULE_PARM_DESC(ddr,
+ "enable DoubleDataRate clocking: 0=no, 1=always, 2=at spi-max-frequency/2");
+
+static void octeon_mmc_acquire_bus(struct octeon_mmc_host *host)
+{
+ if (host->need_bootbus_lock) {
+ down(&octeon_bootbus_sem);
+ /* On cn70XX switch the mmc unit onto the bus. */
+ if (OCTEON_IS_MODEL(OCTEON_CN70XX))
+ writeq(0, (void __iomem *)CVMX_MIO_BOOT_CTL);
+ } else {
+ down(&host->mmc_serializer);
+ }
+}
+
+static void octeon_mmc_release_bus(struct octeon_mmc_host *host)
+{
+ if (host->need_bootbus_lock)
+ up(&octeon_bootbus_sem);
+ else
+ up(&host->mmc_serializer);
+}
+
+struct octeon_mmc_cr_type {
+ u8 ctype;
+ u8 rtype;
+};
+
+/*
+ * The OCTEON MMC host hardware assumes that all commands have fixed
+ * command and response types. These are correct if MMC devices are
+ * being used. However, non-MMC devices like SD use command and
+ * response types that are unexpected by the host hardware.
+ *
+ * The command and response types can be overridden by supplying an
+ * XOR value that is applied to the type. We calculate the XOR value
+ * from the values in this table and the flags passed from the MMC
+ * core.
+ */
+static struct octeon_mmc_cr_type octeon_mmc_cr_types[] = {
+ {0, 0}, /* CMD0 */
+ {0, 3}, /* CMD1 */
+ {0, 2}, /* CMD2 */
+ {0, 1}, /* CMD3 */
+ {0, 0}, /* CMD4 */
+ {0, 1}, /* CMD5 */
+ {0, 1}, /* CMD6 */
+ {0, 1}, /* CMD7 */
+ {1, 1}, /* CMD8 */
+ {0, 2}, /* CMD9 */
+ {0, 2}, /* CMD10 */
+ {1, 1}, /* CMD11 */
+ {0, 1}, /* CMD12 */
+ {0, 1}, /* CMD13 */
+ {1, 1}, /* CMD14 */
+ {0, 0}, /* CMD15 */
+ {0, 1}, /* CMD16 */
+ {1, 1}, /* CMD17 */
+ {1, 1}, /* CMD18 */
+ {3, 1}, /* CMD19 */
+ {2, 1}, /* CMD20 */
+ {0, 0}, /* CMD21 */
+ {0, 0}, /* CMD22 */
+ {0, 1}, /* CMD23 */
+ {2, 1}, /* CMD24 */
+ {2, 1}, /* CMD25 */
+ {2, 1}, /* CMD26 */
+ {2, 1}, /* CMD27 */
+ {0, 1}, /* CMD28 */
+ {0, 1}, /* CMD29 */
+ {1, 1}, /* CMD30 */
+ {1, 1}, /* CMD31 */
+ {0, 0}, /* CMD32 */
+ {0, 0}, /* CMD33 */
+ {0, 0}, /* CMD34 */
+ {0, 1}, /* CMD35 */
+ {0, 1}, /* CMD36 */
+ {0, 0}, /* CMD37 */
+ {0, 1}, /* CMD38 */
+ {0, 4}, /* CMD39 */
+ {0, 5}, /* CMD40 */
+ {0, 0}, /* CMD41 */
+ {2, 1}, /* CMD42 */
+ {0, 0}, /* CMD43 */
+ {0, 0}, /* CMD44 */
+ {0, 0}, /* CMD45 */
+ {0, 0}, /* CMD46 */
+ {0, 0}, /* CMD47 */
+ {0, 0}, /* CMD48 */
+ {0, 0}, /* CMD49 */
+ {0, 0}, /* CMD50 */
+ {0, 0}, /* CMD51 */
+ {0, 0}, /* CMD52 */
+ {0, 0}, /* CMD53 */
+ {0, 0}, /* CMD54 */
+ {0, 1}, /* CMD55 */
+ {0xff, 0xff}, /* CMD56 */
+ {0, 0}, /* CMD57 */
+ {0, 0}, /* CMD58 */
+ {0, 0}, /* CMD59 */
+ {0, 0}, /* CMD60 */
+ {0, 0}, /* CMD61 */
+ {0, 0}, /* CMD62 */
+ {0, 0} /* CMD63 */
+};
+
+struct octeon_mmc_cr_mods {
+ u8 ctype_xor;
+ u8 rtype_xor;
+};
+
+/*
+ * The functions below are used for the EMMC-17978 workaround.
+ *
+ * Due to an imperfection in the design of the MMC bus hardware,
+ * the 2nd to last cache block of a DMA read must be locked into the L2 Cache.
+ * Otherwise, data corruption may occur.
+ */
+
+static inline void *phys_to_ptr(u64 address)
+{
+ return (void *)(address | (1ull<<63)); /* XKPHYS */
+}
+
+/**
+ * Lock a single line into L2. The line is zeroed before locking
+ * to make sure no dram accesses are made.
+ *
+ * @addr Physical address to lock
+ */
+static void l2c_lock_line(u64 addr)
+{
+ char *addr_ptr = phys_to_ptr(addr);
+ asm volatile (
+ "cache 31, %[line]" /* Unlock the line */
+ :: [line] "m" (*addr_ptr));
+}
+
+/**
+ * Locks a memory region in the L2 cache
+ *
+ * @start - start address to begin locking
+ * @len - length in bytes to lock
+ */
+static void l2c_lock_mem_region(u64 start, u64 len)
+{
+ u64 end;
+
+ /* Round start/end to cache line boundaries */
+ end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
+ start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
+
+ while (start <= end) {
+ l2c_lock_line(start);
+ start += CVMX_CACHE_LINE_SIZE;
+ }
+ asm volatile("sync");
+}
+
+/**
+ * Unlock a single line in the L2 cache.
+ *
+ * @addr Physical address to unlock
+ *
+ * Return Zero on success
+ */
+static void l2c_unlock_line(u64 addr)
+{
+ char *addr_ptr = phys_to_ptr(addr);
+ asm volatile (
+ "cache 23, %[line]" /* Unlock the line */
+ :: [line] "m" (*addr_ptr));
+}
+
+/**
+ * Unlock a memory region in the L2 cache
+ *
+ * @start - start address to unlock
+ * @len - length to unlock in bytes
+ */
+static void l2c_unlock_mem_region(u64 start, u64 len)
+{
+ u64 end;
+
+ /* Round start/end to cache line boundaries */
+ end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
+ start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
+
+ while (start <= end) {
+ l2c_unlock_line(start);
+ start += CVMX_CACHE_LINE_SIZE;
+ }
+}
+
+static struct octeon_mmc_cr_mods octeon_mmc_get_cr_mods(struct mmc_command *cmd)
+{
+ struct octeon_mmc_cr_type *cr;
+ u8 desired_ctype, hardware_ctype;
+ u8 desired_rtype, hardware_rtype;
+ struct octeon_mmc_cr_mods r;
+
+ desired_ctype = desired_rtype = 0;
+
+ cr = octeon_mmc_cr_types + (cmd->opcode & 0x3f);
+ hardware_ctype = cr->ctype;
+ hardware_rtype = cr->rtype;
+ if (cmd->opcode == MMC_GEN_CMD)
+ hardware_ctype = (cmd->arg & 1) ? 1 : 2;
+
+ switch (mmc_cmd_type(cmd)) {
+ case MMC_CMD_ADTC:
+ desired_ctype = (cmd->data->flags & MMC_DATA_WRITE) ? 2 : 1;
+ break;
+ case MMC_CMD_AC:
+ case MMC_CMD_BC:
+ case MMC_CMD_BCR:
+ desired_ctype = 0;
+ break;
+ }
+
+ switch (mmc_resp_type(cmd)) {
+ case MMC_RSP_NONE:
+ desired_rtype = 0;
+ break;
+ case MMC_RSP_R1:/* MMC_RSP_R5, MMC_RSP_R6, MMC_RSP_R7 */
+ case MMC_RSP_R1B:
+ desired_rtype = 1;
+ break;
+ case MMC_RSP_R2:
+ desired_rtype = 2;
+ break;
+ case MMC_RSP_R3: /* MMC_RSP_R4 */
+ desired_rtype = 3;
+ break;
+ }
+ r.ctype_xor = desired_ctype ^ hardware_ctype;
+ r.rtype_xor = desired_rtype ^ hardware_rtype;
+ return r;
+}
+
+static bool octeon_mmc_switch_val_changed(struct octeon_mmc_slot *slot,
+ u64 new_val)
+{
+ /* Match BUS_ID, HS_TIMING, BUS_WIDTH, POWER_CLASS, CLK_HI, CLK_LO */
+ u64 m = 0x3001070fffffffffull;
+ return (slot->cached_switch & m) != (new_val & m);
+}
+
+static unsigned int octeon_mmc_timeout_to_wdog(struct octeon_mmc_slot *slot,
+ unsigned int ns)
+{
+ u64 bt = (u64)slot->clock * (u64)ns;
+ return (unsigned int)(bt / 1000000000);
+}
+
+static irqreturn_t octeon_mmc_interrupt(int irq, void *dev_id)
+{
+ struct octeon_mmc_host *host = dev_id;
+ union cvmx_mio_emm_int emm_int;
+ struct mmc_request *req;
+ bool host_done;
+ union cvmx_mio_emm_rsp_sts rsp_sts;
+ unsigned long flags = 0;
+
+ if (host->need_irq_handler_lock)
+ spin_lock_irqsave(&host->irq_handler_lock, flags);
+ else
+ __acquire(&host->irq_handler_lock);
+ emm_int.u64 = readq(host->base + OCT_MIO_EMM_INT);
+ req = host->current_req;
+ writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
+
+ if (!req)
+ goto out;
+
+ rsp_sts.u64 = readq(host->base + OCT_MIO_EMM_RSP_STS);
+
+ if (host->dma_err_pending) {
+ host->current_req = NULL;
+ host->dma_err_pending = false;
+ req->done(req);
+ host_done = true;
+ goto no_req_done;
+ }
+
+ if (!host->dma_active && emm_int.s.buf_done && req->data) {
+ unsigned int type = (rsp_sts.u64 >> 7) & 3;
+
+ if (type == 1) {
+ /* Read */
+ int dbuf = rsp_sts.s.dbuf;
+ struct sg_mapping_iter *smi = &host->smi;
+ unsigned int data_len =
+ req->data->blksz * req->data->blocks;
+ unsigned int bytes_xfered;
+ u64 dat = 0;
+ int shift = -1;
+
+ /* Auto inc from offset zero */
+ writeq((u64)(0x10000 | (dbuf << 6)),
+ host->base + OCT_MIO_EMM_BUF_IDX);
+
+ for (bytes_xfered = 0; bytes_xfered < data_len;) {
+ if (smi->consumed >= smi->length) {
+ if (!sg_miter_next(smi))
+ break;
+ smi->consumed = 0;
+ }
+ if (shift < 0) {
+ dat = readq(host->base +
+ OCT_MIO_EMM_BUF_DAT);
+ shift = 56;
+ }
+
+ while (smi->consumed < smi->length &&
+ shift >= 0) {
+ ((u8 *)(smi->addr))[smi->consumed] =
+ (dat >> shift) & 0xff;
+ bytes_xfered++;
+ smi->consumed++;
+ shift -= 8;
+ }
+ }
+ sg_miter_stop(smi);
+ req->data->bytes_xfered = bytes_xfered;
+ req->data->error = 0;
+ } else if (type == 2) {
+ /* write */
+ req->data->bytes_xfered = req->data->blksz *
+ req->data->blocks;
+ req->data->error = 0;
+ }
+ }
+ host_done = emm_int.s.cmd_done || emm_int.s.dma_done ||
+ emm_int.s.cmd_err || emm_int.s.dma_err;
+ if (host_done && req->done) {
+ if (rsp_sts.s.rsp_bad_sts ||
+ rsp_sts.s.rsp_crc_err ||
+ rsp_sts.s.rsp_timeout ||
+ rsp_sts.s.blk_crc_err ||
+ rsp_sts.s.blk_timeout ||
+ rsp_sts.s.dbuf_err) {
+ req->cmd->error = -EILSEQ;
+ } else {
+ req->cmd->error = 0;
+ }
+
+ if (host->dma_active && req->data) {
+ req->data->error = 0;
+ req->data->bytes_xfered = req->data->blocks *
+ req->data->blksz;
+ if (!(req->data->flags & MMC_DATA_WRITE) &&
+ req->data->sg_len > 1) {
+ size_t r = sg_copy_from_buffer(req->data->sg,
+ req->data->sg_len, host->linear_buf,
+ req->data->bytes_xfered);
+ WARN_ON(r != req->data->bytes_xfered);
+ }
+ }
+ if (rsp_sts.s.rsp_val) {
+ u64 rsp_hi;
+ u64 rsp_lo = readq(host->base + OCT_MIO_EMM_RSP_LO);
+
+ switch (rsp_sts.s.rsp_type) {
+ case 1:
+ case 3:
+ req->cmd->resp[0] = (rsp_lo >> 8) & 0xffffffff;
+ req->cmd->resp[1] = 0;
+ req->cmd->resp[2] = 0;
+ req->cmd->resp[3] = 0;
+ break;
+ case 2:
+ req->cmd->resp[3] = rsp_lo & 0xffffffff;
+ req->cmd->resp[2] = (rsp_lo >> 32) & 0xffffffff;
+ rsp_hi = readq(host->base + OCT_MIO_EMM_RSP_HI);
+ req->cmd->resp[1] = rsp_hi & 0xffffffff;
+ req->cmd->resp[0] = (rsp_hi >> 32) & 0xffffffff;
+ default:
+ break;
+ }
+ }
+ if (emm_int.s.dma_err && rsp_sts.s.dma_pend) {
+ /* Try to clean up failed DMA */
+ union cvmx_mio_emm_dma emm_dma;
+
+ emm_dma.u64 = readq(host->base + OCT_MIO_EMM_DMA);
+ emm_dma.s.dma_val = 1;
+ emm_dma.s.dat_null = 1;
+ emm_dma.s.bus_id = rsp_sts.s.bus_id;
+ writeq(emm_dma.u64, host->base + OCT_MIO_EMM_DMA);
+ host->dma_err_pending = true;
+ host_done = false;
+ goto no_req_done;
+ }
+
+ host->current_req = NULL;
+ req->done(req);
+ }
+no_req_done:
+ if (host->n_minus_one) {
+ l2c_unlock_mem_region(host->n_minus_one, 512);
+ host->n_minus_one = 0;
+ }
+ if (host_done)
+ octeon_mmc_release_bus(host);
+out:
+ if (host->need_irq_handler_lock)
+ spin_unlock_irqrestore(&host->irq_handler_lock, flags);
+ else
+ __release(&host->irq_handler_lock);
+ return IRQ_RETVAL(emm_int.u64 != 0);
+}
+
+static void octeon_mmc_switch_to(struct octeon_mmc_slot *slot)
+{
+ struct octeon_mmc_host *host = slot->host;
+ struct octeon_mmc_slot *old_slot;
+ union cvmx_mio_emm_switch sw;
+ union cvmx_mio_emm_sample samp;
+
+ if (slot->bus_id == host->last_slot)
+ goto out;
+
+ if (host->last_slot >= 0 && host->slot[host->last_slot]) {
+ old_slot = host->slot[host->last_slot];
+ old_slot->cached_switch = readq(host->base + OCT_MIO_EMM_SWITCH);
+ old_slot->cached_rca = readq(host->base + OCT_MIO_EMM_RCA);
+ }
+ writeq(slot->cached_rca, host->base + OCT_MIO_EMM_RCA);
+ sw.u64 = slot->cached_switch;
+ sw.s.bus_id = 0;
+ writeq(sw.u64, host->base + OCT_MIO_EMM_SWITCH);
+ sw.s.bus_id = slot->bus_id;
+ writeq(sw.u64, host->base + OCT_MIO_EMM_SWITCH);
+
+ samp.u64 = 0;
+ samp.s.cmd_cnt = slot->cmd_cnt;
+ samp.s.dat_cnt = slot->dat_cnt;
+ writeq(samp.u64, host->base + OCT_MIO_EMM_SAMPLE);
+out:
+ host->last_slot = slot->bus_id;
+}
+
+static void octeon_mmc_dma_request(struct mmc_host *mmc,
+ struct mmc_request *mrq)
+{
+ struct octeon_mmc_slot *slot;
+ struct octeon_mmc_host *host;
+ struct mmc_command *cmd;
+ struct mmc_data *data;
+ union cvmx_mio_emm_int emm_int;
+ union cvmx_mio_emm_dma emm_dma;
+ union cvmx_mio_ndf_dma_cfg dma_cfg;
+
+ cmd = mrq->cmd;
+ if (mrq->data == NULL || mrq->data->sg == NULL || !mrq->data->sg_len ||
+ mrq->stop == NULL || mrq->stop->opcode != MMC_STOP_TRANSMISSION) {
+ dev_err(&mmc->card->dev,
+ "Error: octeon_mmc_dma_request no data\n");
+ cmd->error = -EINVAL;
+ if (mrq->done)
+ mrq->done(mrq);
+ return;
+ }
+
+ slot = mmc_priv(mmc);
+ host = slot->host;
+
+ /* Only a single user of the bootbus at a time. */
+ octeon_mmc_acquire_bus(host);
+
+ octeon_mmc_switch_to(slot);
+
+ data = mrq->data;
+
+ if (data->timeout_ns)
+ writeq(octeon_mmc_timeout_to_wdog(slot, data->timeout_ns),
+ host->base + OCT_MIO_EMM_WDOG);
+
+ WARN_ON(host->current_req);
+ host->current_req = mrq;
+
+ host->sg_idx = 0;
+
+ WARN_ON(data->blksz * data->blocks > host->linear_buf_size);
+
+ if ((data->flags & MMC_DATA_WRITE) && data->sg_len > 1) {
+ size_t r = sg_copy_to_buffer(data->sg, data->sg_len,
+ host->linear_buf, data->blksz * data->blocks);
+ WARN_ON(data->blksz * data->blocks != r);
+ }
+
+ dma_cfg.u64 = 0;
+ dma_cfg.s.en = 1;
+ dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
+#ifdef __LITTLE_ENDIAN
+ dma_cfg.s.endian = 1;
+#endif
+ dma_cfg.s.size = ((data->blksz * data->blocks) / 8) - 1;
+ if (!host->big_dma_addr) {
+ if (data->sg_len > 1)
+ dma_cfg.s.adr = virt_to_phys(host->linear_buf);
+ else
+ dma_cfg.s.adr = sg_phys(data->sg);
+ }
+ writeq(dma_cfg.u64, host->ndf_base + OCT_MIO_NDF_DMA_CFG);
+ if (host->big_dma_addr) {
+ u64 addr;
+
+ if (data->sg_len > 1)
+ addr = virt_to_phys(host->linear_buf);
+ else
+ addr = sg_phys(data->sg);
+ writeq(addr, host->ndf_base + OCT_MIO_EMM_DMA_ADR);
+ }
+
+ emm_dma.u64 = 0;
+ emm_dma.s.bus_id = slot->bus_id;
+ emm_dma.s.dma_val = 1;
+ emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
+ emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
+ if (mmc_card_mmc(mmc->card) ||
+ (mmc_card_sd(mmc->card) &&
+ (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
+ emm_dma.s.multi = 1;
+ emm_dma.s.block_cnt = data->blocks;
+ emm_dma.s.card_addr = cmd->arg;
+
+ emm_int.u64 = 0;
+ emm_int.s.dma_done = 1;
+ emm_int.s.cmd_err = 1;
+ emm_int.s.dma_err = 1;
+ /* Clear the bit. */
+ writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
+ if (!host->has_ciu3)
+ writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT_EN);
+ host->dma_active = true;
+
+ if ((OCTEON_IS_MODEL(OCTEON_CN6XXX) ||
+ OCTEON_IS_MODEL(OCTEON_CNF7XXX)) &&
+ cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK &&
+ (data->blksz * data->blocks) > 1024) {
+ host->n_minus_one = dma_cfg.s.adr +
+ (data->blksz * data->blocks) - 1024;
+ l2c_lock_mem_region(host->n_minus_one, 512);
+ }
+
+ if (mmc->card && mmc_card_sd(mmc->card))
+ writeq(0x00b00000ull, host->base + OCT_MIO_EMM_STS_MASK);
+ else
+ writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
+ writeq(emm_dma.u64, host->base + OCT_MIO_EMM_DMA);
+}
+
+static void octeon_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct octeon_mmc_slot *slot;
+ struct octeon_mmc_host *host;
+ struct mmc_command *cmd;
+ union cvmx_mio_emm_int emm_int;
+ union cvmx_mio_emm_cmd emm_cmd;
+ struct octeon_mmc_cr_mods mods;
+
+ cmd = mrq->cmd;
+
+ if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK ||
+ cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) {
+ octeon_mmc_dma_request(mmc, mrq);
+ return;
+ }
+
+ mods = octeon_mmc_get_cr_mods(cmd);
+
+ slot = mmc_priv(mmc);
+ host = slot->host;
+
+ /* Only a single user of the bootbus at a time. */
+ octeon_mmc_acquire_bus(host);
+
+ octeon_mmc_switch_to(slot);
+
+ WARN_ON(host->current_req);
+ host->current_req = mrq;
+
+ emm_int.u64 = 0;
+ emm_int.s.cmd_done = 1;
+ emm_int.s.cmd_err = 1;
+ if (cmd->data) {
+ if (cmd->data->flags & MMC_DATA_READ) {
+ sg_miter_start(&host->smi, mrq->data->sg,
+ mrq->data->sg_len,
+ SG_MITER_ATOMIC | SG_MITER_TO_SG);
+ } else {
+ struct sg_mapping_iter *smi = &host->smi;
+ unsigned int data_len =
+ mrq->data->blksz * mrq->data->blocks;
+ unsigned int bytes_xfered;
+ u64 dat = 0;
+ int shift = 56;
+ /*
+ * Copy data to the xmit buffer before
+ * issuing the command
+ */
+ sg_miter_start(smi, mrq->data->sg,
+ mrq->data->sg_len, SG_MITER_FROM_SG);
+ /* Auto inc from offset zero, dbuf zero */
+ writeq(0x10000ull, host->base + OCT_MIO_EMM_BUF_IDX);
+
+ for (bytes_xfered = 0; bytes_xfered < data_len;) {
+ if (smi->consumed >= smi->length) {
+ if (!sg_miter_next(smi))
+ break;
+ smi->consumed = 0;
+ }
+
+ while (smi->consumed < smi->length &&
+ shift >= 0) {
+
+ dat |= (u64)(((u8 *)(smi->addr))
+ [smi->consumed]) << shift;
+ bytes_xfered++;
+ smi->consumed++;
+ shift -= 8;
+ }
+ if (shift < 0) {
+ writeq(dat, host->base +
+ OCT_MIO_EMM_BUF_DAT);
+ shift = 56;
+ dat = 0;
+ }
+ }
+ sg_miter_stop(smi);
+ }
+ if (cmd->data->timeout_ns)
+ writeq(octeon_mmc_timeout_to_wdog(slot,
+ cmd->data->timeout_ns),
+ host->base + OCT_MIO_EMM_WDOG);
+ } else {
+ writeq(((u64)slot->clock * 850ull) / 1000ull,
+ host->base + OCT_MIO_EMM_WDOG);
+ }
+ /* Clear the bit. */
+ writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
+ writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT_EN);
+ host->dma_active = false;
+
+ emm_cmd.u64 = 0;
+ emm_cmd.s.cmd_val = 1;
+ emm_cmd.s.ctype_xor = mods.ctype_xor;
+ emm_cmd.s.rtype_xor = mods.rtype_xor;
+ if (mmc_cmd_type(cmd) == MMC_CMD_ADTC)
+ emm_cmd.s.offset = 64 -
+ ((cmd->data->blksz * cmd->data->blocks) / 8);
+ emm_cmd.s.bus_id = slot->bus_id;
+ emm_cmd.s.cmd_idx = cmd->opcode;
+ emm_cmd.s.arg = cmd->arg;
+ writeq(0, host->base + OCT_MIO_EMM_STS_MASK);
+ writeq(emm_cmd.u64, host->base + OCT_MIO_EMM_CMD);
+}
+
+static void octeon_mmc_reset_bus(struct octeon_mmc_slot *slot)
+{
+ union cvmx_mio_emm_cfg emm_cfg;
+ union cvmx_mio_emm_switch emm_switch;
+ u64 wdog = 0;
+
+ emm_cfg.u64 = readq(slot->host->base + OCT_MIO_EMM_CFG);
+ emm_switch.u64 = readq(slot->host->base + OCT_MIO_EMM_SWITCH);
+ wdog = readq(slot->host->base + OCT_MIO_EMM_WDOG);
+
+ emm_switch.s.switch_exe = 0;
+ emm_switch.s.switch_err0 = 0;
+ emm_switch.s.switch_err1 = 0;
+ emm_switch.s.switch_err2 = 0;
+ emm_switch.s.bus_id = 0;
+ writeq(emm_switch.u64, slot->host->base + OCT_MIO_EMM_SWITCH);
+ emm_switch.s.bus_id = slot->bus_id;
+ writeq(emm_switch.u64, slot->host->base + OCT_MIO_EMM_SWITCH);
+
+ slot->cached_switch = emm_switch.u64;
+
+ msleep(20);
+
+ writeq(wdog, slot->host->base + OCT_MIO_EMM_WDOG);
+}
+
+static void octeon_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct octeon_mmc_slot *slot;
+ struct octeon_mmc_host *host;
+ int bus_width;
+ int clock;
+ bool ddr_clock;
+ int hs_timing;
+ int power_class = 10;
+ int clk_period;
+ int timeout = 2000;
+ union cvmx_mio_emm_switch emm_switch;
+ union cvmx_mio_emm_rsp_sts emm_sts;
+
+ slot = mmc_priv(mmc);
+ host = slot->host;
+
+ /* Only a single user of the bootbus at a time. */
+ octeon_mmc_acquire_bus(host);
+
+ octeon_mmc_switch_to(slot);
+
+ /*
+ * Reset the chip on each power off
+ */
+ if (ios->power_mode == MMC_POWER_OFF) {
+ octeon_mmc_reset_bus(slot);
+ if (!IS_ERR(mmc->supply.vmmc))
+ regulator_disable(mmc->supply.vmmc);
+ else /* Legacy power GPIO */
+ gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
+ } else {
+ if (!IS_ERR(mmc->supply.vmmc))
+ regulator_enable(mmc->supply.vmmc);
+ else /* Legacy power GPIO */
+ gpiod_set_value_cansleep(slot->pwr_gpiod, 1);
+ }
+
+ switch (ios->bus_width) {
+ case MMC_BUS_WIDTH_8:
+ bus_width = 2;
+ break;
+ case MMC_BUS_WIDTH_4:
+ bus_width = 1;
+ break;
+ case MMC_BUS_WIDTH_1:
+ bus_width = 0;
+ break;
+ default:
+ bus_width = 0;
+ break;
+ }
+
+ hs_timing = (ios->timing == MMC_TIMING_MMC_HS);
+ ddr_clock = (bus_width && ios->timing >= MMC_TIMING_UHS_DDR50);
+
+ if (ddr_clock)
+ bus_width |= 4;
+
+ if (ios->clock) {
+ slot->clock = ios->clock;
+ slot->bus_width = bus_width;
+
+ clock = slot->clock;
+
+ if (clock > 52000000)
+ clock = 52000000;
+
+ clk_period = (octeon_get_io_clock_rate() + clock - 1) /
+ (2 * clock);
+
+ /* until clock-renengotiate-on-CRC is in */
+ if (ddr_clock && ddr > 1)
+ clk_period *= 2;
+
+ emm_switch.u64 = 0;
+ emm_switch.s.hs_timing = hs_timing;
+ emm_switch.s.bus_width = bus_width;
+ emm_switch.s.power_class = power_class;
+ emm_switch.s.clk_hi = clk_period;
+ emm_switch.s.clk_lo = clk_period;
+
+ if (!octeon_mmc_switch_val_changed(slot, emm_switch.u64))
+ goto out;
+
+ writeq(((u64)clock * 850ull) / 1000ull,
+ host->base + OCT_MIO_EMM_WDOG);
+ writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
+ emm_switch.s.bus_id = slot->bus_id;
+ writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
+ slot->cached_switch = emm_switch.u64;
+
+ do {
+ emm_sts.u64 = readq(host->base + OCT_MIO_EMM_RSP_STS);
+ if (!emm_sts.s.switch_val)
+ break;
+ udelay(100);
+ } while (timeout-- > 0);
+
+ if (timeout <= 0)
+ goto out;
+ }
+out:
+ octeon_mmc_release_bus(host);
+}
+
+static const struct mmc_host_ops octeon_mmc_ops = {
+ .request = octeon_mmc_request,
+ .set_ios = octeon_mmc_set_ios,
+ .get_ro = mmc_gpio_get_ro,
+ .get_cd = mmc_gpio_get_cd,
+};
+
+static void octeon_mmc_set_clock(struct octeon_mmc_slot *slot,
+ unsigned int clock)
+{
+ struct mmc_host *mmc = slot->mmc;
+
+ clock = min(clock, mmc->f_max);
+ clock = max(clock, mmc->f_min);
+ slot->clock = clock;
+}
+
+static int octeon_mmc_initlowlevel(struct octeon_mmc_slot *slot,
+ int bus_width)
+{
+ union cvmx_mio_emm_switch emm_switch;
+ struct octeon_mmc_host *host = slot->host;
+
+ host->emm_cfg |= 1ull << slot->bus_id;
+ writeq(host->emm_cfg, slot->host->base + OCT_MIO_EMM_CFG);
+ octeon_mmc_set_clock(slot, 400000);
+
+ /* Program initial clock speed and power */
+ emm_switch.u64 = 0;
+ emm_switch.s.power_class = 10;
+ emm_switch.s.clk_hi = (slot->sclock / slot->clock) / 2;
+ emm_switch.s.clk_lo = (slot->sclock / slot->clock) / 2;
+
+ writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
+ emm_switch.s.bus_id = slot->bus_id;
+ writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
+ slot->cached_switch = emm_switch.u64;
+
+ writeq(((u64)slot->clock * 850ull) / 1000ull,
+ host->base + OCT_MIO_EMM_WDOG);
+ writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
+ writeq(1, host->base + OCT_MIO_EMM_RCA);
+ return 0;
+}
+
+static int octeon_mmc_slot_probe(struct platform_device *slot_pdev,
+ struct octeon_mmc_host *host)
+{
+ struct mmc_host *mmc;
+ struct octeon_mmc_slot *slot;
+ struct device *dev = &slot_pdev->dev;
+ struct device_node *node = slot_pdev->dev.of_node;
+ u32 id, bus_width, max_freq, cmd_skew, dat_skew;
+ u64 clock_period;
+ int ret;
+
+ ret = of_property_read_u32(node, "reg", &id);
+ if (ret) {
+ dev_err(dev, "Missing or invalid reg property on %s\n",
+ of_node_full_name(node));
+ return ret;
+ }
+
+ if (id >= OCTEON_MAX_MMC || host->slot[id]) {
+ dev_err(dev, "Invalid reg property on %s\n",
+ of_node_full_name(node));
+ return -EINVAL;
+ }
+
+ mmc = mmc_alloc_host(sizeof(struct octeon_mmc_slot), dev);
+ if (!mmc) {
+ dev_err(dev, "alloc host failed\n");
+ return -ENOMEM;
+ }
+
+ slot = mmc_priv(mmc);
+ slot->mmc = mmc;
+ slot->host = host;
+
+ /*
+ * The "cavium,bus-max-width" property is DEPRECATED and should
+ * not be used. We handle it here to support older firmware.
+ * Going forward, the standard "bus-width" property is used
+ * instead of the Cavium-specific property.
+ */
+ ret = of_property_read_u32(node, "bus-width", &bus_width);
+ if (ret) {
+ /* Try legacy "cavium,bus-max-width" property. */
+ ret = of_property_read_u32(node, "cavium,bus-max-width",
+ &bus_width);
+ if (ret) {
+ /* No bus width specified, use default. */
+ bus_width = 8;
+ dev_info(dev, "Default bus width %u used for slot %u\n",
+ bus_width, id);
+ }
+ }
+
+
+ switch (bus_width) {
+ case 1:
+ case 4:
+ case 8:
+ break;
+ default:
+ dev_err(dev, "Invalid bus width for slot %u\n", id);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ /*
+ * The "spi-max-frequency" property is DEPRECATED and should
+ * not be used. We handle it here to support older firmware.
+ * Going forward, the standard "max-frequency" property is
+ * used instead.
+ */
+ ret = of_property_read_u32(node, "max-frequency", &max_freq);
+ if (ret) {
+ /* Try legacy "spi-max-frequency" property. */
+ ret = of_property_read_u32(node, "spi-max-frequency",
+ &max_freq);
+ if (ret) {
+ /* No frequency properties found, use default. */
+ max_freq = 52000000;
+ dev_info(dev, "Default %u frequency used for slot %u\n",
+ id, max_freq);
+ }
+ }
+
+ /* Get regulators and the supported OCR mask */
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err;
+
+ /* Alternatively a GPIO may be specified to control slot power */
+ slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
+
+ /* Octeon specific DT properties */
+ ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
+ if (ret)
+ cmd_skew = 0;
+
+ ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
+ if (ret)
+ dat_skew = 0;
+
+ /*
+ * Set up host parameters.
+ */
+ mmc->ops = &octeon_mmc_ops;
+ mmc->f_min = 400000;
+ mmc->f_max = max_freq;
+ mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
+ MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA |
+ MMC_CAP_ERASE;
+ mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
+ MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
+ MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
+
+ /* post-sdk23 caps */
+ mmc->caps |=
+ ((mmc->f_max >= 12000000) * MMC_CAP_UHS_SDR12) |
+ ((mmc->f_max >= 25000000) * MMC_CAP_UHS_SDR25) |
+ ((mmc->f_max >= 50000000) * MMC_CAP_UHS_SDR50) |
+ MMC_CAP_CMD23;
+
+ if ((!IS_ERR(mmc->supply.vmmc)) || (slot->pwr_gpiod))
+ mmc->caps |= MMC_CAP_POWER_OFF_CARD;
+
+ /* "1.8v" capability is actually 1.8-or-3.3v */
+ if (ddr)
+ mmc->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR;
+
+ mmc->max_segs = 64;
+ mmc->max_seg_size = host->linear_buf_size;
+ mmc->max_req_size = host->linear_buf_size;
+ mmc->max_blk_size = 512;
+ mmc->max_blk_count = mmc->max_req_size / 512;
+
+ slot->clock = mmc->f_min;
+ slot->sclock = octeon_get_io_clock_rate();
+
+ clock_period = 1000000000000ull / slot->sclock; /* period in pS */
+ slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period;
+ slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period;
+
+ slot->bus_width = bus_width;
+ slot->bus_id = id;
+ slot->cached_rca = 1;
+
+ /* Only a single user of the bootbus at a time. */
+ octeon_mmc_acquire_bus(host);
+ host->slot[id] = slot;
+
+ octeon_mmc_switch_to(slot);
+ /* Initialize MMC Block. */
+ octeon_mmc_initlowlevel(slot, bus_width);
+
+ octeon_mmc_release_bus(host);
+
+ ret = mmc_add_host(mmc);
+ if (ret) {
+ dev_err(dev, "mmc_add_host() returned %d\n", ret);
+ goto err;
+ }
+
+ return 0;
+
+err:
+ slot->host->slot[id] = NULL;
+
+ gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
+
+ mmc_free_host(slot->mmc);
+ return ret;
+}
+
+static int octeon_mmc_slot_remove(struct octeon_mmc_slot *slot)
+{
+ mmc_remove_host(slot->mmc);
+
+ slot->host->slot[slot->bus_id] = NULL;
+
+ gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
+
+ mmc_free_host(slot->mmc);
+
+ return 0;
+}
+
+static int octeon_mmc_probe(struct platform_device *pdev)
+{
+ struct octeon_mmc_host *host;
+ struct resource *res;
+ void __iomem *base;
+ int mmc_irq[9];
+ int i;
+ int ret = 0;
+ struct device_node *node = pdev->dev.of_node;
+ struct device_node *cn;
+ bool cn78xx_style;
+ u64 t;
+
+ host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
+ if (!host) {
+ dev_err(&pdev->dev, "devm_kzalloc failed\n");
+ return -ENOMEM;
+ }
+
+ spin_lock_init(&host->irq_handler_lock);
+ sema_init(&host->mmc_serializer, 1);
+
+ cn78xx_style = of_device_is_compatible(node, "cavium,octeon-7890-mmc");
+ if (cn78xx_style) {
+ host->need_bootbus_lock = false;
+ host->big_dma_addr = true;
+ host->need_irq_handler_lock = true;
+ host->has_ciu3 = true;
+ /*
+ * First seven are the EMM_INT bits 0..6, then two for
+ * the EMM_DMA_INT bits
+ */
+ for (i = 0; i < 9; i++) {
+ mmc_irq[i] = platform_get_irq(pdev, i);
+ if (mmc_irq[i] < 0)
+ return mmc_irq[i];
+
+ /* work around legacy u-boot device trees */
+ irq_set_irq_type(mmc_irq[i], IRQ_TYPE_EDGE_RISING);
+ }
+ } else {
+ host->need_bootbus_lock = true;
+ host->big_dma_addr = false;
+ host->need_irq_handler_lock = false;
+ host->has_ciu3 = false;
+ /* First one is EMM second NDF_DMA */
+ for (i = 0; i < 2; i++) {
+ mmc_irq[i] = platform_get_irq(pdev, i);
+ if (mmc_irq[i] < 0)
+ return mmc_irq[i];
+ }
+ }
+ host->last_slot = -1;
+
+ if (bb_size < 512 || bb_size >= (1 << 24))
+ bb_size = 1 << 18;
+ host->linear_buf_size = bb_size;
+ host->linear_buf = devm_kzalloc(&pdev->dev, host->linear_buf_size,
+ GFP_KERNEL);
+
+ if (!host->linear_buf) {
+ dev_err(&pdev->dev, "devm_kzalloc failed\n");
+ return -ENOMEM;
+ }
+
+ host->pdev = pdev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Platform resource[0] is missing\n");
+ return -ENXIO;
+ }
+ base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+ host->base = (void __iomem *)base;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res) {
+ dev_err(&pdev->dev, "Platform resource[1] is missing\n");
+ return -EINVAL;
+ }
+ base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+ host->ndf_base = (void __iomem *)base;
+
+ /*
+ * Clear out any pending interrupts that may be left over from
+ * bootloader.
+ */
+ t = readq(host->base + OCT_MIO_EMM_INT);
+ writeq(t, host->base + OCT_MIO_EMM_INT);
+ if (cn78xx_style) {
+ /* Only CMD_DONE, DMA_DONE, CMD_ERR, DMA_ERR */
+ for (i = 1; i <= 4; i++) {
+ ret = devm_request_irq(&pdev->dev, mmc_irq[i],
+ octeon_mmc_interrupt,
+ 0, DRV_NAME, host);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Error: devm_request_irq %d\n",
+ mmc_irq[i]);
+ return ret;
+ }
+ }
+ } else {
+ ret = devm_request_irq(&pdev->dev, mmc_irq[0],
+ octeon_mmc_interrupt, 0, DRV_NAME, host);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Error: devm_request_irq %d\n",
+ mmc_irq[0]);
+ return ret;
+ }
+ }
+
+ host->global_pwr_gpiod = devm_gpiod_get_optional(&pdev->dev, "power",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(host->global_pwr_gpiod)) {
+ dev_err(&host->pdev->dev, "Invalid POWER GPIO\n");
+ return PTR_ERR(host->global_pwr_gpiod);
+ }
+
+ platform_set_drvdata(pdev, host);
+
+ for_each_child_of_node(node, cn) {
+ struct platform_device *slot_pdev;
+
+ slot_pdev = of_platform_device_create(cn, NULL, &pdev->dev);
+ ret = octeon_mmc_slot_probe(slot_pdev, host);
+ if (ret) {
+ dev_err(&host->pdev->dev, "Error populating slots\n");
+ gpiod_set_value_cansleep(host->global_pwr_gpiod, 0);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int octeon_mmc_remove(struct platform_device *pdev)
+{
+ union cvmx_mio_ndf_dma_cfg ndf_dma_cfg;
+ struct octeon_mmc_host *host = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < OCTEON_MAX_MMC; i++) {
+ if (host->slot[i])
+ octeon_mmc_slot_remove(host->slot[i]);
+ }
+
+ ndf_dma_cfg.u64 = readq(host->ndf_base + OCT_MIO_NDF_DMA_CFG);
+ ndf_dma_cfg.s.en = 0;
+ writeq(ndf_dma_cfg.u64, host->ndf_base + OCT_MIO_NDF_DMA_CFG);
+
+ gpiod_set_value_cansleep(host->global_pwr_gpiod, 0);
+
+ return 0;
+}
+
+static const struct of_device_id octeon_mmc_match[] = {
+ {
+ .compatible = "cavium,octeon-6130-mmc",
+ },
+ {
+ .compatible = "cavium,octeon-7890-mmc",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, octeon_mmc_match);
+
+static struct platform_driver octeon_mmc_driver = {
+ .probe = octeon_mmc_probe,
+ .remove = octeon_mmc_remove,
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = octeon_mmc_match,
+ },
+};
+
+static int __init octeon_mmc_init(void)
+{
+ return platform_driver_register(&octeon_mmc_driver);
+}
+
+static void __exit octeon_mmc_cleanup(void)
+{
+ platform_driver_unregister(&octeon_mmc_driver);
+}
+
+module_init(octeon_mmc_init);
+module_exit(octeon_mmc_cleanup);
+
+MODULE_AUTHOR("Cavium Inc. <support@cavium.com>");
+MODULE_DESCRIPTION("low-level driver for Cavium OCTEON MMC/SSD card");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-12 18:18 [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller Steven J. Hill
@ 2016-07-19 18:31 ` Steven J. Hill
2016-08-23 14:53 ` Ulf Hansson
1 sibling, 0 replies; 16+ messages in thread
From: Steven J. Hill @ 2016-07-19 18:31 UTC (permalink / raw)
To: linux-mmc, devicetree, linux-mips
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Ralf Baechle
On 07/12/2016 01:18 PM, Steven J. Hill wrote:
> The OCTEON MMC controller is currently found on cn61XX and cn71XX
> devices. Device parameters are configured from device tree data.
> eMMC, MMC and SD devices are supported. Tested on Rhino labs UTM8
> and Cavium CN7130 board.
>
> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
> Acked-by: David Daney <david.daney@cavium.com>
>
Has anyone reviewed the driver or have any comments? Thanks.
Steve
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
@ 2016-07-19 18:31 ` Steven J. Hill
0 siblings, 0 replies; 16+ messages in thread
From: Steven J. Hill @ 2016-07-19 18:31 UTC (permalink / raw)
To: linux-mmc, devicetree, linux-mips
Cc: Ulf Hansson, Rob Herring, Mark Rutland, Ralf Baechle
On 07/12/2016 01:18 PM, Steven J. Hill wrote:
> The OCTEON MMC controller is currently found on cn61XX and cn71XX
> devices. Device parameters are configured from device tree data.
> eMMC, MMC and SD devices are supported. Tested on Rhino labs UTM8
> and Cavium CN7130 board.
>
> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
> Acked-by: David Daney <david.daney@cavium.com>
>
Has anyone reviewed the driver or have any comments? Thanks.
Steve
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-19 18:31 ` Steven J. Hill
(?)
@ 2016-07-19 21:19 ` Ulf Hansson
2016-07-21 13:46 ` Ralf Baechle
2016-08-12 5:41 ` Steven J. Hill
-1 siblings, 2 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-07-19 21:19 UTC (permalink / raw)
To: Steven J. Hill
Cc: linux-mmc, devicetree, linux-mips, Rob Herring, Mark Rutland,
Ralf Baechle
On 19 July 2016 at 20:31, Steven J. Hill <Steven.Hill@caviumnetworks.com> wrote:
> On 07/12/2016 01:18 PM, Steven J. Hill wrote:
>>
>> The OCTEON MMC controller is currently found on cn61XX and cn71XX
>> devices. Device parameters are configured from device tree data.
>> eMMC, MMC and SD devices are supported. Tested on Rhino labs UTM8
>> and Cavium CN7130 board.
>>
>> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
>> Acked-by: David Daney <david.daney@cavium.com>
>>
> Has anyone reviewed the driver or have any comments? Thanks.
Sorry I need more time, been partly out of office lately.
I intend to review this prior rc1 is out, but no promises.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-19 21:19 ` Ulf Hansson
@ 2016-07-21 13:46 ` Ralf Baechle
2016-08-23 14:56 ` Ulf Hansson
2016-08-12 5:41 ` Steven J. Hill
1 sibling, 1 reply; 16+ messages in thread
From: Ralf Baechle @ 2016-07-21 13:46 UTC (permalink / raw)
To: Ulf Hansson
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland
On Tue, Jul 19, 2016 at 11:19:43PM +0200, Ulf Hansson wrote:
> > Has anyone reviewed the driver or have any comments? Thanks.
>
> Sorry I need more time, been partly out of office lately.
> I intend to review this prior rc1 is out, but no promises.
>
> Kind regards
> Uffe
Ulf,
if you decide to accept this patch, could you also push the bindings
patch ("[V8,1/2] mmc: OCTEON: Add DT bindings for OCTEON MMC controller.")
upstream? I think they should be merged together.
Thanks,
Ralf
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-19 21:19 ` Ulf Hansson
2016-07-21 13:46 ` Ralf Baechle
@ 2016-08-12 5:41 ` Steven J. Hill
2016-08-22 14:05 ` Ulf Hansson
1 sibling, 1 reply; 16+ messages in thread
From: Steven J. Hill @ 2016-08-12 5:41 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, David Daney
On 07/19/2016 04:19 PM, Ulf Hansson wrote:
>
> Sorry I need more time, been partly out of office lately.
> I intend to review this prior rc1 is out, but no promises.
>
Hello Ulf.
I was just checking to see if you had a chance to look at the MMC patch?
I just did a fresh pull from Linus' a few minutes ago and it still not
in the tree. How much longer do you think we will have to wait? Thanks.
Steve
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-08-12 5:41 ` Steven J. Hill
@ 2016-08-22 14:05 ` Ulf Hansson
0 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-08-22 14:05 UTC (permalink / raw)
To: Steven J. Hill; +Cc: linux-mmc, David Daney
On 12 August 2016 at 07:41, Steven J. Hill <Steven.Hill@cavium.com> wrote:
> On 07/19/2016 04:19 PM, Ulf Hansson wrote:
>>
>>
>> Sorry I need more time, been partly out of office lately.
>> I intend to review this prior rc1 is out, but no promises.
>>
> Hello Ulf.
>
> I was just checking to see if you had a chance to look at the MMC patch? I
> just did a fresh pull from Linus' a few minutes ago and it still not in the
> tree. How much longer do you think we will have to wait? Thanks.
Sorry for the delay. I will look at it during the week, just been
catching up on mmc but was delaying those reviews that requires some
more heavy efforts.
I will make sure this get its deserved priority, again apologize for the delay!
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-12 18:18 [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller Steven J. Hill
2016-07-19 18:31 ` Steven J. Hill
@ 2016-08-23 14:53 ` Ulf Hansson
2016-08-23 17:41 ` David Daney
1 sibling, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-08-23 14:53 UTC (permalink / raw)
To: Steven J. Hill
Cc: linux-mmc, devicetree, linux-mips, Rob Herring, Mark Rutland,
Ralf Baechle
On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
> The OCTEON MMC controller is currently found on cn61XX and cn71XX
> devices. Device parameters are configured from device tree data.
> eMMC, MMC and SD devices are supported. Tested on Rhino labs UTM8
> and Cavium CN7130 board.
>
> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
> Acked-by: David Daney <david.daney@cavium.com>
>
> ---
> v8:
> - Convert driver to use readq()/writeq() functions.
> - Work around legacy u-boot device trees.
> - Enable EMMC interrupts properly.
> - Support DDR signalling. The timings are tighter, so
> there may be failures with some FDT settings.
> - Quiesce the device by calling mmc_remove_host() and
> mmc_free_host() when unloading the driver.
> - Set MIO_BOOT_CTL on cn70xx to select proper mmc mode
> which is part of acquiring the bus.
> - Use of_property_read_u32() helper for cleaner device
> tree accesses.
> - Properly implement multi-block DMA. The Octeon's DMA
> enginer cannot do scatter-gather.
> - Add driver parameters to limit multi-block transfers.
> - Use octeon_bootbus_sem.
> - Improve GPIO support and make actual requests to use the
> GPIO lines before using and freeing them after.
>
> v7:
> - Revert to legacy device tree bindings handled in driver
> - Tone down the warning printed when legacy bindings in use
>
> v6:
> - Split up patch
> - Moved device tree fixup to platform code
>
> Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com>
> ---
> drivers/mmc/host/Kconfig | 10 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/octeon_mmc.c | 1331 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1342 insertions(+)
> create mode 100644 drivers/mmc/host/octeon_mmc.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 0aa484c..7ef7f8b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -332,6 +332,16 @@ config MMC_SDHCI_IPROC
>
> If unsure, say N.
>
> +config MMC_OCTEON
> + tristate "Cavium OCTEON Multimedia Card Interface support"
> + depends on CAVIUM_OCTEON_SOC
> + help
> + This selects Cavium OCTEON Multimedia card Interface.
> + If you have an OCTEON board with a Multimedia Card slot,
> + say Y or M here.
> +
> + If unsure, say N.
> +
> config MMC_MOXART
> tristate "MOXART SD/MMC Host Controller support"
> depends on ARCH_MOXART && MMC
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index af918d2..c43bd7d 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_MMC_SDHCI_SPEAR) += sdhci-spear.o
> obj-$(CONFIG_MMC_WBSD) += wbsd.o
> obj-$(CONFIG_MMC_AU1X) += au1xmmc.o
> obj-$(CONFIG_MMC_MTK) += mtk-sd.o
> +obj-$(CONFIG_MMC_OCTEON) += octeon_mmc.o
> obj-$(CONFIG_MMC_OMAP) += omap.o
> obj-$(CONFIG_MMC_OMAP_HS) += omap_hsmmc.o
> obj-$(CONFIG_MMC_ATMELMCI) += atmel-mci.o
> diff --git a/drivers/mmc/host/octeon_mmc.c b/drivers/mmc/host/octeon_mmc.c
> new file mode 100644
> index 0000000..d2ef434
> --- /dev/null
> +++ b/drivers/mmc/host/octeon_mmc.c
> @@ -0,0 +1,1331 @@
> +/*
> + * Driver for MMC and SSD cards for Cavium OCTEON SOCs.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2012-2016 Cavium Inc.
> + */
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/scatterlist.h>
> +#include <linux/interrupt.h>
> +#include <linux/blkdev.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/slot-gpio.h>
> +#include <net/irda/parameters.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <asm/byteorder.h>
> +#include <asm/octeon/octeon.h>
We shouldn't include SoC specific headers in a generic mmc driver.
If there are dependencies to perform SoC specific operations, then you
should use platform callbacks. Or better, if those operations can be
made through generic interfaces.
> +
> +#define DRV_NAME "octeon_mmc"
> +
> +#define OCTEON_MAX_MMC 4
> +
> +#define OCT_MIO_NDF_DMA_CFG 0x00
> +#define OCT_MIO_EMM_DMA_ADR 0x08
> +
> +#define OCT_MIO_EMM_CFG 0x00
> +#define OCT_MIO_EMM_SWITCH 0x48
> +#define OCT_MIO_EMM_DMA 0x50
> +#define OCT_MIO_EMM_CMD 0x58
> +#define OCT_MIO_EMM_RSP_STS 0x60
> +#define OCT_MIO_EMM_RSP_LO 0x68
> +#define OCT_MIO_EMM_RSP_HI 0x70
> +#define OCT_MIO_EMM_INT 0x78
> +#define OCT_MIO_EMM_INT_EN 0x80
> +#define OCT_MIO_EMM_WDOG 0x88
> +#define OCT_MIO_EMM_SAMPLE 0x90
> +#define OCT_MIO_EMM_STS_MASK 0x98
> +#define OCT_MIO_EMM_RCA 0xa0
> +#define OCT_MIO_EMM_BUF_IDX 0xe0
> +#define OCT_MIO_EMM_BUF_DAT 0xe8
> +
> +#define CVMX_MIO_BOOT_CTL CVMX_ADD_IO_SEG(0x00011800000000D0ull)
> +
> +struct octeon_mmc_host {
> + void __iomem *base;
> + void __iomem *ndf_base;
> + u64 emm_cfg;
> + u64 n_minus_one; /* OCTEON II workaround location */
> + int last_slot;
> +
> + struct semaphore mmc_serializer;
> + struct mmc_request *current_req;
> + unsigned int linear_buf_size;
> + void *linear_buf;
> + struct sg_mapping_iter smi;
> + int sg_idx;
> + bool dma_active;
> +
> + struct platform_device *pdev;
> + struct gpio_desc *global_pwr_gpiod;
> + bool dma_err_pending;
> + bool need_bootbus_lock;
> + bool big_dma_addr;
> + bool need_irq_handler_lock;
> + spinlock_t irq_handler_lock;
> + bool has_ciu3;
> +
> + struct octeon_mmc_slot *slot[OCTEON_MAX_MMC];
> +};
> +
> +struct octeon_mmc_slot {
> + struct mmc_host *mmc; /* slot-level mmc_core object */
> + struct octeon_mmc_host *host; /* common hw for all 4 slots */
> +
> + unsigned int clock;
> + unsigned int sclock;
> +
> + u64 cached_switch;
> + u64 cached_rca;
> +
> + unsigned int cmd_cnt; /* sample delay */
> + unsigned int dat_cnt; /* sample delay */
> +
> + int bus_width;
> + int bus_id;
> +
> + /* Legacy property - in future mmc->supply.vmmc should be used */
> + struct gpio_desc *pwr_gpiod;
> +};
> +
> +static int bb_size = 1 << 18;
> +module_param(bb_size, int, S_IRUGO);
> +MODULE_PARM_DESC(bb_size,
> + "Size of DMA linearizing buffer (max transfer size).");
> +
> +static int ddr = 2;
> +module_param(ddr, int, S_IRUGO);
> +MODULE_PARM_DESC(ddr,
> + "enable DoubleDataRate clocking: 0=no, 1=always, 2=at spi-max-frequency/2");
The module params here seem like a leftover from a debugging exercise.
Please remove them.
> +
> +static void octeon_mmc_acquire_bus(struct octeon_mmc_host *host)
> +{
> + if (host->need_bootbus_lock) {
> + down(&octeon_bootbus_sem);
> + /* On cn70XX switch the mmc unit onto the bus. */
> + if (OCTEON_IS_MODEL(OCTEON_CN70XX))
According to my upper comment about SoC specific code, please move
this code out of the driver.
You may use a DT compatible string to perform specific operations for
some versions of the IP/SoC, although not to call SoC specific code
directly.
This comment applies to other places for $subject patch as well,
although I am not going to comment on each of them. I leave that to
you to figure out.
> + writeq(0, (void __iomem *)CVMX_MIO_BOOT_CTL);
> + } else {
> + down(&host->mmc_serializer);
> + }
> +}
> +
[...]
> +
> +/*
> + * The functions below are used for the EMMC-17978 workaround.
> + *
> + * Due to an imperfection in the design of the MMC bus hardware,
> + * the 2nd to last cache block of a DMA read must be locked into the L2 Cache.
> + * Otherwise, data corruption may occur.
> + */
Isn't these operations also depending on the SoC/Arch? If so, perhaps
you need a set of platform callbacks even for these.
> +
> +static inline void *phys_to_ptr(u64 address)
> +{
> + return (void *)(address | (1ull<<63)); /* XKPHYS */
> +}
> +
> +/**
> + * Lock a single line into L2. The line is zeroed before locking
> + * to make sure no dram accesses are made.
> + *
> + * @addr Physical address to lock
> + */
> +static void l2c_lock_line(u64 addr)
> +{
> + char *addr_ptr = phys_to_ptr(addr);
> + asm volatile (
> + "cache 31, %[line]" /* Unlock the line */
> + :: [line] "m" (*addr_ptr));
> +}
> +
> +/**
> + * Locks a memory region in the L2 cache
> + *
> + * @start - start address to begin locking
> + * @len - length in bytes to lock
> + */
> +static void l2c_lock_mem_region(u64 start, u64 len)
> +{
> + u64 end;
> +
> + /* Round start/end to cache line boundaries */
> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
> +
> + while (start <= end) {
> + l2c_lock_line(start);
> + start += CVMX_CACHE_LINE_SIZE;
> + }
> + asm volatile("sync");
> +}
> +
> +/**
> + * Unlock a single line in the L2 cache.
> + *
> + * @addr Physical address to unlock
> + *
> + * Return Zero on success
> + */
> +static void l2c_unlock_line(u64 addr)
> +{
> + char *addr_ptr = phys_to_ptr(addr);
> + asm volatile (
> + "cache 23, %[line]" /* Unlock the line */
> + :: [line] "m" (*addr_ptr));
> +}
> +
> +/**
> + * Unlock a memory region in the L2 cache
> + *
> + * @start - start address to unlock
> + * @len - length to unlock in bytes
> + */
> +static void l2c_unlock_mem_region(u64 start, u64 len)
> +{
> + u64 end;
> +
> + /* Round start/end to cache line boundaries */
> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
> +
> + while (start <= end) {
> + l2c_unlock_line(start);
> + start += CVMX_CACHE_LINE_SIZE;
> + }
> +}
> +
[...]
> +
> +static void octeon_mmc_dma_request(struct mmc_host *mmc,
> + struct mmc_request *mrq)
> +{
> + struct octeon_mmc_slot *slot;
> + struct octeon_mmc_host *host;
> + struct mmc_command *cmd;
> + struct mmc_data *data;
> + union cvmx_mio_emm_int emm_int;
> + union cvmx_mio_emm_dma emm_dma;
> + union cvmx_mio_ndf_dma_cfg dma_cfg;
> +
> + cmd = mrq->cmd;
> + if (mrq->data == NULL || mrq->data->sg == NULL || !mrq->data->sg_len ||
> + mrq->stop == NULL || mrq->stop->opcode != MMC_STOP_TRANSMISSION) {
> + dev_err(&mmc->card->dev,
> + "Error: octeon_mmc_dma_request no data\n");
> + cmd->error = -EINVAL;
> + if (mrq->done)
> + mrq->done(mrq);
> + return;
> + }
> +
> + slot = mmc_priv(mmc);
> + host = slot->host;
> +
> + /* Only a single user of the bootbus at a time. */
> + octeon_mmc_acquire_bus(host);
> +
> + octeon_mmc_switch_to(slot);
> +
> + data = mrq->data;
> +
> + if (data->timeout_ns)
> + writeq(octeon_mmc_timeout_to_wdog(slot, data->timeout_ns),
> + host->base + OCT_MIO_EMM_WDOG);
> +
> + WARN_ON(host->current_req);
> + host->current_req = mrq;
> +
> + host->sg_idx = 0;
> +
> + WARN_ON(data->blksz * data->blocks > host->linear_buf_size);
> +
> + if ((data->flags & MMC_DATA_WRITE) && data->sg_len > 1) {
> + size_t r = sg_copy_to_buffer(data->sg, data->sg_len,
> + host->linear_buf, data->blksz * data->blocks);
> + WARN_ON(data->blksz * data->blocks != r);
> + }
> +
> + dma_cfg.u64 = 0;
> + dma_cfg.s.en = 1;
> + dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
> +#ifdef __LITTLE_ENDIAN
> + dma_cfg.s.endian = 1;
> +#endif
> + dma_cfg.s.size = ((data->blksz * data->blocks) / 8) - 1;
> + if (!host->big_dma_addr) {
> + if (data->sg_len > 1)
> + dma_cfg.s.adr = virt_to_phys(host->linear_buf);
> + else
> + dma_cfg.s.adr = sg_phys(data->sg);
> + }
> + writeq(dma_cfg.u64, host->ndf_base + OCT_MIO_NDF_DMA_CFG);
> + if (host->big_dma_addr) {
> + u64 addr;
> +
> + if (data->sg_len > 1)
> + addr = virt_to_phys(host->linear_buf);
> + else
> + addr = sg_phys(data->sg);
> + writeq(addr, host->ndf_base + OCT_MIO_EMM_DMA_ADR);
> + }
> +
> + emm_dma.u64 = 0;
> + emm_dma.s.bus_id = slot->bus_id;
> + emm_dma.s.dma_val = 1;
> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
> + if (mmc_card_mmc(mmc->card) ||
> + (mmc_card_sd(mmc->card) &&
> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
> + emm_dma.s.multi = 1;
Could you elaborate on exactly what you are doing here. I don't
understand why you need to check whether the card supports CMD23.
Moreover you must not access mmc->card unless you make sure there is a
valid pointer for it.
> + emm_dma.s.block_cnt = data->blocks;
> + emm_dma.s.card_addr = cmd->arg;
> +
> + emm_int.u64 = 0;
> + emm_int.s.dma_done = 1;
> + emm_int.s.cmd_err = 1;
> + emm_int.s.dma_err = 1;
> + /* Clear the bit. */
> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
> + if (!host->has_ciu3)
> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT_EN);
> + host->dma_active = true;
> +
> + if ((OCTEON_IS_MODEL(OCTEON_CN6XXX) ||
> + OCTEON_IS_MODEL(OCTEON_CNF7XXX)) &&
> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK &&
> + (data->blksz * data->blocks) > 1024) {
> + host->n_minus_one = dma_cfg.s.adr +
> + (data->blksz * data->blocks) - 1024;
> + l2c_lock_mem_region(host->n_minus_one, 512);
> + }
> +
> + if (mmc->card && mmc_card_sd(mmc->card))
> + writeq(0x00b00000ull, host->base + OCT_MIO_EMM_STS_MASK);
> + else
> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
Some explanation of what goes on here would be nice. You are writing
some magic mask depending on whether it SD or MMC.
Does that also mean this controller don't support SDIO? If so, you may
enable MMC_CAP2_NO_SDIO.
> + writeq(emm_dma.u64, host->base + OCT_MIO_EMM_DMA);
> +}
> +
> +static void octeon_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> + struct octeon_mmc_slot *slot;
> + struct octeon_mmc_host *host;
> + struct mmc_command *cmd;
> + union cvmx_mio_emm_int emm_int;
> + union cvmx_mio_emm_cmd emm_cmd;
> + struct octeon_mmc_cr_mods mods;
> +
> + cmd = mrq->cmd;
> +
> + if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK ||
> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) {
Instead of checking the opcode, perhaps you should check the number
blocks/bytes that is about to be transfers.
Or is there a particular reason to why multiblock transfers are required?
> + octeon_mmc_dma_request(mmc, mrq);
> + return;
> + }
> +
> + mods = octeon_mmc_get_cr_mods(cmd);
> +
> + slot = mmc_priv(mmc);
> + host = slot->host;
[...]
> +
> +static void octeon_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct octeon_mmc_slot *slot;
> + struct octeon_mmc_host *host;
> + int bus_width;
> + int clock;
> + bool ddr_clock;
> + int hs_timing;
> + int power_class = 10;
> + int clk_period;
> + int timeout = 2000;
> + union cvmx_mio_emm_switch emm_switch;
> + union cvmx_mio_emm_rsp_sts emm_sts;
> +
> + slot = mmc_priv(mmc);
> + host = slot->host;
> +
> + /* Only a single user of the bootbus at a time. */
> + octeon_mmc_acquire_bus(host);
> +
> + octeon_mmc_switch_to(slot);
> +
> + /*
> + * Reset the chip on each power off
> + */
> + if (ios->power_mode == MMC_POWER_OFF) {
> + octeon_mmc_reset_bus(slot);
> + if (!IS_ERR(mmc->supply.vmmc))
> + regulator_disable(mmc->supply.vmmc);
You shouldn't access this regulator directly, as it's controlled by
the mmc core. Instead use mmc_regulator_set_ocr()
> + else /* Legacy power GPIO */
> + gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
> + } else {
> + if (!IS_ERR(mmc->supply.vmmc))
> + regulator_enable(mmc->supply.vmmc);
Similar comment as above. Use mmc_regulator_set_ocr().
> + else /* Legacy power GPIO */
> + gpiod_set_value_cansleep(slot->pwr_gpiod, 1);
> + }
> +
> + switch (ios->bus_width) {
> + case MMC_BUS_WIDTH_8:
> + bus_width = 2;
> + break;
> + case MMC_BUS_WIDTH_4:
> + bus_width = 1;
> + break;
> + case MMC_BUS_WIDTH_1:
> + bus_width = 0;
> + break;
> + default:
> + bus_width = 0;
> + break;
> + }
[...]
> +static const struct mmc_host_ops octeon_mmc_ops = {
> + .request = octeon_mmc_request,
> + .set_ios = octeon_mmc_set_ios,
> + .get_ro = mmc_gpio_get_ro,
> + .get_cd = mmc_gpio_get_cd,
> +};
> +
> +static void octeon_mmc_set_clock(struct octeon_mmc_slot *slot,
> + unsigned int clock)
> +{
> + struct mmc_host *mmc = slot->mmc;
> +
> + clock = min(clock, mmc->f_max);
> + clock = max(clock, mmc->f_min);
> + slot->clock = clock;
> +}
> +
> +static int octeon_mmc_initlowlevel(struct octeon_mmc_slot *slot,
> + int bus_width)
> +{
> + union cvmx_mio_emm_switch emm_switch;
> + struct octeon_mmc_host *host = slot->host;
> +
> + host->emm_cfg |= 1ull << slot->bus_id;
> + writeq(host->emm_cfg, slot->host->base + OCT_MIO_EMM_CFG);
> + octeon_mmc_set_clock(slot, 400000);
> +
> + /* Program initial clock speed and power */
> + emm_switch.u64 = 0;
> + emm_switch.s.power_class = 10;
> + emm_switch.s.clk_hi = (slot->sclock / slot->clock) / 2;
> + emm_switch.s.clk_lo = (slot->sclock / slot->clock) / 2;
> +
> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
> + emm_switch.s.bus_id = slot->bus_id;
> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
> + slot->cached_switch = emm_switch.u64;
> +
> + writeq(((u64)slot->clock * 850ull) / 1000ull,
> + host->base + OCT_MIO_EMM_WDOG);
> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
> + writeq(1, host->base + OCT_MIO_EMM_RCA);
Perhaps some comments explaining what goes on.
> + return 0;
> +}
> +
> +static int octeon_mmc_slot_probe(struct platform_device *slot_pdev,
> + struct octeon_mmc_host *host)
> +{
> + struct mmc_host *mmc;
> + struct octeon_mmc_slot *slot;
> + struct device *dev = &slot_pdev->dev;
> + struct device_node *node = slot_pdev->dev.of_node;
> + u32 id, bus_width, max_freq, cmd_skew, dat_skew;
> + u64 clock_period;
> + int ret;
> +
> + ret = of_property_read_u32(node, "reg", &id);
> + if (ret) {
> + dev_err(dev, "Missing or invalid reg property on %s\n",
> + of_node_full_name(node));
> + return ret;
> + }
> +
> + if (id >= OCTEON_MAX_MMC || host->slot[id]) {
> + dev_err(dev, "Invalid reg property on %s\n",
> + of_node_full_name(node));
> + return -EINVAL;
> + }
> +
> + mmc = mmc_alloc_host(sizeof(struct octeon_mmc_slot), dev);
> + if (!mmc) {
> + dev_err(dev, "alloc host failed\n");
> + return -ENOMEM;
> + }
> +
> + slot = mmc_priv(mmc);
> + slot->mmc = mmc;
> + slot->host = host;
> +
> + /*
> + * The "cavium,bus-max-width" property is DEPRECATED and should
> + * not be used. We handle it here to support older firmware.
> + * Going forward, the standard "bus-width" property is used
> + * instead of the Cavium-specific property.
> + */
I think you should call mmc_of_parse() as it will parse for the common
mmc DT properties.
After that, you should parse for the deprecated mmc cavium DT
properties and when such is found, update the corresponding values for
bus width and max freq.
Perhaps you also need a default value for max freq, so you need to
check that as the final thing.
> + ret = of_property_read_u32(node, "bus-width", &bus_width);
> + if (ret) {
> + /* Try legacy "cavium,bus-max-width" property. */
> + ret = of_property_read_u32(node, "cavium,bus-max-width",
> + &bus_width);
> + if (ret) {
> + /* No bus width specified, use default. */
> + bus_width = 8;
> + dev_info(dev, "Default bus width %u used for slot %u\n",
> + bus_width, id);
> + }
> + }
> +
> +
> + switch (bus_width) {
> + case 1:
> + case 4:
> + case 8:
> + break;
> + default:
> + dev_err(dev, "Invalid bus width for slot %u\n", id);
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + /*
> + * The "spi-max-frequency" property is DEPRECATED and should
> + * not be used. We handle it here to support older firmware.
> + * Going forward, the standard "max-frequency" property is
> + * used instead.
> + */
> + ret = of_property_read_u32(node, "max-frequency", &max_freq);
> + if (ret) {
> + /* Try legacy "spi-max-frequency" property. */
> + ret = of_property_read_u32(node, "spi-max-frequency",
> + &max_freq);
> + if (ret) {
> + /* No frequency properties found, use default. */
> + max_freq = 52000000;
> + dev_info(dev, "Default %u frequency used for slot %u\n",
> + id, max_freq);
> + }
> + }
> +
> + /* Get regulators and the supported OCR mask */
> + ret = mmc_regulator_get_supply(mmc);
> + if (ret == -EPROBE_DEFER)
> + goto err;
> +
> + /* Alternatively a GPIO may be specified to control slot power */
> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
No, I am not happy with this as we discussed earlier. You need to
parse the DTB from SoC specifc code and manage the power GPIO from
there.
Ideally you should register the power GPIO as a GPIO regulator for the
cavium mmc slot device.
In that way you will get the calculated OCR mask just by calling
mmc_regulator_get_supply() and you don't need to use the GPIO API to
deal with powers.
> +
> + /* Octeon specific DT properties */
> + ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
> + if (ret)
> + cmd_skew = 0;
> +
> + ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
> + if (ret)
> + dat_skew = 0;
> +
> + /*
> + * Set up host parameters.
> + */
> + mmc->ops = &octeon_mmc_ops;
> + mmc->f_min = 400000;
> + mmc->f_max = max_freq;
> + mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> + MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA |
The MMC_CAP_8_BIT_DATA and MMC_CAP_4_BIT_DATA is supposed to be
assigned depending on the configuration in DTS, not hardcoded like
this.
There's actually also DT bindings parses by mmc_of_parse() for
MMC_CAP_MMC_HIGHSPEED and MMC_CAP_SD_HIGHSPEED, although if you know
that your HW always supports these modes, it's fine to enabled them
like this.
> + MMC_CAP_ERASE;
To use MMC_CAP_ERASE, it's recomended to notify the mmc core about
your used request busy timeout.
So please assign the mmc->max_busy_timeout to its correct value.
> + mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
> + MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
> + MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
This you need to explain. I thought your power GPIO only supported on
and off. In other words it's a fixed regulator, no?
Anyway, when you converted to GPIO regulators you will get this mask
assigned by calling mmc_regulator_get_supply(), so you don't need any
of this at all.
> +
> + /* post-sdk23 caps */
> + mmc->caps |=
> + ((mmc->f_max >= 12000000) * MMC_CAP_UHS_SDR12) |
> + ((mmc->f_max >= 25000000) * MMC_CAP_UHS_SDR25) |
> + ((mmc->f_max >= 50000000) * MMC_CAP_UHS_SDR50) |
> + MMC_CAP_CMD23;
Supporting UHS mode requires you to be able to switch the I/O voltage
level from ~3.3 V to 1.8 V.
How do you manage that without implementing the following host ops?
->start_signal_voltage_switch()
->card_busy()
Also note that we have common mmc DT bindings for MMC_CAP_UHS*, which
is parsed via mmc_of_parse().
> +
> + if ((!IS_ERR(mmc->supply.vmmc)) || (slot->pwr_gpiod))
> + mmc->caps |= MMC_CAP_POWER_OFF_CARD;
This cap is used only for SDIO scenarios. I don't think you need it!?
> +
> + /* "1.8v" capability is actually 1.8-or-3.3v */
Yes, there is a lacking capablity for eMMC 3.3 V, although I don't
think you need to care here...
> + if (ddr)
> + mmc->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR;
... I assume it's safe to enable MMC_CAP_1_8V_DDR as that applies to
eMMCs. Or you HW only supports 3.3V I/O voltage for eMMCs?
Although, because of the upper comment about UHS modes, you should
probably not enable MMC_CAP_UHS_DDR50 at all.
> +
> + mmc->max_segs = 64;
> + mmc->max_seg_size = host->linear_buf_size;
> + mmc->max_req_size = host->linear_buf_size;
> + mmc->max_blk_size = 512;
> + mmc->max_blk_count = mmc->max_req_size / 512;
> +
> + slot->clock = mmc->f_min;
> + slot->sclock = octeon_get_io_clock_rate();
> +
> + clock_period = 1000000000000ull / slot->sclock; /* period in pS */
> + slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period;
> + slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period;
> +
> + slot->bus_width = bus_width;
> + slot->bus_id = id;
> + slot->cached_rca = 1;
> +
> + /* Only a single user of the bootbus at a time. */
> + octeon_mmc_acquire_bus(host);
> + host->slot[id] = slot;
> +
> + octeon_mmc_switch_to(slot);
> + /* Initialize MMC Block. */
> + octeon_mmc_initlowlevel(slot, bus_width);
> +
> + octeon_mmc_release_bus(host);
> +
> + ret = mmc_add_host(mmc);
> + if (ret) {
> + dev_err(dev, "mmc_add_host() returned %d\n", ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + slot->host->slot[id] = NULL;
> +
> + gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
> +
> + mmc_free_host(slot->mmc);
> + return ret;
> +}
> +
> +static int octeon_mmc_slot_remove(struct octeon_mmc_slot *slot)
> +{
> + mmc_remove_host(slot->mmc);
> +
> + slot->host->slot[slot->bus_id] = NULL;
> +
> + gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
> +
> + mmc_free_host(slot->mmc);
> +
> + return 0;
> +}
> +
> +static int octeon_mmc_probe(struct platform_device *pdev)
> +{
> + struct octeon_mmc_host *host;
> + struct resource *res;
> + void __iomem *base;
> + int mmc_irq[9];
> + int i;
> + int ret = 0;
> + struct device_node *node = pdev->dev.of_node;
> + struct device_node *cn;
> + bool cn78xx_style;
> + u64 t;
> +
> + host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
> + if (!host) {
> + dev_err(&pdev->dev, "devm_kzalloc failed\n");
> + return -ENOMEM;
> + }
> +
> + spin_lock_init(&host->irq_handler_lock);
> + sema_init(&host->mmc_serializer, 1);
> +
> + cn78xx_style = of_device_is_compatible(node, "cavium,octeon-7890-mmc");
> + if (cn78xx_style) {
> + host->need_bootbus_lock = false;
> + host->big_dma_addr = true;
> + host->need_irq_handler_lock = true;
> + host->has_ciu3 = true;
> + /*
> + * First seven are the EMM_INT bits 0..6, then two for
> + * the EMM_DMA_INT bits
> + */
> + for (i = 0; i < 9; i++) {
> + mmc_irq[i] = platform_get_irq(pdev, i);
> + if (mmc_irq[i] < 0)
> + return mmc_irq[i];
> +
> + /* work around legacy u-boot device trees */
> + irq_set_irq_type(mmc_irq[i], IRQ_TYPE_EDGE_RISING);
> + }
> + } else {
> + host->need_bootbus_lock = true;
> + host->big_dma_addr = false;
> + host->need_irq_handler_lock = false;
> + host->has_ciu3 = false;
> + /* First one is EMM second NDF_DMA */
> + for (i = 0; i < 2; i++) {
> + mmc_irq[i] = platform_get_irq(pdev, i);
> + if (mmc_irq[i] < 0)
> + return mmc_irq[i];
> + }
> + }
> + host->last_slot = -1;
> +
> + if (bb_size < 512 || bb_size >= (1 << 24))
> + bb_size = 1 << 18;
> + host->linear_buf_size = bb_size;
> + host->linear_buf = devm_kzalloc(&pdev->dev, host->linear_buf_size,
> + GFP_KERNEL);
> +
> + if (!host->linear_buf) {
> + dev_err(&pdev->dev, "devm_kzalloc failed\n");
> + return -ENOMEM;
> + }
> +
> + host->pdev = pdev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Platform resource[0] is missing\n");
> + return -ENXIO;
> + }
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> + host->base = (void __iomem *)base;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res) {
> + dev_err(&pdev->dev, "Platform resource[1] is missing\n");
> + return -EINVAL;
> + }
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> + host->ndf_base = (void __iomem *)base;
> +
> + /*
> + * Clear out any pending interrupts that may be left over from
> + * bootloader.
> + */
> + t = readq(host->base + OCT_MIO_EMM_INT);
> + writeq(t, host->base + OCT_MIO_EMM_INT);
> + if (cn78xx_style) {
> + /* Only CMD_DONE, DMA_DONE, CMD_ERR, DMA_ERR */
> + for (i = 1; i <= 4; i++) {
> + ret = devm_request_irq(&pdev->dev, mmc_irq[i],
> + octeon_mmc_interrupt,
> + 0, DRV_NAME, host);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Error: devm_request_irq %d\n",
> + mmc_irq[i]);
> + return ret;
> + }
> + }
> + } else {
> + ret = devm_request_irq(&pdev->dev, mmc_irq[0],
> + octeon_mmc_interrupt, 0, DRV_NAME, host);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Error: devm_request_irq %d\n",
> + mmc_irq[0]);
> + return ret;
> + }
> + }
> +
> + host->global_pwr_gpiod = devm_gpiod_get_optional(&pdev->dev, "power",
> + GPIOD_OUT_HIGH);
Again, please don't use power GPIOS in the driver. Ideally you should
register the power GPIO as a GPIO regulator for the cavium mmc device.
Although I wonder what this "global" power GPIO actually is powering?
Perhaps it is just needed to allow the child node slots to power the
cards? If so, it should be registered as a supply (parent) for those
GPIO regulators instead and then you don't need to deal with it at all
in the driver...
> + if (IS_ERR(host->global_pwr_gpiod)) {
> + dev_err(&host->pdev->dev, "Invalid POWER GPIO\n");
> + return PTR_ERR(host->global_pwr_gpiod);
> + }
> +
> + platform_set_drvdata(pdev, host);
> +
> + for_each_child_of_node(node, cn) {
> + struct platform_device *slot_pdev;
> +
> + slot_pdev = of_platform_device_create(cn, NULL, &pdev->dev);
> + ret = octeon_mmc_slot_probe(slot_pdev, host);
> + if (ret) {
> + dev_err(&host->pdev->dev, "Error populating slots\n");
> + gpiod_set_value_cansleep(host->global_pwr_gpiod, 0);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-07-21 13:46 ` Ralf Baechle
@ 2016-08-23 14:56 ` Ulf Hansson
0 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-08-23 14:56 UTC (permalink / raw)
To: Ralf Baechle
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland
On 21 July 2016 at 15:46, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Tue, Jul 19, 2016 at 11:19:43PM +0200, Ulf Hansson wrote:
>
>> > Has anyone reviewed the driver or have any comments? Thanks.
>>
>> Sorry I need more time, been partly out of office lately.
>> I intend to review this prior rc1 is out, but no promises.
>>
>> Kind regards
>> Uffe
>
> Ulf,
>
> if you decide to accept this patch, could you also push the bindings
> patch ("[V8,1/2] mmc: OCTEON: Add DT bindings for OCTEON MMC controller.")
> upstream? I think they should be merged together.
Ralf, sorry for the delay.
No worries, I will pick the bindings as once I am happy with the
driver (patch 2/2).
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-08-23 14:53 ` Ulf Hansson
@ 2016-08-23 17:41 ` David Daney
0 siblings, 0 replies; 16+ messages in thread
From: David Daney @ 2016-08-23 17:41 UTC (permalink / raw)
To: Ulf Hansson
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 08/23/2016 07:53 AM, Ulf Hansson wrote:
> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
[...]
>> +#include <asm/byteorder.h>
>> +#include <asm/octeon/octeon.h>
>
OK, we will duplicate any needed definitions from octeon.h into the
driver source file.
> We shouldn't include SoC specific headers in a generic mmc driver.
It is not really a generic MMC driver. It is a driver for an MMC host
found only on two families of Cavium SoCs.
>
> If there are dependencies to perform SoC specific operations, then you
> should use platform callbacks. Or better, if those operations can be
> made through generic interfaces.
>
This means that code for this device will be spread across two files.
One for things deemed SoC specific, somewhere in arch/mips, everything
else (which is really SoS specific too, but deemed to be generic) in
this file.
>> +
>> +#define DRV_NAME "octeon_mmc"
>> +
>> +#define OCTEON_MAX_MMC 4
>> +
>> +#define OCT_MIO_NDF_DMA_CFG 0x00
>> +#define OCT_MIO_EMM_DMA_ADR 0x08
>> +
>> +#define OCT_MIO_EMM_CFG 0x00
>> +#define OCT_MIO_EMM_SWITCH 0x48
>> +#define OCT_MIO_EMM_DMA 0x50
>> +#define OCT_MIO_EMM_CMD 0x58
>> +#define OCT_MIO_EMM_RSP_STS 0x60
>> +#define OCT_MIO_EMM_RSP_LO 0x68
>> +#define OCT_MIO_EMM_RSP_HI 0x70
>> +#define OCT_MIO_EMM_INT 0x78
>> +#define OCT_MIO_EMM_INT_EN 0x80
>> +#define OCT_MIO_EMM_WDOG 0x88
>> +#define OCT_MIO_EMM_SAMPLE 0x90
>> +#define OCT_MIO_EMM_STS_MASK 0x98
>> +#define OCT_MIO_EMM_RCA 0xa0
>> +#define OCT_MIO_EMM_BUF_IDX 0xe0
>> +#define OCT_MIO_EMM_BUF_DAT 0xe8
>> +
>> +#define CVMX_MIO_BOOT_CTL CVMX_ADD_IO_SEG(0x00011800000000D0ull)
>> +
>> +struct octeon_mmc_host {
>> + void __iomem *base;
>> + void __iomem *ndf_base;
>> + u64 emm_cfg;
>> + u64 n_minus_one; /* OCTEON II workaround location */
>> + int last_slot;
>> +
>> + struct semaphore mmc_serializer;
>> + struct mmc_request *current_req;
>> + unsigned int linear_buf_size;
>> + void *linear_buf;
>> + struct sg_mapping_iter smi;
>> + int sg_idx;
>> + bool dma_active;
>> +
>> + struct platform_device *pdev;
>> + struct gpio_desc *global_pwr_gpiod;
>> + bool dma_err_pending;
>> + bool need_bootbus_lock;
>> + bool big_dma_addr;
>> + bool need_irq_handler_lock;
>> + spinlock_t irq_handler_lock;
>> + bool has_ciu3;
>> +
>> + struct octeon_mmc_slot *slot[OCTEON_MAX_MMC];
>> +};
>> +
>> +struct octeon_mmc_slot {
>> + struct mmc_host *mmc; /* slot-level mmc_core object */
>> + struct octeon_mmc_host *host; /* common hw for all 4 slots */
>> +
>> + unsigned int clock;
>> + unsigned int sclock;
>> +
>> + u64 cached_switch;
>> + u64 cached_rca;
>> +
>> + unsigned int cmd_cnt; /* sample delay */
>> + unsigned int dat_cnt; /* sample delay */
>> +
>> + int bus_width;
>> + int bus_id;
>> +
>> + /* Legacy property - in future mmc->supply.vmmc should be used */
>> + struct gpio_desc *pwr_gpiod;
>> +};
>> +
>> +static int bb_size = 1 << 18;
>> +module_param(bb_size, int, S_IRUGO);
>> +MODULE_PARM_DESC(bb_size,
>> + "Size of DMA linearizing buffer (max transfer size).");
>> +
bb_size, is a performance tuning parameter. We can just hard code it to
some size and not allow it to be changed.
>> +static int ddr = 2;
>> +module_param(ddr, int, S_IRUGO);
>> +MODULE_PARM_DESC(ddr,
>> + "enable DoubleDataRate clocking: 0=no, 1=always, 2=at spi-max-frequency/2");
>
> The module params here seem like a leftover from a debugging exercise.
> Please remove them.
Yes, this "ddr" thing must be removed.
>
>> +
>> +static void octeon_mmc_acquire_bus(struct octeon_mmc_host *host)
>> +{
>> + if (host->need_bootbus_lock) {
>> + down(&octeon_bootbus_sem);
>> + /* On cn70XX switch the mmc unit onto the bus. */
>> + if (OCTEON_IS_MODEL(OCTEON_CN70XX))
>
> According to my upper comment about SoC specific code, please move
> this code out of the driver.
>
> You may use a DT compatible string to perform specific operations for
> some versions of the IP/SoC, although not to call SoC specific code
> directly.
>
> This comment applies to other places for $subject patch as well,
> although I am not going to comment on each of them. I leave that to
> you to figure out.
>
>> + writeq(0, (void __iomem *)CVMX_MIO_BOOT_CTL);
>> + } else {
>> + down(&host->mmc_serializer);
>> + }
>> +}
>> +
>
I guess we will put it in arch/mips/cavium-octeon/octeon-mmc-platform.c
or something.
> [...]
>
>> +
>> +/*
>> + * The functions below are used for the EMMC-17978 workaround.
>> + *
>> + * Due to an imperfection in the design of the MMC bus hardware,
>> + * the 2nd to last cache block of a DMA read must be locked into the L2 Cache.
>> + * Otherwise, data corruption may occur.
>> + */
>
> Isn't these operations also depending on the SoC/Arch? If so, perhaps
> you need a set of platform callbacks even for these.
>
>> +
>> +static inline void *phys_to_ptr(u64 address)
>> +{
>> + return (void *)(address | (1ull<<63)); /* XKPHYS */
>> +}
>> +
>> +/**
>> + * Lock a single line into L2. The line is zeroed before locking
>> + * to make sure no dram accesses are made.
>> + *
>> + * @addr Physical address to lock
>> + */
>> +static void l2c_lock_line(u64 addr)
>> +{
>> + char *addr_ptr = phys_to_ptr(addr);
>> + asm volatile (
>> + "cache 31, %[line]" /* Unlock the line */
>> + :: [line] "m" (*addr_ptr));
>> +}
>> +
>> +/**
>> + * Locks a memory region in the L2 cache
>> + *
>> + * @start - start address to begin locking
>> + * @len - length in bytes to lock
>> + */
>> +static void l2c_lock_mem_region(u64 start, u64 len)
>> +{
>> + u64 end;
>> +
>> + /* Round start/end to cache line boundaries */
>> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
>> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
>> +
>> + while (start <= end) {
>> + l2c_lock_line(start);
>> + start += CVMX_CACHE_LINE_SIZE;
>> + }
>> + asm volatile("sync");
>> +}
>> +
>> +/**
>> + * Unlock a single line in the L2 cache.
>> + *
>> + * @addr Physical address to unlock
>> + *
>> + * Return Zero on success
>> + */
>> +static void l2c_unlock_line(u64 addr)
>> +{
>> + char *addr_ptr = phys_to_ptr(addr);
>> + asm volatile (
>> + "cache 23, %[line]" /* Unlock the line */
>> + :: [line] "m" (*addr_ptr));
>> +}
>> +
>> +/**
>> + * Unlock a memory region in the L2 cache
>> + *
>> + * @start - start address to unlock
>> + * @len - length to unlock in bytes
>> + */
>> +static void l2c_unlock_mem_region(u64 start, u64 len)
>> +{
>> + u64 end;
>> +
>> + /* Round start/end to cache line boundaries */
>> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
>> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
>> +
>> + while (start <= end) {
>> + l2c_unlock_line(start);
>> + start += CVMX_CACHE_LINE_SIZE;
>> + }
>> +}
>> +
>
> [...]
I guess we move this code too.
>
>> +
>> +static void octeon_mmc_dma_request(struct mmc_host *mmc,
>> + struct mmc_request *mrq)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + struct mmc_command *cmd;
>> + struct mmc_data *data;
>> + union cvmx_mio_emm_int emm_int;
>> + union cvmx_mio_emm_dma emm_dma;
>> + union cvmx_mio_ndf_dma_cfg dma_cfg;
>> +
>> + cmd = mrq->cmd;
>> + if (mrq->data == NULL || mrq->data->sg == NULL || !mrq->data->sg_len ||
>> + mrq->stop == NULL || mrq->stop->opcode != MMC_STOP_TRANSMISSION) {
>> + dev_err(&mmc->card->dev,
>> + "Error: octeon_mmc_dma_request no data\n");
>> + cmd->error = -EINVAL;
>> + if (mrq->done)
>> + mrq->done(mrq);
>> + return;
>> + }
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>> +
>> + /* Only a single user of the bootbus at a time. */
>> + octeon_mmc_acquire_bus(host);
>> +
>> + octeon_mmc_switch_to(slot);
>> +
>> + data = mrq->data;
>> +
>> + if (data->timeout_ns)
>> + writeq(octeon_mmc_timeout_to_wdog(slot, data->timeout_ns),
>> + host->base + OCT_MIO_EMM_WDOG);
>> +
>> + WARN_ON(host->current_req);
>> + host->current_req = mrq;
>> +
>> + host->sg_idx = 0;
>> +
>> + WARN_ON(data->blksz * data->blocks > host->linear_buf_size);
>> +
>> + if ((data->flags & MMC_DATA_WRITE) && data->sg_len > 1) {
>> + size_t r = sg_copy_to_buffer(data->sg, data->sg_len,
>> + host->linear_buf, data->blksz * data->blocks);
>> + WARN_ON(data->blksz * data->blocks != r);
>> + }
>> +
>> + dma_cfg.u64 = 0;
>> + dma_cfg.s.en = 1;
>> + dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>> +#ifdef __LITTLE_ENDIAN
>> + dma_cfg.s.endian = 1;
>> +#endif
>> + dma_cfg.s.size = ((data->blksz * data->blocks) / 8) - 1;
>> + if (!host->big_dma_addr) {
>> + if (data->sg_len > 1)
>> + dma_cfg.s.adr = virt_to_phys(host->linear_buf);
>> + else
>> + dma_cfg.s.adr = sg_phys(data->sg);
>> + }
>> + writeq(dma_cfg.u64, host->ndf_base + OCT_MIO_NDF_DMA_CFG);
>> + if (host->big_dma_addr) {
>> + u64 addr;
>> +
>> + if (data->sg_len > 1)
>> + addr = virt_to_phys(host->linear_buf);
>> + else
>> + addr = sg_phys(data->sg);
>> + writeq(addr, host->ndf_base + OCT_MIO_EMM_DMA_ADR);
>> + }
>> +
>> + emm_dma.u64 = 0;
>> + emm_dma.s.bus_id = slot->bus_id;
>> + emm_dma.s.dma_val = 1;
>> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
>> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>> + if (mmc_card_mmc(mmc->card) ||
>> + (mmc_card_sd(mmc->card) &&
>> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
>> + emm_dma.s.multi = 1;
>
> Could you elaborate on exactly what you are doing here. I don't
> understand why you need to check whether the card supports CMD23.
The MMC host hardware doesn't issue single commands, because that would
require the driver and OS MMC core to do work to determine the proper
sequence of commands. Instead, this hardware is superior to most other
MMC bus hosts, the sequence of MMC command required to execute a
transfer are issued automatically by the bus hardware.
Because the interface expected by the Linux MMC core is at a lower level
than what the OCTEON MMC host can accept we need to examine the the
mmc_request and card capabilities to determine which operations most
closely match what is being requested.
In the case of emm_dma.s.multi above, the bus hardware will
automatically issue CMD23 when this bit is set, so we only set it if we
think it is a valid thing to do.
>
> Moreover you must not access mmc->card unless you make sure there is a
> valid pointer for it.
OK, it has never been found to be invalid in the wild. When a transfer
is requested, it always targets some card, but we can add a check at the
top to return an error code if the card is somehow NULL.
>
>> + emm_dma.s.block_cnt = data->blocks;
>> + emm_dma.s.card_addr = cmd->arg;
>> +
>> + emm_int.u64 = 0;
>> + emm_int.s.dma_done = 1;
>> + emm_int.s.cmd_err = 1;
>> + emm_int.s.dma_err = 1;
>> + /* Clear the bit. */
>> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
>> + if (!host->has_ciu3)
>> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT_EN);
>> + host->dma_active = true;
>> +
>> + if ((OCTEON_IS_MODEL(OCTEON_CN6XXX) ||
>> + OCTEON_IS_MODEL(OCTEON_CNF7XXX)) &&
>> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK &&
>> + (data->blksz * data->blocks) > 1024) {
>> + host->n_minus_one = dma_cfg.s.adr +
>> + (data->blksz * data->blocks) - 1024;
>> + l2c_lock_mem_region(host->n_minus_one, 512);
>> + }
>> +
>> + if (mmc->card && mmc_card_sd(mmc->card))
>> + writeq(0x00b00000ull, host->base + OCT_MIO_EMM_STS_MASK);
>> + else
>> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
>
> Some explanation of what goes on here would be nice. You are writing
> some magic mask depending on whether it SD or MMC.
>
> Does that also mean this controller don't support SDIO? If so, you may
> enable MMC_CAP2_NO_SDIO.
>
See comment above about how magical the controller is. We have to
analyze the request and tell the controller which bits in the card
status to consider when detecting errors in the command sequencing.
We will add a comment about this.
>> + writeq(emm_dma.u64, host->base + OCT_MIO_EMM_DMA);
>> +}
>> +
>> +static void octeon_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + struct mmc_command *cmd;
>> + union cvmx_mio_emm_int emm_int;
>> + union cvmx_mio_emm_cmd emm_cmd;
>> + struct octeon_mmc_cr_mods mods;
>> +
>> + cmd = mrq->cmd;
>> +
>> + if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK ||
>> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) {
>
> Instead of checking the opcode, perhaps you should check the number
> blocks/bytes that is about to be transfers.
>
> Or is there a particular reason to why multiblock transfers are required?
See comment above about the magical command sequencing. We have to
analyze the request to see if it makes sense to try to run it with using
the command sequences that use DMA.
>
>> + octeon_mmc_dma_request(mmc, mrq);
>> + return;
>> + }
>> +
>> + mods = octeon_mmc_get_cr_mods(cmd);
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>
> [...]
>
>> +
>> +static void octeon_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + int bus_width;
>> + int clock;
>> + bool ddr_clock;
>> + int hs_timing;
>> + int power_class = 10;
>> + int clk_period;
>> + int timeout = 2000;
>> + union cvmx_mio_emm_switch emm_switch;
>> + union cvmx_mio_emm_rsp_sts emm_sts;
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>> +
>> + /* Only a single user of the bootbus at a time. */
>> + octeon_mmc_acquire_bus(host);
>> +
>> + octeon_mmc_switch_to(slot);
>> +
>> + /*
>> + * Reset the chip on each power off
>> + */
>> + if (ios->power_mode == MMC_POWER_OFF) {
>> + octeon_mmc_reset_bus(slot);
>> + if (!IS_ERR(mmc->supply.vmmc))
>> + regulator_disable(mmc->supply.vmmc);
>
> You shouldn't access this regulator directly, as it's controlled by
> the mmc core. Instead use mmc_regulator_set_ocr()
>
>> + else /* Legacy power GPIO */
>> + gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
>> + } else {
>> + if (!IS_ERR(mmc->supply.vmmc))
>> + regulator_enable(mmc->supply.vmmc);
>
> Similar comment as above. Use mmc_regulator_set_ocr().
>
>> + else /* Legacy power GPIO */
>> + gpiod_set_value_cansleep(slot->pwr_gpiod, 1);
>> + }
>> +
>> + switch (ios->bus_width) {
>> + case MMC_BUS_WIDTH_8:
>> + bus_width = 2;
>> + break;
>> + case MMC_BUS_WIDTH_4:
>> + bus_width = 1;
>> + break;
>> + case MMC_BUS_WIDTH_1:
>> + bus_width = 0;
>> + break;
>> + default:
>> + bus_width = 0;
>> + break;
>> + }
>
> [...]
>
>> +static const struct mmc_host_ops octeon_mmc_ops = {
>> + .request = octeon_mmc_request,
>> + .set_ios = octeon_mmc_set_ios,
>> + .get_ro = mmc_gpio_get_ro,
>> + .get_cd = mmc_gpio_get_cd,
>> +};
>> +
>> +static void octeon_mmc_set_clock(struct octeon_mmc_slot *slot,
>> + unsigned int clock)
>> +{
>> + struct mmc_host *mmc = slot->mmc;
>> +
>> + clock = min(clock, mmc->f_max);
>> + clock = max(clock, mmc->f_min);
>> + slot->clock = clock;
>> +}
>> +
>> +static int octeon_mmc_initlowlevel(struct octeon_mmc_slot *slot,
>> + int bus_width)
>> +{
>> + union cvmx_mio_emm_switch emm_switch;
>> + struct octeon_mmc_host *host = slot->host;
>> +
>> + host->emm_cfg |= 1ull << slot->bus_id;
>> + writeq(host->emm_cfg, slot->host->base + OCT_MIO_EMM_CFG);
>> + octeon_mmc_set_clock(slot, 400000);
>> +
>> + /* Program initial clock speed and power */
>> + emm_switch.u64 = 0;
>> + emm_switch.s.power_class = 10;
>> + emm_switch.s.clk_hi = (slot->sclock / slot->clock) / 2;
>> + emm_switch.s.clk_lo = (slot->sclock / slot->clock) / 2;
>> +
>> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
>> + emm_switch.s.bus_id = slot->bus_id;
>> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
>> + slot->cached_switch = emm_switch.u64;
>> +
>> + writeq(((u64)slot->clock * 850ull) / 1000ull,
>> + host->base + OCT_MIO_EMM_WDOG);
>> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
>> + writeq(1, host->base + OCT_MIO_EMM_RCA);
>
> Perhaps some comments explaining what goes on.
>
>> + return 0;
>> +}
>> +
>> +static int octeon_mmc_slot_probe(struct platform_device *slot_pdev,
>> + struct octeon_mmc_host *host)
>> +{
>> + struct mmc_host *mmc;
>> + struct octeon_mmc_slot *slot;
>> + struct device *dev = &slot_pdev->dev;
>> + struct device_node *node = slot_pdev->dev.of_node;
>> + u32 id, bus_width, max_freq, cmd_skew, dat_skew;
>> + u64 clock_period;
>> + int ret;
>> +
>> + ret = of_property_read_u32(node, "reg", &id);
>> + if (ret) {
>> + dev_err(dev, "Missing or invalid reg property on %s\n",
>> + of_node_full_name(node));
>> + return ret;
>> + }
>> +
>> + if (id >= OCTEON_MAX_MMC || host->slot[id]) {
>> + dev_err(dev, "Invalid reg property on %s\n",
>> + of_node_full_name(node));
>> + return -EINVAL;
>> + }
>> +
>> + mmc = mmc_alloc_host(sizeof(struct octeon_mmc_slot), dev);
>> + if (!mmc) {
>> + dev_err(dev, "alloc host failed\n");
>> + return -ENOMEM;
>> + }
>> +
>> + slot = mmc_priv(mmc);
>> + slot->mmc = mmc;
>> + slot->host = host;
>> +
>> + /*
>> + * The "cavium,bus-max-width" property is DEPRECATED and should
>> + * not be used. We handle it here to support older firmware.
>> + * Going forward, the standard "bus-width" property is used
>> + * instead of the Cavium-specific property.
>> + */
>
> I think you should call mmc_of_parse() as it will parse for the common
> mmc DT properties.
>
> After that, you should parse for the deprecated mmc cavium DT
> properties and when such is found, update the corresponding values for
> bus width and max freq.
>
> Perhaps you also need a default value for max freq, so you need to
> check that as the final thing.
>
>> + ret = of_property_read_u32(node, "bus-width", &bus_width);
>> + if (ret) {
>> + /* Try legacy "cavium,bus-max-width" property. */
>> + ret = of_property_read_u32(node, "cavium,bus-max-width",
>> + &bus_width);
>> + if (ret) {
>> + /* No bus width specified, use default. */
>> + bus_width = 8;
>> + dev_info(dev, "Default bus width %u used for slot %u\n",
>> + bus_width, id);
>> + }
>> + }
>> +
>> +
>> + switch (bus_width) {
>> + case 1:
>> + case 4:
>> + case 8:
>> + break;
>> + default:
>> + dev_err(dev, "Invalid bus width for slot %u\n", id);
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + /*
>> + * The "spi-max-frequency" property is DEPRECATED and should
>> + * not be used. We handle it here to support older firmware.
>> + * Going forward, the standard "max-frequency" property is
>> + * used instead.
>> + */
>> + ret = of_property_read_u32(node, "max-frequency", &max_freq);
>> + if (ret) {
>> + /* Try legacy "spi-max-frequency" property. */
>> + ret = of_property_read_u32(node, "spi-max-frequency",
>> + &max_freq);
>> + if (ret) {
>> + /* No frequency properties found, use default. */
>> + max_freq = 52000000;
>> + dev_info(dev, "Default %u frequency used for slot %u\n",
>> + id, max_freq);
>> + }
>> + }
>> +
>> + /* Get regulators and the supported OCR mask */
>> + ret = mmc_regulator_get_supply(mmc);
>> + if (ret == -EPROBE_DEFER)
>> + goto err;
>> +
>> + /* Alternatively a GPIO may be specified to control slot power */
>> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
>
> No, I am not happy with this as we discussed earlier. You need to
> parse the DTB from SoC specifc code and manage the power GPIO from
> there.
>
> Ideally you should register the power GPIO as a GPIO regulator for the
> cavium mmc slot device.
What do we do if the GPIO doensn't really control power to the card, but
rather is just a bus isolator on the data bus lines? The device tree
will still have a property called "power" (as that is a legacy binding
that cannot be changed), but no power control is done.
In this case, is it still appropiate to use the regulator framework?
I don't see what is gained. This is an SoC specific MMC controller that
is connected in a manner that doesn't really match the Linux regulator
framework. Is it really cleaner to put 100 lines of ugly hacks in the
platform code instead of a couple of lines here in the driver? What is
achieved? We arn't creating an elegant framework, but instead jumping
through hoops to make an archectual mismatch between various Linux
driver frameworks be bent to fit as a matter of principle.
If that is what you require to merge the driver we will do it.
>
> In that way you will get the calculated OCR mask just by calling
> mmc_regulator_get_supply() and you don't need to use the GPIO API to
> deal with powers.
>
>> +
>> + /* Octeon specific DT properties */
>> + ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
>> + if (ret)
>> + cmd_skew = 0;
>> +
>> + ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
>> + if (ret)
>> + dat_skew = 0;
>> +
>> + /*
>> + * Set up host parameters.
>> + */
>> + mmc->ops = &octeon_mmc_ops;
>> + mmc->f_min = 400000;
>> + mmc->f_max = max_freq;
>> + mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
>> + MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA |
>
> The MMC_CAP_8_BIT_DATA and MMC_CAP_4_BIT_DATA is supposed to be
> assigned depending on the configuration in DTS, not hardcoded like
> this.
>
> There's actually also DT bindings parses by mmc_of_parse() for
> MMC_CAP_MMC_HIGHSPEED and MMC_CAP_SD_HIGHSPEED, although if you know
> that your HW always supports these modes, it's fine to enabled them
> like this.
>
We will fix these up.
>> + MMC_CAP_ERASE;
>
> To use MMC_CAP_ERASE, it's recomended to notify the mmc core about
> your used request busy timeout.
>
> So please assign the mmc->max_busy_timeout to its correct value.
>
>> + mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
>> + MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
>> + MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
>
> This you need to explain. I thought your power GPIO only supported on
> and off. In other words it's a fixed regulator, no?
>
> Anyway, when you converted to GPIO regulators you will get this mask
> assigned by calling mmc_regulator_get_supply(), so you don't need any
> of this at all.
>
>> +
>> + /* post-sdk23 caps */
>> + mmc->caps |=
>> + ((mmc->f_max >= 12000000) * MMC_CAP_UHS_SDR12) |
>> + ((mmc->f_max >= 25000000) * MMC_CAP_UHS_SDR25) |
>> + ((mmc->f_max >= 50000000) * MMC_CAP_UHS_SDR50) |
>> + MMC_CAP_CMD23;
>
> Supporting UHS mode requires you to be able to switch the I/O voltage
> level from ~3.3 V to 1.8 V.
>
> How do you manage that without implementing the following host ops?
> ->start_signal_voltage_switch()
> ->card_busy()
>
> Also note that we have common mmc DT bindings for MMC_CAP_UHS*, which
> is parsed via mmc_of_parse().
We will sort this out too.
>
>> +
>> + if ((!IS_ERR(mmc->supply.vmmc)) || (slot->pwr_gpiod))
>> + mmc->caps |= MMC_CAP_POWER_OFF_CARD;
>
> This cap is used only for SDIO scenarios. I don't think you need it!?
>
>> +
>> + /* "1.8v" capability is actually 1.8-or-3.3v */
>
> Yes, there is a lacking capablity for eMMC 3.3 V, although I don't
> think you need to care here...
>
>> + if (ddr)
>> + mmc->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR;
>
> ... I assume it's safe to enable MMC_CAP_1_8V_DDR as that applies to
> eMMCs. Or you HW only supports 3.3V I/O voltage for eMMCs?
>
> Although, because of the upper comment about UHS modes, you should
> probably not enable MMC_CAP_UHS_DDR50 at all.
Agreed.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
@ 2016-08-23 17:41 ` David Daney
0 siblings, 0 replies; 16+ messages in thread
From: David Daney @ 2016-08-23 17:41 UTC (permalink / raw)
To: Ulf Hansson
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 08/23/2016 07:53 AM, Ulf Hansson wrote:
> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
[...]
>> +#include <asm/byteorder.h>
>> +#include <asm/octeon/octeon.h>
>
OK, we will duplicate any needed definitions from octeon.h into the
driver source file.
> We shouldn't include SoC specific headers in a generic mmc driver.
It is not really a generic MMC driver. It is a driver for an MMC host
found only on two families of Cavium SoCs.
>
> If there are dependencies to perform SoC specific operations, then you
> should use platform callbacks. Or better, if those operations can be
> made through generic interfaces.
>
This means that code for this device will be spread across two files.
One for things deemed SoC specific, somewhere in arch/mips, everything
else (which is really SoS specific too, but deemed to be generic) in
this file.
>> +
>> +#define DRV_NAME "octeon_mmc"
>> +
>> +#define OCTEON_MAX_MMC 4
>> +
>> +#define OCT_MIO_NDF_DMA_CFG 0x00
>> +#define OCT_MIO_EMM_DMA_ADR 0x08
>> +
>> +#define OCT_MIO_EMM_CFG 0x00
>> +#define OCT_MIO_EMM_SWITCH 0x48
>> +#define OCT_MIO_EMM_DMA 0x50
>> +#define OCT_MIO_EMM_CMD 0x58
>> +#define OCT_MIO_EMM_RSP_STS 0x60
>> +#define OCT_MIO_EMM_RSP_LO 0x68
>> +#define OCT_MIO_EMM_RSP_HI 0x70
>> +#define OCT_MIO_EMM_INT 0x78
>> +#define OCT_MIO_EMM_INT_EN 0x80
>> +#define OCT_MIO_EMM_WDOG 0x88
>> +#define OCT_MIO_EMM_SAMPLE 0x90
>> +#define OCT_MIO_EMM_STS_MASK 0x98
>> +#define OCT_MIO_EMM_RCA 0xa0
>> +#define OCT_MIO_EMM_BUF_IDX 0xe0
>> +#define OCT_MIO_EMM_BUF_DAT 0xe8
>> +
>> +#define CVMX_MIO_BOOT_CTL CVMX_ADD_IO_SEG(0x00011800000000D0ull)
>> +
>> +struct octeon_mmc_host {
>> + void __iomem *base;
>> + void __iomem *ndf_base;
>> + u64 emm_cfg;
>> + u64 n_minus_one; /* OCTEON II workaround location */
>> + int last_slot;
>> +
>> + struct semaphore mmc_serializer;
>> + struct mmc_request *current_req;
>> + unsigned int linear_buf_size;
>> + void *linear_buf;
>> + struct sg_mapping_iter smi;
>> + int sg_idx;
>> + bool dma_active;
>> +
>> + struct platform_device *pdev;
>> + struct gpio_desc *global_pwr_gpiod;
>> + bool dma_err_pending;
>> + bool need_bootbus_lock;
>> + bool big_dma_addr;
>> + bool need_irq_handler_lock;
>> + spinlock_t irq_handler_lock;
>> + bool has_ciu3;
>> +
>> + struct octeon_mmc_slot *slot[OCTEON_MAX_MMC];
>> +};
>> +
>> +struct octeon_mmc_slot {
>> + struct mmc_host *mmc; /* slot-level mmc_core object */
>> + struct octeon_mmc_host *host; /* common hw for all 4 slots */
>> +
>> + unsigned int clock;
>> + unsigned int sclock;
>> +
>> + u64 cached_switch;
>> + u64 cached_rca;
>> +
>> + unsigned int cmd_cnt; /* sample delay */
>> + unsigned int dat_cnt; /* sample delay */
>> +
>> + int bus_width;
>> + int bus_id;
>> +
>> + /* Legacy property - in future mmc->supply.vmmc should be used */
>> + struct gpio_desc *pwr_gpiod;
>> +};
>> +
>> +static int bb_size = 1 << 18;
>> +module_param(bb_size, int, S_IRUGO);
>> +MODULE_PARM_DESC(bb_size,
>> + "Size of DMA linearizing buffer (max transfer size).");
>> +
bb_size, is a performance tuning parameter. We can just hard code it to
some size and not allow it to be changed.
>> +static int ddr = 2;
>> +module_param(ddr, int, S_IRUGO);
>> +MODULE_PARM_DESC(ddr,
>> + "enable DoubleDataRate clocking: 0=no, 1=always, 2=at spi-max-frequency/2");
>
> The module params here seem like a leftover from a debugging exercise.
> Please remove them.
Yes, this "ddr" thing must be removed.
>
>> +
>> +static void octeon_mmc_acquire_bus(struct octeon_mmc_host *host)
>> +{
>> + if (host->need_bootbus_lock) {
>> + down(&octeon_bootbus_sem);
>> + /* On cn70XX switch the mmc unit onto the bus. */
>> + if (OCTEON_IS_MODEL(OCTEON_CN70XX))
>
> According to my upper comment about SoC specific code, please move
> this code out of the driver.
>
> You may use a DT compatible string to perform specific operations for
> some versions of the IP/SoC, although not to call SoC specific code
> directly.
>
> This comment applies to other places for $subject patch as well,
> although I am not going to comment on each of them. I leave that to
> you to figure out.
>
>> + writeq(0, (void __iomem *)CVMX_MIO_BOOT_CTL);
>> + } else {
>> + down(&host->mmc_serializer);
>> + }
>> +}
>> +
>
I guess we will put it in arch/mips/cavium-octeon/octeon-mmc-platform.c
or something.
> [...]
>
>> +
>> +/*
>> + * The functions below are used for the EMMC-17978 workaround.
>> + *
>> + * Due to an imperfection in the design of the MMC bus hardware,
>> + * the 2nd to last cache block of a DMA read must be locked into the L2 Cache.
>> + * Otherwise, data corruption may occur.
>> + */
>
> Isn't these operations also depending on the SoC/Arch? If so, perhaps
> you need a set of platform callbacks even for these.
>
>> +
>> +static inline void *phys_to_ptr(u64 address)
>> +{
>> + return (void *)(address | (1ull<<63)); /* XKPHYS */
>> +}
>> +
>> +/**
>> + * Lock a single line into L2. The line is zeroed before locking
>> + * to make sure no dram accesses are made.
>> + *
>> + * @addr Physical address to lock
>> + */
>> +static void l2c_lock_line(u64 addr)
>> +{
>> + char *addr_ptr = phys_to_ptr(addr);
>> + asm volatile (
>> + "cache 31, %[line]" /* Unlock the line */
>> + :: [line] "m" (*addr_ptr));
>> +}
>> +
>> +/**
>> + * Locks a memory region in the L2 cache
>> + *
>> + * @start - start address to begin locking
>> + * @len - length in bytes to lock
>> + */
>> +static void l2c_lock_mem_region(u64 start, u64 len)
>> +{
>> + u64 end;
>> +
>> + /* Round start/end to cache line boundaries */
>> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
>> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
>> +
>> + while (start <= end) {
>> + l2c_lock_line(start);
>> + start += CVMX_CACHE_LINE_SIZE;
>> + }
>> + asm volatile("sync");
>> +}
>> +
>> +/**
>> + * Unlock a single line in the L2 cache.
>> + *
>> + * @addr Physical address to unlock
>> + *
>> + * Return Zero on success
>> + */
>> +static void l2c_unlock_line(u64 addr)
>> +{
>> + char *addr_ptr = phys_to_ptr(addr);
>> + asm volatile (
>> + "cache 23, %[line]" /* Unlock the line */
>> + :: [line] "m" (*addr_ptr));
>> +}
>> +
>> +/**
>> + * Unlock a memory region in the L2 cache
>> + *
>> + * @start - start address to unlock
>> + * @len - length to unlock in bytes
>> + */
>> +static void l2c_unlock_mem_region(u64 start, u64 len)
>> +{
>> + u64 end;
>> +
>> + /* Round start/end to cache line boundaries */
>> + end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
>> + start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
>> +
>> + while (start <= end) {
>> + l2c_unlock_line(start);
>> + start += CVMX_CACHE_LINE_SIZE;
>> + }
>> +}
>> +
>
> [...]
I guess we move this code too.
>
>> +
>> +static void octeon_mmc_dma_request(struct mmc_host *mmc,
>> + struct mmc_request *mrq)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + struct mmc_command *cmd;
>> + struct mmc_data *data;
>> + union cvmx_mio_emm_int emm_int;
>> + union cvmx_mio_emm_dma emm_dma;
>> + union cvmx_mio_ndf_dma_cfg dma_cfg;
>> +
>> + cmd = mrq->cmd;
>> + if (mrq->data == NULL || mrq->data->sg == NULL || !mrq->data->sg_len ||
>> + mrq->stop == NULL || mrq->stop->opcode != MMC_STOP_TRANSMISSION) {
>> + dev_err(&mmc->card->dev,
>> + "Error: octeon_mmc_dma_request no data\n");
>> + cmd->error = -EINVAL;
>> + if (mrq->done)
>> + mrq->done(mrq);
>> + return;
>> + }
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>> +
>> + /* Only a single user of the bootbus at a time. */
>> + octeon_mmc_acquire_bus(host);
>> +
>> + octeon_mmc_switch_to(slot);
>> +
>> + data = mrq->data;
>> +
>> + if (data->timeout_ns)
>> + writeq(octeon_mmc_timeout_to_wdog(slot, data->timeout_ns),
>> + host->base + OCT_MIO_EMM_WDOG);
>> +
>> + WARN_ON(host->current_req);
>> + host->current_req = mrq;
>> +
>> + host->sg_idx = 0;
>> +
>> + WARN_ON(data->blksz * data->blocks > host->linear_buf_size);
>> +
>> + if ((data->flags & MMC_DATA_WRITE) && data->sg_len > 1) {
>> + size_t r = sg_copy_to_buffer(data->sg, data->sg_len,
>> + host->linear_buf, data->blksz * data->blocks);
>> + WARN_ON(data->blksz * data->blocks != r);
>> + }
>> +
>> + dma_cfg.u64 = 0;
>> + dma_cfg.s.en = 1;
>> + dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>> +#ifdef __LITTLE_ENDIAN
>> + dma_cfg.s.endian = 1;
>> +#endif
>> + dma_cfg.s.size = ((data->blksz * data->blocks) / 8) - 1;
>> + if (!host->big_dma_addr) {
>> + if (data->sg_len > 1)
>> + dma_cfg.s.adr = virt_to_phys(host->linear_buf);
>> + else
>> + dma_cfg.s.adr = sg_phys(data->sg);
>> + }
>> + writeq(dma_cfg.u64, host->ndf_base + OCT_MIO_NDF_DMA_CFG);
>> + if (host->big_dma_addr) {
>> + u64 addr;
>> +
>> + if (data->sg_len > 1)
>> + addr = virt_to_phys(host->linear_buf);
>> + else
>> + addr = sg_phys(data->sg);
>> + writeq(addr, host->ndf_base + OCT_MIO_EMM_DMA_ADR);
>> + }
>> +
>> + emm_dma.u64 = 0;
>> + emm_dma.s.bus_id = slot->bus_id;
>> + emm_dma.s.dma_val = 1;
>> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
>> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>> + if (mmc_card_mmc(mmc->card) ||
>> + (mmc_card_sd(mmc->card) &&
>> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
>> + emm_dma.s.multi = 1;
>
> Could you elaborate on exactly what you are doing here. I don't
> understand why you need to check whether the card supports CMD23.
The MMC host hardware doesn't issue single commands, because that would
require the driver and OS MMC core to do work to determine the proper
sequence of commands. Instead, this hardware is superior to most other
MMC bus hosts, the sequence of MMC command required to execute a
transfer are issued automatically by the bus hardware.
Because the interface expected by the Linux MMC core is at a lower level
than what the OCTEON MMC host can accept we need to examine the the
mmc_request and card capabilities to determine which operations most
closely match what is being requested.
In the case of emm_dma.s.multi above, the bus hardware will
automatically issue CMD23 when this bit is set, so we only set it if we
think it is a valid thing to do.
>
> Moreover you must not access mmc->card unless you make sure there is a
> valid pointer for it.
OK, it has never been found to be invalid in the wild. When a transfer
is requested, it always targets some card, but we can add a check at the
top to return an error code if the card is somehow NULL.
>
>> + emm_dma.s.block_cnt = data->blocks;
>> + emm_dma.s.card_addr = cmd->arg;
>> +
>> + emm_int.u64 = 0;
>> + emm_int.s.dma_done = 1;
>> + emm_int.s.cmd_err = 1;
>> + emm_int.s.dma_err = 1;
>> + /* Clear the bit. */
>> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT);
>> + if (!host->has_ciu3)
>> + writeq(emm_int.u64, host->base + OCT_MIO_EMM_INT_EN);
>> + host->dma_active = true;
>> +
>> + if ((OCTEON_IS_MODEL(OCTEON_CN6XXX) ||
>> + OCTEON_IS_MODEL(OCTEON_CNF7XXX)) &&
>> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK &&
>> + (data->blksz * data->blocks) > 1024) {
>> + host->n_minus_one = dma_cfg.s.adr +
>> + (data->blksz * data->blocks) - 1024;
>> + l2c_lock_mem_region(host->n_minus_one, 512);
>> + }
>> +
>> + if (mmc->card && mmc_card_sd(mmc->card))
>> + writeq(0x00b00000ull, host->base + OCT_MIO_EMM_STS_MASK);
>> + else
>> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
>
> Some explanation of what goes on here would be nice. You are writing
> some magic mask depending on whether it SD or MMC.
>
> Does that also mean this controller don't support SDIO? If so, you may
> enable MMC_CAP2_NO_SDIO.
>
See comment above about how magical the controller is. We have to
analyze the request and tell the controller which bits in the card
status to consider when detecting errors in the command sequencing.
We will add a comment about this.
>> + writeq(emm_dma.u64, host->base + OCT_MIO_EMM_DMA);
>> +}
>> +
>> +static void octeon_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + struct mmc_command *cmd;
>> + union cvmx_mio_emm_int emm_int;
>> + union cvmx_mio_emm_cmd emm_cmd;
>> + struct octeon_mmc_cr_mods mods;
>> +
>> + cmd = mrq->cmd;
>> +
>> + if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK ||
>> + cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) {
>
> Instead of checking the opcode, perhaps you should check the number
> blocks/bytes that is about to be transfers.
>
> Or is there a particular reason to why multiblock transfers are required?
See comment above about the magical command sequencing. We have to
analyze the request to see if it makes sense to try to run it with using
the command sequences that use DMA.
>
>> + octeon_mmc_dma_request(mmc, mrq);
>> + return;
>> + }
>> +
>> + mods = octeon_mmc_get_cr_mods(cmd);
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>
> [...]
>
>> +
>> +static void octeon_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> + struct octeon_mmc_slot *slot;
>> + struct octeon_mmc_host *host;
>> + int bus_width;
>> + int clock;
>> + bool ddr_clock;
>> + int hs_timing;
>> + int power_class = 10;
>> + int clk_period;
>> + int timeout = 2000;
>> + union cvmx_mio_emm_switch emm_switch;
>> + union cvmx_mio_emm_rsp_sts emm_sts;
>> +
>> + slot = mmc_priv(mmc);
>> + host = slot->host;
>> +
>> + /* Only a single user of the bootbus at a time. */
>> + octeon_mmc_acquire_bus(host);
>> +
>> + octeon_mmc_switch_to(slot);
>> +
>> + /*
>> + * Reset the chip on each power off
>> + */
>> + if (ios->power_mode == MMC_POWER_OFF) {
>> + octeon_mmc_reset_bus(slot);
>> + if (!IS_ERR(mmc->supply.vmmc))
>> + regulator_disable(mmc->supply.vmmc);
>
> You shouldn't access this regulator directly, as it's controlled by
> the mmc core. Instead use mmc_regulator_set_ocr()
>
>> + else /* Legacy power GPIO */
>> + gpiod_set_value_cansleep(slot->pwr_gpiod, 0);
>> + } else {
>> + if (!IS_ERR(mmc->supply.vmmc))
>> + regulator_enable(mmc->supply.vmmc);
>
> Similar comment as above. Use mmc_regulator_set_ocr().
>
>> + else /* Legacy power GPIO */
>> + gpiod_set_value_cansleep(slot->pwr_gpiod, 1);
>> + }
>> +
>> + switch (ios->bus_width) {
>> + case MMC_BUS_WIDTH_8:
>> + bus_width = 2;
>> + break;
>> + case MMC_BUS_WIDTH_4:
>> + bus_width = 1;
>> + break;
>> + case MMC_BUS_WIDTH_1:
>> + bus_width = 0;
>> + break;
>> + default:
>> + bus_width = 0;
>> + break;
>> + }
>
> [...]
>
>> +static const struct mmc_host_ops octeon_mmc_ops = {
>> + .request = octeon_mmc_request,
>> + .set_ios = octeon_mmc_set_ios,
>> + .get_ro = mmc_gpio_get_ro,
>> + .get_cd = mmc_gpio_get_cd,
>> +};
>> +
>> +static void octeon_mmc_set_clock(struct octeon_mmc_slot *slot,
>> + unsigned int clock)
>> +{
>> + struct mmc_host *mmc = slot->mmc;
>> +
>> + clock = min(clock, mmc->f_max);
>> + clock = max(clock, mmc->f_min);
>> + slot->clock = clock;
>> +}
>> +
>> +static int octeon_mmc_initlowlevel(struct octeon_mmc_slot *slot,
>> + int bus_width)
>> +{
>> + union cvmx_mio_emm_switch emm_switch;
>> + struct octeon_mmc_host *host = slot->host;
>> +
>> + host->emm_cfg |= 1ull << slot->bus_id;
>> + writeq(host->emm_cfg, slot->host->base + OCT_MIO_EMM_CFG);
>> + octeon_mmc_set_clock(slot, 400000);
>> +
>> + /* Program initial clock speed and power */
>> + emm_switch.u64 = 0;
>> + emm_switch.s.power_class = 10;
>> + emm_switch.s.clk_hi = (slot->sclock / slot->clock) / 2;
>> + emm_switch.s.clk_lo = (slot->sclock / slot->clock) / 2;
>> +
>> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
>> + emm_switch.s.bus_id = slot->bus_id;
>> + writeq(emm_switch.u64, host->base + OCT_MIO_EMM_SWITCH);
>> + slot->cached_switch = emm_switch.u64;
>> +
>> + writeq(((u64)slot->clock * 850ull) / 1000ull,
>> + host->base + OCT_MIO_EMM_WDOG);
>> + writeq(0xe4f90080ull, host->base + OCT_MIO_EMM_STS_MASK);
>> + writeq(1, host->base + OCT_MIO_EMM_RCA);
>
> Perhaps some comments explaining what goes on.
>
>> + return 0;
>> +}
>> +
>> +static int octeon_mmc_slot_probe(struct platform_device *slot_pdev,
>> + struct octeon_mmc_host *host)
>> +{
>> + struct mmc_host *mmc;
>> + struct octeon_mmc_slot *slot;
>> + struct device *dev = &slot_pdev->dev;
>> + struct device_node *node = slot_pdev->dev.of_node;
>> + u32 id, bus_width, max_freq, cmd_skew, dat_skew;
>> + u64 clock_period;
>> + int ret;
>> +
>> + ret = of_property_read_u32(node, "reg", &id);
>> + if (ret) {
>> + dev_err(dev, "Missing or invalid reg property on %s\n",
>> + of_node_full_name(node));
>> + return ret;
>> + }
>> +
>> + if (id >= OCTEON_MAX_MMC || host->slot[id]) {
>> + dev_err(dev, "Invalid reg property on %s\n",
>> + of_node_full_name(node));
>> + return -EINVAL;
>> + }
>> +
>> + mmc = mmc_alloc_host(sizeof(struct octeon_mmc_slot), dev);
>> + if (!mmc) {
>> + dev_err(dev, "alloc host failed\n");
>> + return -ENOMEM;
>> + }
>> +
>> + slot = mmc_priv(mmc);
>> + slot->mmc = mmc;
>> + slot->host = host;
>> +
>> + /*
>> + * The "cavium,bus-max-width" property is DEPRECATED and should
>> + * not be used. We handle it here to support older firmware.
>> + * Going forward, the standard "bus-width" property is used
>> + * instead of the Cavium-specific property.
>> + */
>
> I think you should call mmc_of_parse() as it will parse for the common
> mmc DT properties.
>
> After that, you should parse for the deprecated mmc cavium DT
> properties and when such is found, update the corresponding values for
> bus width and max freq.
>
> Perhaps you also need a default value for max freq, so you need to
> check that as the final thing.
>
>> + ret = of_property_read_u32(node, "bus-width", &bus_width);
>> + if (ret) {
>> + /* Try legacy "cavium,bus-max-width" property. */
>> + ret = of_property_read_u32(node, "cavium,bus-max-width",
>> + &bus_width);
>> + if (ret) {
>> + /* No bus width specified, use default. */
>> + bus_width = 8;
>> + dev_info(dev, "Default bus width %u used for slot %u\n",
>> + bus_width, id);
>> + }
>> + }
>> +
>> +
>> + switch (bus_width) {
>> + case 1:
>> + case 4:
>> + case 8:
>> + break;
>> + default:
>> + dev_err(dev, "Invalid bus width for slot %u\n", id);
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + /*
>> + * The "spi-max-frequency" property is DEPRECATED and should
>> + * not be used. We handle it here to support older firmware.
>> + * Going forward, the standard "max-frequency" property is
>> + * used instead.
>> + */
>> + ret = of_property_read_u32(node, "max-frequency", &max_freq);
>> + if (ret) {
>> + /* Try legacy "spi-max-frequency" property. */
>> + ret = of_property_read_u32(node, "spi-max-frequency",
>> + &max_freq);
>> + if (ret) {
>> + /* No frequency properties found, use default. */
>> + max_freq = 52000000;
>> + dev_info(dev, "Default %u frequency used for slot %u\n",
>> + id, max_freq);
>> + }
>> + }
>> +
>> + /* Get regulators and the supported OCR mask */
>> + ret = mmc_regulator_get_supply(mmc);
>> + if (ret == -EPROBE_DEFER)
>> + goto err;
>> +
>> + /* Alternatively a GPIO may be specified to control slot power */
>> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
>
> No, I am not happy with this as we discussed earlier. You need to
> parse the DTB from SoC specifc code and manage the power GPIO from
> there.
>
> Ideally you should register the power GPIO as a GPIO regulator for the
> cavium mmc slot device.
What do we do if the GPIO doensn't really control power to the card, but
rather is just a bus isolator on the data bus lines? The device tree
will still have a property called "power" (as that is a legacy binding
that cannot be changed), but no power control is done.
In this case, is it still appropiate to use the regulator framework?
I don't see what is gained. This is an SoC specific MMC controller that
is connected in a manner that doesn't really match the Linux regulator
framework. Is it really cleaner to put 100 lines of ugly hacks in the
platform code instead of a couple of lines here in the driver? What is
achieved? We arn't creating an elegant framework, but instead jumping
through hoops to make an archectual mismatch between various Linux
driver frameworks be bent to fit as a matter of principle.
If that is what you require to merge the driver we will do it.
>
> In that way you will get the calculated OCR mask just by calling
> mmc_regulator_get_supply() and you don't need to use the GPIO API to
> deal with powers.
>
>> +
>> + /* Octeon specific DT properties */
>> + ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
>> + if (ret)
>> + cmd_skew = 0;
>> +
>> + ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
>> + if (ret)
>> + dat_skew = 0;
>> +
>> + /*
>> + * Set up host parameters.
>> + */
>> + mmc->ops = &octeon_mmc_ops;
>> + mmc->f_min = 400000;
>> + mmc->f_max = max_freq;
>> + mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
>> + MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA |
>
> The MMC_CAP_8_BIT_DATA and MMC_CAP_4_BIT_DATA is supposed to be
> assigned depending on the configuration in DTS, not hardcoded like
> this.
>
> There's actually also DT bindings parses by mmc_of_parse() for
> MMC_CAP_MMC_HIGHSPEED and MMC_CAP_SD_HIGHSPEED, although if you know
> that your HW always supports these modes, it's fine to enabled them
> like this.
>
We will fix these up.
>> + MMC_CAP_ERASE;
>
> To use MMC_CAP_ERASE, it's recomended to notify the mmc core about
> your used request busy timeout.
>
> So please assign the mmc->max_busy_timeout to its correct value.
>
>> + mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
>> + MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
>> + MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
>
> This you need to explain. I thought your power GPIO only supported on
> and off. In other words it's a fixed regulator, no?
>
> Anyway, when you converted to GPIO regulators you will get this mask
> assigned by calling mmc_regulator_get_supply(), so you don't need any
> of this at all.
>
>> +
>> + /* post-sdk23 caps */
>> + mmc->caps |=
>> + ((mmc->f_max >= 12000000) * MMC_CAP_UHS_SDR12) |
>> + ((mmc->f_max >= 25000000) * MMC_CAP_UHS_SDR25) |
>> + ((mmc->f_max >= 50000000) * MMC_CAP_UHS_SDR50) |
>> + MMC_CAP_CMD23;
>
> Supporting UHS mode requires you to be able to switch the I/O voltage
> level from ~3.3 V to 1.8 V.
>
> How do you manage that without implementing the following host ops?
> ->start_signal_voltage_switch()
> ->card_busy()
>
> Also note that we have common mmc DT bindings for MMC_CAP_UHS*, which
> is parsed via mmc_of_parse().
We will sort this out too.
>
>> +
>> + if ((!IS_ERR(mmc->supply.vmmc)) || (slot->pwr_gpiod))
>> + mmc->caps |= MMC_CAP_POWER_OFF_CARD;
>
> This cap is used only for SDIO scenarios. I don't think you need it!?
>
>> +
>> + /* "1.8v" capability is actually 1.8-or-3.3v */
>
> Yes, there is a lacking capablity for eMMC 3.3 V, although I don't
> think you need to care here...
>
>> + if (ddr)
>> + mmc->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR;
>
> ... I assume it's safe to enable MMC_CAP_1_8V_DDR as that applies to
> eMMCs. Or you HW only supports 3.3V I/O voltage for eMMCs?
>
> Although, because of the upper comment about UHS modes, you should
> probably not enable MMC_CAP_UHS_DDR50 at all.
Agreed.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-08-23 17:41 ` David Daney
@ 2016-08-23 19:46 ` Ulf Hansson
-1 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-08-23 19:46 UTC (permalink / raw)
To: David Daney
Cc: Steven J. Hill, linux-mmc, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, Rob Herring, Mark Rutland,
Ralf Baechle
On 23 August 2016 at 19:41, David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org> wrote:
> On 08/23/2016 07:53 AM, Ulf Hansson wrote:
>>
>> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org> wrote:
>
> [...]
>
>>> +#include <asm/byteorder.h>
>>> +#include <asm/octeon/octeon.h>
>>
>>
>
> OK, we will duplicate any needed definitions from octeon.h into the driver
> source file.
Why can't you share it via a platfrom data header at
include/linux/platform_data/* ?
[...]
>>
>
> I guess we will put it in arch/mips/cavium-octeon/octeon-mmc-platform.c or
> something.
There is also drivers/soc/* to consider. I am not sure what suits you best.
[...]
>>> + emm_dma.u64 = 0;
>>> + emm_dma.s.bus_id = slot->bus_id;
>>> + emm_dma.s.dma_val = 1;
>>> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
>>> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>>> + if (mmc_card_mmc(mmc->card) ||
>>> + (mmc_card_sd(mmc->card) &&
>>> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
>>> + emm_dma.s.multi = 1;
>>
>>
>> Could you elaborate on exactly what you are doing here. I don't
>> understand why you need to check whether the card supports CMD23.
>
>
>
> The MMC host hardware doesn't issue single commands, because that would
> require the driver and OS MMC core to do work to determine the proper
> sequence of commands. Instead, this hardware is superior to most other MMC
> bus hosts, the sequence of MMC command required to execute a transfer are
> issued automatically by the bus hardware.
Oh, nice! Actually I have heard of similar HW, although I am not sure
whether there is some mmc driver that has deployed support for this.
Anyway, perhaps we at a later point can think of if there is a clever
way to optimize the request path for these kind of HWs.
>
> Because the interface expected by the Linux MMC core is at a lower level
> than what the OCTEON MMC host can accept we need to examine the the
> mmc_request and card capabilities to determine which operations most closely
> match what is being requested.
>
> In the case of emm_dma.s.multi above, the bus hardware will automatically
> issue CMD23 when this bit is set, so we only set it if we think it is a
> valid thing to do.
I noticed that the similar check is done in the mmc block layer,
perhaps we should add a helper function like mmc_card_cmd23() which
tells whether the cards supports CMD23.
If you like to add a helper as part of this series, it would be nice -
although we can also deal with that later if you prefer so.
>
>
>>
>> Moreover you must not access mmc->card unless you make sure there is a
>> valid pointer for it.
>
>
> OK, it has never been found to be invalid in the wild. When a transfer is
> requested, it always targets some card, but we can add a check at the top to
> return an error code if the card is somehow NULL.
That's probably because you also requires a multi block transfer for
dma jobs, which is when the card has been created...
I would have verified that the card exists...
[...]
>>> +
>>> + /* Alternatively a GPIO may be specified to control slot power */
>>> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power",
>>> GPIOD_OUT_LOW);
>>
>>
>> No, I am not happy with this as we discussed earlier. You need to
>> parse the DTB from SoC specifc code and manage the power GPIO from
>> there.
>>
>> Ideally you should register the power GPIO as a GPIO regulator for the
>> cavium mmc slot device.
>
>
> What do we do if the GPIO doensn't really control power to the card, but
> rather is just a bus isolator on the data bus lines? The device tree will
That more sounds like a pinctrl to me then.
> still have a property called "power" (as that is a legacy binding that
> cannot be changed), but no power control is done.
>
> In this case, is it still appropiate to use the regulator framework?
Probably not.
>
> I don't see what is gained. This is an SoC specific MMC controller that is
> connected in a manner that doesn't really match the Linux regulator
> framework. Is it really cleaner to put 100 lines of ugly hacks in the
> platform code instead of a couple of lines here in the driver? What is
> achieved? We arn't creating an elegant framework, but instead jumping
> through hoops to make an archectual mismatch between various Linux driver
> frameworks be bent to fit as a matter of principle.
>
> If that is what you require to merge the driver we will do it.
We have cleaned up lots of mmc hacks that dealt with "regulators" in
all kind of home-brewed manners. It was a mess.
In this particular case it also seems a little bit unclear what the
regulators (power GPIOs) is really used for. Very similar to the
experience I had with the earlier hacks.
So, please try to describe in detail about what the power GPIO are and
how/if they connects to the card. if they are considered as to control
I/O voltage level or power to the card, then those shall be modelled
as regulators.
Sorry if you find me being stubborn on this, although I hope the
background described above makes you understand a bit better.
[...]
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
@ 2016-08-23 19:46 ` Ulf Hansson
0 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-08-23 19:46 UTC (permalink / raw)
To: David Daney
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 23 August 2016 at 19:41, David Daney <ddaney@caviumnetworks.com> wrote:
> On 08/23/2016 07:53 AM, Ulf Hansson wrote:
>>
>> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
>
> [...]
>
>>> +#include <asm/byteorder.h>
>>> +#include <asm/octeon/octeon.h>
>>
>>
>
> OK, we will duplicate any needed definitions from octeon.h into the driver
> source file.
Why can't you share it via a platfrom data header at
include/linux/platform_data/* ?
[...]
>>
>
> I guess we will put it in arch/mips/cavium-octeon/octeon-mmc-platform.c or
> something.
There is also drivers/soc/* to consider. I am not sure what suits you best.
[...]
>>> + emm_dma.u64 = 0;
>>> + emm_dma.s.bus_id = slot->bus_id;
>>> + emm_dma.s.dma_val = 1;
>>> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0;
>>> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
>>> + if (mmc_card_mmc(mmc->card) ||
>>> + (mmc_card_sd(mmc->card) &&
>>> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
>>> + emm_dma.s.multi = 1;
>>
>>
>> Could you elaborate on exactly what you are doing here. I don't
>> understand why you need to check whether the card supports CMD23.
>
>
>
> The MMC host hardware doesn't issue single commands, because that would
> require the driver and OS MMC core to do work to determine the proper
> sequence of commands. Instead, this hardware is superior to most other MMC
> bus hosts, the sequence of MMC command required to execute a transfer are
> issued automatically by the bus hardware.
Oh, nice! Actually I have heard of similar HW, although I am not sure
whether there is some mmc driver that has deployed support for this.
Anyway, perhaps we at a later point can think of if there is a clever
way to optimize the request path for these kind of HWs.
>
> Because the interface expected by the Linux MMC core is at a lower level
> than what the OCTEON MMC host can accept we need to examine the the
> mmc_request and card capabilities to determine which operations most closely
> match what is being requested.
>
> In the case of emm_dma.s.multi above, the bus hardware will automatically
> issue CMD23 when this bit is set, so we only set it if we think it is a
> valid thing to do.
I noticed that the similar check is done in the mmc block layer,
perhaps we should add a helper function like mmc_card_cmd23() which
tells whether the cards supports CMD23.
If you like to add a helper as part of this series, it would be nice -
although we can also deal with that later if you prefer so.
>
>
>>
>> Moreover you must not access mmc->card unless you make sure there is a
>> valid pointer for it.
>
>
> OK, it has never been found to be invalid in the wild. When a transfer is
> requested, it always targets some card, but we can add a check at the top to
> return an error code if the card is somehow NULL.
That's probably because you also requires a multi block transfer for
dma jobs, which is when the card has been created...
I would have verified that the card exists...
[...]
>>> +
>>> + /* Alternatively a GPIO may be specified to control slot power */
>>> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power",
>>> GPIOD_OUT_LOW);
>>
>>
>> No, I am not happy with this as we discussed earlier. You need to
>> parse the DTB from SoC specifc code and manage the power GPIO from
>> there.
>>
>> Ideally you should register the power GPIO as a GPIO regulator for the
>> cavium mmc slot device.
>
>
> What do we do if the GPIO doensn't really control power to the card, but
> rather is just a bus isolator on the data bus lines? The device tree will
That more sounds like a pinctrl to me then.
> still have a property called "power" (as that is a legacy binding that
> cannot be changed), but no power control is done.
>
> In this case, is it still appropiate to use the regulator framework?
Probably not.
>
> I don't see what is gained. This is an SoC specific MMC controller that is
> connected in a manner that doesn't really match the Linux regulator
> framework. Is it really cleaner to put 100 lines of ugly hacks in the
> platform code instead of a couple of lines here in the driver? What is
> achieved? We arn't creating an elegant framework, but instead jumping
> through hoops to make an archectual mismatch between various Linux driver
> frameworks be bent to fit as a matter of principle.
>
> If that is what you require to merge the driver we will do it.
We have cleaned up lots of mmc hacks that dealt with "regulators" in
all kind of home-brewed manners. It was a mess.
In this particular case it also seems a little bit unclear what the
regulators (power GPIOs) is really used for. Very similar to the
experience I had with the earlier hacks.
So, please try to describe in detail about what the power GPIO are and
how/if they connects to the card. if they are considered as to control
I/O voltage level or power to the card, then those shall be modelled
as regulators.
Sorry if you find me being stubborn on this, although I hope the
background described above makes you understand a bit better.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-08-23 19:46 ` Ulf Hansson
@ 2016-08-23 19:59 ` David Daney
-1 siblings, 0 replies; 16+ messages in thread
From: David Daney @ 2016-08-23 19:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 08/23/2016 12:46 PM, Ulf Hansson wrote:
> On 23 August 2016 at 19:41, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 08/23/2016 07:53 AM, Ulf Hansson wrote:
>>>
>>> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
>>
>> [...]
>>
>>>> +#include <asm/byteorder.h>
>>>> +#include <asm/octeon/octeon.h>
>>>
>>>
>>
>> OK, we will duplicate any needed definitions from octeon.h into the driver
>> source file.
>
> Why can't you share it via a platfrom data header at
> include/linux/platform_data/* ?
>
It isn't "platform_data", it is register layout definitions (thousands
of lines of them), so I don't think it it appropriate to place in
include/linux.
I think the cleanest approach is to put the register definitions in the
driver file, which is the only user, and delete the definition header
files in arch/mips/include/...
David.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
@ 2016-08-23 19:59 ` David Daney
0 siblings, 0 replies; 16+ messages in thread
From: David Daney @ 2016-08-23 19:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 08/23/2016 12:46 PM, Ulf Hansson wrote:
> On 23 August 2016 at 19:41, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 08/23/2016 07:53 AM, Ulf Hansson wrote:
>>>
>>> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
>>
>> [...]
>>
>>>> +#include <asm/byteorder.h>
>>>> +#include <asm/octeon/octeon.h>
>>>
>>>
>>
>> OK, we will duplicate any needed definitions from octeon.h into the driver
>> source file.
>
> Why can't you share it via a platfrom data header at
> include/linux/platform_data/* ?
>
It isn't "platform_data", it is register layout definitions (thousands
of lines of them), so I don't think it it appropriate to place in
include/linux.
I think the cleanest approach is to put the register definitions in the
driver file, which is the only user, and delete the definition header
files in arch/mips/include/...
David.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.
2016-08-23 19:59 ` David Daney
(?)
@ 2016-08-24 7:32 ` Ulf Hansson
-1 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-08-24 7:32 UTC (permalink / raw)
To: David Daney
Cc: Steven J. Hill, linux-mmc, devicetree, linux-mips, Rob Herring,
Mark Rutland, Ralf Baechle
On 23 August 2016 at 21:59, David Daney <ddaney@caviumnetworks.com> wrote:
> On 08/23/2016 12:46 PM, Ulf Hansson wrote:
>>
>> On 23 August 2016 at 19:41, David Daney <ddaney@caviumnetworks.com> wrote:
>>>
>>> On 08/23/2016 07:53 AM, Ulf Hansson wrote:
>>>>
>>>>
>>>> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@cavium.com> wrote:
>>>
>>>
>>> [...]
>>>
>>>>> +#include <asm/byteorder.h>
>>>>> +#include <asm/octeon/octeon.h>
>>>>
>>>>
>>>>
>>>
>>> OK, we will duplicate any needed definitions from octeon.h into the
>>> driver
>>> source file.
>>
>>
>> Why can't you share it via a platfrom data header at
>> include/linux/platform_data/* ?
>>
>
> It isn't "platform_data", it is register layout definitions (thousands of
> lines of them), so I don't think it it appropriate to place in
> include/linux.
>
> I think the cleanest approach is to put the register definitions in the
> driver file, which is the only user, and delete the definition header files
> in arch/mips/include/...
>
> David.
I guess we are not looking at the same header file. :-)
arch/mips/include/asm/octeon/octeon.h contains declarations of
functions/structs and even globally exported variables.
At a closer look this header need a serious cleanup anyway...
* Some of the functions/structs are not used or even implemented.
** Some of the functions/structs is used only internally by the SoC
specific code, thus should be moved to a local header.
*** Some of the functions/structs/exported variables is being used by
several clients. The cavium mmc driver is only one of them.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-08-24 7:32 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-12 18:18 [PATCH V8 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller Steven J. Hill
2016-07-19 18:31 ` Steven J. Hill
2016-07-19 18:31 ` Steven J. Hill
2016-07-19 21:19 ` Ulf Hansson
2016-07-21 13:46 ` Ralf Baechle
2016-08-23 14:56 ` Ulf Hansson
2016-08-12 5:41 ` Steven J. Hill
2016-08-22 14:05 ` Ulf Hansson
2016-08-23 14:53 ` Ulf Hansson
2016-08-23 17:41 ` David Daney
2016-08-23 17:41 ` David Daney
[not found] ` <57BC8ACA.1040506-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
2016-08-23 19:46 ` Ulf Hansson
2016-08-23 19:46 ` Ulf Hansson
2016-08-23 19:59 ` David Daney
2016-08-23 19:59 ` David Daney
2016-08-24 7:32 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.