On 2015/8/4 21:16, Hongtao Wu wrote:
This patch adds MMC host driver for Spreadtrum SoC.
The following coding style may be not meet kernel coding style.
I am not sure this kind of coding style is better or worse.
1) A macro that represent some bits of a register is added a prefix "__",
for example:
#define SDHOST_16_HOST_CTRL_2 0x3E
#define __TIMING_MODE_SDR12 0x0000
#define __TIMING_MODE_SDR25 0x0001
#define __TIMING_MODE_SDR50 0x0002
I think it is more useful to distinguish a register from a bit of this
register.
2) A function in order to operate a register is also added a prefix "_".
If the functions(A) call other function(B), we added a prefix "__" before
B,
for example:
static inline void _sdhost_enable_int(struct sdhost_host *host, u32 mask)
{
__local_writel(mask, host, SDHOST_32_INT_ST_EN);
__local_writel(mask, host, SDHOST_32_INT_SIG_EN);
}
I think this make the relationship of the function call more explicit.
Hi Shawn,
Thanks for your kindly reply.
According to your suggestion, I modified the following points:
1) delete some redundant mdelay().
2) Add error handling in some functions.
pls add a Series-changes tag to detail the diff between v1 & v2
Signed-off-by: Billows Wu(WuHongtao) <[email protected]>
---
[...]
+static void _send_cmd(struct sdhost_host *host, struct mmc_command *cmd)
+{
+ 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);
+
+ _sdhost_disable_all_int(host);
+
+ if (38 == cmd->opcode) {
It would be nice to use "MMC_ERASE " instear of "38"
+ /* if it is erase command , it's busy time will long,
+ * so we set long timeout value here.
+ */
+ mod_timer(&host->timer, jiffies + 10 * HZ);
how can you get 10*HZ?
Actually, something should be diff between secure/nosecure
erase/trim/discard
mmc_erase_timeout does calculate the busy time yet.
Might you can get busytime from cmd.busy_timeout!
+ _sdhost_writeb(host, __DATA_TIMEOUT_MAX_VAL, SDHOST_8_TIMEOUT);
+ } else {
+ mod_timer(&host->timer, jiffies + 3 * HZ);
Ditto.
+ _sdhost_writeb(host, host->data_timeout_val, SDHOST_8_TIMEOUT);
+ }
+
[...]
+ _sdhost_writel(host, sg_dma_address(data->sg),
+ SDHOST_32_SYS_ADDR);
+ } else {
+ WARN_ON(1);
Why need dump here?
+ flag |= _INT_ERR_ADMA;
+ _sdhost_set_dma(host, __32ADMA_MOD);
+ _sdhost_set_32_blk_cnt(host, data->blocks);
+ _sdhost_writel(host, sg_dma_address(data->sg),
[...]
+ 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);
+
No warning from checkpatch?
+ _sdhost_set_trans_and_cmd(host, if_mult, if_read, auto_cmd, if_mult,
+ if_dma, cmd->opcode, if_has_data, rsp_type);
+}
+
+static irqreturn_t _irq(int irq, void *param)
+{
+ /* maybe _timeout_func run in one core and _irq run in
+ * another core, this will panic if access cmd->data
+ */
+ if ((!mrq) || (!cmd)) {
It would be nice if you can use "goto out" here.
+ spin_unlock(&host->lock);
+ return IRQ_NONE;
+ }
+ data = cmd->data;
+
+ intmask = _sdhost_readl(host, SDHOST_32_INT_ST);
+ if (!intmask) {
Ditto.
+ spin_unlock(&host->lock);
+ return IRQ_NONE;
+ }
+ pr_debug("%s(%s) CMD%d, intmask 0x%x, filter = 0x%x\n", __func__,
+ host->device_name, cmd->opcode, intmask, host->int_filter);
+
+ /* disable unused interrupt */
disable or clear ?
+ _sdhost_clear_int(host, intmask);
+ /* just care about the interrupt that we want */
+ intmask &= host->int_filter;
It's not a good idea. If you don't care a irq, disable it while probing.
+
+ while (intmask) {
+ if (_INT_FILTER_ERR & intmask) {
+ /* some error happened in command */
[...]
+ _send_cmd(host, mrq->stop);
Are you sure it's fine to call _send_cmd in irq_handler not half bottom?
I wonder about the performance.
+ } else {
+ /* request finish with error, so reset it and
+ * stop the request
+ */
+ _sdhost_reset(host, _RST_CMD | _RST_DATA);
[...]
+
+static void sdhost_hw_reset(struct mmc_host *mmc)
+{
hw_reset means host trigger RST_n io of emmc to let it enter pre-idle
and init card for the first time or for err recovery if ext_csd enable
the reset bit. Your controller doesn't have rst pin? Even a gpio is okay.
sdhost_reset_emmc for what?
+ struct sdhost_host *host = mmc_priv(mmc);
+
+ _runtime_get(host);
+
+ /* close LDO and open LDO again. */
+ _signal_voltage_on_off(host, 0);
+ if (mmc->supply.vmmc)
+ mmc_regulator_set_ocr(host->mmc, mmc->supply.vmmc, 0);
+ if (mmc->supply.vmmc)
+ mmc_regulator_set_ocr(host->mmc, mmc->supply.vmmc,
+ host->ios.vdd);
+
+ _signal_voltage_on_off(host, 1);
+ mmiowb();
+ _runtime_put(host);
+
+}
+
+static const struct mmc_host_ops sdhost_ops = {
+ .request = sdhost_request,
+ .set_ios = sdhost_set_ios,
+ .get_ro = sdhost_get_ro,
+ .get_cd = sdhost_get_cd,
+
+ .start_signal_voltage_switch = sdhost_set_vqmmc,
+ .card_busy = sdhost_card_busy,
+ .hw_reset = sdhost_hw_reset,
+};
remove the blank line
+
+static void get_caps_info(struct sdhost_host *host,
[...]
+ }
+
+ host->clk = of_clk_get(np, 0);
+ if (IS_ERR_OR_NULL(host->clk)) {
+ ret = PTR_ERR(host->clk);
+ dev_err(&pdev->dev, "can not get clock: %d\n", ret);
+ goto err;
+ }
+
+ host->clk_parent = of_clk_get(np, 1);
+ if (IS_ERR_OR_NULL(host->clk_parent)) {
+ ret = PTR_ERR(host->clk_parent);
+ dev_err(&pdev->dev, "can not get parent clock: %d\n", ret);
+ goto err;
+ }
+
First, it's hard to understand what are clk and clk_parent. I guess that
clk is clock_out for emmc devices and clk_parent is for controller itself.
And it isn't a good idea to get them by fixed order.
host->clk_xxx= devm_clk_get(host->dev, "clk-name-in-dt") might be better?
+ ret = of_property_read_string(np, "sprd,name", &host->device_name);
[...]
+ ret = of_property_read_u32_array(np, "sprd,delay", sdhost_delay, 3);
+ 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");
+
pls fix coding style issue.
+ return 0;
+
[...]
+
+static struct platform_driver sdhost_driver = {
+ .probe = sdhost_probe,
+ .shutdown = sdhost_shutdown,
+ .driver = {
+ .owner = THIS_MODULE,
+ .pm = &sdhost_dev_pm_ops,
+ .name = DRIVER_NAME,
+ .of_match_table = of_match_ptr(sdhost_of_match),
+ },
+};
+
I think you need sdhost_remove hook to release something.
Have you test it by bind/unbind your driver repeatly?
+module_platform_driver(sdhost_driver);
+
+MODULE_DESCRIPTION("Spreadtrum sdio host controller driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mmc/host/sprd_sdhost.h b/drivers/mmc/host/sprd_sdhost.h
new file mode 100644
index 0000000..e921616
--- /dev/null
+++ b/drivers/mmc/host/sprd_sdhost.h
@@ -0,0 +1,591 @@
+/*
[...]
+ char *clk_name;
+ char *clk_parent_name;
I can't find where to use it
+ u32 base_clk;
+
[...]
+/* Controller registers */
+#ifdef SPRD_SDHOST_4_BYTE_ALIGNE
+static inline void __local_writeb(u8 val, struct sdhost_host *host,
+ u32 reg)
+{
+ u32 addr;
+ u32 value;
+ u32 ofst;
+
+ ofst = (reg & 0x3) << 3;
+ addr = reg & (~((u32) (0x3)));
+ value = readl_relaxed((host->ioaddr + addr));
+ value &= (~(((u32) ((u8) (-1))) << ofst));
+ value |= (((u32) val) << ofst);
+ writel_relaxed(value, (host->ioaddr + addr));
+}
+
Can we use writeb/writew/readb/readw instead?
+static inline void __local_writew(u16 val, struct sdhost_host *host,
+ u32 reg)
[...]
+static inline u32 _sdhost_calc_div(u32 base_clk, u32 clk)
+{
+ u32 N;
+
"div" will be better?
At least avoid using upper case for variable.
just my drive-by comment :)
+ if (base_clk <= clk)
+ return 0;
+
+ N = (u32) (base_clk / clk);
+ N = (N >> 1);
+ if (N)
+ N--;
+ if ((base_clk / ((N + 1) << 1)) > clk)
+ N++;
+ if (__CLK_MAX_DIV < N)
+ N = __CLK_MAX_DIV;
+
+ return N;
+}
+
+static inline void _sdhost_clk_set_and_on(struct sdhost_host *host,
+ u32 div)
+{
+ u16 ctrl = 0;
+
+ __local_writew(0, host, SDHOST_16_CLK_CTRL);
+ ctrl |= (u16) (((div & 0x300) >> 2) | ((div & 0xFF) << 8));
+ ctrl |= __CLK_IN_EN;
+ __local_writew(ctrl, host, SDHOST_16_CLK_CTRL);
+ while (!(__CLK_IN_STABLE & __local_readw(host, SDHOST_16_CLK_CTRL)))
+ ;
+}
I'm not sure if your clk can still be unready for some reasons(known or
unknown).
So how about timeout to break it and cast a dev_err here.
Further on, do something to recover it?
If not the case, just ignore this comment.
+
+#define SDHOST_8_TIMEOUT 0x2E
+#define __DATA_TIMEOUT_MAX_VAL 0xe
+
[...]
+
+/* spredtrum define it byself */
+static inline void _sdhost_reset_emmc(struct sdhost_host *host)
+{
+ __local_writeb(0, host, SDHOST_8_RST);
+ mdelay(2);
+ __local_writeb(_RST_EMMC, host, SDHOST_8_RST);
+}
According to JEDEC eMMC spec
tRstW >= 1us ; RST_n pulse width
tRSCA >= 200us ; RST_n to Command time
tRSTH >= 1us ; RST_n high period
I prefer to add at least 200us after unreset.
Ignore this comment if you will not use it before sending cmd.
+
+#define SDHOST_32_INT_ST 0x30
+#define SDHOST_32_INT_ST_EN 0x34
+#define SDHOST_32_INT_SIG_EN 0x38
+#define _INT_CMD_END 0x00000001
+#define _INT_TRAN_END 0x00000002
+#define _INT_DMA_END 0x00000008
+#define _INT_WR_RDY 0x00000010 /* not used */
+#define _INT_RD_RDY 0x00000020 /* not used */
+#define _INT_ERR 0x00008000
+#define _INT_ERR_CMD_TIMEOUT 0x00010000
+#define _INT_ERR_CMD_CRC 0x00020000
+#define _INT_ERR_CMD_END 0x00040000
+#define _INT_ERR_CMD_INDEX 0x00080000
+#define _INT_ERR_DATA_TIMEOUT 0x00100000
+#define _INT_ERR_DATA_CRC 0x00200000
+#define _INT_ERR_DATA_END 0x00400000
+#define _INT_ERR_CUR_LIMIT 0x00800000
+#define _INT_ERR_ACMD 0x01000000
+#define _INT_ERR_ADMA 0x02000000
[...]
+
+#endif
BTW, don't you need a documentation to elaborate more about your
controller or dt-binding?
--
1.7.9.5
--
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
