[RFC PATCH v2 0/6] Try to get rid of CONFIG_MMC_DEBUG

[RFC PATCH v2 0/6] Try to get rid of CONFIG_MMC_DEBUG

[Shawn Lin]

CONFIG_MMC_DEBUG was introduced long time ago and mostly
it's a all-or-none proposition for compile-time. By looking
at how the mmc core use it, it's obviously pointless to still
carry on this option.

But referring to the host drivers, it's hard to say should we
remove it now. The following drivers[1] will print some extra log
on irq context for debugging. Probably folks still need to debug
it sometimes but making it default behaviour seems to introduce some
unnecessary burden for their drivers I guess. Anyway, we downgrade this
option for hosts only and ask the following added host drivers to use
this for debugging instead of adding their private macro to activate
their debug functions.

This patchset also cleanup wbsd and improve sdhci a bit.

[1]:
drivers/mmc/host/s3cmci.c
drivers/mmc/host/omap.c
drivers/mmc/host/omap_hsmmc.c
drivers/mmc/host/tmio_mmc_core.c


Changes in v2:
- rework the changes for sdhci suggested by Adrian

Shawn Lin (6):
  mmc: core: remove check of host->removed for rescan routine
  mmc: core: always check the length of sglist with total data size
  mmc: core: turn the pr_info under CONFIG_MMC_DEBUG into pr_debug
  mmc: Kconfig: downgrade CONFIG_MMC_DEBUG for host drivers only
  mmc: wbsd: remove CONFIG_MMC_DEBUG from the driver
  mmc: sdhci: remove CONFIG_MMC_DEBUG from the driver

 drivers/mmc/Kconfig      |  7 -------
 drivers/mmc/Makefile     |  2 --
 drivers/mmc/core/core.c  | 35 +++++++----------------------------
 drivers/mmc/host/Kconfig |  9 +++++++++
 drivers/mmc/host/sdhci.c | 15 +++++++--------
 drivers/mmc/host/wbsd.c  |  2 --
 include/linux/mmc/host.h |  3 ---
 7 files changed, 23 insertions(+), 50 deletions(-)

-- 
1.9.1

[RFC PATCH v2 1/6] mmc: core: remove check of host->removed for rescan routine

[Shawn Lin]

