All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card
@ 2022-04-25 16:00 Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 1/5] mmc: core: Capture eMMC and SD card errors Shaik Sajida Bhanu
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu

Changes since V4:
	- Defined new macro to increment err_stats members when error occured
	  as suggested by Adrain Hunter.
	- Called err_stats members increment function after printing the error
	  as suggested by Adrain Hunter.
	- Considered INDEX and END_BIT errors same as CRC errors as suggested
	  by Adrain Hunter.
	- Removed Null check for host in debug fs functions and Reordered
	  err_stats declarationas suggested by Adrain Hunter.
	- Removed err_state variable stuff and updated err_state debug fs entry
	  based on the err_stats members state as suggested by Adrain Hunter.

Changes since V3:
	- Dropped error stats feature flag as suggested by Adrain Hunter.
	- Separated error state related changes in separate patches as
	  suggested by Adrain Hunter.
	  [PATCH V4 4/7] : error state debug fs
	  [PATCH V4 5/7] : error state enable function
	  [PATCH V4 6/7] : error state enable in error case
	- Note: we are enabling error state before calling sdhci_dumpregs
	  we couldn't add the err state in error stats array as err state
	  is not error type.
	- Corrected Signed-off-by order as suggested by Bjron Andersson.
	- Moved error state enable code from sdhci_dumpregs to error
	  conditions as suggested by Adrain Hunter

Changes since V2:
	- Removed userspace error stats clear debug fs entry as suggested
	  by Adrain Hunter.
	- Split patch into 4 patches
	  [PATCH V3 1/4] : sdhci driver
	  [PATCH V3 2/4] : debug fs entries
	  [PATCH V3 3/4] : core driver
	  [PATCH V3 4/4] : cqhci driver
	- Used for loop to print error messages instead of using printf
	  statements for all error messages as suggested by Adrain Hunter.
	- Introduced one flag to enable error stats feature, if any other
	  client wants to use this feature, they need to enable that flag.
	- Moved reset command timeout error statement to card init flow
	  as suggested by Adrain Hunter.

Changes since V1:
	- Removed sysfs entry for eMMC and SD card error statistics and added
	  debugfs entry as suggested by Adrian Hunter and Ulf Hansson.

Shaik Sajida Bhanu (5):
  mmc: core: Capture eMMC and SD card errors
  mmc: sdhci: Capture eMMC and SD card errors
  mmc: debugfs: Add debug fs entry for mmc driver
  mmc: debugfs: Add debug fs error state entry for mmc driver
  mmc: cqhci: Capture eMMC and SD card errors

 drivers/mmc/core/core.c       |  6 ++++
 drivers/mmc/core/debugfs.c    | 81 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/cqhci-core.c |  9 ++++-
 drivers/mmc/host/sdhci.c      | 54 ++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h      |  3 ++
 include/linux/mmc/host.h      | 26 ++++++++++++++
 include/linux/mmc/mmc.h       |  6 ++++
 7 files changed, 171 insertions(+), 14 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V5 1/5] mmc: core: Capture eMMC and SD card errors
  2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
@ 2022-04-25 16:00 ` Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 2/5] mmc: sdhci: " Shaik Sajida Bhanu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu, Liangliang Lu, Bao D . Nguyen

Add changes to capture eMMC and SD card errors.
This is useful for debug and testing.

Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---
 drivers/mmc/core/core.c  |  6 ++++++
 include/linux/mmc/host.h | 26 ++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8cc2b74..6d086e9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2247,6 +2247,12 @@ void mmc_rescan(struct work_struct *work)
 		if (freqs[i] <= host->f_min)
 			break;
 	}
+
+	/*
+	 * Ignore the command timeout errors observed during
+	 * the card init as those are excepted.
+	 */
+	host->err_stats[MMC_ERR_CMD_TIMEOUT] = 0;
 	mmc_release_host(host);
 
  out:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c193c50..eb8bc5b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -93,6 +93,25 @@ struct mmc_clk_phase_map {
 
 struct mmc_host;
 
+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_CTRL_TIMEOUT,
+	MMC_ERR_UNEXPECTED_IRQ,
+	MMC_ERR_MAX,
+};
+
 struct mmc_host_ops {
 	/*
 	 * It is optional for the host to implement pre_req and post_req in
@@ -501,6 +520,7 @@ struct mmc_host {
 	/* Host Software Queue support */
 	bool			hsq_enabled;
 
+	u32			err_stats[MMC_ERR_MAX];
 	unsigned long		private[] ____cacheline_aligned;
 };
 
@@ -635,6 +655,12 @@ static inline enum dma_data_direction mmc_get_dma_dir(struct mmc_data *data)
 	return data->flags & MMC_DATA_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
 }
 
+static inline void mmc_debugfs_err_stats_inc(struct mmc_host *host,
+					     enum mmc_err_stat stat)
+{
+	host->err_stats[stat] += 1;
+}
+
 int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
 int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V5 2/5] mmc: sdhci: Capture eMMC and SD card errors
  2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 1/5] mmc: core: Capture eMMC and SD card errors Shaik Sajida Bhanu
@ 2022-04-25 16:00 ` Shaik Sajida Bhanu
  2022-04-26  7:51   ` Adrian Hunter
  2022-04-25 16:00 ` [PATCH V5 3/5] mmc: debugfs: Add debug fs entry for mmc driver Shaik Sajida Bhanu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu, Liangliang Lu, Bao D . Nguyen

Add changes to capture eMMC and SD card errors.
This is useful for debug and testing.

Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---
 drivers/mmc/host/sdhci.c | 54 ++++++++++++++++++++++++++++++++++++------------
 drivers/mmc/host/sdhci.h |  3 +++
 include/linux/mmc/mmc.h  |  6 ++++++
 3 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2215202..1cda28ba 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -224,6 +224,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
 		if (timedout) {
 			pr_err("%s: Reset 0x%x never completed.\n",
 				mmc_hostname(host->mmc), (int)mask);
+			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
 			sdhci_dumpregs(host);
 			return;
 		}
