mt7621-mmc driver improvements

mt7621-mmc driver improvements

[George Hilliard]

This is a series of patches to provide a little TLC for the mt7621-mmc driver.
My original goal was to get it working on the MT7688, and I have succeeded.  I
suspect it will now work on any of the MT762x line.  The main change was
getting the driver to use the pinctrl subsystem instead of hand-jamming the
pinctrl registers -- the bit offsets were only correct for the MT7621.

Because of this change, the driver now expects a pinctrl device reference in
the mmc controller's device tree node; without it, it will bail out.  This
could break existing setups that don't specify it because it "just worked" up
until now.  So currently I just let the old behavior fall away because this is
a staging driver.  But if this is a problem, the old behavior could be added
back as a fallback.

Beyond that, there are largely code cleanups and a couple other correctness
fixes that I hope are self-explanatory.  The TODO list is largely unchanged,
aside from the couple of TODO comments in the code that I have addressed.
Ultimately, I think this driver could potentially be merged with the "real"
mtk-mmc driver as the TODO suggests, but someone who is more familiar with the
IP core will have to do that.  Mediatek documentation (that I can find) is very
sparse.  Besides, their codebases have begun to diverge.

Feedback welcome!

[PATCH 01/10] staging: mt7621-mmc: fix unused variable compiler warning

[George Hilliard]

The compiler complains:

    drivers/staging/mt7621-mmc/dbg.c: In function ‘msdc_debug_proc_write’:
    drivers/staging/mt7621-mmc/dbg.c:237:12: warning: unused variable ‘size’ [-Wunused-variable]
      int mode, size;
                ^~~~
    drivers/staging/mt7621-mmc/dbg.c:237:6: warning: unused variable ‘mode’ [-Wunused-variable]
      int mode, size;
          ^~~~

Remove these declarations.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/dbg.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/mt7621-mmc/dbg.c b/drivers/staging/mt7621-mmc/dbg.c
index c7c091fa1da0..69b38c7e3179 100644
--- a/drivers/staging/mt7621-mmc/dbg.c
+++ b/drivers/staging/mt7621-mmc/dbg.c
@@ -233,7 +233,6 @@ static ssize_t msdc_debug_proc_write(struct file *file,
 
 	int cmd, p1, p2;
 	int id, zone;
-	int mode, size;
 
 	if (count == 0)
 		return -1;
-- 
2.20.1

[PATCH 02/10] staging: mt7621-mmc: Remove obsolete comments

[George Hilliard]

These comments don't contain useful code or alternate implementation
ideas.  Remove them.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 4b26ec896a96..1d357b898952 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -100,7 +100,6 @@ static int cd_active_low = 1;
 struct msdc_hw msdc0_hw = {
 	.clk_src        = 0,
 	.flags          = MSDC_CD_PIN_EN | MSDC_REMOVABLE,
-//	.flags          = MSDC_WP_PIN_EN | MSDC_CD_PIN_EN | MSDC_REMOVABLE,
 };
 
 /* end of +++ */
@@ -1671,7 +1670,6 @@ static int msdc_drv_probe(struct platform_device *pdev)
 	host->timeout_clks = DEFAULT_DTOC * 65536;
 
 	host->mrq = NULL;
-	//init_MUTEX(&host->sem); /* we don't need to support multiple threads access */
 
 	dma_coerce_mask_and_coherent(mmc_dev(mmc), DMA_BIT_MASK(32));
 
-- 
2.20.1

[PATCH 03/10] staging: mt7621-mmc: Remove references to nonexistent mt7621_spi_ops

[George Hilliard]

This struct does not exist, and when it is looked up in the
compatibility tree, it returns null.  Remove these nonfunctional lines.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-spi/spi-mt7621.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index b509f9fe3346..d260b42ff328 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -58,8 +58,6 @@ struct mt7621_spi {
 	unsigned int		speed;
 	struct clk		*clk;
 	int			pending_write;
-
-	struct mt7621_spi_ops	*ops;
 };
 
 static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi)
