[RESEND RFC 0/3] mmc: sdhci: sdhci-msm: Add more debug logs

[RESEND RFC 0/3] mmc: sdhci: sdhci-msm: Add more debug logs

[Ritesh Harjani]

Resending this small set of patches.

Below is a small set of patches which provide useful info for
debugging issues.

Patch 1 :- This provide callback mechanism for platform drivers
to dump their necessary register info at the time of sdhci_dumpregs.

Patch 2 :- This implements this callback (->platform_dumpregs)
for sdhci-msm.

Patch 3 :- Adds more logs for data errors to aid debugging.

Ritesh Harjani (1):
  mmc: sdhci: Add more debug info in case of data error

Sahitya Tummala (2):
  mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.
  mmc: sdhci-msm: Implement platform_dumpregs callback in sdhci-msm

 drivers/mmc/host/sdhci-msm.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     | 24 ++++++++++++++++++++++--
 drivers/mmc/host/sdhci.h     |  1 +
 3 files changed, 57 insertions(+), 2 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[RESEND RFC 1/3] mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.

[Ritesh Harjani]

From: Sahitya Tummala <stummala@codeaurora.org>

Add new host operation ->platform_dumpregs to provide a
mechanism through which host drivers can dump platform
specific registers in addition to SDHC registers
during error conditions.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 3 +++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2390980..73a8918 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -101,6 +101,9 @@ static void sdhci_dumpregs(struct sdhci_host *host)
 			       readl(host->ioaddr + SDHCI_ADMA_ADDRESS));
 	}
 
+	if (host->ops->platform_dumpregs)
+		host->ops->platform_dumpregs(host);
+
 	pr_err(DRIVER_NAME ": ===========================================\n");
 }
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0b66f21..400f3a1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -564,6 +564,7 @@ struct sdhci_ops {
 					 struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
+	void	(*platform_dumpregs)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[RESEND RFC 2/3] mmc: sdhci-msm: Implement platform_dumpregs callback in sdhci-msm

[Ritesh Harjani]

From: Sahitya Tummala <stummala@codeaurora.org>

Implement ->platform_dumpregs host operation to print the
platform specific registers in addition to standard SDHC
register during error conditions.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 32879b8..1241dbd 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -29,6 +29,11 @@
 #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
 #define CORE_VERSION_MINOR_MASK		0xff
 
+#define CORE_MCI_DATA_CNT	0x30
+#define CORE_MCI_STATUS		0x34
+#define CORE_MCI_FIFO_CNT	0x44
+#define CORE_MCI_STATUS2	0x6c
+
 #define CORE_HC_MODE		0x78
 #define HC_MODE_EN		0x1
 #define CORE_POWER		0x0
@@ -77,6 +82,10 @@
 #define CORE_HC_SELECT_IN_HS400	(6 << 19)
 #define CORE_HC_SELECT_IN_MASK	(7 << 19)
 
+#define CORE_VENDOR_SPEC_FUNC2		0x110
+#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR0	0x114
+#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR1	0x118
+
 #define CORE_CSR_CDC_CTLR_CFG0		0x130
 #define CORE_SW_TRIG_FULL_CALIB		BIT(16)
 #define CORE_HW_AUTOCAL_ENA		BIT(17)
@@ -658,6 +667,30 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	return ret;
 }
 
+static void sdhci_msm_dumpregs(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	pr_err("----------- PLATFORM REGISTER DUMP -----------\n");
+
+	pr_err("Data cnt: 0x%08x | Fifo cnt: 0x%08x | Int sts: 0x%08x | Int sts2: 0x%08x\n",
+	       readl_relaxed(msm_host->core_mem + CORE_MCI_DATA_CNT),
+	       readl_relaxed(msm_host->core_mem + CORE_MCI_FIFO_CNT),
+	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS),
+	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS2));
+	pr_err("DLL cfg:  0x%08x | DLL sts:  0x%08x | SDCC ver: 0x%08x\n",
+	       readl_relaxed(host->ioaddr + CORE_DLL_CONFIG),
+	       readl_relaxed(host->ioaddr + CORE_DLL_STATUS),
+	       readl_relaxed(msm_host->core_mem + CORE_MCI_VERSION));
+	pr_err("Vndr func: 0x%08x | Vndr adma err : addr0: 0x%08x addr1: 0x%08x\n",
+	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC),
+	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR0),
+	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR1));
+	pr_err("Vndr func2: 0x%08x\n",
+	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_FUNC2));
+}
+
 static int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
 {
 	int tuning_seq_cnt = 3;
@@ -1035,6 +1068,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
 	.voltage_switch = sdhci_msm_voltage_switch,
+	.platform_dumpregs = sdhci_msm_dumpregs,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[RESEND RFC 3/3] mmc: sdhci: Add more debug info in case of data error

[Ritesh Harjani]

print error log message and dump sdhc registers for debugging
purpose in case of data errors (except when tuning commands
generate CRC/timeout/end bit errors).

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 73a8918..2e51e49 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2616,9 +2616,26 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 			host->ops->adma_workaround(host, intmask);
 	}
 
