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

From: Brian Norris <briannorris@chromium.org>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Shawn Guo <shawnguo@kernel.org>,
	Fabio Estevam <festevam@gmail.com>,
	Haibo Chen <haibo.chen@nxp.com>,
	Broadcom internal kernel review list 
	<bcm-kernel-feedback-list@broadcom.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Michal Simek <michal.simek@xilinx.com>,
	Faiz Abbas <faiz_abbas@ti.com>,
	linux-mmc@vger.kernel.org, Jonathan Hunter <jonathanh@nvidia.com>,
	Al Cooper <alcooperx@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	Sowjanya Komatineni <skomatineni@nvidia.com>,
	linux-kernel@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 2/7] mmc: sdhci-of-arasan: Fix SDHCI_RESET_ALL for CQHCI
Date: Wed, 19 Oct 2022 15:19:20 -0700	[thread overview]
Message-ID: <Y1B36AnqJtolGQEP@google.com> (raw)
In-Reply-To: <14efb3e6-96cf-f42e-16aa-c45001ec632e@gmail.com>

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.

Brian