[PATCH v2 0/7] mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI

[PATCH v2 0/7] mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

This is a series of nearly identical fixes for several SDHCI host
drivers. Patch #2 (for sdhci-of-arasan; plus its dependency in patch #1)
is the only one I've tested, and I wrote it due to a bug described
there.

I then noticed that several other drivers do the same thing, and that
commit df57d73276b8 ("mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for
Intel GLK-based controllers") points out the likely-repeated bug.

Thus, I include additional patches (compile-tested only) that apply
similar fixes to the other drivers which call cqhci_init() but not
cqhci_deactivate(). They contain appropriate disclaimers and the
relevant parties are CC'd. I would suggest only merging them if you get
some kind of ACK from people familiar with the relevant hardware.

Notably, I do *not* patch drivers/mmc/host/mtk-sd.c although it uses
CQHCI, because it doesn't seem to be an SDHCI-based controller, and so
even if it has a similar bug, it's not clear to me how to patch it.

- Brian

Changes in v2:
 - Rely on cqhci_deactivate() to safely handle (ignore)
   not-yet-initialized CQE support

Brian Norris (7):
  mmc: cqhci: Handle deactivate() when not yet initialized
  mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-brcmstb: Fix SDHCI_RESET_ALL for CQHCI
  mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-tegra: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-pci-*: Drop redundant ->cqe_private check

 drivers/mmc/host/cqhci-core.c      | 2 +-
 drivers/mmc/host/sdhci-brcmstb.c   | 3 +++
 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 drivers/mmc/host/sdhci-of-arasan.c | 3 +++
 drivers/mmc/host/sdhci-pci-core.c  | 3 +--
 drivers/mmc/host/sdhci-pci-gli.c   | 3 +--
 drivers/mmc/host/sdhci-tegra.c     | 3 +++
 drivers/mmc/host/sdhci_am654.c     | 3 +++
 8 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 0/7] mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

This is a series of nearly identical fixes for several SDHCI host
drivers. Patch #2 (for sdhci-of-arasan; plus its dependency in patch #1)
is the only one I've tested, and I wrote it due to a bug described
there.

I then noticed that several other drivers do the same thing, and that
commit df57d73276b8 ("mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for
Intel GLK-based controllers") points out the likely-repeated bug.

Thus, I include additional patches (compile-tested only) that apply
similar fixes to the other drivers which call cqhci_init() but not
cqhci_deactivate(). They contain appropriate disclaimers and the
relevant parties are CC'd. I would suggest only merging them if you get
some kind of ACK from people familiar with the relevant hardware.

Notably, I do *not* patch drivers/mmc/host/mtk-sd.c although it uses
CQHCI, because it doesn't seem to be an SDHCI-based controller, and so
even if it has a similar bug, it's not clear to me how to patch it.

- Brian

Changes in v2:
 - Rely on cqhci_deactivate() to safely handle (ignore)
   not-yet-initialized CQE support

Brian Norris (7):
  mmc: cqhci: Handle deactivate() when not yet initialized
  mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-brcmstb: Fix SDHCI_RESET_ALL for CQHCI
  mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-tegra: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI
  mmc: sdhci-pci-*: Drop redundant ->cqe_private check

 drivers/mmc/host/cqhci-core.c      | 2 +-
 drivers/mmc/host/sdhci-brcmstb.c   | 3 +++
 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 drivers/mmc/host/sdhci-of-arasan.c | 3 +++
 drivers/mmc/host/sdhci-pci-core.c  | 3 +--
 drivers/mmc/host/sdhci-pci-gli.c   | 3 +--
 drivers/mmc/host/sdhci-tegra.c     | 3 +++
 drivers/mmc/host/sdhci_am654.c     | 3 +++
 8 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] mmc: cqhci: Handle deactivate() when not yet initialized

[Brian Norris]

Several SDHCI drivers need to deactivate command queueing in their reset
hook (see sdhci_cqhci_reset() / sdhci-pci-core.c, for example), and
several more are coming. Such drivers also tend to initialize CQHCI
support after they've already performed one or more resets.

Rather than rely on careful ordering of cqhci_init() within the host
setup and reset sequence, let's do a simple NULL check -- deactivating a
non-initialized CQHCI instance is harmless.

This is an important prerequisite patch for several SDHCI controller
bugfixes that follow.

Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - New in v2

 drivers/mmc/host/cqhci-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
