mmc: meson-gx: series with smaller improvements

PATCH Superseded Heiner Kallweit 2 participants 15 messages Mar 3, 2017 – Mar 4, 2017

Subsystems (2)

Arm/Amlogic Meson SoC support MMC, SD and SDIO

lore

Share & tools

lore

Heiner Kallweit

author

2017-03-03 19:49 UTC #0 lore raw

v4 cover mmc: meson-gx: series with smaller improvements

New series is limited to smaller refactorings w/o functional changes.

Heiner Kallweit (10):
mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
mmc: meson-gx: make two functions return void
mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
mmc: meson-gx: remove member parent_mux from struct meson_host
mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
mmc: meson-gx: improve initial configuration
mmc: meson-gx: remove member mrq from struct meson_host
mmc: meson-gx: replace magic timeout numbers with constants

 drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 76 deletions(-)

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:51 UTC #1 lore raw

PATCH v4 1/10 mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd

Core ensures that there are no commands with cmd->data being set and nothing to transfer. And we don't have to reset bit CMD_CFG_DATA_NUM because cmd_cfg was zero-initialized and this bit isn't set.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- adjusted commit message because branch xfer_bytes == 0 is never reached
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +1 -8

index 5a959783..07a7399c 100644
@@ -516,14 +516,7 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 			desc->cmd_cfg &= ~CMD_CFG_DATA_WR;
 		}
 
-		if (xfer_bytes > 0) {
-			desc->cmd_cfg &= ~CMD_CFG_DATA_NUM;
-			desc->cmd_data = host->bounce_dma_addr & CMD_DATA_MASK;
-		} else {
-			/* write data to data_addr */
-			desc->cmd_cfg |= CMD_CFG_DATA_NUM;
-			desc->cmd_data = 0;
-		}
+		desc->cmd_data = host->bounce_dma_addr & CMD_DATA_MASK;
 
 		cmd_cfg_timeout = 12;
 	} else {

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:52 UTC #2 lore raw

PATCH v4 2/10 mmc: meson-gx: make two functions return void

In reply to Heiner Kallweit

The return value of meson_mmc_request_done and meson_mmc_read_resp isn't used, so make both functions return void.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +3 -6

index 07a7399c..b99461ff 100644
@@ -426,7 +426,8 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			__func__, orig, val);
 }
 
-static int meson_mmc_request_done(struct mmc_host *mmc, struct mmc_request *mrq)
+static void meson_mmc_request_done(struct mmc_host *mmc,
+				   struct mmc_request *mrq)
 {
 	struct meson_host *host = mmc_priv(mmc);

@@ -435,8 +436,6 @@ static int meson_mmc_request_done(struct mmc_host *mmc, struct mmc_request *mrq)
 	host->mrq = NULL;
 	host->cmd = NULL;
 	mmc_request_done(host->mmc, mrq);
-
-	return 0;
 }
 
 static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)

@@ -554,7 +553,7 @@ static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		meson_mmc_start_cmd(mmc, mrq->cmd);
 }
 
-static int meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
+static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
 {
 	struct meson_host *host = mmc_priv(mmc);

@@ -566,8 +565,6 @@ static int meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
 	} else if (cmd->flags & MMC_RSP_PRESENT) {
 		cmd->resp[0] = readl(host->regs + SD_EMMC_CMD_RSP);
 	}
-
-	return 0;
 }
 
 static irqreturn_t meson_mmc_irq(int irq, void *dev_id)

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:53 UTC #3 lore raw

PATCH v4 3/10 mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host

In reply to Heiner Kallweit

Member ocr_mask is never used and member irq we can replace with a local variable in meson_mmc_probe. So let's remove both members.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +6 -8

index b99461ff..b9de3d47 100644
@@ -127,8 +127,6 @@ struct meson_host {
 
 	spinlock_t lock;
 	void __iomem *regs;
-	int irq;
-	u32 ocr_mask;
 	struct clk *core_clk;
 	struct clk_mux mux;
 	struct clk *mux_clk;

@@ -712,7 +710,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct meson_host *host;
 	struct mmc_host *mmc;
-	int ret;
+	int ret, irq;
 
 	mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev);
 	if (!mmc)

