Skip to contentSkip to conversation

mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd

PATCH Superseded Heiner Kallweit 3 participants 18 messages Feb 18, 2017 – Mar 3, 2017

Subsystems (2)
Arm/Amlogic Meson SoC support MMC, SD and SDIO

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Heiner Kallweit

author
#0 lore raw

PATCH v3 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

---
 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
author
#1 lore raw

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

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

---
 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
author
#2 lore raw

PATCH v3 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

---
 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
author
#3 lore raw

PATCH v3 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

---
 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
author
#4 lore raw

PATCH v3 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

---
 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
author
#5 lore raw

PATCH v3 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]>

---
v2:
- extended commit message
v3:
- adjust error path in probe

---
 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
#6 lore raw

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

W dniu 18.02.2017 o 14:19, Heiner Kallweit pisze:

Quoted 51 lines

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]>

---
v2:
- extended commit message
v3:
- adjust error path in probe

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/meson-gx-mmc.c +3 -3
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;

drivers/mmc/host/meson-gx-mmc.c static int meson_mmc_probe(struct platform_device *pdev) · line 800

Quoted 10 lines
drivers/mmc/host/meson-gx-mmc.c +2 -1
@@ -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;

Doesn't the same problem exist with a core clock? It also could error when being enabled, and it will be disabled even if not enabled before...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 525 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170218/7ebfe0b4/attachment.sig>

author
#7 lore raw

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

Am 18.02.2017 um 14:57 schrieb Micha? Zegan:

Quoted 56 lines

W dniu 18.02.2017 o 14:19, Heiner Kallweit pisze:

Quoted 53 lines

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]>

---
v2:
- extended commit message
v3:
- adjust error path in probe

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/meson-gx-mmc.c +3 -3
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;
Quoted 11 lines

drivers/mmc/host/meson-gx-mmc.c static int meson_mmc_probe(struct platform_device *pdev) · line 800

Quoted 8 lines
drivers/mmc/host/meson-gx-mmc.c +2 -1
-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;

Doesn't the same problem exist with a core clock? It also could error when being enabled, and it will be disabled even if not enabled before...

Yes, there it's basically the same issue. Feel free to submit a patch.

#8 lore raw

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

W dniu 18.02.2017 o 16:22, Heiner Kallweit pisze:

Quoted 68 lines

Am 18.02.2017 um 14:57 schrieb Micha? Zegan:

Quoted 56 lines

W dniu 18.02.2017 o 14:19, Heiner Kallweit pisze:

Quoted 53 lines

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]>

---
v2:
- extended commit message
v3:
- adjust error path in probe

---
 drivers/mmc/host/meson-gx-mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/mmc/host/meson-gx-mmc.c +3 -3
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;
Quoted 11 lines

drivers/mmc/host/meson-gx-mmc.c static int meson_mmc_probe(struct platform_device *pdev) · line 800

Quoted 8 lines
drivers/mmc/host/meson-gx-mmc.c +2 -1
-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;

Doesn't the same problem exist with a core clock? It also could error when being enabled, and it will be disabled even if not enabled before...

Quoted 2 lines

Yes, there it's basically the same issue. Feel free to submit a patch.

Okay, I will submit a patch based on this series to fix that too, thanks.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 525 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170218/8d1750e3/attachment.sig>

maintainer
#9 lore raw

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

In reply to Heiner Kallweit
Quoted 9 lines · 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]>
Quoted 51 lines

---
v2:
- extended commit message
v3:
- adjust error path in probe

---
 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;
author
#10 lore raw

PATCH v3 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

---
 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
author
#11 lore raw

PATCH v3 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

---
 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
author
#12 lore raw

PATCH v3 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

---
 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
author
#13 lore raw

PATCH v3 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]>

---
v3:
- added to patch series

---
 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
maintainer
#14 lore raw

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

Quoted 3 lines · Heiner Kallweit

Replace timeout magic numbers with proper constants.

Signed-off-by Heiner Kallweit <[email protected]>
Acked-by Kevin Hilman <[email protected]>
Quoted 33 lines

---
v3:
- added to patch series

---
 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;
maintainer
#15 lore raw

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

In reply to Heiner Kallweit

Hi Heinar,

Quoted 6 lines · Heiner Kallweit

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]>

Could you send this series one more time, collecting the remaining tags and including the patches from Micha? Zegan?

I think the MMC maintainers would appreciate one final batch with everything collected together, and including a cover letter.

Thanks for all your great work on improving this driver,

Kevin

author
#16 lore raw

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

Am 01.03.2017 um 19:09 schrieb Kevin Hilman:

Quoted 8 lines

Hi Heinar,

Quoted 4 lines · Heiner Kallweit

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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Heiner Kallweit <[email protected]>
Acked-by Kevin Hilman <[email protected]>

Could you send this series one more time, collecting the remaining tags and including the patches from Micha? Zegan?

Patchwork tracks all the tags coming in for a patch, there it's visible that you acked my 10 patches + the two from Michal.
So I would leave it up to Ulf whether he prefers to have the series re-sent (with just the few remaining acks as changes).

Heiner

Quoted 7 lines

I think the MMC maintainers would appreciate one final batch with everything collected together, and including a cover letter.

Thanks for all your great work on improving this driver,

Kevin

maintainer
#17 lore raw

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

Quoted 9 lines · Heiner Kallweit

Am 01.03.2017 um 19:09 schrieb Kevin Hilman:

Quoted 8 lines

Hi Heinar,

Quoted 4 lines · Heiner Kallweit

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.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Signed-off-by Heiner Kallweit <[email protected]>
Acked-by Kevin Hilman <[email protected]>

Could you send this series one more time, collecting the remaining tags and including the patches from Micha? Zegan?

Patchwork tracks all the tags coming in for a patch, there it's visible that you acked my 10 patches + the two from Michal.

Yes, I'm well aware of how patchwork works, yet my request still stands.

Quoted 2 lines

So I would leave it up to Ulf whether he prefers to have the series re-sent (with just the few remaining acks as changes).

That's your choice, but some friendly advice from a kernel maintainer:
making things as easy as possible for maintainers/reviewers greatly improves speed your code will get merged.

Kevin