index b3d7d6d8d654..1fa1d24abb2e 100644
--- a/drivers/mmc/host/cqhci-core.c
+++ b/drivers/mmc/host/cqhci-core.c
@@ -315,7 +315,7 @@ int cqhci_deactivate(struct mmc_host *mmc)
 {
 	struct cqhci_host *cq_host = mmc->cqe_private;
 
-	if (cq_host->enabled && cq_host->activated)
+	if (cq_host && cq_host->enabled && cq_host->activated)
 		__cqhci_disable(cq_host);
 
 	return 0;
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 1/7] mmc: cqhci: Handle deactivate() when not yet initialized

[Brian Norris]

Several SDHCI drivers need to deactivate command queueing in their reset
hook (see sdhci_cqhci_reset() / sdhci-pci-core.c, for example), and
several more are coming. Such drivers also tend to initialize CQHCI
support after they've already performed one or more resets.

Rather than rely on careful ordering of cqhci_init() within the host
setup and reset sequence, let's do a simple NULL check -- deactivating a
non-initialized CQHCI instance is harmless.

This is an important prerequisite patch for several SDHCI controller
bugfixes that follow.

Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - New in v2

 drivers/mmc/host/cqhci-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
index b3d7d6d8d654..1fa1d24abb2e 100644
--- a/drivers/mmc/host/cqhci-core.c
+++ b/drivers/mmc/host/cqhci-core.c
@@ -315,7 +315,7 @@ int cqhci_deactivate(struct mmc_host *mmc)
 {
 	struct cqhci_host *cq_host = mmc->cqe_private;
 
-	if (cq_host->enabled && cq_host->activated)
+	if (cq_host && cq_host->enabled && cq_host->activated)
 		__cqhci_disable(cq_host);
 
 	return 0;
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but one
particular case I hit commonly enough: mmc_suspend() -> mmc_power_off().
Typically we will eventually deactivate CQE (cqhci_suspend() ->
cqhci_deactivate()), but that's not guaranteed -- in particular, if
we perform a partial (e.g., interrupted) system suspend.

The same bug was already found and fixed for two other drivers, in v5.7
and v5.9:

5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers

The latter is especially prescient, saying "other drivers using CQHCI
might benefit from a similar change, if they also have CQHCI reset by
SDHCI_RESET_ALL."

So like these other patches, deactivate CQHCI when resetting the
controller.

Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1")
Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Rely on cqhci_deactivate() to safely handle (ignore)
   not-yet-initialized CQE support

 drivers/mmc/host/sdhci-of-arasan.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 3997cad1f793..b30f0d6baf5b 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_FORCE_CDTEST) {
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but one
particular case I hit commonly enough: mmc_suspend() -> mmc_power_off().
Typically we will eventually deactivate CQE (cqhci_suspend() ->
cqhci_deactivate()), but that's not guaranteed -- in particular, if
we perform a partial (e.g., interrupted) system suspend.

The same bug was already found and fixed for two other drivers, in v5.7
and v5.9:

5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers

The latter is especially prescient, saying "other drivers using CQHCI
might benefit from a similar change, if they also have CQHCI reset by
SDHCI_RESET_ALL."

So like these other patches, deactivate CQHCI when resetting the
controller.

Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1")
Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Rely on cqhci_deactivate() to safely handle (ignore)
   not-yet-initialized CQE support

 drivers/mmc/host/sdhci-of-arasan.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 3997cad1f793..b30f0d6baf5b 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_FORCE_CDTEST) {
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] mmc: sdhci-brcmstb: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: d46ba2d17f90 ("mmc: sdhci-brcmstb: Add support for Command Queuing (CQE)")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Rely on cqhci_deactivate() to handle NULL cqe_private, instead of
   moving around CQE capability flags

 drivers/mmc/host/sdhci-brcmstb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index aff36a933ebe..d479ca39c987 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -55,6 +55,9 @@ static void brcmstb_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	/* Reset will clear this, so re-enable it */
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 3/7] mmc: sdhci-brcmstb: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: d46ba2d17f90 ("mmc: sdhci-brcmstb: Add support for Command Queuing (CQE)")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Rely on cqhci_deactivate() to handle NULL cqe_private, instead of
   moving around CQE capability flags

 drivers/mmc/host/sdhci-brcmstb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index aff36a933ebe..d479ca39c987 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -55,6 +55,9 @@ static void brcmstb_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	/* Reset will clear this, so re-enable it */
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: bb6e358169bf ("mmc: sdhci-esdhc-imx: add CMDQ support")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Drop unnecessary ESDHC_FLAG_CQHCI check

 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 55981b0f0b10..c07df7b71b22 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1288,6 +1288,9 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 
 static void esdhc_reset(struct sdhci_host *host, u8 mask)
 {
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: bb6e358169bf ("mmc: sdhci-esdhc-imx: add CMDQ support")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Drop unnecessary ESDHC_FLAG_CQHCI check

 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 55981b0f0b10..c07df7b71b22 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1288,6 +1288,9 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 
 static void esdhc_reset(struct sdhci_host *host, u8 mask)
 {
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/7] mmc: sdhci-tegra: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: 3c4019f97978 ("mmc: tegra: HW Command Queue Support for Tegra SDMMC")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Drop unnecessary 'enable_hwcq' check

 drivers/mmc/host/sdhci-tegra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 413925bce0ca..0a80335a42ec 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -367,6 +367,9 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
 	const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
 	u32 misc_ctrl, clk_ctrl, pad_ctrl;
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (!(mask & SDHCI_RESET_ALL))
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 5/7] mmc: sdhci-tegra: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: 3c4019f97978 ("mmc: tegra: HW Command Queue Support for Tegra SDMMC")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - Drop unnecessary 'enable_hwcq' check

 drivers/mmc/host/sdhci-tegra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 413925bce0ca..0a80335a42ec 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -367,6 +367,9 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
 	const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
 	u32 misc_ctrl, clk_ctrl, pad_ctrl;
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (!(mask & SDHCI_RESET_ALL))
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/7] mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 drivers/mmc/host/sdhci_am654.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 8f1023480e12..187a21086791 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -378,6 +378,9 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 6/7] mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

 [[ NOTE: this is completely untested by the author, but included solely
    because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
    SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
    drivers using CQHCI might benefit from a similar change, if they
    also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
    bug on at least MSM, Arasan, and Intel hardware. ]]

SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.

It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.

Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 drivers/mmc/host/sdhci_am654.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 8f1023480e12..187a21086791 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -378,6 +378,9 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
 
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
+
 	sdhci_reset(host, mask);
 
 	if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] mmc: sdhci-pci-*: Drop redundant ->cqe_private check

[Brian Norris]

An earlier patch ("mmc: cqhci: Handle deactivate() when not yet
initialized") makes these redundant.

I keep these as a separate patch, since the earlier patch is a
prerequisite to some important bugfixes that need to be backported via
linux-stable.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - New in v2

 drivers/mmc/host/sdhci-pci-core.c | 3 +--
 drivers/mmc/host/sdhci-pci-gli.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 169b84761041..63d62a9228d7 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -236,8 +236,7 @@ static void sdhci_pci_dumpregs(struct mmc_host *mmc)
 
 static void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
 {
-	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
-	    host->mmc->cqe_private)
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
 		cqhci_deactivate(host->mmc);
 	sdhci_reset(host, mask);
 }
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 4d509f656188..5a13fe961620 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -924,8 +924,7 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
 
 static void sdhci_gl9763e_reset(struct sdhci_host *host, u8 mask)
 {
-	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
-	    host->mmc->cqe_private)
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
 		cqhci_deactivate(host->mmc);
 	sdhci_reset(host, mask);
 }
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 7/7] mmc: sdhci-pci-*: Drop redundant ->cqe_private check

[Brian Norris]

An earlier patch ("mmc: cqhci: Handle deactivate() when not yet
initialized") makes these redundant.

I keep these as a separate patch, since the earlier patch is a
prerequisite to some important bugfixes that need to be backported via
linux-stable.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 - New in v2

 drivers/mmc/host/sdhci-pci-core.c | 3 +--
 drivers/mmc/host/sdhci-pci-gli.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 169b84761041..63d62a9228d7 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -236,8 +236,7 @@ static void sdhci_pci_dumpregs(struct mmc_host *mmc)
 
 static void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
 {
-	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
-	    host->mmc->cqe_private)
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
 		cqhci_deactivate(host->mmc);
 	sdhci_reset(host, mask);
 }
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 4d509f656188..5a13fe961620 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -924,8 +924,7 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
 
 static void sdhci_gl9763e_reset(struct sdhci_host *host, u8 mask)
 {
-	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
-	    host->mmc->cqe_private)
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
 		cqhci_deactivate(host->mmc);
 	sdhci_reset(host, mask);
 }
-- 
2.38.0.413.g74048e4d9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Florian Fainelli]

