[PATCH 1/2] mmc: sdhci: Properly set DMA mask

[PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The implementation of sdhci_set_dma_mask() is conflating two things: on
one hand it uses the SDHCI_USE_64_BIT_DMA flag to determine whether or
not to use the 64-bit addressing capability of the controller and on the
other hand it also uses that flag to set a DMA mask for the controller's
parent device.

However, a controller supporting 64-bit addressing doesn't mean that it
needs to support addressing 64 bits of address range. It's perfectly
acceptable to use 64-bit addressing for a 32-bit address range or even
smaller, even if that makes little sense, considering the extra overhead
of the 64-bit addressing descriptors.

But it is fairly common for hardware to support somewhere between 32 and
64 bits of address range. Tegra124 and Tegra210, for example, support 34
bits and the newer Tegra186 and Tegra194 support 40 bits. The latter can
also use an IOMMU for address translation, which has an input address
range of 48 bits. This causes problems with the current algorithm in the
SDHCI core for choosing the DMA mask. If the DMA mask is set to 64 bits,
the DMA memory allocations can (and usually do because the allocator
starts from the top) end up beyond the 40 bit boundary addressable by
the SDHCI controller, causing IOMMU faults.

For Tegra specifically this problem is currently worked around by
setting the SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk. This causes the DMA
mask to always be set to 32 bits and therefore all allocations will fit
within the range addressable by the controller.

This commit reworks the code in sdhci_set_dma_mask() to fix the above
issue. The rationale behind this change is that the SDHCI controller
driver should be the authoritative source of the DMA mask setting. The
SDHCI core has no way of knowing what the individual SDHCI controllers
are capable of. So instead of overriding the DMA mask depending on
whether or not 64-bit addressing mode is used, the DMA mask is only
modified if absolutely necessary. On one hand, if the controller can
only address 32 bits of memory or less, we disable use of 64-bit
addressing mode because it is not needed. On the other hand, we also
want to make sure that if we don't support 64-bit addressing mode, such
as in the case where the BROKEN_64_BIT_DMA quirk is set, we do restrict
the DMA mask to fit the capabilities. The latter is an inconsistency by
the driver, so we warn about it to make sure it will be addressed in the
driver.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7c6c93e85b7e..01f81e96be23 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3499,27 +3499,35 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 	struct device *dev = mmc_dev(mmc);
-	int ret = -EINVAL;
+	u64 dma_mask = dma_get_mask(dev);
+	u64 dma32 = DMA_BIT_MASK(32);
+	int ret = 0;
 
 	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
 		host->flags &= ~SDHCI_USE_64_BIT_DMA;
 
-	/* Try 64-bit mask if hardware is capable  of it */
-	if (host->flags & SDHCI_USE_64_BIT_DMA) {
-		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
-		if (ret) {
-			pr_warn("%s: Failed to set 64-bit DMA mask.\n",
-				mmc_hostname(mmc));
-			host->flags &= ~SDHCI_USE_64_BIT_DMA;
-		}
+	/*
+	 * Hardware that can't address more than the 32-bit address range does
+	 * not need to use 64-bit addressing mode, even if it supports it.
+	 */
+	if ((host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask <= dma32)) {
+		pr_debug("%s: controller needs addresses <= 32-bits\n",
+			mmc_hostname(mmc));
+		host->flags &= ~SDHCI_USE_64_BIT_DMA;
 	}
 
-	/* 32-bit mask as default & fallback */
-	if (ret) {
-		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	/*
+	 * If the hardware doesn't support 64-bit addressing, make sure to
+	 * restrict the DMA mask so we don't get buffers allocated beyond the
+	 * 32-bit boundary.
+	 */
+	if (!(host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask > dma32)) {
+		WARN(1, "64-bit DMA not supported, DMA mask %llx\n", dma_mask);
+
+		ret = dma_set_mask_and_coherent(dev, dma32);
 		if (ret)
-			pr_warn("%s: Failed to set 32-bit DMA mask.\n",
-				mmc_hostname(mmc));
+			pr_warn("%s: failed to set 32-bit DMA mask: %d\n",
+				mmc_hostname(mmc), ret);
 	}
 
 	return ret;
-- 
2.19.1

[PATCH 2/2] mmc: sdhci: tegra: Set DMA mask depending on generation

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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

Starting with Tegra186, all of the 40 bits can be used.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 35 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index e6ace31e2a41..c861e0ee7ef2 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -92,6 +93,7 @@
 struct sdhci_tegra_soc_data {
 	const struct sdhci_pltfm_data *pdata;
 	u32 nvquirks;
+	u64 dma_mask;
 };
 
 /* Magic pull up and pull down pad calibration offsets */
@@ -862,6 +864,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
 	.pdata = &sdhci_tegra20_pdata,
 	.nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
 		    NVQUIRK_ENABLE_BLOCK_GAP_DET,
+	.dma_mask = DMA_BIT_MASK(32),
 };
 
 static const struct sdhci_pltfm_data sdhci_tegra30_pdata = {
@@ -890,6 +893,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = {
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104 |
 		    NVQUIRK_HAS_PADCALIB,
+	.dma_mask = DMA_BIT_MASK(32),
 };
 
 static const struct sdhci_ops tegra114_sdhci_ops = {
@@ -919,6 +923,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_mask = DMA_BIT_MASK(32),
 };
 
 static const struct sdhci_pltfm_data sdhci_tegra124_pdata = {
@@ -928,22 +933,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_mask = DMA_BIT_MASK(34),
 };
 
 static const struct sdhci_ops tegra210_sdhci_ops = {
@@ -977,6 +973,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra210 = {
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104,
+	.dma_mask = DMA_BIT_MASK(34),
 };
 
 static const struct sdhci_ops tegra186_sdhci_ops = {
@@ -998,15 +995,7 @@ 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,
 };
 
@@ -1017,6 +1006,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra186 = {
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104,
+	.dma_mask = DMA_BIT_MASK(40),
 };
 
 static const struct of_device_id sdhci_tegra_dt_match[] = {
@@ -1045,6 +1035,11 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 		return -EINVAL;
 	soc_data = match->data;
 
+	rc = dma_set_mask_and_coherent(&pdev->dev, soc_data->dma_mask);
+	if (rc)
+		dev_warn(&pdev->dev, "failed to set DMA mask %llx: %d\n",
+			 soc_data->dma_mask, rc);
+
 	host = sdhci_pltfm_init(pdev, soc_data->pdata, sizeof(*tegra_host));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
-- 
2.19.1

Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Christoph Hellwig]

> +	u64 dma_mask = dma_get_mask(dev);

This is not a driver API.  I think what you want is
dma_get_required_mask to query the mask.  But in that case
you still need to always actually set a mask in the driver as well.

Something like this patch:

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a22e11a65658..36c61778d8f3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3500,6 +3500,13 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
 	struct device *dev = mmc_dev(mmc);
 	int ret = -EINVAL;
 
+	/*
+	 * Systems that can't address more than 32-bits do not need to use
+	 * 64-bit addressing mode, even if the device supports it.
+	 */
+	if (dma_get_required_mask(dev) <= DMA_BIT_MASK(32))
+		host->flags &= ~SDHCI_USE_64_BIT_DMA;
+
 	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
 		host->flags &= ~SDHCI_USE_64_BIT_DMA;
 

Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On Fri, Jan 04, 2019 at 09:43:54AM -0800, Christoph Hellwig wrote:
> > +	u64 dma_mask = dma_get_mask(dev);
> 
> This is not a driver API.  I think what you want is
> dma_get_required_mask to query the mask.  But in that case
> you still need to always actually set a mask in the driver as well.

That's slightly different from what I want to do here. The purpose of
this part of the patch is that when the SDHCI hardware supports only 32
bits of address space, then we want to prevent 64-bit addressing mode
from being used because it isn't useful.

So what we do want to check here is the DMA mask configured by the
driver (or the default set by the bus, or wherever it came from).
dma_get_required_mask() returns the DMA mask required to address all of
system memory. That would perhaps be a useful additional check, but it's
orthogonal to what I'm trying to do here.

Is there something else appropriate that I could use to query the DMA
mask set for a device?

Thierry

> Something like this patch:
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a22e11a65658..36c61778d8f3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3500,6 +3500,13 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
>  	struct device *dev = mmc_dev(mmc);
>  	int ret = -EINVAL;
>  
> +	/*
> +	 * Systems that can't address more than 32-bits do not need to use
> +	 * 64-bit addressing mode, even if the device supports it.
> +	 */
> +	if (dma_get_required_mask(dev) <= DMA_BIT_MASK(32))
> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
> +
>  	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
>  		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Adrian Hunter]

On 4/01/19 12:47 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The implementation of sdhci_set_dma_mask() is conflating two things: on
> one hand it uses the SDHCI_USE_64_BIT_DMA flag to determine whether or
> not to use the 64-bit addressing capability of the controller and on the
> other hand it also uses that flag to set a DMA mask for the controller's
> parent device.
> 
> However, a controller supporting 64-bit addressing doesn't mean that it
> needs to support addressing 64 bits of address range. It's perfectly
> acceptable to use 64-bit addressing for a 32-bit address range or even
> smaller, even if that makes little sense, considering the extra overhead
> of the 64-bit addressing descriptors.
> 
> But it is fairly common for hardware to support somewhere between 32 and
> 64 bits of address range. Tegra124 and Tegra210, for example, support 34
> bits and the newer Tegra186 and Tegra194 support 40 bits. The latter can
> also use an IOMMU for address translation, which has an input address
> range of 48 bits. This causes problems with the current algorithm in the
> SDHCI core for choosing the DMA mask. If the DMA mask is set to 64 bits,
> the DMA memory allocations can (and usually do because the allocator
> starts from the top) end up beyond the 40 bit boundary addressable by
> the SDHCI controller, causing IOMMU faults.
> 
> For Tegra specifically this problem is currently worked around by
> setting the SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk. This causes the DMA
> mask to always be set to 32 bits and therefore all allocations will fit
> within the range addressable by the controller.
> 
> This commit reworks the code in sdhci_set_dma_mask() to fix the above
> issue. The rationale behind this change is that the SDHCI controller
> driver should be the authoritative source of the DMA mask setting. The
> SDHCI core has no way of knowing what the individual SDHCI controllers
> are capable of. So instead of overriding the DMA mask depending on
> whether or not 64-bit addressing mode is used, the DMA mask is only
> modified if absolutely necessary. On one hand, if the controller can
> only address 32 bits of memory or less, we disable use of 64-bit
> addressing mode because it is not needed. On the other hand, we also
> want to make sure that if we don't support 64-bit addressing mode, such
> as in the case where the BROKEN_64_BIT_DMA quirk is set, we do restrict
> the DMA mask to fit the capabilities. The latter is an inconsistency by
> the driver, so we warn about it to make sure it will be addressed in the
> driver.

sdhci_set_dma_mask() was added because people did want sdhci to set the DMA
mask.  Also using 64-bit DMA even with 32-bit systems has the advantage of
reducing exposure to problems i.e. the same logic is used with the same SoC
irrespective of whether or not it is in 32-bit compatibility mode.  So the
policy for sdhci is always to use 64-bit DMA if it is supported.

I suggest we add a new sdhci op for ->set_dma_mask() and call that instead
of sdhci_set_dma_mask() if it is not NULL.

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7c6c93e85b7e..01f81e96be23 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3499,27 +3499,35 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  	struct device *dev = mmc_dev(mmc);
> -	int ret = -EINVAL;
> +	u64 dma_mask = dma_get_mask(dev);
> +	u64 dma32 = DMA_BIT_MASK(32);
> +	int ret = 0;
>  
>  	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
>  		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>  
> -	/* Try 64-bit mask if hardware is capable  of it */
> -	if (host->flags & SDHCI_USE_64_BIT_DMA) {
> -		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> -		if (ret) {
> -			pr_warn("%s: Failed to set 64-bit DMA mask.\n",
> -				mmc_hostname(mmc));
> -			host->flags &= ~SDHCI_USE_64_BIT_DMA;
> -		}
> +	/*
> +	 * Hardware that can't address more than the 32-bit address range does
> +	 * not need to use 64-bit addressing mode, even if it supports it.
> +	 */
> +	if ((host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask <= dma32)) {
> +		pr_debug("%s: controller needs addresses <= 32-bits\n",
> +			mmc_hostname(mmc));
> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>  	}
>  
> -	/* 32-bit mask as default & fallback */
> -	if (ret) {
> -		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	/*
> +	 * If the hardware doesn't support 64-bit addressing, make sure to
> +	 * restrict the DMA mask so we don't get buffers allocated beyond the
> +	 * 32-bit boundary.
> +	 */
> +	if (!(host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask > dma32)) {
> +		WARN(1, "64-bit DMA not supported, DMA mask %llx\n", dma_mask);
> +
> +		ret = dma_set_mask_and_coherent(dev, dma32);
>  		if (ret)
> -			pr_warn("%s: Failed to set 32-bit DMA mask.\n",
> -				mmc_hostname(mmc));
> +			pr_warn("%s: failed to set 32-bit DMA mask: %d\n",
> +				mmc_hostname(mmc), ret);
>  	}
>  
>  	return ret;
> 

Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

On Thu, Jan 10, 2019 at 04:11:33PM +0200, Adrian Hunter wrote:
> On 4/01/19 12:47 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The implementation of sdhci_set_dma_mask() is conflating two things: on
> > one hand it uses the SDHCI_USE_64_BIT_DMA flag to determine whether or
> > not to use the 64-bit addressing capability of the controller and on the
> > other hand it also uses that flag to set a DMA mask for the controller's
> > parent device.
> > 
> > However, a controller supporting 64-bit addressing doesn't mean that it
> > needs to support addressing 64 bits of address range. It's perfectly
> > acceptable to use 64-bit addressing for a 32-bit address range or even
> > smaller, even if that makes little sense, considering the extra overhead
> > of the 64-bit addressing descriptors.
> > 
> > But it is fairly common for hardware to support somewhere between 32 and
> > 64 bits of address range. Tegra124 and Tegra210, for example, support 34
> > bits and the newer Tegra186 and Tegra194 support 40 bits. The latter can
> > also use an IOMMU for address translation, which has an input address
> > range of 48 bits. This causes problems with the current algorithm in the
> > SDHCI core for choosing the DMA mask. If the DMA mask is set to 64 bits,
> > the DMA memory allocations can (and usually do because the allocator
> > starts from the top) end up beyond the 40 bit boundary addressable by
> > the SDHCI controller, causing IOMMU faults.
> > 
> > For Tegra specifically this problem is currently worked around by
> > setting the SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk. This causes the DMA
> > mask to always be set to 32 bits and therefore all allocations will fit
> > within the range addressable by the controller.
> > 
> > This commit reworks the code in sdhci_set_dma_mask() to fix the above
> > issue. The rationale behind this change is that the SDHCI controller
> > driver should be the authoritative source of the DMA mask setting. The
> > SDHCI core has no way of knowing what the individual SDHCI controllers
> > are capable of. So instead of overriding the DMA mask depending on
> > whether or not 64-bit addressing mode is used, the DMA mask is only
> > modified if absolutely necessary. On one hand, if the controller can
> > only address 32 bits of memory or less, we disable use of 64-bit
> > addressing mode because it is not needed. On the other hand, we also
> > want to make sure that if we don't support 64-bit addressing mode, such
> > as in the case where the BROKEN_64_BIT_DMA quirk is set, we do restrict
> > the DMA mask to fit the capabilities. The latter is an inconsistency by
> > the driver, so we warn about it to make sure it will be addressed in the
> > driver.
> 
> sdhci_set_dma_mask() was added because people did want sdhci to set the DMA
> mask.  Also using 64-bit DMA even with 32-bit systems has the advantage of
> reducing exposure to problems i.e. the same logic is used with the same SoC
> irrespective of whether or not it is in 32-bit compatibility mode.  So the
> policy for sdhci is always to use 64-bit DMA if it is supported.
> 
> I suggest we add a new sdhci op for ->set_dma_mask() and call that instead
> of sdhci_set_dma_mask() if it is not NULL.

Some drivers are already doing something similar by overriding the DMA
mask again in ->enable_dma(). I had briefly considered doing that for
Tegra, but after thinking about it, it just became clear to me that we
shouldn't need to override this in every driver. I just don't think it's
correct for the MMC core to muck with the DMA mask. Just because the
hardware supports the SDHCI 64-bit addressing mode doesn't mean that all
64 bits can be addressed by the hardware. The DMA mask defines what the
valid address range is for the device and it's already conventional for
drivers to set this early in their ->probe() implementation (or have the
bus set it up). It seems wasteful to have to redo that in a custom
callback.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Hunter, Adrian]

> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: Thursday, January 10, 2019 6:02 PM
> To: Hunter, Adrian <adrian.hunter@intel.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Jonathan Hunter
> <jonathanh@nvidia.com>; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Krishna Reddy <vdumpa@nvidia.com>; linux-
> mmc@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask
> 
> On Thu, Jan 10, 2019 at 04:11:33PM +0200, Adrian Hunter wrote:
> > On 4/01/19 12:47 PM, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The implementation of sdhci_set_dma_mask() is conflating two things:
> > > on one hand it uses the SDHCI_USE_64_BIT_DMA flag to determine
> > > whether or not to use the 64-bit addressing capability of the
> > > controller and on the other hand it also uses that flag to set a DMA
> > > mask for the controller's parent device.
> > >
> > > However, a controller supporting 64-bit addressing doesn't mean that
> > > it needs to support addressing 64 bits of address range. It's
> > > perfectly acceptable to use 64-bit addressing for a 32-bit address
> > > range or even smaller, even if that makes little sense, considering
> > > the extra overhead of the 64-bit addressing descriptors.
> > >
> > > But it is fairly common for hardware to support somewhere between 32
> > > and
> > > 64 bits of address range. Tegra124 and Tegra210, for example,
> > > support 34 bits and the newer Tegra186 and Tegra194 support 40 bits.
> > > The latter can also use an IOMMU for address translation, which has
> > > an input address range of 48 bits. This causes problems with the
> > > current algorithm in the SDHCI core for choosing the DMA mask. If
> > > the DMA mask is set to 64 bits, the DMA memory allocations can (and
> > > usually do because the allocator starts from the top) end up beyond
> > > the 40 bit boundary addressable by the SDHCI controller, causing IOMMU
> faults.
> > >
> > > For Tegra specifically this problem is currently worked around by
> > > setting the SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk. This causes the
> > > DMA mask to always be set to 32 bits and therefore all allocations
> > > will fit within the range addressable by the controller.
> > >
> > > This commit reworks the code in sdhci_set_dma_mask() to fix the
> > > above issue. The rationale behind this change is that the SDHCI
> > > controller driver should be the authoritative source of the DMA mask
> > > setting. The SDHCI core has no way of knowing what the individual
> > > SDHCI controllers are capable of. So instead of overriding the DMA
> > > mask depending on whether or not 64-bit addressing mode is used, the
> > > DMA mask is only modified if absolutely necessary. On one hand, if
> > > the controller can only address 32 bits of memory or less, we
> > > disable use of 64-bit addressing mode because it is not needed. On
> > > the other hand, we also want to make sure that if we don't support
> > > 64-bit addressing mode, such as in the case where the
> > > BROKEN_64_BIT_DMA quirk is set, we do restrict the DMA mask to fit
> > > the capabilities. The latter is an inconsistency by the driver, so
> > > we warn about it to make sure it will be addressed in the driver.
> >
> > sdhci_set_dma_mask() was added because people did want sdhci to set
> > the DMA mask.  Also using 64-bit DMA even with 32-bit systems has the
> > advantage of reducing exposure to problems i.e. the same logic is used
> > with the same SoC irrespective of whether or not it is in 32-bit
> > compatibility mode.  So the policy for sdhci is always to use 64-bit DMA if it
> is supported.
> >
> > I suggest we add a new sdhci op for ->set_dma_mask() and call that
> > instead of sdhci_set_dma_mask() if it is not NULL.
> 
> Some drivers are already doing something similar by overriding the DMA
> mask again in ->enable_dma(). I had briefly considered doing that for Tegra,
> but after thinking about it, it just became clear to me that we shouldn't need
> to override this in every driver. I just don't think it's correct for the MMC core
> to muck with the DMA mask. Just because the hardware supports the SDHCI
> 64-bit addressing mode doesn't mean that all
> 64 bits can be addressed by the hardware. The DMA mask defines what the
> valid address range is for the device and it's already conventional for drivers
> to set this early in their ->probe() implementation (or have the bus set it up).
> It seems wasteful to have to redo that in a custom callback.

What do you suggest?

Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Christoph Hellwig]

On Thu, Jan 10, 2019 at 11:59:11AM +0100, Thierry Reding wrote:
> On Fri, Jan 04, 2019 at 09:43:54AM -0800, Christoph Hellwig wrote:
> > > +	u64 dma_mask = dma_get_mask(dev);
> > 
> > This is not a driver API.  I think what you want is
> > dma_get_required_mask to query the mask.  But in that case
> > you still need to always actually set a mask in the driver as well.
> 
> That's slightly different from what I want to do here. The purpose of
> this part of the patch is that when the SDHCI hardware supports only 32
> bits of address space, then we want to prevent 64-bit addressing mode
> from being used because it isn't useful.
> 
> So what we do want to check here is the DMA mask configured by the
> driver (or the default set by the bus, or wherever it came from).
> dma_get_required_mask() returns the DMA mask required to address all of
> system memory. That would perhaps be a useful additional check, but it's
> orthogonal to what I'm trying to do here.
> 
> Is there something else appropriate that I could use to query the DMA
> mask set for a device?

I think the problem is that you try to mix up responsibility of who
needs to set the DMA mask.  Either we want the core sdhci code set
it as we do, or we move it to the driver, probably optionally.

So instead of trying to read something back you should either:

 - add a set_dma_mask to override it
 - add a 64bit_mode_dma_mask field to strct mmc_host, initialize
   that to 64-bit by default and let host drivers override it

While the first one creates a little more boilerplate code it actually
looks cleaner to me.  The added benefit is that it can also replace the
usage of SDHCI_QUIRK2_BROKEN_64_BIT_DMA.