| Message ID | 20220914095628.26093-2-pshete@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
Hi Prathamesh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tegra/for-next]
[also build test ERROR on linus/master v6.0-rc5 next-20220915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Prathamesh-Shete/mmc-sdhci-tegra-Separate-T19x-and-T23x-SoC-data/20220914-175832
base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: hexagon-randconfig-r045-20220915 (https://download.01.org/0day-ci/archive/20220916/202209160420.bcAdUgK2-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9817941f43bffe4de367526928377b9723068fb5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Prathamesh-Shete/mmc-sdhci-tegra-Separate-T19x-and-T23x-SoC-data/20220914-175832
git checkout 9817941f43bffe4de367526928377b9723068fb5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/mmc/host/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/mmc/host/sdhci-tegra.c:1795:35: error: no member named 'ids' in 'struct iommu_fwspec'
tegra_host->streamid = fwspec->ids[0] & 0xffff;
~~~~~~ ^
1 error generated.
vim +1795 drivers/mmc/host/sdhci-tegra.c
1638
1639 static int sdhci_tegra_probe(struct platform_device *pdev)
1640 {
1641 const struct sdhci_tegra_soc_data *soc_data;
1642 struct sdhci_host *host;
1643 struct sdhci_pltfm_host *pltfm_host;
1644 struct sdhci_tegra *tegra_host;
1645 struct clk *clk;
1646 struct iommu_fwspec *fwspec;
1647 int rc;
1648
1649 soc_data = of_device_get_match_data(&pdev->dev);
1650 if (!soc_data)
1651 return -EINVAL;
1652
1653 host = sdhci_pltfm_init(pdev, soc_data->pdata, sizeof(*tegra_host));
1654 if (IS_ERR(host))
1655 return PTR_ERR(host);
1656 pltfm_host = sdhci_priv(host);
1657
1658 tegra_host = sdhci_pltfm_priv(pltfm_host);
1659 tegra_host->ddr_signaling = false;
1660 tegra_host->pad_calib_required = false;
1661 tegra_host->pad_control_available = false;
1662 tegra_host->soc_data = soc_data;
1663
1664 if (soc_data->nvquirks & NVQUIRK_HAS_ANDROID_GPT_SECTOR)
1665 host->mmc->caps2 |= MMC_CAP2_ALT_GPT_TEGRA;
1666
1667 if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL) {
1668 rc = tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host);
1669 if (rc == 0)
1670 host->mmc_host_ops.start_signal_voltage_switch =
1671 sdhci_tegra_start_signal_voltage_switch;
1672 }
1673
1674 /* Hook to periodically rerun pad calibration */
1675 if (soc_data->nvquirks & NVQUIRK_HAS_PADCALIB)
1676 host->mmc_host_ops.request = tegra_sdhci_request;
1677
1678 host->mmc_host_ops.hs400_enhanced_strobe =
1679 tegra_sdhci_hs400_enhanced_strobe;
1680
1681 if (!host->ops->platform_execute_tuning)
1682 host->mmc_host_ops.execute_tuning =
1683 tegra_sdhci_execute_hw_tuning;
1684
1685 rc = mmc_of_parse(host->mmc);
1686 if (rc)
1687 goto err_parse_dt;
1688
1689 if (tegra_host->soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
1690 host->mmc->caps |= MMC_CAP_1_8V_DDR;
1691
1692 /* HW busy detection is supported, but R1B responses are required. */
1693 host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
1694
1695 /* GPIO CD can be set as a wakeup source */
1696 host->mmc->caps |= MMC_CAP_CD_WAKE;
1697
1698 tegra_sdhci_parse_dt(host);
1699
1700 tegra_host->power_gpio = devm_gpiod_get_optional(&pdev->dev, "power",
1701 GPIOD_OUT_HIGH);
1702 if (IS_ERR(tegra_host->power_gpio)) {
1703 rc = PTR_ERR(tegra_host->power_gpio);
1704 goto err_power_req;
1705 }
1706
1707 /*
1708 * Tegra210 has a separate SDMMC_LEGACY_TM clock used for host
1709 * timeout clock and SW can choose TMCLK or SDCLK for hardware
1710 * data timeout through the bit USE_TMCLK_FOR_DATA_TIMEOUT of
1711 * the register SDHCI_TEGRA_VENDOR_SYS_SW_CTRL.
1712 *
1713 * USE_TMCLK_FOR_DATA_TIMEOUT bit default is set to 1 and SDMMC uses
1714 * 12Mhz TMCLK which is advertised in host capability register.
1715 * With TMCLK of 12Mhz provides maximum data timeout period that can
1716 * be achieved is 11s better than using SDCLK for data timeout.
1717 *
1718 * So, TMCLK is set to 12Mhz and kept enabled all the time on SoC's
1719 * supporting separate TMCLK.
1720 */
1721
1722 if (soc_data->nvquirks & NVQUIRK_HAS_TMCLK) {
1723 clk = devm_clk_get(&pdev->dev, "tmclk");
1724 if (IS_ERR(clk)) {
1725 rc = PTR_ERR(clk);
1726 if (rc == -EPROBE_DEFER)
1727 goto err_power_req;
1728
1729 dev_warn(&pdev->dev, "failed to get tmclk: %d\n", rc);
1730 clk = NULL;
1731 }
1732
1733 clk_set_rate(clk, 12000000);
1734 rc = clk_prepare_enable(clk);
1735 if (rc) {
1736 dev_err(&pdev->dev,
1737 "failed to enable tmclk: %d\n", rc);
1738 goto err_power_req;
1739 }
1740
1741 tegra_host->tmclk = clk;
1742 }
1743
1744 clk = devm_clk_get(mmc_dev(host->mmc), NULL);
1745 if (IS_ERR(clk)) {
1746 rc = dev_err_probe(&pdev->dev, PTR_ERR(clk),
1747 "failed to get clock\n");
1748 goto err_clk_get;
1749 }
1750 pltfm_host->clk = clk;
1751
1752 tegra_host->rst = devm_reset_control_get_exclusive(&pdev->dev,
1753 "sdhci");
1754 if (IS_ERR(tegra_host->rst)) {
1755 rc = PTR_ERR(tegra_host->rst);
1756 dev_err(&pdev->dev, "failed to get reset control: %d\n", rc);
1757 goto err_rst_get;
1758 }
1759
1760 rc = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
1761 if (rc)
1762 goto err_rst_get;
1763
1764 pm_runtime_enable(&pdev->dev);
1765 rc = pm_runtime_resume_and_get(&pdev->dev);
1766 if (rc)
1767 goto err_pm_get;
1768
1769 rc = reset_control_assert(tegra_host->rst);
1770 if (rc)
1771 goto err_rst_assert;
1772
1773 usleep_range(2000, 4000);
1774
1775 rc = reset_control_deassert(tegra_host->rst);
1776 if (rc)
1777 goto err_rst_assert;
1778
1779 usleep_range(2000, 4000);
1780
1781 rc = sdhci_tegra_add_host(host);
1782 if (rc)
1783 goto err_add_host;
1784
1785 /* Program MC streamID for DMA transfers */
1786 if (soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
1787 fwspec = dev_iommu_fwspec_get(&pdev->dev);
1788 if (fwspec == NULL) {
1789 rc = -ENODEV;
1790 dev_err(mmc_dev(host->mmc),
1791 "failed to get MC streamid: %d\n",
1792 rc);
1793 goto err_rst_get;
1794 } else {
> 1795 tegra_host->streamid = fwspec->ids[0] & 0xffff;
1796 tegra_sdhci_writel(host, tegra_host->streamid |
1797 (tegra_host->streamid << 8),
1798 SDHCI_TEGRA_CIF2AXI_CTRL_0);
1799 }
1800 }
1801
1802 return 0;
1803
1804 err_add_host:
1805 reset_control_assert(tegra_host->rst);
1806 err_rst_assert:
1807 pm_runtime_put_sync_suspend(&pdev->dev);
1808 err_pm_get:
1809 pm_runtime_disable(&pdev->dev);
1810 err_rst_get:
1811 err_clk_get:
1812 clk_disable_unprepare(tegra_host->tmclk);
1813 err_power_req:
1814 err_parse_dt:
1815 sdhci_pltfm_free(pdev);
1816 return rc;
1817 }
1818
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) /* 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; 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; + } else { + tegra_host->streamid = fwspec->ids[0] & 0xffff; + tegra_sdhci_writel(host, tegra_host->streamid | + (tegra_host->streamid << 8), + SDHCI_TEGRA_CIF2AXI_CTRL_0); + } + } + 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;