The intention of this check was to prevent the conflict between
hotplug and removing driver for whatever reason. Currently it
doesn't improve anything and the following rescan process could
still saftly perform the scan flow. So these code seems pointless
now and let's remove them.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/core/core.c  | 13 -------------
 include/linux/mmc/host.h |  3 ---
 2 files changed, 16 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2643126..b311ec9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1769,13 +1769,6 @@ void mmc_detach_bus(struct mmc_host *host)
 static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
 				bool cd_irq)
 {
-#ifdef CONFIG_MMC_DEBUG
-	unsigned long flags;
-	spin_lock_irqsave(&host->lock, flags);
-	WARN_ON(host->removed);
-	spin_unlock_irqrestore(&host->lock, flags);
-#endif
-
 	/*
 	 * If the device is configured as wakeup, we prevent a new sleep for
 	 * 5 s to give provision for user space to consume the event.
@@ -2646,12 +2639,6 @@ void mmc_start_host(struct mmc_host *host)
 
 void mmc_stop_host(struct mmc_host *host)
 {
-#ifdef CONFIG_MMC_DEBUG
-	unsigned long flags;
-	spin_lock_irqsave(&host->lock, flags);
-	host->removed = 1;
-	spin_unlock_irqrestore(&host->lock, flags);
-#endif
 	if (host->slot.cd_irq >= 0) {
 		if (host->slot.cd_wake_enabled)
 			disable_irq_wake(host->slot.cd_irq);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ebd1ceb..4617c21 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -328,9 +328,6 @@ struct mmc_host {
 	unsigned int		use_spi_crc:1;
 	unsigned int		claimed:1;	/* host exclusively claimed */
 	unsigned int		bus_dead:1;	/* bus has been released */
-#ifdef CONFIG_MMC_DEBUG
-	unsigned int		removed:1;	/* host is being removed */
-#endif
 	unsigned int		can_retune:1;	/* re-tuning can be used */
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
-- 
1.9.1

[RFC PATCH v2 2/6] mmc: core: always check the length of sglist with total data size

[Shawn Lin]

All the check within mmc_mrq_prep seems to be all-or-none
proposition, so it doesn't make sense to only check the
length of sglist only under the CONFIG_MMC_DEBUG context.
I'd prefer to always keep the check there unconditionally.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b311ec9..634f9ca 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -295,10 +295,8 @@ static void mmc_mrq_pr_debug(struct mmc_host *host, struct mmc_request *mrq)
 
 static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
 {
-#ifdef CONFIG_MMC_DEBUG
-	unsigned int i, sz;
+	unsigned int i, sz = 0;
 	struct scatterlist *sg;
-#endif
 
 	if (mrq->cmd) {
 		mrq->cmd->error = 0;
@@ -314,13 +312,12 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
 		    mrq->data->blocks > host->max_blk_count ||
 		    mrq->data->blocks * mrq->data->blksz > host->max_req_size)
 			return -EINVAL;
-#ifdef CONFIG_MMC_DEBUG
-		sz = 0;
+
 		for_each_sg(mrq->data->sg, sg, mrq->data->sg_len, i)
 			sz += sg->length;
 		if (sz != mrq->data->blocks * mrq->data->blksz)
 			return -EINVAL;
-#endif
+
 		mrq->data->error = 0;
 		mrq->data->mrq = mrq;
 		if (mrq->stop) {
-- 
1.9.1

[RFC PATCH v2 3/6] mmc: core: turn the pr_info under CONFIG_MMC_DEBUG into pr_debug

[Shawn Lin]

There are lots of debug message in core.c which use pr_debug
for better dynamic log level control. So it doesn't make sense
for those print to still keep working only under CONFIG_MMC_DEBUG.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/core/core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 634f9ca..731aa9f 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2436,10 +2436,9 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 {
 	host->f_init = freq;
 
-#ifdef CONFIG_MMC_DEBUG
-	pr_info("%s: %s: trying to init card at %u Hz\n",
+	pr_debug("%s: %s: trying to init card at %u Hz\n",
 		mmc_hostname(host), __func__, host->f_init);
-#endif
+
 	mmc_power_up(host, host->ocr_avail);
 
 	/*
@@ -2670,9 +2669,7 @@ int mmc_power_save_host(struct mmc_host *host)
 {
 	int ret = 0;
 
-#ifdef CONFIG_MMC_DEBUG
-	pr_info("%s: %s: powering down\n", mmc_hostname(host), __func__);
-#endif
+	pr_debug("%s: %s: powering down\n", mmc_hostname(host), __func__);
 
 	mmc_bus_get(host);
 
@@ -2696,9 +2693,7 @@ int mmc_power_restore_host(struct mmc_host *host)
 {
 	int ret;
 
-#ifdef CONFIG_MMC_DEBUG
-	pr_info("%s: %s: powering up\n", mmc_hostname(host), __func__);
-#endif
+	pr_debug("%s: %s: powering up\n", mmc_hostname(host), __func__);
 
 	mmc_bus_get(host);
 
-- 
1.9.1

[RFC PATCH v2 4/6] mmc: Kconfig: downgrade CONFIG_MMC_DEBUG for host drivers only

[Shawn Lin]

We have removed all code depending on CONFIG_MMC_DEBUG
from mmc core now. So it's safe to make CONFIG_MMC_DEBUG
just for host drivers only and we expect to kill this option
in the future.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/Kconfig      | 7 -------
 drivers/mmc/Makefile     | 2 --
 drivers/mmc/host/Kconfig | 9 +++++++++
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 7e803fc4..ec21388 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -12,13 +12,6 @@ menuconfig MMC
 	  If you want MMC/SD/SDIO support, you should say Y here and
 	  also to your specific host controller driver.
 
-config MMC_DEBUG
-	bool "MMC debugging"
-	depends on MMC != n
-	help
-	  This is an option for use by developers; most people should
-	  say N here.  This enables MMC core and driver debugging.
-
 if MMC
 
 source "drivers/mmc/core/Kconfig"
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 416b6d1..26ab7af 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -2,7 +2,5 @@
 # Makefile for the kernel mmc device drivers.
 #
 
-subdir-ccflags-$(CONFIG_MMC_DEBUG) := -DDEBUG
-
 obj-$(CONFIG_MMC)		+= core/
 obj-$(subst m,y,$(CONFIG_MMC))	+= host/
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2242633..66b1749 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -4,6 +4,15 @@
 
 comment "MMC/SD/SDIO Host Controller Drivers"
 
+config MMC_DEBUG
+	bool "MMC host drivers debugginG"
+	depends on MMC != n
+	help
+	  This is an option for use by developers; most people should
+	  say N here. This enables MMC host driver debugging. And further
+	  added host drivers please don't invent their private macro for
+	  debugging.
+
 config MMC_ARMMMCI
 	tristate "ARM AMBA Multimedia Card Interface support"
 	depends on ARM_AMBA
-- 
1.9.1

[RFC PATCH v2 5/6] mmc: wbsd: remove CONFIG_MMC_DEBUG from the driver

[Shawn Lin]

wbsd only use this to print some unsupported command.
However the pr_warn should be enough for dynamic log
control and CONFIG_MMC_DEBUG seems bogus here. Remove it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/wbsd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mmc/host/wbsd.c b/drivers/mmc/host/wbsd.c
index 9668616..546aaf8 100644
--- a/drivers/mmc/host/wbsd.c
+++ b/drivers/mmc/host/wbsd.c
@@ -802,10 +802,8 @@ static void wbsd_request(struct mmc_host *mmc, struct mmc_request *mrq)
 			break;
 
 		default:
-#ifdef CONFIG_MMC_DEBUG
 			pr_warn("%s: Data command %d is not supported by this controller\n",
 				mmc_hostname(host->mmc), cmd->opcode);
-#endif
 			cmd->error = -EINVAL;
 
 			goto done;
-- 
1.9.1

[RFC PATCH v2 6/6] mmc: sdhci: remove CONFIG_MMC_DEBUG from the driver

[Shawn Lin]

sdhci uses CONFIG_MMC_DEBUG for showing ADMA descriptor
when occurring ADMA error. And it's also used to  dump the
registers whenever calling sdhci_add_host.

On one hand, I don't see any burden to always print the state
ADMA descriptor as it's rare and will help folks better understand
what was happening when seeing ADMA error.

On the other, folks may be interested in checking some registers
at probe time. So we remove the sdhci_dumpregs from __sdhci_add_host
and print some really useful registers in sdhci_setup_host.

Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- rework the changes for sdhci suggested by Adrian

 drivers/mmc/host/sdhci.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d43..cf2166e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2502,7 +2502,6 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		sdhci_finish_command(host);
 }
 
-#ifdef CONFIG_MMC_DEBUG
 static void sdhci_adma_show_error(struct sdhci_host *host)
 {
 	void *desc = host->adma_table;
@@ -2530,9 +2529,6 @@ static void sdhci_adma_show_error(struct sdhci_host *host)
 			break;
 	}
 }
-#else
-static void sdhci_adma_show_error(struct sdhci_host *host) { }
-#endif
 
 static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 {
@@ -3230,6 +3226,13 @@ int sdhci_setup_host(struct sdhci_host *host)
 	if (ret == -EPROBE_DEFER)
 		return ret;
 
+	DBG("Version:   0x%08x | Present:  0x%08x\n",
+	    sdhci_readw(host, SDHCI_HOST_VERSION),
+	    sdhci_readl(host, SDHCI_PRESENT_STATE));
+	DBG("Caps:      0x%08x | Caps_1:   0x%08x\n",
+	    sdhci_readl(host, SDHCI_CAPABILITIES),
+	    sdhci_readl(host, SDHCI_CAPABILITIES_1));
+
 	sdhci_read_caps(host);
 
 	override_timeout_clk = host->timeout_clk;
@@ -3747,10 +3750,6 @@ int __sdhci_add_host(struct sdhci_host *host)
 		goto untasklet;
 	}
 
-#ifdef CONFIG_MMC_DEBUG
-	sdhci_dumpregs(host);
-#endif
-
 	ret = sdhci_led_register(host);
 	if (ret) {
 		pr_err("%s: Failed to register LED device: %d\n",
-- 
1.9.1

Re: [RFC PATCH v2 6/6] mmc: sdhci: remove CONFIG_MMC_DEBUG from the driver

[Adrian Hunter]

On 19/07/17 10:55, Shawn Lin wrote:
> sdhci uses CONFIG_MMC_DEBUG for showing ADMA descriptor
> when occurring ADMA error. And it's also used to  dump the
> registers whenever calling sdhci_add_host.
> 
> On one hand, I don't see any burden to always print the state
> ADMA descriptor as it's rare and will help folks better understand
> what was happening when seeing ADMA error.
> 
> On the other, folks may be interested in checking some registers
> at probe time. So we remove the sdhci_dumpregs from __sdhci_add_host
> and print some really useful registers in sdhci_setup_host.
> 
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> 
> ---
> 
> Changes in v2:
> - rework the changes for sdhci suggested by Adrian
> 
>  drivers/mmc/host/sdhci.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d43..cf2166e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2502,7 +2502,6 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
>  		sdhci_finish_command(host);
>  }
>  
> -#ifdef CONFIG_MMC_DEBUG
>  static void sdhci_adma_show_error(struct sdhci_host *host)
>  {
>  	void *desc = host->adma_table;
> @@ -2530,9 +2529,6 @@ static void sdhci_adma_show_error(struct sdhci_host *host)
>  			break;
>  	}
>  }
> -#else
> -static void sdhci_adma_show_error(struct sdhci_host *host) { }
> -#endif
>  
>  static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  {
> @@ -3230,6 +3226,13 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	if (ret == -EPROBE_DEFER)
>  		return ret;
>  
> +	DBG("Version:   0x%08x | Present:  0x%08x\n",
> +	    sdhci_readw(host, SDHCI_HOST_VERSION),
> +	    sdhci_readl(host, SDHCI_PRESENT_STATE));
> +	DBG("Caps:      0x%08x | Caps_1:   0x%08x\n",
> +	    sdhci_readl(host, SDHCI_CAPABILITIES),
> +	    sdhci_readl(host, SDHCI_CAPABILITIES_1));
> +
>  	sdhci_read_caps(host);
>  
>  	override_timeout_clk = host->timeout_clk;
> @@ -3747,10 +3750,6 @@ int __sdhci_add_host(struct sdhci_host *host)
>  		goto untasklet;
>  	}
>  
> -#ifdef CONFIG_MMC_DEBUG
> -	sdhci_dumpregs(host);
> -#endif
> -
>  	ret = sdhci_led_register(host);
>  	if (ret) {
>  		pr_err("%s: Failed to register LED device: %d\n",
> 

Re: [RFC PATCH v2 0/6] Try to get rid of CONFIG_MMC_DEBUG

[Ulf Hansson]

On 19 July 2017 at 09:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> CONFIG_MMC_DEBUG was introduced long time ago and mostly
> it's a all-or-none proposition for compile-time. By looking
> at how the mmc core use it, it's obviously pointless to still
> carry on this option.
>
> But referring to the host drivers, it's hard to say should we
> remove it now. The following drivers[1] will print some extra log
> on irq context for debugging. Probably folks still need to debug
> it sometimes but making it default behaviour seems to introduce some
> unnecessary burden for their drivers I guess. Anyway, we downgrade this
> option for hosts only and ask the following added host drivers to use
> this for debugging instead of adding their private macro to activate
> their debug functions.
>
> This patchset also cleanup wbsd and improve sdhci a bit.
>
> [1]:
> drivers/mmc/host/s3cmci.c
> drivers/mmc/host/omap.c
> drivers/mmc/host/omap_hsmmc.c
> drivers/mmc/host/tmio_mmc_core.c
>
>
> Changes in v2:
> - rework the changes for sdhci suggested by Adrian
>
> Shawn Lin (6):
>   mmc: core: remove check of host->removed for rescan routine
>   mmc: core: always check the length of sglist with total data size
>   mmc: core: turn the pr_info under CONFIG_MMC_DEBUG into pr_debug
>   mmc: Kconfig: downgrade CONFIG_MMC_DEBUG for host drivers only
>   mmc: wbsd: remove CONFIG_MMC_DEBUG from the driver
>   mmc: sdhci: remove CONFIG_MMC_DEBUG from the driver
>
>  drivers/mmc/Kconfig      |  7 -------
>  drivers/mmc/Makefile     |  2 --
>  drivers/mmc/core/core.c  | 35 +++++++----------------------------
>  drivers/mmc/host/Kconfig |  9 +++++++++
>  drivers/mmc/host/sdhci.c | 15 +++++++--------
>  drivers/mmc/host/wbsd.c  |  2 --
>  include/linux/mmc/host.h |  3 ---
>  7 files changed, 23 insertions(+), 50 deletions(-)
>
> --

Thanks, applied for next!

Kind regards
Uffe