mmc: tegra: Implement enable_dma() to set dma_mask

PATCH In review Nicolin Chen 2 participants 4 messages Aug 14, 2019 – Sep 4, 2019

Subsystems (3)

MMC, SD and SDIO Tegra architecture support Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Nicolin Chen

author

2019-08-14 00:56 UTC #0 lore raw

PATCH v2 1/1 mmc: tegra: Implement enable_dma() to set dma_mask

[ Integrated the change and commit message made by Thierry Reding ]

The SDHCI controller found in early Tegra SoCs (from Tegra20 through Tegra114) used an AHB interface to the memory controller, which allowed only 32 bits of memory to be addressed.

Starting with Tegra124, this limitation was removed by making the SDHCI controllers native MCCIF clients, which means that they got increased bandwidth and better arbitration to the memory controller as well as an address range extended to 40 bits, out of which only 34 were actually used (bits 34-39 are tied to 0 in the controller).

For Tegra186, all of the 40 bits can be used; For Tegra194, 39-bit can be used.

So far, sdhci-tegra driver has been relying on sdhci core to configure the DMA_BIT_MASK between 32-bit or 64-bit, using one of quirks2 flags:
SDHCI_QUIRK2_BROKEN_64_BIT_DMA. However, there is a common way, being mentioned in sdhci.c file, to set dma_mask via enable_dma() callback in the device driver directly.

So this patch implements an enable_dma() callback in the sdhci-tegra, in order to set an accurate DMA_BIT_MASK, other than just 32/64 bits.

Signed-off-by Nicolin Chen <[email protected]>

---

Changelog
v1->v2:
* Applied to older SoC, suggested by Thierry.
* Note that only Tegra210, Tegra186 and Tegra194 are tested.

 drivers/mmc/host/sdhci-tegra.c | 48 ++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 20 deletions(-)

drivers/mmc/host/sdhci-tegra.c +28 ~~-20~~

index f4d4761cf20a..6156ffb145cd 100644
@@ -16,6 +16,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
+#include <linux/dma-mapping.h>
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>

@@ -104,6 +105,7 @@
 
 struct sdhci_tegra_soc_data {
 	const struct sdhci_pltfm_data *pdata;
+	u64 dma_bit_mask;
 	u32 nvquirks;
 	u8 min_tap_delay;
 	u8 max_tap_delay;

@@ -749,6 +751,19 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 }
 
+static int tegra_sdhci_enable_dma(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
+	struct device *dev = mmc_dev(host->mmc);
+
+	if (soc_data->dma_bit_mask)
+		return dma_set_mask_and_coherent(dev, soc_data->dma_bit_mask);
+
+	return 0;
+}
+
 static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);

