diff mbox series

[v3,6/7] mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI

Message ID 20221024105229.v3.6.I35ca9d6220ba48304438b992a76647ca8e5b126f@changeid
State New
Headers
Series mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI |

Commit Message

Brian Norris Oct. 24, 2022, 5:55 p.m. UTC 
  [[ 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(-)

Comments

Adrian Hunter Oct. 25, 2022, 1:10 p.m. UTC | #1 
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 ?
Brian Norris Oct. 25, 2022, 9:45 p.m. UTC | #2 
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
Florian Fainelli Oct. 25, 2022, 9:53 p.m. UTC | #3 
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().
Brian Norris Oct. 25, 2022, 10:26 p.m. UTC | #4 
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
Florian Fainelli Oct. 25, 2022, 11:29 p.m. UTC | #5 
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.
Adrian Hunter Oct. 26, 2022, 5:36 a.m. UTC | #6 
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
Brian Norris Oct. 26, 2022, 6:18 p.m. UTC | #7 
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
Florian Fainelli Oct. 26, 2022, 6:23 p.m. UTC | #8 
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.
Adrian Hunter Oct. 27, 2022, 5:45 a.m. UTC | #9 
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 mbox series

Patch

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);