2018-06-08 08:19:59

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 0/6] mmc: add support for sdhci 4.0

From the SD host controller version 4.0 on, SDHCI implementation either
is version 3 compatible or version 4 mode. This patch-set covers those
changes which are common for SDHCI 4.0 version, regardless of whether
they are used with SD or eMMC storage devices.

This patchset also added a new sdhci driver for Spreadtrum's controller
which supports v4.0 mode.

This patchset has been tested on Spreadtrum's mobile phone, emmc can be
initialized, mounted, read and written, with these changes for common
sdhci framework and sdhci-sprd driver.

Chunyan Zhang (6):
mmc: sdhci: add sd host v4 mode
mmc: sdhci: made changes for System Address register of SDMA
mmc: sdhci: add ADMA2 64-bit addressing support for V4 mode
mmc: sdhci: add 32-bit block count support for v4 mode
mmc: sdhci: add CMD23 support for v4 mode
mmc: host: sdhci-sprd: added Spreadtrum's host controller R11

drivers/mmc/host/Kconfig | 13 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-sprd-r11.c | 472 ++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 85 +++++--
drivers/mmc/host/sdhci.h | 31 ++-
5 files changed, 580 insertions(+), 22 deletions(-)
create mode 100644 drivers/mmc/host/sdhci-sprd-r11.c

--
2.7.4



2018-06-08 08:20:14

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 2/6] mmc: sdhci: made changes for System Address register of SDMA

According to the SD host controller specification version 4.10, when
Host Version 4 is enabled, SDMA uses ADMA System Address register
(05Fh-058h) instead of using SDMA System Address register to
support both 32-bit and 64-bit addressing.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cf5695f..f57201f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -805,6 +805,7 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
{
u8 ctrl;
+ u32 reg;
struct mmc_data *data = cmd->data;

if (sdhci_data_line_cmd(cmd))
@@ -894,8 +895,10 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
SDHCI_ADMA_ADDRESS_HI);
} else {
WARN_ON(sg_cnt != 1);
+ reg = host->v4_mode ? SDHCI_ADMA_ADDRESS :
+ SDHCI_DMA_ADDRESS;
sdhci_writel(host, sdhci_sdma_address(host),
- SDHCI_DMA_ADDRESS);
+ reg);
}
}

@@ -2721,6 +2724,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
*/
if (intmask & SDHCI_INT_DMA_END) {
u32 dmastart, dmanow;
+ u32 reg;

dmastart = sdhci_sdma_address(host);
dmanow = dmastart + host->data->bytes_xfered;
@@ -2733,7 +2737,9 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
host->data->bytes_xfered = dmanow - dmastart;
DBG("DMA base 0x%08x, transferred 0x%06x bytes, next 0x%08x\n",
dmastart, host->data->bytes_xfered, dmanow);
- sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
+ reg = host->v4_mode ? SDHCI_ADMA_ADDRESS :
+ SDHCI_DMA_ADDRESS;
+ sdhci_writel(host, dmanow, reg);
}

if (intmask & SDHCI_INT_DATA_END) {
--
2.7.4


2018-06-08 08:20:28

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 4/6] mmc: sdhci: add 32-bit block count support for v4 mode

When Host Version 4 is enabled, SDMA System Address register is
re-defined as 32-bit Block Count, and SDMA uses ADMA System
Address register (05Fh-058h) instead.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 3 ++-
drivers/mmc/host/sdhci.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5d3b0d8..b8ee124 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -943,7 +943,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
/* Set the DMA boundary value and block size */
sdhci_writew(host, SDHCI_MAKE_BLKSZ(host->sdma_boundary, data->blksz),
SDHCI_BLOCK_SIZE);
- sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
+ reg = host->v4_mode ? SDHCI_32BIT_BLK_CNT : SDHCI_BLOCK_COUNT;
+ sdhci_writew(host, data->blocks, reg);
}

static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 820a863..1e84539 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -28,6 +28,7 @@

#define SDHCI_DMA_ADDRESS 0x00
#define SDHCI_ARGUMENT2 SDHCI_DMA_ADDRESS
+#define SDHCI_32BIT_BLK_CNT SDHCI_DMA_ADDRESS

#define SDHCI_BLOCK_SIZE 0x04
#define SDHCI_MAKE_BLKSZ(dma, blksz) (((dma & 0x7) << 12) | (blksz & 0xFFF))
--
2.7.4


2018-06-08 08:20:35

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 6/6] mmc: host: sdhci-sprd: added Spreadtrum's host controller R11

From: Chunyan Zhang <[email protected]>

This patch adds the initial support of Secure Digital Host Controller
Interface compliant controller - R11 found in some latest Spreadtrum
chipsets.

R11 is a variant based on SD v4.0 specification.

With this driver, mmc can be initialized, can be mounted, read and
written.

Original-by: Billows Wu <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/Kconfig | 13 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-sprd-r11.c | 472 ++++++++++++++++++++++++++++++++++++++
3 files changed, 486 insertions(+)
create mode 100644 drivers/mmc/host/sdhci-sprd-r11.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..563baf5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -584,6 +584,19 @@ config MMC_SDRICOH_CS
To compile this driver as a module, choose M here: the
module will be called sdricoh_cs.

+config MMC_SDHCI_SPRD_R11
+ tristate "Spreadtrum SDIO host Controller(IP version: R11P0)"
+ depends on ARCH_SPRD
+ depends on MMC_SDHCI_PLTFM
+ select MMC_SDHCI_IO_ACCESSORS
+ help
+ This selects the SDIO Host Controller(R11P0) in Spreadtrum
+ SoCs.
+
+ If you have a controller with this interface, say Y or M here.
+
+ If unsure, say N.
+
config MMC_TMIO_CORE
tristate

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..417394d 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -89,6 +89,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_CQHCI) += cqhci.o
+obj-$(CONFIG_MMC_SDHCI_SPRD_R11) += sdhci-sprd-r11.o