@@ -1228,6 +1243,7 @@ static const struct sdhci_ops tegra_sdhci_ops = {
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
+	.enable_dma = tegra_sdhci_enable_dma,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
 	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,

@@ -1246,6 +1262,7 @@ static const struct sdhci_pltfm_data sdhci_tegra20_pdata = {
 
 static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
 	.pdata = &sdhci_tegra20_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(32),
 	.nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
 		    NVQUIRK_ENABLE_BLOCK_GAP_DET,
 };

@@ -1272,6 +1289,7 @@ static const struct sdhci_pltfm_data sdhci_tegra30_pdata = {
 
 static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
 	.pdata = &sdhci_tegra30_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(32),
 	.nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 |
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104 |

@@ -1284,6 +1302,7 @@ static const struct sdhci_ops tegra114_sdhci_ops = {
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
+	.enable_dma = tegra_sdhci_enable_dma,
 	.reset      = tegra_sdhci_reset,
 	.platform_execute_tuning = tegra_sdhci_execute_tuning,
 	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,

@@ -1304,6 +1323,7 @@ static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
 
 static const struct sdhci_tegra_soc_data soc_data_tegra114 = {
 	.pdata = &sdhci_tegra114_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(32),
 };
 
 static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {

@@ -1313,22 +1333,13 @@ static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
 		  SDHCI_QUIRK_NO_HISPD_BIT |
 		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-		   /*
-		    * The TRM states that the SD/MMC controller found on
-		    * Tegra124 can address 34 bits (the maximum supported by
-		    * the Tegra memory controller), but tests show that DMA
-		    * to or from above 4 GiB doesn't work. This is possibly
-		    * caused by missing programming, though it's not obvious
-		    * what sequence is required. Mark 64-bit DMA broken for
-		    * now to fix this for existing users (e.g. Nyan boards).
-		    */
-		   SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 	.ops  = &tegra114_sdhci_ops,
 };
 
 static const struct sdhci_tegra_soc_data soc_data_tegra124 = {
 	.pdata = &sdhci_tegra124_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(34),
 };
 
 static const struct sdhci_ops tegra210_sdhci_ops = {

@@ -1337,6 +1348,7 @@ static const struct sdhci_ops tegra210_sdhci_ops = {
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
+	.enable_dma = tegra_sdhci_enable_dma,
 	.reset      = tegra_sdhci_reset,
 	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
 	.voltage_switch = tegra_sdhci_voltage_switch,

@@ -1356,6 +1368,7 @@ static const struct sdhci_pltfm_data sdhci_tegra210_pdata = {
 
 static const struct sdhci_tegra_soc_data soc_data_tegra210 = {
 	.pdata = &sdhci_tegra210_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(34),
 	.nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
 		    NVQUIRK_HAS_PADCALIB |
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |

@@ -1370,6 +1383,7 @@ static const struct sdhci_ops tegra186_sdhci_ops = {
 	.write_l    = tegra_sdhci_writel,
 	.set_clock  = tegra_sdhci_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
+	.enable_dma = tegra_sdhci_enable_dma,
 	.reset      = tegra_sdhci_reset,
 	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
 	.voltage_switch = tegra_sdhci_voltage_switch,

@@ -1384,20 +1398,13 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
 		  SDHCI_QUIRK_NO_HISPD_BIT |
 		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-		   /* SDHCI controllers on Tegra186 support 40-bit addressing.
-		    * IOVA addresses are 48-bit wide on Tegra186.
-		    * With 64-bit dma mask used for SDHCI, accesses can
-		    * be broken. Disable 64-bit dma, which would fall back
-		    * to 32-bit dma mask. Ideally 40-bit dma mask would work,
-		    * But it is not supported as of now.
-		    */
-		   SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 	.ops  = &tegra186_sdhci_ops,
 };
 
 static const struct sdhci_tegra_soc_data soc_data_tegra186 = {
 	.pdata = &sdhci_tegra186_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(40),
 	.nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
 		    NVQUIRK_HAS_PADCALIB |
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |

@@ -1410,6 +1417,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra186 = {
 
 static const struct sdhci_tegra_soc_data soc_data_tegra194 = {
 	.pdata = &sdhci_tegra186_pdata,
+	.dma_bit_mask = DMA_BIT_MASK(39),
 	.nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
 		    NVQUIRK_HAS_PADCALIB |
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |

Signature · 1 line

2.17.1

Adrian Hunter

maintainer

2019-08-15 11:48 UTC #1 lore raw

Quoted 15 lines · Nicolin Chen

[ Integrated the change and commit message made by Thierry Reding ]
The SDHCI controller found in early Tegra SoCs (from Tegra20 through Tegra114) used an AHB interface to the memory controller, which allowed only 32 bits of memory to be addressed.
Starting with Tegra124, this limitation was removed by making the SDHCI controllers native MCCIF clients, which means that they got increased bandwidth and better arbitration to the memory controller as well as an address range extended to 40 bits, out of which only 34 were actually used (bits 34-39 are tied to 0 in the controller).
For Tegra186, all of the 40 bits can be used; For Tegra194, 39-bit can be used.

Quoted 8 lines

So far, sdhci-tegra driver has been relying on sdhci core to configure the DMA_BIT_MASK between 32-bit or 64-bit, using one of quirks2 flags:
SDHCI_QUIRK2_BROKEN_64_BIT_DMA. However, there is a common way, being mentioned in sdhci.c file, to set dma_mask via enable_dma() callback in the device driver directly.
So this patch implements an enable_dma() callback in the sdhci-tegra, in order to set an accurate DMA_BIT_MASK, other than just 32/64 bits.

Is there a reason this cannot be done at probe time?

Nicolin Chen

author

2019-08-15 23:15 UTC #2 lore raw

Quoted 22 lines · Adrian Hunter

Quoted 21 lines · Nicolin Chen
[ Integrated the change and commit message made by Thierry Reding ]
The SDHCI controller found in early Tegra SoCs (from Tegra20 through Tegra114) used an AHB interface to the memory controller, which allowed only 32 bits of memory to be addressed.
Starting with Tegra124, this limitation was removed by making the SDHCI controllers native MCCIF clients, which means that they got increased bandwidth and better arbitration to the memory controller as well as an address range extended to 40 bits, out of which only 34 were actually used (bits 34-39 are tied to 0 in the controller).
For Tegra186, all of the 40 bits can be used; For Tegra194, 39-bit can be used.
So far, sdhci-tegra driver has been relying on sdhci core to configure the DMA_BIT_MASK between 32-bit or 64-bit, using one of quirks2 flags:
SDHCI_QUIRK2_BROKEN_64_BIT_DMA. However, there is a common way, being mentioned in sdhci.c file, to set dma_mask via enable_dma() callback in the device driver directly.

Quoted 4 lines

Quoted 2 lines
So this patch implements an enable_dma() callback in the sdhci-tegra, in order to set an accurate DMA_BIT_MASK, other than just 32/64 bits.
Is there a reason this cannot be done at probe time?

It's supposed to be done at probe() time. But sdhci_setup_host() does both 32-bit/64-bit dma_mask setting and dma_alloc(), so if the dma_mask isn't correctly set inside sdhci_setup_host(), the allocation would fall to a 64-bit IOVA range that hardware does not support -- smmu error would happen and crash the system. On the other hand, ->enable_dma() is called in that function right after 32-bit/64-bit dma_mask setting. Or is there any other way of adding to probe() that I am missing here?

Thanks
Nicolin

Adrian Hunter

maintainer

2019-09-04 08:31 UTC #3 lore raw

Quoted 28 lines · Nicolin Chen

Quoted 22 lines · Adrian Hunter
Quoted 21 lines · Nicolin Chen
[ Integrated the change and commit message made by Thierry Reding ]
The SDHCI controller found in early Tegra SoCs (from Tegra20 through Tegra114) used an AHB interface to the memory controller, which allowed only 32 bits of memory to be addressed.
Starting with Tegra124, this limitation was removed by making the SDHCI controllers native MCCIF clients, which means that they got increased bandwidth and better arbitration to the memory controller as well as an address range extended to 40 bits, out of which only 34 were actually used (bits 34-39 are tied to 0 in the controller).
For Tegra186, all of the 40 bits can be used; For Tegra194, 39-bit can be used.
So far, sdhci-tegra driver has been relying on sdhci core to configure the DMA_BIT_MASK between 32-bit or 64-bit, using one of quirks2 flags:
SDHCI_QUIRK2_BROKEN_64_BIT_DMA. However, there is a common way, being mentioned in sdhci.c file, to set dma_mask via enable_dma() callback in the device driver directly.
Quoted 4 lines
Quoted 2 lines
So this patch implements an enable_dma() callback in the sdhci-tegra, in order to set an accurate DMA_BIT_MASK, other than just 32/64 bits.
Is there a reason this cannot be done at probe time?

Quoted 8 lines

It's supposed to be done at probe() time. But sdhci_setup_host() does both 32-bit/64-bit dma_mask setting and dma_alloc(), so if the dma_mask isn't correctly set inside sdhci_setup_host(), the allocation would fall to a 64-bit IOVA range that hardware does not support -- smmu error would happen and crash the system. On the other hand, ->enable_dma() is called in that function right after 32-bit/64-bit dma_mask setting. Or is there any other way of adding to probe() that I am missing here?

Hmmm. Maybe we should clean that up a bit. What do about this:

From: Adrian Hunter <[email protected]>
Date: Wed, 4 Sep 2019 11:28:51 +0300
Subject: [PATCH] mmc: sdhci: Let drivers define their DMA mask

Add host operation ->set_dma_mask() so that drivers can define their own DMA masks.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci.c | 12 ++++--------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

drivers/mmc/host/sdhci.c +4 -8

index 66c2cf89ee22..43d32ba63ff5 100644
@@ -3756,18 +3756,14 @@ int sdhci_setup_host(struct sdhci_host *host)
 		host->flags &= ~SDHCI_USE_ADMA;
 	}
 
-	/*
-	 * It is assumed that a 64-bit capable device has set a 64-bit DMA mask
-	 * and *must* do 64-bit DMA.  A driver has the opportunity to change
-	 * that during the first call to ->enable_dma().  Similarly
-	 * SDHCI_QUIRK2_BROKEN_64_BIT_DMA must be left to the drivers to
-	 * implement.
-	 */
 	if (sdhci_can_64bit_dma(host))
 		host->flags |= SDHCI_USE_64_BIT_DMA;
 
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
-		ret = sdhci_set_dma_mask(host);
+		if (host->ops->set_dma_mask)
+			ret = host->ops->set_dma_mask(host);
+		else
+			ret = sdhci_set_dma_mask(host);
 
 		if (!ret && host->ops->enable_dma)
 			ret = host->ops->enable_dma(host);

drivers/mmc/host/sdhci.h +1 -0

index 81e23784475a..a9ab2deaeb45 100644
@@ -623,6 +623,7 @@ struct sdhci_ops {
 	u32		(*irq)(struct sdhci_host *host, u32 intmask);
 
 	int		(*enable_dma)(struct sdhci_host *host);
+	int		(*set_dma_mask)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	/* get_timeout_clock should return clk rate in unit of Hz */

Signature · 1 line

2.17.1