@@ -1716,6 +1717,7 @@ static bool sdhci_send_command_retry(struct sdhci_host *host,
 		if (!timeout--) {
 			pr_err("%s: Controller never released inhibit bit(s).\n",
 			       mmc_hostname(host->mmc));
+			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
 			sdhci_dumpregs(host);
 			cmd->error = -EIO;
 			return false;
@@ -1965,6 +1967,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 		if (timedout) {
 			pr_err("%s: Internal clock never stabilised.\n",
 			       mmc_hostname(host->mmc));
+			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
 			sdhci_dumpregs(host);
 			return;
 		}
@@ -1987,6 +1990,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 			if (timedout) {
 				pr_err("%s: PLL clock never stabilised.\n",
 				       mmc_hostname(host->mmc));
+				sdhci_err_stats_inc(host, CTRL_TIMEOUT);
 				sdhci_dumpregs(host);
 				return;
 			}
@@ -3161,6 +3165,7 @@ static void sdhci_timeout_timer(struct timer_list *t)
 	if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
 		pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
 		       mmc_hostname(host->mmc));
+		sdhci_err_stats_inc(host, REQ_TIMEOUT);
 		sdhci_dumpregs(host);
 
 		host->cmd->error = -ETIMEDOUT;
@@ -3183,6 +3188,7 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
 	    (host->cmd && sdhci_data_line_cmd(host->cmd))) {
 		pr_err("%s: Timeout waiting for hardware interrupt.\n",
 		       mmc_hostname(host->mmc));
+		sdhci_err_stats_inc(host, REQ_TIMEOUT);
 		sdhci_dumpregs(host);
 
 		if (host->data) {
@@ -3234,17 +3240,21 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
 			return;
 		pr_err("%s: Got command interrupt 0x%08x even though no command operation was in progress.\n",
 		       mmc_hostname(host->mmc), (unsigned)intmask);
+		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
 		sdhci_dumpregs(host);
 		return;
 	}
 
 	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
 		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
-		if (intmask & SDHCI_INT_TIMEOUT)
+		if (intmask & SDHCI_INT_TIMEOUT) {
 			host->cmd->error = -ETIMEDOUT;
-		else
+			sdhci_err_stats_inc(host, CMD_TIMEOUT);
+		} else {
 			host->cmd->error = -EILSEQ;
-
+			if (!mmc_op_tuning(host->cmd->opcode))
+				sdhci_err_stats_inc(host, CMD_CRC);
+		}
 		/* Treat data command CRC error the same as data CRC error */
 		if (host->cmd->data &&
 		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
@@ -3266,6 +3276,8 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
 			  -ETIMEDOUT :
 			  -EILSEQ;
 
+		sdhci_err_stats_inc(host, AUTO_CMD);
+
 		if (sdhci_auto_cmd23(host, mrq)) {
 			mrq->sbc->error = err;
 			__sdhci_finish_mrq(host, mrq);
@@ -3342,6 +3354,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 			if (intmask & SDHCI_INT_DATA_TIMEOUT) {
 				host->data_cmd = NULL;
 				data_cmd->error = -ETIMEDOUT;
+				sdhci_err_stats_inc(host, CMD_TIMEOUT);
 				__sdhci_finish_mrq(host, data_cmd->mrq);
 				return;
 			}
@@ -3370,23 +3383,29 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 
 		pr_err("%s: Got data interrupt 0x%08x even though no data operation was in progress.\n",
 		       mmc_hostname(host->mmc), (unsigned)intmask);
+		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
 		sdhci_dumpregs(host);
 
 		return;
 	}
 
-	if (intmask & SDHCI_INT_DATA_TIMEOUT)
+	if (intmask & SDHCI_INT_DATA_TIMEOUT) {
 		host->data->error = -ETIMEDOUT;
-	else if (intmask & SDHCI_INT_DATA_END_BIT)
+		sdhci_err_stats_inc(host, DAT_TIMEOUT);
+	} else if (intmask & SDHCI_INT_DATA_END_BIT)
 		host->data->error = -EILSEQ;
 	else if ((intmask & SDHCI_INT_DATA_CRC) &&
 		SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
-			!= MMC_BUS_TEST_R)
+			!= MMC_BUS_TEST_R) {
 		host->data->error = -EILSEQ;
+		if (!mmc_op_tuning(host->cmd->opcode))
+			sdhci_err_stats_inc(host, DAT_CRC);
+	}
 	else if (intmask & SDHCI_INT_ADMA_ERROR) {
 		pr_err("%s: ADMA error: 0x%08x\n", mmc_hostname(host->mmc),
 		       intmask);
 		sdhci_adma_show_error(host);
+		sdhci_err_stats_inc(host, ADMA);
 		host->data->error = -EIO;
 		if (host->ops->adma_workaround)
 			host->ops->adma_workaround(host, intmask);
@@ -3584,6 +3603,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	if (unexpected) {
 		pr_err("%s: Unexpected interrupt 0x%08x.\n",
 			   mmc_hostname(host->mmc), unexpected);
+		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
 		sdhci_dumpregs(host);
 	}
 
@@ -3905,20 +3925,27 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
 	if (!host->cqe_on)
 		return false;
 
-	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC))
+	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
 		*cmd_error = -EILSEQ;
-	else if (intmask & SDHCI_INT_TIMEOUT)
+		if (!mmc_op_tuning(host->cmd->opcode))
+			sdhci_err_stats_inc(host, CMD_CRC);
+	} else if (intmask & SDHCI_INT_TIMEOUT) {
 		*cmd_error = -ETIMEDOUT;
-	else
+		sdhci_err_stats_inc(host, CMD_TIMEOUT);
+	} else
 		*cmd_error = 0;
 
-	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
+	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
 		*data_error = -EILSEQ;
-	else if (intmask & SDHCI_INT_DATA_TIMEOUT)
+		if (!mmc_op_tuning(host->cmd->opcode))
+			sdhci_err_stats_inc(host, DAT_CRC);
+	} else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
 		*data_error = -ETIMEDOUT;
-	else if (intmask & SDHCI_INT_ADMA_ERROR)
+		sdhci_err_stats_inc(host, DAT_TIMEOUT);
+	} else if (intmask & SDHCI_INT_ADMA_ERROR) {
 		*data_error = -EIO;
-	else
+		sdhci_err_stats_inc(host, ADMA);
+	} else
 		*data_error = 0;
 
 	/* Clear selected interrupts. */
@@ -3934,6 +3961,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
 		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 		pr_err("%s: CQE: Unexpected interrupt 0x%08x.\n",
 		       mmc_hostname(host->mmc), intmask);
