MIME-Version: 1.0
References: <1637130012-21846-1-git-send-email-quic_c_sbhanu@quicinc.com>
In-Reply-To: <1637130012-21846-1-git-send-email-quic_c_sbhanu@quicinc.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 25 Nov 2021 12:30:39 +0100
Message-ID: <CAPDyKFqd9ThDUPAYom32w4D8nZYxbvYnJgA9dmQnFu2qEhhMig@mail.gmail.com>
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
To:     Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
Cc:     adrian.hunter@intel.com, riteshh@codeaurora.org,
        asutoshd@quicinc.com, agross@kernel.org,
        bjorn.andersson@linaro.org, linux-mmc@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        stummala@codeaurora.org, vbadigan@codeaurora.org,
        quic_rampraka@quicinc.com, quic_pragalla@quicinc.com,
        sartgarg@codeaurora.org, nitirawa@codeaurora.org,
        sayalil@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 17 Nov 2021 at 07:20, Shaik Sajida Bhanu
<quic_c_sbhanu@quicinc.com> wrote:
>
> Add sysfs entry to query eMMC and SD card errors statistics.
> This feature is useful for debug and testing.
>
> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
> ---
>  drivers/mmc/host/cqhci.h     |   1 +
>  drivers/mmc/host/sdhci-msm.c | 192 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |  17 ++++
>  drivers/mmc/host/sdhci.h     |   1 +
>  include/linux/mmc/host.h     |   1 +
>  5 files changed, 212 insertions(+)

To allow an easier review, I strongly suggest splitting up the changes
in smaller pieces. Maybe in three steps: add interfaces, implement
them, add sysfs - or something along those lines.

I am also trying to understand the benefit of having these stats
available. Can you perhaps share a little bit of background on how
they are usable for you? Is it for debug purpose or does it have other
purposes too?

If it turns out that we agree on finding these stats valuable for us,
then I am inclined to think that this should be integrated closer with
the mmc core.

The error codes that are propagated to the core from failed requests
are common error codes, so we should be able to use that information
from the core itself, I think.

Kind regards
Uffe