@@ -744,8 +742,8 @@ static int meson_mmc_probe(struct platform_device *pdev)
 		goto free_host;
 	}
 
-	host->irq = platform_get_irq(pdev, 0);
-	if (host->irq == 0) {
+	irq = platform_get_irq(pdev, 0);
+	if (!irq) {
 		dev_err(&pdev->dev, "failed to get interrupt resource.\n");
 		ret = -EINVAL;
 		goto free_host;

@@ -773,9 +771,9 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	writel(IRQ_EN_MASK, host->regs + SD_EMMC_STATUS);
 	writel(IRQ_EN_MASK, host->regs + SD_EMMC_IRQ_EN);
 
-	ret = devm_request_threaded_irq(&pdev->dev, host->irq,
-					meson_mmc_irq, meson_mmc_irq_thread,
-					IRQF_SHARED, DRIVER_NAME, host);
+	ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
+					meson_mmc_irq_thread, IRQF_SHARED,
+					DRIVER_NAME, host);
 	if (ret)
 		goto free_host;

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:54 UTC #4 lore raw

PATCH v4 4/10 mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init

In reply to Heiner Kallweit

Because the DT requires a fixed number of mux parent clocks, variable mux_parent_count can be replaced with constant MUX_CLK_NUM_PARENTS, so remove it.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- adjusted commit message
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +1 -3

index b9de3d47..ea2db3ce 100644
@@ -242,7 +242,6 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	char clk_name[32];
 	int i, ret = 0;
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
-	unsigned int mux_parent_count = 0;
 	const char *clk_div_parents[1];
 	u32 clk_reg, cfg;

@@ -261,7 +260,6 @@ static int meson_mmc_clk_init(struct meson_host *host)
 		}
 
 		mux_parent_names[i] = __clk_get_name(host->mux_parent[i]);
-		mux_parent_count++;
 	}
 
 	/* create the mux */

@@ -270,7 +268,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	init.ops = &clk_mux_ops;
 	init.flags = 0;
 	init.parent_names = mux_parent_names;
-	init.num_parents = mux_parent_count;
+	init.num_parents = MUX_CLK_NUM_PARENTS;
 
 	host->mux.reg = host->regs + SD_EMMC_CLOCK;
 	host->mux.shift = CLK_SRC_SHIFT;

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:55 UTC #5 lore raw

PATCH v4 5/10 mmc: meson-gx: remove member parent_mux from struct meson_host

In reply to Heiner Kallweit

Member mux_parent isn't used outside meson_mmc_clk_init. So remove it and replace it with a local variable in meson_mmc_clk_init.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +6 -8

index ea2db3ce..68e76fa8 100644
@@ -130,7 +130,6 @@ struct meson_host {
 	struct clk *core_clk;
 	struct clk_mux mux;
 	struct clk *mux_clk;
-	struct clk *mux_parent[MUX_CLK_NUM_PARENTS];
 	unsigned long current_clock;
 
 	struct clk_divider cfg_div;

@@ -247,19 +246,18 @@ static int meson_mmc_clk_init(struct meson_host *host)
 
 	/* get the mux parents */
 	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
+		struct clk *clk;
 		char name[16];
 
 		snprintf(name, sizeof(name), "clkin%d", i);
-		host->mux_parent[i] = devm_clk_get(host->dev, name);
-		if (IS_ERR(host->mux_parent[i])) {
-			ret = PTR_ERR(host->mux_parent[i]);
-			if (PTR_ERR(host->mux_parent[i]) != -EPROBE_DEFER)
+		clk = devm_clk_get(host->dev, name);
+		if (IS_ERR(clk)) {
+			if (clk != ERR_PTR(-EPROBE_DEFER))
 				dev_err(host->dev, "Missing clock %s\n", name);
-			host->mux_parent[i] = NULL;
-			return ret;
+			return PTR_ERR(clk);
 		}
 
-		mux_parent_names[i] = __clk_get_name(host->mux_parent[i]);
+		mux_parent_names[i] = __clk_get_name(clk);
 	}
 
 	/* create the mux */

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:56 UTC #6 lore raw

PATCH v4 6/10 mmc: meson-gx: fix error path in meson_mmc_clk_init / meson_mmc_probe

In reply to Heiner Kallweit

The condition should be "if (ret)" as the disable/unprepare is supposed to be executed if the previous command fails. In addition adjust the error path in probe to properly deal with the case that cfg_div_clk can be registered successfully but enable/prepare fails.
In this case we shouldn't call clk_disable_unprepare.

Reported-by Micha? Zegan <[email protected]>

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- extended commit message
v3:
- adjust error path in probe
v4:
- added acked-by

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +5 -4

index 68e76fa8..002e4aac 100644
@@ -321,7 +321,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	host->mmc->f_min = clk_round_rate(host->cfg_div_clk, 400000);
 
 	ret = meson_mmc_clk_set(host, host->mmc->f_min);
-	if (!ret)
+	if (ret)
 		clk_disable_unprepare(host->cfg_div_clk);
 
 	return ret;

@@ -771,7 +771,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 					meson_mmc_irq_thread, IRQF_SHARED,
 					DRIVER_NAME, host);
 	if (ret)