On 10/19/22 14:54, Brian Norris wrote:
> SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
> tracking that properly in software. When out of sync, we may trigger
> various timeouts.
> 
> It's not typical to perform resets while CQE is enabled, but one
> particular case I hit commonly enough: mmc_suspend() -> mmc_power_off().
> Typically we will eventually deactivate CQE (cqhci_suspend() ->
> cqhci_deactivate()), but that's not guaranteed -- in particular, if
> we perform a partial (e.g., interrupted) system suspend.
> 
> The same bug was already found and fixed for two other drivers, in v5.7
> and v5.9:
> 
> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
> 
> The latter is especially prescient, saying "other drivers using CQHCI
> might benefit from a similar change, if they also have CQHCI reset by
> SDHCI_RESET_ALL."
> 
> So like these other patches, deactivate CQHCI when resetting the
> controller.
> 
> Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
> Changes in v2:
>   - Rely on cqhci_deactivate() to safely handle (ignore)
>     not-yet-initialized CQE support
> 
>   drivers/mmc/host/sdhci-of-arasan.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 3997cad1f793..b30f0d6baf5b 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>   
> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> +		cqhci_deactivate(host->mmc);
> +
>   	sdhci_reset(host, mask);

Cannot this be absorbed by sdhci_reset() that all of these drivers 
appear to be utilizing since you have access to the host and the mask to 
make that decision?
-- 
Florian

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Florian Fainelli]

On 10/19/22 14:54, Brian Norris wrote:
> SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
> tracking that properly in software. When out of sync, we may trigger
> various timeouts.
> 
> It's not typical to perform resets while CQE is enabled, but one
> particular case I hit commonly enough: mmc_suspend() -> mmc_power_off().
> Typically we will eventually deactivate CQE (cqhci_suspend() ->
> cqhci_deactivate()), but that's not guaranteed -- in particular, if
> we perform a partial (e.g., interrupted) system suspend.
> 
> The same bug was already found and fixed for two other drivers, in v5.7
> and v5.9:
> 
> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
> 
> The latter is especially prescient, saying "other drivers using CQHCI
> might benefit from a similar change, if they also have CQHCI reset by
> SDHCI_RESET_ALL."
> 
> So like these other patches, deactivate CQHCI when resetting the
> controller.
> 
> Fixes: 84362d79f436 ("mmc: sdhci-of-arasan: Add CQHCI support for arasan,sdhci-5.1")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
> Changes in v2:
>   - Rely on cqhci_deactivate() to safely handle (ignore)
>     not-yet-initialized CQE support
> 
>   drivers/mmc/host/sdhci-of-arasan.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 3997cad1f793..b30f0d6baf5b 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>   
> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> +		cqhci_deactivate(host->mmc);
> +
>   	sdhci_reset(host, mask);

Cannot this be absorbed by sdhci_reset() that all of these drivers 
appear to be utilizing since you have access to the host and the mask to 
make that decision?
-- 
Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
> On 10/19/22 14:54, Brian Norris wrote:
> > The same bug was already found and fixed for two other drivers, in v5.7
> > and v5.9:
> > 
> > 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
> > df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
> > 
> > The latter is especially prescient, saying "other drivers using CQHCI
> > might benefit from a similar change, if they also have CQHCI reset by
> > SDHCI_RESET_ALL."

> > --- a/drivers/mmc/host/sdhci-of-arasan.c
> > +++ b/drivers/mmc/host/sdhci-of-arasan.c
> > @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
> >   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> > +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> > +		cqhci_deactivate(host->mmc);
> > +
> >   	sdhci_reset(host, mask);
> 
> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
> be utilizing since you have access to the host and the mask to make that
> decision?

It potentially could.

I don't know if this is a specified SDHCI behavior that really belongs
in the common helper, or if this is just a commonly-shared behavior. Per
the comments I quote above ("if they also have CQHCI reset by
SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
behavior.

I suppose it's not all that harmful to do this even if some SDHCI
controller doesn't have the same behavior/quirk.

I guess I also don't know if any SDHCI controllers will support command
queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
set MMC_CAP2_CQE.

Brian

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Brian Norris]

On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
> On 10/19/22 14:54, Brian Norris wrote:
> > The same bug was already found and fixed for two other drivers, in v5.7
> > and v5.9:
> > 
> > 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
> > df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
> > 
> > The latter is especially prescient, saying "other drivers using CQHCI
> > might benefit from a similar change, if they also have CQHCI reset by
> > SDHCI_RESET_ALL."

> > --- a/drivers/mmc/host/sdhci-of-arasan.c
> > +++ b/drivers/mmc/host/sdhci-of-arasan.c
> > @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
> >   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> > +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> > +		cqhci_deactivate(host->mmc);
> > +
> >   	sdhci_reset(host, mask);
> 
> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
> be utilizing since you have access to the host and the mask to make that
> decision?

