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

[RFC PATCH 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 sdhci as it doesn't seem sensible to
use CONFIG_MMC_DEBUG for their cases.

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



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 |  8 --------
 drivers/mmc/host/wbsd.c  |  2 --
 include/linux/mmc/host.h |  3 ---
 7 files changed, 16 insertions(+), 50 deletions(-)

-- 
1.9.1

[RFC PATCH 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>
---

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

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

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

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

 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 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, git-blame points out that CONFIG_MMC_DEBUG for
sdhci_add_host was added since it's merged for the first time.
I don't know what exactly the intention was, but I guess folks
don't need it at all? IMHO, it's another all-or-none proposition.
I'd prefer to remove this sdhci_dumpregs from sdhci_add_host totally.

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

---

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d43..82f1761 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)
 {
@@ -3747,10 +3743,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 6/6] mmc: sdhci: remove CONFIG_MMC_DEBUG from the driver

[Adrian Hunter]

On 18/07/17 12:01, 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, git-blame points out that CONFIG_MMC_DEBUG for
> sdhci_add_host was added since it's merged for the first time.
> I don't know what exactly the intention was, but I guess folks
> don't need it at all? IMHO, it's another all-or-none proposition.
> I'd prefer to remove this sdhci_dumpregs from sdhci_add_host totally.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
>  drivers/mmc/host/sdhci.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d43..82f1761 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)
>  {
> @@ -3747,10 +3743,6 @@ int __sdhci_add_host(struct sdhci_host *host)
>  		goto untasklet;
>  	}
>  
> -#ifdef CONFIG_MMC_DEBUG
> -	sdhci_dumpregs(host);
> -#endif

We should still DBG() the interesting registers like SDHCI_HOST_VERSION,
SDHCI_PRESENT_STATE, SDHCI_CAPABILITIES, SDHCI_CAPABILITIES_1.  Better to do
it earlier on though, like in sdhci_setup_host() after checking
mmc_regulator_get_supply().