-		goto free_host;
+		goto err_div_clk;
 
 	mmc->max_blk_count = CMD_CFG_LENGTH_MASK;
 	mmc->max_req_size = mmc->max_blk_count * mmc->max_blk_size;

@@ -784,7 +784,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	if (host->bounce_buf == NULL) {
 		dev_err(host->dev, "Unable to map allocate DMA bounce buffer.\n");
 		ret = -ENOMEM;
-		goto free_host;
+		goto err_div_clk;
 	}
 
 	mmc->ops = &meson_mmc_ops;

@@ -792,8 +792,9 @@ static int meson_mmc_probe(struct platform_device *pdev)
 
 	return 0;
 
-free_host:
+err_div_clk:
 	clk_disable_unprepare(host->cfg_div_clk);
+free_host:
 	clk_disable_unprepare(host->core_clk);
 	mmc_free_host(mmc);
 	return ret;

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:57 UTC #7 lore raw

PATCH v4 7/10 mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init

In reply to Heiner Kallweit

CLK core does a deep copy of init.name, therefore it's fully ok to provide a local variable.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- fixed commit message
- added acked-by
v3:
- no changes
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/meson-gx-mmc.c +1 -1

index 002e4aac..684cc088 100644
@@ -281,7 +281,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
 
 	/* create the divider */
 	snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev));
-	init.name = devm_kstrdup(host->dev, clk_name, GFP_KERNEL);
+	init.name = clk_name;
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(host->mux_clk);

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:58 UTC #8 lore raw

PATCH v4 8/10 mmc: meson-gx: improve initial configuration

In reply to Heiner Kallweit

Config values which are not changed during runtime we can set in the probe function already.
The block size setting is overwritten later in meson_mmc_start_cmd anyway if needed, so it doesn't harm if we remove this setting in meson_mmc_set_ios.

In addition:
- write config register only if configuration changed
- Initialize clocks after other config registers have been initialized

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- added acked-by
v3:
- rebased
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +21 ~~-16~~

index 684cc088..e904b0a5 100644
@@ -394,15 +394,6 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	val &= ~(CFG_BUS_WIDTH_MASK << CFG_BUS_WIDTH_SHIFT);
 	val |= bus_width << CFG_BUS_WIDTH_SHIFT;
 