It potentially could.

I don't know if this is a specified SDHCI behavior that really belongs
in the common helper, or if this is just a commonly-shared behavior. Per
the comments I quote above ("if they also have CQHCI reset by
SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
behavior.

I suppose it's not all that harmful to do this even if some SDHCI
controller doesn't have the same behavior/quirk.

I guess I also don't know if any SDHCI controllers will support command
queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
set MMC_CAP2_CQE.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Adrian Hunter]

On 20/10/22 01:19, Brian Norris wrote:
> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>> On 10/19/22 14:54, Brian Norris wrote:
>>> The same bug was already found and fixed for two other drivers, in v5.7
>>> and v5.9:
>>>
>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>
>>> The latter is especially prescient, saying "other drivers using CQHCI
>>> might benefit from a similar change, if they also have CQHCI reset by
>>> SDHCI_RESET_ALL."
> 
>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>> +		cqhci_deactivate(host->mmc);
>>> +
>>>   	sdhci_reset(host, mask);
>>
>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>> be utilizing since you have access to the host and the mask to make that
>> decision?
> 
> It potentially could.
> 
> I don't know if this is a specified SDHCI behavior that really belongs
> in the common helper, or if this is just a commonly-shared behavior. Per
> the comments I quote above ("if they also have CQHCI reset by
> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
> behavior.
> 
> I suppose it's not all that harmful to do this even if some SDHCI
> controller doesn't have the same behavior/quirk.
> 
> I guess I also don't know if any SDHCI controllers will support command
> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
> set MMC_CAP2_CQE.

SDHCI and CQHCI are separate modules and are not dependent, so they cannot
call into each other directly (and should not).  A new CQE API would be
needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
and wrapped in mmc/host.h:

static inline void mmc_cqe_notify_reset(struct mmc_host *host)
{
	if (host->cqe_ops->cqe_notify_reset)
		host->cqe_ops->cqe_notify_reset(host);
}

Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
although in this case there is so little code it could be static inline and
added in a new include file instead, say sdhci-cqhci.h e.g.

#include "cqhci.h"
#include "sdhci.h"

static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
{
	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
	    host->mmc->cqe_private)
		cqhci_deactivate(host->mmc);
	sdhci_reset(host, mask);
}

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Adrian Hunter]

On 20/10/22 01:19, Brian Norris wrote:
> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>> On 10/19/22 14:54, Brian Norris wrote:
>>> The same bug was already found and fixed for two other drivers, in v5.7
>>> and v5.9:
>>>
>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>
>>> The latter is especially prescient, saying "other drivers using CQHCI
>>> might benefit from a similar change, if they also have CQHCI reset by
>>> SDHCI_RESET_ALL."
> 
>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>   	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>> +		cqhci_deactivate(host->mmc);
>>> +
>>>   	sdhci_reset(host, mask);
>>
>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>> be utilizing since you have access to the host and the mask to make that
>> decision?
> 
> It potentially could.
> 
> I don't know if this is a specified SDHCI behavior that really belongs
> in the common helper, or if this is just a commonly-shared behavior. Per
> the comments I quote above ("if they also have CQHCI reset by
> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
> behavior.
> 
> I suppose it's not all that harmful to do this even if some SDHCI
> controller doesn't have the same behavior/quirk.
> 
> I guess I also don't know if any SDHCI controllers will support command
> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
> set MMC_CAP2_CQE.

SDHCI and CQHCI are separate modules and are not dependent, so they cannot
call into each other directly (and should not).  A new CQE API would be
needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
and wrapped in mmc/host.h:

static inline void mmc_cqe_notify_reset(struct mmc_host *host)
{
	if (host->cqe_ops->cqe_notify_reset)
		host->cqe_ops->cqe_notify_reset(host);
}

Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
although in this case there is so little code it could be static inline and
added in a new include file instead, say sdhci-cqhci.h e.g.

#include "cqhci.h"
#include "sdhci.h"

static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
{
	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
	    host->mmc->cqe_private)
		cqhci_deactivate(host->mmc);
	sdhci_reset(host, mask);
}


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI

[Bough Chen]