+		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
 		sdhci_dumpregs(host);
 	}
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d7929d7..95a08f0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -356,6 +356,9 @@ struct sdhci_adma2_64_desc {
  */
 #define MMC_CMD_TRANSFER_TIME	(10 * NSEC_PER_MSEC) /* max 10 ms */
 
+#define sdhci_err_stats_inc(host, err_name) \
+	mmc_debugfs_err_stats_inc((host)->mmc, MMC_ERR_##err_name)
+
 enum sdhci_cookie {
 	COOKIE_UNMAPPED,
 	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index d9a65c6..9c50bc4 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -99,6 +99,12 @@ static inline bool mmc_op_multi(u32 opcode)
 	       opcode == MMC_READ_MULTIPLE_BLOCK;
 }
 
+static inline bool mmc_op_tuning(u32 opcode)
+{
+	return opcode == MMC_SEND_TUNING_BLOCK ||
+			opcode == MMC_SEND_TUNING_BLOCK_HS200;
+}
+
 /*
  * MMC_SWITCH argument format:
  *
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V5 3/5] mmc: debugfs: Add debug fs entry for mmc driver
  2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 1/5] mmc: core: Capture eMMC and SD card errors Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 2/5] mmc: sdhci: " Shaik Sajida Bhanu
@ 2022-04-25 16:00 ` Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 4/5] mmc: debugfs: Add debug fs error state " Shaik Sajida Bhanu
  2022-04-25 16:00 ` [PATCH V5 5/5] mmc: cqhci: Capture eMMC and SD card errors Shaik Sajida Bhanu
  4 siblings, 0 replies; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu, Liangliang Lu, Bao D . Nguyen

Add debug fs entry to query eMMC and SD card errors statistics

Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---
 drivers/mmc/core/debugfs.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 3fdbc80..6aa5a60 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -223,6 +223,59 @@ static int mmc_clock_opt_set(void *data, u64 val)
 DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
 	"%llu\n");
 
+static int mmc_err_stats_show(struct seq_file *file, void *data)
+{
+	struct mmc_host *host = (struct mmc_host *)file->private;
+	const char *desc[MMC_ERR_MAX] = {
+		[MMC_ERR_CMD_TIMEOUT] = "Command Timeout Occurred",
+		[MMC_ERR_CMD_CRC] = "Command CRC Errors Occurred",
+		[MMC_ERR_DAT_TIMEOUT] = "Data Timeout Occurred",
+		[MMC_ERR_DAT_CRC] = "Data CRC Errors Occurred",
+		[MMC_ERR_AUTO_CMD] = "Auto-Cmd Error Occurred",
+		[MMC_ERR_ADMA] = "ADMA Error Occurred",
+		[MMC_ERR_TUNING] = "Tuning Error Occurred",
+		[MMC_ERR_CMDQ_RED] = "CMDQ RED Errors",
+		[MMC_ERR_CMDQ_GCE] = "CMDQ GCE Errors",
+		[MMC_ERR_CMDQ_ICCE] = "CMDQ ICCE Errors",
+		[MMC_ERR_REQ_TIMEOUT] = "Request Timedout",
+		[MMC_ERR_CMDQ_REQ_TIMEOUT] = "CMDQ Request Timedout",
+		[MMC_ERR_ICE_CFG] = "ICE Config Errors",
+		[MMC_ERR_CTRL_TIMEOUT] = "Controller Timedout errors",
+		[MMC_ERR_UNEXPECTED_IRQ] = "Unexpected IRQ errors",
+	};
+	int i;
+
+	for (i = 0; i < MMC_ERR_MAX; i++) {
+		if (desc[i])
+			seq_printf(file, "# %s:\t %d\n",
+					desc[i], host->err_stats[i]);
+	}
+
+	return 0;
+}
+
+static int mmc_err_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, mmc_err_stats_show, inode->i_private);
+}
+
+static ssize_t mmc_err_stats_write(struct file *filp, const char __user *ubuf,
+				   size_t cnt, loff_t *ppos)
+{
+	struct mmc_host *host = filp->f_mapping->host->i_private;
+
+	pr_debug("%s: Resetting MMC error statistics\n", __func__);
+	memset(host->err_stats, 0, sizeof(host->err_stats));
+
+	return cnt;
+}
+
+static const struct file_operations mmc_err_stats_fops = {
+	.open	= mmc_err_stats_open,
+	.read	= seq_read,
+	.write	= mmc_err_stats_write,
+};
+
 void mmc_add_host_debugfs(struct mmc_host *host)
 {
 	struct dentry *root;
@@ -236,6 +289,9 @@ void mmc_add_host_debugfs(struct mmc_host *host)
 	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
 				   &mmc_clock_fops);
 
+	debugfs_create_file("err_stats", 0600, root, host,
+			    &mmc_err_stats_fops);
+
 #ifdef CONFIG_FAIL_MMC_REQUEST
 	if (fail_request)
 		setup_fault_attr(&fail_default_attr, fail_request);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
  2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
                   ` (2 preceding siblings ...)
  2022-04-25 16:00 ` [PATCH V5 3/5] mmc: debugfs: Add debug fs entry for mmc driver Shaik Sajida Bhanu
@ 2022-04-25 16:00 ` Shaik Sajida Bhanu
  2022-04-26  7:54   ` Adrian Hunter
  2022-04-25 16:00 ` [PATCH V5 5/5] mmc: cqhci: Capture eMMC and SD card errors Shaik Sajida Bhanu
  4 siblings, 1 reply; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu, Liangliang Lu, Bao D . Nguyen

Add debug fs entry error state to query eMMC and SD card errors statistics.
If any errors occurred in eMMC and SD card driver level then
err_state value will be set to 1.

Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---
 drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 6aa5a60..2f5b63f 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
 
 DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
 	"%llu\n");
+static int mmc_err_state_get(void *data, u64 *val)
+{
+	struct mmc_host *host = data;
+
+	if (!host)
+		return -EINVAL;
+
+	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
+	       host->err_stats[MMC_ERR_ADMA] ||
+	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
+	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
+	       host->err_stats[MMC_ERR_CMDQ_RED] ||
+	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
+	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
+	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
+	       host->err_stats[MMC_ERR_DAT_CRC] ||
+	       host->err_stats[MMC_ERR_CMD_CRC] ||
+	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
 
 static int mmc_err_stats_show(struct seq_file *file, void *data)
 {
@@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
 	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
 				   &mmc_clock_fops);
 
+	debugfs_create_file("err_state", 0600, root, host,
+			    &mmc_err_state);
 	debugfs_create_file("err_stats", 0600, root, host,
 			    &mmc_err_stats_fops);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V5 5/5] mmc: cqhci: Capture eMMC and SD card errors
  2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
                   ` (3 preceding siblings ...)
  2022-04-25 16:00 ` [PATCH V5 4/5] mmc: debugfs: Add debug fs error state " Shaik Sajida Bhanu
@ 2022-04-25 16:00 ` Shaik Sajida Bhanu
  4 siblings, 0 replies; 13+ messages in thread
From: Shaik Sajida Bhanu @ 2022-04-25 16:00 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Shaik Sajida Bhanu, Liangliang Lu, Bao D . Nguyen

Add changes to capture eMMC and SD card errors.
This is useful for debug and testing.

Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---
 drivers/mmc/host/cqhci-core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
index b0d30c3..b3d7d6d 100644
--- a/drivers/mmc/host/cqhci-core.c
+++ b/drivers/mmc/host/cqhci-core.c
@@ -822,8 +822,15 @@ irqreturn_t cqhci_irq(struct mmc_host *mmc, u32 intmask, int cmd_error,
 	pr_debug("%s: cqhci: IRQ status: 0x%08x\n", mmc_hostname(mmc), status);
 
 	if ((status & (CQHCI_IS_RED | CQHCI_IS_GCE | CQHCI_IS_ICCE)) ||
-	    cmd_error || data_error)
+	    cmd_error || data_error) {
+		if (status & CQHCI_IS_RED)
+			mmc_debugfs_err_stats_inc(mmc, MMC_ERR_CMDQ_RED);
+		if (status & CQHCI_IS_GCE)
+			mmc_debugfs_err_stats_inc(mmc, MMC_ERR_CMDQ_GCE);
+		if (status & CQHCI_IS_ICCE)
+			mmc_debugfs_err_stats_inc(mmc, MMC_ERR_CMDQ_ICCE);
 		cqhci_error_irq(mmc, status, cmd_error, data_error);
+	}
 
 	if (status & CQHCI_IS_TCC) {
 		/* read TCN and complete the request */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 2/5] mmc: sdhci: Capture eMMC and SD card errors
  2022-04-25 16:00 ` [PATCH V5 2/5] mmc: sdhci: " Shaik Sajida Bhanu
@ 2022-04-26  7:51   ` Adrian Hunter
  2022-05-09  9:34     ` Sajida Bhanu (Temp)
  0 siblings, 1 reply; 13+ messages in thread
From: Adrian Hunter @ 2022-04-26  7:51 UTC (permalink / raw)
  To: Shaik Sajida Bhanu, ulf.hansson, wsa+renesas,
	yoshihiro.shimoda.uh, linus.walleij, digetx, briannorris,
	quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
> Add changes to capture eMMC and SD card errors.
> This is useful for debug and testing.
> 
> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>

Looks good.  A couple of minor comments.

> ---
>  drivers/mmc/host/sdhci.c | 54 ++++++++++++++++++++++++++++++++++++------------
>  drivers/mmc/host/sdhci.h |  3 +++
>  include/linux/mmc/mmc.h  |  6 ++++++
>  3 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2215202..1cda28ba 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -224,6 +224,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
>  		if (timedout) {
>  			pr_err("%s: Reset 0x%x never completed.\n",
>  				mmc_hostname(host->mmc), (int)mask);
> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>  			sdhci_dumpregs(host);
>  			return;
>  		}
> @@ -1716,6 +1717,7 @@ static bool sdhci_send_command_retry(struct sdhci_host *host,
>  		if (!timeout--) {
>  			pr_err("%s: Controller never released inhibit bit(s).\n",
>  			       mmc_hostname(host->mmc));
> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>  			sdhci_dumpregs(host);
>  			cmd->error = -EIO;
>  			return false;
> @@ -1965,6 +1967,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  		if (timedout) {
>  			pr_err("%s: Internal clock never stabilised.\n",
>  			       mmc_hostname(host->mmc));
> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>  			sdhci_dumpregs(host);
>  			return;
>  		}
> @@ -1987,6 +1990,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>  			if (timedout) {
>  				pr_err("%s: PLL clock never stabilised.\n",
>  				       mmc_hostname(host->mmc));
> +				sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>  				sdhci_dumpregs(host);
>  				return;
>  			}
> @@ -3161,6 +3165,7 @@ static void sdhci_timeout_timer(struct timer_list *t)
>  	if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
>  		pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
>  		       mmc_hostname(host->mmc));
> +		sdhci_err_stats_inc(host, REQ_TIMEOUT);
>  		sdhci_dumpregs(host);
>  
>  		host->cmd->error = -ETIMEDOUT;
> @@ -3183,6 +3188,7 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
>  	    (host->cmd && sdhci_data_line_cmd(host->cmd))) {
>  		pr_err("%s: Timeout waiting for hardware interrupt.\n",
>  		       mmc_hostname(host->mmc));
> +		sdhci_err_stats_inc(host, REQ_TIMEOUT);
>  		sdhci_dumpregs(host);
>  
>  		if (host->data) {
> @@ -3234,17 +3240,21 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
>  			return;
>  		pr_err("%s: Got command interrupt 0x%08x even though no command operation was in progress.\n",
>  		       mmc_hostname(host->mmc), (unsigned)intmask);
> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>  		sdhci_dumpregs(host);
>  		return;
>  	}
>  
>  	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
>  		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
> -		if (intmask & SDHCI_INT_TIMEOUT)
> +		if (intmask & SDHCI_INT_TIMEOUT) {
>  			host->cmd->error = -ETIMEDOUT;
> -		else
> +			sdhci_err_stats_inc(host, CMD_TIMEOUT);
> +		} else {
>  			host->cmd->error = -EILSEQ;
> -
> +			if (!mmc_op_tuning(host->cmd->opcode))
> +				sdhci_err_stats_inc(host, CMD_CRC);
> +		}
>  		/* Treat data command CRC error the same as data CRC error */
>  		if (host->cmd->data &&
>  		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
> @@ -3266,6 +3276,8 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
>  			  -ETIMEDOUT :
>  			  -EILSEQ;
>  
> +		sdhci_err_stats_inc(host, AUTO_CMD);
> +
>  		if (sdhci_auto_cmd23(host, mrq)) {
>  			mrq->sbc->error = err;
>  			__sdhci_finish_mrq(host, mrq);
> @@ -3342,6 +3354,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  			if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>  				host->data_cmd = NULL;
>  				data_cmd->error = -ETIMEDOUT;
> +				sdhci_err_stats_inc(host, CMD_TIMEOUT);
>  				__sdhci_finish_mrq(host, data_cmd->mrq);
>  				return;
>  			}
> @@ -3370,23 +3383,29 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  
>  		pr_err("%s: Got data interrupt 0x%08x even though no data operation was in progress.\n",
>  		       mmc_hostname(host->mmc), (unsigned)intmask);
> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>  		sdhci_dumpregs(host);
>  
>  		return;
>  	}
>  
> -	if (intmask & SDHCI_INT_DATA_TIMEOUT)
> +	if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>  		host->data->error = -ETIMEDOUT;
> -	else if (intmask & SDHCI_INT_DATA_END_BIT)
> +		sdhci_err_stats_inc(host, DAT_TIMEOUT);
> +	} else if (intmask & SDHCI_INT_DATA_END_BIT)
>  		host->data->error = -EILSEQ;

Seems to be missing here:

		sdhci_err_stats_inc(host, DAT_CRC);

Also it would be nice to have braces {} on all arms of if-else-if
Can use checkpatch.pl --strict to see where

>  	else if ((intmask & SDHCI_INT_DATA_CRC) &&
>  		SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
> -			!= MMC_BUS_TEST_R)
> +			!= MMC_BUS_TEST_R) {
>  		host->data->error = -EILSEQ;
> +		if (!mmc_op_tuning(host->cmd->opcode))
> +			sdhci_err_stats_inc(host, DAT_CRC);
> +	}
>  	else if (intmask & SDHCI_INT_ADMA_ERROR) {
>  		pr_err("%s: ADMA error: 0x%08x\n", mmc_hostname(host->mmc),
>  		       intmask);
>  		sdhci_adma_show_error(host);
> +		sdhci_err_stats_inc(host, ADMA);
>  		host->data->error = -EIO;
>  		if (host->ops->adma_workaround)
>  			host->ops->adma_workaround(host, intmask);
> @@ -3584,6 +3603,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  	if (unexpected) {
>  		pr_err("%s: Unexpected interrupt 0x%08x.\n",
>  			   mmc_hostname(host->mmc), unexpected);
> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>  		sdhci_dumpregs(host);
>  	}
>  
> @@ -3905,20 +3925,27 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>  	if (!host->cqe_on)
>  		return false;
>  
> -	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC))
> +	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
>  		*cmd_error = -EILSEQ;
> -	else if (intmask & SDHCI_INT_TIMEOUT)
> +		if (!mmc_op_tuning(host->cmd->opcode))
> +			sdhci_err_stats_inc(host, CMD_CRC);
> +	} else if (intmask & SDHCI_INT_TIMEOUT) {
>  		*cmd_error = -ETIMEDOUT;
> -	else
> +		sdhci_err_stats_inc(host, CMD_TIMEOUT);
> +	} else
>  		*cmd_error = 0;
>  
> -	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
> +	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
>  		*data_error = -EILSEQ;
> -	else if (intmask & SDHCI_INT_DATA_TIMEOUT)
> +		if (!mmc_op_tuning(host->cmd->opcode))
> +			sdhci_err_stats_inc(host, DAT_CRC);
> +	} else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>  		*data_error = -ETIMEDOUT;
> -	else if (intmask & SDHCI_INT_ADMA_ERROR)
> +		sdhci_err_stats_inc(host, DAT_TIMEOUT);
> +	} else if (intmask & SDHCI_INT_ADMA_ERROR) {
>  		*data_error = -EIO;
> -	else
> +		sdhci_err_stats_inc(host, ADMA);
> +	} else
>  		*data_error = 0;
>  
>  	/* Clear selected interrupts. */
> @@ -3934,6 +3961,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>  		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
>  		pr_err("%s: CQE: Unexpected interrupt 0x%08x.\n",
>  		       mmc_hostname(host->mmc), intmask);
> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>  		sdhci_dumpregs(host);
>  	}
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d7929d7..95a08f0 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -356,6 +356,9 @@ struct sdhci_adma2_64_desc {
>   */
>  #define MMC_CMD_TRANSFER_TIME	(10 * NSEC_PER_MSEC) /* max 10 ms */
>  
> +#define sdhci_err_stats_inc(host, err_name) \
> +	mmc_debugfs_err_stats_inc((host)->mmc, MMC_ERR_##err_name)
> +
>  enum sdhci_cookie {
>  	COOKIE_UNMAPPED,
>  	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index d9a65c6..9c50bc4 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -99,6 +99,12 @@ static inline bool mmc_op_multi(u32 opcode)
>  	       opcode == MMC_READ_MULTIPLE_BLOCK;
>  }
>  
> +static inline bool mmc_op_tuning(u32 opcode)
> +{
> +	return opcode == MMC_SEND_TUNING_BLOCK ||
> +			opcode == MMC_SEND_TUNING_BLOCK_HS200;
> +}
> +
>  /*
>   * MMC_SWITCH argument format:
>   *

There does not seem to be any:

	sdhci_err_stats_inc(host, TUNING);

MMC_ERR_TUNING does not seem to get used.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
  2022-04-25 16:00 ` [PATCH V5 4/5] mmc: debugfs: Add debug fs error state " Shaik Sajida Bhanu
@ 2022-04-26  7:54   ` Adrian Hunter
  2022-04-26  7:58     ` Adrian Hunter
       [not found]     ` <5c2a9eae-3ab2-1fa4-c7df-ba7384e07b24@quicinc.com>
  0 siblings, 2 replies; 13+ messages in thread
From: Adrian Hunter @ 2022-04-26  7:54 UTC (permalink / raw)
  To: Shaik Sajida Bhanu, ulf.hansson, wsa+renesas,
	yoshihiro.shimoda.uh, linus.walleij, digetx, briannorris,
	quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
> Add debug fs entry error state to query eMMC and SD card errors statistics.
> If any errors occurred in eMMC and SD card driver level then
> err_state value will be set to 1.
> 
> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
> ---
>  drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 6aa5a60..2f5b63f 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>  
>  DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>  	"%llu\n");

A blank line would be nice here

> +static int mmc_err_state_get(void *data, u64 *val)
> +{
> +	struct mmc_host *host = data;
> +
> +	if (!host)
> +		return -EINVAL;
> +

I am not sure why you have left out some err_stats[].
Why not all of them?  At least, it needs a comment to explain.

> +	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
> +	       host->err_stats[MMC_ERR_ADMA] ||
> +	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
> +	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
> +	       host->err_stats[MMC_ERR_CMDQ_RED] ||
> +	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
> +	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
> +	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
> +	       host->err_stats[MMC_ERR_DAT_CRC] ||
> +	       host->err_stats[MMC_ERR_CMD_CRC] ||
> +	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>  
>  static int mmc_err_stats_show(struct seq_file *file, void *data)
>  {
> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>  	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>  				   &mmc_clock_fops);
>  
> +	debugfs_create_file("err_state", 0600, root, host,
> +			    &mmc_err_state);
>  	debugfs_create_file("err_stats", 0600, root, host,
>  			    &mmc_err_stats_fops);
>  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
  2022-04-26  7:54   ` Adrian Hunter
@ 2022-04-26  7:58     ` Adrian Hunter
  2022-05-09  9:44       ` Sajida Bhanu (Temp)
       [not found]     ` <5c2a9eae-3ab2-1fa4-c7df-ba7384e07b24@quicinc.com>
  1 sibling, 1 reply; 13+ messages in thread
From: Adrian Hunter @ 2022-04-26  7:58 UTC (permalink / raw)
  To: Shaik Sajida Bhanu, ulf.hansson, wsa+renesas,
	yoshihiro.shimoda.uh, linus.walleij, digetx, briannorris,
	quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

On 26/04/22 10:54, Adrian Hunter wrote:
> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>> If any errors occurred in eMMC and SD card driver level then
>> err_state value will be set to 1.
>>
>> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
>> ---
>>  drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>> index 6aa5a60..2f5b63f 100644
>> --- a/drivers/mmc/core/debugfs.c
>> +++ b/drivers/mmc/core/debugfs.c
>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>  
>>  DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>  	"%llu\n");
> 
> A blank line would be nice here
> 
>> +static int mmc_err_state_get(void *data, u64 *val)
>> +{
>> +	struct mmc_host *host = data;
>> +
>> +	if (!host)
>> +		return -EINVAL;
>> +
> 
> I am not sure why you have left out some err_stats[].
> Why not all of them?  At least, it needs a comment to explain.
> 
>> +	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>> +	       host->err_stats[MMC_ERR_ADMA] ||
>> +	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>> +	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>> +	       host->err_stats[MMC_ERR_CMDQ_RED] ||
>> +	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
>> +	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>> +	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>> +	       host->err_stats[MMC_ERR_DAT_CRC] ||
>> +	       host->err_stats[MMC_ERR_CMD_CRC] ||
>> +	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
>> +
>> +	return 0;
>> +}
>> +
>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");

Also, if possible, please use DEFINE_DEBUGFS_ATTRIBUTE / debugfs_create_file_unsafe
in this case

>>  
>>  static int mmc_err_stats_show(struct seq_file *file, void *data)
>>  {
>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>  	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>  				   &mmc_clock_fops);
>>  
>> +	debugfs_create_file("err_state", 0600, root, host,
>> +			    &mmc_err_state);
>>  	debugfs_create_file("err_stats", 0600, root, host,
>>  			    &mmc_err_stats_fops);
>>  
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 2/5] mmc: sdhci: Capture eMMC and SD card errors
  2022-04-26  7:51   ` Adrian Hunter
@ 2022-05-09  9:34     ` Sajida Bhanu (Temp)
  0 siblings, 0 replies; 13+ messages in thread
From: Sajida Bhanu (Temp) @ 2022-05-09  9:34 UTC (permalink / raw)
  To: Adrian Hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

Hi,

Thank you for the review.

Please find the review comments.

Thanks,

Sajida

On 4/26/2022 1:21 PM, Adrian Hunter wrote:
> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>> Add changes to capture eMMC and SD card errors.
>> This is useful for debug and testing.
>>
>> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
> Looks good.  A couple of minor comments.
Thank you
>
>> ---
>>   drivers/mmc/host/sdhci.c | 54 ++++++++++++++++++++++++++++++++++++------------
>>   drivers/mmc/host/sdhci.h |  3 +++
>>   include/linux/mmc/mmc.h  |  6 ++++++
>>   3 files changed, 50 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 2215202..1cda28ba 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -224,6 +224,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
>>   		if (timedout) {
>>   			pr_err("%s: Reset 0x%x never completed.\n",
>>   				mmc_hostname(host->mmc), (int)mask);
>> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>>   			sdhci_dumpregs(host);
>>   			return;
>>   		}
>> @@ -1716,6 +1717,7 @@ static bool sdhci_send_command_retry(struct sdhci_host *host,
>>   		if (!timeout--) {
>>   			pr_err("%s: Controller never released inhibit bit(s).\n",
>>   			       mmc_hostname(host->mmc));
>> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>>   			sdhci_dumpregs(host);
>>   			cmd->error = -EIO;
>>   			return false;
>> @@ -1965,6 +1967,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>>   		if (timedout) {
>>   			pr_err("%s: Internal clock never stabilised.\n",
>>   			       mmc_hostname(host->mmc));
>> +			sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>>   			sdhci_dumpregs(host);
>>   			return;
>>   		}
>> @@ -1987,6 +1990,7 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>>   			if (timedout) {
>>   				pr_err("%s: PLL clock never stabilised.\n",
>>   				       mmc_hostname(host->mmc));
>> +				sdhci_err_stats_inc(host, CTRL_TIMEOUT);
>>   				sdhci_dumpregs(host);
>>   				return;
>>   			}
>> @@ -3161,6 +3165,7 @@ static void sdhci_timeout_timer(struct timer_list *t)
>>   	if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
>>   		pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
>>   		       mmc_hostname(host->mmc));
>> +		sdhci_err_stats_inc(host, REQ_TIMEOUT);
>>   		sdhci_dumpregs(host);
>>   
>>   		host->cmd->error = -ETIMEDOUT;
>> @@ -3183,6 +3188,7 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
>>   	    (host->cmd && sdhci_data_line_cmd(host->cmd))) {
>>   		pr_err("%s: Timeout waiting for hardware interrupt.\n",
>>   		       mmc_hostname(host->mmc));
>> +		sdhci_err_stats_inc(host, REQ_TIMEOUT);
>>   		sdhci_dumpregs(host);
>>   
>>   		if (host->data) {
>> @@ -3234,17 +3240,21 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
>>   			return;
>>   		pr_err("%s: Got command interrupt 0x%08x even though no command operation was in progress.\n",
>>   		       mmc_hostname(host->mmc), (unsigned)intmask);
>> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>>   		sdhci_dumpregs(host);
>>   		return;
>>   	}
>>   
>>   	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
>>   		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
>> -		if (intmask & SDHCI_INT_TIMEOUT)
>> +		if (intmask & SDHCI_INT_TIMEOUT) {
>>   			host->cmd->error = -ETIMEDOUT;
>> -		else
>> +			sdhci_err_stats_inc(host, CMD_TIMEOUT);
>> +		} else {
>>   			host->cmd->error = -EILSEQ;
>> -
>> +			if (!mmc_op_tuning(host->cmd->opcode))
>> +				sdhci_err_stats_inc(host, CMD_CRC);
>> +		}
>>   		/* Treat data command CRC error the same as data CRC error */
>>   		if (host->cmd->data &&
>>   		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
>> @@ -3266,6 +3276,8 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
>>   			  -ETIMEDOUT :
>>   			  -EILSEQ;
>>   
>> +		sdhci_err_stats_inc(host, AUTO_CMD);
>> +
>>   		if (sdhci_auto_cmd23(host, mrq)) {
>>   			mrq->sbc->error = err;
>>   			__sdhci_finish_mrq(host, mrq);
>> @@ -3342,6 +3354,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>>   			if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>>   				host->data_cmd = NULL;
>>   				data_cmd->error = -ETIMEDOUT;
>> +				sdhci_err_stats_inc(host, CMD_TIMEOUT);
>>   				__sdhci_finish_mrq(host, data_cmd->mrq);
>>   				return;
>>   			}
>> @@ -3370,23 +3383,29 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>>   
>>   		pr_err("%s: Got data interrupt 0x%08x even though no data operation was in progress.\n",
>>   		       mmc_hostname(host->mmc), (unsigned)intmask);
>> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>>   		sdhci_dumpregs(host);
>>   
>>   		return;
>>   	}
>>   
>> -	if (intmask & SDHCI_INT_DATA_TIMEOUT)
>> +	if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>>   		host->data->error = -ETIMEDOUT;
>> -	else if (intmask & SDHCI_INT_DATA_END_BIT)
>> +		sdhci_err_stats_inc(host, DAT_TIMEOUT);
>> +	} else if (intmask & SDHCI_INT_DATA_END_BIT)
>>   		host->data->error = -EILSEQ;
> Seems to be missing here:
>
> 		sdhci_err_stats_inc(host, DAT_CRC);
>
> Also it would be nice to have braces {} on all arms of if-else-if
> Can use checkpatch.pl --strict to see where
Sure Thank you
>
>>   	else if ((intmask & SDHCI_INT_DATA_CRC) &&
>>   		SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
>> -			!= MMC_BUS_TEST_R)
>> +			!= MMC_BUS_TEST_R) {
>>   		host->data->error = -EILSEQ;
>> +		if (!mmc_op_tuning(host->cmd->opcode))
>> +			sdhci_err_stats_inc(host, DAT_CRC);
>> +	}
>>   	else if (intmask & SDHCI_INT_ADMA_ERROR) {
>>   		pr_err("%s: ADMA error: 0x%08x\n", mmc_hostname(host->mmc),
>>   		       intmask);
>>   		sdhci_adma_show_error(host);
>> +		sdhci_err_stats_inc(host, ADMA);
>>   		host->data->error = -EIO;
>>   		if (host->ops->adma_workaround)
>>   			host->ops->adma_workaround(host, intmask);
>> @@ -3584,6 +3603,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>   	if (unexpected) {
>>   		pr_err("%s: Unexpected interrupt 0x%08x.\n",
>>   			   mmc_hostname(host->mmc), unexpected);
>> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>>   		sdhci_dumpregs(host);
>>   	}
>>   
>> @@ -3905,20 +3925,27 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>>   	if (!host->cqe_on)
>>   		return false;
>>   
>> -	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC))
>> +	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
>>   		*cmd_error = -EILSEQ;
>> -	else if (intmask & SDHCI_INT_TIMEOUT)
>> +		if (!mmc_op_tuning(host->cmd->opcode))
>> +			sdhci_err_stats_inc(host, CMD_CRC);
>> +	} else if (intmask & SDHCI_INT_TIMEOUT) {
>>   		*cmd_error = -ETIMEDOUT;
>> -	else
>> +		sdhci_err_stats_inc(host, CMD_TIMEOUT);
>> +	} else
>>   		*cmd_error = 0;
>>   
>> -	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
>> +	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
>>   		*data_error = -EILSEQ;
>> -	else if (intmask & SDHCI_INT_DATA_TIMEOUT)
>> +		if (!mmc_op_tuning(host->cmd->opcode))
>> +			sdhci_err_stats_inc(host, DAT_CRC);
>> +	} else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>>   		*data_error = -ETIMEDOUT;
>> -	else if (intmask & SDHCI_INT_ADMA_ERROR)
>> +		sdhci_err_stats_inc(host, DAT_TIMEOUT);
>> +	} else if (intmask & SDHCI_INT_ADMA_ERROR) {
>>   		*data_error = -EIO;
>> -	else
>> +		sdhci_err_stats_inc(host, ADMA);
>> +	} else
>>   		*data_error = 0;
>>   
>>   	/* Clear selected interrupts. */
>> @@ -3934,6 +3961,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>>   		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
>>   		pr_err("%s: CQE: Unexpected interrupt 0x%08x.\n",
>>   		       mmc_hostname(host->mmc), intmask);
>> +		sdhci_err_stats_inc(host, UNEXPECTED_IRQ);
>>   		sdhci_dumpregs(host);
>>   	}
>>   
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index d7929d7..95a08f0 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -356,6 +356,9 @@ struct sdhci_adma2_64_desc {
>>    */
>>   #define MMC_CMD_TRANSFER_TIME	(10 * NSEC_PER_MSEC) /* max 10 ms */
>>   
>> +#define sdhci_err_stats_inc(host, err_name) \
>> +	mmc_debugfs_err_stats_inc((host)->mmc, MMC_ERR_##err_name)
>> +
>>   enum sdhci_cookie {
>>   	COOKIE_UNMAPPED,
>>   	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index d9a65c6..9c50bc4 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -99,6 +99,12 @@ static inline bool mmc_op_multi(u32 opcode)
>>   	       opcode == MMC_READ_MULTIPLE_BLOCK;
>>   }
>>   
>> +static inline bool mmc_op_tuning(u32 opcode)
>> +{
>> +	return opcode == MMC_SEND_TUNING_BLOCK ||
>> +			opcode == MMC_SEND_TUNING_BLOCK_HS200;
>> +}
>> +
>>   /*
>>    * MMC_SWITCH argument format:
>>    *
> There does not seem to be any:
>
> 	sdhci_err_stats_inc(host, TUNING);
>
> MMC_ERR_TUNING does not seem to get used.

Yes... Thank you for pointing.

We need to update this, will update this in patch set.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
  2022-04-26  7:58     ` Adrian Hunter
@ 2022-05-09  9:44       ` Sajida Bhanu (Temp)
  0 siblings, 0 replies; 13+ messages in thread
From: Sajida Bhanu (Temp) @ 2022-05-09  9:44 UTC (permalink / raw)
  To: Adrian Hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

Hi,

Thank you for review.

Please find the inline comments.

Thanks,

Sajida

On 4/26/2022 1:28 PM, Adrian Hunter wrote:
> On 26/04/22 10:54, Adrian Hunter wrote:
>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>> If any errors occurred in eMMC and SD card driver level then
>>> err_state value will be set to 1.
>>>
>>> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
>>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>>> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
>>> ---
>>>   drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>> index 6aa5a60..2f5b63f 100644
>>> --- a/drivers/mmc/core/debugfs.c
>>> +++ b/drivers/mmc/core/debugfs.c
>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>   
>>>   DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>>   	"%llu\n");
>> A blank line would be nice here
>>
>>> +static int mmc_err_state_get(void *data, u64 *val)
>>> +{
>>> +	struct mmc_host *host = data;
>>> +
>>> +	if (!host)
>>> +		return -EINVAL;
>>> +
>> I am not sure why you have left out some err_stats[].
>> Why not all of them?  At least, it needs a comment to explain.
>>
>>> +	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_ADMA] ||
>>> +	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_RED] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>> +	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_DAT_CRC] ||
>>> +	       host->err_stats[MMC_ERR_CMD_CRC] ||
>>> +	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
> Also, if possible, please use DEFINE_DEBUGFS_ATTRIBUTE / debugfs_create_file_unsafe
> in this case
Sure.. will use  DEFINE_DEBUGFS_ATTRIBUTE
>
>>>   
>>>   static int mmc_err_stats_show(struct seq_file *file, void *data)
>>>   {
>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>>   	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>>   				   &mmc_clock_fops);
>>>   
>>> +	debugfs_create_file("err_state", 0600, root, host,
>>> +			    &mmc_err_state);
>>>   	debugfs_create_file("err_stats", 0600, root, host,
>>>   			    &mmc_err_stats_fops);
>>>   

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
       [not found]     ` <5c2a9eae-3ab2-1fa4-c7df-ba7384e07b24@quicinc.com>
@ 2022-05-09 10:03       ` Adrian Hunter
  2022-05-09 10:32         ` Sajida Bhanu (Temp)
  0 siblings, 1 reply; 13+ messages in thread
From: Adrian Hunter @ 2022-05-09 10:03 UTC (permalink / raw)
  To: Sajida Bhanu (Temp),
	ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh, linus.walleij,
	digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen

On 9/05/22 12:42, Sajida Bhanu (Temp) wrote:
> Hi,
> 
> Thank you for the review.
> 
> Please find the inline comments.
> 
> Thanks,
> 
> Sajida
> 
> On 4/26/2022 1:24 PM, Adrian Hunter wrote:
> 
>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>> If any errors occurred in eMMC and SD card driver level then
>>> err_state value will be set to 1.
>>>
>>> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
>>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>>> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
>>> ---
>>>  drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>> index 6aa5a60..2f5b63f 100644
>>> --- a/drivers/mmc/core/debugfs.c
>>> +++ b/drivers/mmc/core/debugfs.c
>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>  
>>>  DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>>  	"%llu\n");
>> A blank line would be nice here
> Sure
>>> +static int mmc_err_state_get(void *data, u64 *val)
>>> +{
>>> +	struct mmc_host *host = data;
>>> +
>>> +	if (!host)
>>> +		return -EINVAL;
>>> +
>> I am not sure why you have left out some err_stats[].
>> Why not all of them?  At least, it needs a comment to explain.
> 
> MMC_ERR_ICE_CFG --> we don't have ICE config.

So err_stats[MMC_ERR_ICE_CFG]  would be zero and make
no difference.

If you are going to check all then you could loop
through them

	*val = 0;
	for (i = 0; i < MMC_ERR_MAX; i++) {
		if (host->err_stats[i]) {
			*val = 1;
			break;
		}
	}

> 
> Remaining we need to update, Thank you for pointing.
> 
>>> +	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_ADMA] ||
>>> +	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_RED] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>> +	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>> +	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>> +	       host->err_stats[MMC_ERR_DAT_CRC] ||
>>> +	       host->err_stats[MMC_ERR_CMD_CRC] ||
>>> +	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>>>  
>>>  static int mmc_err_stats_show(struct seq_file *file, void *data)
>>>  {
>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>>  	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>>  				   &mmc_clock_fops);
>>>  
>>> +	debugfs_create_file("err_state", 0600, root, host,
>>> +			    &mmc_err_state);
>>>  	debugfs_create_file("err_stats", 0600, root, host,
>>>  			    &mmc_err_stats_fops);
>>>  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver
  2022-05-09 10:03       ` Adrian Hunter
@ 2022-05-09 10:32         ` Sajida Bhanu (Temp)
  0 siblings, 0 replies; 13+ messages in thread