-	val &= ~(CFG_BLK_LEN_MASK << CFG_BLK_LEN_SHIFT);
-	val |= ilog2(SD_EMMC_CFG_BLK_SIZE) << CFG_BLK_LEN_SHIFT;
-
-	val &= ~(CFG_RESP_TIMEOUT_MASK << CFG_RESP_TIMEOUT_SHIFT);
-	val |= ilog2(SD_EMMC_CFG_RESP_TIMEOUT) << CFG_RESP_TIMEOUT_SHIFT;
-
-	val &= ~(CFG_RC_CC_MASK << CFG_RC_CC_SHIFT);
-	val |= ilog2(SD_EMMC_CFG_CMD_GAP) << CFG_RC_CC_SHIFT;
-
 	val &= ~CFG_DDR;
 	if (ios->timing == MMC_TIMING_UHS_DDR50 ||
 	    ios->timing == MMC_TIMING_MMC_DDR52 ||

@@ -413,11 +404,11 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->timing == MMC_TIMING_MMC_HS400)
 		val |= CFG_CHK_DS;
 
-	writel(val, host->regs + SD_EMMC_CFG);
-
-	if (val != orig)
+	if (val != orig) {
+		writel(val, host->regs + SD_EMMC_CFG);
 		dev_dbg(host->dev, "%s: SD_EMMC_CFG: 0x%08x -> 0x%08x\n",
 			__func__, orig, val);
+	}
 }
 
 static void meson_mmc_request_done(struct mmc_host *mmc,

@@ -695,6 +686,17 @@ static int meson_mmc_get_cd(struct mmc_host *mmc)
 	return status;
 }
 
+static void meson_mmc_cfg_init(struct meson_host *host)
+{
+	u32 cfg = 0;
+
+	cfg |= ilog2(SD_EMMC_CFG_RESP_TIMEOUT) << CFG_RESP_TIMEOUT_SHIFT;
+	cfg |= ilog2(SD_EMMC_CFG_CMD_GAP) << CFG_RC_CC_SHIFT;
+	cfg |= ilog2(SD_EMMC_CFG_BLK_SIZE) << CFG_BLK_LEN_SHIFT;
+
+	writel(cfg, host->regs + SD_EMMC_CFG);
+}
+
 static const struct mmc_host_ops meson_mmc_ops = {
 	.request	= meson_mmc_request,
 	.set_ios	= meson_mmc_set_ios,

@@ -755,10 +757,6 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_host;
 
-	ret = meson_mmc_clk_init(host);
-	if (ret)
-		goto free_host;
-
 	/* Stop execution */
 	writel(0, host->regs + SD_EMMC_START);

@@ -767,6 +765,13 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	writel(IRQ_EN_MASK, host->regs + SD_EMMC_STATUS);
 	writel(IRQ_EN_MASK, host->regs + SD_EMMC_IRQ_EN);
 
+	/* set config to sane default */
+	meson_mmc_cfg_init(host);
+
+	ret = meson_mmc_clk_init(host);
+	if (ret)
+		goto free_host;
+
 	ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
 					meson_mmc_irq_thread, IRQF_SHARED,
 					DRIVER_NAME, host);

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 19:59 UTC #9 lore raw

PATCH v4 9/10 mmc: meson-gx: remove member mrq from struct meson_host

In reply to Heiner Kallweit

Struct mmc_command includes a reference to the related mmc_request. Therefore we don't have to store mrq separately in struct meson_host. And we can remove some now unneeded WARN_ON's.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v2:
- extended commit message
- added acked-by
v3:
- rebased
v4:
- no changes

---
 drivers/mmc/host/meson-gx-mmc.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +3 ~~-21~~