> -----Original Message-----
> From: Brian Norris <briannorris@chromium.org>
> Sent: 2022年10月20日 5:55
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <festevam@gmail.com>; Bough Chen
> <haibo.chen@nxp.com>; Broadcom internal kernel review list
> <bcm-kernel-feedback-list@broadcom.com>; dl-linux-imx <linux-imx@nxp.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Florian Fainelli
> <f.fainelli@gmail.com>; Michal Simek <michal.simek@xilinx.com>; Faiz Abbas
> <faiz_abbas@ti.com>; linux-mmc@vger.kernel.org; Jonathan Hunter
> <jonathanh@nvidia.com>; Al Cooper <alcooperx@gmail.com>;
> linux-arm-kernel@lists.infradead.org; Sowjanya Komatineni
> <skomatineni@nvidia.com>; linux-kernel@vger.kernel.org; Thierry Reding
> <thierry.reding@gmail.com>; Adrian Hunter <adrian.hunter@intel.com>;
> Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris
> <briannorris@chromium.org>
> Subject: [PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI
> 
>  [[ NOTE: this is completely untested by the author, but included solely
>     because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
>     SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
>     drivers using CQHCI might benefit from a similar change, if they
>     also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
>     bug on at least MSM, Arasan, and Intel hardware. ]]
> 
> SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
> tracking that properly in software. When out of sync, we may trigger various
> timeouts.
> 
> It's not typical to perform resets while CQE is enabled, but this may occur in
> some suspend or error recovery scenarios.
> 
> Fixes: bb6e358169bf ("mmc: sdhci-esdhc-imx: add CMDQ support")
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

> ---
> 
> Changes in v2:
>  - Drop unnecessary ESDHC_FLAG_CQHCI check
> 
>  drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 55981b0f0b10..c07df7b71b22 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1288,6 +1288,9 @@ static void esdhc_set_uhs_signaling(struct sdhci_host
> *host, unsigned timing)
> 
>  static void esdhc_reset(struct sdhci_host *host, u8 mask)  {
> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask &
> SDHCI_RESET_ALL))
> +		cqhci_deactivate(host->mmc);
> +
>  	sdhci_reset(host, mask);
> 
>  	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> --
> 2.38.0.413.g74048e4d9e-goog

RE: [PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI

[Bough Chen]

> -----Original Message-----
> From: Brian Norris <briannorris@chromium.org>
> Sent: 2022年10月20日 5:55
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <festevam@gmail.com>; Bough Chen
> <haibo.chen@nxp.com>; Broadcom internal kernel review list
> <bcm-kernel-feedback-list@broadcom.com>; dl-linux-imx <linux-imx@nxp.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Florian Fainelli
> <f.fainelli@gmail.com>; Michal Simek <michal.simek@xilinx.com>; Faiz Abbas
> <faiz_abbas@ti.com>; linux-mmc@vger.kernel.org; Jonathan Hunter
> <jonathanh@nvidia.com>; Al Cooper <alcooperx@gmail.com>;
> linux-arm-kernel@lists.infradead.org; Sowjanya Komatineni
> <skomatineni@nvidia.com>; linux-kernel@vger.kernel.org; Thierry Reding
> <thierry.reding@gmail.com>; Adrian Hunter <adrian.hunter@intel.com>;
> Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris
> <briannorris@chromium.org>
> Subject: [PATCH v2 4/7] mms: sdhci-esdhc-imx: Fix SDHCI_RESET_ALL for CQHCI
> 
>  [[ NOTE: this is completely untested by the author, but included solely
>     because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
>     SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
>     drivers using CQHCI might benefit from a similar change, if they
>     also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
>     bug on at least MSM, Arasan, and Intel hardware. ]]
> 
> SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
> tracking that properly in software. When out of sync, we may trigger various
> timeouts.
> 
> It's not typical to perform resets while CQE is enabled, but this may occur in
> some suspend or error recovery scenarios.
> 
> Fixes: bb6e358169bf ("mmc: sdhci-esdhc-imx: add CMDQ support")
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

> ---
> 
> Changes in v2:
>  - Drop unnecessary ESDHC_FLAG_CQHCI check
> 
>  drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 55981b0f0b10..c07df7b71b22 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1288,6 +1288,9 @@ static void esdhc_set_uhs_signaling(struct sdhci_host
> *host, unsigned timing)
> 
>  static void esdhc_reset(struct sdhci_host *host, u8 mask)  {
> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask &
> SDHCI_RESET_ALL))
> +		cqhci_deactivate(host->mmc);
> +
>  	sdhci_reset(host, mask);
> 
>  	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> --
> 2.38.0.413.g74048e4d9e-goog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Florian Fainelli]