ifeq ($(CONFIG_CB710_DEBUG),y)
CFLAGS-cb710-mmc += -DDEBUG
diff --git a/drivers/mmc/host/sdhci-sprd-r11.c b/drivers/mmc/host/sdhci-sprd-r11.c
new file mode 100644
index 0000000..fcd093a5
--- /dev/null
+++ b/drivers/mmc/host/sdhci-sprd-r11.c
@@ -0,0 +1,472 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Secure Digital Host Controller
+//
+// Copyright (C) 2018 Spreadtrum, Inc.
+// Author: Chunyan Zhang <[email protected]>
+
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/highmem.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include "sdhci-pltfm.h"
+
+#define SDHCI_SPRD_REG_32_DLL_DLY_OFFSET 0x208
+#define SDHCIBSPRD_IT_WR_DLY_INV (1 << 5)
+#define SDHCI_SPRD_BIT_CMD_DLY_INV (1 << 13)
+#define SDHCI_SPRD_BIT_POSRD_DLY_INV (1 << 21)
+#define SDHCI_SPRD_BIT_NEGRD_DLY_INV (1 << 29)
+
+#define SDHCI_SPRD_REG_32_BUSY_POSI 0x250
+#define SDHCI_SPRD_BIT_OUTR_CLK_AUTO_EN (1 << 25)
+#define SDHCI_SPRD_BIT_INNR_CLK_AUTO_EN (1 << 24)
+
+#define SDHCI_SPRD_REG_DEBOUNCE 0x28C
+#define SDHCI_SPRD_BIT_DLL_BAK (1 << 0)
+#define SDHCI_SPRD_BIT_DLL_VAL (1 << 1)
+
+#define SDHCI_SPRD_INT_SIGNAL_MASK 0x1B7F410B
+
+/* SDHCI_HOST_CONTROL2 */
+#define SDHCI_SPRD_CTRL_HS200 0x0005
+#define SDHCI_SPRD_CTRL_HS400 0x0006
+
+/* SDHCI_SOFTWARE_RESET */
+#define SDHCI_HW_RESET_CARD 0x8 /* For Spreadtrum's design */
+
+#define SDHCI_SPRD_MAX_CUR 1020
+#define SDHCI_SPRD_CLK_MAX_DIV 0x3FF
+
+struct sdhci_sprd_host {
+ u32 version;
+ struct clk *clk_sdio;
+ struct clk *clk_source;
+ struct clk *clk_enable;
+ u32 base_rate;
+};
+
+#define TO_SPRD_HOST(host) sdhci_pltfm_priv(sdhci_priv(host))
+
+static int sdhci_sprd_get_dt_resource(struct platform_device *pdev,
+ struct sdhci_sprd_host *sprd_host)
+{
+ int ret = 0;
+ struct clk *clk;
+
+ clk = devm_clk_get(&pdev->dev, "sdio");
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ dev_warn(&pdev->dev, "Failed to get sdio clock (%d)\n", ret);
+ goto out;
+ }
+ sprd_host->clk_sdio = clk;
+
+ clk = devm_clk_get(&pdev->dev, "source");
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ dev_warn(&pdev->dev, "Failed to get source clock (%d)\n", ret);
+ goto out;
+ }
+ sprd_host->clk_source = clk;
+
+ clk_set_parent(sprd_host->clk_sdio, sprd_host->clk_source);
+ sprd_host->base_rate = clk_get_rate(sprd_host->clk_source);
+ if (!sprd_host->base_rate) {
+ sprd_host->base_rate = 26000000;
+ dev_warn(&pdev->dev, "The source clock rate is 0\n");
+ }
+
+ clk = devm_clk_get(&pdev->dev, "enable");
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ dev_warn(&pdev->dev, "Failed to get gate clock (%d)\n", ret);
+ goto out;
+ }
+ sprd_host->clk_enable = clk;
+
+out:
+ return ret;
+}
+
+static void sdhci_sprd_set_mmc_struct(struct platform_device *pdev,
+ struct mmc_host *mmc)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct sdhci_host *host = mmc_priv(mmc);
+
+ mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
+ MMC_CAP_ERASE | MMC_CAP_CMD23;
+
+ mmc_of_parse(mmc);
+ mmc_of_parse_voltage(np, &host->ocr_mask);
+
+ mmc->ocr_avail = 0x40000;
+ mmc->ocr_avail_sdio = mmc->ocr_avail;
+ mmc->ocr_avail_sd = mmc->ocr_avail;
+ mmc->ocr_avail_mmc = mmc->ocr_avail;
+
+ mmc->max_current_330 = SDHCI_SPRD_MAX_CUR;
+ mmc->max_current_300 = SDHCI_SPRD_MAX_CUR;
+ mmc->max_current_180 = SDHCI_SPRD_MAX_CUR;
+
+ host->dma_mask = DMA_BIT_MASK(64);
+ mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
+}
+
+static void sdhci_sprd_init_config(struct sdhci_host *host)
+{
+ u16 val;
+
+ /* set 64-bit addressing modes */
+ val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ val |= SDHCI_CTRL_64BIT_ADDR;
+ sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+
+ /* set dll backup mode */
+ val = sdhci_readl(host, SDHCI_SPRD_REG_DEBOUNCE);
+ val |= SDHCI_SPRD_BIT_DLL_BAK | SDHCI_SPRD_BIT_DLL_VAL;
+ sdhci_writel(host, val, SDHCI_SPRD_REG_DEBOUNCE);
+}
+
+static inline u32 sdhci_sprd_readl(struct sdhci_host *host, int reg)
+{
+ if (unlikely(reg == SDHCI_MAX_CURRENT))
+ return SDHCI_SPRD_MAX_CUR;
+
+ return readl_relaxed(host->ioaddr + reg);
+}
+
+static inline void sdhci_sprd_writel(struct sdhci_host *host, u32 val, int reg)
+{
+ /* SDHCI_MAX_CURRENT is reserved on Spreadtrum's platform */
+ if (unlikely(reg == SDHCI_MAX_CURRENT))
+ return;
+
+ if (unlikely(reg == SDHCI_SIGNAL_ENABLE || reg == SDHCI_INT_ENABLE))
+ val = val & SDHCI_SPRD_INT_SIGNAL_MASK;
+
+ return writel_relaxed(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_sprd_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+ if (unlikely(reg == SDHCI_SOFTWARE_RESET)) {
+ if (readb_relaxed(host->ioaddr + reg) & SDHCI_HW_RESET_CARD)
+ val |= SDHCI_HW_RESET_CARD;
+ }
+
+ return writeb_relaxed(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_sprd_sd_clk_off(struct sdhci_host *host)
+{
+ u16 ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+
+ ctrl &= (~SDHCI_CLOCK_CARD_EN);
+ sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static inline void
+sdhci_sprd_set_dll_invert(struct sdhci_host *host, u32 mask, bool en)
+{
+ u32 dll_dly_offset;
+
+ dll_dly_offset = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_DLY_OFFSET);
+ if (en)
+ dll_dly_offset |= mask;
+ else
+ dll_dly_offset &= ~mask;
+ sdhci_writel(host, dll_dly_offset, SDHCI_SPRD_REG_32_DLL_DLY_OFFSET);
+}
+
+static inline u32 sdhci_sprd_calc_div(u32 base_clk, u32 clk)
+{
+ u32 div;
+
+ /* select 2x clock source */
+ if (base_clk <= clk * 2)
+ return 0;
+
+ div = (u32) (base_clk / (clk * 2));
+
+ if ((base_clk / div) > (clk * 2))
+ div++;
+
+ if (div > SDHCI_SPRD_CLK_MAX_DIV)
+ div = SDHCI_SPRD_CLK_MAX_DIV;
+
+ if (div % 2)
+ div = (div + 1) / 2;
+ else
+ div = div / 2;
+
+ return div;
+}
+
+static inline void _sdhci_sprd_set_clock(struct sdhci_host *host,
+ unsigned int clk)
+{
+ struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+ u32 div, val, mask;
+
+ div = sdhci_sprd_calc_div(sprd_host->base_rate, clk);
+
+ clk |= ((div & 0x300) >> 2) | ((div & 0xFF) << 8);
+ sdhci_enable_clk(host, clk);
+
+ /* enable auto gate sdhc_enable_auto_gate */
+ val = sdhci_readl(host, SDHCI_SPRD_REG_32_BUSY_POSI);
+ mask = SDHCI_SPRD_BIT_OUTR_CLK_AUTO_EN |
+ SDHCI_SPRD_BIT_INNR_CLK_AUTO_EN;
+ if (mask != (val & mask)) {
+ val |= mask;
+ sdhci_writel(host, val, SDHCI_SPRD_REG_32_BUSY_POSI);
+ }
+}
+
+static void sdhci_sprd_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+ bool en = false;
+
+ if (clock == 0) {
+ sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+ } else if (clock != host->clock) {
+ sdhci_sprd_sd_clk_off(host);
+ _sdhci_sprd_set_clock(host, clock);
+
+ if (clock <= 400000)
+ en = true;
+ sdhci_sprd_set_dll_invert(host, SDHCI_SPRD_BIT_CMD_DLY_INV |
+ SDHCI_SPRD_BIT_POSRD_DLY_INV, en);
+ } else {
+ _sdhci_sprd_set_clock(host, clock);
+ }
+
+}
+
+static unsigned int sdhci_sprd_get_max_clock(struct sdhci_host *host)
+{
+ struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+
+ return clk_round_rate(sprd_host->clk_sdio, ULONG_MAX);
+}
+
+static unsigned int sdhci_sprd_get_min_clock(struct sdhci_host *host)
+{
+ return 400000;
+}
+
+static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
+{
+ sdhci_reset(host, mask);
+}
+
+static void sdhci_sprd_set_uhs_signaling(struct sdhci_host *host,
+ unsigned int timing)
+{
+ u16 ctrl_2;
+
+ if (timing == host->timing)
+ return;
+
+ ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ /* Select Bus Speed Mode for host */
+ ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
+ switch (timing) {
+ case MMC_TIMING_UHS_SDR12:
+ ctrl_2 = SDHCI_CTRL_UHS_SDR12;
+ break;
+ case MMC_TIMING_MMC_HS:
+ case MMC_TIMING_SD_HS:
+ case MMC_TIMING_UHS_SDR25:
+ ctrl_2 = SDHCI_CTRL_UHS_SDR25;
+ break;
+ case MMC_TIMING_UHS_SDR50:
+ ctrl_2 = SDHCI_CTRL_UHS_SDR50;
+ break;
+ case MMC_TIMING_UHS_SDR104:
+ ctrl_2 = SDHCI_CTRL_UHS_SDR104;
+ break;
+ case MMC_TIMING_UHS_DDR50:
+ case MMC_TIMING_MMC_DDR52:
+ ctrl_2 = SDHCI_CTRL_UHS_DDR50;
+ break;
+ case MMC_TIMING_MMC_HS200:
+ ctrl_2 = SDHCI_SPRD_CTRL_HS200;
+ break;
+ case MMC_TIMING_MMC_HS400:
+ ctrl_2 = SDHCI_SPRD_CTRL_HS400;
+ break;
+ default:
+ break;
+ }
+
+ sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+}
+
+static void sdhci_sprd_hw_reset(struct sdhci_host *host)
+{
+ int val;
+
+ /* Note: don't use sdhci_readb/writeb() API here */
+ val = readb_relaxed(host->ioaddr + SDHCI_SOFTWARE_RESET);
+ val &= ~SDHCI_HW_RESET_CARD;
+ writeb_relaxed(val, host->ioaddr + SDHCI_SOFTWARE_RESET);
+ udelay(10);
+
+ val |= SDHCI_HW_RESET_CARD;
+ writeb_relaxed(val, host->ioaddr + SDHCI_SOFTWARE_RESET);
+ udelay(300);
+}
+
+static struct sdhci_ops sdhci_sprd_ops = {
+ .read_l = sdhci_sprd_readl,
+ .write_l = sdhci_sprd_writel,
+ .write_b = sdhci_sprd_writeb,
+ .set_clock = sdhci_sprd_set_clock,
+ .get_max_clock = sdhci_sprd_get_max_clock,
+ .get_min_clock = sdhci_sprd_get_min_clock,
+ .set_bus_width = sdhci_set_bus_width,
+ .reset = sdhci_sprd_reset,
+ .set_uhs_signaling = sdhci_sprd_set_uhs_signaling,
+ .hw_reset = sdhci_sprd_hw_reset,
+};
+
+static const struct sdhci_pltfm_data sdhci_sprd_pdata = {
+ .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+ .quirks2 = SDHCI_QUIRK2_BROKEN_HS200,
+ .ops = &sdhci_sprd_ops,
+};
+
+static int sdhci_sprd_probe(struct platform_device *pdev)
+{
+ struct sdhci_host *host;
+ struct sdhci_sprd_host *sprd_host;
+ int ret = 0;
+
+ host = sdhci_pltfm_init(pdev, &sdhci_sprd_pdata, sizeof(*sprd_host));
+ if (IS_ERR(host))
+ return PTR_ERR(host);
+
+ sprd_host = TO_SPRD_HOST(host);
+
+ ret = sdhci_sprd_get_dt_resource(pdev, sprd_host);
+ if (ret)
+ goto pltfm_free;
+
+ clk_prepare_enable(sprd_host->clk_sdio);
+ clk_prepare_enable(sprd_host->clk_enable);
+
+ sdhci_sprd_init_config(host);
+
+ sdhci_sprd_set_mmc_struct(pdev, host->mmc);
+
+ host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
+ sprd_host->version = ((host->version & SDHCI_VENDOR_VER_MASK) >>
+ SDHCI_VENDOR_VER_SHIFT);
+
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_suspend_ignore_children(&pdev->dev, 1);
+
+ ret = sdhci_add_host(host);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to add mmc host: %d\n", ret);
+ goto pm_runtime_disable;
+ }
+
+ return 0;
+
+pm_runtime_disable:
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+
+ clk_disable_unprepare(sprd_host->clk_sdio);
+ clk_disable_unprepare(sprd_host->clk_enable);
+
+pltfm_free:
+ sdhci_pltfm_free(pdev);
+ return ret;
+}
+
+static int sdhci_sprd_remove(struct platform_device *pdev)
+{
+ struct sdhci_host *host = platform_get_drvdata(pdev);
+ struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+ struct mmc_host *mmc = host->mmc;
+
+ mmc_remove_host(mmc);
+ clk_disable_unprepare(sprd_host->clk_sdio);
+ clk_disable_unprepare(sprd_host->clk_enable);
+
+ mmc_free_host(mmc);
+
+ return 0;
+}
+
+static const struct of_device_id sdhci_sprd_of_match[] = {
+ { .compatible = "sprd,sdhc-r11", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sdhci_sprd_of_match);
+
+#ifdef CONFIG_PM
+static int sdhci_sprd_runtime_suspend(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+
+ sdhci_runtime_suspend_host(host);
+
+ clk_disable_unprepare(sprd_host->clk_sdio);
+ clk_disable_unprepare(sprd_host->clk_enable);
+
+ return 0;
+}
+
+static int sdhci_sprd_runtime_resume(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+
+ clk_prepare_enable(sprd_host->clk_enable);
+ clk_prepare_enable(sprd_host->clk_sdio);
+
+ sdhci_runtime_resume_host(host);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops sdhci_sprd_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(sdhci_sprd_runtime_suspend,
+ sdhci_sprd_runtime_resume, NULL)
+};
+
+static struct platform_driver sdhci_sprd_driver = {
+ .probe = sdhci_sprd_probe,
+ .remove = sdhci_sprd_remove,
+ .driver = {
+ .name = "sdhci_sprd_r11",
+ .of_match_table = of_match_ptr(sdhci_sprd_of_match),
+ .pm = &sdhci_sprd_pm_ops,
+ },
+};
+module_platform_driver(sdhci_sprd_driver);
+
+MODULE_DESCRIPTION("Spreadtrum sdio host controller r11 driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:sdhci-sprd-r11");
--
2.7.4


2018-06-08 08:27:54

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 5/6] mmc: sdhci: add CMD23 support for v4 mode

Host Driver Version 4.10 adds a new bit in Host Control 2 Register
for selecting Auto CMD23 or Auto CMD12 for ADMA3 data transfer.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
drivers/mmc/host/sdhci.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b8ee124..3b2af7e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -954,6 +954,20 @@ static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
!mrq->cap_cmd_during_tfr;
}