-	if (host->data->error)
+	if (host->data->error) {
+		bool pr_msg = true;
+
+		if (intmask & (SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_TIMEOUT |
+		    SDHCI_INT_DATA_END_BIT)) {
+			command = SDHCI_GET_CMD(sdhci_readw(host,
+							    SDHCI_COMMAND));
+			if (command == MMC_SEND_TUNING_BLOCK_HS200 ||
+			    command == MMC_SEND_TUNING_BLOCK)
+				pr_msg = false;
+		}
+		if (pr_msg) {
+			pr_err("%s: data txfr (0x%08x) error: %d\n",
+			       mmc_hostname(host->mmc), intmask,
+			       host->data->error);
+			sdhci_dumpregs(host);
+		}
+
 		sdhci_finish_data(host);
-	else {
+	} else {
 		if (intmask & (SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL))
 			sdhci_transfer_pio(host);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

Re: [RESEND RFC 1/3] mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.

[Shawn Lin]

On 2017/1/10 14:41, Ritesh Harjani wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
>
> Add new host operation ->platform_dumpregs to provide a
> mechanism through which host drivers can dump platform
> specific registers in addition to SDHC registers
> during error conditions.
>

Although we have been preventing from adding new callback
for sdhci core, this one makes sense as there are more and more
vendor registers outside the scope of SDHCI spec.


> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 3 +++
>  drivers/mmc/host/sdhci.h | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2390980..73a8918 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -101,6 +101,9 @@ static void sdhci_dumpregs(struct sdhci_host *host)
>  			       readl(host->ioaddr + SDHCI_ADMA_ADDRESS));
>  	}
>
> +	if (host->ops->platform_dumpregs)
> +		host->ops->platform_dumpregs(host);
> +
>  	pr_err(DRIVER_NAME ": ===========================================\n");
>  }
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0b66f21..400f3a1 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -564,6 +564,7 @@ struct sdhci_ops {
>  					 struct mmc_card *card,
>  					 unsigned int max_dtr, int host_drv,
>  					 int card_drv, int *drv_type);
> +	void	(*platform_dumpregs)(struct sdhci_host *host);
>  };
>
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>


-- 
Best Regards
Shawn Lin

Re: [RESEND RFC 1/3] mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.

[Ritesh Harjani]

Hi Shawn,

On 1/10/2017 2:45 PM, Shawn Lin wrote:
> On 2017/1/10 14:41, Ritesh Harjani wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> Add new host operation ->platform_dumpregs to provide a
>> mechanism through which host drivers can dump platform
>> specific registers in addition to SDHC registers
>> during error conditions.
>>
>
> Although we have been preventing from adding new callback
> for sdhci core, this one makes sense as there are more and more
> vendor registers outside the scope of SDHCI spec.
Sure thanks, shall I add your Reviewed-by on this patch 1/3.
Would you be able to review other as well?



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND RFC 1/3] mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.

[Adrian Hunter]

On 18/01/17 09:57, Ritesh Harjani wrote:
> Hi Shawn,
> 
> On 1/10/2017 2:45 PM, Shawn Lin wrote:
>> On 2017/1/10 14:41, Ritesh Harjani wrote:
>>> From: Sahitya Tummala <stummala@codeaurora.org>
>>>
>>> Add new host operation ->platform_dumpregs to provide a
>>> mechanism through which host drivers can dump platform
>>> specific registers in addition to SDHC registers
>>> during error conditions.
>>>
>>
>> Although we have been preventing from adding new callback
>> for sdhci core, this one makes sense as there are more and more
>> vendor registers outside the scope of SDHCI spec.

We are not prevented from adding new callbacks, but they have to represent
logical functions not quirks.

This patch seems fine to me except the name "platform_dumpregs" because
"platform" doesn't mean "platform" here.  Just plain "dumpregs" is better.

Re: [RESEND RFC 2/3] mmc: sdhci-msm: Implement platform_dumpregs callback in sdhci-msm

[Adrian Hunter]

On 10/01/17 08:41, Ritesh Harjani wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> Implement ->platform_dumpregs host operation to print the
> platform specific registers in addition to standard SDHC
> register during error conditions.

