Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI

[Florian Fainelli]

On 10/19/22 23:29, Adrian Hunter wrote:
> On 20/10/22 01:19, Brian Norris wrote:
> > On Wed, Oct 19, 2022 at 02:59:39PM -0700, Florian Fainelli wrote:
> > > On 10/19/22 14:54, Brian Norris wrote:
> > > > The same bug was already found and fixed for two other drivers, in v5.7
> > > > and v5.9:
> > > >
> > > > 5cf583f1fb9c mmc: sdhci-msm: Deactivate CQE during SDHC reset
> > > > df57d73276b8 mmc: sdhci-pci: Fix SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers
> > > >
> > > > The latter is especially prescient, saying "other drivers using CQHCI
> > > > might benefit from a similar change, if they also have CQHCI reset by
> > > > SDHCI_RESET_ALL."
> >
> > > > --- a/drivers/mmc/host/sdhci-of-arasan.c
> > > > +++ b/drivers/mmc/host/sdhci-of-arasan.c
> > > > @@ -366,6 +366,9 @@ static void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
> > > >    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > >    	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> > > > +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> > > > +		cqhci_deactivate(host->mmc);
> > > > +
> > > >    	sdhci_reset(host, mask);
> > >
> > > Cannot this be absorbed by sdhci_reset() that all of these drivers appear to
> > > be utilizing since you have access to the host and the mask to make that
> > > decision?
> >
> > It potentially could.
> >
> > I don't know if this is a specified SDHCI behavior that really belongs
> > in the common helper, or if this is just a commonly-shared behavior. Per
> > the comments I quote above ("if they also have CQHCI reset by
> > SDHCI_RESET_ALL"), I chose to leave that as an implementation-specific
> > behavior.
> >
> > I suppose it's not all that harmful to do this even if some SDHCI
> > controller doesn't have the same behavior/quirk.
> >
> > I guess I also don't know if any SDHCI controllers will support command
> > queueing (MMC_CAP2_CQE) via somethings *besides* CQHCI. I see
> > CQE support in sdhci-sprd.c without CQHCI, although that driver doesn't
> > set MMC_CAP2_CQE.
>
> SDHCI and CQHCI are separate modules and are not dependent, so they cannot
> call into each other directly (and should not).  A new CQE API would be
> needed in mmc_cqe_ops e.g. (*cqe_notify_reset)(struct mmc_host *host),
> and wrapped in mmc/host.h:
>
> static inline void mmc_cqe_notify_reset(struct mmc_host *host)
> {
> 	if (host->cqe_ops->cqe_notify_reset)
> 		host->cqe_ops->cqe_notify_reset(host);
> }
>
> Alternatively, you could make a new module for SDHCI/CQHCI helper functions,
> although in this case there is so little code it could be static inline and
> added in a new include file instead, say sdhci-cqhci.h e.g.
>
> #include "cqhci.h"
> #include "sdhci.h"
>
> static inline void sdhci_cqhci_reset(struct sdhci_host *host, u8 mask)
> {
> 	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL) &&
> 	    host->mmc->cqe_private)
> 		cqhci_deactivate(host->mmc);
> 	sdhci_reset(host, mask);
> }

I like the simplicity of the inline helper, especially towards 
backports. May suggest to name it sdhci_and_cqhci_reset() to illustrate 
that it does both, and does not apply specifically CQHCI that would be 
"embedded" into SDHCI, but your call here.
--
Florian