| Message ID | 20221024105229.v3.6.I35ca9d6220ba48304438b992a76647ca8e5b126f@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI | expand |
On 24/10/22 20:55, 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. > > Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E") > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > > Changes in v3: > - Use new SDHCI+CQHCI helper > > drivers/mmc/host/sdhci_am654.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 8f1023480e12..6a282c7a221e 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); What about sdhci_reset in sdhci_am654_ops ?
On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > On 24/10/22 20:55, Brian Norris wrote: > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > > index 8f1023480e12..6a282c7a221e 100644 > > --- a/drivers/mmc/host/sdhci_am654.c > > +++ b/drivers/mmc/host/sdhci_am654.c > > @@ -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); > > What about sdhci_reset in sdhci_am654_ops ? Oops, I think you caught a big fallacy in some of my patches: I assumed there was a single reset() implementation in a given driver (an unwise assumption, I realize). I see at least sdhci-brcmstb.c also has several variant ops that call sdhci_reset(), and I should probably convert them too. I'll take another pass through the series for v4, fixing this and the other smaller cosmetic issues. I may retain some Acks, depending on whether I think the changes are substantial. Thanks, Brian
On 10/25/22 14:45, Brian Norris wrote: > On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >> On 24/10/22 20:55, Brian Norris wrote: >>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>> index 8f1023480e12..6a282c7a221e 100644 >>> --- a/drivers/mmc/host/sdhci_am654.c >>> +++ b/drivers/mmc/host/sdhci_am654.c > >>> @@ -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); >> >> What about sdhci_reset in sdhci_am654_ops ? > > Oops, I think you caught a big fallacy in some of my patches: I assumed > there was a single reset() implementation in a given driver (an unwise > assumption, I realize). I see at least sdhci-brcmstb.c also has several > variant ops that call sdhci_reset(), and I should probably convert them > too. You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible which implies using brcmstb_reset().
On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: > On 10/25/22 14:45, Brian Norris wrote: > > On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > > > On 24/10/22 20:55, Brian Norris wrote: > > > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > > > > index 8f1023480e12..6a282c7a221e 100644 > > > > --- a/drivers/mmc/host/sdhci_am654.c > > > > +++ b/drivers/mmc/host/sdhci_am654.c > > > > > > @@ -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); > > > > > > What about sdhci_reset in sdhci_am654_ops ? > > > > Oops, I think you caught a big fallacy in some of my patches: I assumed > > there was a single reset() implementation in a given driver (an unwise > > assumption, I realize). I see at least sdhci-brcmstb.c also has several > > variant ops that call sdhci_reset(), and I should probably convert them > > too. > > You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the > enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible > which implies using brcmstb_reset(). I don't see any in-tree device trees for these chips (which is OK), and that's not what the Documentation/ says, and AFAICT nothing in the driver is limiting other variants from specifying the "supports-cqe" flag in their (out-of-tree) device tree. The closest thing I see is that an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I missing something? Now of course, you probably know behind the scenes that there are no other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT I have no way of knowing that a priori. The driver and bindings give (too much?) flexibility. Poking around, I think the only other one I might have missed would be gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is otherwise relying on the default sdhci_pci_ops. So I'd either have to change the common sdhci_pci_ops, or else start a new copy/paste/modify 'struct sdhci_ops' for it... This really does start to get messy when poking around on drivers I can't test. As in, it shouldn't be harmful to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they aren't using some other CQE implementation), but the more invasive it gets (say, rewriting a bunch of other ops), the easier it is to get something wrong. Thoughts welcome. Brian
On 10/25/22 15:26, Brian Norris wrote: > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >> On 10/25/22 14:45, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>> On 24/10/22 20:55, Brian Norris wrote: >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>> >>>>> @@ -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); >>>> >>>> What about sdhci_reset in sdhci_am654_ops ? >>> >>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>> there was a single reset() implementation in a given driver (an unwise >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>> variant ops that call sdhci_reset(), and I should probably convert them >>> too. >> >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >> which implies using brcmstb_reset(). > > I don't see any in-tree device trees for these chips (which is OK), and > that's not what the Documentation/ says, and AFAICT nothing in the > driver is limiting other variants from specifying the "supports-cqe" > flag in their (out-of-tree) device tree. The closest thing I see is that > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > missing something? > > Now of course, you probably know behind the scenes that there are no > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > I have no way of knowing that a priori. The driver and bindings give > (too much?) flexibility. Yes that is fair enough, I will amend the binding document to make it clearer that 'supports-cqe' only applies in case of "brcm,bcm7216-sdhci" and not for other compatibles.
On 26/10/22 01:26, Brian Norris wrote: > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >> On 10/25/22 14:45, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>> On 24/10/22 20:55, Brian Norris wrote: >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>> >>>>> @@ -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); >>>> >>>> What about sdhci_reset in sdhci_am654_ops ? >>> >>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>> there was a single reset() implementation in a given driver (an unwise >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>> variant ops that call sdhci_reset(), and I should probably convert them >>> too. I checked and found only sdhci_am654_ops >> >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >> which implies using brcmstb_reset(). > > I don't see any in-tree device trees for these chips (which is OK), and > that's not what the Documentation/ says, and AFAICT nothing in the > driver is limiting other variants from specifying the "supports-cqe" > flag in their (out-of-tree) device tree. The closest thing I see is that > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > missing something? It was mentioned in the patch from the Fixes tag. > > Now of course, you probably know behind the scenes that there are no > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > I have no way of knowing that a priori. The driver and bindings give > (too much?) flexibility. > > Poking around, I think the only other one I might have missed would be > gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is > otherwise relying on the default sdhci_pci_ops. So I'd either have to It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK to me. > change the common sdhci_pci_ops, or else start a new copy/paste/modify > 'struct sdhci_ops' for it... This really does start to get messy when > poking around on drivers I can't test. As in, it shouldn't be harmful > to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they > aren't using some other CQE implementation), but the more invasive it > gets (say, rewriting a bunch of other ops), the easier it is to get > something wrong. AFAICS it was just sdhci_am654_ops
Hi Adrian, On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: > On 26/10/22 01:26, Brian Norris wrote: > > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: > >> On 10/25/22 14:45, Brian Norris wrote: > >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > >>>> On 24/10/22 20:55, Brian Norris wrote: > >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > >>>>> index 8f1023480e12..6a282c7a221e 100644 > >>>>> --- a/drivers/mmc/host/sdhci_am654.c > >>>>> +++ b/drivers/mmc/host/sdhci_am654.c > >>> > >>>>> @@ -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); > >>>> > >>>> What about sdhci_reset in sdhci_am654_ops ? > >>> > >>> Oops, I think you caught a big fallacy in some of my patches: I assumed > >>> there was a single reset() implementation in a given driver (an unwise > >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several > >>> variant ops that call sdhci_reset(), and I should probably convert them > >>> too. > > I checked and found only sdhci_am654_ops And...how about sdhci_j721e_8bit_ops in that same driver? > >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the > >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible > >> which implies using brcmstb_reset(). > > > > I don't see any in-tree device trees for these chips (which is OK), and > > that's not what the Documentation/ says, and AFAICT nothing in the > > driver is limiting other variants from specifying the "supports-cqe" > > flag in their (out-of-tree) device tree. The closest thing I see is that > > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > > missing something? > > It was mentioned in the patch from the Fixes tag. OK, good note. If I don't patch the other seemingly-unaffected variants in brcmstb, I'll at least update the commit message, since the code doesn't tell me they're unaffected. > > Now of course, you probably know behind the scenes that there are no > > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > > I have no way of knowing that a priori. The driver and bindings give > > (too much?) flexibility. > > > > Poking around, I think the only other one I might have missed would be > > gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is > > otherwise relying on the default sdhci_pci_ops. So I'd either have to > > It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK > to me. Ugh, of course you're right. I think I'm mixing up past history and stuff I'm trying to patch now. I *am* patching gl9763e already in this series, but simply as a refactor, and not any additional bugfix. > > change the common sdhci_pci_ops, or else start a new copy/paste/modify > > 'struct sdhci_ops' for it... This really does start to get messy when > > poking around on drivers I can't test. As in, it shouldn't be harmful > > to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they > > aren't using some other CQE implementation), but the more invasive it > > gets (say, rewriting a bunch of other ops), the easier it is to get > > something wrong. > > AFAICS it was just sdhci_am654_ops Agreed it's less to change than I thought. But I think you (and I) also missed sdhci_j721e_8bit_ops. Assuming I'm not totally off-base yet again...v4 is coming sooner or later. Brian
On 10/26/22 11:18, Brian Norris wrote: > Hi Adrian, > > On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: >> On 26/10/22 01:26, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >>>> On 10/25/22 14:45, Brian Norris wrote: >>>>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>>>> On 24/10/22 20:55, Brian Norris wrote: >>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>>>> >>>>>>> @@ -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); >>>>>> >>>>>> What about sdhci_reset in sdhci_am654_ops ? >>>>> >>>>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>>>> there was a single reset() implementation in a given driver (an unwise >>>>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>>>> variant ops that call sdhci_reset(), and I should probably convert them >>>>> too. >> >> I checked and found only sdhci_am654_ops > > And...how about sdhci_j721e_8bit_ops in that same driver? > >>>> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >>>> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >>>> which implies using brcmstb_reset(). >>> >>> I don't see any in-tree device trees for these chips (which is OK), and >>> that's not what the Documentation/ says, and AFAICT nothing in the >>> driver is limiting other variants from specifying the "supports-cqe" >>> flag in their (out-of-tree) device tree. The closest thing I see is that >>> an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on >>> brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I >>> missing something? >> >> It was mentioned in the patch from the Fixes tag. > > OK, good note. If I don't patch the other seemingly-unaffected variants > in brcmstb, I'll at least update the commit message, since the code > doesn't tell me they're unaffected. You can mention in the commit message that they are unaffected and quote me on that if you feel like this needs to be explicitly said.
On 26/10/22 21:18, Brian Norris wrote: > Hi Adrian, > > On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: >> On 26/10/22 01:26, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >>>> On 10/25/22 14:45, Brian Norris wrote: >>>>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>>>> On 24/10/22 20:55, Brian Norris wrote: >>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>>>> >>>>>>> @@ -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); >>>>>> >>>>>> What about sdhci_reset in sdhci_am654_ops ? >>>>> >>>>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>>>> there was a single reset() implementation in a given driver (an unwise >>>>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>>>> variant ops that call sdhci_reset(), and I should probably convert them >>>>> too. >> >> I checked and found only sdhci_am654_ops > > And...how about sdhci_j721e_8bit_ops in that same driver? > >>>> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >>>> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >>>> which implies using brcmstb_reset(). >>> >>> I don't see any in-tree device trees for these chips (which is OK), and >>> that's not what the Documentation/ says, and AFAICT nothing in the >>> driver is limiting other variants from specifying the "supports-cqe" >>> flag in their (out-of-tree) device tree. The closest thing I see is that >>> an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on >>> brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I >>> missing something? >> >> It was mentioned in the patch from the Fixes tag. > > OK, good note. If I don't patch the other seemingly-unaffected variants > in brcmstb, I'll at least update the commit message, since the code > doesn't tell me they're unaffected. > >>> Now of course, you probably know behind the scenes that there are no >>> other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT >>> I have no way of knowing that a priori. The driver and bindings give >>> (too much?) flexibility. >>> >>> Poking around, I think the only other one I might have missed would be >>> gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is >>> otherwise relying on the default sdhci_pci_ops. So I'd either have to >> >> It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK >> to me. > > Ugh, of course you're right. I think I'm mixing up past history and > stuff I'm trying to patch now. I *am* patching gl9763e already in this > series, but simply as a refactor, and not any additional bugfix. > >>> change the common sdhci_pci_ops, or else start a new copy/paste/modify >>> 'struct sdhci_ops' for it... This really does start to get messy when >>> poking around on drivers I can't test. As in, it shouldn't be harmful >>> to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they >>> aren't using some other CQE implementation), but the more invasive it >>> gets (say, rewriting a bunch of other ops), the easier it is to get >>> something wrong. >> >> AFAICS it was just sdhci_am654_ops > > Agreed it's less to change than I thought. But I think you (and I) also > missed sdhci_j721e_8bit_ops. You are right! Thanks for spotting that! > > Assuming I'm not totally off-base yet again...v4 is coming sooner or > later. > > Brian
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 8f1023480e12..6a282c7a221e 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);
[[ 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. Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E") Signed-off-by: Brian Norris <briannorris@chromium.org> --- Changes in v3: - Use new SDHCI+CQHCI helper drivers/mmc/host/sdhci_am654.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)