[v7,2/2] mmc: Add mmc driver for Sunplus SP7021

Addressed comments from kernel test robot.
Remove unused vairable. Add previous prototype.
Addressed make error and warning in kernel 5.18-rc1.

Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
---
Changes in v5:
 - Addressed comments from Krzysztof.
 - Remove the relevant sdcard code.
 - Submit the emmc code only.

Changes in v6:
 - Addressed comments from Arnd.
 - Use SYSTEM_SLEEP_PM_OPS instead of SET_SYSTEM_SLEEP_PM_OPS.
 - Use RUNTIME_PM_OPS instead of SET_RUNTIME_PM_OPS.
 - Remove #ifdef check.
 - Fixed mutex lock and unlock imbalance issue.

Changes in v7:
 - Addressed comments from kernel test robot.
 - Remove unused vairable.
 - Add previous prototype.
 - Addressed make error and warning in kernel 5.18-rc1.

 MAINTAINERS                    |    1 +
 drivers/mmc/host/Kconfig       |    9 +
 drivers/mmc/host/Makefile      |    1 +
 drivers/mmc/host/sunplus-mmc.c | 1183 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1194 insertions(+)
 create mode 100644 drivers/mmc/host/sunplus-mmc.c

()

On Thu, 14 Apr 2022 at 05:45, Tony Huang <tonyhuang.sunplus@gmail.com> wrote:
>
> Addressed comments from kernel test robot.
> Remove unused vairable. Add previous prototype.
> Addressed make error and warning in kernel 5.18-rc1.

I think a new driver deserves some information about the HW in the
commit message. We don't need details, but some top level information
would be good.

Can you please add this?