You could add an example of the prints so we can see it looks nice.

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 32879b8..1241dbd 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -29,6 +29,11 @@
>  #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
>  #define CORE_VERSION_MINOR_MASK		0xff
>  
> +#define CORE_MCI_DATA_CNT	0x30
> +#define CORE_MCI_STATUS		0x34
> +#define CORE_MCI_FIFO_CNT	0x44
> +#define CORE_MCI_STATUS2	0x6c
> +
>  #define CORE_HC_MODE		0x78
>  #define HC_MODE_EN		0x1
>  #define CORE_POWER		0x0
> @@ -77,6 +82,10 @@
>  #define CORE_HC_SELECT_IN_HS400	(6 << 19)
>  #define CORE_HC_SELECT_IN_MASK	(7 << 19)
>  
> +#define CORE_VENDOR_SPEC_FUNC2		0x110
> +#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR0	0x114
> +#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR1	0x118
> +
>  #define CORE_CSR_CDC_CTLR_CFG0		0x130
>  #define CORE_SW_TRIG_FULL_CALIB		BIT(16)
>  #define CORE_HW_AUTOCAL_ENA		BIT(17)
> @@ -658,6 +667,30 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>  	return ret;
>  }
>  
> +static void sdhci_msm_dumpregs(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	pr_err("----------- PLATFORM REGISTER DUMP -----------\n");

"PLATFORM" isn't right here.  It should be something that identifies the IP
/ driver. e.g. "SDHCI MSM"

> +
> +	pr_err("Data cnt: 0x%08x | Fifo cnt: 0x%08x | Int sts: 0x%08x | Int sts2: 0x%08x\n",

What sdhci_dumpregs() should do is prefix the message with the host name i.e.

	pr_err("%s: sdhci: Sys addr: 0x%08x | Version:  0x%08x\n",
	       sdhci_readl(host, SDHCI_DMA_ADDRESS),
	       sdhci_readw(host, SDHCI_HOST_VERSION),
	       mmc_hostname(host->mmc));

And then we should do that here too.

> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_DATA_CNT),
> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_FIFO_CNT),
> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS),
> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS2));
> +	pr_err("DLL cfg:  0x%08x | DLL sts:  0x%08x | SDCC ver: 0x%08x\n",
> +	       readl_relaxed(host->ioaddr + CORE_DLL_CONFIG),
> +	       readl_relaxed(host->ioaddr + CORE_DLL_STATUS),
> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_VERSION));
> +	pr_err("Vndr func: 0x%08x | Vndr adma err : addr0: 0x%08x addr1: 0x%08x\n",
> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC),
> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR0),
> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR1));
> +	pr_err("Vndr func2: 0x%08x\n",
> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_FUNC2));
> +}
> +
>  static int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
>  {
>  	int tuning_seq_cnt = 3;
> @@ -1035,6 +1068,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>  	.voltage_switch = sdhci_msm_voltage_switch,
> +	.platform_dumpregs = sdhci_msm_dumpregs,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> 

Re: [RESEND RFC 3/3] mmc: sdhci: Add more debug info in case of data error

[Adrian Hunter]

On 10/01/17 08:41, Ritesh Harjani wrote:
> print error log message and dump sdhc registers for debugging
> purpose in case of data errors (except when tuning commands
> generate CRC/timeout/end bit errors).

It is a bit ugly and probably only useful during driver development, so I am
not at all enthusiastic about this.

> 
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 73a8918..2e51e49 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2616,9 +2616,26 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  			host->ops->adma_workaround(host, intmask);
>  	}
>  
> -	if (host->data->error)
> +	if (host->data->error) {
> +		bool pr_msg = true;
> +
> +		if (intmask & (SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_TIMEOUT |
> +		    SDHCI_INT_DATA_END_BIT)) {
> +			command = SDHCI_GET_CMD(sdhci_readw(host,
> +							    SDHCI_COMMAND));
> +			if (command == MMC_SEND_TUNING_BLOCK_HS200 ||
> +			    command == MMC_SEND_TUNING_BLOCK)
> +				pr_msg = false;
> +		}
> +		if (pr_msg) {
> +			pr_err("%s: data txfr (0x%08x) error: %d\n",
> +			       mmc_hostname(host->mmc), intmask,
> +			       host->data->error);
> +			sdhci_dumpregs(host);
> +		}
> +
>  		sdhci_finish_data(host);
> -	else {
> +	} else {
>  		if (intmask & (SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL))
>  			sdhci_transfer_pio(host);
>  
> 

Re: [RESEND RFC 2/3] mmc: sdhci-msm: Implement platform_dumpregs callback in sdhci-msm

[Ritesh Harjani]

Hi Adrian,

On 1/19/2017 4:25 PM, Adrian Hunter wrote:
> On 10/01/17 08:41, Ritesh Harjani wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> Implement ->platform_dumpregs host operation to print the
>> platform specific registers in addition to standard SDHC
>> register during error conditions.
>
> You could add an example of the prints so we can see it looks nice.
Sure. Below is an example, taken during bootup. That's why you see all 
registers as 0.

http://pastebin.com/7j94punq

>
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 32879b8..1241dbd 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -29,6 +29,11 @@
>>  #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
>>  #define CORE_VERSION_MINOR_MASK		0xff
>>
>> +#define CORE_MCI_DATA_CNT	0x30
>> +#define CORE_MCI_STATUS		0x34
>> +#define CORE_MCI_FIFO_CNT	0x44
>> +#define CORE_MCI_STATUS2	0x6c
>> +
>>  #define CORE_HC_MODE		0x78
>>  #define HC_MODE_EN		0x1
>>  #define CORE_POWER		0x0
>> @@ -77,6 +82,10 @@
>>  #define CORE_HC_SELECT_IN_HS400	(6 << 19)
>>  #define CORE_HC_SELECT_IN_MASK	(7 << 19)
>>
>> +#define CORE_VENDOR_SPEC_FUNC2		0x110
>> +#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR0	0x114
>> +#define CORE_VENDOR_SPEC_ADMA_ERR_ADDR1	0x118
>> +
>>  #define CORE_CSR_CDC_CTLR_CFG0		0x130
>>  #define CORE_SW_TRIG_FULL_CALIB		BIT(16)
>>  #define CORE_HW_AUTOCAL_ENA		BIT(17)
>> @@ -658,6 +667,30 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>  	return ret;
>>  }
>>
>> +static void sdhci_msm_dumpregs(struct sdhci_host *host)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	pr_err("----------- PLATFORM REGISTER DUMP -----------\n");
>
> "PLATFORM" isn't right here.  It should be something that identifies the IP
> / driver. e.g. "SDHCI MSM"
Sure.

>
>> +
>> +	pr_err("Data cnt: 0x%08x | Fifo cnt: 0x%08x | Int sts: 0x%08x | Int sts2: 0x%08x\n",
>
> What sdhci_dumpregs() should do is prefix the message with the host name i.e.

I feel it may be redundant info. As all sdhci-msm register dump with 
come with mmc0/mmc1 prefixed with it.
Instead I see sdhci_dumpregs already adds hostname in the starting 
header of register dump. So this may not be required I guess.
I have added an example dumpregs (http://pastebin.com/7j94punq).

Please let me know if you still think otherwise.

Regards
Ritesh


>
> 	pr_err("%s: sdhci: Sys addr: 0x%08x | Version:  0x%08x\n",
> 	       sdhci_readl(host, SDHCI_DMA_ADDRESS),
> 	       sdhci_readw(host, SDHCI_HOST_VERSION),
> 	       mmc_hostname(host->mmc));
>
> And then we should do that here too.
>
>> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_DATA_CNT),
>> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_FIFO_CNT),
>> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS),
>> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_STATUS2));
>> +	pr_err("DLL cfg:  0x%08x | DLL sts:  0x%08x | SDCC ver: 0x%08x\n",
>> +	       readl_relaxed(host->ioaddr + CORE_DLL_CONFIG),
>> +	       readl_relaxed(host->ioaddr + CORE_DLL_STATUS),
>> +	       readl_relaxed(msm_host->core_mem + CORE_MCI_VERSION));
>> +	pr_err("Vndr func: 0x%08x | Vndr adma err : addr0: 0x%08x addr1: 0x%08x\n",
>> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC),
>> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR0),
>> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_ADMA_ERR_ADDR1));
>> +	pr_err("Vndr func2: 0x%08x\n",
>> +	       readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC_FUNC2));
>> +}
>> +
>>  static int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
>>  {
>>  	int tuning_seq_cnt = 3;
>> @@ -1035,6 +1068,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>  	.set_bus_width = sdhci_set_bus_width,
>>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>>  	.voltage_switch = sdhci_msm_voltage_switch,
>> +	.platform_dumpregs = sdhci_msm_dumpregs,
>>  };
>>
>>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND RFC 1/3] mmc: sdhci: Add platform_dumpregs callback support to sdhci_ops.

