diff mbox series

[v3,06/15] mmc: Use logging instead of printf()

Message ID 20240811122924.4035982-7-sjg@chromium.org
State Superseded
Delegated to: Tom Rini
Headers show
Series spl: mmc: Some tweaks for SPL, particularly with MMC | expand

Commit Message

Simon Glass Aug. 11, 2024, 12:29 p.m. UTC
The code makes quite a few uses of __func__ which puts the function
name into the resulting SPL image. Use the log subsystem instead, to
reduce size.

The CONFIG_LOGF_FUNC option can be used to enable the function name.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v3:
- Keep hex prefix on values which currently have it
- Fix up the second parts of the 'mmc%d busy' log_warning()
- Restore the original error message for configuring DLL

Changes in v2:
- Various updates to log messages
- Drop an unnecessary cast

 drivers/mmc/sdhci.c | 52 +++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

Comments

Quentin Schulz Aug. 12, 2024, 9:35 a.m. UTC | #1
Hi Simon,

On 8/11/24 2:29 PM, Simon Glass wrote:
> The code makes quite a few uses of __func__ which puts the function
> name into the resulting SPL image. Use the log subsystem instead, to
> reduce size.
> 
> The CONFIG_LOGF_FUNC option can be used to enable the function name.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v3:
> - Keep hex prefix on values which currently have it
> - Fix up the second parts of the 'mmc%d busy' log_warning()
> - Restore the original error message for configuring DLL
> 
> Changes in v2:
> - Various updates to log messages
> - Drop an unnecessary cast
> 
>   drivers/mmc/sdhci.c | 52 +++++++++++++++++++--------------------------
>   1 file changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 560b7e889c7..4833b5158c7 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -32,8 +32,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
>   	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>   	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>   		if (timeout == 0) {
> -			printf("%s: Reset 0x%x never completed.\n",
> -			       __func__, (int)mask);
> +			log_warning("Reset %#x never completed\n", mask);

TIL about %#x thanks :)

>   			return;
>   		}
>   		timeout--;
> @@ -139,8 +138,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data)
>   	do {
>   		stat = sdhci_readl(host, SDHCI_INT_STATUS);
>   		if (stat & SDHCI_INT_ERROR) {
> -			pr_debug("%s: Error detected in status(0x%X)!\n",
> -				 __func__, stat);
> +			log_debug("Error detected in status(%#x)!\n", stat);

It used to be uppercase and is now lowercase, though %#X would probably 
do the job too (albeit it would apparently write 0X prefix instead of 0x).

Otherwise looks good to me,

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin
Simon Glass Aug. 13, 2024, 12:17 p.m. UTC | #2
Hi Quentin,

On Mon, 12 Aug 2024 at 03:35, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 8/11/24 2:29 PM, Simon Glass wrote:
> > The code makes quite a few uses of __func__ which puts the function
> > name into the resulting SPL image. Use the log subsystem instead, to
> > reduce size.
> >
> > The CONFIG_LOGF_FUNC option can be used to enable the function name.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Keep hex prefix on values which currently have it
> > - Fix up the second parts of the 'mmc%d busy' log_warning()
> > - Restore the original error message for configuring DLL
> >
> > Changes in v2:
> > - Various updates to log messages
> > - Drop an unnecessary cast
> >
> >   drivers/mmc/sdhci.c | 52 +++++++++++++++++++--------------------------
> >   1 file changed, 22 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> > index 560b7e889c7..4833b5158c7 100644
> > --- a/drivers/mmc/sdhci.c
> > +++ b/drivers/mmc/sdhci.c
> > @@ -32,8 +32,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
> >       sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
> >       while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
> >               if (timeout == 0) {
> > -                     printf("%s: Reset 0x%x never completed.\n",
> > -                            __func__, (int)mask);
> > +                     log_warning("Reset %#x never completed\n", mask);
>
> TIL about %#x thanks :)
>
> >                       return;
> >               }
> >               timeout--;
> > @@ -139,8 +138,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data)
> >       do {
> >               stat = sdhci_readl(host, SDHCI_INT_STATUS);
> >               if (stat & SDHCI_INT_ERROR) {
> > -                     pr_debug("%s: Error detected in status(0x%X)!\n",
> > -                              __func__, stat);
> > +                     log_debug("Error detected in status(%#x)!\n", stat);
>
> It used to be uppercase and is now lowercase, though %#X would probably
> do the job too (albeit it would apparently write 0X prefix instead of 0x).

Right, but we should use lower-case hex in U-Boot.

>
> Otherwise looks good to me,
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> Thanks!
> Quentin

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 560b7e889c7..4833b5158c7 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -32,8 +32,7 @@  static void sdhci_reset(struct sdhci_host *host, u8 mask)
 	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
 		if (timeout == 0) {
-			printf("%s: Reset 0x%x never completed.\n",
-			       __func__, (int)mask);
+			log_warning("Reset %#x never completed\n", mask);
 			return;
 		}
 		timeout--;
@@ -139,8 +138,7 @@  static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data)
 	do {
 		stat = sdhci_readl(host, SDHCI_INT_STATUS);
 		if (stat & SDHCI_INT_ERROR) {
-			pr_debug("%s: Error detected in status(0x%X)!\n",
-				 __func__, stat);
+			log_debug("Error detected in status(%#x)!\n", stat);
 			return -EIO;
 		}
 		if (!transfer_done && (stat & rdy)) {
@@ -173,7 +171,7 @@  static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data)
 		if (timeout-- > 0)
 			udelay(10);
 		else {
-			printf("%s: Transfer data timeout\n", __func__);
+			log_err("Transfer data timeout\n");
 			return -ETIMEDOUT;
 		}
 	} while (!(stat & SDHCI_INT_DATA_END));
@@ -232,13 +230,13 @@  static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
 		if (time >= cmd_timeout) {
-			printf("%s: MMC: %d busy ", __func__, mmc_dev);
+			log_warning("mmc%d busy ", mmc_dev);
 			if (2 * cmd_timeout <= SDHCI_CMD_MAX_TIMEOUT) {
 				cmd_timeout += cmd_timeout;
-				printf("timeout increasing to: %u ms.\n",
-				       cmd_timeout);
+				log_warning("timeout increasing to: %u ms\n",
+					    cmd_timeout);
 			} else {
-				puts("timeout.\n");
+				log_warning("timeout\n");
 				return -ECOMM;
 			}
 		}
@@ -316,8 +314,8 @@  static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		}
 
 		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
-			printf("%s: Timeout for status update: %08x %08x\n",
-			       __func__, stat, mask);
+			log_warning("Timeout for status update: %08x %08x\n",
+				    stat, mask);
 			return -ETIMEDOUT;
 		}
 	} while ((stat & mask) != mask);
