| Message ID | 20220914095628.26093-1-pshete@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] mmc: sdhci-tegra: Separate T19x and T23x SoC data | expand |
On Wed, Sep 14, 2022 at 03:26:26PM +0530, Prathamesh Shete wrote: > As per T23x MSS IAS SMMU clients are supposed This reference isn't useful because this document is not available publicly. If this information exists in the TRM, then make a reference to that, otherwise just leave out the reference and keep the rest of the comment. > to program streamid from their respective address spaces instead of MC s/streamid/stream ID/ to match the ARM SMMU spelling. > override > Define NVQUIRK_PROGRAM_MC_STREAMID and use it to program SMMU streamid > from the SDMMC client address space Maybe make this all look more like one big paragraph. Right now it looks fragments of sentences thrown together and is difficult to read. > > Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index a6c5bbae77b4..b66b0cc51497 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -25,6 +25,7 @@ > #include <linux/mmc/slot-gpio.h> > #include <linux/gpio/consumer.h> > #include <linux/ktime.h> > +#include <linux/iommu.h> > > #include <soc/tegra/common.h> > > @@ -94,6 +95,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 +124,7 @@ > #define NVQUIRK_HAS_TMCLK BIT(10) > > #define NVQUIRK_HAS_ANDROID_GPT_SECTOR BIT(11) > +#define NVQUIRK_PROGRAM_MC_STREAMID BIT(17) Why is this called "program MC stream ID"? What's programmed is the SMMU stream ID, right? Perhaps just leave out that MC_ prefix altogether since there's no ambiguity with any other quirk to begin with. Also, why skip from bit 11 (of the GPT sector quirk) to bit 17? Can we not use bit 12 as the next one? > > /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */ > #define SDHCI_TEGRA_CQE_BASE_ADDR 0xF000 > @@ -177,6 +181,7 @@ struct sdhci_tegra { > bool enable_hwcq; > unsigned long curr_clk_rate; > u8 tuned_tap_delay; > + u32 streamid; > }; > > static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) > @@ -1564,6 +1569,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_MC_STREAMID | > NVQUIRK_HAS_TMCLK, > .min_tap_delay = 95, > .max_tap_delay = 111, > @@ -1637,6 +1643,7 @@ static int sdhci_tegra_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_tegra *tegra_host; > struct clk *clk; > + struct iommu_fwspec *fwspec; The above is largely sorted in reverse christmas tree order, so this one sticks out a bit. Maybe put it before the clk declaration. Not usually a big deal, really, but since you're going to touch this anyway, may as well touch that up. > int rc; > > soc_data = of_device_get_match_data(&pdev->dev); > @@ -1775,6 +1782,23 @@ static int sdhci_tegra_probe(struct platform_device *pdev) > if (rc) > goto err_add_host; > > + /* Program MC streamID for DMA transfers */ > + if (soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) { > + fwspec = dev_iommu_fwspec_get(&pdev->dev); > + if (fwspec == NULL) { > + rc = -ENODEV; > + dev_err(mmc_dev(host->mmc), > + "failed to get MC streamid: %d\n", > + rc); > + goto err_rst_get; Do we really want to make this fatal? What if somebody really wants to not put the SD/MMC controllers behind an IOMMU? It's quite unlikely to happen, but it's technically possible. Also, there was a brief time where the DTS files didn't have any iommus properties, so if somebody were to use a DTS from that era and a kernel with this patch applied, they'd end up with non-functional SD/MMC. Again, that's very unlikely to happen, but it could, if for example this patch ends up being back-ported or something like that. I think it's safe (and easier) to ignore this case. Perhaps if you really want people to use SD/MMC you may want to add a warning here instead. But even that shouldn't be necessary. If the stream ID is not programmed as required, the SMMU should fault and give people the hint that they need to fix this. > + } else { > + tegra_host->streamid = fwspec->ids[0] & 0xffff; > + tegra_sdhci_writel(host, tegra_host->streamid | > + (tegra_host->streamid << 8), > + SDHCI_TEGRA_CIF2AXI_CTRL_0); Perhaps define macros for the read/write stream ID fields in this register? Otherwise it might be confusing why you're writing the value twice. Thierry > + } > + } > + > return 0; > > err_add_host: > @@ -1861,6 +1885,8 @@ static int sdhci_tegra_suspend(struct device *dev) > static int sdhci_tegra_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host); > int ret; > > ret = mmc_gpio_set_cd_wake(host->mmc, false); > @@ -1871,6 +1897,13 @@ static int sdhci_tegra_resume(struct device *dev) > if (ret) > return ret; > > + /* Re-program MC streamID for DMA transfers */ > + if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) { > + tegra_sdhci_writel(host, tegra_host->streamid | > + (tegra_host->streamid << 8), > + SDHCI_TEGRA_CIF2AXI_CTRL_0); > + } > + > ret = sdhci_resume_host(host); > if (ret) > goto disable_clk; > -- > 2.17.1 >
On Wed, Sep 14, 2022 at 03:26:28PM +0530, Prathamesh Shete wrote: > Ensure tegra_host member "curr_clk_rate" holds the actual clock rate > instead of requested clock rate for proper use during tuning correction > algorithm. Ideally there shouldn't be a deviation between host_clk and the actual clock rate that was set. Perhaps it'd be good to provide a bit more information on what the deviation can be and when that happens, as well as what the consequences are. That would make it a bit more obvious why this fix is needed. Thierry > > Fixes: ea8fc5953e8b ("mmc: tegra: update hw tuning process") > > Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 7d16dc41fe91..42b018d4ebc3 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -778,7 +778,7 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > dev_err(dev, "failed to set clk rate to %luHz: %d\n", > host_clk, err); > > - tegra_host->curr_clk_rate = host_clk; > + tegra_host->curr_clk_rate = clk_get_rate(pltfm_host->clk); > if (tegra_host->ddr_signaling) > host->max_clk = host_clk; > else > -- > 2.17.1 >
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 2d2d8260c681..a6c5bbae77b4 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -1556,7 +1556,21 @@ static const struct sdhci_tegra_soc_data soc_data_tegra194 = { .max_tap_delay = 139, }; +static const struct sdhci_tegra_soc_data soc_data_tegra234 = { + .pdata = &sdhci_tegra186_pdata, + .dma_mask = DMA_BIT_MASK(39), + .nvquirks = NVQUIRK_NEEDS_PAD_CONTROL | + NVQUIRK_HAS_PADCALIB | + NVQUIRK_DIS_CARD_CLK_CONFIG_TAP | + NVQUIRK_ENABLE_SDR50 | + NVQUIRK_ENABLE_SDR104 | + NVQUIRK_HAS_TMCLK, + .min_tap_delay = 95, + .max_tap_delay = 111, +}; + static const struct of_device_id sdhci_tegra_dt_match[] = { + { .compatible = "nvidia,tegra234-sdhci", .data = &soc_data_tegra234 }, { .compatible = "nvidia,tegra194-sdhci", .data = &soc_data_tegra194 }, { .compatible = "nvidia,tegra186-sdhci", .data = &soc_data_tegra186 }, { .compatible = "nvidia,tegra210-sdhci", .data = &soc_data_tegra210 },