[PATCH v2] mmc: tegra: Implement enable_dma() to set dma_mask

[PATCH v2] mmc: tegra: Implement enable_dma() to set dma_mask

[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.

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 <nicoleotsuka@gmail.com>
---

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(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index f4d4761cf20a..6156ffb145cd 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -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 |
-- 
2.17.1

Re: [PATCH v2] mmc: tegra: Implement enable_dma() to set dma_mask

[Adrian Hunter]

On 14/08/19 3:57 AM, Nicolin Chen wrote:
> [ 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.

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

Re: [PATCH v2] mmc: tegra: Implement enable_dma() to set dma_mask

[Nicolin Chen]

On Thu, Aug 15, 2019 at 02:48:20PM +0300, Adrian Hunter wrote:
> On 14/08/19 3:57 AM, Nicolin Chen wrote:
> > [ 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.
> 
> 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

Re: [PATCH v2] mmc: tegra: Implement enable_dma() to set dma_mask

[Adrian Hunter]

On 16/08/19 2:16 AM, Nicolin Chen wrote:
> On Thu, Aug 15, 2019 at 02:48:20PM +0300, Adrian Hunter wrote:
>> On 14/08/19 3:57 AM, Nicolin Chen wrote:
>>> [ 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.
>>
>> 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?

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

From: Adrian Hunter <adrian.hunter@intel.com>
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 <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 12 ++++--------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 66c2cf89ee22..43d32ba63ff5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -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);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 81e23784475a..a9ab2deaeb45 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -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 */
-- 
2.17.1