[PATCH V3 0/3] scsi: ufs-qcom: Enable Dumping of Hibern8, MCQ, and Testbus Registers

[PATCH V3 0/3] scsi: ufs-qcom: Enable Dumping of Hibern8, MCQ, and Testbus Registers

[Manish Pandey]

Submitting a series of patches aimed at enhancing the debugging and monitoring capabilities
of the UFS-QCOM driver. These patches introduce new functionalities that will significantly
aid in diagnosing and resolving issues related to hardware and software operations.

---
Changes in v3:
- Addressed Bart's comments.
Changes in v2:
- Rebased patchsets.
- Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/

---
Manish Pandey (3):
  scsi: ufs-qcom: Add support for dumping HW and SW hibern8 count
  scsi: ufs-qcom: Add support for dumping MCQ registers
  scsi: ufs-qcom: Add support for testbus registers

 drivers/ufs/host/ufs-qcom.c | 129 ++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-qcom.h |  11 +++
 2 files changed, 140 insertions(+)

-- 
2.17.1

[PATCH V3 1/3] scsi: ufs-qcom: Add support for dumping HW and SW hibern8 count

[Manish Pandey]

This patch adds functionality to dump both hardware and software
hibern8 enter counts. This enhancement will aid in monitoring and
debugging hibern8 state transitions by providing detailed count
information.

Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 9 +++++++++
 drivers/ufs/host/ufs-qcom.h | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1b37449fbffc..f5181773c0e5 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1573,6 +1573,15 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 
 	host = ufshcd_get_variant(hba);
 
+	dev_err(hba->dev, "HW_H8_ENTER_CNT=%d\n", ufshcd_readl(hba, REG_UFS_HW_H8_ENTER_CNT));
+	dev_err(hba->dev, "HW_H8_EXIT_CNT=%d\n", ufshcd_readl(hba, REG_UFS_HW_H8_EXIT_CNT));
+
+	dev_err(hba->dev, "SW_H8_ENTER_CNT=%d\n", ufshcd_readl(hba, REG_UFS_SW_H8_ENTER_CNT));
+	dev_err(hba->dev, "SW_H8_EXIT_CNT=%d\n", ufshcd_readl(hba, REG_UFS_SW_H8_EXIT_CNT));
+
+	dev_err(hba->dev, "SW_AFTER_HW_H8_ENTER_CNT=%d\n",
+			ufshcd_readl(hba, REG_UFS_SW_AFTER_HW_H8_ENTER_CNT));
+
 	ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4,
 			 "HCI Vendor Specific Registers ");
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index d0e6ec9128e7..a41db017009f 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -75,6 +75,15 @@ enum {
 	UFS_UFS_DBG_RD_EDTL_RAM			= 0x1900,
 };
 
+/* Vendor-specific Hibern8 count registers for the QCOM UFS host controller. */
+enum {
+	REG_UFS_HW_H8_ENTER_CNT			= 0x2700,
+	REG_UFS_SW_H8_ENTER_CNT			= 0x2704,
+	REG_UFS_SW_AFTER_HW_H8_ENTER_CNT	= 0x2708,
+	REG_UFS_HW_H8_EXIT_CNT			= 0x270C,
+	REG_UFS_SW_H8_EXIT_CNT			= 0x2710,
+};
+
 enum {
 	UFS_MEM_CQIS_VS		= 0x8,
 };
-- 
2.17.1

[PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[Manish Pandey]

This patch adds functionality to dump MCQ registers.
This will help in diagnosing issues related to MCQ
operations by providing detailed register dumps.

Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
---

Changes in v3:
- Addressed Bart's review comments by adding explanations for the
  in_task() and usleep_range() calls.
Changes in v2:
- Rebased patchsets.
- Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
---
 drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-qcom.h |  2 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index f5181773c0e5..fb9da04c0d35 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
 	return 0;
 }
 