>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v5:
>  - Addressed comments from Krzysztof.
>  - Remove the relevant sdcard code.
>  - Submit the emmc code only.
>
> Changes in v6:
>  - Addressed comments from Arnd.
>  - Use SYSTEM_SLEEP_PM_OPS instead of SET_SYSTEM_SLEEP_PM_OPS.
>  - Use RUNTIME_PM_OPS instead of SET_RUNTIME_PM_OPS.
>  - Remove #ifdef check.
>  - Fixed mutex lock and unlock imbalance issue.
>
> Changes in v7:
>  - Addressed comments from kernel test robot.
>  - Remove unused vairable.
>  - Add previous prototype.
>  - Addressed make error and warning in kernel 5.18-rc1.
>
>  MAINTAINERS                    |    1 +
>  drivers/mmc/host/Kconfig       |    9 +
>  drivers/mmc/host/Makefile      |    1 +
>  drivers/mmc/host/sunplus-mmc.c | 1183 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1194 insertions(+)
>  create mode 100644 drivers/mmc/host/sunplus-mmc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cdd809a..2439234 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18874,6 +18874,7 @@ M:      Tony Huang <tonyhuang.sunplus@gmail.com>
>  M:     Li-hao Kuo <lhjeff911@gmail.com>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/mmc/sunplus,mmc.yaml
> +F:     drivers/mmc/host/sunplus-mmc.c
>
>  SUNPLUS OCOTP DRIVER
>  M:     Vincent Shih <vincent.sunplus@gmail.com>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index af6c3c3..d99ee3b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -14,6 +14,15 @@ config MMC_DEBUG
>           added host drivers please don't invent their private macro for
>           debugging.
>
> +config MMC_SUNPLUS
> +       tristate "Sunplus SP7021 MMC Controller"
> +       depends on ARCH_SUNPLUS || COMPILE_TEST
> +       help
> +         If you say yes here, you will get support for eMMC host interface
> +         on Sunplus SoCs.
> +
> +         If unsure, say N
> +
>  config MMC_ARMMMCI
>         tristate "ARM AMBA Multimedia Card Interface support"
>         depends on ARM_AMBA
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 4e4ceb3..ba0c6d0 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)       += sdhci-pic32.o
>  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)                += sdhci-brcmstb.o
>  obj-$(CONFIG_MMC_SDHCI_OMAP)           += sdhci-omap.o
>  obj-$(CONFIG_MMC_SDHCI_SPRD)           += sdhci-sprd.o
> +obj-$(CONFIG_MMC_SUNPLUS)              += sunplus-mmc.o
>  obj-$(CONFIG_MMC_CQHCI)                        += cqhci.o
>  cqhci-y                                        += cqhci-core.o
>  cqhci-$(CONFIG_MMC_CRYPTO)             += cqhci-crypto.o
> diff --git a/drivers/mmc/host/sunplus-mmc.c b/drivers/mmc/host/sunplus-mmc.c
> new file mode 100644
> index 0000000..9183437
> --- /dev/null
> +++ b/drivers/mmc/host/sunplus-mmc.c
> @@ -0,0 +1,1183 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Sunplus Inc.
> + * Author: Tony Huang <tonyhuang.sunplus@gmail.com>
> + * Author: Li-hao Kuo <lhjeff911@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/slot-gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +#define SPMMC_MIN_CLK                  400000
> +#define SPMMC_MAX_CLK                  52000000
> +#define SPMMC_MAX_BLK_COUNT            65536
> +#define SPMMC_MAX_TUNABLE_DLY  7
> +
> +#define SPMMC_CARD_MEDIATYPE_SRCDST_REG 0x0000
> +#define SPMMC_MEDIA_TYPE               GENMASK(2, 0)
> +#define SPMMC_DMA_SOURCE               GENMASK(6, 4)
> +#define SPMMC_DMA_DESTINATION          GENMASK(10, 8)
> +#define SPMMC_MEDIA_NONE       0
> +#define SPMMC_MEDIA_SD         6
> +#define SPMMC_MEDIA_MS         7
> +
> +#define SPMMC_SDRAM_SECTOR_0_SIZE_REG  0x0008
> +#define SPMMC_DMA_BASE_ADDR_REG                0x000C
> +#define SPMMC_HW_DMA_CTRL_REG          0x0010
> +#define SPMMC_HW_DMA_RST       BIT(9)
> +#define SPMMC_DMAIDLE          BIT(10)
> +
> +#define SPMMC_MAX_DMA_MEMORY_SECTORS   8
> +
> +#define SPMMC_SDRAM_SECTOR_1_ADDR_REG 0x0018
> +
> +#define SPMMC_SD_INT_REG       0x0088
> +#define SPMMC_SDINT_SDCMPEN    BIT(0)
> +#define SPMMC_SDINT_SDCMP      BIT(1)
> +#define SPMMC_SDINT_SDCMPCLR   BIT(2)
> +#define SPMMC_SDINT_SDIOEN     BIT(3)
> +#define SPMMC_SDINT_SDIO       BIT(4)
> +#define SPMMC_SDINT_SDIOCLR    BIT(5)
> +
> +#define SPMMC_SD_PAGE_NUM_REG  0x008C
> +
> +#define SPMMC_SD_CONFIG0_REG   0x0090
> +#define SPMMC_SD_PIO_MODE      BIT(0)
> +#define SPMMC_SD_DDR_MODE      BIT(1)
> +#define SPMMC_SD_LEN_MODE      BIT(2)
> +#define SPMMC_SD_TRANS_MODE    GENMASK(5, 4)
> +#define SPMMC_SD_AUTO_RESPONSE BIT(6)
> +#define SPMMC_SD_CMD_DUMMY     BIT(7)
> +#define SPMMC_SD_RSP_CHK_EN    BIT(8)
> +#define SPMMC_SDIO_MODE                BIT(9)
> +#define SPMMC_SD_MMC_MODE      BIT(10)
> +#define SPMMC_SD_DATA_WD       BIT(11)
> +#define SPMMC_RX4_EN           BIT(14)
> +#define SPMMC_SD_RSP_TYPE      BIT(15)
> +#define SPMMC_MMC8_EN          BIT(18)
> +#define SPMMC_CLOCK_DIVISION   GENMASK(31, 20)
> +
> +#define SPMMC_SDIO_CTRL_REG            0x0094
> +#define SPMMC_INT_MULTI_TRIG           BIT(6)
> +
> +#define SPMMC_SD_RST_REG               0x0098
> +#define SPMMC_SD_CTRL_REG              0x009C
> +#define SPMMC_NEW_COMMAND_TRIGGER      BIT(0)
> +#define SPMMC_DUMMY_CLOCK_TRIGGER      BIT(1)
> +
> +#define SPMMC_SD_STATUS_REG                                            0x00A0
> +#define SPMMC_SDSTATUS_DUMMY_READY                             BIT(0)
> +#define SPMMC_SDSTATUS_RSP_BUF_FULL                            BIT(1)
> +#define SPMMC_SDSTATUS_TX_DATA_BUF_EMPTY               BIT(2)
> +#define SPMMC_SDSTATUS_RX_DATA_BUF_FULL                        BIT(3)
> +#define SPMMC_SDSTATUS_CMD_PIN_STATUS                  BIT(4)
> +#define SPMMC_SDSTATUS_DAT0_PIN_STATUS                 BIT(5)
> +#define SPMMC_SDSTATUS_RSP_TIMEOUT                             BIT(6)
> +#define SPMMC_SDSTATUS_CARD_CRC_CHECK_TIMEOUT  BIT(7)
> +#define SPMMC_SDSTATUS_STB_TIMEOUT                             BIT(8)
> +#define SPMMC_SDSTATUS_RSP_CRC7_ERROR                  BIT(9)
> +#define SPMMC_SDSTATUS_CRC_TOKEN_CHECK_ERROR   BIT(10)
> +#define SPMMC_SDSTATUS_RDATA_CRC16_ERROR               BIT(11)
> +#define SPMMC_SDSTATUS_SUSPEND_STATE_READY             BIT(12)
> +#define SPMMC_SDSTATUS_BUSY_CYCLE                              BIT(13)
> +#define SPMMC_SDSTATUS_DAT1_PIN_STATUS                 BIT(14)
> +#define SPMMC_SDSTATUS_SD_SENSE_STATUS                 BIT(15)
> +#define SPMMC_SDSTATUS_BOOT_ACK_TIMEOUT                        BIT(16)
> +#define SPMMC_SDSTATUS_BOOT_DATA_TIMEOUT               BIT(17)
> +#define SPMMC_SDSTATUS_BOOT_ACK_ERROR                  BIT(18)
> +
> +#define SPMMC_SD_STATE_REG             0x00A4
> +#define SPMMC_CRCTOKEN_CHECK_RESULT    GENMASK(6, 4)
> +#define SPMMC_SDSTATE_ERROR            BIT(13)
> +#define SPMMC_SDSTATE_FINISH   BIT(14)
> +
> +#define SPMMC_SD_HW_STATE_REG          0x00A8
> +#define SPMMC_SD_BLOCKSIZE_REG         0x00AC
> +
> +#define SPMMC_SD_CONFIG1_REG           0x00B0
> +#define SPMMC_TX_DUMMY_NUM             GENMASK(8, 0)
> +#define SPMMC_SD_HIGH_SPEED_EN         BIT(31)
> +
> +#define SPMMC_SD_TIMING_CONFIG0_REG 0x00B4
> +#define SPMMC_SD_CLOCK_DELAY   GENMASK(2, 0)
> +#define SPMMC_SD_WRITE_DATA_DELAY      GENMASK(6, 4)
> +#define SPMMC_SD_WRITE_COMMAND_DELAY   GENMASK(10, 8)
> +#define SPMMC_SD_READ_RESPONSE_DELAY   GENMASK(14, 12)
> +#define SPMMC_SD_READ_DATA_DELAY       GENMASK(18, 16)
> +#define SPMMC_SD_READ_CRC_DELAY        GENMASK(22, 20)
> +
> +#define SPMMC_SD_PIODATATX_REG         0x00BC
> +#define SPMMC_SD_PIODATARX_REG         0x00C0
> +#define SPMMC_SD_CMDBUF0_3_REG         0x00C4
> +#define SPMMC_SD_CMDBUF4_REG           0x00C8
> +#define SPMMC_SD_RSPBUF0_3_REG         0x00CC
> +#define SPMMC_SD_RSPBUF4_5_REG         0x00D0
> +
> +#define SPMMC_MAX_RETRIES (8 * 8)
> +
> +struct spmmc_tuning_info {
> +       int enable_tuning;
> +       int need_tuning;
> +       int retried; /* how many times has been retried */
> +       u32 rd_crc_dly:3;
> +       u32 rd_dat_dly:3;
> +       u32 rd_rsp_dly:3;
> +       u32 wr_cmd_dly:3;
> +       u32 wr_dat_dly:3;
> +       u32 clk_dly:3;
> +};
> +
> +static const u8 tuning_blk_pattern_4bit[] = {
> +       0xff, 0x0f, 0xff, 0x00, 0xff, 0xcc, 0xc3, 0xcc,
> +       0xc3, 0x3c, 0xcc, 0xff, 0xfe, 0xff, 0xfe, 0xef,
> +       0xff, 0xdf, 0xff, 0xdd, 0xff, 0xfb, 0xff, 0xfb,
> +       0xbf, 0xff, 0x7f, 0xff, 0x77, 0xf7, 0xbd, 0xef,
> +       0xff, 0xf0, 0xff, 0xf0, 0x0f, 0xfc, 0xcc, 0x3c,
> +       0xcc, 0x33, 0xcc, 0xcf, 0xff, 0xef, 0xff, 0xee,
> +       0xff, 0xfd, 0xff, 0xfd, 0xdf, 0xff, 0xbf, 0xff,
> +       0xbb, 0xff, 0xf7, 0xff, 0xf7, 0x7f, 0x7b, 0xde,
> +};
> +
> +static const u8 tuning_blk_pattern_8bit[] = {
> +       0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00,
> +       0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc, 0xcc,
> +       0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff, 0xff,
> +       0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee, 0xff,
> +       0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd, 0xdd,
> +       0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff, 0xbb,
> +       0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff, 0xff,
> +       0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, 0xff,
> +       0xff, 0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00,
> +       0x00, 0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc,
> +       0xcc, 0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff,
> +       0xff, 0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee,
> +       0xff, 0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd,
> +       0xdd, 0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff,
> +       0xbb, 0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff,
> +       0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
> +};

