Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Oct 2022 12:52:46 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Adrian Hunter <adrian.hunter@intel.com>,
        Prathamesh Shete <pshete@nvidia.com>
Cc:     ulf.hansson@linaro.org, jonathanh@nvidia.com,
        p.zabel@pengutronix.de, linux-mmc@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        anrao@nvidia.com, smangipudi@nvidia.com, kyarlagadda@nvidia.com
Subject: Re: [PATCH v6 2/4] mmc: sdhci-tegra: Add support to program MC
 stream ID
Message-ID: <Yz6zfrVq9cP/wrJb@orome>
References: <a5c231e8-f28a-e692-5961-58e6838711ed@intel.com>
 <20220928125648.19636-1-pshete@nvidia.com>
 <20220928125648.19636-2-pshete@nvidia.com>
 <34f002a4-b811-106b-52b5-6041674b15e9@intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="GYjMp6IFoAg2M0P0"
Content-Disposition: inline
In-Reply-To: <34f002a4-b811-106b-52b5-6041674b15e9@intel.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk


--GYjMp6IFoAg2M0P0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 28, 2022 at 05:18:19PM +0300, Adrian Hunter wrote:
> On 28/09/22 15:56, Prathamesh Shete wrote:
> > SMMU clients are supposed to program stream ID from
> > their respective address spaces instead of MC override.
> > Define NVQUIRK_PROGRAM_STREAMID and use it to program
> > SMMU stream ID from the SDMMC client address space.
> >=20
> > Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
>=20
> One observation below.
>=20
> Nevertheless:
>=20
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>=20
> > ---
> >  drivers/mmc/host/sdhci-tegra.c | 44 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >=20
> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-te=
gra.c
> > index a6c5bbae77b4..60ce3e80f248 100644
> > --- a/drivers/mmc/host/sdhci-tegra.c
> > +++ b/drivers/mmc/host/sdhci-tegra.c
> > @@ -25,6 +25,10 @@
> >  #include <linux/mmc/slot-gpio.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/ktime.h>
> > +#ifdef CONFIG_IOMMU_API
> > +#include <linux/iommu.h>
> > +#include <linux/bitops.h>
> > +#endif
> > =20
> >  #include <soc/tegra/common.h>
> > =20
> > @@ -94,6 +98,8 @@
> >  #define SDHCI_TEGRA_AUTO_CAL_STATUS			0x1ec
> >  #define SDHCI_TEGRA_AUTO_CAL_ACTIVE			BIT(31)
> > =20
> > +#define SDHCI_TEGRA_CIF2AXI_CTRL_0			0x1fc
> > +
> >  #define NVQUIRK_FORCE_SDHCI_SPEC_200			BIT(0)
> >  #define NVQUIRK_ENABLE_BLOCK_GAP_DET			BIT(1)
> >  #define NVQUIRK_ENABLE_SDHCI_SPEC_300			BIT(2)
> > @@ -121,6 +127,7 @@
> >  #define NVQUIRK_HAS_TMCLK				BIT(10)
> > =20
> >  #define NVQUIRK_HAS_ANDROID_GPT_SECTOR			BIT(11)
> > +#define NVQUIRK_PROGRAM_STREAMID			BIT(12)
> > =20
> >  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> >  #define SDHCI_TEGRA_CQE_BASE_ADDR			0xF000
> > @@ -177,6 +184,9 @@ struct sdhci_tegra {
> >  	bool enable_hwcq;
> >  	unsigned long curr_clk_rate;
> >  	u8 tuned_tap_delay;
> > +#ifdef CONFIG_IOMMU_API
> > +	u32 streamid;
> > +#endif
> >  };
> > =20
> >  static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> > @@ -1564,6 +1574,7 @@ static const struct sdhci_tegra_soc_data soc_data=
_tegra234 =3D {
> >  		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
> >  		    NVQUIRK_ENABLE_SDR50 |
> >  		    NVQUIRK_ENABLE_SDR104 |
> > +		    NVQUIRK_PROGRAM_STREAMID |
> >  		    NVQUIRK_HAS_TMCLK,
> >  	.min_tap_delay =3D 95,
> >  	.max_tap_delay =3D 111,
> > @@ -1775,6 +1786,25 @@ static int sdhci_tegra_probe(struct platform_dev=
ice *pdev)
> >  	if (rc)
> >  		goto err_add_host;
> > =20
> > +	/* Program MC streamID for DMA transfers */
> > +#ifdef CONFIG_IOMMU_API
> > +	if (soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
> > +		struct iommu_fwspec *fwspec;
> > +
> > +		fwspec =3D dev_iommu_fwspec_get(&pdev->dev);
> > +		if (fwspec =3D=3D NULL) {
> > +			dev_warn(mmc_dev(host->mmc),
> > +				"iommu fwspec is NULL, continue without stream ID\n");
>=20
> It will still program a zero streamid upon resume.

This was confusing to me, but I think what you mean is that the check
for fwspec =3D=3D NULL should also be done in sdhci_tegra_resume().

>=20
> > +		} else {
> > +			tegra_host->streamid =3D fwspec->ids[0] & 0xff;
> > +			tegra_sdhci_writel(host, tegra_host->streamid |
> > +						FIELD_PREP(GENMASK(15, 8),
> > +						tegra_host->streamid),
> > +						SDHCI_TEGRA_CIF2AXI_CTRL_0);
> > +		}
> > +	}
> > +#endif
> > +
> >  	return 0;
> > =20
> >  err_add_host:
> > @@ -1861,6 +1891,10 @@ static int sdhci_tegra_suspend(struct device *de=
v)
> >  static int sdhci_tegra_resume(struct device *dev)
> >  {
> >  	struct sdhci_host *host =3D dev_get_drvdata(dev);
> > +#ifdef CONFIG_IOMMU_API
> > +	struct sdhci_pltfm_host *pltfm_host =3D sdhci_priv(host);
> > +	struct sdhci_tegra *tegra_host =3D sdhci_pltfm_priv(pltfm_host);
> > +#endif
> >  	int ret;
> > =20
> >  	ret =3D mmc_gpio_set_cd_wake(host->mmc, false);
> > @@ -1871,6 +1905,16 @@ static int sdhci_tegra_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> > =20
> > +	/* Re-program MC streamID for DMA transfers */
> > +#ifdef CONFIG_IOMMU_API
> > +	if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
> > +		tegra_sdhci_writel(host, tegra_host->streamid |
> > +					FIELD_PREP(GENMASK(15, 8),
> > +					tegra_host->streamid),
> > +					SDHCI_TEGRA_CIF2AXI_CTRL_0);
> > +	}
> > +#endif

So this here should be exactly the same as in sdhci_tegra_probe(). Which
would be an argument for putting this into a separate function.

That said, I'm not sure if this matters at all. If fwspec =3D=3D NULL, that
implies that device tree didn't have an "iommus" property, which also
implies that SMMU translations for this device will not be enabled at
all, if I recall correctly, so whether this CIF2AXI_CTRL register is
programmed or not in that case should be irrelevant.

Still, might be good to keep this the same in both sdhci_tegra_probe()
and sdhci_tegra_resume() for consistency.

With that fixed, the series:

Acked-by: Thierry Reding <treding@nvidia.com>

--GYjMp6IFoAg2M0P0
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmM+s3wACgkQ3SOs138+
s6EQeg//bDHPfg9KdsFStVnvP7sP/AtLWVOwl08YsFNGLAb1U6hT+lLyVkXPBw/K
zfWcP6Mg2r3mlpqRCN2rlA2B1XYPg9OIkVetZ89rOyIeOV/n/9oRbjlzqYSPqwL4
e0SNOIwGWPI84LYgSkHXi1f+wa8/XkVM2WZx/YNJR7RXVwHpourFog/va+0cq6wX
dfU6YAU6ragHdu83sJxc72VFnDLqRRmFhu3lnisO3vkWwWR2mBubj7a5Liha7Jkc
Uu9JPRtcS40BX7mlZFGA1M0ZltCB24cLCNkoLUlzlSzkLAXURvsBtZMdJwQzAj9N
t/QibHe4096yB+rP5V5O5sZzM9aTjD5KRpqbiQM/EOi/Y0orQUM6y0UhEWJF4s6V
n7fLxV/Ww9SzDSwwfPoPQylY40Adb69vDhzDxCvCa9lBjotf0iz1oiJGFZmAG41U
vD2NzIdcLX+oVx1uVfv+ZmytaKCFFtwrLmu7bsH3bNr3RoIPrWXO5vVnmvKQmyJy
IaygKAsXWac+gRvlX3yE4svgrOQ4b4XD47w4Mf6mZBzAgzpzSVVgJ3AOpxqNQw5o
ftCShW+l6mkp8NlgzEKzx2+XN/G7pPV3LFvTAbqIZWukBoSqOT5S4Egly+7ZEB39
gHgeka78NJPf2jIij0yclTiCMxT6xkEbZzGS+fnbmetvduNwMh4=
=p3hS
-----END PGP SIGNATURE-----

--GYjMp6IFoAg2M0P0--