+static inline void sdhci_set_auto_cmd23(struct sdhci_host *host,
+ struct mmc_command *cmd)
+{
+ u16 ctrl2;
+
+ if (host->v4_mode) {
+ ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ ctrl2 |= SDHCI_CMD23_ENABLE;
+ sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
+ } else {
+ sdhci_writel(host, cmd->mrq->sbc->arg, SDHCI_ARGUMENT2);
+ }
+}
+
static void sdhci_set_transfer_mode(struct sdhci_host *host,
struct mmc_command *cmd)
{
@@ -989,7 +1003,7 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
mode |= SDHCI_TRNS_AUTO_CMD12;
else if (cmd->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
mode |= SDHCI_TRNS_AUTO_CMD23;
- sdhci_writel(host, cmd->mrq->sbc->arg, SDHCI_ARGUMENT2);
+ sdhci_set_auto_cmd23(host, cmd);
}
}

diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1e84539..d5e1c10 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -185,6 +185,7 @@
#define SDHCI_CTRL_DRV_TYPE_D 0x0030
#define SDHCI_CTRL_EXEC_TUNING 0x0040
#define SDHCI_CTRL_TUNED_CLK 0x0080
+#define SDHCI_CMD23_ENABLE 0x0800
#define SDHCI_CTRL_V4_MODE 0x1000
#define SDHCI_CTRL_64BIT_ADDR 0x2000
#define SDHCI_CTRL_PRESET_VAL_ENABLE 0x8000
--
2.7.4