[Ritesh Harjani]

On 1/19/2017 4:13 PM, Adrian Hunter wrote:
> On 18/01/17 09:57, Ritesh Harjani wrote:
>> Hi Shawn,
>>
>> On 1/10/2017 2:45 PM, Shawn Lin wrote:
>>> On 2017/1/10 14:41, Ritesh Harjani wrote:
>>>> From: Sahitya Tummala <stummala@codeaurora.org>
>>>>
>>>> Add new host operation ->platform_dumpregs to provide a
>>>> mechanism through which host drivers can dump platform
>>>> specific registers in addition to SDHC registers
>>>> during error conditions.
>>>>
>>>
>>> Although we have been preventing from adding new callback
>>> for sdhci core, this one makes sense as there are more and more
>>> vendor registers outside the scope of SDHCI spec.
>
> We are not prevented from adding new callbacks, but they have to represent
> logical functions not quirks.
>
> This patch seems fine to me except the name "platform_dumpregs" because
I took platform prefix similar to platform_execute_tuning.


> "platform" doesn't mean "platform" here.  Just plain "dumpregs" is better.
Sure, I will only keep "dumpregs" in next revision.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND RFC 3/3] mmc: sdhci: Add more debug info in case of data error

[Ritesh Harjani]

Hi Adrian,

