RE: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver

["Jyan Chou [周芷安]" <jyanchou@realtek.com>]

> Some comments below wrt cqhci
> 
> > —--
> > v6 -> v7:
> > - Remove reset-names in driver and adjust reset control's code.
> >
> > v5 -> v6:
> > - Fix linux coding style issues.
> > - Drop useless code that is not described in the bindings.
> > - Replace devm_clk_get and clk_prepare_enable with
> devm_clk_get_enabled.
> > - Replace EXPORT_SYMBOL with EXPORT_SYMBOL_GPL.
> >
> > v4 -> v5:
> > - Fix linux coding style issues.
> > - Fix test robot build errors to make good use of setup_tran_desc
> >   call back function.
> > - Remove useless function.
> >
> > v3 -> v4:
> > - Modify dma mode selection and dma addressing bit to statisfy
> >   linux coding style.
> >
> > v1 -> v2:
> > - Remove dw_mci_cqe_set_tran_desc due to the duplicated function.
> > - Add ->pre_enable() / ->post_disable()
> >
> > v0 -> v1:
> > - Seperate different support into single patch.
> > - Fix the compiler complains.
> > ---
> > ---
> >  drivers/mmc/host/Kconfig      |   13 +
> >  drivers/mmc/host/Makefile     |    1 +
> >  drivers/mmc/host/dw_mmc_cqe.c | 1467
> > +++++++++++++++++++++++++++++++++
> drivers/mmc/host/dw_mmc_cqe.h |
> > 456 ++++++++++
> >  4 files changed, 1937 insertions(+)
> >  create mode 100644 drivers/mmc/host/dw_mmc_cqe.c  create mode
> 100644
> > drivers/mmc/host/dw_mmc_cqe.h
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index
> > 58bd5fe4cd25..06bb4de28cc4 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -837,6 +837,19 @@ config MMC_DW_STARFIVE
> >         Synopsys DesignWare Memory Card Interface driver. Select this
> option
> >         for platforms based on StarFive JH7110 SoC.
> >
> > +config MMC_DW_CQE
> > +     tristate "Synopsys DesignWare Memory Card with CQE Interface"
> > +     depends on ARC || ARM || ARM64 || MIPS || COMPILE_TEST
> > +     select MMC_CQHCI
> > +     help
> > +      This selects support for the Synopsys DesignWare Mobile Storage IP
> > +      block after JEDEC Standard version 5.1. Select this option for SD and
> > +      MMC interfaces that use command queue.
> > +
> > +      If you have a controller with this interface, say Y or M here.
> > +
> > +      If unsure, say Y.
> > +
> >  config MMC_SH_MMCIF
> >       tristate "SuperH Internal MMCIF support"
> >       depends on SUPERH || ARCH_RENESAS || COMPILE_TEST diff --git
> > a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index
> > d0be4465f3ec..464fe58f8541 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)             +=
> dw_mmc-k3.o
> >  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
> >  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
> >  obj-$(CONFIG_MMC_DW_STARFIVE)        += dw_mmc-starfive.o
> > +obj-$(CONFIG_MMC_DW_CQE)     += dw_mmc_cqe.o
> >  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
> >  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
> >  obj-$(CONFIG_MMC_VUB300)     += vub300.o
> > diff --git a/drivers/mmc/host/dw_mmc_cqe.c
> > b/drivers/mmc/host/dw_mmc_cqe.c new file mode 100644 index
> > 000000000000..eb00d6a474b2
> > --- /dev/null
> > +++ b/drivers/mmc/host/dw_mmc_cqe.c
> > @@ -0,0 +1,1467 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Synopsys DesignWare Multimedia Card Interface driver with CMDQ
> > +support
> > + *  (Based on Synopsys DesignWare Multimedia Card Interface driver)
> > + *
> > + * Copyright (c) 2023 Realtek Semiconductor Corp  */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/ioport.h>
> > +#include <linux/irq.h>
> > +#include <linux/mmc/card.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd.h>
> > +#include <linux/mmc/sdio.h>
> > +#include <linux/mmc/slot-gpio.h>
> > +#include <linux/module.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h> #include <linux/seq_file.h>
> > +#include <linux/sizes.h> #include <linux/slab.h> #include
> > +<linux/stat.h>
> > +
> > +#include "dw_mmc_cqe.h"
> > +#include "cqhci.h"
> > +
> > +#define DW_MCI_FREQ_MAX      200000000       /* unit: HZ */
> > +#define DW_MCI_FREQ_MIN      100000          /* unit: HZ */
> > +#define DW_MCI_CMDQ_DISABLED 0x30f0001 #define
> DW_MCI_CMDQ_ENABLED
> > +0x30f0101
> > +#define DW_MCI_POWEROFF              0x3220301
> > +#define DW_MCI_DESC_LEN              0x100000
> > +#define DW_MCI_MAX_SCRIPT_BLK        128
> > +#define DW_MCI_TIMEOUT_Ms    200
> > +#define DW_MCI_TIMEOUT               200000
> > +#define TUNING_ERR           531
> 
> Could just use EIO
> 

Okay, we will correct it in our new version. 

> > +#define DW_MCI_NOT_READY     9999
> > +
> > +DECLARE_COMPLETION(dw_mci_wait);
> > +
> > +static int dw_mci_cqe_regs_show(struct dw_mci *host,
> > +                             struct mmc_command *cmd, u32
> cmd_flags)
> > +{
> > +     dev_info(host->dev, "opcode = %d, arg = 0x%x, cmdflags = 0x%x\n",
> > +              cmd->opcode, cmd->arg, cmd_flags);
> > +     dev_info(host->dev, "status_int = 0x%x\n", host->normal_interrupt);
> > +     dev_info(host->dev, "error_int = 0x%x\n", host->error_interrupt);
> > +     dev_info(host->dev, "auto_error_int = 0x%x\n",
> > +host->auto_error_interrupt);
> > +
> > +     return 0;
> > +}
> > +
> > +static void dw_mci_cqe_dumpregs(struct mmc_host *mmc) {
> > +     struct dw_mci_slot *slot = mmc_priv(mmc);
> > +     struct dw_mci *host = slot->host;
> > +
> > +     dev_info(host->dev, "%s: cmd idx 0x%08x\n", __func__,
> > +mcq_readw(host, CMD_R)); }
> > +
> > +static void dw_mci_cqe_setup_tran_desc(struct mmc_data *data,
> > +                                    struct cqhci_host *cq_host,
> > +                                     u8 *desc,
> > +                                     int sg_count) {
> > +     struct scatterlist *sg;
> > +     u32 cur_blk_cnt, remain_blk_cnt;
> > +     unsigned int begin, end;
> > +     int i, len;
> > +     bool last = false;
> > +     bool dma64 = cq_host->dma64;
> > +     dma_addr_t addr;
> > +
> > +     for_each_sg(data->sg, sg, sg_count, i) {
> > +             addr = sg_dma_address(sg);
> > +             len = sg_dma_len(sg);
> > +             remain_blk_cnt  = len >> 9;
> > +
> > +             while (remain_blk_cnt) {
> > +                     if (remain_blk_cnt > DW_MCI_MAX_SCRIPT_BLK)
> 
> Use max_seg_size then that won't happen
> 

We will remove this useless check, thanks.

> > +                             cur_blk_cnt =
> DW_MCI_MAX_SCRIPT_BLK;
> > +                     else
> > +                             cur_blk_cnt = remain_blk_cnt;
> > +
> > +                     begin = addr / SZ_128M;
> > +                     end = (addr + cur_blk_cnt * SZ_512) / SZ_128M;
> > +
> > +                     if (begin != end)
> > +                             cur_blk_cnt = (end * SZ_128M - addr) /
> > + SZ_512;
> > +
> > +                     if ((i + 1) == sg_count && remain_blk_cnt ==
> cur_blk_cnt)
> > +                             last = true;
> > +
> > +                     cqhci_set_tran_desc(desc, addr,
> > +                                         (cur_blk_cnt << 9), last,
> > + dma64);
> > +
> > +                     addr = addr + (cur_blk_cnt << 9);
> > +                     remain_blk_cnt -= cur_blk_cnt;
> > +                     desc += cq_host->trans_desc_len;
> > +             }
> > +     }
> > +}
> 
> Provide a hook for cqhci_set_tran_desc() instead of cqhci_prep_tran_desc()
> You'll need to check the details, but something like:
> 
> 
> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
> index b3d7d6d8d654..98e7e9d3030d 100644
> --- a/drivers/mmc/host/cqhci-core.c
> +++ b/drivers/mmc/host/cqhci-core.c
> @@ -522,7 +522,10 @@ static int cqhci_prep_tran_desc(struct mmc_request
> *mrq,
> 
>                 if ((i+1) == sg_count)
>                         end = true;
> -               cqhci_set_tran_desc(desc, addr, len, end, dma64);
> +               if (cq_host->ops->set_tran_desc)
> +                       cq_host->ops->set_tran_desc(&desc, addr, len,
> end, dma64);
> +               else
> +                       cqhci_set_tran_desc(desc, addr, len, end,
> + dma64);
>                 desc += cq_host->trans_desc_len;
>         }
> 
> And:
> 
> #define BOUNDARY_OK(addr, len) \
>         ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> 
> 
> static void dw_mci_cqe_set_tran_desc(u8 **desc, dma_addr_t addr, int len,
> bool end, bool dma64) {
>         int tmplen, offset;
> 
>         if (likely(!len || BOUNDARY_OK(addr, len))) {
>                 cqhci_set_tran_desc(*desc, addr, len, end, dma64);
>                 return;
>         }
> 
>         offset = addr & (SZ_128M - 1);
>         tmplen = SZ_128M - offset;
>         cqhci_set_tran_desc(*desc, addr, tmplen, false, dma64);
> 
>         addr += tmplen;
>         len -= tmplen;
>         *desc += cq_host->trans_desc_len;
>         cqhci_set_tran_desc(*desc, addr, len, end, dma64); }
>  

Thanks, we will refactor this part to a more accurately way.

> > +
> > +static void dw_mci_cqe_enable(struct mmc_host *mmc) {
> > +     struct dw_mci_slot *slot = mmc_priv(mmc);
> > +     struct dw_mci *host = slot->host;
> > +
> > +     mcq_writeb(host, SW_RST_R, SDMMC_RST_DAT);
> > +     mcq_writew(host, XFER_MODE_R,
> > +                ((1 << SDMMC_MULTI_BLK_SEL) |
> SDMMC_BLOCK_COUNT_ENABLE
> > +                     | SDMMC_DMA_ENABLE));
> > +
> > +     mcq_writeb(host, HOST_CTRL1_R,
> > +                (mcq_readb(host, HOST_CTRL1_R) & 0xe7) |
> > +                     (SDMMC_ADMA2_32 << SDMMC_DMA_SEL));
> > +     mcq_writew(host, BLOCKSIZE_R, 0x200);
> > +     mcq_writew(host, BLOCKCOUNT_R, 0);
> > +
> > +     mcq_writel(host, SDMASA_R, 0);
> > +
> > +     cqhci_writel(host->cqe, 0x10, CQHCI_SSC1);
> > +     cqhci_writel(host->cqe, 0, CQHCI_CTL);
> > +
> > +     if (cqhci_readl(host->cqe, CQHCI_CTL) && CQHCI_HALT) {
> > +             dev_err(host->dev, "%s: cqhci: CQE failed to exit halt
> state\n",
> > +                     mmc_hostname(mmc));
> > +     }
> > +
> > +     dw_mci_clr_signal_int(host);
> > +     dw_mci_en_cqe_int(host);
> > +}
> > +
> > +static void dw_mci_cqe_pre_enable(struct mmc_host *mmc) {
> > +     struct cqhci_host *cq_host = mmc->cqe_private;
> > +     u32 reg;
> > +
> > +     reg = cqhci_readl(cq_host, CQHCI_CFG);
> > +     reg |= CQHCI_ENABLE;
> > +     cqhci_writel(cq_host, reg, CQHCI_CFG); }
> > +
> > +static void dw_mci_cqe_post_disable(struct mmc_host *mmc) {
> > +     struct cqhci_host *cq_host = mmc->cqe_private;
> > +     u32 reg;
> > +
> > +     reg = cqhci_readl(cq_host, CQHCI_CFG);
> > +     reg &= ~CQHCI_ENABLE;
> > +     cqhci_writel(cq_host, reg, CQHCI_CFG); }
> > +
> > +static const struct cqhci_host_ops dw_mci_cqhci_host_ops = {
> > +     .enable = dw_mci_cqe_enable,
> > +     .dumpregs = dw_mci_cqe_dumpregs,
> > +     .pre_enable = dw_mci_cqe_pre_enable,
> > +     .post_disable = dw_mci_cqe_post_disable,
> > +     .setup_tran_desc = dw_mci_cqe_setup_tran_desc, };
> > +
> 
> [SNIP]
> 
> > +
> > +static irqreturn_t dw_mci_cqe_interrupt(int irq, void *dev_id) {
> > +     struct dw_mci *host = dev_id;
> > +     struct mmc_host *mmc = host->slot->mmc;
> > +     struct cqhci_host *cq_host = NULL;
> > +     int cmd_error = 0, data_error = 0;
> > +
> > +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE))
> > +             cq_host = mmc->cqe_private;
> > +
> > +     dw_mci_get_int(host);
> > +
> > +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> > +             if (!mmc->cqe_on && !cq_host->activated)
> 
> Shouldn't really look at internals like mmc->cqe_on or cq_host->activated.
> There are the cqhci_host_ops ->enable() and ->disable() callbacks to keep track
> of whether cqhci is expecting interrupts.

Does this means we need to use cqhci_host_ops ->enable() and ->disable() callbacks
instead of mmc->cqe_on && !cq_host->activated? Thanks.

> 
> > +                     dw_mci_clr_signal_int(host);
> > +     } else {
> > +             dw_mci_clr_signal_int(host);
> > +     }
> > +
> > +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE) &&
> > +         mmc->cqe_on && cq_host->activated) {
> 
> As above
> 
> > +             if (host->normal_interrupt & SDMMC_ERR_INTERRUPT) {
> > +                     dev_err(host->dev, "cmdq error: interrupt
> status=%08x, error interrupt=0x%08x, CQIS=0x%x, CQTCN=0x%x\n",
> > +                             host->normal_interrupt,
> host->error_interrupt,
> > +                             readl(host->cqe->mmio + CQHCI_IS),
> > +                             readl(host->cqe->mmio + CQHCI_TCN));
> > +
> > +                     dw_mci_cqe_command_complete(host,
> host->error_interrupt, &cmd_error);
> > +                     dw_mci_cqe_data_complete(host,
> host->error_interrupt, &data_error);
> > +             }
> > +             cqhci_irq(mmc, (u32)(host->normal_interrupt), cmd_error,
> data_error);
> > +             dw_mci_clr_int(host);
> > +
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     if (host->int_waiting) {
> > +             del_timer(&host->timer);
> > +             complete(host->int_waiting);
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
>