>
> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
> index ba9387e..f72a1d6 100644
> --- a/drivers/mmc/host/cqhci.h
> +++ b/drivers/mmc/host/cqhci.h
> @@ -286,6 +286,7 @@ struct cqhci_host_ops {
>                                  u64 *data);
>         void (*pre_enable)(struct mmc_host *mmc);
>         void (*post_disable)(struct mmc_host *mmc);
> +       void (*err_stats)(struct mmc_host *mmc, unsigned long flags);
>  #ifdef CONFIG_MMC_CRYPTO
>         int (*program_key)(struct cqhci_host *cq_host,
>                            const union cqhci_crypto_cfg_entry *cfg, int slot);
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 50c71e0..e1dcd2d 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -242,6 +242,23 @@ struct sdhci_msm_variant_ops {
>                         u32 offset);
>  };
>
> +enum {
> +       MMC_ERR_CMD_TIMEOUT,
> +       MMC_ERR_CMD_CRC,
> +       MMC_ERR_DAT_TIMEOUT,
> +       MMC_ERR_DAT_CRC,
> +       MMC_ERR_AUTO_CMD,
> +       MMC_ERR_ADMA,
> +       MMC_ERR_TUNING,
> +       MMC_ERR_CMDQ_RED,
> +       MMC_ERR_CMDQ_GCE,
> +       MMC_ERR_CMDQ_ICCE,
> +       MMC_ERR_REQ_TIMEOUT,
> +       MMC_ERR_CMDQ_REQ_TIMEOUT,
> +       MMC_ERR_ICE_CFG,
> +       MMC_ERR_MAX,
> +};
> +
>  /*
>   * From V5, register spaces have changed. Wrap this info in a structure
>   * and choose the data_structure based on version info mentioned in DT.
> @@ -285,6 +302,8 @@ struct sdhci_msm_host {
>         u32 dll_config;
>         u32 ddr_config;
>         bool vqmmc_enabled;
> +       bool err_occurred;
> +       u32  err_stats[MMC_ERR_MAX];
>  };
>
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -2106,9 +2125,20 @@ static void sdhci_msm_set_timeout(struct sdhci_host *host, struct mmc_command *c
>                 host->data_timeout = 22LL * NSEC_PER_SEC;
>  }
>
> +void sdhci_msm_cqe_err_stats(struct mmc_host *mmc, unsigned long flags)
> +{
> +       struct sdhci_host *host = mmc_priv(mmc);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (flags & BIT(0))
> +               msm_host->err_stats[MMC_ERR_CMDQ_REQ_TIMEOUT]++;
> +}
> +
>  static const struct cqhci_host_ops sdhci_msm_cqhci_ops = {
>         .enable         = sdhci_msm_cqe_enable,
>         .disable        = sdhci_msm_cqe_disable,
> +       .err_stats      = sdhci_msm_cqe_err_stats,
>  #ifdef CONFIG_MMC_CRYPTO
>         .program_key    = sdhci_msm_program_key,
>  #endif
> @@ -2403,6 +2433,46 @@ static void sdhci_msm_dump_vendor_regs(struct sdhci_host *host)
>                 readl_relaxed(host->ioaddr +
>                         msm_offset->core_vendor_spec_func2),
>                 readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec3));
> +       msm_host->err_occurred = true;
> +}
> +
> +void sdhci_msm_err_stats(struct sdhci_host *host, u32 intmask)
> +{
> +       int command;
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (!msm_host->err_occurred)
> +               msm_host->err_stats[MMC_ERR_CMD_TIMEOUT] = 0;
> +
> +       if (host && host->mmc && host->mmc->timer) {
> +               msm_host->err_stats[MMC_ERR_REQ_TIMEOUT]++;
> +               host->mmc->timer = false;
> +       }
> +
> +       if (intmask & SDHCI_INT_CRC) {
> +               command = SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND));
> +               if (command != MMC_SEND_TUNING_BLOCK ||
> +                   command != MMC_SEND_TUNING_BLOCK_HS200)
> +                       msm_host->err_stats[MMC_ERR_CMD_CRC]++;
> +       } else if ((intmask & SDHCI_INT_TIMEOUT) ||
> +               (intmask & SDHCI_INT_DATA_TIMEOUT))
> +               msm_host->err_stats[MMC_ERR_CMD_TIMEOUT]++;
> +       else if (intmask & SDHCI_INT_DATA_CRC) {
> +               command = SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND));
> +               if (command != MMC_SEND_TUNING_BLOCK ||
> +                   command != MMC_SEND_TUNING_BLOCK_HS200)
> +                       msm_host->err_stats[MMC_ERR_DAT_CRC]++;
> +       } else if (intmask & SDHCI_INT_DATA_TIMEOUT)
> +               msm_host->err_stats[MMC_ERR_DAT_TIMEOUT]++;
> +       else if (intmask & CQHCI_IS_RED)
> +               msm_host->err_stats[MMC_ERR_CMDQ_RED]++;
> +       else if (intmask & CQHCI_IS_GCE)
> +               msm_host->err_stats[MMC_ERR_CMDQ_GCE]++;
> +       else if (intmask & CQHCI_IS_ICCE)
> +               msm_host->err_stats[MMC_ERR_CMDQ_ICCE]++;
> +       else if (intmask & SDHCI_INT_ADMA_ERROR)
> +               msm_host->err_stats[MMC_ERR_ADMA]++;
>  }
>
>  static const struct sdhci_msm_variant_ops mci_var_ops = {
> @@ -2456,6 +2526,7 @@ static const struct sdhci_ops sdhci_msm_ops = {
>         .dump_vendor_regs = sdhci_msm_dump_vendor_regs,
>         .set_power = sdhci_set_power_noreg,
>         .set_timeout = sdhci_msm_set_timeout,
> +       .err_stats = sdhci_msm_err_stats,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> @@ -2482,6 +2553,125 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
>         of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
>  }
>
> +static ssize_t err_state_show(struct device *dev,
> +                       struct device_attribute *attr, char *buf)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (!host || !host->mmc)
> +               return -EINVAL;
> +
> +       return scnprintf(buf, PAGE_SIZE, "%d\n", !!msm_host->err_occurred);
> +}
> +
> +static ssize_t err_state_store(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       unsigned int val;
> +
> +       if (kstrtouint(buf, 0, &val))
> +               return -EINVAL;
> +       if (!host || !host->mmc)
> +               return -EINVAL;
> +
> +       msm_host->err_occurred = !!val;
> +       if (!val)
> +               memset(msm_host->err_stats, 0, sizeof(msm_host->err_stats));
> +
> +       return count;
> +}
> +static DEVICE_ATTR_RW(err_state);
> +
> +static ssize_t err_stats_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       char tmp[100];
> +
> +       if (!host || !host->mmc)
> +               return -EINVAL;
> +
> +       scnprintf(tmp, sizeof(tmp), "# Command Timeout Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMD_TIMEOUT]);
> +       strlcpy(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Command CRC Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMD_CRC]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Data Timeout Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_DAT_TIMEOUT]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Data CRC Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_DAT_CRC]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Auto-Cmd Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_ADMA]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# ADMA Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_ADMA]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Tuning Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_TUNING]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# CMDQ RED Errors: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMDQ_RED]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# CMDQ GCE Errors: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMDQ_GCE]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# CMDQ ICCE Errors: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMDQ_ICCE]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# CMDQ Request Timedout: %d\n",
> +               msm_host->err_stats[MMC_ERR_CMDQ_REQ_TIMEOUT]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       scnprintf(tmp, sizeof(tmp), "# Request Timedout Error: %d\n",
> +               msm_host->err_stats[MMC_ERR_REQ_TIMEOUT]);
> +       strlcat(buf, tmp, PAGE_SIZE);
> +
> +       return strlen(buf);
> +}
> +static DEVICE_ATTR_RO(err_stats);
> +
> +static struct attribute *sdhci_msm_sysfs_attrs[] = {
> +       &dev_attr_err_state.attr,
> +       &dev_attr_err_stats.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group sdhci_msm_sysfs_group = {
> +       .name = "qcom",
> +       .attrs = sdhci_msm_sysfs_attrs,
> +};
> +
> +static int sdhci_msm_init_sysfs(struct platform_device *pdev)
> +{
> +       int ret;
> +
> +       ret = sysfs_create_group(&pdev->dev.kobj, &sdhci_msm_sysfs_group);
> +       if (ret)
> +               dev_err(&pdev->dev, "%s: Failed sdhci_msm sysfs group err=%d\n",
> +                       __func__, ret);
> +       return ret;
> +}
>
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
> @@ -2734,6 +2924,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (ret)
>                 goto pm_runtime_disable;
>
> +       sdhci_msm_init_sysfs(pdev);
> +
>         pm_runtime_mark_last_busy(&pdev->dev);
>         pm_runtime_put_autosuspend(&pdev->dev);
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 07c6da1..f82a3eff 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3159,6 +3159,13 @@ static void sdhci_timeout_timer(struct timer_list *t)
>         spin_lock_irqsave(&host->lock, flags);
>
>         if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
> +               if (host->ops->err_stats) {
> +                       u32 intmask;
> +
> +                       host->mmc->timer = true;
> +                       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +                       host->ops->err_stats(host, intmask);
> +               }
>                 pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
>                        mmc_hostname(host->mmc));
>                 sdhci_dumpregs(host);
> @@ -3181,6 +3188,13 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
>
>         if (host->data || host->data_cmd ||
>             (host->cmd && sdhci_data_line_cmd(host->cmd))) {
> +               if (host->ops->err_stats) {
> +                       u32 intmask;
> +
> +                       host->mmc->timer = true;
> +                       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +                       host->ops->err_stats(host, intmask);
> +               }
>                 pr_err("%s: Timeout waiting for hardware interrupt.\n",
>                        mmc_hostname(host->mmc));
>                 sdhci_dumpregs(host);
> @@ -3466,6 +3480,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>         }
>
>         intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +       if (host->ops->err_stats)
> +               host->ops->err_stats(host, intmask);
> +
>         if (!intmask || intmask == 0xffffffff) {
>                 result = IRQ_NONE;
>                 goto out;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d7929d7..a1546b3 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -658,6 +658,7 @@ struct sdhci_ops {
>         void    (*request_done)(struct sdhci_host *host,
>                                 struct mmc_request *mrq);
>         void    (*dump_vendor_regs)(struct sdhci_host *host);
> +       void    (*err_stats)(struct sdhci_host *host, u32 intmask);
>  };
>
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 7afb57c..33186ff 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -492,6 +492,7 @@ struct mmc_host {
>         int                     cqe_qdepth;
>         bool                    cqe_enabled;
>         bool                    cqe_on;
> +       bool                    timer;
>
>         /* Inline encryption support */
>  #ifdef CONFIG_MMC_CRYPTO
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>