index e904b0a5..5281014e 100644
@@ -122,7 +122,6 @@
 struct meson_host {
 	struct	device		*dev;
 	struct	mmc_host	*mmc;
-	struct	mmc_request	*mrq;
 	struct	mmc_command	*cmd;
 
 	spinlock_t lock;

@@ -416,9 +415,6 @@ static void meson_mmc_request_done(struct mmc_host *mmc,
 {
 	struct meson_host *host = mmc_priv(mmc);
 
-	WARN_ON(host->mrq != mrq);
-
-	host->mrq = NULL;
 	host->cmd = NULL;
 	mmc_request_done(host->mmc, mrq);
 }

@@ -525,13 +521,9 @@ static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct meson_host *host = mmc_priv(mmc);
 
-	WARN_ON(host->mrq != NULL);
-
 	/* Stop execution */
 	writel(0, host->regs + SD_EMMC_START);
 
-	host->mrq = mrq;
-
 	if (mrq->sbc)
 		meson_mmc_start_cmd(mmc, mrq->sbc);
 	else

@@ -555,7 +547,6 @@ static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
 static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 {
 	struct meson_host *host = dev_id;
-	struct mmc_request *mrq;
 	struct mmc_command *cmd;
 	u32 irq_en, status, raw_status;
 	irqreturn_t ret = IRQ_HANDLED;

@@ -565,11 +556,6 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 
 	cmd = host->cmd;
 
-	mrq = host->mrq;
-
-	if (WARN_ON(!mrq))
-		return IRQ_NONE;
-
 	if (WARN_ON(!cmd))
 		return IRQ_NONE;

@@ -616,7 +602,7 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 	else  {
 		dev_warn(host->dev, "Unknown IRQ! status=0x%04x: MMC CMD%u arg=0x%08x flags=0x%08x stop=%d\n",
 			 status, cmd->opcode, cmd->arg,
-			 cmd->flags, mrq->stop ? 1 : 0);
+			 cmd->flags, cmd->mrq->stop ? 1 : 0);
 		if (cmd->data) {
 			struct mmc_data *data = cmd->data;

@@ -643,14 +629,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 {
 	struct meson_host *host = dev_id;
-	struct mmc_request *mrq = host->mrq;
 	struct mmc_command *cmd = host->cmd;
 	struct mmc_data *data;
 	unsigned int xfer_bytes;
 
-	if (WARN_ON(!mrq))
-		return IRQ_NONE;
-
 	if (WARN_ON(!cmd))
 		return IRQ_NONE;

@@ -664,8 +646,8 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 	}
 
 	meson_mmc_read_resp(host->mmc, cmd);
-	if (!data || !data->stop || mrq->sbc)
-		meson_mmc_request_done(host->mmc, mrq);
+	if (!data || !data->stop || cmd->mrq->sbc)
+		meson_mmc_request_done(host->mmc, cmd->mrq);
 	else
 		meson_mmc_start_cmd(host->mmc, data->stop);

Signature · 1 line

2.11.1

Heiner Kallweit

author

2017-03-03 20:00 UTC #10 lore raw

PATCH v4 10/10 mmc: meson-gx: replace magic timeout numbers with constants

In reply to Heiner Kallweit

Replace timeout magic numbers with proper constants.

Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Kevin Hilman <[email protected]>

---
v3:
- added to patch series
v4:
- added acked-by

---
 drivers/mmc/host/meson-gx-mmc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/mmc/host/meson-gx-mmc.c +4 -2

index 5281014e..f244fcf9 100644
@@ -116,6 +116,8 @@
 
 #define SD_EMMC_CFG_BLK_SIZE 512 /* internal buffer max: 512 bytes */
 #define SD_EMMC_CFG_RESP_TIMEOUT 256 /* in clock cycles */
+#define SD_EMMC_CMD_TIMEOUT 1024 /* in ms */
+#define SD_EMMC_CMD_TIMEOUT_DATA 4096 /* in ms */
 #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */
 #define MUX_CLK_NUM_PARENTS 2

@@ -498,10 +500,10 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 
 		desc->cmd_data = host->bounce_dma_addr & CMD_DATA_MASK;
 
-		cmd_cfg_timeout = 12;
+		cmd_cfg_timeout = ilog2(SD_EMMC_CMD_TIMEOUT_DATA);
 	} else {
 		desc->cmd_cfg &= ~CMD_CFG_DATA_IO;
-		cmd_cfg_timeout = 10;
+		cmd_cfg_timeout = ilog2(SD_EMMC_CMD_TIMEOUT);
 	}
 	desc->cmd_cfg |= (cmd_cfg_timeout & CMD_CFG_TIMEOUT_MASK) <<
 		CMD_CFG_TIMEOUT_SHIFT;

Signature · 1 line

2.11.1

Kevin Hilman

maintainer

2017-03-03 21:19 UTC #11 lore raw

v4 cover mmc: meson-gx: series with smaller improvements

In reply to Heiner Kallweit

Quoted 1 line · Heiner Kallweit

New series is limited to smaller refactorings w/o functional changes.

I'd reviewed this series before, but hadn't actually tested it until today. I applied this series onto today's linux-next, and tested on meson-gxbb-odroidc2 and the kernel hangs up right after:

       meson-gx-mmc d0072000.mmc: Got CD GPIO

with no error message or oops/backtrace etc.

Could you clarify how you are testing this, on what tree/branch, on what hardware etc.?

Also, in the cover letter, it's customary to include a summary of what changed since the previous version(s)

Kevin

Quoted 14 lines

Heiner Kallweit (10):
mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
mmc: meson-gx: make two functions return void
mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
mmc: meson-gx: remove member parent_mux from struct meson_host
mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
mmc: meson-gx: improve initial configuration
mmc: meson-gx: remove member mrq from struct meson_host
mmc: meson-gx: replace magic timeout numbers with constants
 drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 76 deletions(-)

Heiner Kallweit

author

2017-03-03 22:54 UTC #12 lore raw

v4 cover mmc: meson-gx: series with smaller improvements

Am 03.03.2017 um 22:19 schrieb Kevin Hilman:

Quoted 8 lines

Quoted 1 line · Heiner Kallweit
New series is limited to smaller refactorings w/o functional changes.
I'd reviewed this series before, but hadn't actually tested it until today. I applied this series onto today's linux-next, and tested on meson-gxbb-odroidc2 and the kernel hangs up right after:

Quoted 7 lines

       meson-gx-mmc d0072000.mmc: Got CD GPIO
with no error message or oops/backtrace etc.
Could you clarify how you are testing this, on what tree/branch, on what hardware etc.?

I'm testing on Odroid C2 with a self-built uboot based on the latest mainline uboot incl. an own eMMC driver which was submitted but is not yet applied to mainline uboot.
The system is running headless with a serial console attached. Storage is a 128 GB Hardkernel eMMC card.

I use latest next kernel + the patches to test.

Does your system work w/o the current patch set?
And do you use HS200 or HS400 mode?

I also figured out that 200MHz w/o tuning is a little fragile and reduced the clock to 60 MHz. This makes no difference in performance as the driver currently is very slow anyway (only 10 - 15 MB/s). When the clock is too high I see lots of CRC errors on the serial console.

This will change with further patches I have in my tree. They allow stable HS200/HS400 with quite basic tuning resulting in 140 MB/s read performance.
Stable configuration here is: 180? core clock phase, 0? tx clock phase, 180? rx clock phase.

Quoted 20 lines

Also, in the cover letter, it's customary to include a summary of what changed since the previous version(s)
Kevin
Quoted 14 lines
Heiner Kallweit (10):
mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
mmc: meson-gx: make two functions return void
mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
mmc: meson-gx: remove member parent_mux from struct meson_host
mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
mmc: meson-gx: improve initial configuration
mmc: meson-gx: remove member mrq from struct meson_host
mmc: meson-gx: replace magic timeout numbers with constants
 drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 76 deletions(-)

Kevin Hilman

maintainer

2017-03-04 05:33 UTC #13 lore raw

v4 cover mmc: meson-gx: series with smaller improvements

Quoted 22 lines · Heiner Kallweit

Am 03.03.2017 um 22:19 schrieb Kevin Hilman:
Quoted 8 lines
Quoted 1 line · Heiner Kallweit
New series is limited to smaller refactorings w/o functional changes.
I'd reviewed this series before, but hadn't actually tested it until today. I applied this series onto today's linux-next, and tested on meson-gxbb-odroidc2 and the kernel hangs up right after:
Quoted 7 lines
       meson-gx-mmc d0072000.mmc: Got CD GPIO
with no error message or oops/backtrace etc.
Could you clarify how you are testing this, on what tree/branch, on what hardware etc.?
I'm testing on Odroid C2 with a self-built uboot based on the latest mainline uboot incl. an own eMMC driver which was submitted but is not yet applied to mainline uboot.
The system is running headless with a serial console attached. Storage is a 128 GB Hardkernel eMMC card.

Quoted 3 lines

I use latest next kernel + the patches to test.
Does your system work w/o the current patch set?

Yes.

Quoted 1 line

And do you use HS200 or HS400 mode?

I don't remember what kind of card is plugged in, and I'm away from the board currently.

Since you have a custom uboot, with a custom MMC driver, I suspect that your uboot is initializing something that the kernel is not, so when a kernel is used that's not using your uboot, something goes wrong.

Any chance you can try with the vendor uboot from Hardkernel?

I tested on odroid-c2 because someone on the #linux-amlogic IRC channel (webczat) reported the hang when testing your patches, so I tried to reproduce and got the same hang.

Until we figure out what the hang is, this series should not be merged.

Kevin

Quoted 32 lines

I also figured out that 200MHz w/o tuning is a little fragile and reduced the clock to 60 MHz. This makes no difference in performance as the driver currently is very slow anyway (only 10 - 15 MB/s). When the clock is too high I see lots of CRC errors on the serial console.
This will change with further patches I have in my tree. They allow stable HS200/HS400 with quite basic tuning resulting in 140 MB/s read performance.
Stable configuration here is: 180? core clock phase, 0? tx clock phase, 180? rx clock phase.
Quoted 20 lines
Also, in the cover letter, it's customary to include a summary of what changed since the previous version(s)
Kevin
Quoted 14 lines
Heiner Kallweit (10):
mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
mmc: meson-gx: make two functions return void
mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
mmc: meson-gx: remove member parent_mux from struct meson_host
mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
mmc: meson-gx: improve initial configuration
mmc: meson-gx: remove member mrq from struct meson_host
mmc: meson-gx: replace magic timeout numbers with constants
 drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 76 deletions(-)

Kevin Hilman

maintainer

2017-03-04 05:36 UTC #14 lore raw

v4 cover mmc: meson-gx: series with smaller improvements

Quoted 28 lines · Kevin Hilman

Quoted 22 lines · Heiner Kallweit
Am 03.03.2017 um 22:19 schrieb Kevin Hilman:
Quoted 8 lines
Quoted 1 line · Heiner Kallweit
New series is limited to smaller refactorings w/o functional changes.
I'd reviewed this series before, but hadn't actually tested it until today. I applied this series onto today's linux-next, and tested on meson-gxbb-odroidc2 and the kernel hangs up right after:
Quoted 7 lines
       meson-gx-mmc d0072000.mmc: Got CD GPIO
with no error message or oops/backtrace etc.
Could you clarify how you are testing this, on what tree/branch, on what hardware etc.?
I'm testing on Odroid C2 with a self-built uboot based on the latest mainline uboot incl. an own eMMC driver which was submitted but is not yet applied to mainline uboot.
The system is running headless with a serial console attached. Storage is a 128 GB Hardkernel eMMC card.
Quoted 3 lines
I use latest next kernel + the patches to test.
Does your system work w/o the current patch set?

Quoted 8 lines

Yes.
Quoted 1 line
And do you use HS200 or HS400 mode?
I don't remember what kind of card is plugged in, and I'm away from the board currently.
From a boot of linux-next without this series, look like it's detecting HS200:

root at buildroot:/# dmesg |grep -i mmc

[    1.359545] meson-gx-mmc d0074000.mmc: allocated mmc-pwrseq
[    1.632159] meson-gx-mmc d0072000.mmc: Got CD GPIO
[    1.778734] mmc1: new high speed SDHC card at address 1234
[    1.779007] mmcblk1: mmc1:1234 SA08G 7.21 GiB
[    1.784197]  mmcblk1: p1
[    1.925634] mmc0: new HS200 MMC card at address 0001
[    1.925875] mmcblk0: mmc0:0001 SDW16G 14.7 GiB
[    1.929550] mmcblk0boot0: mmc0:0001 SDW16G partition 1 4.00 MiB
[    1.935404] mmcblk0boot1: mmc0:0001 SDW16G partition 2 4.00 MiB
[    1.942428]  mmcblk0: p1 p2

Kevin