2018-06-08 08:28:10

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 1/6] mmc: sdhci: add sd host v4 mode

For SD host controller version 4.00 or later ones, there're two
modes of implementation - Version 3.00 compatible mode or
Version 4 mode. This patch introduces a flag to record this.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 6 ++++++
drivers/mmc/host/sdhci.h | 6 ++++++
2 files changed, 12 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2ededa7f..cf5695f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3302,6 +3302,12 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION);
host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT;

+ if (host->version >= SDHCI_SPEC_400) {
+ if (sdhci_readw(host, SDHCI_HOST_CONTROL2) &
+ SDHCI_CTRL_V4_MODE)
+ host->v4_mode = true;
+ }
+
if (host->quirks & SDHCI_QUIRK_MISSING_CAPS)
return;

diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c95b0a4..128b0ba 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -184,6 +184,7 @@
#define SDHCI_CTRL_DRV_TYPE_D 0x0030
#define SDHCI_CTRL_EXEC_TUNING 0x0040
#define SDHCI_CTRL_TUNED_CLK 0x0080
+#define SDHCI_CTRL_V4_MODE 0x1000
#define SDHCI_CTRL_PRESET_VAL_ENABLE 0x8000

#define SDHCI_CAPABILITIES 0x40
@@ -270,6 +271,8 @@
#define SDHCI_SPEC_100 0
#define SDHCI_SPEC_200 1
#define SDHCI_SPEC_300 2
+#define SDHCI_SPEC_400 3
+#define SDHCI_SPEC_410 4

