[PATCH 05/14] mmc: Use logging instead of printf()

Simon Glass sjg at chromium.org
Sun Jul 21 12:08:35 CEST 2024


Hi Sean,

On Sat, 20 Jul 2024 at 16:44, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 7/20/24 02:16, 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 at chromium.org>
> > ---
> >
> >   drivers/mmc/sdhci.c | 46 +++++++++++++++++++--------------------------
> >   1 file changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> > index 560b7e889c7..24998233543 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", (int)mask);
>
> I think this cast can be removed too
>
> >                       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,7 +230,7 @@ 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);
>
> Maybe this should be "mmc%d busy\n"
>
> >                       if (2 * cmd_timeout <= SDHCI_CMD_MAX_TIMEOUT) {
> >                               cmd_timeout += cmd_timeout;
> >                               printf("timeout increasing to: %u ms.\n",
> > @@ -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("tuning\n");
>
> I think this one should be left as-is. Or at least given something more descriptive.

I'll add sdhci in there.

>
> >
> >       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 to wait cmd & data inhibit\n");
>
> Might as well fix the grammar while we're at it
>
> 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");
>
> To distinguish these, can we say "Error while disabling dll"
>
> >                       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 configuring dll\n");
>
> and "Error while enabling dll"
>
> I know this grows .data a bit...

I'll drop the 'while' since error messages mostly assume that.

>
> >                       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: 0x%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;
> >       }
> >
>

Regards,
Simon


More information about the U-Boot mailing list