Re: [RFC PATCH v3 1/3] mmc: sprd: Add MMC host driver for Spreadtrum SoC
From: Hongtao Wu <hidden>
Date: 2015-09-28 07:18:16
quoted
On Thu, Sep 10, 2015 at 9:28 PM, Ulf Hansson [off-list ref] wrote:
On 14 August 2015 at 18:55, Hongtao Wu [off-list ref] wrote:quoted
the Spreadtrum MMC host driver is used to supply EMMC, SD, and SDIO types of memory cardsPerhaps some more information about the controller. Are there any specific features it support or doesn't support!?
Thanks for kindly reply.
Yes, spreadtrum MMC host controllers have some specific features as follows:
(1) We don't have controls for sampling clock tuning and re-tuning, we take
place of them with three registers as follows:
(a) CLK_WR_DL(Offset 080h): Data write clock delay line.
(b) CLK_RD_POS_DL(Offset 0x84h): Posedge data read clock delay line.
(c) CLK_RD_NEG_DL(Offset 088h): Negedge data read clock delay line.
(2) We don't have Power Control Register(Offset 029h), all our controller's
power
come from PMIC, rather than directly from CPU.
(3) We don't have bit[6](Card Insertion), bit[7] (Card Removal), bit[8]
(Card Interrupt)
and bit[12:9] in Normal Interrupt Status Register(Offset 030h). Because the
detect
gpio pin doesn't connect to the register of our host controller. So we can't
operate bit[18:16](Card Detect Pin Level, Card State stable and Card
Inserted)
in Present State Register(Offset 024h).
Moreover it would be nice to know a bit more what this patch contains. For example, you have some debugfs support, what's that?
Debugfs is a debug file system about MMC, where we can check some message about MMC host controller register. Maybe we will improve it and handle that in a separate patch. Also, we have PM and runtime PM support, which may not be so important at this stage. And we will commit it with a separate patch.
quoted
Signed-off-by: Billows Wu(WuHongtao) <redacted> --- drivers/mmc/host/Kconfig | 6 + drivers/mmc/host/Makefile | 1 + drivers/mmc/host/sprd_sdhost.c | 1202
++++++++++++++++++++++++++++++++
quoted
drivers/mmc/host/sprd_sdhost.h | 615 ++++++++++++++++Just by looking at number of lines here, it tells me that the size of header file is just way too big. It's half the size of the c-file, that just can't be right. :-) Just to make it clear. I don't like "one-liner" wrapper functions nor macros. If you remove these kind of stuff, the total size would shrink significantly I belive.
I am sorry. I wrap so many "one-liner" functions. But I have some reasons to do it. Firstly, the registers in some of our MMC host controller is designed to 4-byte aligned, but in others are not. So we use __local_readx() or __local_writex() to calls readx_relaxed() or writex_relaxed(), and use sdhost_readx() or sdhost_writex() to call __local_readx() or __local_writex(). Maybe in the future, all our controller will use 4-byte non-aligned. Then we will use readx_relaxed() or writex_relaxed() directly. Secondly, in sprd_sdhost.h, For every register we supply corresponding operation function, i.e, for SDHOST_16_BLK_SIZE(0x04), we use _sdhost_set_blk_size() to write the block size. Every register corresponding to an operation function, which logic is nice, I think. What is your opinion? The above two points also interpret why the size of the header file is so big.
quoted
drivers/mmc/host/sprd_sdhost_debugfs.c | 212 ++++++ drivers/mmc/host/sprd_sdhost_debugfs.h | 27 +I had a brief look at the debugfs support, let me suggest that we handle that in a separate patch. Perhaps, some of that code can also be made more generic and used by the mmc core instead.
Thanks. I will handle it as a separate patch and use mmc core as possibly as I can.
quoted
6 files changed, 2063 insertions(+) create mode 100644 drivers/mmc/host/sprd_sdhost.c create mode 100644 drivers/mmc/host/sprd_sdhost.h create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.c create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.h
[...]
quoted
diff --git a/drivers/mmc/host/sprd_sdhost.c
b/drivers/mmc/host/sprd_sdhost.c
quoted
new file mode 100644 index 0000000..95639a3--- /dev/null +++ b/drivers/mmc/host/sprd_sdhost.c@@ -0,0 +1,1202 @@ +/* + * linux/drivers/mmc/host/sprd_sdhost.c - Secure Digital Host
Controller
quoted
+ * Interface driver
[...]
quoted
+ +#include "sprd_sdhost.h" +#include "sprd_sdhost_debugfs.h" + +#define DRIVER_NAME "sdhost" +#define SDHOST_CAPS \Please remove this define it makes it harder to read the code.
I will remove it in next version.
quoted
+ (MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | \ + MMC_CAP_ERASE | MMC_CAP_UHS_SDR50 | \ + MMC_CAP_CMD23 | MMC_CAP_HW_RESET) + +struct sdhost_caps_data { + char *name; + u32 ocr_avail; + u32 caps; + u32 caps2; + u32 pm_caps; + u32 base_clk; + u32 signal_default_voltage; +};I don't think you need this struct, as most of this data is already present it struct mmc_host. And for that small pieces that isn't you might as well just add into your struct sdhost_host.
Yes, most of this data can be removed, such as caps, caps2, base_clk. However ocr_avail and signal_default_voltage are useful for us. We can't fetch ocr_avail from external regulator via mmc_regulator_get_supply, because our regulator driver can't supply mmc_regulator_get_ocrmask() interface for MMC driver. And sometimes our external regulator can not supply 3.0v or 1.8v default signal voltage that we expect. So this 3.0v or 1.8v default signal voltage are mandatory.
quoted
+ +struct sdhost_caps_data caps_info_map[] = {This is the wrong way of how to assign capabilties. Instead mmc_of_parse() parses your DT configuration and assign the corresponding caps in the struct mmc_host.quoted
+ { + .name = "sd", + .ocr_avail = MMC_VDD_29_30 | MMC_VDD_30_31,ocr_avail should be fetched from external regulators via mmc_regulator_get_supply() API. I don't think you need this at all then, right?
For us the ocr_avail can not be fetched from external regulator, and now we must set it manually. Could we put the ocr_avail and signal_default_voltage in dts? If you have better possible solution how I can fetch ocr_avail via other way, please tell me. Thanks.
quoted
+ .caps = SDHOST_CAPS, + .caps2 = MMC_CAP2_HC_ERASE_SZ, + .signal_default_voltage = 3000000,Again, if this is needed you shall use the regulator API to find out this information.
Now our regulator driver can not supply this information that we need. However, I will talk to our regulator driver and hardware owner. Maybe they can improve that.
quoted
+ }, + { + .name = "wifi", + .ocr_avail = MMC_VDD_165_195 | MMC_VDD_29_30 | + MMC_VDD_30_31 | MMC_VDD_32_33 | MMC_VDD_33_34, + .caps = SDHOST_CAPS | MMC_CAP_POWER_OFF_CARD | + MMC_CAP_UHS_SDR12, + }, + { + .name = "emmc", + .ocr_avail = MMC_VDD_29_30 | MMC_VDD_30_31, + .caps = SDHOST_CAPS | + MMC_CAP_8_BIT_DATA | MMC_CAP_UHS_SDR12 | + MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_DDR50 | + MMC_CAP_MMC_HIGHSPEED, + .signal_default_voltage = 1800000, + } +}; + +#ifdef CONFIG_PM_RUNTIMERegarding the system PM and runtime PM support, please remove that code from $subject patch and let's add that as a separate patch instead.
OK, I will remove this code in next version, and commit it as a separate patch.
The reason is simply that I see way too many strange things going on in that part of code, so I decided that it's easier for me to comment on them later and separately.
OK, I will improve it.
quoted
+static void _pm_runtime_setting(struct platform_device *pdev, + struct sdhost_host *host); +#else +static void _pm_runtime_setting(struct platform_device *pdev, + struct sdhost_host *host) +{ +} +#endif
[...]
quoted
+static void __get_rsp(struct sdhost_host *host) +{ + u32 i, offset; + unsigned int flags = host->cmd->flags; + u32 *resp = host->cmd->resp; + + if (!(flags & MMC_RSP_PRESENT)) + return; + + if (flags & MMC_RSP_136) { + /* CRC is stripped so we need to do some shifting. */ + for (i = 0, offset = 12; i < 3; i++, offset -= 4) { + resp[i] = + _sdhost_readl(host,Just to make it clear around what I mean with "one-liner" wrapprer functions, _sdhost_readl() is a perfect example. _sdhost_readl() calls __local_readl() which calls readl_relaxed(). This is crazy and completely unnecessary complicated. Please just call readl_relaxed() here, since it helps me review and understand what's going on in the code.
Thanks. Just like the relationship sdhci_readl() and readl(), we wrap readl_relaxed() with sdhost_readl(). Of course, if you think this is unnecessary, we will improve it.
As there are many examples of similar cases, please simplify the code according to my suggestions.
Thanks for your kindly suggestions. Do you think we must modify these "one-liner" wrapprer functions right now? Or can we change it after all our register are 4-byte non-aligned.
quoted
+ SDHOST_32_RESPONSE + offset)
<< 8;
quoted
+ resp[i] |= + _sdhost_readb(host, + SDHOST_32_RESPONSE + offset -
1);
quoted
+ } + resp[3] = _sdhost_readl(host, SDHOST_32_RESPONSE) << 8; + } else { + resp[0] = _sdhost_readl(host, SDHOST_32_RESPONSE); + } +} + +static void _send_cmd(struct sdhost_host *host, struct mmc_command
*cmd)
quoted
+{ + struct mmc_data *data = cmd->data; + int sg_cnt; + u32 flag = 0; + u16 rsp_type = 0; + int if_has_data = 0; + int if_mult = 0; + int if_read = 0; + int if_dma = 0; + u16 auto_cmd = __ACMD_DIS; + + pr_debug("%s(%s) CMD%d, arg 0x%x, flag 0x%x\n", __func__, + host->device_name, cmd->opcode, cmd->arg, cmd->flags); + if (cmd->data) + pr_debug("%s(%s) block size %d, cnt %d\n", __func__, + host->device_name, cmd->data->blksz,
cmd->data->blocks);
quoted
+ + _sdhost_disable_all_int(host);Do you really need to turn the IRQs on/off between each an every command? If not, it would save you from reading/writing to the registers a few times per each commands.
Thanks. I will remove it. We turn the IRQs on/off between each an every command. Because some time ago we recieved some undesirable irq when _send_cmd() function was running, then _irq() function was called. This might be cause some problem. However, the undesirable irq may not apper this days. So it's time to remove the IRQs on/off.
quoted
+ + if (MMC_ERASE == cmd->opcode) {I guess you might already know that the mmc core suffers from a few bugs around how to deal with erase/trim/discard. And it's related to the max_busy_timeout value. Now, this fix you have here is okay for now, but we should really fix the problem in the core. Host drivers shouldn't have to care about this as a specific command.
Thanks. Maybe we shouldn't have to care about MMC_ERASE as a specific command, however, from a great deal measurement, we find that only CMD38 (MMC_ERASE) take a long time. So we set a long timeout value for it, and for others the value is short. BTW, In order to avoid that hardware timeout is invalid, in every time before sending command, we set software timeout(mod_timer()) and hardware timeout (write value to SDHOST_8_TIMEOUT ).
quoted
+ /* if it is erase command , it's busy time will long, + * so we set long timeout value here. + */ + mod_timer(&host->timer, jiffies + + msecs_to_jiffies(host->mmc->max_busy_timeout +
1000));
quoted
+ _sdhost_writeb(host->ioaddr, + __DATA_TIMEOUT_MAX_VAL, SDHOST_8_TIMEOUT); + } else { + mod_timer(&host->timer, + jiffies + (SDHOST_MAX_TIMEOUT + 1) * HZ); + _sdhost_writeb(host, host->data_timeout_val,
SDHOST_8_TIMEOUT);
quoted
+ } +
[...]
quoted
+ host->int_filter = flag; + _sdhost_enable_int(host, flag); + pr_debug("sdhost %s CMD%d rsp:0x%x intflag:0x%x\n" + "if_mult:0x%x if_read:0x%x auto_cmd:0x%x if_dma:0x%x\n", + host->device_name, cmd->opcode, mmc_resp_type(cmd), + flag, if_mult, if_read, auto_cmd, if_dma); + + _sdhost_set_trans_and_cmd(host, if_mult, if_read, auto_cmd,
if_mult,
quoted
+ if_dma, cmd->opcode, if_has_data,
rsp_type);
I don't like think this kind of wrapper function either. It's just way too hard to review and understand.
We supply a corresponding function for each MMC host register. That's why we wrap these functions like this. and from the name of this function _sdhost_set_trans_and_cmd() we can see it is used to set MMC's transfer mode and command. If you think it is hard to review, we will use writel_relaxed() instead of it.
Instead I would prefer if _send_cmd() builds the register value in a u32 variable itself. Then it writes that value by invoking writel_relaxed(). Please follow my suggestion for other similar cases as well.quoted
+} + +static irqreturn_t _irq(int irq, void *param) +{Please consider to split up this function in smaller sub-functions, it would help me to review.
OK! I will handle it.
quoted
+ u32 intmask; + struct sdhost_host *host = (struct sdhost_host *)param; + struct mmc_request *mrq = host->mrq; + struct mmc_command *cmd = host->cmd; + struct mmc_data *data; + + spin_lock(&host->lock); + /* maybe _timeout() run in one core and _irq() run in + * another core, this will panic if access cmd->data + */ + if ((!mrq) || (!cmd)) { + spin_unlock(&host->lock); + return IRQ_NONE; + } + data = cmd->data; + + intmask = _sdhost_readl(host, SDHOST_32_INT_ST); + if (!intmask) { + spin_unlock(&host->lock); + return IRQ_NONE; + } + pr_debug("%s(%s) CMD%d, intmask 0x%x, filter = 0x%x\n",
__func__,
quoted
+ host->device_name, cmd->opcode, intmask,
host->int_filter);
quoted
+ + /* sometimes an undesired interrupt will happen, so we must
clear
quoted
+ * this unused interrupt. + */ + _sdhost_clear_int(host, intmask); + /* just care about the interrupt that we want */ + intmask &= host->int_filter; + + while (intmask) { + if (_INT_FILTER_ERR & intmask) { + /* some error happened in command */ + if (_INT_FILTER_ERR_CMD & intmask) { + if (_INT_ERR_CMD_TIMEOUT & intmask) + cmd->error = -ETIMEDOUT; + else + cmd->error = -EILSEQ; + } + /* some error happened in data token or command + * with R1B + */ + if (_INT_FILTER_ERR_DATA & intmask) { + if (data) { + /* current error is happened in
data
quoted
+ * token + */ + if (_INT_ERR_DATA_TIMEOUT &
intmask)
quoted
+ data->error =
-ETIMEDOUT;
quoted
+ else + data->error = -EILSEQ; + } else { + /* current error is happend in
response
quoted
+ * with busy + */ + if (_INT_ERR_DATA_TIMEOUT &
intmask)
quoted
+ cmd->error = -ETIMEDOUT; + else + cmd->error = -EILSEQ; + } + } + if (_INT_ERR_ACMD & intmask) { + /* Auto cmd12 and cmd23 error is belong
to data
quoted
+ * token error + */ + data->error = -EILSEQ; + } + if (_INT_ERR_ADMA & intmask) + data->error = -EIO; + + pr_debug("sdhost %s int 0x%x\n",
host->device_name,
quoted
+ intmask); + dump_sdio_reg(host); + _sdhost_disable_all_int(host); + /* if current error happened in data token, + * we send cmd12 to stop it + */ + if ((mrq->cmd == cmd) && (mrq->stop)) { + _sdhost_reset(host, _RST_CMD |
_RST_DATA);
quoted
+ _send_cmd(host, mrq->stop); + } else { + /* request finish with error, so reset
it and
quoted
+ * stop the request + */ + _sdhost_reset(host, _RST_CMD |
_RST_DATA);
quoted
+ tasklet_schedule(&host->finish_tasklet);Instead of using tasklets, please convert to threaded IRQ handlers.
OK. I will handle it.
quoted
+ } + goto out; + } else { + /* delete irq that wanted in filter */ + host->int_filter &= ~(_INT_FILTER_NORMAL &
intmask);
quoted
+ if (_INT_DMA_END & intmask) { + _sdhost_writel(host, + _sdhost_readl(host,
SDHOST_32_SYS_ADDR),
quoted
+ SDHOST_32_SYS_ADDR); + } + if (_INT_CMD_END & intmask) { + cmd->error = 0; + __get_rsp(host); + } + if (_INT_TRAN_END & intmask) { + if (data) { + dma_unmap_sg(mmc_dev(host->mmc), + data->sg, data->sg_len, + (data->flags &
MMC_DATA_READ) ?
quoted
+ DMA_FROM_DEVICE : + DMA_TO_DEVICE); + data->error = 0; + data->bytes_xfered = + data->blksz * data->blocks; + } else { + /* R1B also can produce
transferComplete
quoted
+ * interrupt + */ + cmd->error = 0; + } + } + if (!(_INT_FILTER_NORMAL & host->int_filter)) { + /* current cmd finished */ + _sdhost_disable_all_int(host); + if (mrq->sbc == cmd) { + _send_cmd(host, mrq->cmd); + } else if ((mrq->cmd == host->cmd) + && (mrq->stop)) { + _send_cmd(host, mrq->stop); + } else { + /* finish with success and stop
the
quoted
+ * request + */ +
tasklet_schedule(&host->finish_tasklet);
quoted
+ goto out; + } + } + } + + intmask = _sdhost_readl(host, SDHOST_32_INT_ST); + _sdhost_clear_int(host, intmask); + intmask &= host->int_filter; + }; + +out: + spin_unlock(&host->lock); + return IRQ_HANDLED; +} + +static void _tasklet(unsigned long param)As already stated, please convert to threaded IRQ handlers in favor of
tasklets. Thanks. I will deal with it.
quoted
+{ + struct sdhost_host *host = (struct sdhost_host *)param; + unsigned long flags; + struct mmc_request *mrq; + + del_timer(&host->timer); +
[...]
quoted
+static void sdhost_request(struct mmc_host *mmc, struct mmc_request
*mrq)
quoted
+{ + struct sdhost_host *host = mmc_priv(mmc); + unsigned long flags; + + _runtime_get(host); + spin_lock_irqsave(&host->lock, flags); + + host->mrq = mrq; + /* 1 find whether card is still in slot */ + if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE)) { + if (!mmc_gpio_get_cd(host->mmc)) {This means for every new request you will be reading a gpio line to see if the card is still there. That's going to be highly inefficient and affecting performance. I would instead leave this to be entirely controlled by the mmc core. Typically what's needed from your controller would be that it provides an IRQ for a CMD timeout reasonably fast, in the case when card has been removed. Then you will be okay.
Thanks. But I noticed that the standard MMC host driver sdhci.c also to do it as follows: sdhci_request() --> sdhci_do_get_cd() --> mmc_gpio_get_cd(). However I will try to remove it and test whether there is a problem when insert or remove a SD card randomly.
quoted
+ mrq->cmd->error = -ENOMEDIUM; + tasklet_schedule(&host->finish_tasklet); + mmiowb(); + spin_unlock_irqrestore(&host->lock, flags); + return; + } + /* else asume sdcard is present */ + } + + /* + * in our control we can not use auto cmd12 and auto cmd23
together
quoted
+ * so in following program we use auto cmd23 prior to auto cmd12 + */I don't really follow what's the issue is here. Can you perhaps elaborate
a bit? Sorry. Now our host driver don't use neither auto cmd12 nor auto cmd23.
I noticed that you have set MMC_CAP_CMD23, which indicates to the mmc core that it shall use mrq->sbc when creating reqeusts. Now, are you saying that you controller internally handles CMD12 when using the CMD23 way of reading/writing data?
No. Our controller doesn't internally handles CMD12 when using the CMD23 way of reading/writing data. We don't use auto CMD12 or auto CMD23. When createing requests, we use mrq->sbc and CMD23 but not auto command.
I don't like that you need to change the values of the mrq->data. If special treatment is needed, it's better to indicate that through a new mmc cap to the mmc core, somehow...
Thanks. I will remove the change of mrq->data.
quoted
+ pr_debug("%s(%s) CMD%d request %d %d %d\n", + __func__, host->device_name, mrq->cmd->opcode, + !!mrq->sbc, !!mrq->cmd, !!mrq->stop); + host->auto_cmd_mode = __ACMD_DIS; + if (!mrq->sbc && mrq->stop && SDHOST_FLAG_ENABLE_ACMD12) { + host->auto_cmd_mode = __ACMD12; + mrq->data->stop = NULL; + mrq->stop = NULL; + } + + /* 3 send cmd list */ + if ((mrq->sbc) && SDHOST_FLAG_ENABLE_ACMD23) { + host->auto_cmd_mode = __ACMD23; + mrq->data->stop = NULL; + mrq->stop = NULL; + _send_cmd(host, mrq->cmd); + } else if (mrq->sbc) { + mrq->data->stop = NULL; + mrq->stop = NULL; + _send_cmd(host, mrq->sbc); + } else { + _send_cmd(host, mrq->cmd); + } +
[...]
quoted
+ +static void sdhost_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) +{ + struct sdhost_host *host = mmc_priv(mmc); + unsigned long flags; + + pr_debug("%s(%s) ios:\n" + "sdhost clock = %d-->%d\n" + "sdhost vdd = %d-->%d\n" + "sdhost bus_mode = %d-->%d\n" + "sdhost chip_select = %d-->%d\n" + "sdhost power_mode = %d-->%d\n" + "sdhost bus_width = %d-->%d\n" + "sdhost timing = %d-->%d\n" + "sdhost signal_voltage = %d-->%d\n" + "sdhost drv_type = %d-->%d\n", + __func__, host->device_name, + host->ios.clock, ios->clock, + host->ios.vdd, ios->vdd, + host->ios.bus_mode, ios->bus_mode, + host->ios.chip_select, ios->chip_select, + host->ios.power_mode, ios->power_mode, + host->ios.bus_width, ios->bus_width, + host->ios.timing, ios->timing, + host->ios.signal_voltage, ios->signal_voltage, + host->ios.drv_type, ios->drv_type);Huh, this seems like a leftover from an ongoing development. Please
remove.
Potentially we might want this information through a TRACE buffer instead, but then it should be handled by the mmc core and thus helping debugging for all mmc hosts.
Thanks, we will remove this debug information.
quoted
+ + _runtime_get(host); + spin_lock_irqsave(&host->lock, flags); + + if (0 == ios->clock) { + _sdhost_all_clk_off(host); + host->ios.clock = 0; + } else if (ios->clock != host->ios.clock) { + u32 div; + + div = _sdhost_calc_div(host->base_clk, ios->clock); + _sdhost_sd_clk_off(host); + _sdhost_clk_set_and_on(host, div); + _sdhost_sd_clk_on(host); + host->ios.clock = ios->clock; + host->data_timeout_val = + _sdhost_calc_timeout(host->base_clk,
SDHOST_MAX_TIMEOUT);
quoted
+ mmc->max_busy_timeout = (1 << 30) / (ios->clock / 1000); + } + + if (ios->power_mode != host->ios.power_mode) { + switch (ios->power_mode) { + case MMC_POWER_OFF:
[...]
quoted
+ +static int sdhost_get_cd(struct mmc_host *mmc) +{ + struct sdhost_host *host = mmc_priv(mmc); + unsigned long flags; + int gpio_cd; + + _runtime_get(host); + spin_lock_irqsave(&host->lock, flags); + + if (host->mmc->caps & MMC_CAP_NONREMOVABLE) { + spin_unlock_irqrestore(&host->lock, flags); + _runtime_put(host); + return 1; + } + + gpio_cd = mmc_gpio_get_cd(host->mmc); + if (IS_ERR_VALUE(gpio_cd)) + gpio_cd = 1; + mmiowb(); + spin_unlock_irqrestore(&host->lock, flags); + _runtime_put(host); + return !!gpio_cd;I think you should be able to assign your ->get_cd() callback directly to mmc_gpio_get_cd() as I think the other stuff is already managed by the mmc core.
We have already assign our ->get_cd() callback as follows:
static const struct mmc_host_ops sdhost_ops = {
...
.get_cd = sdhost_get_cd,
...
};
Moreover, I don't understand the runtime*() calls here, but let's leave that out of this review...
Sorry, we will remove all PM and PM_RUNTIME function in next version. And commit it as a separate patch, then we talk about it.
quoted
+} + +static int sdhost_card_busy(struct mmc_host *mmc) +{ + struct sdhost_host *host = mmc_priv(mmc); + unsigned long flags; + u32 present_state; + + _runtime_get(host); + spin_lock_irqsave(&host->lock, flags); + + /* Check whether DAT[3:0] is 0000 */ + present_state = _sdhost_readl(host, SDHOST_32_PRES_STATE); + + mmiowb(); + spin_unlock_irqrestore(&host->lock, flags); + _runtime_put(host); + + return !(present_state & _DATA_LVL_MASK);
[...]
quoted
+static int get_caps_info(struct sdhost_host *host) +{ + struct sdhost_caps_data *pdata = NULL; + int index; + int ret; + + for (index = 0; index < sizeof(caps_info_map) / + sizeof(struct sdhost_caps_data); index++) { + if (strcmp(host->device_name,
caps_info_map[index].name) == 0) {quoted
+ pdata = &caps_info_map[index]; + break; + } + } +As stated in the beginning, the above way isn't what you should be doing. Instead...
Thanks. We will deal with it.
quoted
+ host->ocr_avail = pdata->ocr_avail; + host->caps = pdata->caps; + host->caps2 = pdata->caps2; + host->pm_caps = pdata->pm_caps; + host->signal_default_voltage = pdata->signal_default_voltage; + + ret = mmc_of_parse(host->mmc);... let mmc_of_parse() take care of this.quoted
+ if (ret) + pr_err("parse sprd %s controller fail\n",
host->device_name);
quoted
+ + return ret; +} + +static int _get_dt_resource(struct platform_device *pdev, + struct sdhost_host *host) +{ + struct device_node *np = pdev->dev.of_node; + u32 sdhost_delay[3]; + int ret = 0; + + host->res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!host->res) + return -ENOENT; + + host->ioaddr = devm_ioremap_resource(&pdev->dev, host->res); + if (IS_ERR(host->ioaddr)) { + ret = PTR_ERR(host->ioaddr); + dev_err(&pdev->dev, "can not map iomem: %d\n", ret); + goto err; + } + + host->mapbase = host->res->start; + host->irq = platform_get_irq(pdev, 0); + if (host->irq < 0) { + ret = host->irq; + goto err; + } + + host->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR_OR_NULL(host->clk)) { + ret = PTR_ERR(host->clk); + dev_err(&pdev->dev, "can not get clock: %d\n", ret);Remove, printed by the clock framework.
Ok, thanks!
quoted
+ goto err; + } + + host->base_clk = clk_get_rate(host->clk); + + ret = of_property_read_string(np, "sprd,name",
&host->device_name);
quoted
+ if (ret) { + dev_err(&pdev->dev, + "can not read the property of sprd name\n"); + goto err; + } + + ret = get_caps_info(host); + if (ret) + goto err; + + host->detect_gpio = of_get_named_gpio(np, "cd-gpios", 0);Already covered by mmc_of_parse().
Sorry, we will deal with it.
quoted
+ if (!gpio_is_valid(host->detect_gpio)) + host->detect_gpio = -1; + + ret = of_property_read_u32_array(np, "sprd,delay",
sdhost_delay, 3);
quoted
+ if (!ret) { + host->write_delay = sdhost_delay[0]; + host->read_pos_delay = sdhost_delay[1]; + host->read_neg_delay = sdhost_delay[2]; + } else + dev_err(&pdev->dev, + "can not read the property of sprd delay\n"); + + return 0; + +err: + dev_err(&pdev->dev, "sprd_sdhost get basic resource fail\n"); + return ret; +} + +static int _get_ext_resource(struct sdhost_host *host) +{ + int err; + struct mmc_host *mmc = host->mmc; + + host->dma_mask = DMA_BIT_MASK(64); + host->data_timeout_val = 0; + + /* 1 LDO */ + mmc_regulator_get_supply(mmc); + if (IS_ERR_OR_NULL(mmc->supply.vmmc)) { + pr_err("%s(%s): no vmmc regulator found\n", + __func__, host->device_name); + mmc->supply.vmmc = NULL; + } + if (IS_ERR_OR_NULL(mmc->supply.vqmmc)) { + pr_err("%s(%s): no vqmmc regulator found\n", + __func__, host->device_name); + mmc->supply.vqmmc = NULL;First; both regulators are made optional when fetching them via mmc_regulator_get_supply(). Second; the prints are already managed by the mmc core, so I don't think you need it here as well.
Yes, thanks. We will delete this redundant debug informaton.
quoted
+ } else { + regulator_is_supported_voltage(mmc->supply.vqmmc, +
host->signal_default_voltage,
quoted
+
host->signal_default_voltage);
quoted
+ regulator_set_voltage(mmc->supply.vqmmc, + host->signal_default_voltage, + host->signal_default_voltage); + } + host->mmc = mmc; + + /* 2 clock */ + clk_prepare_enable(host->clk); +
[...]
quoted
+static int sdhost_remove(struct platform_device *pdev) +{ + struct sdhost_host *host = platform_get_drvdata(pdev); + struct mmc_host *mmc = host->mmc; + + if (-1 != host->detect_gpio) + mmc_gpio_free_cd(mmc); + + mmc_remove_host(mmc); + clk_disable_unprepare(host->clk); + free_irq(host->irq, host); + release_mem_region(host->res->start, resource_size(host->res));Both free_irq() and release_mem_region() isn't need since I think you used the devm_* functions, right!?
Yes, I will deal with it. Thanks. [...]
quoted
+static const struct of_device_id sdhost_of_match[] = { + {.compatible = "sprd,sdhost-3.0"}, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, sdhost_of_match); + +static struct platform_driver sdhost_driver = { + .probe = sdhost_probe, + .remove = sdhost_remove, + .driver = { + .owner = THIS_MODULE, + .pm = &sdhost_dev_pm_ops, + .name = DRIVER_NAME, + .of_match_table = of_match_ptr(sdhost_of_match), + }, +}; + +module_platform_driver(sdhost_driver); + +MODULE_DESCRIPTION("Spreadtrum sdio host controller driver"); +MODULE_LICENSE("GPL");So I will stop here for now and let you address my comments. Hopefully I should be able to give a quicker response once you post the next version.
Thanks for your kindly review.
Kind regards Uffequoted
diff --git a/drivers/mmc/host/sprd_sdhost.h
b/drivers/mmc/host/sprd_sdhost.h
quoted
new file mode 100644 index 0000000..d5cc438--- /dev/null +++ b/drivers/mmc/host/sprd_sdhost.h@@ -0,0 +1,615 @@ +/* + * linux/drivers/mmc/host/sprd_sdhost.h - Secure Digital Host
Controller
quoted
+ * Interface driver + * + * Copyright (C) 2015 Spreadtrum corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or
(at
quoted
+ * your option) any later version. + * + */ + +#ifndef __SDHOST_H_ +#define __SDHOST_H_ + +#include <linux/clk.h> +#include <linux/compiler.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mmc/card.h> +#include <linux/mmc/host.h> +#include <linux/mmc/mmc.h> +#include <linux/mmc/slot-gpio.h> +#include <linux/scatterlist.h> +#include <linux/types.h> + +/**********************************************************\ + * + * Controller block structure + * +\**********************************************************/ +struct sdhost_host { + /* --globe resource--- */ + spinlock_t lock; + struct mmc_host *mmc; + + /*--basic resource-- */ + struct resource *res; + void __iomem *ioaddr; + int irq; + const char *device_name; + struct platform_device *pdev; + unsigned long mapbase; +
[...]
quoted
+#define SDHOST_32_ARG 0x08 +#define SDHOST_16_TR_MODE 0x0C +#define __ACMD_DIS 0x00 +#define __ACMD12 0x01 +#define __ACMD23 0x02 + +static inline void _sdhost_set_trans_mode(struct sdhost_host *host, + u16 if_mult, u16 if_read, + u16 auto_cmd, + u16 if_blk_cnt, u16 if_dma) +{ + __local_writew((((if_mult ? 1 : 0) << 5) | + ((if_read ? 1 : 0) << 4) | + (auto_cmd << 2) | + ((if_blk_cnt ? 1 : 0) << 1) | + ((if_dma ? 1 : 0) << 0)), host,
SDHOST_16_TR_MODE);
quoted
+}
quoted
+ +#endif -- 1.7.9.5