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.
> >
> > Signed-off-by: Aniruddha TVS Rao <anrao@xxxxxxxxxx>
> > Signed-off-by: Prathamesh Shete <pshete@xxxxxxxxxx>
>
> One observation below.
>
> Nevertheless:
>
> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>
> > ---
> > drivers/mmc/host/sdhci-tegra.c | 44 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.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
> >
> > #include <soc/tegra/common.h>
> >
> > @@ -94,6 +98,8 @@
> > #define SDHCI_TEGRA_AUTO_CAL_STATUS 0x1ec
> > #define SDHCI_TEGRA_AUTO_CAL_ACTIVE BIT(31)
> >
> > +#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)
> >
> > #define NVQUIRK_HAS_ANDROID_GPT_SECTOR BIT(11)
> > +#define NVQUIRK_PROGRAM_STREAMID BIT(12)
> >
> > /* 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
> > };
> >
> > 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 = {
> > NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
> > NVQUIRK_ENABLE_SDR50 |
> > NVQUIRK_ENABLE_SDR104 |
> > + NVQUIRK_PROGRAM_STREAMID |
> > NVQUIRK_HAS_TMCLK,
> > .min_tap_delay = 95,
> > .max_tap_delay = 111,
> > @@ -1775,6 +1786,25 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
> > if (rc)
> > goto err_add_host;
> >
> > + /* Program MC streamID for DMA transfers */
> > +#ifdef CONFIG_IOMMU_API
> > + if (soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
> > + struct iommu_fwspec *fwspec;
> > +
> > + fwspec = dev_iommu_fwspec_get(&pdev->dev);
> > + if (fwspec == NULL) {
> > + dev_warn(mmc_dev(host->mmc),
> > + "iommu fwspec is NULL, continue without stream ID\n");
>
> 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 == NULL should also be done in sdhci_tegra_resume().
>
> > + } else {
> > + tegra_host->streamid = 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;
> >
> > err_add_host:
> > @@ -1861,6 +1891,10 @@ static int sdhci_tegra_suspend(struct device *dev)
> > static int sdhci_tegra_resume(struct device *dev)
> > {
> > struct sdhci_host *host = dev_get_drvdata(dev);
> > +#ifdef CONFIG_IOMMU_API
> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> > +#endif
> > int ret;
> >
> > ret = mmc_gpio_set_cd_wake(host->mmc, false);
> > @@ -1871,6 +1905,16 @@ static int sdhci_tegra_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > + /* 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 == 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@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
