[PATCH V1 0/2] Adjust some error messages for SD UHS-II initialization process

[PATCH V1 0/2] Adjust some error messages for SD UHS-II initialization process

[Victor Shih]

From: Victor Shih <victor.shih@genesyslogic.com.tw>

Summary
=======
It is normal that errors will occur when using non-UHS-II card to enter
the UHS-II card initialization process. We should not be producing error
messages and register dumps. Therefore, switch the error messages to debug
mode and register dumps to dynamic debug mode.

Patch structure
===============
patch#1: for core
patch#2: for sdhci

Changes in v1 (May. 16, 2025)
* Rebase on latest mmc/next.
* Patch#1: Adjust some error messages for SD UHS-II cards.
* Patch#2: Adjust some error messages and register dump for SD UHS-II card

Victor Shih (2):
  mmc: core: Adjust some error messages for SD UHS-II cards
  mmc: sdhci-uhs2: Adjust some error messages and register dump for SD
    UHS-II card

 drivers/mmc/core/sd_uhs2.c    |  8 ++++++--
 drivers/mmc/host/sdhci-uhs2.c | 18 +++++++++---------
 drivers/mmc/host/sdhci.h      | 16 ++++++++++++++++
 3 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH V1 1/2] mmc: core: Adjust some error messages for SD UHS-II cards

[Victor Shih]

From: Victor Shih <victor.shih@genesyslogic.com.tw>

Adjust some error messages to debug mode to avoid causing
misunderstanding it is an error.

Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
---
 drivers/mmc/core/sd_uhs2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
index 1c31d0dfa961..58c4cef37f7c 100644
--- a/drivers/mmc/core/sd_uhs2.c
+++ b/drivers/mmc/core/sd_uhs2.c
@@ -36,6 +36,10 @@
 #include "sd_ops.h"
 #include "mmc_ops.h"
 
+#define DRIVER_NAME "sd_uhs2"
+#define DBG(f, x...) \
+	pr_debug(DRIVER_NAME " [%s()]: " f, __func__, ## x)
+
 #define UHS2_WAIT_CFG_COMPLETE_PERIOD_US  (1 * 1000)
 #define UHS2_WAIT_CFG_COMPLETE_TIMEOUT_MS 100
 
@@ -91,8 +95,8 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
 
 	err = host->ops->uhs2_control(host, UHS2_PHY_INIT);
 	if (err) {
-		pr_err("%s: failed to initial phy for UHS-II!\n",
-		       mmc_hostname(host));
+		DBG("%s: failed to initial phy for UHS-II!\n",
+		    mmc_hostname(host));
 	}
 
 	return err;
-- 
2.43.0

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

[Victor Shih]

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.

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));
 		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");
 		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);
 		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

Re: [PATCH V1 0/2] Adjust some error messages for SD UHS-II initialization process

[Ben Chuang]

Hi,

These patches were contributed by Victor alone. Please remove my
'Signed-off-by'.

Best regards,
Ben Chuang

On Fri, May 16, 2025 at 5:27 PM Victor Shih <victorshihgli@gmail.com> wrote:
>
> From: Victor Shih <victor.shih@genesyslogic.com.tw>
>
> Summary
> =======
> It is normal that errors will occur when using non-UHS-II card to enter
> the UHS-II card initialization process. We should not be producing error
> messages and register dumps. Therefore, switch the error messages to debug
> mode and register dumps to dynamic debug mode.
>
> Patch structure
> ===============
> patch#1: for core
> patch#2: for sdhci
>
> Changes in v1 (May. 16, 2025)
> * Rebase on latest mmc/next.
> * Patch#1: Adjust some error messages for SD UHS-II cards.
> * Patch#2: Adjust some error messages and register dump for SD UHS-II card
>
> Victor Shih (2):
>   mmc: core: Adjust some error messages for SD UHS-II cards
>   mmc: sdhci-uhs2: Adjust some error messages and register dump for SD
>     UHS-II card
>
>  drivers/mmc/core/sd_uhs2.c    |  8 ++++++--
>  drivers/mmc/host/sdhci-uhs2.c | 18 +++++++++---------
>  drivers/mmc/host/sdhci.h      | 16 ++++++++++++++++
>  3 files changed, 31 insertions(+), 11 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH V1 1/2] mmc: core: Adjust some error messages for SD UHS-II cards

[Ulf Hansson]

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 to avoid causing
> misunderstanding it is an error.
>
> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
> ---
>  drivers/mmc/core/sd_uhs2.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> index 1c31d0dfa961..58c4cef37f7c 100644
> --- a/drivers/mmc/core/sd_uhs2.c
> +++ b/drivers/mmc/core/sd_uhs2.c
> @@ -36,6 +36,10 @@
>  #include "sd_ops.h"
>  #include "mmc_ops.h"
>
> +#define DRIVER_NAME "sd_uhs2"
> +#define DBG(f, x...) \
> +       pr_debug(DRIVER_NAME " [%s()]: " f, __func__, ## x)
> +

We don't need a macro for this, just use a pr_debug() below instead.

>  #define UHS2_WAIT_CFG_COMPLETE_PERIOD_US  (1 * 1000)
>  #define UHS2_WAIT_CFG_COMPLETE_TIMEOUT_MS 100
>
> @@ -91,8 +95,8 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
>
>         err = host->ops->uhs2_control(host, UHS2_PHY_INIT);
>         if (err) {
> -               pr_err("%s: failed to initial phy for UHS-II!\n",
> -                      mmc_hostname(host));
> +               DBG("%s: failed to initial phy for UHS-II!\n",
> +                   mmc_hostname(host));
>         }
>
>         return err;
> --
> 2.43.0
>

Kind regards
Uffe

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

[Ulf Hansson]

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?

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

Re: [PATCH V1 1/2] mmc: core: Adjust some error messages for SD UHS-II cards

[Victor Shih]

On Mon, May 19, 2025 at 8:10 PM Ulf Hansson <ulf.hansson@linaro.org> 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 to avoid causing
> > misunderstanding it is an error.
> >
> > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
> > ---
> >  drivers/mmc/core/sd_uhs2.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > index 1c31d0dfa961..58c4cef37f7c 100644
> > --- a/drivers/mmc/core/sd_uhs2.c
> > +++ b/drivers/mmc/core/sd_uhs2.c
> > @@ -36,6 +36,10 @@
> >  #include "sd_ops.h"
> >  #include "mmc_ops.h"
> >
> > +#define DRIVER_NAME "sd_uhs2"
> > +#define DBG(f, x...) \
> > +       pr_debug(DRIVER_NAME " [%s()]: " f, __func__, ## x)
> > +
>
> We don't need a macro for this, just use a pr_debug() below instead.
>

Hi, Ulf

I will drop this macro and use pr_debug() instead in the next version.

Thanks, Victor Shih

> >  #define UHS2_WAIT_CFG_COMPLETE_PERIOD_US  (1 * 1000)
> >  #define UHS2_WAIT_CFG_COMPLETE_TIMEOUT_MS 100
> >
> > @@ -91,8 +95,8 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
> >
> >         err = host->ops->uhs2_control(host, UHS2_PHY_INIT);
> >         if (err) {
> > -               pr_err("%s: failed to initial phy for UHS-II!\n",
> > -                      mmc_hostname(host));
> > +               DBG("%s: failed to initial phy for UHS-II!\n",
> > +                   mmc_hostname(host));
> >         }
> >
> >         return err;
> > --
> > 2.43.0
> >
>
> Kind regards
> Uffe

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

[Victor Shih]

On Mon, May 19, 2025 at 8:25 PM Ulf Hansson <ulf.hansson@linaro.org> 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?
>
> >
> > 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.
>

Hi, Ulf

This macro has been defined in previous patches not the first time it
has appeared here,
are we still going to drop this macro?

Thanks, Victor Shih

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

Hi, Ulf

Ok, I will make this helper function into a separate patch in the next version.

Thanks, Victor Shih

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

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

[Adrian Hunter]

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

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

[Ulf Hansson]

On Wed, 21 May 2025 at 13:04, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> 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.

Good point, I agree!

[...]

Kind regards
Uffe

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

[Ulf Hansson]

On Wed, 21 May 2025 at 12:45, Victor Shih <victorshihgli@gmail.com> wrote:
>
> On Mon, May 19, 2025 at 8:25 PM Ulf Hansson <ulf.hansson@linaro.org> 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?
> >
> > >
> > > 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.
> >
>
> Hi, Ulf
>
> This macro has been defined in previous patches not the first time it
> has appeared here,
> are we still going to drop this macro?

Yes, please.

We don't have macros for other log-prints, so let's not use a macro
for debug prints either.

[...]

Kind regards
Uffe