diff mbox

[V8,2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller.

Message ID 57853474.9050108@cavium.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steven J. Hill July 12, 2016, 6:18 p.m. UTC
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

Comments

Steven J. Hill July 19, 2016, 6:31 p.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson July 19, 2016, 9:19 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ralf Baechle July 21, 2016, 1:46 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven J. Hill Aug. 12, 2016, 5:41 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 22, 2016, 2:05 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 23, 2016, 2:53 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 23, 2016, 2:56 p.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney Aug. 23, 2016, 5:41 p.m. UTC | #8
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 23, 2016, 7:46 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney Aug. 23, 2016, 7:59 p.m. UTC | #10
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.



--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 24, 2016, 7:32 a.m. UTC | #11
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");