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

[Florian Fainelli]

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.
--
Florian