This is a series of 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. So the
fix is now factored into a separate sdhci_and_cqhci_reset() helper,
and it's likely that most/all drivers that support a combo SDHCI/CQHCI
controller will want to use it.
Thus, I include additional patches (compile-tested only) that apply this
helper/fix 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 v4:
- Improve for-stable cherry-picking notes
- Add Adrian's Ack
- Also fix sdhci_am654_ops, sdhci_j721e_8bit_ops
Changes in v3:
- Refactor to a "SDHCI and CQHCI" helper -- sdhci_and_cqhci_reset()
Changes in v2:
- Rely on cqhci_deactivate() to safely handle (ignore)
not-yet-initialized CQE support
Brian Norris (7):
mmc: cqhci: Provide helper for resetting both SDHCI and CQHCI
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-*: Convert drivers to new sdhci_and_cqhci_reset()
drivers/mmc/host/sdhci-brcmstb.c | 3 ++-
drivers/mmc/host/sdhci-cqhci.h | 24 ++++++++++++++++++++++++
drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
drivers/mmc/host/sdhci-msm.c | 10 ++--------
drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
drivers/mmc/host/sdhci-pci-core.c | 11 ++---------
drivers/mmc/host/sdhci-pci-gli.c | 11 ++---------
drivers/mmc/host/sdhci-tegra.c | 3 ++-
drivers/mmc/host/sdhci_am654.c | 7 ++++---
9 files changed, 42 insertions(+), 33 deletions(-)
create mode 100644 drivers/mmc/host/sdhci-cqhci.h
--
2.38.0.135.g90850a2211-goog
[[ 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.
Include this fix by way of the new sdhci_and_cqhci_reset() helper.
This patch depends on (and should not compile without) the patch
entitled "mmc: cqhci: Provide helper for resetting both SDHCI and
CQHCI".
Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E")
Signed-off-by: Brian Norris <[email protected]>
---
Changes in v4:
- Also fix sdhci_am654_ops, sdhci_j721e_8bit_ops
- Add dependency notes
- Drop bouncing Faiz Abbas <[email protected]> address
Changes in v3:
- Use new SDHCI+CQHCI helper
drivers/mmc/host/sdhci_am654.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 8f1023480e12..c2333c7acac9 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -15,6 +15,7 @@
#include <linux/sys_soc.h>
#include "cqhci.h"
+#include "sdhci-cqhci.h"
#include "sdhci-pltfm.h"
/* CTL_CFG Registers */
@@ -378,7 +379,7 @@ 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);
- sdhci_reset(host, mask);
+ sdhci_and_cqhci_reset(host, mask);
if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
@@ -464,7 +465,7 @@ static struct sdhci_ops sdhci_am654_ops = {
.set_clock = sdhci_am654_set_clock,
.write_b = sdhci_am654_write_b,
.irq = sdhci_am654_cqhci_irq,
- .reset = sdhci_reset,
+ .reset = sdhci_and_cqhci_reset,
};
static const struct sdhci_pltfm_data sdhci_am654_pdata = {
@@ -494,7 +495,7 @@ static struct sdhci_ops sdhci_j721e_8bit_ops = {
.set_clock = sdhci_am654_set_clock,
.write_b = sdhci_am654_write_b,
.irq = sdhci_am654_cqhci_irq,
- .reset = sdhci_reset,
+ .reset = sdhci_and_cqhci_reset,
};
static const struct sdhci_pltfm_data sdhci_j721e_8bit_pdata = {
--
2.38.0.135.g90850a2211-goog
[[ 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.
Include this fix by way of the new sdhci_and_cqhci_reset() helper.
I only patch the bcm7216 variant even though others potentially *could*
provide the 'supports-cqe' property (and thus enable CQHCI), because
d46ba2d17f90 ("mmc: sdhci-brcmstb: Add support for Command Queuing
(CQE)") and some Broadcom folks confirm that only the 7216 variant
actually supports it.
This patch depends on (and should not compile without) the patch
entitled "mmc: cqhci: Provide helper for resetting both SDHCI and
CQHCI".
Fixes: d46ba2d17f90 ("mmc: sdhci-brcmstb: Add support for Command Queuing (CQE)")
Signed-off-by: Brian Norris <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
---
Changes in v4:
- Improve commit notes
- Add Adrian's Ack
- Add Florian's Reviewed-by
Changes in v3:
- Use new SDHCI+CQHCI helper
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, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index aff36a933ebe..55d8bd232695 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/delay.h>
+#include "sdhci-cqhci.h"
#include "sdhci-pltfm.h"
#include "cqhci.h"
@@ -55,7 +56,7 @@ 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);
- sdhci_reset(host, mask);
+ sdhci_and_cqhci_reset(host, mask);
/* Reset will clear this, so re-enable it */
if (priv->flags & BRCMSTB_PRIV_FLAGS_GATE_CLOCK)
--
2.38.0.135.g90850a2211-goog
[[ 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.
Include this fix by way of the new sdhci_and_cqhci_reset() helper.
This patch depends on (and should not compile without) the patch
entitled "mmc: cqhci: Provide helper for resetting both SDHCI and
CQHCI".
Fixes: 3c4019f97978 ("mmc: tegra: HW Command Queue Support for Tegra SDMMC")
Signed-off-by: Brian Norris <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
---
Changes in v4:
- Add dependency notes
- Add Adrian's Ack
Changes in v3:
- Use new SDHCI+CQHCI helper
Changes in v2:
- Drop unnecessary 'enable_hwcq' check
drivers/mmc/host/sdhci-tegra.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 413925bce0ca..c71000a07656 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -28,6 +28,7 @@
#include <soc/tegra/common.h>
+#include "sdhci-cqhci.h"
#include "sdhci-pltfm.h"
#include "cqhci.h"
@@ -367,7 +368,7 @@ 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;
- sdhci_reset(host, mask);
+ sdhci_and_cqhci_reset(host, mask);
if (!(mask & SDHCI_RESET_ALL))
return;
--
2.38.0.135.g90850a2211-goog
On 26/10/22 22:42, Brian Norris wrote:
> [[ 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.
>
> Include this fix by way of the new sdhci_and_cqhci_reset() helper.
>
> This patch depends on (and should not compile without) the patch
> entitled "mmc: cqhci: Provide helper for resetting both SDHCI and
> CQHCI".
>
> Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E")
> Signed-off-by: Brian Norris <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
> ---
>
> Changes in v4:
> - Also fix sdhci_am654_ops, sdhci_j721e_8bit_ops
> - Add dependency notes
> - Drop bouncing Faiz Abbas <[email protected]> address
>
> Changes in v3:
> - Use new SDHCI+CQHCI helper
>
> drivers/mmc/host/sdhci_am654.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 8f1023480e12..c2333c7acac9 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -15,6 +15,7 @@
> #include <linux/sys_soc.h>
>
> #include "cqhci.h"
> +#include "sdhci-cqhci.h"
> #include "sdhci-pltfm.h"
>
> /* CTL_CFG Registers */
> @@ -378,7 +379,7 @@ 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);
>
> - sdhci_reset(host, mask);
> + sdhci_and_cqhci_reset(host, mask);
>
> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> @@ -464,7 +465,7 @@ static struct sdhci_ops sdhci_am654_ops = {
> .set_clock = sdhci_am654_set_clock,
> .write_b = sdhci_am654_write_b,
> .irq = sdhci_am654_cqhci_irq,
> - .reset = sdhci_reset,
> + .reset = sdhci_and_cqhci_reset,
> };
>
> static const struct sdhci_pltfm_data sdhci_am654_pdata = {
> @@ -494,7 +495,7 @@ static struct sdhci_ops sdhci_j721e_8bit_ops = {
> .set_clock = sdhci_am654_set_clock,
> .write_b = sdhci_am654_write_b,
> .irq = sdhci_am654_cqhci_irq,
> - .reset = sdhci_reset,
> + .reset = sdhci_and_cqhci_reset,
> };
>
> static const struct sdhci_pltfm_data sdhci_j721e_8bit_pdata = {
On Wed, 26 Oct 2022 at 21:42, Brian Norris <[email protected]> wrote:
>
> This is a series of 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. So the
> fix is now factored into a separate sdhci_and_cqhci_reset() helper,
> and it's likely that most/all drivers that support a combo SDHCI/CQHCI
> controller will want to use it.
>
> Thus, I include additional patches (compile-tested only) that apply this
> helper/fix 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 v4:
> - Improve for-stable cherry-picking notes
> - Add Adrian's Ack
> - Also fix sdhci_am654_ops, sdhci_j721e_8bit_ops
>
> Changes in v3:
> - Refactor to a "SDHCI and CQHCI" helper -- sdhci_and_cqhci_reset()
>
> Changes in v2:
> - Rely on cqhci_deactivate() to safely handle (ignore)
> not-yet-initialized CQE support
>
> Brian Norris (7):
> mmc: cqhci: Provide helper for resetting both SDHCI and CQHCI
> 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-*: Convert drivers to new sdhci_and_cqhci_reset()
>
> drivers/mmc/host/sdhci-brcmstb.c | 3 ++-
> drivers/mmc/host/sdhci-cqhci.h | 24 ++++++++++++++++++++++++
> drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
> drivers/mmc/host/sdhci-msm.c | 10 ++--------
> drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
> drivers/mmc/host/sdhci-pci-core.c | 11 ++---------
> drivers/mmc/host/sdhci-pci-gli.c | 11 ++---------
> drivers/mmc/host/sdhci-tegra.c | 3 ++-
> drivers/mmc/host/sdhci_am654.c | 7 ++++---
> 9 files changed, 42 insertions(+), 33 deletions(-)
> create mode 100644 drivers/mmc/host/sdhci-cqhci.h
>
Patch1 -> patch6, applied for fixes and by adding stable tags. Patch7
applied for next.
Thanks and kind regards
Uffe