The tuning pattern should not be needed in a host driver. Please drop them.

> +
> +enum {
> +       SPMMC_DMA_MODE = 0,
> +       SPMMC_PIO_MODE = 1,

Looks like a bool is better than enum with two values. Please convert.

> +};
> +
> +struct spmmc_host {
> +       void __iomem *base;
> +       struct clk *clk;
> +       struct reset_control *rstc;
> +       spinlock_t lock; /* Prevent races with irq handler */
> +       struct mutex mrq_lock; /* protect spmmc_set_ios() context and spmmc_request() */
> +       /* tasklet used to handle error then finish the request */
> +       struct tasklet_struct tsklet_finish_req;

We are moving away from tasklets for mmc hosts. Please use a threaded
IRQ handler instead.

> +       struct mmc_host *mmc;
> +       struct mmc_request *mrq; /* current mrq */
> +       int irq;
> +       int use_int; /* should raise irq when done? */
> +       int power_state; /* current power state: off/up/on */
> +       int ddr_enabled;
> +       int signal_voltage;
> +       int dmapio_mode;
> +       /*
> +        * for purpose of reducing context switch, only when transfer data that
> +        * length is greater than `dma_int_threshold' should use interrupt
> +        */
> +       int dma_int_threshold;
> +       struct sg_mapping_iter sg_miter; /* for pio mode to access sglist */
> +       int dma_use_int; /* should raise irq when dma done */
> +       struct spmmc_tuning_info tuning_info;
> +};
> +
> +/*
> + * wait for transaction done, return -1 if error.
> + */
> +static inline int spmmc_wait_finish(struct spmmc_host *host)
> +{
> +       /* Wait for transaction finish */
> +       unsigned long timeout = jiffies + msecs_to_jiffies(5000);

Is there a specific reason why you have chosen 5000ms for this
timeout? At least add a define for it.

> +
> +       while (!time_after(jiffies, timeout)) {
> +               if (readl(host->base + SPMMC_SD_STATE_REG) & SPMMC_SDSTATE_FINISH)
> +                       return 0;
> +               if (readl(host->base + SPMMC_SD_STATE_REG) & SPMMC_SDSTATE_ERROR)
> +                       return -EIO;
> +       }

Please replace this with readl_poll_timeout() - or with one of its variants.

> +
> +       return -ETIMEDOUT;
> +}
> +
> +static inline int spmmc_wait_sdstatus(struct spmmc_host *host, unsigned int status_bit)
> +{
> +       unsigned long timeout = jiffies + msecs_to_jiffies(5000);

Why 5000ms?

> +
> +       while (!time_after(jiffies, timeout)) {
> +               if (readl(host->base + SPMMC_SD_STATUS_REG) & status_bit)
> +                       return 0;
> +               if (readl(host->base + SPMMC_SD_STATE_REG) & SPMMC_SDSTATE_ERROR)
> +                       return -EIO;
> +       }

readl_poll_timeout()

> +
> +       return -ETIMEDOUT;
> +}
> +
> +#define spmmc_wait_rspbuf_full(host) spmmc_wait_sdstatus(host, SPMMC_SDSTATUS_RSP_BUF_FULL)
> +#define spmmc_wait_rxbuf_full(host) spmmc_wait_sdstatus(host, SPMMC_SDSTATUS_RX_DATA_BUF_FULL)
> +#define spmmc_wait_txbuf_empty(host) spmmc_wait_sdstatus(host, SPMMC_SDSTATUS_TX_DATA_BUF_EMPTY)
> +
> +static void spmmc_get_rsp(struct spmmc_host *host, struct mmc_command *cmd)
> +{
> +       u32 value0_3, value4_5;
> +
> +       if (unlikely(!(cmd->flags & MMC_RSP_PRESENT)))
> +               return;
> +       if (unlikely(cmd->flags & MMC_RSP_136)) {

I think the two "unlikely"  here are questionable. Can you explain why
they are improving things - or let's just drop them!?

> +               if (spmmc_wait_rspbuf_full(host))
> +                       return;
> +               value0_3 = readl(host->base + SPMMC_SD_RSPBUF0_3_REG);
> +               value4_5 = readl(host->base + SPMMC_SD_RSPBUF4_5_REG) & 0xffff;
> +               cmd->resp[0] = (value0_3 << 8) | (value4_5 >> 8);
> +               cmd->resp[1] = value4_5 << 24;
> +               value0_3 = readl(host->base + SPMMC_SD_RSPBUF0_3_REG);
> +               value4_5 = readl(host->base + SPMMC_SD_RSPBUF4_5_REG) & 0xffff;
> +               cmd->resp[1] |= value0_3 >> 8;
> +               cmd->resp[2] = value0_3 << 24;
> +               cmd->resp[2] |= value4_5 << 8;
> +               value0_3 = readl(host->base + SPMMC_SD_RSPBUF0_3_REG);
> +               value4_5 = readl(host->base + SPMMC_SD_RSPBUF4_5_REG) & 0xffff;
> +               cmd->resp[2] |= value0_3 >> 24;
> +               cmd->resp[3] = value0_3 << 8;
> +               cmd->resp[3] |= value4_5 >> 8;
> +       } else {
> +               if (spmmc_wait_rspbuf_full(host))
> +                       return;
> +               value0_3 = readl(host->base + SPMMC_SD_RSPBUF0_3_REG);
> +               value4_5 = readl(host->base + SPMMC_SD_RSPBUF4_5_REG) & 0xffff;
> +               cmd->resp[0] = (value0_3 << 8) | (value4_5 >> 8);
> +               cmd->resp[1] = value4_5 << 24;
> +       }
> +}
> +
> +static void spmmc_set_bus_clk(struct spmmc_host *host, int clk)
> +{
> +       unsigned int clkdiv;
> +       int f_min = host->mmc->f_min;
> +       int f_max = host->mmc->f_max;
> +       u32 value = readl(host->base + SPMMC_SD_CONFIG0_REG);
> +
> +       if (clk < f_min)
> +               clk = f_min;
> +       if (clk > f_max)
> +               clk = f_max;
> +
> +       clkdiv = (clk_get_rate(host->clk) + clk) / clk - 1;
> +       if (clkdiv > 0xfff)
> +               clkdiv = 0xfff;
> +
> +       value |= FIELD_PREP(SPMMC_CLOCK_DIVISION, clkdiv);
> +       writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +
> +       /*
> +        * In order to reduce the frequency of context switch,
> +        * if it is high speed or upper, we do not use interrupt
> +        * when send a command that without data transferring.
> +        */

This seems questionable to me. Not using IRQs, means that we will do
polling instead, right?

Polling means hogging the CPU, which certainly should be avoided,
unless it's necessary. Therefore, I suggest you drop this feature for
now. Let's instead consider this as a future improvement that can be
done on top of the initial support - if it can be proven to be better.

> +       if (clk > 25000000)
> +               host->use_int = 0;
> +       else
> +               host->use_int = 1;
> +}