Thanks for reviewing.

On 1/19/2017 5:24 PM, Adrian Hunter wrote:
> On 10/01/17 08:41, Ritesh Harjani wrote:
>> print error log message and dump sdhc registers for debugging
>> purpose in case of data errors (except when tuning commands
>> generate CRC/timeout/end bit errors).
>
> It is a bit ugly and probably only useful during driver development, so I am
> not at all enthusiastic about this.
Ok, since this patch was anyway for debugging, I will drop this patch 
for now. I will revisit in case if the need really arises, since this 
could be useful in debugging data txfr error and avoid re-test cycle to 
get sdhci register dumps.



Also,
Do you think we should have this below change added? Could you please 
give your thoughts on this. Jeremy and I feel that even below info can 
be helpful along with sdhci_dumpregs -
He requested me to take below diff as another patch in my next revision 
of this series which he has submitted here -

https://patchwork.kernel.org/patch/9442449/


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 71654b9..5911f98 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -47,6 +47,27 @@  static void sdhci_finish_data(struct sdhci_host *);

  static void sdhci_enable_preset_value(struct sdhci_host *host, bool 
enable);

+static void sdhci_dump_rpm_info(struct sdhci_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	pr_info("%s: 
rpmstatus[pltfm](runtime-suspend:usage_count:disable_depth)(%d:%d:%d)\n",
+		mmc_hostname(mmc), mmc->parent->power.runtime_status,
+		atomic_read(&mmc->parent->power.usage_count),
+		mmc->parent->power.disable_depth);
+}
+
+
+static void sdhci_dump_state(struct sdhci_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	pr_info("%s: clk: %d claimer: %s pwr: %d\n",
+		mmc_hostname(mmc), host->clock,
+		mmc->claimer->comm, host->pwr);
+		sdhci_dump_rpm_info(host);
+}
+
  static void sdhci_dumpregs(struct sdhci_host *host)
  {
  	pr_err(DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
@@ -100,6 +121,10 @@  static void sdhci_dumpregs(struct sdhci_host *host)
  			       readl(host->ioaddr + SDHCI_ADMA_ADDRESS));
  	}

+	if (host->ops->dump_vendor_regs)
+		host->ops->dump_vendor_regs(host);
+
+	sdhci_dump_state(host);
  	pr_err(DRIVER_NAME ": ===========================================\n");
  }

Regards
Ritesh

>
>>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 73a8918..2e51e49 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2616,9 +2616,26 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>>  			host->ops->adma_workaround(host, intmask);
>>  	}
>>
>> -	if (host->data->error)
>> +	if (host->data->error) {
>> +		bool pr_msg = true;
>> +
>> +		if (intmask & (SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_TIMEOUT |
>> +		    SDHCI_INT_DATA_END_BIT)) {
>> +			command = SDHCI_GET_CMD(sdhci_readw(host,
>> +							    SDHCI_COMMAND));
>> +			if (command == MMC_SEND_TUNING_BLOCK_HS200 ||
>> +			    command == MMC_SEND_TUNING_BLOCK)
>> +				pr_msg = false;
>> +		}
>> +		if (pr_msg) {
>> +			pr_err("%s: data txfr (0x%08x) error: %d\n",
>> +			       mmc_hostname(host->mmc), intmask,
>> +			       host->data->error);
>> +			sdhci_dumpregs(host);
>> +		}
>> +
>>  		sdhci_finish_data(host);
>> -	else {
>> +	} else {
>>  		if (intmask & (SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL))
>>  			sdhci_transfer_pio(host);
>>
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project