[PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Shaik Sajida Bhanu]

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(+)

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

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Adrian Hunter]

On 17/11/2021 08:20, Shaik Sajida Bhanu wrote:
> Add sysfs entry to query eMMC and SD card errors statistics.
> This feature is useful for debug and testing.

Did you consider making it part of cqhci.c so that all drivers using
cqhci could benefit?

> 
> 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(+)
> 
> 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
> 

RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Sajida Bhanu (Temp) (QUIC)]

Please find the inline comments.

-----Original Message-----
From: Adrian Hunter <adrian.hunter@intel.com> 
Sent: Wednesday, November 17, 2021 1:15 PM
To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>; riteshh@codeaurora.org; Asutosh Das (asd) <asutoshd@quicinc.com>; ulf.hansson@linaro.org; agross@kernel.org; bjorn.andersson@linaro.org; linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: stummala@codeaurora.org; vbadigan@codeaurora.org; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

On 17/11/2021 08:20, Shaik Sajida Bhanu wrote:
> Add sysfs entry to query eMMC and SD card errors statistics.
> This feature is useful for debug and testing.

Did you consider making it part of cqhci.c so that all drivers using cqhci could benefit?

Hi Adrian,

Thanks for the review.
We are enabling  this changes for both eMMC and SD card as well. So , if we include these changes in cqhci.c then changes will not effect for SD card right.

Thanks,
Sajida
> 
> 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(+)
> 
> 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
> 

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Adrian Hunter]

On 22/11/2021 12:05, Sajida Bhanu (Temp) (QUIC) wrote:
> Please find the inline comments.
> 
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com> 
> Sent: Wednesday, November 17, 2021 1:15 PM
> To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>; riteshh@codeaurora.org; Asutosh Das (asd) <asutoshd@quicinc.com>; ulf.hansson@linaro.org; agross@kernel.org; bjorn.andersson@linaro.org; linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: stummala@codeaurora.org; vbadigan@codeaurora.org; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
> Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
> 
> On 17/11/2021 08:20, Shaik Sajida Bhanu wrote:
>> Add sysfs entry to query eMMC and SD card errors statistics.
>> This feature is useful for debug and testing.
> 
> Did you consider making it part of cqhci.c so that all drivers using cqhci could benefit?
> 
> Hi Adrian,
> 
> Thanks for the review.
> We are enabling  this changes for both eMMC and SD card as well. So , if we include these changes in cqhci.c then changes will not effect for SD card right.

Seems like something mmc core could support perhaps, with drivers calling mmc core functions to add their stats.
Needs Ulf to comment.

Alternatively, SDHCI could do it which would at least give support to other SDHCI drivers.

> 
> Thanks,
> Sajida
>>
>> 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(+)
>>
>> 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
>>
> 

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

Hi Shaik,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc1 next-20211117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shaik-Sajida-Bhanu/mmc-sdhci-msm-Add-eMMC-and-SD-card-err_stat-sysfs-entry/20211117-142139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8ab774587903771821b59471cc723bba6d893942
config: nios2-randconfig-r025-20211117 (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a840d7ed5c9df9bad36a213ed67a131251770770
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shaik-Sajida-Bhanu/mmc-sdhci-msm-Add-eMMC-and-SD-card-err_stat-sysfs-entry/20211117-142139
        git checkout a840d7ed5c9df9bad36a213ed67a131251770770
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci-msm.c:2128:6: warning: no previous prototype for 'sdhci_msm_cqe_err_stats' [-Wmissing-prototypes]
    2128 | void sdhci_msm_cqe_err_stats(struct mmc_host *mmc, unsigned long flags)
         |      ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/mmc/host/sdhci-msm.c:2439:6: warning: no previous prototype for 'sdhci_msm_err_stats' [-Wmissing-prototypes]
    2439 | void sdhci_msm_err_stats(struct sdhci_host *host, u32 intmask)
         |      ^~~~~~~~~~~~~~~~~~~


vim +/sdhci_msm_cqe_err_stats +2128 drivers/mmc/host/sdhci-msm.c

  2127	
> 2128	void sdhci_msm_cqe_err_stats(struct mmc_host *mmc, unsigned long flags)
  2129	{
  2130		struct sdhci_host *host = mmc_priv(mmc);
  2131		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
  2132		struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
  2133	
  2134		if (flags & BIT(0))
  2135			msm_host->err_stats[MMC_ERR_CMDQ_REQ_TIMEOUT]++;
  2136	}
  2137	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24125 bytes --]

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Ulf Hansson]

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
>

RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Sajida Bhanu (Temp) (QUIC)]

-----Original Message-----
From: Ulf Hansson <ulf.hansson@linaro.org> 
Sent: Thursday, November 25, 2021 5:01 PM
To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

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

Hi Ulf,

Thanks for the review

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?

>>>>>>>>>>>> These changes for debug purpose only .. if any errors occurred while testing eMMC and SD card those will be captured in these sysfs entries ,  we can directly go and check the sysfs entries and get to know what is the error occurred in driver level.

Thanks,
Sajida
>
> 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
>

RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Sajida Bhanu (Temp) (QUIC)]

Gentle Reminder.

Thanks,
Sajida
-----Original Message-----
From: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com> 
Sent: Friday, November 26, 2021 10:54 AM
To: Ulf Hansson <ulf.hansson@linaro.org>; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
Subject: RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry



-----Original Message-----
From: Ulf Hansson <ulf.hansson@linaro.org>
Sent: Thursday, November 25, 2021 5:01 PM
To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

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

Hi Ulf,

Thanks for the review

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?

>>>>>>>>>>>> These changes for debug purpose only .. if any errors occurred while testing eMMC and SD card those will be captured in these sysfs entries ,  we can directly go and check the sysfs entries and get to know what is the error occurred in driver level.

Thanks,
Sajida
>
> 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
>

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Adrian Hunter]

On 02/12/2021 08:42, Sajida Bhanu (Temp) (QUIC) wrote:
> Gentle Reminder.
> 
> Thanks,
> Sajida
> -----Original Message-----
> From: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com> 
> Sent: Friday, November 26, 2021 10:54 AM
> To: Ulf Hansson <ulf.hansson@linaro.org>; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
> Subject: RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
> 
> 
> 
> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Thursday, November 25, 2021 5:01 PM
> To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
> Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
> 
> 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
> 
> Hi Ulf,
> 
> Thanks for the review
> 
> 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?
> 
>>>>>>>>>>>>> These changes for debug purpose only .. if any errors occurred while testing eMMC and SD card those will be captured in these sysfs entries ,  we can directly go and check the sysfs entries and get to know what is the error occurred in driver level.

I would suggest using debugfs and adding support in mmc host debugfs
e.g.

static inline void mmc_debugfs_err_stats_enable(struct mmc_host *mmc)
{
	mmc->err_stats_enabled = true;
}

enum mmc_err_stat {
	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,
};

static inline void mmc_debugfs_err_stats_inc(struct mmc_host *mmc, enum mmc_err_stat stat)
{
	mmc->err_stats[stat] += 1;
}

And amend mmc_add_host_debugfs() to create the err_stats file etc.

sdhci.c could call mmc_debugfs_err_stats_enable() and mmc_debugfs_err_stats_inc() as needed.
cqhci.c could call mmc_debugfs_err_stats_inc() as needed.

Ulf, does that sound reasonable?

> 
> Thanks,
> Sajida
>>
>> 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
>>
> 

Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Ulf Hansson]