[...]

> +/*
> + * select the working mode of controller: sd/sdio/emmc
> + */
> +static void spmmc_select_mode(struct spmmc_host *host)

I guess you intend to extend this to be able to set the SDIO mode too,
as a patch on top?

In any case, I suggest you rename this function to spmmc_set_
sdmmc_mode() for now, to better reflect what the function does. Then
when adding SDIO support we can change this, if needed.

> +{
> +       u32 value = readl(host->base + SPMMC_SD_CONFIG0_REG);
> +
> +       value |= SPMMC_SD_MMC_MODE;
> +       value &= ~SPMMC_SDIO_MODE;
> +       writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +}
> +
> +static void spmmc_sw_reset(struct spmmc_host *host)
> +{
> +       u32 value;
> +
> +       /*
> +        * Must reset dma operation first, or it will
> +        * be stuck on sd_state == 0x1c00 because of
> +        * a controller software reset bug
> +        */
> +       value = readl(host->base + SPMMC_HW_DMA_CTRL_REG);
> +       value |= SPMMC_DMAIDLE;
> +       writel(value, host->base + SPMMC_HW_DMA_CTRL_REG);
> +       value &= ~SPMMC_DMAIDLE;
> +       writel(value, host->base + SPMMC_HW_DMA_CTRL_REG);
> +       value = readl(host->base + SPMMC_HW_DMA_CTRL_REG);
> +       value |= SPMMC_HW_DMA_RST;
> +       writel(value, host->base + SPMMC_HW_DMA_CTRL_REG);
> +       writel(0x7, host->base + SPMMC_SD_RST_REG);
> +       while (readl(host->base + SPMMC_SD_HW_STATE_REG) & BIT(6))
> +               ;

White space.

> +}
> +
> +static void spmmc_prepare_cmd(struct spmmc_host *host, struct mmc_command *cmd)
> +{
> +       u32 value;
> +
> +       /* add start bit, according to spec, command format */
> +       value = ((cmd->opcode | 0x40) << 24) | (cmd->arg >> 8);
> +       writel(value, host->base + SPMMC_SD_CMDBUF0_3_REG);
> +       writeb(cmd->arg & 0xff, host->base + SPMMC_SD_CMDBUF4_REG);
> +
> +       /* disable interrupt if needed */
> +       value = readl(host->base + SPMMC_SD_INT_REG);
> +       value |= SPMMC_SDINT_SDCMPCLR; /* sd_cmp_clr */

The comment seems a bit superfluous, please drop it.

> +       if (likely(!host->use_int || cmd->flags & MMC_RSP_136))
> +               value &= ~SPMMC_SDINT_SDCMPEN; /* sdcmpen */

Ditto.

> +       else
> +               value |= SPMMC_SDINT_SDCMPEN;
> +
> +       writel(value, host->base + SPMMC_SD_INT_REG);
> +
> +       value = readl(host->base + SPMMC_SD_CONFIG0_REG);
> +       value &= ~SPMMC_SD_TRANS_MODE; /* sd_trans_mode */

Ditto.

> +       value |= SPMMC_SD_CMD_DUMMY; /* sdcmddummy */

Ditto.

Or, perhaps this actually deserves a comment. What is this bit for?

> +       if (likely(cmd->flags & MMC_RSP_PRESENT)) {

Again, I think using "likely" here is questionable. Please explain how
it helps - or drop it.

> +               value |= SPMMC_SD_AUTO_RESPONSE; /* sdautorsp */
> +       } else {
> +               value &= ~SPMMC_SD_AUTO_RESPONSE;
> +               writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +
> +               return;
> +       }
> +       /*
> +        * Currently, host is not capable of checking R2's CRC7,
> +        * thus, enable crc7 check only for 48 bit response commands
> +        */
> +       if (likely(cmd->flags & MMC_RSP_CRC && !(cmd->flags & MMC_RSP_136)))

Ditto.

> +               value |= SPMMC_SD_RSP_CHK_EN; /* sdrspchk_en */

Here's another comment that doesn't add anything. Please drop this and
all following similar types of comments. I will stop commenting on
them from now on.

I will also stop commenting on "likely/unlikely", as the similar
comments that have already been made, apply to the rest of the code
too.

> +       else
> +               value &= ~SPMMC_SD_RSP_CHK_EN;
> +
> +       if (unlikely(cmd->flags & MMC_RSP_136))
> +               value |= SPMMC_SD_RSP_TYPE; /* sdrsptype */
> +       else
> +               value &= ~SPMMC_SD_RSP_TYPE;
> +       writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +}
> +
> +static void spmmc_prepare_data(struct spmmc_host *host, struct mmc_data *data)
> +{
> +       u32 value, srcdst;
> +
> +       writel(data->blocks - 1, host->base + SPMMC_SD_PAGE_NUM_REG);
> +       writel(data->blksz - 1, host->base + SPMMC_SD_BLOCKSIZE_REG);
> +       value = readl(host->base + SPMMC_SD_CONFIG0_REG);
> +       if (data->flags & MMC_DATA_READ) {
> +               value |= FIELD_PREP(SPMMC_SD_TRANS_MODE, 2); /* sd_trans_mode */
> +               value &= ~SPMMC_SD_AUTO_RESPONSE; /* sdautorsp */
> +               value &= ~SPMMC_SD_CMD_DUMMY; /* sdcmddummy */
> +               srcdst = readl(host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +               srcdst |= FIELD_PREP(SPMMC_DMA_SOURCE, 0x2);
> +               srcdst |= FIELD_PREP(SPMMC_DMA_DESTINATION, 0x1);
> +               writel(srcdst, host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +       } else {
> +               value |= FIELD_PREP(SPMMC_SD_TRANS_MODE, 1);
> +               srcdst = readl(host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +               srcdst |= FIELD_PREP(SPMMC_DMA_SOURCE, 0x1);
> +               srcdst |= FIELD_PREP(SPMMC_DMA_DESTINATION, 0x2);
> +               writel(srcdst, host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +       }
> +
> +       value |= SPMMC_SD_LEN_MODE;
> +       if (likely(host->dmapio_mode == SPMMC_DMA_MODE)) {
> +               struct scatterlist *sg;
> +               dma_addr_t dma_addr;
> +               unsigned int dma_size;
> +               u32 *reg_addr;
> +               int dma_direction = data->flags & MMC_DATA_READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;

Feel free to use mmc_get_dma_dir() instead.

> +               int i, count = 1;
> +
> +               count = dma_map_sg(host->mmc->parent, data->sg, data->sg_len, dma_direction);
> +               if (unlikely(!count || count > SPMMC_MAX_DMA_MEMORY_SECTORS)) {
> +                       data->error = -EINVAL;
> +
> +                       return;
> +               }
> +               for_each_sg(data->sg, sg, count, i) {
> +                       dma_addr = sg_dma_address(sg);
> +                       dma_size = sg_dma_len(sg) / data->blksz - 1;
> +                       if (i == 0) {
> +                               writel(dma_addr, host->base + SPMMC_DMA_BASE_ADDR_REG);
> +                               writel(dma_size, host->base + SPMMC_SDRAM_SECTOR_0_SIZE_REG);
> +                       } else {
> +                               reg_addr = host->base + SPMMC_SDRAM_SECTOR_1_ADDR_REG + (i - 1) * 2;
> +                               writel(dma_addr, reg_addr);
> +                               writel(dma_size, reg_addr + 1);
> +                       }
> +               }
> +               value &= ~SPMMC_SD_PIO_MODE; /* sdpiomode */
> +               writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +               /* enable interrupt if needed */
> +               if (!host->use_int && data->blksz * data->blocks > host->dma_int_threshold) {
> +                       host->dma_use_int = 1;
> +                       value = readl(host->base + SPMMC_SD_INT_REG);
> +                       value |= SPMMC_SDINT_SDCMPEN; /* sdcmpen */
> +                       writel(value, host->base + SPMMC_SD_INT_REG);
> +               }
> +       } else {
> +               value |= SPMMC_SD_PIO_MODE;
> +               value |= SPMMC_RX4_EN; /* rx4_en */
> +               writel(value, host->base + SPMMC_SD_CONFIG0_REG);
> +       }
> +}
> +
> +static inline void spmmc_trigger_transaction(struct spmmc_host *host)
> +{
> +       u32 value = readl(host->base + SPMMC_SD_CTRL_REG);
> +
> +       value |= SPMMC_NEW_COMMAND_TRIGGER; /* trigger transaction */
> +       writel(value, host->base + SPMMC_SD_CTRL_REG);
> +}
> +
> +static void spmmc_send_stop_cmd(struct spmmc_host *host)
> +{
> +       struct mmc_command stop = {};
> +       u32 value;
> +
> +       stop.opcode = MMC_STOP_TRANSMISSION;
> +       stop.arg = 0;
> +       stop.flags = MMC_RSP_R1B;
> +       spmmc_prepare_cmd(host, &stop);
> +       value = readl(host->base + SPMMC_SD_INT_REG);
> +       value &= ~SPMMC_SDINT_SDCMPEN;
> +       value |= FIELD_PREP(SPMMC_SDINT_SDCMPEN, 0); /* sdcmpen */
> +       writel(value, host->base + SPMMC_SD_INT_REG);
> +       spmmc_trigger_transaction(host);
> +       spmmc_wait_finish(host);
> +}
> +
> +/*
> + * check if error occurred during transaction.
> + * @host -  host
> + * @mrq - the mrq
> + * @return 0 if no error otherwise the error number.

It looks a bit weird that we suddenly start documenting functions like
this. Perhaps a simple comment is sufficient?

> + */
> +static int spmmc_check_error(struct spmmc_host *host, struct mmc_request *mrq)
> +{
> +       int ret = 0;
> +       struct mmc_command *cmd = mrq->cmd;
> +       struct mmc_data *data = mrq->data;
> +
> +       u32 value = readl(host->base + SPMMC_SD_STATE_REG);
> +       u32 crc_token = FIELD_GET(SPMMC_CRCTOKEN_CHECK_RESULT, value);
> +
> +       if (unlikely(value & SPMMC_SDSTATE_ERROR)) {
> +               u32 timing_cfg0 = 0;
> +
> +               value = readl(host->base + SPMMC_SD_STATUS_REG);
> +
> +               if (host->tuning_info.enable_tuning) {
> +                       timing_cfg0 = readl(host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +                       host->tuning_info.rd_crc_dly = FIELD_GET(SPMMC_SD_READ_CRC_DELAY,
> +                                                                timing_cfg0);
> +                       host->tuning_info.rd_dat_dly = FIELD_GET(SPMMC_SD_READ_DATA_DELAY,
> +                                                                timing_cfg0);
> +                       host->tuning_info.rd_rsp_dly = FIELD_GET(SPMMC_SD_READ_RESPONSE_DELAY,
> +                                                                timing_cfg0);
> +                       host->tuning_info.wr_cmd_dly = FIELD_GET(SPMMC_SD_WRITE_COMMAND_DELAY,
> +                                                                timing_cfg0);
> +                       host->tuning_info.wr_dat_dly = FIELD_GET(SPMMC_SD_WRITE_DATA_DELAY,
> +                                                                timing_cfg0);
> +               }
> +
> +               if (value & SPMMC_SDSTATUS_RSP_TIMEOUT) {
> +                       ret = -ETIMEDOUT;
> +                       host->tuning_info.wr_cmd_dly++;
> +               } else if (value & SPMMC_SDSTATUS_RSP_CRC7_ERROR) {
> +                       ret = -EILSEQ;
> +                       host->tuning_info.rd_rsp_dly++;
> +               } else if (data) {
> +                       if ((value & SPMMC_SDSTATUS_STB_TIMEOUT)) {
> +                               ret = -ETIMEDOUT;
> +                               host->tuning_info.rd_dat_dly++;
> +                       } else if (value & SPMMC_SDSTATUS_RDATA_CRC16_ERROR) {
> +                               ret = -EILSEQ;
> +                               host->tuning_info.rd_dat_dly++;
> +                       } else if (value & SPMMC_SDSTATUS_CARD_CRC_CHECK_TIMEOUT) {
> +                               ret = -ETIMEDOUT;
> +                               host->tuning_info.rd_crc_dly++;
> +                       } else if (value & SPMMC_SDSTATUS_CRC_TOKEN_CHECK_ERROR) {
> +                               ret = -EILSEQ;
> +                               if (crc_token == 0x5)
> +                                       host->tuning_info.wr_dat_dly++;
> +                               else
> +                                       host->tuning_info.rd_crc_dly++;
> +                       }
> +               }
> +               cmd->error = ret;
> +               if (data) {
> +                       data->error = ret;
> +                       data->bytes_xfered = 0;
> +               }
> +               if (!host->tuning_info.need_tuning && host->tuning_info.enable_tuning)
> +                       cmd->retries = SPMMC_MAX_RETRIES; /* retry it */
> +               spmmc_sw_reset(host);
> +               usleep_range(5000, 5250);

Why these values? Can you please add a comment about why?

> +
> +               if (host->tuning_info.enable_tuning) {
> +                       timing_cfg0 |= FIELD_PREP(SPMMC_SD_READ_CRC_DELAY,
> +                                                      host->tuning_info.rd_crc_dly);
> +                       timing_cfg0 |= FIELD_PREP(SPMMC_SD_READ_DATA_DELAY,
> +                                                      host->tuning_info.rd_dat_dly);
> +                       timing_cfg0 |= FIELD_PREP(SPMMC_SD_READ_RESPONSE_DELAY,
> +                                                      host->tuning_info.rd_rsp_dly);
> +                       timing_cfg0 |= FIELD_PREP(SPMMC_SD_WRITE_COMMAND_DELAY,
> +                                                      host->tuning_info.wr_cmd_dly);
> +                       timing_cfg0 |= FIELD_PREP(SPMMC_SD_WRITE_DATA_DELAY,
> +                                                      host->tuning_info.wr_dat_dly);
> +                       writel(timing_cfg0, host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +               }
> +       } else if (data) {
> +               data->error = 0;
> +               data->bytes_xfered = data->blocks * data->blksz;
> +       }
> +       host->tuning_info.need_tuning = ret;
> +    /* controller will not send cmd 12 automatically if error occurred */
> +       if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK ||
> +           cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) {
> +               spmmc_send_stop_cmd(host);
> +               spmmc_sw_reset(host);
> +       }
> +
> +       return ret;
> +}
> +
> +/*
> + * the strategy is:
> + * 1. if several continuous delays are acceptable, we choose a middle one;
> + * 2. otherwise, we choose the first one.
> + */
> +static inline int __find_best_delay(u8 candidate_dly)
> +{
> +       int f, w, value;
> +
> +       if (!candidate_dly)
> +               return 0;
> +       f = ffs(candidate_dly) - 1;
> +       w = hweight8(candidate_dly);
> +       value = ((1 << w) - 1) << f;
> +       if (0xff == (value & ~candidate_dly))
> +               return (f + w / 2);
> +       else
> +               return (f);
> +}
> +
> +static inline __maybe_unused void spmmc_txdummy(struct spmmc_host *host, int count)

It's certainly unused. Please drop it.

> +{
> +       u32 value;
> +
> +       count &= 0x1ff;
> +       value = readl(host->base + SPMMC_SD_CONFIG1_REG);
> +       value |= FIELD_PREP(SPMMC_TX_DUMMY_NUM, count);
> +       writel(value, host->base + SPMMC_SD_CONFIG1_REG);
> +       value = readl(host->base + SPMMC_SD_CTRL_REG);
> +       value |= SPMMC_DUMMY_CLOCK_TRIGGER; /* trigger tx dummy */
> +       writel(value, host->base + SPMMC_SD_CTRL_REG);
> +}
> +
> +static void spmmc_xfer_data_pio(struct spmmc_host *host, struct mmc_data *data)
> +{
> +       u32 *buf; /* tx/rx 4 bytes one time in pio mode */

Please move the comments to separate lines.

> +       int data_left = data->blocks * data->blksz;
> +       int consumed, remain;
> +
> +       struct sg_mapping_iter *sg_miter = &host->sg_miter;
> +       unsigned int flags = 0;
> +
> +       if (data->flags & MMC_DATA_WRITE)
> +               flags |= SG_MITER_FROM_SG;
> +       else
> +               flags |= SG_MITER_TO_SG;
> +       sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
> +       while (data_left > 0) {
> +               consumed = 0;
> +               if (!sg_miter_next(sg_miter))
> +                       break;
> +               buf = sg_miter->addr;
> +               remain = sg_miter->length;
> +               do {
> +                       if (data->flags & MMC_DATA_WRITE) {
> +                               if (spmmc_wait_txbuf_empty(host))
> +                                       goto done;
> +                               writel(*buf, host->base + SPMMC_SD_PIODATATX_REG);
> +                       } else {
> +                               if (spmmc_wait_rxbuf_full(host))
> +                                       goto done;
> +                               *buf = readl(host->base + SPMMC_SD_PIODATARX_REG);
> +                       }
> +                       buf++;
> +                       consumed += 4;
> +                       remain -= 4;
> +               } while (remain);
> +               sg_miter->consumed = consumed;
> +               data_left -= consumed;
> +       }
> +done:
> +       sg_miter_stop(sg_miter);
> +}
> +
> +static void spmmc_controller_init(struct spmmc_host *host)
> +{
> +       u32 value;
> +       int ret = reset_control_assert(host->rstc);
> +
> +       if (!ret) {
> +               usleep_range(1000, 1250);
> +               ret = reset_control_deassert(host->rstc);
> +       }
> +
> +       value = readl(host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +       value |= FIELD_PREP(SPMMC_MEDIA_TYPE, SPMMC_MEDIA_SD);
> +       writel(value, host->base + SPMMC_CARD_MEDIATYPE_SRCDST_REG);
> +       host->signal_voltage = MMC_SIGNAL_VOLTAGE_330;

I don't see where host->signal_voltage is being used. Please drop it.

> +}
> +
> +static void spmmc_set_power_mode(struct spmmc_host *host, struct mmc_ios *ios)
> +{
> +       if (host->power_state == ios->power_mode)
> +               return;
> +
> +       switch (ios->power_mode) {
> +               /* power off->up->on */

Actually we have MMC_POWER_UNDEFINED too, which is what we start with
from the mmc core point of view.

> +       case MMC_POWER_ON:
> +               spmmc_controller_init(host);
> +               pm_runtime_get_sync(host->mmc->parent);

Why are you controlling the runtime PM from here? Is the controller
itself powering the SD/eMMC card?

In most cases we have external regulators for that, see" vmmc" and
mmc_regulator_get_supply().

> +               break;
> +       case MMC_POWER_UP:
> +               break;
> +       case MMC_POWER_OFF:
> +               pm_runtime_put(host->mmc->parent);
> +               break;
> +       }
> +       host->power_state = ios->power_mode;
> +}
> +
> +/*
> + * 1. unmap scatterlist if needed;
> + * 2. get response & check error conditions;
> + * 3. unlock host->mrq_lock
> + * 4. notify mmc layer the request is done
> + */
> +static void spmmc_finish_request(struct spmmc_host *host, struct mmc_request *mrq)
> +{
> +       struct mmc_command *cmd;
> +       struct mmc_data *data;
> +
> +       if (!mrq)
> +               return;
> +
> +       cmd = mrq->cmd;
> +       data = mrq->data;
> +
> +       if (data && SPMMC_DMA_MODE == host->dmapio_mode) {
> +               int dma_direction = data->flags & MMC_DATA_READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;

mmc_get_dma_dir()

> +
> +               dma_unmap_sg(host->mmc->parent, data->sg, data->sg_len, dma_direction);
> +               host->dma_use_int = 0;
> +       }
> +       spmmc_get_rsp(host, cmd);
> +       spmmc_check_error(host, mrq);
> +       host->mrq = NULL;
> +       mmc_request_done(host->mmc, mrq);
> +}
> +
> +/* Interrupt Service Routine */
> +static irqreturn_t spmmc_irq(int irq, void *dev_id)
> +{
> +       struct spmmc_host *host = dev_id;
> +       u32 value = readl(host->base + SPMMC_SD_INT_REG);
> +
> +       spin_lock(&host->lock);
> +       if ((value & SPMMC_SDINT_SDCMP) && (value & SPMMC_SDINT_SDCMPEN)) {
> +               value &= ~SPMMC_SDINT_SDCMPEN; /* disable sdcmp */
> +               value |= SPMMC_SDINT_SDCMPCLR; /* sd_cmp_clr */
> +               writel(value, host->base + SPMMC_SD_INT_REG);
> +               /*
> +                * if error occurred, we my need send cmd 12 to stop data transaction,
> +                * which is time consuming, so make use of tasklet to handle this.
> +                */
> +               if (unlikely(readl(host->base + SPMMC_SD_STATE_REG) & SPMMC_SDSTATE_ERROR))
> +                       tasklet_schedule(&host->tsklet_finish_req);

As I stated earlier, please use a threaded IRQ instead.

> +               else
> +                       spmmc_finish_request(host, host->mrq);
> +       }
> +       if (value & SPMMC_SDINT_SDIO &&
> +           (value & SPMMC_SDINT_SDIOEN)) {
> +               mmc_signal_sdio_irq(host->mmc);

I was under the impression that SDIO support was going to be added on
top. I suggest we drop this for now.

Moreover, mmc_signal_sdio_irq() is the legacy interface. Please use
sdio_signal_irq() instead, which also comes along with a few new host
callbacks that need to be implemented.

> +       }
> +       spin_unlock(&host->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +

[...]

> +/*
> + * Return values for the get_cd callback should be:
> + *   0 for a absent card
> + *   1 for a present card
> + *   -ENOSYS when not supported (equal to NULL callback)
> + *   or a negative errno value when something bad happened
> + */
> +static int spmmc_get_cd(struct mmc_host *mmc)
> +{
> +       int ret = 0;
> +
> +       if (mmc_can_gpio_cd(mmc))
> +               ret = mmc_gpio_get_cd(mmc);
> +
> +       if (ret < 0)
> +               ret = 0;
> +
> +       return ret;
> +}
> +
> +static int spmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +       struct spmmc_host *host = mmc_priv(mmc);
> +       const u8 *blk_pattern;
> +       u8 *blk_test;
> +       int blksz;
> +       u8 smpl_dly = 0, candidate_dly = 0;
> +       u32 value;
> +
> +       if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
> +               blk_pattern = tuning_blk_pattern_8bit;
> +               blksz = sizeof(tuning_blk_pattern_8bit);
> +       } else if (mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
> +               blk_pattern = tuning_blk_pattern_4bit;
> +               blksz = sizeof(tuning_blk_pattern_4bit);
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       blk_test = kmalloc(blksz, GFP_KERNEL);
> +       if (!blk_test)
> +               return -ENOMEM;
> +
> +       host->tuning_info.enable_tuning = 0;
> +       do {
> +               struct mmc_request mrq = {NULL};
> +               struct mmc_command cmd = {0};
> +               struct mmc_command stop = {0};
> +               struct mmc_data data = {0};
> +               struct scatterlist sg;
> +
> +               cmd.opcode = opcode;
> +               cmd.arg = 0;
> +               cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +               stop.opcode = MMC_STOP_TRANSMISSION;
> +               stop.arg = 0;
> +               stop.flags = MMC_RSP_R1B | MMC_CMD_AC;
> +
> +               data.blksz = blksz;
> +               data.blocks = 1;
> +               data.flags = MMC_DATA_READ;
> +               data.sg = &sg;
> +               data.sg_len = 1;
> +
> +               sg_init_one(&sg, blk_test, blksz);
> +               mrq.cmd = &cmd;
> +               mrq.stop = &stop;
> +               mrq.data = &data;
> +               host->mrq = &mrq;

Please convert this code into using mmc_send_tuning() and
mmc_send_abort_tuning() instead.

> +
> +               value = readl(host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +               value |= FIELD_PREP(SPMMC_SD_READ_RESPONSE_DELAY, smpl_dly);
> +               value |= FIELD_PREP(SPMMC_SD_READ_DATA_DELAY, smpl_dly);
> +               value |= FIELD_PREP(SPMMC_SD_READ_CRC_DELAY, smpl_dly);
> +               writel(value, host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +
> +               mmc_wait_for_req(mmc, &mrq);
> +               if (!cmd.error && !data.error) {
> +                       if (!memcmp(blk_pattern, blk_test, blksz))
> +                               candidate_dly |= (1 << smpl_dly);
> +               }
> +       } while (smpl_dly++ <= SPMMC_MAX_TUNABLE_DLY);
> +       host->tuning_info.enable_tuning = 1;
> +
> +       if (candidate_dly) {
> +               smpl_dly = __find_best_delay(candidate_dly);
> +               value = readl(host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +               value |= FIELD_PREP(SPMMC_SD_READ_RESPONSE_DELAY, smpl_dly);
> +               value |= FIELD_PREP(SPMMC_SD_READ_DATA_DELAY, smpl_dly);
> +               value |= FIELD_PREP(SPMMC_SD_READ_CRC_DELAY, smpl_dly);
> +               writel(value, host->base + SPMMC_SD_TIMING_CONFIG0_REG);
> +               return 0;
> +       }
> +
> +       return -EIO;
> +}
> +
> +static void spmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)

Let's drop SDIO support for now.

> +{
> +       struct spmmc_host *host = mmc_priv(mmc);
> +       u32 value = readl(host->base + SPMMC_SD_INT_REG);
> +
> +       value &= ~SPMMC_SDINT_SDIOCLR;
> +       value |= FIELD_PREP(SPMMC_SDINT_SDIOCLR, 1); /* sdio_int_clr */
> +       if (enable)
> +               value |= SPMMC_SDINT_SDIOEN;
> +       else
> +               value &= ~SPMMC_SDINT_SDIOEN;
> +       writel(value, host->base + SPMMC_SD_INT_REG);
> +}
> +

[...]

> +
> +static int spmmc_drv_probe(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc;
> +       struct resource *res;
> +       struct spmmc_host *host;
> +       int ret = 0;
> +
> +       mmc = mmc_alloc_host(sizeof(*host), &pdev->dev);
> +       if (!mmc) {
> +               ret = -ENOMEM;
> +               goto probe_free_host;
> +       }
> +
> +       host = mmc_priv(mmc);
> +       host->mmc = mmc;
> +       host->power_state = MMC_POWER_OFF;
> +       host->dma_int_threshold = 1024;
> +       host->dmapio_mode = SPMMC_DMA_MODE;
> +
> +       host->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +       if (IS_ERR(host->base))
> +               return PTR_ERR(host->base);
> +
> +       host->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(host->clk))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(host->clk), "clk get fail\n");
> +
> +       host->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       if (IS_ERR(host->rstc))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(host->rstc), "rst get fail\n");
> +
> +       host->irq = platform_get_irq(pdev, 0);
> +       if (host->irq <= 0)
> +               return host->irq;
> +
> +       ret = devm_request_irq(&pdev->dev, host->irq, spmmc_irq, IRQF_SHARED,
> +                              dev_name(&pdev->dev), host);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare_enable(host->clk);
> +       if (ret)
> +               return dev_err_probe(&pdev->dev, ret, "failed to enable clk\n");
> +
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto probe_free_host;
> +
> +       spin_lock_init(&host->lock);
> +       mutex_init(&host->mrq_lock);
> +       tasklet_init(&host->tsklet_finish_req, tsklet_func_finish_req, (unsigned long)host);
> +       mmc->ops = &spmmc_ops;
> +       mmc->f_min = SPMMC_MIN_CLK;
> +       if (mmc->f_max > SPMMC_MAX_CLK)
> +               mmc->f_max = SPMMC_MAX_CLK;
> +
> +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;

Please use mmc_regulator_get_supply() to get the ocr_avail mask
instead of hardcoding it.

> +       mmc->max_seg_size = SPMMC_MAX_BLK_COUNT * 512;
> +       /* Host controller supports up to "SPMMC_MAX_DMA_MEMORY_SECTORS"*/
> +       /* a.k.a. max scattered memory segments per request*/
> +       mmc->max_segs = SPMMC_MAX_DMA_MEMORY_SECTORS;
> +       mmc->max_req_size = SPMMC_MAX_BLK_COUNT * 512;
> +       /* Limited by the max value of dma_size & data_length, set it to 512 bytes for now */
> +       mmc->max_blk_size = 512;
> +       mmc->max_blk_count = SPMMC_MAX_BLK_COUNT; /* Limited by sd_page_num */

Please re-format the code and comment above so it becomes it more
readable. Perhaps a few newlines would help.

> +
> +       dev_set_drvdata(&pdev->dev, host);
> +       spmmc_controller_init(host);
> +       spmmc_select_mode(host);
> +       mmc_add_host(mmc);
> +       host->tuning_info.enable_tuning = 1;
> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);

To be safe, I suggest you call mmc_add_host() as the last thing in
->probe(). Otherwise you end up enabling runtime PM, when the host is
already up and running from the core point of view.

> +
> +       return ret;
> +
> +probe_free_host:
> +       if (mmc)
> +               mmc_free_host(mmc);
> +
> +       return ret;
> +}
> +
> +static int spmmc_drv_remove(struct platform_device *dev)
> +{
> +       struct spmmc_host *host = platform_get_drvdata(dev);
> +
> +       mmc_remove_host(host->mmc);
> +       clk_disable(host->clk);
> +       clk_unprepare(host->clk);

clk_disable_unprepare().

> +       pm_runtime_disable(&dev->dev);
> +       platform_set_drvdata(dev, NULL);
> +       mmc_free_host(host->mmc);
> +
> +       return 0;
> +}
> +
> +static int spmmc_drv_suspend(struct platform_device *dev, pm_message_t state)

This is a legacy PM callback, please drop it.

> +{
> +       struct spmmc_host *host;
> +
> +       host = platform_get_drvdata(dev);
> +       mutex_lock(&host->mrq_lock); /* Make sure that no one is holding the controller */
> +       mutex_unlock(&host->mrq_lock);
> +       clk_disable(host->clk);
> +
> +       return 0;
> +}
> +
> +static int spmmc_drv_resume(struct platform_device *dev)

Ditto.

> +{
> +       struct spmmc_host *host;
> +
> +       host = platform_get_drvdata(dev);
> +
> +       return clk_enable(host->clk);
> +}
> +
> +static int spmmc_pm_suspend(struct device *dev)
> +{
> +       pm_runtime_force_suspend(dev);

Just point the PM callback to pm_runtime_force_suspend() instead of
adding an unessary wrapper function.

 > +
> +       return 0;
> +}
> +
> +static int spmmc_pm_resume(struct device *dev)
> +{
> +       pm_runtime_force_resume(dev);

Just point the PM callback to pm_runtime_force_resume() instead of
adding an unessary wrapper function.

> +
> +       return 0;
> +}
> +
> +static int spmmc_pm_runtime_suspend(struct device *dev)

You proably need __maybe_unused here.

> +{
> +       struct spmmc_host *host;
> +
> +       host = dev_get_drvdata(dev);
> +       clk_disable(host->clk);

clk_disable_unprepare()

> +
> +       return 0;
> +}
> +
> +static int spmmc_pm_runtime_resume(struct device *dev)

You proably need __maybe_unused here.

> +{
> +       struct spmmc_host *host;
> +
> +       host = dev_get_drvdata(dev);
> +
> +       return clk_enable(host->clk);

clk_prepare_enable().

> +}
> +
> +static const struct dev_pm_ops spmmc_pm_ops = {
> +       SYSTEM_SLEEP_PM_OPS(spmmc_pm_suspend, spmmc_pm_resume)
> +       RUNTIME_PM_OPS(spmmc_pm_runtime_suspend, spmmc_pm_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id spmmc_of_table[] = {
> +       {
> +               .compatible = "sunplus,sp7021-mmc",
> +       },
> +       {/* sentinel */}
> +};
> +MODULE_DEVICE_TABLE(of, spmmc_of_table);
> +
> +static struct platform_driver spmmc_driver = {
> +       .probe = spmmc_drv_probe,
> +       .remove = spmmc_drv_remove,
> +       .suspend = spmmc_drv_suspend,
> +       .resume = spmmc_drv_resume,
> +       .driver = {
> +               .name = "spmmc",
> +               .pm = pm_ptr(&spmmc_pm_ops),
> +               .of_match_table = spmmc_of_table,
> +       },
> +};
> +module_platform_driver(spmmc_driver);
> +
> +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@gmail.com>");
> +MODULE_AUTHOR("Li-hao Kuo <lhjeff911@gmail.com>");
> +MODULE_DESCRIPTION("Sunplus MMC controller driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>

Kind regards
Uffe