lkml.org 
[lkml]   [2022]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH V4 2/7] mmc: sdhci: Capture eMMC and SD card errors
    From
    On 2.3.2022 15.03, Shaik Sajida Bhanu wrote:
    > Add changes to capture eMMC and SD card errors.
    > This is useful for debug and testing.
    >
    > Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
    > Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
    > Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
    > Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
    > ---
    > drivers/mmc/host/sdhci.c | 52 +++++++++++++++++++++++++++++++++++++-----------
    > 1 file changed, 40 insertions(+), 12 deletions(-)
    >
    > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
    > index 07c6da1..741fb06 100644
    > --- a/drivers/mmc/host/sdhci.c
    > +++ b/drivers/mmc/host/sdhci.c
    > @@ -3159,6 +3159,7 @@ static void sdhci_timeout_timer(struct timer_list *t)
    > spin_lock_irqsave(&host->lock, flags);
    >
    > if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_REQ_TIMEOUT);

    Let's make a macro for this:

    #define sdhci_err_stats_inc(host, err_name) \
    mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_ ## err_name)

    > pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
    > mmc_hostname(host->mmc));

    Please move the err_stats_inc here after the error message i.e.

    sdhci_err_stats_inc(host, REQ_TIMEOUT);

    > sdhci_dumpregs(host);
    > @@ -3181,6 +3182,7 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
    >
    > if (host->data || host->data_cmd ||
    > (host->cmd && sdhci_data_line_cmd(host->cmd))) {
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_REQ_TIMEOUT);
    > pr_err("%s: Timeout waiting for hardware interrupt.\n",
    > mmc_hostname(host->mmc));

    Please move the err_stats_inc here after the error message i.e.

    sdhci_err_stats_inc(host, REQ_TIMEOUT);

    > sdhci_dumpregs(host);
    > @@ -3240,11 +3242,15 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
    >
    > if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
    > SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
    > - if (intmask & SDHCI_INT_TIMEOUT)
    > + if (intmask & SDHCI_INT_TIMEOUT) {
    > host->cmd->error = -ETIMEDOUT;
    > - else
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_CMD_TIMEOUT);
    > + } else {
    > host->cmd->error = -EILSEQ;
    > -
    > + if (host->cmd->opcode != MMC_SEND_TUNING_BLOCK ||
    > + host->cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200)

    That should be && not ||

    Please add a helper as below, then this can be:

    if (!mmc_op_tuning(host->cmd->opcode))
    sdhci_err_stats_inc(host, CMD_CRC);


    diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
    index d9a65c6a8816..d809bdfcc59b 100644
    --- a/include/linux/mmc/mmc.h
    +++ b/include/linux/mmc/mmc.h
    @@ -99,6 +99,12 @@ static inline bool mmc_op_multi(u32 opcode)
    opcode == MMC_READ_MULTIPLE_BLOCK;
    }

    +static inline bool mmc_op_tuning(u32 opcode)
    +{
    + return opcode == MMC_SEND_TUNING_BLOCK ||
    + opcode == MMC_SEND_TUNING_BLOCK_HS200;
    +}
    +
    /*
    * MMC_SWITCH argument format:
    *


    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_CMD_CRC);
    > + }
    > /* Treat data command CRC error the same as data CRC error */
    > if (host->cmd->data &&
    > (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
    > @@ -3265,6 +3271,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
    > int err = (auto_cmd_status & SDHCI_AUTO_CMD_TIMEOUT) ?
    > -ETIMEDOUT :
    > -EILSEQ;

    Should be a blank line here after the local variable declarations.

    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_AUTO_CMD);
    >
    > if (sdhci_auto_cmd23(host, mrq)) {
    > mrq->sbc->error = err;
    > @@ -3342,6 +3349,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
    > if (intmask & SDHCI_INT_DATA_TIMEOUT) {
    > host->data_cmd = NULL;
    > data_cmd->error = -ETIMEDOUT;
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_CMD_TIMEOUT);
    > __sdhci_finish_mrq(host, data_cmd->mrq);
    > return;
    > }
    > @@ -3375,18 +3383,25 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
    > return;
    > }
    >
    > - if (intmask & SDHCI_INT_DATA_TIMEOUT)
    > + if (intmask & SDHCI_INT_DATA_TIMEOUT) {
    > host->data->error = -ETIMEDOUT;
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_DAT_TIMEOUT);
    > + }

    The parenthesis should be with the 'else'. Please use checkpatch with the --strict option
    and fix the issues.

    > else if (intmask & SDHCI_INT_DATA_END_BIT)
    > host->data->error = -EILSEQ;
    > else if ((intmask & SDHCI_INT_DATA_CRC) &&
    > SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
    > - != MMC_BUS_TEST_R)
    > + != MMC_BUS_TEST_R) {
    > host->data->error = -EILSEQ;
    > + if (host->cmd->opcode != MMC_SEND_TUNING_BLOCK ||
    > + host->cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200)
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_DAT_CRC);

    As previously:

    if (!mmc_op_tuning(host->cmd->opcode))
    sdhci_err_stats_inc(host, DAT_CRC);
    > + }
    > else if (intmask & SDHCI_INT_ADMA_ERROR) {
    > pr_err("%s: ADMA error: 0x%08x\n", mmc_hostname(host->mmc),
    > intmask);
    > sdhci_adma_show_error(host);
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_ADMA);
    > host->data->error = -EIO;
    > if (host->ops->adma_workaround)
    > host->ops->adma_workaround(host, intmask);
    > @@ -3905,20 +3920,33 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
    > if (!host->cqe_on)
    > return false;
    >
    > - if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC))
    > + if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
    > *cmd_error = -EILSEQ;
    > - else if (intmask & SDHCI_INT_TIMEOUT)
    > + if (intmask & SDHCI_INT_CRC) {
    > + if (host->cmd->opcode != MMC_SEND_TUNING_BLOCK ||
    > + host->cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200)

    Please count INDEX and END_BIT errors same as CRC errors i.e. just

    if (!mmc_op_tuning(host->cmd->opcode))
    sdhci_err_stats_inc(host, CMD_CRC);
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_CMD_CRC);
    > + }
    > + } else if (intmask & SDHCI_INT_TIMEOUT) {
    > *cmd_error = -ETIMEDOUT;
    > - else
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_CMD_TIMEOUT);
    > + } else
    > *cmd_error = 0;
    >
    > - if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
    > + if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
    > *data_error = -EILSEQ;
    > - else if (intmask & SDHCI_INT_DATA_TIMEOUT)
    > + if (intmask & SDHCI_INT_DATA_CRC) {
    > + if (host->cmd->opcode != MMC_SEND_TUNING_BLOCK ||
    > + host->cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200)

    Please count END_BIT error same as CRC error i.e. just

    if (!mmc_op_tuning(host->cmd->opcode))
    sdhci_err_stats_inc(host, DAT_CRC);
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_DAT_CRC);
    > + }
    > + } else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
    > *data_error = -ETIMEDOUT;
    > - else if (intmask & SDHCI_INT_ADMA_ERROR)
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_DAT_TIMEOUT);
    > + } else if (intmask & SDHCI_INT_ADMA_ERROR) {
    > *data_error = -EIO;
    > - else
    > + mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_ADMA);
    > + } else
    > *data_error = 0;
    >
    > /* Clear selected interrupts. */

    \
     
     \ /
      Last update: 2022-03-08 10:38    [W:4.193 / U:0.536 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site