On 10/19/22 23:29, Adrian Hunter wrote:
> On 20/10/22 01:19, Brian Norris wrote:
>> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>>> On 10/19/22 14:54, Brian Norris wrote:
>>>> The same bug was already found and fixed for two other drivers, in v5.7
>>>> and v5.9:
>>>>
>>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>>
>>>> The latter is especially prescient, saying "other drivers using CQHCI
>>>> might benefit from a similar change, if they also have CQHCI reset by
>>>> SDHCI_RESET_ALL."
>>
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>>    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>    	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>>> +		cqhci_deactivate(host->mmc);
>>>> +
>>>>    	sdhci_reset(host, mask);
>>>
>>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>>> be utilizing since you have access to the host and the mask to make that
>>> decision?
>>
>> It potentially could.
>>
>> I don't know if this is a specified SDHCI behavior that really belongs
>> in the common helper, or if this is just a commonly-shared behavior. Per
>> the comments I quote above ("if they also have CQHCI reset by
>> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
>> behavior.
>>
>> I suppose it's not all that harmful to do this even if some SDHCI
>> controller doesn't have the same behavior/quirk.
>>
>> I guess I also don't know if any SDHCI controllers will support command
>> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
>> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
>> set MMC_CAP2_CQE.
> 
> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
> call into each other directly (and should not).  A new CQE API would be
> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
> and wrapped in mmc/host.h:
> 
> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
> {
> 	if (host->cqe_ops->cqe_notify_reset)
> 		host->cqe_ops->cqe_notify_reset(host);
> }
> 
> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
> although in this case there is so little code it could be static inline and
> added in a new include file instead, say sdhci-cqhci.h e.g.
> 
> #include "cqhci.h"
> #include "sdhci.h"
> 
> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
> {
> 	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
> 	    host->mmc->cqe_private)
> 		cqhci_deactivate(host->mmc);
> 	sdhci_reset(host, mask);
> }
> 

I like the simplicity of the inline helper, especially towards 
backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate 
that it does both, and does not apply specifically CQHCI that would be 
"embedded" into SDHCI, but your call here.
-- 
Florian

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Florian Fainelli]

On 10/19/22 23:29, Adrian Hunter wrote:
> On 20/10/22 01:19, Brian Norris wrote:
>> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>>> On 10/19/22 14:54, Brian Norris wrote:
>>>> The same bug was already found and fixed for two other drivers, in v5.7
>>>> and v5.9:
>>>>
>>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>>
>>>> The latter is especially prescient, saying "other drivers using CQHCI
>>>> might benefit from a similar change, if they also have CQHCI reset by
>>>> SDHCI_RESET_ALL."
>>
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>>    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>    	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>>> +		cqhci_deactivate(host->mmc);
>>>> +
>>>>    	sdhci_reset(host, mask);
>>>
>>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>>> be utilizing since you have access to the host and the mask to make that
>>> decision?
>>
>> It potentially could.
>>
>> I don't know if this is a specified SDHCI behavior that really belongs
>> in the common helper, or if this is just a commonly-shared behavior. Per
>> the comments I quote above ("if they also have CQHCI reset by
>> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
>> behavior.
>>
>> I suppose it's not all that harmful to do this even if some SDHCI
>> controller doesn't have the same behavior/quirk.
>>
>> I guess I also don't know if any SDHCI controllers will support command
>> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
>> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
>> set MMC_CAP2_CQE.
> 
> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
> call into each other directly (and should not).  A new CQE API would be
> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
> and wrapped in mmc/host.h:
> 
> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
> {
> 	if (host->cqe_ops->cqe_notify_reset)
> 		host->cqe_ops->cqe_notify_reset(host);
> }
> 
> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
> although in this case there is so little code it could be static inline and
> added in a new include file instead, say sdhci-cqhci.h e.g.
> 
> #include "cqhci.h"
> #include "sdhci.h"
> 
> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
> {
> 	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
> 	    host->mmc->cqe_private)
> 		cqhci_deactivate(host->mmc);
> 	sdhci_reset(host, mask);
> }
> 

I like the simplicity of the inline helper, especially towards 
backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate 
that it does both, and does not apply specifically CQHCI that would be 
"embedded" into SDHCI, but your call here.
-- 
Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Adrian Hunter]