+static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
+{
+	/* sleep intermittently to prevent CPU hog during data dumps. */
+	/* RES_MCQ_1 */
+	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_2 */
+	ufshcd_dump_regs(hba, 0x400, 256 * 4, "MCQ HCI 1da0400-1da07f0 ");
+	usleep_range(1000, 1100);
+
+	/*RES_MCQ_VS */
+	ufshcd_dump_regs(hba, 0x0, 5 * 4, "MCQ VS 1da4000-1da4010 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_1 */
+	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ SQD 1da5000-1da53f0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_2 */
+	ufshcd_dump_regs(hba, 0x400, 256 * 4, "MCQ SQD 1da5400-1da57f0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_3 */
+	ufshcd_dump_regs(hba, 0x800, 256 * 4, "MCQ SQD 1da5800-1da5bf0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_4 */
+	ufshcd_dump_regs(hba, 0xc00, 256 * 4, "MCQ SQD 1da5c00-1da5ff0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_5 */
+	ufshcd_dump_regs(hba, 0x1000, 256 * 4, "MCQ SQD 1da6000-1da63f0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_6 */
+	ufshcd_dump_regs(hba, 0x1400, 256 * 4, "MCQ SQD 1da6400-1da67f0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_7 */
+	ufshcd_dump_regs(hba, 0x1800, 256 * 4, "MCQ SQD 1da6800-1da6bf0 ");
+	usleep_range(1000, 1100);
+
+	/* RES_MCQ_SQD_8 */
+	ufshcd_dump_regs(hba, 0x1c00, 256 * 4, "MCQ SQD 1da6c00-1da6ff0 ");
+	usleep_range(1000, 1100);
+}
+
 static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 {
 	u32 reg;
@@ -1624,6 +1672,18 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 
 	reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TMRLUT);
 	ufshcd_dump_regs(hba, reg, 9 * 4, "UFS_DBG_RD_REG_TMRLUT ");
+
+	if (hba->mcq_enabled) {
+		reg = ufs_qcom_get_debug_reg_offset(host, UFS_RD_REG_MCQ);
+		ufshcd_dump_regs(hba, reg, 64 * 4, "HCI MCQ Debug Registers ");
+	}
+
+	/* ensure below dumps occur only in task context due to blocking calls. */
+	if (in_task()) {
+		/* Dump MCQ Host Vendor Specific Registers */
+		if (hba->mcq_enabled)
+			ufs_qcom_dump_mcq_hci_regs(hba);
+	}
 }
 
 /**
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index a41db017009f..03a3fee56041 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -50,6 +50,8 @@ enum {
 	 */
 	UFS_AH8_CFG				= 0xFC,
 
+	UFS_RD_REG_MCQ                          = 0xD00,
+
 	REG_UFS_MEM_ICE_CONFIG			= 0x260C,
 	REG_UFS_MEM_ICE_NUM_CORE		= 0x2664,
 
-- 
2.17.1

[PATCH V3 3/3] scsi: ufs-qcom: Add support for testbus registers

[Manish Pandey]

This patch introduces support for dumping testbus registers,
enhancing the debugging capabilities for UFS-QCOM drivers.

Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
---
Changes in v3:
- Annotated the 'testbus' declaration with __free.
- Converted the switch-statements into an array lookup.
- Introduced struct testbus_info{} for handling testbus switch-statements to an array lookup.
Changes in v2:
- Rebased patchsets.
- Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/

---
 drivers/ufs/host/ufs-qcom.c | 53 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index fb9da04c0d35..c32b1268d299 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -17,6 +17,7 @@
 #include <linux/time.h>
 #include <linux/unaligned.h>
 #include <linux/units.h>
+#include <linux/cleanup.h>
 
 #include <soc/qcom/ice.h>
 
@@ -98,6 +99,24 @@ static const struct __ufs_qcom_bw_table {
 	[MODE_MAX][0][0]		    = { 7643136,	819200 },
 };
 
+static const struct {
+	int nminor;
+	char *prefix;
+} testbus_info[TSTBUS_MAX] = {
+	[TSTBUS_UAWM]     = {32, "TSTBUS_UAWM "},
+	[TSTBUS_UARM]     = {32, "TSTBUS_UARM "},
+	[TSTBUS_TXUC]     = {32, "TSTBUS_TXUC "},
+	[TSTBUS_RXUC]     = {32, "TSTBUS_RXUC "},
+	[TSTBUS_DFC]      = {32, "TSTBUS_DFC "},
+	[TSTBUS_TRLUT]    = {32, "TSTBUS_TRLUT "},
+	[TSTBUS_TMRLUT]   = {32, "TSTBUS_TMRLUT "},
+	[TSTBUS_OCSC]     = {32, "TSTBUS_OCSC "},
+	[TSTBUS_UTP_HCI]  = {32, "TSTBUS_UTP_HCI "},
+	[TSTBUS_COMBINED] = {32, "TSTBUS_COMBINED "},
+	[TSTBUS_WRAPPER]  = {32, "TSTBUS_WRAPPER "},
+	[TSTBUS_UNIPRO]   = {256, "TSTBUS_UNIPRO "}
+};
+
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
 
@@ -1566,6 +1585,33 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
 	return 0;
 }
 
+static void ufs_qcom_dump_testbus(struct ufs_hba *hba)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	int i, j, nminor = 0, testbus_len = 0;
+	u32 *testbus __free(kfree) = NULL;
+	char *prefix;
+
+	testbus = kmalloc(256 * sizeof(u32), GFP_KERNEL);
+	if (!testbus)
+		return;
+
+	for (j = 0; j < TSTBUS_MAX; j++) {
+		nminor = testbus_info[j].nminor;
+		prefix = testbus_info[j].prefix;
+		host->testbus.select_major = j;
+		testbus_len = nminor * sizeof(u32);
+		for (i = 0; i < nminor; i++) {
+			host->testbus.select_minor = i;
+			ufs_qcom_testbus_config(host);
+			testbus[i] = ufshcd_readl(hba, UFS_TEST_BUS);
+			usleep_range(100, 200);
+		}
+		print_hex_dump(KERN_ERR, prefix, DUMP_PREFIX_OFFSET,
+				16, 4, testbus, testbus_len, false);
+	}
+}
+
 static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
 {
 	/* sleep intermittently to prevent CPU hog during data dumps. */
@@ -1680,9 +1726,14 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 
 	/* ensure below dumps occur only in task context due to blocking calls. */
 	if (in_task()) {
-		/* Dump MCQ Host Vendor Specific Registers */
+		/* dump MCQ Host Vendor Specific Registers */
 		if (hba->mcq_enabled)
 			ufs_qcom_dump_mcq_hci_regs(hba);
+
+		/* sleep a bit intermittently as we are dumping too much data */
+		ufshcd_dump_regs(hba, UFS_TEST_BUS, 4, "UFS_TEST_BUS ");
+		usleep_range(1000, 1100);
+		ufs_qcom_dump_testbus(hba);
 	}
 }
 
-- 
2.17.1

Re: [PATCH V3 1/3] scsi: ufs-qcom: Add support for dumping HW and SW hibern8 count

[Manivannan Sadhasivam]

On Thu, Mar 13, 2025 at 10:46:33AM +0530, Manish Pandey wrote:
> This patch adds functionality to dump both hardware and software

No 'patch' in description. Once the patch is merged, it will become as 'commit'.

Also, please use imperative mood to describe the change. I don't know why this
is not followed since I believe, I've given this comment before also (if you
didn't read the process documentation).

> hibern8 enter counts. This enhancement will aid in monitoring and
> debugging hibern8 state transitions by providing detailed count
> information.
> 
> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 9 +++++++++
>  drivers/ufs/host/ufs-qcom.h | 9 +++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1b37449fbffc..f5181773c0e5 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1573,6 +1573,15 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
>  
>  	host = ufshcd_get_variant(hba);
>  
> +	dev_err(hba->dev, "HW_H8_ENTER_CNT=%d\n", ufshcd_readl(hba, REG_UFS_HW_H8_ENTER_CNT));
> +	dev_err(hba->dev, "HW_H8_EXIT_CNT=%d\n", ufshcd_readl(hba, REG_UFS_HW_H8_EXIT_CNT));
> +
> +	dev_err(hba->dev, "SW_H8_ENTER_CNT=%d\n", ufshcd_readl(hba, REG_UFS_SW_H8_ENTER_CNT));
> +	dev_err(hba->dev, "SW_H8_EXIT_CNT=%d\n", ufshcd_readl(hba, REG_UFS_SW_H8_EXIT_CNT));
> +
> +	dev_err(hba->dev, "SW_AFTER_HW_H8_ENTER_CNT=%d\n",
> +			ufshcd_readl(hba, REG_UFS_SW_AFTER_HW_H8_ENTER_CNT));
> +
>  	ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4,
>  			 "HCI Vendor Specific Registers ");
>  
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index d0e6ec9128e7..a41db017009f 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -75,6 +75,15 @@ enum {
>  	UFS_UFS_DBG_RD_EDTL_RAM			= 0x1900,
>  };
>  
> +/* Vendor-specific Hibern8 count registers for the QCOM UFS host controller. */

Get rid of 'for the QCOM UFS host controller'.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[Manivannan Sadhasivam]

On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
> This patch adds functionality to dump MCQ registers.
> This will help in diagnosing issues related to MCQ
> operations by providing detailed register dumps.
> 

Same comment as previous patch. Also, make use of 75 column width.

> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
> ---
> 
> Changes in v3:
> - Addressed Bart's review comments by adding explanations for the
>   in_task() and usleep_range() calls.
> Changes in v2:
> - Rebased patchsets.
> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
> ---
>  drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
>  drivers/ufs/host/ufs-qcom.h |  2 ++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index f5181773c0e5..fb9da04c0d35 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
>  	return 0;
>  }
>  
> +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
> +{
> +	/* sleep intermittently to prevent CPU hog during data dumps. */
> +	/* RES_MCQ_1 */
> +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
> +	usleep_range(1000, 1100);

If your motivation is just to not hog the CPU, use cond_resched().

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V3 3/3] scsi: ufs-qcom: Add support for testbus registers

[Manivannan Sadhasivam]

On Thu, Mar 13, 2025 at 10:46:35AM +0530, Manish Pandey wrote:
> This patch introduces support for dumping testbus registers,
> enhancing the debugging capabilities for UFS-QCOM drivers.
> 

Same comment as patch 1.

> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
> ---
> Changes in v3:
> - Annotated the 'testbus' declaration with __free.
> - Converted the switch-statements into an array lookup.
> - Introduced struct testbus_info{} for handling testbus switch-statements to an array lookup.
> Changes in v2:
> - Rebased patchsets.
> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
> 
> ---
>  drivers/ufs/host/ufs-qcom.c | 53 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index fb9da04c0d35..c32b1268d299 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/time.h>
>  #include <linux/unaligned.h>
>  #include <linux/units.h>
> +#include <linux/cleanup.h>

Sort includes alphabetically.

>  
>  #include <soc/qcom/ice.h>
>  
> @@ -98,6 +99,24 @@ static const struct __ufs_qcom_bw_table {
>  	[MODE_MAX][0][0]		    = { 7643136,	819200 },
>  };
>  
> +static const struct {
> +	int nminor;
> +	char *prefix;
> +} testbus_info[TSTBUS_MAX] = {
> +	[TSTBUS_UAWM]     = {32, "TSTBUS_UAWM "},
> +	[TSTBUS_UARM]     = {32, "TSTBUS_UARM "},
> +	[TSTBUS_TXUC]     = {32, "TSTBUS_TXUC "},
> +	[TSTBUS_RXUC]     = {32, "TSTBUS_RXUC "},
> +	[TSTBUS_DFC]      = {32, "TSTBUS_DFC "},
> +	[TSTBUS_TRLUT]    = {32, "TSTBUS_TRLUT "},
> +	[TSTBUS_TMRLUT]   = {32, "TSTBUS_TMRLUT "},
> +	[TSTBUS_OCSC]     = {32, "TSTBUS_OCSC "},
> +	[TSTBUS_UTP_HCI]  = {32, "TSTBUS_UTP_HCI "},
> +	[TSTBUS_COMBINED] = {32, "TSTBUS_COMBINED "},
> +	[TSTBUS_WRAPPER]  = {32, "TSTBUS_WRAPPER "},
> +	[TSTBUS_UNIPRO]   = {256, "TSTBUS_UNIPRO "}
> +};
> +
>  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
>  
> @@ -1566,6 +1585,33 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
>  	return 0;
>  }
>  
> +static void ufs_qcom_dump_testbus(struct ufs_hba *hba)
> +{
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	int i, j, nminor = 0, testbus_len = 0;
> +	u32 *testbus __free(kfree) = NULL;
> +	char *prefix;
> +
> +	testbus = kmalloc(256 * sizeof(u32), GFP_KERNEL);

Use kmalloc_array().

> +	if (!testbus)
> +		return;
> +
> +	for (j = 0; j < TSTBUS_MAX; j++) {
> +		nminor = testbus_info[j].nminor;
> +		prefix = testbus_info[j].prefix;
> +		host->testbus.select_major = j;
> +		testbus_len = nminor * sizeof(u32);
> +		for (i = 0; i < nminor; i++) {
> +			host->testbus.select_minor = i;
> +			ufs_qcom_testbus_config(host);
> +			testbus[i] = ufshcd_readl(hba, UFS_TEST_BUS);
> +			usleep_range(100, 200);

Why this delay is required?

> +		}
> +		print_hex_dump(KERN_ERR, prefix, DUMP_PREFIX_OFFSET,
> +				16, 4, testbus, testbus_len, false);
> +	}
> +}
> +
>  static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
>  {
>  	/* sleep intermittently to prevent CPU hog during data dumps. */
> @@ -1680,9 +1726,14 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
>  
>  	/* ensure below dumps occur only in task context due to blocking calls. */
>  	if (in_task()) {
> -		/* Dump MCQ Host Vendor Specific Registers */
> +		/* dump MCQ Host Vendor Specific Registers */

Spurious change.

>  		if (hba->mcq_enabled)
>  			ufs_qcom_dump_mcq_hci_regs(hba);
> +
> +		/* sleep a bit intermittently as we are dumping too much data */
> +		ufshcd_dump_regs(hba, UFS_TEST_BUS, 4, "UFS_TEST_BUS ");
> +		usleep_range(1000, 1100);

Same comment as previous patch. Use cond_resched().

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[MANISH PANDEY]

On 3/18/2025 12:14 PM, Manivannan Sadhasivam wrote:
> On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
>> This patch adds functionality to dump MCQ registers.
>> This will help in diagnosing issues related to MCQ
>> operations by providing detailed register dumps.
>>
> 
> Same comment as previous patch. Also, make use of 75 column width.
> 
will Update in next patch set.>> Signed-off-by: Manish Pandey 
<quic_mapa@quicinc.com>
>> ---
>>
>> Changes in v3:
>> - Addressed Bart's review comments by adding explanations for the
>>    in_task() and usleep_range() calls.
>> Changes in v2:
>> - Rebased patchsets.
>> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
>>   drivers/ufs/host/ufs-qcom.h |  2 ++
>>   2 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index f5181773c0e5..fb9da04c0d35 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
>>   	return 0;
>>   }
>>   
>> +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
>> +{
>> +	/* sleep intermittently to prevent CPU hog during data dumps. */
>> +	/* RES_MCQ_1 */
>> +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
>> +	usleep_range(1000, 1100);
> 
> If your motivation is just to not hog the CPU, use cond_resched().
> 
> - Mani
> 
The intention here is to introduce a specific delay between each dump. 
Therefore, i would like to use usleep_range() instead of cond_resched(). 
Please let me know if i am getting it wrong..

Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[Manivannan Sadhasivam]

On Wed, Mar 19, 2025 at 11:51:07AM +0530, MANISH PANDEY wrote:
> 
> 
> On 3/18/2025 12:14 PM, Manivannan Sadhasivam wrote:
> > On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
> > > This patch adds functionality to dump MCQ registers.
> > > This will help in diagnosing issues related to MCQ
> > > operations by providing detailed register dumps.
> > > 
> > 
> > Same comment as previous patch. Also, make use of 75 column width.
> > 
> will Update in next patch set.>> Signed-off-by: Manish Pandey
> <quic_mapa@quicinc.com>
> > > ---
> > > 
> > > Changes in v3:
> > > - Addressed Bart's review comments by adding explanations for the
> > >    in_task() and usleep_range() calls.
> > > Changes in v2:
> > > - Rebased patchsets.
> > > - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
> > >   drivers/ufs/host/ufs-qcom.h |  2 ++
> > >   2 files changed, 62 insertions(+)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index f5181773c0e5..fb9da04c0d35 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
> > >   	return 0;
> > >   }
> > > +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
> > > +{
> > > +	/* sleep intermittently to prevent CPU hog during data dumps. */
> > > +	/* RES_MCQ_1 */
> > > +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
> > > +	usleep_range(1000, 1100);
> > 
> > If your motivation is just to not hog the CPU, use cond_resched().
> > 
> > - Mani
> > 
> The intention here is to introduce a specific delay between each dump.

What is the reason for that?

> Therefore, i would like to use usleep_range() instead of cond_resched().
> Please let me know if i am getting it wrong..
> 

Without knowing the reason, I cannot judge. Your comment said that you do not
want to hog the CPU during dump. But now you are saying that you wanted to have
a delay. Both are contradictions.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[MANISH PANDEY]

On 3/24/2025 1:09 PM, Manivannan Sadhasivam wrote:
> On Wed, Mar 19, 2025 at 11:51:07AM +0530, MANISH PANDEY wrote:
>>
>>
>> On 3/18/2025 12:14 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
>>>> This patch adds functionality to dump MCQ registers.
>>>> This will help in diagnosing issues related to MCQ
>>>> operations by providing detailed register dumps.
>>>>
>>>
>>> Same comment as previous patch. Also, make use of 75 column width.
>>>
>> will Update in next patch set.>> Signed-off-by: Manish Pandey
>> <quic_mapa@quicinc.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Addressed Bart's review comments by adding explanations for the
>>>>     in_task() and usleep_range() calls.
>>>> Changes in v2:
>>>> - Rebased patchsets.
>>>> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
>>>> ---
>>>>    drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
>>>>    drivers/ufs/host/ufs-qcom.h |  2 ++
>>>>    2 files changed, 62 insertions(+)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index f5181773c0e5..fb9da04c0d35 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
>>>>    	return 0;
>>>>    }
>>>> +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
>>>> +{
>>>> +	/* sleep intermittently to prevent CPU hog during data dumps. */
>>>> +	/* RES_MCQ_1 */
>>>> +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
>>>> +	usleep_range(1000, 1100);
>>>
>>> If your motivation is just to not hog the CPU, use cond_resched().
>>>
>>> - Mani
>>>
>> The intention here is to introduce a specific delay between each dump.
> 
> What is the reason for that?
> 
>> Therefore, i would like to use usleep_range() instead of cond_resched().
>> Please let me know if i am getting it wrong..
>>
> 
> Without knowing the reason, I cannot judge. Your comment said that you do not
> want to hog the CPU during dump. But now you are saying that you wanted to have
> a delay. Both are contradictions.
> 
> - Mani
> 
Hi Mani, Could you please clarify what you meant by delay? Did you mean 
udelay? That's not the case here, as we are using usleep(), which is 
similar to cond_resched(). I believe both serve the same purpose in this 
case. Therefore, I chose usleep() to provide a fixed delay between dumps 
since we are dumping a large amount of data. Additionally, I wanted to 
avoid any extra scheduling latency associated with cond_resched().

How ever i am open to change it to cond_resched() if needed.
Please suggest.

Regards
Manish

Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[Manivannan Sadhasivam]

On Tue, Mar 25, 2025 at 02:32:58PM +0530, MANISH PANDEY wrote:
> 
> 
> On 3/24/2025 1:09 PM, Manivannan Sadhasivam wrote:
> > On Wed, Mar 19, 2025 at 11:51:07AM +0530, MANISH PANDEY wrote:
> > > 
> > > 
> > > On 3/18/2025 12:14 PM, Manivannan Sadhasivam wrote:
> > > > On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
> > > > > This patch adds functionality to dump MCQ registers.
> > > > > This will help in diagnosing issues related to MCQ
> > > > > operations by providing detailed register dumps.
> > > > > 
> > > > 
> > > > Same comment as previous patch. Also, make use of 75 column width.
> > > > 
> > > will Update in next patch set.>> Signed-off-by: Manish Pandey
> > > <quic_mapa@quicinc.com>
> > > > > ---
> > > > > 
> > > > > Changes in v3:
> > > > > - Addressed Bart's review comments by adding explanations for the
> > > > >     in_task() and usleep_range() calls.
> > > > > Changes in v2:
> > > > > - Rebased patchsets.
> > > > > - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@quicinc.com/
> > > > > ---
> > > > >    drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
> > > > >    drivers/ufs/host/ufs-qcom.h |  2 ++
> > > > >    2 files changed, 62 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > > index f5181773c0e5..fb9da04c0d35 100644
> > > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > > @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
> > > > >    	return 0;
> > > > >    }
> > > > > +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
> > > > > +{
> > > > > +	/* sleep intermittently to prevent CPU hog during data dumps. */
> > > > > +	/* RES_MCQ_1 */
> > > > > +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
> > > > > +	usleep_range(1000, 1100);
> > > > 
> > > > If your motivation is just to not hog the CPU, use cond_resched().
> > > > 
> > > > - Mani
> > > > 
> > > The intention here is to introduce a specific delay between each dump.
> > 
> > What is the reason for that?
> > 
> > > Therefore, i would like to use usleep_range() instead of cond_resched().
> > > Please let me know if i am getting it wrong..
> > > 
> > 
> > Without knowing the reason, I cannot judge. Your comment said that you do not
> > want to hog the CPU during dump. But now you are saying that you wanted to have
> > a delay. Both are contradictions.
> > 
> > - Mani
> > 
> Hi Mani, Could you please clarify what you meant by delay? Did you mean
> udelay? That's not the case here, as we are using usleep(), which is similar
> to cond_resched(). I believe both serve the same purpose in this case.

Even though usleep() allows the scheduler to reschedule other tasks, both are
not the same. usleep() puts the thread to sleep until the elapsed time and other
tasks may be scheduled in the meantime. But cond_resched() will allow the
scheduler to schedule other tasks *only* if necessary. So the scheduler may
decide to continue executing the current thread if it is well within its
timeslice.

> Therefore, I chose usleep() to provide a fixed delay between dumps since we
> are dumping a large amount of data. Additionally, I wanted to avoid any
> extra scheduling latency associated with cond_resched().
> 

This doesn't make sense to me. Why do you want to provide a fixed delay? Will
that delay affect the UFS controller by any means? If not (afaik), you should
use cond_resched() as that will allow for faster dumps and also not hog the CPU.

- Mani

-- 
மணிவண்ணன் சதாசிவம்