diff mbox series

[v2,4/6] mmc: host: sdhci-esdhc-imx.c: disable auto-tuning when necessary

Message ID 1629285415-7495-4-git-send-email-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/6] mmc: sdhci-esdhc-imx: remove redundant code for manual tuning | expand

Commit Message

Bough Chen Aug. 18, 2021, 11:16 a.m. UTC
From: Haibo Chen <haibo.chen@nxp.com>

Add a method to enable/disable auto-tuning function. auto-tuning function
is conflict with sdio interrupt. For sdio device with sdio interrupt,
need to disable auto-tuning function.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Kevin Groeneveld Dec. 5, 2022, 2:59 p.m. UTC | #1
Thank you Haibo for pointing me here from 
https://www.spinics.net/lists/linux-mmc/msg73270.html.

On 2021-08-18 07:16, haibo.chen@nxp.com wrote:
> Add a method to enable/disable auto-tuning function. auto-tuning function
> is conflict with sdio interrupt. For sdio device with sdio interrupt,
> need to disable auto-tuning function.

I tested this patch on an imx8mm system and it made things completely 
unstable. I was never really able to log into the system properly and 
just got lots of messages similar to the following:

[   31.946640] rcu: INFO: rcu_preempt self-detected stall on CPU
[   31.952422] rcu:     0-....: (2106 ticks this GP) 
idle=849/1/0x4000000000000000 softirq=902/904 fqs=743
[   31.961663]  (t=2100 jiffies g=33 q=1158)
[   31.965682] Task dump for CPU 0:
[   31.968915] task:kworker/0:1     state:R  running task     stack: 
0 pid:   33 ppid:     2 flags:0x0000000a
[   31.978859] Workqueue:  0x0 (pm)

While working on this I also came across 
https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/uSDHC-auto-tuning-and-possible-SDIO-failures/ta-p/1352855 
which seems to address the same issue as your proposed patch.

That article suggests only enabling auto tuning for one data line as a 
workaround. I tried this method and so far it seems to have addressed 
the -84 errors I was seeing with SDIO communication to a WiFi module.

Some thoughts / questions:

Why does this proposed patch make my system unstable? (I was testing 
with a v5.16 mainline based kernel, but I did not see anything in later 
versions of sdhci-esdhc-imx that seemed like this should be a problem.)

Why does this patch try to disable auto tune entirely vs just setting it 
up for one data bit as suggested in the NXP knowledge base article?

As some other have suggested it seems like it would be nicer if the 
workaround could be applied automatically if the device using the SDIO 
interface enabled IRQs. Having to include a non standard entry in the DT 
for a hardware bug you may not know about or understand seems error 
prone. I guess maybe some device could generate an IRQ before they 
actually enable IRQs? In that case maybe a DT entry is required, but 
maybe the driver could generate a warning if IRQs are enabled without 
the DT entry?


Thanks,
Kevin
Bough Chen Dec. 9, 2022, 8:43 a.m. UTC | #2
> -----Original Message-----
> From: Kevin Groeneveld <kgroeneveld@lenbrook.com>
> Sent: 2022年12月5日 23:00
> To: Bough Chen <haibo.chen@nxp.com>; adrian.hunter@intel.com;
> ulf.hansson@linaro.org; shawnguo@kernel.org; robh+dt@kernel.org;
> s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; festevam@gmail.com; linux-mmc@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 4/6] mmc: host: sdhci-esdhc-imx.c: disable auto-tuning
> when necessary
> 
> Thank you Haibo for pointing me here from
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spi
> nics.net%2Flists%2Flinux-mmc%2Fmsg73270.html&amp;data=05%7C01%7Chai
> bo.chen%40nxp.com%7C2c5b5f4d53d04051475308dad6d16673%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C638058492114803922%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=sCV8u6Gv7x%2Bqi6kYSvZ
> uZUZeQQ1TaKPwhKpizt49qps%3D&amp;reserved=0.
> 
> On 2021-08-18 07:16, haibo.chen@nxp.com wrote:
> > Add a method to enable/disable auto-tuning function. auto-tuning
> > function is conflict with sdio interrupt. For sdio device with sdio
> > interrupt, need to disable auto-tuning function.
> 
> I tested this patch on an imx8mm system and it made things completely
> unstable. I was never really able to log into the system properly and just got lots
> of messages similar to the following:
> 
> [   31.946640] rcu: INFO: rcu_preempt self-detected stall on CPU
> [   31.952422] rcu:     0-....: (2106 ticks this GP)
> idle=849/1/0x4000000000000000 softirq=902/904 fqs=743
> [   31.961663]  (t=2100 jiffies g=33 q=1158)
> [   31.965682] Task dump for CPU 0:
> [   31.968915] task:kworker/0:1     state:R  running task     stack:
> 0 pid:   33 ppid:     2 flags:0x0000000a
> [   31.978859] Workqueue:  0x0 (pm)