@@ -330,13 +328,11 @@ static int mt7621_spi_probe(struct platform_device *pdev)
 	struct resource *r;
 	int status = 0;
 	struct clk *clk;
-	struct mt7621_spi_ops *ops;
 	int ret;
 
 	match = of_match_device(mt7621_spi_match, &pdev->dev);
 	if (!match)
 		return -EINVAL;
-	ops = (struct mt7621_spi_ops *)match->data;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, r);
@@ -375,7 +371,6 @@ static int mt7621_spi_probe(struct platform_device *pdev)
 	rs->clk = clk;
 	rs->master = master;
 	rs->sys_freq = clk_get_rate(rs->clk);
-	rs->ops = ops;
 	rs->pending_write = 0;
 	dev_info(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
 
-- 
2.20.1

[PATCH 04/10] staging: mt7621-mmc: Fix warning when reloading module with debug msgs enabled

[George Hilliard]

The kernel complained:

    [  510.277151] WARNING: CPU: 0 PID: 395 at fs/proc/generic.c:360 proc_register+0xf0/0x108
    [  510.292891] proc_dir_entry '/proc/msdc_debug' already registered

when doing a modprobe/rmmod/modprobe of this module if debug messages
are compiled in.  Fix this by removing the proc entry when the module is
unloaded.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/dbg.c | 15 ++++++++++++---
 drivers/staging/mt7621-mmc/dbg.h |  3 ++-
 drivers/staging/mt7621-mmc/sd.c  |  4 ++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-mmc/dbg.c b/drivers/staging/mt7621-mmc/dbg.c
index 69b38c7e3179..5764b80893e9 100644
--- a/drivers/staging/mt7621-mmc/dbg.c
+++ b/drivers/staging/mt7621-mmc/dbg.c
@@ -295,9 +295,18 @@ static const struct file_operations msdc_debug_fops = {
 	.release	= single_release,
 };
 
-void msdc_debug_proc_init(void)
+// Keep ahold of the proc entry we create so it can be freed during
+// module removal
+struct proc_dir_entry *msdc_debug_proc_entry;
+
+void __init msdc_debug_proc_init(void)
 {
-	proc_create("msdc_debug", 0660, NULL, &msdc_debug_fops);
+	msdc_debug_proc_entry = proc_create("msdc_debug", 0660,
+					    NULL, &msdc_debug_fops);
+}
+
+void __exit msdc_debug_proc_deinit(void)
+{
+	proc_remove(msdc_debug_proc_entry);
 }
-EXPORT_SYMBOL_GPL(msdc_debug_proc_init);
 #endif
diff --git a/drivers/staging/mt7621-mmc/dbg.h b/drivers/staging/mt7621-mmc/dbg.h
index 2d447b2d92ae..d100324aa3fe 100644
--- a/drivers/staging/mt7621-mmc/dbg.h
+++ b/drivers/staging/mt7621-mmc/dbg.h
@@ -93,7 +93,8 @@ enum msdc_dbg {
 
 extern unsigned int sd_debug_zone[4];
 #define TAG "msdc"
-void msdc_debug_proc_init(void);
+void __init msdc_debug_proc_init(void);
+void __exit msdc_debug_proc_deinit(void);
 
 u32 msdc_time_calc(u32 old_L32, u32 old_H32, u32 new_L32, u32 new_H32);
 void msdc_performance(u32 opcode, u32 sizes, u32 bRx, u32 ticks);
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 1d357b898952..a7c7ec0d7bbb 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1843,6 +1843,10 @@ static int __init mt_msdc_init(void)
 
 static void __exit mt_msdc_exit(void)
 {
+#if defined(MT6575_SD_DEBUG)
+	msdc_debug_proc_deinit();
+#endif
+
 	platform_driver_unregister(&mt_msdc_driver);
 }
 
-- 
2.20.1

[PATCH 05/10] staging: mt7621-mmc: Use pinctrl subsystem to select SDXC pin mode

[George Hilliard]

The driver previously grabbed the SD pins for itself, ignoring the pin
controller.  Replace this direct register access with appropriate calls
to the pinctrl subsystem.

This also allows this driver to work on related devices that have a
different pin controller mapping, such as the MT7688.  The hardcoded
bit index was incorrect on that device.

This change could break SD controller functionality on existing devices
whose device trees do not specify a pin controller and state for the SD
node.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index a7c7ec0d7bbb..97ed7510e96d 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -38,6 +38,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/interrupt.h>
 
 #include <linux/mmc/host.h>
@@ -1599,6 +1600,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
 	struct msdc_host *host;
 	struct msdc_hw *hw;
 	int ret;
+	struct pinctrl *pctrl;
+	struct pinctrl_state *pins_default;
 
 	hw = &msdc0_hw;
 
@@ -1671,6 +1674,25 @@ static int msdc_drv_probe(struct platform_device *pdev)
 
 	host->mrq = NULL;
 
+	/* Read pin control settings from device tree */
+	pctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(pctrl)) {
+		ret = PTR_ERR(pctrl);
+		dev_err(&pdev->dev, "Cannot find pinctrl in device tree\n");
+		goto host_free;
+	}
+
+	pins_default = pinctrl_lookup_state(pctrl, PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(pins_default)) {
+		ret = PTR_ERR(pins_default);
+		dev_err(&pdev->dev, "Cannot find pinctrl state default\n");
+		goto host_free;
+	}
+
+	ret = pinctrl_select_state(pctrl, pins_default);
+	if (ret < 0)
+		dev_warn(&pdev->dev, "Cannot select pinctrl state\n");
+
 	dma_coerce_mask_and_coherent(mmc_dev(mmc), DMA_BIT_MASK(32));
 
 	/* using dma_alloc_coherent*/  /* todo: using 1, for all 4 slots */
@@ -1822,12 +1844,6 @@ static struct platform_driver mt_msdc_driver = {
 static int __init mt_msdc_init(void)
 {
 	int ret;
-	u32 reg;
-
-	// Set the pins for sdxc to sdxc mode
-	//FIXME: this should be done by pinctl and not by the sd driver
-	reg = readl((void __iomem *)(RALINK_SYSCTL_BASE + 0x60)) & ~(0x3 << 18);
-	writel(reg, (void __iomem *)(RALINK_SYSCTL_BASE + 0x60));
 
 	ret = platform_driver_register(&mt_msdc_driver);
 	if (ret) {
-- 
2.20.1

Re: [PATCH 05/10] staging: mt7621-mmc: Use pinctrl subsystem to select SDXC pin mode

[Jonas Gorski]

On Wed, 20 Feb 2019 at 06:18, George Hilliard
<thirtythreeforty@gmail.com> wrote:
>
> The driver previously grabbed the SD pins for itself, ignoring the pin
> controller.  Replace this direct register access with appropriate calls
> to the pinctrl subsystem.
>
> This also allows this driver to work on related devices that have a
> different pin controller mapping, such as the MT7688.  The hardcoded
> bit index was incorrect on that device.
>
> This change could break SD controller functionality on existing devices
> whose device trees do not specify a pin controller and state for the SD
> node.
>
> Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
> ---
>  drivers/staging/mt7621-mmc/sd.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index a7c7ec0d7bbb..97ed7510e96d 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -38,6 +38,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/spinlock.h>
>  #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/interrupt.h>
>
>  #include <linux/mmc/host.h>
> @@ -1599,6 +1600,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
>         struct msdc_host *host;
>         struct msdc_hw *hw;
>         int ret;
> +       struct pinctrl *pctrl;
> +       struct pinctrl_state *pins_default;
>
>         hw = &msdc0_hw;
>
> @@ -1671,6 +1674,25 @@ static int msdc_drv_probe(struct platform_device *pdev)
>
>         host->mrq = NULL;
>
> +       /* Read pin control settings from device tree */
> +       pctrl = devm_pinctrl_get(&pdev->dev);
> +       if (IS_ERR(pctrl)) {
> +               ret = PTR_ERR(pctrl);
> +               dev_err(&pdev->dev, "Cannot find pinctrl in device tree\n");
> +               goto host_free;
> +       }
> +
> +       pins_default = pinctrl_lookup_state(pctrl, PINCTRL_STATE_DEFAULT);
> +       if (IS_ERR(pins_default)) {
> +               ret = PTR_ERR(pins_default);
> +               dev_err(&pdev->dev, "Cannot find pinctrl state default\n");
> +               goto host_free;
> +       }
> +
> +       ret = pinctrl_select_state(pctrl, pins_default);
> +       if (ret < 0)
> +               dev_warn(&pdev->dev, "Cannot select pinctrl state\n");
> +

Selecting the PINCTRL_STATE_DEFAULT is already automatically done by
the driver core on probe time[1], before calling the probe function,
so this seems redundant here. The only difference is that you enforce
the presence of a pinctrl node, while the core code is fine without
one.


Regards
Jonas

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/pinctrl.c#L21

[PATCH 06/10] staging: mt7621-mmc: Bill the caller for I/O time

[George Hilliard]

When waiting on completions, use the _io variant so the caller is
charged as using I/O.

This should have no effect on the module's functionality, only improve
CPU accounting.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 97ed7510e96d..f12f9d6611c9 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -494,7 +494,7 @@ static unsigned int msdc_command_resp(struct msdc_host   *host,
 	//sdr_set_bits(host->base + MSDC_INTEN, wints);
 
 	spin_unlock(&host->lock);
-	if (!wait_for_completion_timeout(&host->cmd_done, 10 * timeout)) {
+	if (!wait_for_completion_io_timeout(&host->cmd_done, 10 * timeout)) {
 		dev_err(mmc_dev(host->mmc),
 			"%d -> XXX CMD<%d> wait_for_completion timeout ARG<0x%.8x>\n",
 			host->id, opcode, cmd->arg);
@@ -697,7 +697,7 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		msdc_dma_start(host);
 
 		spin_unlock(&host->lock);
-		if (!wait_for_completion_timeout(&host->xfer_done, DAT_TIMEOUT)) {
+		if (!wait_for_completion_io_timeout(&host->xfer_done, DAT_TIMEOUT)) {
 			dev_err(mmc_dev(host->mmc),
 				"%d -> XXX CMD<%d> wait xfer_done<%d> timeout!!\n",
 				host->id, cmd->opcode,
-- 
2.20.1

[PATCH 07/10] staging: mt7621-mmc: Initialize completions a single time during probe

[George Hilliard]

The module was initializing completions whenever it was going to wait on
them, and not when the completion was allocated.  This is incorrect
according to the completion docs:

    Calling init_completion() on the same completion object twice is
    most likely a bug [...]

Re-initialization is also unnecessary because the module never uses
complete_all().  Fix this by only ever initializing the completion a
single time.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index f12f9d6611c9..942c0d63d710 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -468,7 +468,9 @@ static unsigned int msdc_command_start(struct msdc_host   *host,
 	host->cmd     = cmd;
 	host->cmd_rsp = resp;
 
-	init_completion(&host->cmd_done);
+	// The completion should have been consumed by the previous command
+	// response handler, because the mmc requests should be serialized
+	BUG_ON(completion_done(&host->cmd_done));
 
 	sdr_set_bits(host->base + MSDC_INTEN, wints);
 	sdc_send_cmd(rawcmd, cmd->arg);
@@ -490,7 +492,6 @@ static unsigned int msdc_command_resp(struct msdc_host   *host,
 		    MSDC_INT_ACMD19_DONE;
 
 	BUG_ON(in_interrupt());
-	//init_completion(&host->cmd_done);
 	//sdr_set_bits(host->base + MSDC_INTEN, wints);
 
 	spin_unlock(&host->lock);
@@ -674,7 +675,10 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		//msdc_clr_fifo(host);  /* no need */
 
 		msdc_dma_on();  /* enable DMA mode first!! */
-		init_completion(&host->xfer_done);
+
+		// The completion should have been consumed by the previous xfer
+		// response handler, because the mmc requests should be serialized
+		BUG_ON(completion_done(&host->xfer_done));
 
 		/* start the command first*/
 		if (msdc_command_start(host, cmd, CMD_TIMEOUT) != 0)
@@ -693,7 +697,6 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		/* for read, the data coming too fast, then CRC error
 		 *  start DMA no business with CRC.
 		 */
-		//init_completion(&host->xfer_done);
 		msdc_dma_start(host);
 
 		spin_unlock(&host->lock);
@@ -1708,6 +1711,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
 	}
 	msdc_init_gpd_bd(host, &host->dma);
 
+	init_completion(&host->cmd_done);
+	init_completion(&host->xfer_done);
 	INIT_DELAYED_WORK(&host->card_delaywork, msdc_tasklet_card);
 	spin_lock_init(&host->lock);
 	msdc_init_hw(host);
-- 
2.20.1

[PATCH 08/10] staging: mt7621-mmc: Check for nonzero number of scatterlist entries

[George Hilliard]

The buffer descriptor setup loop is correct only if it is setting up at
least one bd struct.  Besides, there is an error somewhere if
dma_map_sg() returns 0.  So add a paranoid check for this condition.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 942c0d63d710..736e1d23b391 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -594,7 +594,12 @@ static void msdc_dma_setup(struct msdc_host *host, struct msdc_dma *dma,
 	struct bd *bd;
 	u32 j;
 
-	BUG_ON(sglen > MAX_BD_NUM); /* not support currently */
+	// Shouldn't happen; we configure the mmc host layer
+	// based on MAX_BD_NUM:
+	BUG_ON(sglen > MAX_BD_NUM);
+	// Correct setup below requires at least one bd
+	// (and dma_map_sg should not return 0):
+	BUG_ON(sglen == 0);
 
 	gpd = dma->gpd;
 	bd  = dma->bd;
-- 
2.20.1

[PATCH 09/10] staging: mt7621-mmc: Remove redundant host->mmc->f_max write

[George Hilliard]

This is set once during initialization and never changed.  Don't bother
setting it again in the interrupt handler.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 736e1d23b391..472318a979cb 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -210,7 +210,6 @@ static void msdc_tasklet_card(struct work_struct *work)
 	host->card_inserted = inserted;
 
 	if (!host->suspend) {
-		host->mmc->f_max = HOST_MAX_MCLK;
 		mmc_detect_change(host->mmc, msecs_to_jiffies(20));
 	}
 
-- 
2.20.1

[PATCH 10/10] staging: mt7621-mmc: Immediately notify mmc layer of card change detection

[George Hilliard]

There is no need to delay notifying the mmc layer.  Schedule the delayed
work to run immediately.

Signed-off-by: George Hilliard <thirtythreeforty@gmail.com>
---
 drivers/staging/mt7621-mmc/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 472318a979cb..0a7f8384edf8 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1355,7 +1355,7 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
 	if (intsts & MSDC_INT_CDSC) {
 		if (host->mmc->caps & MMC_CAP_NEEDS_POLL)
 			return IRQ_HANDLED;
-		schedule_delayed_work(&host->card_delaywork, HZ);
+		schedule_delayed_work(&host->card_delaywork, 0);
 		/* tuning when plug card ? */
 	}
 
-- 
2.20.1