> -
>  	ret = sdhci_led_register(host);
>  	if (ret) {
>  		pr_err("%s: Failed to register LED device: %d\n",
> 

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

[Shawn Lin]

Hi Adrian,

On 2017/7/18 17:38, Adrian Hunter wrote:
> On 18/07/17 12:01, 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, git-blame points out that CONFIG_MMC_DEBUG for
>> sdhci_add_host was added since it's merged for the first time.
>> I don't know what exactly the intention was, but I guess folks
>> don't need it at all? IMHO, it's another all-or-none proposition.
>> I'd prefer to remove this sdhci_dumpregs from sdhci_add_host totally.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>>   drivers/mmc/host/sdhci.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ecd0d43..82f1761 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)
>>   {
>> @@ -3747,10 +3743,6 @@ int __sdhci_add_host(struct sdhci_host *host)
>>   		goto untasklet;
>>   	}
>>   
>> -#ifdef CONFIG_MMC_DEBUG
>> -	sdhci_dumpregs(host);
>> -#endif
> 
> We should still DBG() the interesting registers like SDHCI_HOST_VERSION,
> SDHCI_PRESENT_STATE, SDHCI_CAPABILITIES, SDHCI_CAPABILITIES_1.  Better to do
> it earlier on though, like in sdhci_setup_host() after checking
> mmc_regulator_get_supply().
> 

Okay. So would you like to kill the CONFIG_MMC_DEBUG around
this dempregs and keep the output log always there OR you still
want to keep it under CONFIG_MMC_DEBUG?


Thanks.

>> -
>>   	ret = sdhci_led_register(host);
>>   	if (ret) {
>>   		pr_err("%s: Failed to register LED device: %d\n",
>>
> 
> 
> 
> 

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

[Adrian Hunter]

On 19/07/17 03:32, Shawn Lin wrote:
> Hi Adrian,
> 
> On 2017/7/18 17:38, Adrian Hunter wrote:
>> On 18/07/17 12:01, 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, git-blame points out that CONFIG_MMC_DEBUG for
>>> sdhci_add_host was added since it's merged for the first time.
>>> I don't know what exactly the intention was, but I guess folks
>>> don't need it at all? IMHO, it's another all-or-none proposition.
>>> I'd prefer to remove this sdhci_dumpregs from sdhci_add_host totally.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>>   drivers/mmc/host/sdhci.c | 8 --------
>>>   1 file changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index ecd0d43..82f1761 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)
>>>   {
>>> @@ -3747,10 +3743,6 @@ int __sdhci_add_host(struct sdhci_host *host)
>>>           goto untasklet;
>>>       }
>>>   -#ifdef CONFIG_MMC_DEBUG
>>> -    sdhci_dumpregs(host);
>>> -#endif
>>
>> We should still DBG() the interesting registers like SDHCI_HOST_VERSION,
>> SDHCI_PRESENT_STATE, SDHCI_CAPABILITIES, SDHCI_CAPABILITIES_1.  Better to do
>> it earlier on though, like in sdhci_setup_host() after checking
>> mmc_regulator_get_supply().
>>
> 
> Okay. So would you like to kill the CONFIG_MMC_DEBUG around
> this dempregs and keep the output log always there OR you still
> want to keep it under CONFIG_MMC_DEBUG?

We absolutely want register dumps if there are unexpected errors, so the dump
cannot use dynamic debug.  But we also want to be able to see the interesting
registers at probe time.  We could make use of DYNAMIC_DEBUG_BRANCH and
DEFINE_DYNAMIC_DEBUG_METADATA but that is not something that is commonly done.
So that leaves adding a few extra pr_debugs (DBG macro for sdhci) at probe time
i.e. Remove CONFIG_MMC_DEBUG and add something like:

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d4350e8a..1b619ccc27f5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3230,6 +3230,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;

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

[Shawn Lin]

On 2017/7/19 15:16, Adrian Hunter wrote:
> On 19/07/17 03:32, Shawn Lin wrote:
>> Hi Adrian,
>>
>> On 2017/7/18 17:38, Adrian Hunter wrote:
>>> On 18/07/17 12:01, 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, git-blame points out that CONFIG_MMC_DEBUG for
>>>> sdhci_add_host was added since it's merged for the first time.
>>>> I don't know what exactly the intention was, but I guess folks
>>>> don't need it at all? IMHO, it's another all-or-none proposition.
>>>> I'd prefer to remove this sdhci_dumpregs from sdhci_add_host totally.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>>    drivers/mmc/host/sdhci.c | 8 --------
>>>>    1 file changed, 8 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index ecd0d43..82f1761 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)
>>>>    {
>>>> @@ -3747,10 +3743,6 @@ int __sdhci_add_host(struct sdhci_host *host)
>>>>            goto untasklet;
>>>>        }
>>>>    -#ifdef CONFIG_MMC_DEBUG
>>>> -    sdhci_dumpregs(host);
>>>> -#endif
>>>
>>> We should still DBG() the interesting registers like SDHCI_HOST_VERSION,
>>> SDHCI_PRESENT_STATE, SDHCI_CAPABILITIES, SDHCI_CAPABILITIES_1.  Better to do
>>> it earlier on though, like in sdhci_setup_host() after checking
>>> mmc_regulator_get_supply().
>>>
>>
>> Okay. So would you like to kill the CONFIG_MMC_DEBUG around
>> this dempregs and keep the output log always there OR you still
>> want to keep it under CONFIG_MMC_DEBUG?
> 
> We absolutely want register dumps if there are unexpected errors, so the dump
> cannot use dynamic debug.  But we also want to be able to see the interesting

of course.

> registers at probe time.  We could make use of DYNAMIC_DEBUG_BRANCH and
> DEFINE_DYNAMIC_DEBUG_METADATA but that is not something that is commonly done.
> So that leaves adding a few extra pr_debugs (DBG macro for sdhci) at probe time
> i.e. Remove CONFIG_MMC_DEBUG and add something like:
> 

Looks good. I will respin it and thanks for this suggestion.

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d4350e8a..1b619ccc27f5 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3230,6 +3230,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;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>