Re: [PATCH V1 2/2] mmc: sdhci-uhs2: Adjust some error messages and register dump for SD UHS-II card

[Adrian Hunter <adrian.hunter@intel.com>]

On 19/05/2025 15:24, Ulf Hansson wrote:
> On Fri, 16 May 2025 at 11:27, Victor Shih <victorshihgli@gmail.com> wrote:
>>
>> From: Victor Shih <victor.shih@genesyslogic.com.tw>
>>
>> Adjust some error messages to debug mode and register dump to dynamic
>> debug mode to avoid causing misunderstanding it is an error.
> 
> Dumping the register may be useful for the debug level, I am not sure.
> Maybe Adrian has an opinion?

My understanding was that the original issue was that these messages
appear when it is not a UHS-II card, so the register dump should also
become debug-only.

> 
>>
>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
>> ---
>>  drivers/mmc/host/sdhci-uhs2.c | 18 +++++++++---------
>>  drivers/mmc/host/sdhci.h      | 16 ++++++++++++++++
>>  2 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-uhs2.c b/drivers/mmc/host/sdhci-uhs2.c
>> index c53b64d50c0d..9ff867aee985 100644
>> --- a/drivers/mmc/host/sdhci-uhs2.c
>> +++ b/drivers/mmc/host/sdhci-uhs2.c
>> @@ -99,8 +99,8 @@ void sdhci_uhs2_reset(struct sdhci_host *host, u16 mask)
>>         /* hw clears the bit when it's done */
>>         if (read_poll_timeout_atomic(sdhci_readw, val, !(val & mask), 10,
>>                                      UHS2_RESET_TIMEOUT_100MS, true, host, SDHCI_UHS2_SW_RESET)) {
>> -               pr_warn("%s: %s: Reset 0x%x never completed. %s: clean reset bit.\n", __func__,
>> -                       mmc_hostname(host->mmc), (int)mask, mmc_hostname(host->mmc));
>> +               DBG("%s: %s: Reset 0x%x never completed. %s: clean reset bit.\n", __func__,
>> +                   mmc_hostname(host->mmc), (int)mask, mmc_hostname(host->mmc));
> 
> As I said on patch1, please use pr_debug() and drop the macro.
> 
>>                 sdhci_writeb(host, 0, SDHCI_UHS2_SW_RESET);
>>                 return;
>>         }
>> @@ -335,8 +335,8 @@ static int sdhci_uhs2_interface_detect(struct sdhci_host *host)
>>         if (read_poll_timeout(sdhci_readl, val, (val & SDHCI_UHS2_IF_DETECT),
>>                               100, UHS2_INTERFACE_DETECT_TIMEOUT_100MS, true,
>>                               host, SDHCI_PRESENT_STATE)) {
>> -               pr_warn("%s: not detect UHS2 interface in 100ms.\n", mmc_hostname(host->mmc));
>> -               sdhci_dumpregs(host);
>> +               DBG("%s: not detect UHS2 interface in 100ms.\n", mmc_hostname(host->mmc));
>> +               sdhci_dbg_dumpregs(host, "UHS2 interface detect timeout in 100ms");
> 
> If we really need this, I think we should first introduce the helper
> function in a separate patch, that precedes $subject patch in the
> series.
> 
>>                 return -EIO;
>>         }
>>
>> @@ -345,8 +345,8 @@ static int sdhci_uhs2_interface_detect(struct sdhci_host *host)
>>
>>         if (read_poll_timeout(sdhci_readl, val, (val & SDHCI_UHS2_LANE_SYNC),
>>                               100, UHS2_LANE_SYNC_TIMEOUT_150MS, true, host, SDHCI_PRESENT_STATE)) {
>> -               pr_warn("%s: UHS2 Lane sync fail in 150ms.\n", mmc_hostname(host->mmc));
>> -               sdhci_dumpregs(host);
>> +               DBG("%s: UHS2 Lane sync fail in 150ms.\n", mmc_hostname(host->mmc));
>> +               sdhci_dbg_dumpregs(host, "UHS2 Lane sync fail in 150ms");
>>                 return -EIO;
>>         }
>>
>> @@ -417,12 +417,12 @@ static int sdhci_uhs2_do_detect_init(struct mmc_host *mmc)
>>                 host->ops->uhs2_pre_detect_init(host);
>>
>>         if (sdhci_uhs2_interface_detect(host)) {
>> -               pr_warn("%s: cannot detect UHS2 interface.\n", mmc_hostname(host->mmc));
>> +               DBG("%s: cannot detect UHS2 interface.\n", mmc_hostname(host->mmc));
>>                 return -EIO;
>>         }
>>
>>         if (sdhci_uhs2_init(host)) {
>> -               pr_warn("%s: UHS2 init fail.\n", mmc_hostname(host->mmc));
>> +               DBG("%s: UHS2 init fail.\n", mmc_hostname(host->mmc));
>>                 return -EIO;
>>         }
>>
>> @@ -504,7 +504,7 @@ static int sdhci_uhs2_check_dormant(struct sdhci_host *host)
>>         if (read_poll_timeout(sdhci_readl, val, (val & SDHCI_UHS2_IN_DORMANT_STATE),
>>                               100, UHS2_CHECK_DORMANT_TIMEOUT_100MS, true, host,
>>                               SDHCI_PRESENT_STATE)) {
>> -               pr_warn("%s: UHS2 IN_DORMANT fail in 100ms.\n", mmc_hostname(host->mmc));
>> +               DBG("%s: UHS2 IN_DORMANT fail in 100ms.\n", mmc_hostname(host->mmc));
>>                 sdhci_dumpregs(host);

If the message is to be a debug message, then the register dump
should be also i.e. use sdhci_dbg_dumpregs

>>                 return -EIO;
>>         }
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index cd0e35a80542..2c28240e6003 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -898,4 +898,20 @@ void sdhci_switch_external_dma(struct sdhci_host *host, bool en);
>>  void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable);
>>  void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd);
>>
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
>> +#define SDHCI_DBG_ANYWAY 0
>> +#elif defined(DEBUG)
>> +#define SDHCI_DBG_ANYWAY 1
>> +#else
>> +#define SDHCI_DBG_ANYWAY 0
>> +#endif
>> +
>> +#define sdhci_dbg_dumpregs(host, fmt)                                  \
>> +do {                                                                   \
>> +       DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);                 \
>> +       if (DYNAMIC_DEBUG_BRANCH(descriptor) || SDHCI_DBG_ANYWAY)       \
>> +               sdhci_dumpregs(host);                                   \
>> +} while (0)
>> +
>>  #endif /* __SDHCI_HW_H */
>> --
>> 2.43.0
>>
> 
> Kind regards
> Uffe