From: Sajida Bhanu (Temp) @ 2022-05-09 10:32 UTC (permalink / raw)
  To: Adrian Hunter, ulf.hansson, wsa+renesas, yoshihiro.shimoda.uh,
	linus.walleij, digetx, briannorris, quic_riteshh
  Cc: linux-mmc, linux-kernel, quic_asutoshd, quic_rampraka,
	quic_pragalla, quic_sartgarg, quic_nitirawa, quic_sayalil,
	Liangliang Lu, Bao D . Nguyen


On 5/9/2022 3:33 PM, Adrian Hunter wrote:
> On 9/05/22 12:42, Sajida Bhanu (Temp) wrote:
>> Hi,
>>
>> Thank you for the review.
>>
>> Please find the inline comments.
>>
>> Thanks,
>>
>> Sajida
>>
>> On 4/26/2022 1:24 PM, Adrian Hunter wrote:
>>
>>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>>> If any errors occurred in eMMC and SD card driver level then
>>>> err_state value will be set to 1.
>>>>
>>>> Signed-off-by: Liangliang Lu <quic_luliang@quicinc.com>
>>>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>>>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>>>> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
>>>> ---
>>>>   drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>>>   1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>>> index 6aa5a60..2f5b63f 100644
>>>> --- a/drivers/mmc/core/debugfs.c
>>>> +++ b/drivers/mmc/core/debugfs.c
>>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>>   
>>>>   DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>>>   	"%llu\n");
>>> A blank line would be nice here
>> Sure
>>>> +static int mmc_err_state_get(void *data, u64 *val)
>>>> +{
>>>> +	struct mmc_host *host = data;
>>>> +
>>>> +	if (!host)
>>>> +		return -EINVAL;
>>>> +
>>> I am not sure why you have left out some err_stats[].
>>> Why not all of them?  At least, it needs a comment to explain.
>> MMC_ERR_ICE_CFG --> we don't have ICE config.
> So err_stats[MMC_ERR_ICE_CFG]  would be zero and make
> no difference.
>
> If you are going to check all then you could loop
> through them
>
> 	*val = 0;
> 	for (i = 0; i < MMC_ERR_MAX; i++) {
> 		if (host->err_stats[i]) {
> 			*val = 1;
> 			break;
> 		}
> 	}
Sure Thanks
>> Remaining we need to update, Thank you for pointing.
>>
>>>> +	*val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>>> +	       host->err_stats[MMC_ERR_ADMA] ||
>>>> +	       host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>>> +	       host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>>> +	       host->err_stats[MMC_ERR_CMDQ_RED] ||
>>>> +	       host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>>> +	       host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>>> +	       host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>>> +	       host->err_stats[MMC_ERR_DAT_CRC] ||
>>>> +	       host->err_stats[MMC_ERR_CMD_CRC] ||
>>>> +	       host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>>>>   
>>>>   static int mmc_err_stats_show(struct seq_file *file, void *data)
>>>>   {
>>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>>>   	debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>>>   				   &mmc_clock_fops);
>>>>   
>>>> +	debugfs_create_file("err_state", 0600, root, host,
>>>> +			    &mmc_err_state);
>>>>   	debugfs_create_file("err_stats", 0600, root, host,
>>>>   			    &mmc_err_stats_fops);
>>>>   

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-05-09 10:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-25 16:00 [PATCH V5 0/5] mmc: add error statistics for eMMC and SD card Shaik Sajida Bhanu
2022-04-25 16:00 ` [PATCH V5 1/5] mmc: core: Capture eMMC and SD card errors Shaik Sajida Bhanu
2022-04-25 16:00 ` [PATCH V5 2/5] mmc: sdhci: " Shaik Sajida Bhanu
2022-04-26  7:51   ` Adrian Hunter
2022-05-09  9:34     ` Sajida Bhanu (Temp)
2022-04-25 16:00 ` [PATCH V5 3/5] mmc: debugfs: Add debug fs entry for mmc driver Shaik Sajida Bhanu
2022-04-25 16:00 ` [PATCH V5 4/5] mmc: debugfs: Add debug fs error state " Shaik Sajida Bhanu
2022-04-26  7:54   ` Adrian Hunter
2022-04-26  7:58     ` Adrian Hunter
2022-05-09  9:44       ` Sajida Bhanu (Temp)
     [not found]     ` <5c2a9eae-3ab2-1fa4-c7df-ba7384e07b24@quicinc.com>
2022-05-09 10:03       ` Adrian Hunter
2022-05-09 10:32         ` Sajida Bhanu (Temp)
2022-04-25 16:00 ` [PATCH V5 5/5] mmc: cqhci: Capture eMMC and SD card errors Shaik Sajida Bhanu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.