Well that's far from the two-liner I had in mind to fix our bounce-buffers
problem with Tegra, but it feels correct. Testing for ACPI and PCI would be
appreciated (I am rather confident that ACPI will be ok, but the PCI part
should be reviewed by someone who knows better).
64-bit capable devices are supposed to set their own DMA mask. Currently
this does not happen for sdhci devices excepted for the two (sdhci-acpi
and sdhci-pci) that define a enable_dma() hook and do it there. However
this hook is called from several places while DMA mask is supposed to
be set only once ; for instance the sdhci-acpi driver maintains a flag
just to make sure the DMA mask is set only upon the first call of this
hook.
For the vast majority of drivers that do not define a enable_dma() hook, the
default 32-bit DMA mask is used and there is a risk of using unneeded bounce
buffers on hosts capable of 64-bit addressing.
The first patch adds a default DMA mask setting function that is called when
a DMA-capable host is added. It tries to set sane DMA masks according to the
device's reported capabilities.
The addition of this function seems to make the same code in sdhci-acpi and
sdhci-pci redundant, so it is removed from these drivers. On top of making
this series a negative line count, it also removes one usage of the obsolete
pci_set_dma_mask() function.
Thanks to Arnd for the insightful discussion that led to this.
Changes since v2:
- Generalize the solution to all sdhci drivers instead of just Tegra
- Remove supposedly-redundant code from the sdhci-acpi and sdhci-pci drivers
Alexandre Courbot (3):
mmc: sdhci: Set DMA mask when adding host
mmc: sdhci-acpi: Remove enable_dma() hook
mmc: sdhci-pci: Do not set DMA mask in enable_dma()
drivers/mmc/host/sdhci-acpi.c | 30 -------------------------
drivers/mmc/host/sdhci-pci-core.c | 15 -------------
drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++------
3 files changed, 39 insertions(+), 52 deletions(-)
--
2.7.2
DMA mask will already be set by sdhci_set_dma_mask(), which
is equivalent to the removed code since pci_set_dma_mask()
expands to its DMA-API counterpart.
There should also be no reason to set the DMA mask after probe.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/mmc/host/sdhci-pci-core.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index df3b8eced8c4..62aa5d0efcee 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -1302,7 +1302,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
{
struct sdhci_pci_slot *slot;
struct pci_dev *pdev;
- int ret = -1;
slot = sdhci_priv(host);
pdev = slot->chip->pdev;
@@ -1314,20 +1313,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
"doesn't fully claim to support it.\n");
}
- if (host->flags & SDHCI_USE_64_BIT_DMA) {
- if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) {
- host->flags &= ~SDHCI_USE_64_BIT_DMA;
- } else {
- ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
- if (ret)
- dev_warn(&pdev->dev, "Failed to set 64-bit DMA mask\n");
- }
- }
- if (ret)
- ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- if (ret)
- return ret;
-
pci_set_master(pdev);
return 0;
--
2.7.2
Set the DMA mask in sdhci_add_host() after we determined the
capabilities of the device. 64-bit devices in particular are given the
proper mask that ensures bounce buffers are not used.
Also disable DMA if no proper DMA mask can be set, as the DMA-API
documentation specifies.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fd9139947fa3..00fb45ba6f39 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2857,6 +2857,34 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
EXPORT_SYMBOL_GPL(sdhci_alloc_host);
+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;
+
+ 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));
+ }
+
+ /* 32-bit mask as default & fallback */
+ if (ret) {
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+ if (ret)
+ pr_warn("%s: Failed to set 32-bit DMA mask.\n",
+ mmc_hostname(mmc));
+ }
+
+ return ret;
+}
+
int sdhci_add_host(struct sdhci_host *host)
{
struct mmc_host *mmc;
@@ -2932,13 +2960,17 @@ int sdhci_add_host(struct sdhci_host *host)
host->flags |= SDHCI_USE_64_BIT_DMA;
if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
- if (host->ops->enable_dma) {
- if (host->ops->enable_dma(host)) {
- pr_warn("%s: No suitable DMA available - falling back to PIO\n",
- mmc_hostname(mmc));
- host->flags &=
- ~(SDHCI_USE_SDMA | SDHCI_USE_ADMA);
- }
+ ret = sdhci_set_dma_mask(host);
+
+ if (!ret && host->ops->enable_dma)
+ ret = host->ops->enable_dma(host);
+
+ if (ret) {
+ pr_warn("%s: No suitable DMA available - falling back to PIO\n",
+ mmc_hostname(mmc));
+ host->flags &= ~(SDHCI_USE_SDMA | SDHCI_USE_ADMA);
+
+ ret = 0;
}
}
--
2.7.2
This hook was solely used to set the DMA mask, which is now done
by the newly-added sdhci_set_dma_mask() function.
The use of a flag to ensure the mask is only set once is a strong hint
that it should not have been done there anyway.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/mmc/host/sdhci-acpi.c | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 195ff0853dc8..416da6f3629c 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -75,7 +75,6 @@ struct sdhci_acpi_host {
const struct sdhci_acpi_slot *slot;
struct platform_device *pdev;
bool use_runtime_pm;
- bool dma_setup;
};
static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
@@ -83,33 +82,6 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
return c->slot && (c->slot->flags & flag);
}
-static int sdhci_acpi_enable_dma(struct sdhci_host *host)
-{
- struct sdhci_acpi_host *c = sdhci_priv(host);
- struct device *dev = &c->pdev->dev;
- int err = -1;
-
- if (c->dma_setup)
- return 0;
-
- if (host->flags & SDHCI_USE_64_BIT_DMA) {
- if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) {
- host->flags &= ~SDHCI_USE_64_BIT_DMA;
- } else {
- err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
- if (err)
- dev_warn(dev, "Failed to set 64-bit DMA mask\n");
- }
- }
-
- if (err)
- err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
-
- c->dma_setup = !err;
-
- return err;
-}
-
static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
{
u8 reg;
@@ -127,7 +99,6 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
static const struct sdhci_ops sdhci_acpi_ops_dflt = {
.set_clock = sdhci_set_clock,
- .enable_dma = sdhci_acpi_enable_dma,
.set_bus_width = sdhci_set_bus_width,
.reset = sdhci_reset,
.set_uhs_signaling = sdhci_set_uhs_signaling,
@@ -135,7 +106,6 @@ static const struct sdhci_ops sdhci_acpi_ops_dflt = {
static const struct sdhci_ops sdhci_acpi_ops_int = {
.set_clock = sdhci_set_clock,
- .enable_dma = sdhci_acpi_enable_dma,
.set_bus_width = sdhci_set_bus_width,
.reset = sdhci_reset,
.set_uhs_signaling = sdhci_set_uhs_signaling,
--
2.7.2
On Friday 04 March 2016 19:38:43 Alexandre Courbot wrote:
> Set the DMA mask in sdhci_add_host() after we determined the
> capabilities of the device. 64-bit devices in particular are given the
> proper mask that ensures bounce buffers are not used.
>
> Also disable DMA if no proper DMA mask can be set, as the DMA-API
> documentation specifies.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
> ---
> drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index fd9139947fa3..00fb45ba6f39 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2857,6 +2857,34 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
>
> EXPORT_SYMBOL_GPL(sdhci_alloc_host);
>
> +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;
> +
> + 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));
> + }
> +
I think you need to disable the SDHCI_USE_64_BIT_DMA flag when
dma_set_mask_and_coherent() fails here. Otherwise looks good.
Arnd
On Friday 04 March 2016 19:38:44 Alexandre Courbot wrote:
> This hook was solely used to set the DMA mask, which is now done
> by the newly-added sdhci_set_dma_mask() function.
>
> The use of a flag to ensure the mask is only set once is a strong hint
> that it should not have been done there anyway.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
>
Acked-by: Arnd Bergmann <[email protected]>
On Friday 04 March 2016 19:38:45 Alexandre Courbot wrote:
> DMA mask will already be set by sdhci_set_dma_mask(), which
> is equivalent to the removed code since pci_set_dma_mask()
> expands to its DMA-API counterpart.
>
> There should also be no reason to set the DMA mask after probe.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
>
Acked-by: Arnd Bergmann <[email protected]>
On Friday 04 March 2016 19:38:42 Alexandre Courbot wrote:
> Well that's far from the two-liner I had in mind to fix our bounce-buffers
> problem with Tegra, but it feels correct. Testing for ACPI and PCI would be
> appreciated (I am rather confident that ACPI will be ok, but the PCI part
> should be reviewed by someone who knows better).
>
> 64-bit capable devices are supposed to set their own DMA mask. Currently
> this does not happen for sdhci devices excepted for the two (sdhci-acpi
> and sdhci-pci) that define a enable_dma() hook and do it there. However
> this hook is called from several places while DMA mask is supposed to
> be set only once ; for instance the sdhci-acpi driver maintains a flag
> just to make sure the DMA mask is set only upon the first call of this
> hook.
>
> For the vast majority of drivers that do not define a enable_dma() hook, the
> default 32-bit DMA mask is used and there is a risk of using unneeded bounce
> buffers on hosts capable of 64-bit addressing.
>
> The first patch adds a default DMA mask setting function that is called when
> a DMA-capable host is added. It tries to set sane DMA masks according to the
> device's reported capabilities.
>
> The addition of this function seems to make the same code in sdhci-acpi and
> sdhci-pci redundant, so it is removed from these drivers. On top of making
> this series a negative line count, it also removes one usage of the obsolete
> pci_set_dma_mask() function.
>
> Thanks to Arnd for the insightful discussion that led to this.
Looks great overall, much simpler and saner in the new version. I found
one small bug, which appears to have been preexisting, but you moved
it to a different file now.
Arnd
On Sat, Mar 5, 2016 at 12:57 AM, Arnd Bergmann <[email protected]> wrote:
> On Friday 04 March 2016 19:38:43 Alexandre Courbot wrote:
>> Set the DMA mask in sdhci_add_host() after we determined the
>> capabilities of the device. 64-bit devices in particular are given the
>> proper mask that ensures bounce buffers are not used.
>>
>> Also disable DMA if no proper DMA mask can be set, as the DMA-API
>> documentation specifies.
>>
>> Signed-off-by: Alexandre Courbot <[email protected]>
>> ---
>> drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index fd9139947fa3..00fb45ba6f39 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2857,6 +2857,34 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
>>
>> EXPORT_SYMBOL_GPL(sdhci_alloc_host);
>>
>> +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;
>> +
>> + 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));
>> + }
>> +
>
> I think you need to disable the SDHCI_USE_64_BIT_DMA flag when
> dma_set_mask_and_coherent() fails here. Otherwise looks good.
Ah, you're right, thanks. v4 is on the way.