On 21/10/22 20:45, Florian Fainelli wrote:
> On 10/19/22 23:29, Adrian Hunter wrote:
>> On 20/10/22 01:19, Brian Norris wrote:
>>> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>>>> On 10/19/22 14:54, Brian Norris wrote:
>>>>> The same bug was already found and fixed for two other drivers, in v5.7
>>>>> and v5.9:
>>>>>
>>>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>>>
>>>>> The latter is especially prescient, saying "other drivers using CQHCI
>>>>> might benefit from a similar change, if they also have CQHCI reset by
>>>>> SDHCI_RESET_ALL."
>>>
>>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>>>        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>        struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>>> +    if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>>>> +        cqhci_deactivate(host->mmc);
>>>>> +
>>>>>        sdhci_reset(host, mask);
>>>>
>>>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>>>> be utilizing since you have access to the host and the mask to make that
>>>> decision?
>>>
>>> It potentially could.
>>>
>>> I don't know if this is a specified SDHCI behavior that really belongs
>>> in the common helper, or if this is just a commonly-shared behavior. Per
>>> the comments I quote above ("if they also have CQHCI reset by
>>> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
>>> behavior.
>>>
>>> I suppose it's not all that harmful to do this even if some SDHCI
>>> controller doesn't have the same behavior/quirk.
>>>
>>> I guess I also don't know if any SDHCI controllers will support command
>>> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
>>> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
>>> set MMC_CAP2_CQE.
>>
>> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
>> call into each other directly (and should not).  A new CQE API would be
>> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
>> and wrapped in mmc/host.h:
>>
>> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
>> {
>>     if (host->cqe_ops->cqe_notify_reset)
>>         host->cqe_ops->cqe_notify_reset(host);
>> }
>>
>> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
>> although in this case there is so little code it could be static inline and
>> added in a new include file instead, say sdhci-cqhci.h e.g.
>>
>> #include "cqhci.h"
>> #include "sdhci.h"
>>
>> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
>> {
>>     if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
>>         host->mmc->cqe_private)
>>         cqhci_deactivate(host->mmc);
>>     sdhci_reset(host, mask);
>> }
>>
> 
> I like the simplicity of the inline helper, especially towards backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate that it does both, and does not apply specifically CQHCI that would be "embedded" into SDHCI, but your call here.

sdhci_and_cqhci_reset() is fine by me

Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Adrian Hunter]

On 21/10/22 20:45, Florian Fainelli wrote:
> On 10/19/22 23:29, Adrian Hunter wrote:
>> On 20/10/22 01:19, Brian Norris wrote:
>>> On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
>>>> On 10/19/22 14:54, Brian Norris wrote:
>>>>> The same bug was already found and fixed for two other drivers, in v5.7
>>>>> and v5.9:
>>>>>
>>>>> 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
>>>>> df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
>>>>>
>>>>> The latter is especially prescient, saying "other drivers using CQHCI
>>>>> might benefit from a similar change, if they also have CQHCI reset by
>>>>> SDHCI_RESET_ALL."
>>>
>>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>>> @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>>>>>        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>        struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>>> +    if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
>>>>> +        cqhci_deactivate(host->mmc);
>>>>> +
>>>>>        sdhci_reset(host, mask);
>>>>
>>>> Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
>>>> be utilizing since you have access to the host and the mask to make that
>>>> decision?
>>>
>>> It potentially could.
>>>
>>> I don't know if this is a specified SDHCI behavior that really belongs
>>> in the common helper, or if this is just a commonly-shared behavior. Per
>>> the comments I quote above ("if they also have CQHCI reset by
>>> SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
>>> behavior.
>>>
>>> I suppose it's not all that harmful to do this even if some SDHCI
>>> controller doesn't have the same behavior/quirk.
>>>
>>> I guess I also don't know if any SDHCI controllers will support command
>>> queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
>>> CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
>>> set MMC_CAP2_CQE.
>>
>> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
>> call into each other directly (and should not).  A new CQE API would be
>> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
>> and wrapped in mmc/host.h:
>>
>> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
>> {
>>     if (host->cqe_ops->cqe_notify_reset)
>>         host->cqe_ops->cqe_notify_reset(host);
>> }
>>
>> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
>> although in this case there is so little code it could be static inline and
>> added in a new include file instead, say sdhci-cqhci.h e.g.
>>
>> #include "cqhci.h"
>> #include "sdhci.h"
>>
>> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
>> {
>>     if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
>>         host->mmc->cqe_private)
>>         cqhci_deactivate(host->mmc);
>>     sdhci_reset(host, mask);
>> }
>>
> 
> I like the simplicity of the inline helper, especially towards backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate that it does both, and does not apply specifically CQHCI that would be "embedded" into SDHCI, but your call here.

sdhci_and_cqhci_reset() is fine by me


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel