| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b4b7219c2b6430b9c320c8d9ac1cc8b@realtek.com>
Date: Tue, 5 Dec 2023 09:19:09 +0000
From: Jyan Chou [周芷安] <jyanchou@...ltek.com>
To: Adrian Hunter <adrian.hunter@...el.com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"jh80.chung@...sung.com" <jh80.chung@...sung.com>,
"riteshh@...eaurora.org" <riteshh@...eaurora.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>
CC: "conor+dt@...nel.org" <conor+dt@...nel.org>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"arnd@...db.de" <arnd@...db.de>,
"briannorris@...omium.org" <briannorris@...omium.org>,
"doug@...morgal.com" <doug@...morgal.com>,
"tonyhuang.sunplus@...il.com" <tonyhuang.sunplus@...il.com>,
"abel.vesa@...aro.org" <abel.vesa@...aro.org>,
"william.qiu@...rfivetech.com" <william.qiu@...rfivetech.com>
Subject: RE: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver
> 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;
> > +}
> > +
>
Powered by blists - more mailing lists