On Thu, 2 Dec 2021 at 08:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 02/12/2021 08:42, Sajida Bhanu (Temp) (QUIC) wrote:
> > Gentle Reminder.
> >
> > Thanks,
> > Sajida
> > -----Original Message-----
> > From: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> > Sent: Friday, November 26, 2021 10:54 AM
> > To: Ulf Hansson <ulf.hansson@linaro.org>; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> > Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
> > Subject: RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
> >
> >
> >
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Thursday, November 25, 2021 5:01 PM
> > To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> > Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
> > Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry
> >
> > 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
> >
> > Hi Ulf,
> >
> > Thanks for the review
> >
> > 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?
> >
> >>>>>>>>>>>>> These changes for debug purpose only .. if any errors occurred while testing eMMC and SD card those will be captured in these sysfs entries ,  we can directly go and check the sysfs entries and get to know what is the error occurred in driver level.
>
> I would suggest using debugfs and adding support in mmc host debugfs
> e.g.
>
> static inline void mmc_debugfs_err_stats_enable(struct mmc_host *mmc)
> {
>         mmc->err_stats_enabled = true;
> }
>
> enum mmc_err_stat {
>         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,
> };
>
> static inline void mmc_debugfs_err_stats_inc(struct mmc_host *mmc, enum mmc_err_stat stat)
> {
>         mmc->err_stats[stat] += 1;
> }
>
> And amend mmc_add_host_debugfs() to create the err_stats file etc.
>
> sdhci.c could call mmc_debugfs_err_stats_enable() and mmc_debugfs_err_stats_inc() as needed.
> cqhci.c could call mmc_debugfs_err_stats_inc() as needed.
>
> Ulf, does that sound reasonable?

Yes, it does! Thanks for having a closer look!

[...]

Kind regards
Uffe

RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

[Sajida Bhanu (Temp) (QUIC)]

Hi,

Thank you for suggestion .. will make the changes .

Thanks,
Sajida
-----Original Message-----
From: Ulf Hansson <ulf.hansson@linaro.org> 
Sent: Thursday, December 2, 2021 1:35 PM
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>; riteshh@codeaurora.org; Asutosh Das (asd) <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; Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; nitirawa@codeaurora.org; sayalil@codeaurora.org
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card err_stat sysfs entry

On Thu, 2 Dec 2021 at 08:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 02/12/2021 08:42, Sajida Bhanu (Temp) (QUIC) wrote:
> > Gentle Reminder.
> >
> > Thanks,
> > Sajida
> > -----Original Message-----
> > From: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> > Sent: Friday, November 26, 2021 10:54 AM
> > To: Ulf Hansson <ulf.hansson@linaro.org>; Sajida Bhanu (Temp) (QUIC) 
> > <quic_c_sbhanu@quicinc.com>
> > Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das 
> > (asd) <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; Ram Prakash Gupta 
> > (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) 
> > <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; 
> > nitirawa@codeaurora.org; sayalil@codeaurora.org
> > Subject: RE: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card 
> > err_stat sysfs entry
> >
> >
> >
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Thursday, November 25, 2021 5:01 PM
> > To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> > Cc: adrian.hunter@intel.com; riteshh@codeaurora.org; Asutosh Das 
> > (asd) <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; Ram Prakash Gupta 
> > (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) 
> > <quic_pragalla@quicinc.com>; sartgarg@codeaurora.org; 
> > nitirawa@codeaurora.org; sayalil@codeaurora.org
> > Subject: Re: [PATCH V1] mmc: sdhci-msm: Add eMMC and SD card 
> > err_stat sysfs entry
> >
> > 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
> >
> > Hi Ulf,
> >
> > Thanks for the review
> >
> > 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?
> >
> >>>>>>>>>>>>> These changes for debug purpose only .. if any errors occurred while testing eMMC and SD card those will be captured in these sysfs entries ,  we can directly go and check the sysfs entries and get to know what is the error occurred in driver level.
>
> I would suggest using debugfs and adding support in mmc host debugfs 
> e.g.
>
> static inline void mmc_debugfs_err_stats_enable(struct mmc_host *mmc) 
> {
>         mmc->err_stats_enabled = true; }
>
> enum mmc_err_stat {
>         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,
> };
>
> static inline void mmc_debugfs_err_stats_inc(struct mmc_host *mmc, 
> enum mmc_err_stat stat) {
>         mmc->err_stats[stat] += 1;
> }
>
> And amend mmc_add_host_debugfs() to create the err_stats file etc.
>
> sdhci.c could call mmc_debugfs_err_stats_enable() and mmc_debugfs_err_stats_inc() as needed.
> cqhci.c could call mmc_debugfs_err_stats_inc() as needed.
>
> Ulf, does that sound reasonable?

Yes, it does! Thanks for having a closer look!

[...]

Kind regards
Uffe