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

[Brian Norris]

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