@@ -358,7 +356,7 @@  static int sdhci_execute_tuning(struct udevice *dev, uint opcode)
 	struct mmc *mmc = mmc_get_mmc_dev(dev);
 	struct sdhci_host *host = mmc->priv;
 
-	debug("%s\n", __func__);
+	log_debug("sdhci tuning\n");
 
 	if (host->ops && host->ops->platform_execute_tuning) {
 		err = host->ops->platform_execute_tuning(mmc, opcode);
@@ -380,8 +378,7 @@  int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) &
 			   (SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT)) {
 		if (timeout == 0) {
-			printf("%s: Timeout to wait cmd & data inhibit\n",
-			       __func__);
+			log_err("Timeout waiting for cmd & data inhibit\n");
 			return -EBUSY;
 		}
 
@@ -397,7 +394,7 @@  int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	if (host->ops && host->ops->set_delay) {
 		ret = host->ops->set_delay(host);
 		if (ret) {
-			printf("%s: Error while setting tap delay\n", __func__);
+			log_err("Error while setting tap delay\n");
 			return ret;
 		}
 	}
@@ -405,7 +402,7 @@  int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	if (host->ops && host->ops->config_dll) {
 		ret = host->ops->config_dll(host, clock, false);
 		if (ret) {
-			printf("%s: Error while configuring dll\n", __func__);
+			log_err("Error configuring dll\n");
 			return ret;
 		}
 	}
@@ -456,7 +453,7 @@  int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	if (host->ops && host->ops->config_dll) {
 		ret = host->ops->config_dll(host, clock, true);
 		if (ret) {
-			printf("%s: Error while configuring dll\n", __func__);
+			log_err("Error while configuring dll\n");
 			return ret;
 		}
 	}
@@ -472,8 +469,7 @@  int sdhci_set_clock(struct mmc *mmc, unsigned int clock)
 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 		& SDHCI_CLOCK_INT_STABLE)) {
 		if (timeout == 0) {
-			printf("%s: Internal clock never stabilised.\n",
-			       __func__);
+			log_err("Internal clock never stabilised.\n");
 			return -EBUSY;
 		}
 		timeout--;
@@ -738,8 +734,7 @@  static int sdhci_init(struct mmc *mmc)
 	if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR) {
 		host->align_buffer = memalign(8, 512 * 1024);
 		if (!host->align_buffer) {
-			printf("%s: Aligned buffer alloc failed!!!\n",
-			       __func__);
+			log_err("Aligned buffer alloc failed\n");
 			return -ENOMEM;
 		}
 	}
@@ -881,20 +876,18 @@  int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host,
 #else
 	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 #endif
-	debug("%s, caps: 0x%x\n", __func__, caps);
+	log_debug("caps: %#x\n", caps);
 
 #if CONFIG_IS_ENABLED(MMC_SDHCI_SDMA)
 	if ((caps & SDHCI_CAN_DO_SDMA)) {
 		host->flags |= USE_SDMA;
 	} else {
-		debug("%s: Your controller doesn't support SDMA!!\n",
-		      __func__);
+		log_debug("Controller doesn't support SDMA\n");
 	}
 #endif
 #if CONFIG_IS_ENABLED(MMC_SDHCI_ADMA)
 	if (!(caps & SDHCI_CAN_DO_ADMA2)) {
-		printf("%s: Your controller doesn't support ADMA!!\n",
-		       __func__);
+		log_err("Controller doesn't support ADMA\n");
 		return -EINVAL;
 	}
 	if (!host->adma_desc_table) {
@@ -927,7 +920,7 @@  int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host,
 #else
 		caps_1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
 #endif
-		debug("%s, caps_1: 0x%x\n", __func__, caps_1);
+		log_debug("caps_1: %#x\n", caps_1);
 		host->clk_mul = (caps_1 & SDHCI_CLOCK_MUL_MASK) >>
 				SDHCI_CLOCK_MUL_SHIFT;
 
@@ -953,8 +946,7 @@  int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host,
 			host->max_clk *= host->clk_mul;
 	}
 	if (host->max_clk == 0) {
-		printf("%s: Hardware doesn't specify base clock frequency\n",
-		       __func__);
+		log_err("Hardware doesn't specify base clock frequency\n");
 		return -EINVAL;
 	}
 	if (f_max && (f_max < host->max_clk))
@@ -1047,7 +1039,7 @@  int add_sdhci(struct sdhci_host *host, u32 f_max, u32 f_min)
 
 	host->mmc = mmc_create(&host->cfg, host);
 	if (host->mmc == NULL) {
-		printf("%s: mmc create fail!\n", __func__);
+		log_err("mmc create fail\n");
 		return -ENOMEM;
 	}