These patch also exist on our local tree, and we do not meet this issue. Can you show me
The detail change you added?

> 
> While working on this I also came across
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommuni
> ty.nxp.com%2Ft5%2Fi-MX-Processors-Knowledge-Base%2FuSDHC-auto-tuning-a
> nd-possible-SDIO-failures%2Fta-p%2F1352855&amp;data=05%7C01%7Chaibo.c
> hen%40nxp.com%7C2c5b5f4d53d04051475308dad6d16673%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638058492114960153%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=w3VEjXfQKTIXvIIef1INySnQFU
> xW09uafNRdkkv8e7M%3D&amp;reserved=0
> which seems to address the same issue as your proposed patch.
> 
> That article suggests only enabling auto tuning for one data line as a
> workaround. I tried this method and so far it seems to have addressed the -84
> errors I was seeing with SDIO communication to a WiFi module.
> 
> Some thoughts / questions:
> 
> Why does this proposed patch make my system unstable? (I was testing with a
> v5.16 mainline based kernel, but I did not see anything in later versions of
> sdhci-esdhc-imx that seemed like this should be a problem.)
> 
> Why does this patch try to disable auto tune entirely vs just setting it up for one
> data bit as suggested in the NXP knowledge base article?
> 
> As some other have suggested it seems like it would be nicer if the workaround
> could be applied automatically if the device using the SDIO interface enabled
> IRQs. Having to include a non standard entry in the DT for a hardware bug you
> may not know about or understand seems error prone. I guess maybe some
> device could generate an IRQ before they actually enable IRQs? In that case
> maybe a DT entry is required, but maybe the driver could generate a warning if
> IRQs are enabled without the DT entry?

Yes, your method seems better, I will try to do like that. Thanks

Best Regards
Haibo Chen
> 
> 
> Thanks,
> Kevin
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index f18d169bc8ff..3af6519c561b 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -226,6 +226,7 @@  struct esdhc_platform_data {
 	unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
 	unsigned int tuning_start_tap;	/* The start delay cell point in tuning procedure */
 	unsigned int strobe_dll_delay_target;	/* The delay cell for strobe pad (read clock) */
+	bool broken_auto_tuning;	/* Disable the auto tuning circuit */
 };
 
 struct esdhc_soc_data {
@@ -672,8 +673,10 @@  static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 			if (val & SDHCI_CTRL_EXEC_TUNING) {
 				v |= ESDHC_MIX_CTRL_EXE_TUNE;
 				m |= ESDHC_MIX_CTRL_FBCLK_SEL;
-				m |= ESDHC_MIX_CTRL_AUTO_TUNE_EN;
-				usdhc_auto_tuning_mode_sel(host);
+				if (!imx_data->boarddata.broken_auto_tuning) {
+					usdhc_auto_tuning_mode_sel(host);
+					m |= ESDHC_MIX_CTRL_AUTO_TUNE_EN;
+				}
 			} else {
 				v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
 			}
@@ -1041,13 +1044,16 @@  static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
 
 static void esdhc_post_tuning(struct sdhci_host *host)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	u32 reg;
 
-	usdhc_auto_tuning_mode_sel(host);
-
 	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
 	reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
-	reg |= ESDHC_MIX_CTRL_AUTO_TUNE_EN;
+	if (!imx_data->boarddata.broken_auto_tuning) {
+		usdhc_auto_tuning_mode_sel(host);
+		reg |= ESDHC_MIX_CTRL_AUTO_TUNE_EN;
+	}
 	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
 }
 
@@ -1522,7 +1528,8 @@  sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 	of_property_read_u32(np, "fsl,tuning-step", &boarddata->tuning_step);
 	of_property_read_u32(np, "fsl,tuning-start-tap",
 			     &boarddata->tuning_start_tap);
-
+	if (of_property_read_bool(np, "fsl,broken-auto-tuning"))
+		boarddata->broken_auto_tuning = true;
 	of_property_read_u32(np, "fsl,strobe-dll-delay-target",
 				&boarddata->strobe_dll_delay_target);
 	if (of_find_property(np, "no-1-8-v", NULL))