/*
* End of controller registers.
@@ -551,6 +554,9 @@ struct sdhci_host {
u32 sdma_boundary;

unsigned long private[0] ____cacheline_aligned;
+
+ /* Host Version 4 Enable */
+ bool v4_mode;
};

struct sdhci_ops {
--
2.7.4


2018-06-08 08:28:25

by Chunyan Zhang

[permalink] [raw]
Subject: [PATCH 3/6] mmc: sdhci: add ADMA2 64-bit addressing support for V4 mode

ADMA2 64-bit addressing support is divided into V3 mode and V4 mode.
So there are two kinds of descriptors for ADMA2 64-bit addressing
i.e. 96-bit Descriptor for V3 mode, and 128-bit Descriptor for V4
mode. 128-bit Descriptor is aligned to 8-byte.

For V4 mode, ADMA2 64-bit addressing is enabled via Host Control 2
register.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 50 +++++++++++++++++++++++++++++++++++-------------
drivers/mmc/host/sdhci.h | 23 +++++++++++++++++-----
2 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f57201f..5d3b0d8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -585,6 +585,8 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
void *desc, *align;
char *buffer;
int len, offset, i;
+ unsigned int adma2_align = SDHCI_ADMA2_ALIGN(host);
+ unsigned int adma2_mask = SDHCI_ADMA2_MASK(host);

/*
* The spec does not specify endianness of descriptor table.
@@ -608,8 +610,8 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
* buffer for the (up to three) bytes that screw up the
* alignment.
*/
- offset = (SDHCI_ADMA2_ALIGN - (addr & SDHCI_ADMA2_MASK)) &
- SDHCI_ADMA2_MASK;
+ offset = (adma2_align - (addr & adma2_align)) &
+ adma2_mask;
if (offset) {
if (data->flags & MMC_DATA_WRITE) {
buffer = sdhci_kmap_atomic(sg, &flags);
@@ -623,8 +625,8 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,

BUG_ON(offset > 65536);

- align += SDHCI_ADMA2_ALIGN;
- align_addr += SDHCI_ADMA2_ALIGN;
+ align += adma2_align;
+ align_addr += adma2_align;

desc += host->desc_sz;

@@ -668,13 +670,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
void *align;
char *buffer;
unsigned long flags;
+ unsigned int adma2_align = SDHCI_ADMA2_ALIGN(host);
+ unsigned int adma2_mask = SDHCI_ADMA2_MASK(host);

if (data->flags & MMC_DATA_READ) {
bool has_unaligned = false;

/* Do a quick scan of the SG list for any unaligned mappings */
for_each_sg(data->sg, sg, host->sg_count, i)
- if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
+ if (sg_dma_address(sg) & adma2_mask) {
has_unaligned = true;
break;
}
@@ -686,15 +690,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
align = host->align_buffer;

for_each_sg(data->sg, sg, host->sg_count, i) {
- if (sg_dma_address(sg) & SDHCI_ADMA2_MASK) {
- size = SDHCI_ADMA2_ALIGN -
- (sg_dma_address(sg) & SDHCI_ADMA2_MASK);
+ if (sg_dma_address(sg) & adma2_mask) {
+ size = adma2_align -
+ (sg_dma_address(sg) & adma2_mask);

buffer = sdhci_kmap_atomic(sg, &flags);
memcpy(buffer, align, size);
sdhci_kunmap_atomic(buffer, &flags);

- align += SDHCI_ADMA2_ALIGN;
+ align += adma2_align;
}
}
}
@@ -3400,6 +3404,26 @@ static int sdhci_allocate_bounce_buffer(struct sdhci_host *host)
return 0;
}

+static inline bool sdhci_use_64bit_dma(struct sdhci_host *host)
+{
+ u32 addr64bit_en;
+
+ /*
+ * According to SD Host Controller spec v4.10, bit[27] added from
+ * version 4.10 in Capabilities Register is used as 64-bit System
+ * Address support for V4 mode, 64-bit DMA Addressing for V4 mode
+ * is enabled only if 64-bit Addressing =1 in the Host Control 2
+ * register.
+ */
+ if (host->version == SDHCI_SPEC_410 && host->v4_mode) {
+ addr64bit_en = (sdhci_readw(host, SDHCI_HOST_CONTROL2) &
+ SDHCI_CTRL_64BIT_ADDR);
+ return addr64bit_en && (host->caps & SDHCI_CAN_64BIT_V4);
+ }
+
+ return host->caps & SDHCI_CAN_64BIT;
+}
+
int sdhci_setup_host(struct sdhci_host *host)
{
struct mmc_host *mmc;
@@ -3471,7 +3495,7 @@ int sdhci_setup_host(struct sdhci_host *host)
* SDHCI_QUIRK2_BROKEN_64_BIT_DMA must be left to the drivers to
* implement.
*/
- if (host->caps & SDHCI_CAN_64BIT)
+ if (sdhci_use_64bit_dma(host))
host->flags |= SDHCI_USE_64_BIT_DMA;

if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
@@ -3505,15 +3529,15 @@ int sdhci_setup_host(struct sdhci_host *host)
*/
if (host->flags & SDHCI_USE_64_BIT_DMA) {
host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
- SDHCI_ADMA2_64_DESC_SZ;
- host->desc_sz = SDHCI_ADMA2_64_DESC_SZ;
+ SDHCI_ADMA2_64_DESC_SZ(host);
+ host->desc_sz = SDHCI_ADMA2_64_DESC_SZ(host);
} else {
host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
SDHCI_ADMA2_32_DESC_SZ;
host->desc_sz = SDHCI_ADMA2_32_DESC_SZ;
}

- host->align_buffer_sz = SDHCI_MAX_SEGS * SDHCI_ADMA2_ALIGN;
+ host->align_buffer_sz = SDHCI_MAX_SEGS * SDHCI_ADMA2_ALIGN(host);
buf = dma_alloc_coherent(mmc_dev(mmc), host->align_buffer_sz +
host->adma_table_sz, &dma, GFP_KERNEL);
if (!buf) {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 128b0ba..820a863 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -185,6 +185,7 @@
#define SDHCI_CTRL_EXEC_TUNING 0x0040
#define SDHCI_CTRL_TUNED_CLK 0x0080
#define SDHCI_CTRL_V4_MODE 0x1000
+#define SDHCI_CTRL_64BIT_ADDR 0x2000
#define SDHCI_CTRL_PRESET_VAL_ENABLE 0x8000

#define SDHCI_CAPABILITIES 0x40
@@ -206,6 +207,7 @@
#define SDHCI_CAN_VDD_300 0x02000000
#define SDHCI_CAN_VDD_180 0x04000000
#define SDHCI_CAN_64BIT 0x10000000
+#define SDHCI_CAN_64BIT_V4 0x8000000

#define SDHCI_SUPPORT_SDR50 0x00000001
#define SDHCI_SUPPORT_SDR104 0x00000002
@@ -297,9 +299,14 @@ struct sdhci_adma2_32_desc {
__le32 addr;
} __packed __aligned(4);

-/* ADMA2 data alignment */
-#define SDHCI_ADMA2_ALIGN 4
-#define SDHCI_ADMA2_MASK (SDHCI_ADMA2_ALIGN - 1)
+/*
+ * ADMA2 data alignment
+ * According to SD Host Controller spec v4.10, if Host Version 4 Enable is set
+ * in the Host Control 2 register, 128-bit Descriptor will be selected which
+ * shall be aligned 8-byte address boundary.
+ */
+#define SDHCI_ADMA2_ALIGN(host) ((host)->v4_mode ? 8 : 4)
+#define SDHCI_ADMA2_MASK(host) (SDHCI_ADMA2_ALIGN(host) - 1)

/*
* ADMA2 descriptor alignment. Some controllers (e.g. Intel) require 8 byte
@@ -308,8 +315,14 @@ struct sdhci_adma2_32_desc {
*/
#define SDHCI_ADMA2_DESC_ALIGN 8

-/* ADMA2 64-bit DMA descriptor size */
-#define SDHCI_ADMA2_64_DESC_SZ 12
+/*
+ * ADMA2 64-bit DMA descriptor size
+ * According to SD Host Controller spec v4.10, there are two kinds of
+ * descriptors for 64-bit addressing mode: 96-bit Descriptor and 128-bit
+ * Descriptor, if Host Version 4 Enable is set in the Host Control 2
+ * register, 128-bit Descriptor will be selected.
+ */
+#define SDHCI_ADMA2_64_DESC_SZ(host) ((host)->v4_mode ? 16 : 12)

/*
* ADMA2 64-bit descriptor. Note 12-byte descriptor can't always be 8-byte
--
2.7.4


2018-06-11 07:15:46

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 6/6] mmc: host: sdhci-sprd: added Spreadtrum's host controller R11

On 8 June 2018 at 10:18, Chunyan Zhang <[email protected]> wrote:
> From: Chunyan Zhang <[email protected]>
>
> This patch adds the initial support of Secure Digital Host Controller
> Interface compliant controller - R11 found in some latest Spreadtrum
> chipsets.
>
> R11 is a variant based on SD v4.0 specification.
>
> With this driver, mmc can be initialized, can be mounted, read and
> written.
>
> Original-by: Billows Wu <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 13 ++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/sdhci-sprd-r11.c | 472 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 486 insertions(+)
> create mode 100644 drivers/mmc/host/sdhci-sprd-r11.c

This is a DT based driver. Please add a separate patch describing the
corresponding bindings and compatibles.

[...]

> +static int sdhci_sprd_get_dt_resource(struct platform_device *pdev,
> + struct sdhci_sprd_host *sprd_host)
> +{
> + int ret = 0;
> + struct clk *clk;
> +
> + clk = devm_clk_get(&pdev->dev, "sdio");
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + dev_warn(&pdev->dev, "Failed to get sdio clock (%d)\n", ret);
> + goto out;
> + }
> + sprd_host->clk_sdio = clk;
> +
> + clk = devm_clk_get(&pdev->dev, "source");
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + dev_warn(&pdev->dev, "Failed to get source clock (%d)\n", ret);
> + goto out;
> + }
> + sprd_host->clk_source = clk;
> +
> + clk_set_parent(sprd_host->clk_sdio, sprd_host->clk_source);
> + sprd_host->base_rate = clk_get_rate(sprd_host->clk_source);
> + if (!sprd_host->base_rate) {
> + sprd_host->base_rate = 26000000;
> + dev_warn(&pdev->dev, "The source clock rate is 0\n");
> + }
> +

The above can be managed by the assigned-clock* DT bindings. Please
have a look at:

Documentation/devicetree/bindings/clock/clock-bindings.txt
drivers/clk/clk-conf.c

> + clk = devm_clk_get(&pdev->dev, "enable");
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + dev_warn(&pdev->dev, "Failed to get gate clock (%d)\n", ret);

The printed name of the clock doesn't match the name used in
devm_clk_get() call.

BTW, I think devm_clk_get() already prints some information when it
fails to lookup a clock. Isn't that sufficient?

> + goto out;
> + }
> + sprd_host->clk_enable = clk;
> +
> +out:
> + return ret;
> +}
> +
> +static void sdhci_sprd_set_mmc_struct(struct platform_device *pdev,
> + struct mmc_host *mmc)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
> + MMC_CAP_ERASE | MMC_CAP_CMD23;
> +
> + mmc_of_parse(mmc);
> + mmc_of_parse_voltage(np, &host->ocr_mask);

mmc_of_parse_voltage() is intended to be used for controllers that
internally manages the powered to the card. Is that really the case?

I assume you have external regulator(s) to manage that, no?

> +
> + mmc->ocr_avail = 0x40000;
> + mmc->ocr_avail_sdio = mmc->ocr_avail;
> + mmc->ocr_avail_sd = mmc->ocr_avail;
> + mmc->ocr_avail_mmc = mmc->ocr_avail;

If there is external regulators used, all the above can go away. In
either case, at least the *_sdio, *_sd, *_mmc can go away.

> +
> + mmc->max_current_330 = SDHCI_SPRD_MAX_CUR;
> + mmc->max_current_300 = SDHCI_SPRD_MAX_CUR;
> + mmc->max_current_180 = SDHCI_SPRD_MAX_CUR;

This should probably also be fetched used an external regulator and
sdhci already manages that.

> +
> + host->dma_mask = DMA_BIT_MASK(64);
> + mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
> +}

[...]

> +
> +static unsigned int sdhci_sprd_get_min_clock(struct sdhci_host *host)
> +{
> + return 400000;
> +}

Isn't there a more straightforward way to assign the minimum clock
rate? Do you really need to use a callback?

> +
> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
> +{
> + sdhci_reset(host, mask);
> +}

Looks like an unnecessary wrapper function.

[...]

> +static int sdhci_sprd_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + struct sdhci_sprd_host *sprd_host;
> + int ret = 0;
> +
> + host = sdhci_pltfm_init(pdev, &sdhci_sprd_pdata, sizeof(*sprd_host));
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + sprd_host = TO_SPRD_HOST(host);
> +
> + ret = sdhci_sprd_get_dt_resource(pdev, sprd_host);
> + if (ret)
> + goto pltfm_free;
> +
> + clk_prepare_enable(sprd_host->clk_sdio);
> + clk_prepare_enable(sprd_host->clk_enable);
> +
> + sdhci_sprd_init_config(host);
> +
> + sdhci_sprd_set_mmc_struct(pdev, host->mmc);
> +
> + host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> + sprd_host->version = ((host->version & SDHCI_VENDOR_VER_MASK) >>
> + SDHCI_VENDOR_VER_SHIFT);
> +
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_suspend_ignore_children(&pdev->dev, 1);
> +
> + ret = sdhci_add_host(host);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add mmc host: %d\n", ret);
> + goto pm_runtime_disable;
> + }
> +

Looks like there is a missing call to pm_runtime_put() here, no?

Unless it's intentional to prevent runtime suspend, for whatever reasons!?

> + return 0;
> +
> +pm_runtime_disable:
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> +
> + clk_disable_unprepare(sprd_host->clk_sdio);
> + clk_disable_unprepare(sprd_host->clk_enable);
> +
> +pltfm_free:
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}
> +
> +static int sdhci_sprd_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
> + struct mmc_host *mmc = host->mmc;
> +
> + mmc_remove_host(mmc);
> + clk_disable_unprepare(sprd_host->clk_sdio);
> + clk_disable_unprepare(sprd_host->clk_enable);
> +
> + mmc_free_host(mmc);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sdhci_sprd_of_match[] = {
> + { .compatible = "sprd,sdhc-r11", },

As stated, don't forget to document this.

[...]

Kind regards
Uffe

2018-06-13 06:00:05

by Chunyan Zhang

[permalink] [raw]
Subject: Re: [PATCH 6/6] mmc: host: sdhci-sprd: added Spreadtrum's host controller R11

Hi Ulf,

On 11 June 2018 at 15:15, Ulf Hansson <[email protected]> wrote:
> On 8 June 2018 at 10:18, Chunyan Zhang <[email protected]> wrote:
>> From: Chunyan Zhang <[email protected]>
>>
>> This patch adds the initial support of Secure Digital Host Controller
>> Interface compliant controller - R11 found in some latest Spreadtrum
>> chipsets.
>>
>> R11 is a variant based on SD v4.0 specification.
>>
>> With this driver, mmc can be initialized, can be mounted, read and
>> written.
>>
>> Original-by: Billows Wu <[email protected]>
>> Signed-off-by: Chunyan Zhang <[email protected]>
>> ---
>> drivers/mmc/host/Kconfig | 13 ++
>> drivers/mmc/host/Makefile | 1 +
>> drivers/mmc/host/sdhci-sprd-r11.c | 472 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 486 insertions(+)
>> create mode 100644 drivers/mmc/host/sdhci-sprd-r11.c
>
> This is a DT based driver. Please add a separate patch describing the
> corresponding bindings and compatibles.

Ok.

>
> [...]
>
>> +static int sdhci_sprd_get_dt_resource(struct platform_device *pdev,
>> + struct sdhci_sprd_host *sprd_host)
>> +{
>> + int ret = 0;
>> + struct clk *clk;
>> +
>> + clk = devm_clk_get(&pdev->dev, "sdio");
>> + if (IS_ERR(clk)) {
>> + ret = PTR_ERR(clk);
>> + dev_warn(&pdev->dev, "Failed to get sdio clock (%d)\n", ret);
>> + goto out;
>> + }
>> + sprd_host->clk_sdio = clk;
>> +
>> + clk = devm_clk_get(&pdev->dev, "source");
>> + if (IS_ERR(clk)) {
>> + ret = PTR_ERR(clk);
>> + dev_warn(&pdev->dev, "Failed to get source clock (%d)\n", ret);
>> + goto out;
>> + }
>> + sprd_host->clk_source = clk;
>> +
>> + clk_set_parent(sprd_host->clk_sdio, sprd_host->clk_source);
>> + sprd_host->base_rate = clk_get_rate(sprd_host->clk_source);
>> + if (!sprd_host->base_rate) {
>> + sprd_host->base_rate = 26000000;
>> + dev_warn(&pdev->dev, "The source clock rate is 0\n");
>> + }
>> +
>
> The above can be managed by the assigned-clock* DT bindings. Please
> have a look at:
>

Ah, didn't notice these bindings, managing in this way is indeed better.

> Documentation/devicetree/bindings/clock/clock-bindings.txt
> drivers/clk/clk-conf.c
>
>> + clk = devm_clk_get(&pdev->dev, "enable");
>> + if (IS_ERR(clk)) {
>> + ret = PTR_ERR(clk);
>> + dev_warn(&pdev->dev, "Failed to get gate clock (%d)\n", ret);
>
> The printed name of the clock doesn't match the name used in
> devm_clk_get() call.
>
> BTW, I think devm_clk_get() already prints some information when it
> fails to lookup a clock. Isn't that sufficient?

Right, will remove this print.

>
>> + goto out;
>> + }
>> + sprd_host->clk_enable = clk;
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static void sdhci_sprd_set_mmc_struct(struct platform_device *pdev,
>> + struct mmc_host *mmc)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct sdhci_host *host = mmc_priv(mmc);
>> +
>> + mmc->caps = MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
>> + MMC_CAP_ERASE | MMC_CAP_CMD23;
>> +
>> + mmc_of_parse(mmc);
>> + mmc_of_parse_voltage(np, &host->ocr_mask);
>
> mmc_of_parse_voltage() is intended to be used for controllers that
> internally manages the powered to the card. Is that really the case?

I guess it is not, will remove this,

>
> I assume you have external regulator(s) to manage that, no?
>
>> +
>> + mmc->ocr_avail = 0x40000;
>> + mmc->ocr_avail_sdio = mmc->ocr_avail;
>> + mmc->ocr_avail_sd = mmc->ocr_avail;
>> + mmc->ocr_avail_mmc = mmc->ocr_avail;
>

and this,

> If there is external regulators used, all the above can go away. In
> either case, at least the *_sdio, *_sd, *_mmc can go away.
>
>> +
>> + mmc->max_current_330 = SDHCI_SPRD_MAX_CUR;
>> + mmc->max_current_300 = SDHCI_SPRD_MAX_CUR;
>> + mmc->max_current_180 = SDHCI_SPRD_MAX_CUR;
>

also this :)

> This should probably also be fetched used an external regulator and
> sdhci already manages that.
>
>> +
>> + host->dma_mask = DMA_BIT_MASK(64);
>> + mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
>> +}
>
> [...]
>
>> +
>> +static unsigned int sdhci_sprd_get_min_clock(struct sdhci_host *host)
>> +{
>> + return 400000;
>> +}
>
> Isn't there a more straightforward way to assign the minimum clock
> rate? Do you really need to use a callback?
>

Do you mean setting mmc->f_min directly after sdhci_add_host()?
We would get a different f_min without this callback, since
SDHCI_CLOCK_MUL_MASK in caps1 register is reserved.

>> +
>> +static void sdhci_sprd_reset(struct sdhci_host *host, u8 mask)
>> +{
>> + sdhci_reset(host, mask);
>> +}
>
> Looks like an unnecessary wrapper function.

Ok, will take out of this wrapper.

>
> [...]
>
>> +static int sdhci_sprd_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host;
>> + struct sdhci_sprd_host *sprd_host;
>> + int ret = 0;
>> +
>> + host = sdhci_pltfm_init(pdev, &sdhci_sprd_pdata, sizeof(*sprd_host));
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + sprd_host = TO_SPRD_HOST(host);
>> +
>> + ret = sdhci_sprd_get_dt_resource(pdev, sprd_host);
>> + if (ret)
>> + goto pltfm_free;
>> +
>> + clk_prepare_enable(sprd_host->clk_sdio);
>> + clk_prepare_enable(sprd_host->clk_enable);
>> +
>> + sdhci_sprd_init_config(host);
>> +
>> + sdhci_sprd_set_mmc_struct(pdev, host->mmc);
>> +
>> + host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
>> + sprd_host->version = ((host->version & SDHCI_VENDOR_VER_MASK) >>
>> + SDHCI_VENDOR_VER_SHIFT);
>> +
>> + pm_runtime_get_noresume(&pdev->dev);
>> + pm_runtime_set_active(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_suspend_ignore_children(&pdev->dev, 1);
>> +
>> + ret = sdhci_add_host(host);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to add mmc host: %d\n", ret);
>> + goto pm_runtime_disable;
>> + }
>> +
>
> Looks like there is a missing call to pm_runtime_put() here, no?
>

Yes, will add back.

> Unless it's intentional to prevent runtime suspend, for whatever reasons!?
>
>> + return 0;
>> +
>> +pm_runtime_disable:
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_set_suspended(&pdev->dev);
>> +
>> + clk_disable_unprepare(sprd_host->clk_sdio);
>> + clk_disable_unprepare(sprd_host->clk_enable);
>> +
>> +pltfm_free:
>> + sdhci_pltfm_free(pdev);
>> + return ret;
>> +}
>> +
>> +static int sdhci_sprd_remove(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
>> + struct mmc_host *mmc = host->mmc;
>> +
>> + mmc_remove_host(mmc);
>> + clk_disable_unprepare(sprd_host->clk_sdio);
>> + clk_disable_unprepare(sprd_host->clk_enable);
>> +
>> + mmc_free_host(mmc);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sdhci_sprd_of_match[] = {
>> + { .compatible = "sprd,sdhc-r11", },
>
> As stated, don't forget to document this.

Ok.

Thanks for your comments,
Chunyan

>
> [...]
>
> Kind regards
> Uffe