* [PATCH v2 0/2] mmc: sdhci: Set DMA mask if specified @ 2016-03-01 4:32 Alexandre Courbot 2016-03-01 4:32 ` [PATCH v2 1/2] mmc: sdhci: Set DMA mask Alexandre Courbot 2016-03-01 4:32 ` [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid " Alexandre Courbot 0 siblings, 2 replies; 11+ messages in thread From: Alexandre Courbot @ 2016-03-01 4:32 UTC (permalink / raw) To: Ulf Hansson, Arnd Bergmann, Stephen Warren, Thierry Reding Cc: linux-mmc, linux-tegra, linux-kernel, gnurou, Alexandre Courbot Allow host drivers to set the sdhci_host's dma_mask in order to specify the supported DMA range of a device. Without this, devices that are capable of 64-bit addressing might end up using bounce buffers because the default DMA mask is 32-bit. Changes since v1: - Set the DMA mask in the common code - Set the capabilities of the various Tegra controllers Alexandre Courbot (2): mmc: sdhci: Set DMA mask mmc: sdhci-tegra: Specify valid DMA mask drivers/mmc/host/sdhci-tegra.c | 8 ++++++++ drivers/mmc/host/sdhci.c | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-) -- 2.7.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] mmc: sdhci: Set DMA mask 2016-03-01 4:32 [PATCH v2 0/2] mmc: sdhci: Set DMA mask if specified Alexandre Courbot @ 2016-03-01 4:32 ` Alexandre Courbot 2016-03-01 21:30 ` Arnd Bergmann 2016-03-01 4:32 ` [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid " Alexandre Courbot 1 sibling, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2016-03-01 4:32 UTC (permalink / raw) To: Ulf Hansson, Arnd Bergmann, Stephen Warren, Thierry Reding Cc: linux-mmc, linux-tegra, linux-kernel, gnurou, Alexandre Courbot Set the DMA mask if specified by the host's dma_mask member, and not only if the DMA flags are set. Use dma_set_mask_and_coherent() to set it instead of messing directly with the device's dma_mask. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/mmc/host/sdhci.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index fd9139947fa3..0693f52d238a 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2997,10 +2997,12 @@ int sdhci_add_host(struct sdhci_host *host) * mask, but PIO does not need the hw shim so we set a new * mask here in that case. */ - if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) { + if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) host->dma_mask = DMA_BIT_MASK(64); - mmc_dev(mmc)->dma_mask = &host->dma_mask; - } + + if (host->dma_mask && + dma_set_mask_and_coherent(mmc_dev(mmc), host->dma_mask)) + pr_warn("%s: cannot set DMA mask\n", mmc_hostname(mmc)); if (host->version >= SDHCI_SPEC_300) host->max_clk = (caps[0] & SDHCI_CLOCK_V3_BASE_MASK) -- 2.7.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mmc: sdhci: Set DMA mask 2016-03-01 4:32 ` [PATCH v2 1/2] mmc: sdhci: Set DMA mask Alexandre Courbot @ 2016-03-01 21:30 ` Arnd Bergmann 2016-03-04 6:09 ` Alexandre Courbot 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2016-03-01 21:30 UTC (permalink / raw) To: Alexandre Courbot Cc: Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra, linux-kernel, gnurou On Tuesday 01 March 2016 13:32:43 Alexandre Courbot wrote: > */ > - if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) { > + if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) > host->dma_mask = DMA_BIT_MASK(64); > - mmc_dev(mmc)->dma_mask = &host->dma_mask; > - } > + > + if (host->dma_mask && > + dma_set_mask_and_coherent(mmc_dev(mmc), host->dma_mask)) > + pr_warn("%s: cannot set DMA mask\n", mmc_hostname(mmc)); > Looks good, Acked-by: Arnd Bergmann <arnd@arndb.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mmc: sdhci: Set DMA mask 2016-03-01 21:30 ` Arnd Bergmann @ 2016-03-04 6:09 ` Alexandre Courbot 0 siblings, 0 replies; 11+ messages in thread From: Alexandre Courbot @ 2016-03-04 6:09 UTC (permalink / raw) To: Arnd Bergmann Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Wed, Mar 2, 2016 at 6:30 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 01 March 2016 13:32:43 Alexandre Courbot wrote: >> */ >> - if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) { >> + if (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))) >> host->dma_mask = DMA_BIT_MASK(64); >> - mmc_dev(mmc)->dma_mask = &host->dma_mask; >> - } >> + >> + if (host->dma_mask && >> + dma_set_mask_and_coherent(mmc_dev(mmc), host->dma_mask)) >> + pr_warn("%s: cannot set DMA mask\n", mmc_hostname(mmc)); >> > > Looks good, > > Acked-by: Arnd Bergmann <arnd@arndb.de> Withdrawing this patch after discussing with Arnd. The test detects devices that are not capable of DMA, so it makes no sense to call a DMA function right after that. A better solution to the problem this patch tried to solve is on the way. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-01 4:32 [PATCH v2 0/2] mmc: sdhci: Set DMA mask if specified Alexandre Courbot 2016-03-01 4:32 ` [PATCH v2 1/2] mmc: sdhci: Set DMA mask Alexandre Courbot @ 2016-03-01 4:32 ` Alexandre Courbot 2016-03-01 21:34 ` Arnd Bergmann 1 sibling, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2016-03-01 4:32 UTC (permalink / raw) To: Ulf Hansson, Arnd Bergmann, Stephen Warren, Thierry Reding Cc: linux-mmc, linux-tegra, linux-kernel, gnurou, Alexandre Courbot On T210, the sdhci controller can address more than 32 bits of address space. Failing to express this fact results in the use of bounce buffers and affects performance. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/mmc/host/sdhci-tegra.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 83c4bf7bc16c..66808ac64db5 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -25,6 +25,7 @@ #include <linux/mmc/mmc.h> #include <linux/mmc/slot-gpio.h> #include <linux/gpio/consumer.h> +#include <linux/dma-mapping.h> #include "sdhci-pltfm.h" @@ -52,6 +53,7 @@ struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; u32 nvquirks; + u64 dma_mask; }; struct sdhci_tegra { @@ -289,6 +291,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 = { @@ -307,6 +310,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra30 = { .nvquirks = NVQUIRK_ENABLE_SDHCI_SPEC_300 | NVQUIRK_ENABLE_SDR50 | NVQUIRK_ENABLE_SDR104, + .dma_mask = DMA_BIT_MASK(32), }; static const struct sdhci_ops tegra114_sdhci_ops = { @@ -338,6 +342,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra114 = { .nvquirks = NVQUIRK_ENABLE_SDR50 | NVQUIRK_ENABLE_DDR50 | NVQUIRK_ENABLE_SDR104, + .dma_mask = DMA_BIT_MASK(32), }; static const struct sdhci_pltfm_data sdhci_tegra210_pdata = { @@ -353,6 +358,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_mask = DMA_BIT_MASK(34), }; static const struct of_device_id sdhci_tegra_dt_match[] = { @@ -385,6 +391,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev) return PTR_ERR(host); pltfm_host = sdhci_priv(host); + host->dma_mask = soc_data->dma_mask; + tegra_host = devm_kzalloc(&pdev->dev, sizeof(*tegra_host), GFP_KERNEL); if (!tegra_host) { dev_err(mmc_dev(host->mmc), "failed to allocate tegra_host\n"); -- 2.7.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-01 4:32 ` [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid " Alexandre Courbot @ 2016-03-01 21:34 ` Arnd Bergmann 2016-03-02 10:36 ` Alexandre Courbot 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2016-03-01 21:34 UTC (permalink / raw) To: Alexandre Courbot Cc: Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra, linux-kernel, gnurou On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote: > On T210, the sdhci controller can address more than 32 bits of address > space. Failing to express this fact results in the use of bounce > buffers and affects performance. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA) flags that are checked in the first patch? > @@ -289,6 +291,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), > }; Can you describe what the specific bug is in these controllers? Do you mean they support SDHCI_USE_SDMA or SDHCI_USE_ADMA in theory but you still have to prevent them from using high addresses? > @@ -353,6 +358,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_mask = DMA_BIT_MASK(34), > }; > > static const struct of_device_id sdhci_tegra_dt_match[] = { This one still completely weirds me out. What kind of odd limitation does the controller have in Tegra 210? Are there actually any machines with more than 16GB? Arnd ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-01 21:34 ` Arnd Bergmann @ 2016-03-02 10:36 ` Alexandre Courbot 2016-03-02 11:25 ` Arnd Bergmann 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2016-03-02 10:36 UTC (permalink / raw) To: Arnd Bergmann Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Wed, Mar 2, 2016 at 6:34 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote: >> On T210, the sdhci controller can address more than 32 bits of address >> space. Failing to express this fact results in the use of bounce >> buffers and affects performance. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA) > flags that are checked in the first patch? The test is against (!(host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))), (see the '!') so it will be true (and the DMA mask will be set) if both flags are *not* set (why we set the mask to 64 bits here in that case, I don't know). T210 is capable of SDMA, so we cannot use this condition for that purpose (maybe you missed the '!', in which case I understand why you were surprised). > >> @@ -289,6 +291,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), >> }; > > Can you describe what the specific bug is in these controllers? Do you mean they > support SDHCI_USE_SDMA or SDHCI_USE_ADMA in theory but you still have to prevent > them from using high addresses? Ok, I think you probably missed the '!' then. :) > >> @@ -353,6 +358,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_mask = DMA_BIT_MASK(34), >> }; >> >> static const struct of_device_id sdhci_tegra_dt_match[] = { > > This one still completely weirds me out. What kind of odd limitation does > the controller have in Tegra 210? > > Are there actually any machines with more than 16GB? It is not a limitation of the controller - I am just limiting the mask to the range of physical memory we can ever access on T210. My intent here is to overcome the default 32-bit mask, not to limit the range, so I could have set a 64-bit mask if not for my OCD. :P But actually looking at how the various flags are interpreted in sdhci_add_host(), I see the following: /* * 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 (caps[0] & SDHCI_CAN_64BIT) host->flags |= SDHCI_USE_64_BIT_DMA; Since this relies on what the hardware declares being capable of and is set on T210, I am very tempted to set a 64-bit dma_mask here and call it a day, but the above comment seems to suggest that this should have been done before. If you think this is cool though, I will just do that and in conjunction with patch 1 this will do the job nicely. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-02 10:36 ` Alexandre Courbot @ 2016-03-02 11:25 ` Arnd Bergmann 2016-03-04 6:08 ` Alexandre Courbot 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2016-03-02 11:25 UTC (permalink / raw) To: Alexandre Courbot Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Wednesday 02 March 2016 19:36:23 Alexandre Courbot wrote: > On Wed, Mar 2, 2016 at 6:34 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote: > >> On T210, the sdhci controller can address more than 32 bits of address > >> space. Failing to express this fact results in the use of bounce > >> buffers and affects performance. > >> > >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > > > I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA) > > flags that are checked in the first patch? > > The test is against (!(host->flags & (SDHCI_USE_SDMA | > SDHCI_USE_ADMA))), (see the '!') so it will be true (and the DMA mask > will be set) if both flags are *not* set (why we set the mask to 64 > bits here in that case, I don't know). > > T210 is capable of SDMA, so we cannot use this condition for that > purpose (maybe you missed the '!', in which case I understand why you > were surprised). Ok, I see now that this code was just setting a fake mask in case of PIO mode, which doesn't apply here. That in turn means that your first patch is wrong though: For a device that is not DMA capable (neither SDMA nor ADMA claimed to be supported), we should not call dma_set_mask_and_coherent() because that is likely to fail as well. It's an ugly hack to just override the mask in that case, and I'd say it requires a comment explaining what is going on. > >> @@ -289,6 +291,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), > >> }; > > > > Can you describe what the specific bug is in these controllers? Do you mean they > > support SDHCI_USE_SDMA or SDHCI_USE_ADMA in theory but you still have to prevent > > them from using high addresses? > > Ok, I think you probably missed the '!' then. :) I missed the larger context of that check too, but I think I've got it now. > >> @@ -353,6 +358,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_mask = DMA_BIT_MASK(34), > >> }; > >> > >> static const struct of_device_id sdhci_tegra_dt_match[] = { > > > > This one still completely weirds me out. What kind of odd limitation does > > the controller have in Tegra 210? > > > > Are there actually any machines with more than 16GB? > > It is not a limitation of the controller - I am just limiting the mask > to the range of physical memory we can ever access on T210. My intent > here is to overcome the default 32-bit mask, not to limit the range, > so I could have set a 64-bit mask if not for my OCD. :P > > But actually looking at how the various flags are interpreted in > sdhci_add_host(), I see the following: > > /* > * 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 (caps[0] & SDHCI_CAN_64BIT) > host->flags |= SDHCI_USE_64_BIT_DMA; > > Since this relies on what the hardware declares being capable of and > is set on T210, I am very tempted to set a 64-bit dma_mask here and > call it a day, but the above comment seems to suggest that this should > have been done before. Right, that sounds good, that also makes it independent of the specific Tegra SoC, right? > If you think this is cool though, I will just do that and in > conjunction with patch 1 this will do the job nicely. as mentioned above, I now have doubts about patch 1, why do you still need this now? Arnd ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-02 11:25 ` Arnd Bergmann @ 2016-03-04 6:08 ` Alexandre Courbot 2016-03-04 6:43 ` Alexandre Courbot 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2016-03-04 6:08 UTC (permalink / raw) To: Arnd Bergmann Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Wed, Mar 2, 2016 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 02 March 2016 19:36:23 Alexandre Courbot wrote: >> On Wed, Mar 2, 2016 at 6:34 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote: >> >> On T210, the sdhci controller can address more than 32 bits of address >> >> space. Failing to express this fact results in the use of bounce >> >> buffers and affects performance. >> >> >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> > >> > I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA) >> > flags that are checked in the first patch? >> >> The test is against (!(host->flags & (SDHCI_USE_SDMA | >> SDHCI_USE_ADMA))), (see the '!') so it will be true (and the DMA mask >> will be set) if both flags are *not* set (why we set the mask to 64 >> bits here in that case, I don't know). >> >> T210 is capable of SDMA, so we cannot use this condition for that >> purpose (maybe you missed the '!', in which case I understand why you >> were surprised). > > Ok, I see now that this code was just setting a fake mask in case > of PIO mode, which doesn't apply here. That in turn means that > your first patch is wrong though: > > For a device that is not DMA capable (neither SDMA nor ADMA > claimed to be supported), we should not call dma_set_mask_and_coherent() > because that is likely to fail as well. It's an ugly hack to > just override the mask in that case, and I'd say it requires > a comment explaining what is going on. Yeah, I'm not too sure what is the point of setting the fake mask to be honest, but you are definitely right that it is a contradiction to call a DMA function on a device that is not DMA-capable. > >> >> @@ -289,6 +291,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), >> >> }; >> > >> > Can you describe what the specific bug is in these controllers? Do you mean they >> > support SDHCI_USE_SDMA or SDHCI_USE_ADMA in theory but you still have to prevent >> > them from using high addresses? >> >> Ok, I think you probably missed the '!' then. :) > > I missed the larger context of that check too, but I think I've got it now. > >> >> @@ -353,6 +358,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_mask = DMA_BIT_MASK(34), >> >> }; >> >> >> >> static const struct of_device_id sdhci_tegra_dt_match[] = { >> > >> > This one still completely weirds me out. What kind of odd limitation does >> > the controller have in Tegra 210? >> > >> > Are there actually any machines with more than 16GB? >> >> It is not a limitation of the controller - I am just limiting the mask >> to the range of physical memory we can ever access on T210. My intent >> here is to overcome the default 32-bit mask, not to limit the range, >> so I could have set a 64-bit mask if not for my OCD. :P >> >> But actually looking at how the various flags are interpreted in >> sdhci_add_host(), I see the following: >> >> /* >> * 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 (caps[0] & SDHCI_CAN_64BIT) >> host->flags |= SDHCI_USE_64_BIT_DMA; >> >> Since this relies on what the hardware declares being capable of and >> is set on T210, I am very tempted to set a 64-bit dma_mask here and >> call it a day, but the above comment seems to suggest that this should >> have been done before. > > Right, that sounds good, that also makes it independent of the specific > Tegra SoC, right? Yes - it would just be a 2-liner that should set things properly for all 64-bit capable controllers. >> If you think this is cool though, I will just do that and in >> conjunction with patch 1 this will do the job nicely. > > as mentioned above, I now have doubts about patch 1, why do you still > need this now? We would not need patch 1 anymore. Let me send this and see if it looks better. Thanks, Alex. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-04 6:08 ` Alexandre Courbot @ 2016-03-04 6:43 ` Alexandre Courbot 2016-03-04 8:38 ` Arnd Bergmann 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2016-03-04 6:43 UTC (permalink / raw) To: Arnd Bergmann Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Fri, Mar 4, 2016 at 3:08 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Wed, Mar 2, 2016 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wednesday 02 March 2016 19:36:23 Alexandre Courbot wrote: >>> On Wed, Mar 2, 2016 at 6:34 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> > On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote: >>> >> On T210, the sdhci controller can address more than 32 bits of address >>> >> space. Failing to express this fact results in the use of bounce >>> >> buffers and affects performance. >>> >> >>> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >>> > >>> > I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA) >>> > flags that are checked in the first patch? >>> >>> The test is against (!(host->flags & (SDHCI_USE_SDMA | >>> SDHCI_USE_ADMA))), (see the '!') so it will be true (and the DMA mask >>> will be set) if both flags are *not* set (why we set the mask to 64 >>> bits here in that case, I don't know). >>> >>> T210 is capable of SDMA, so we cannot use this condition for that >>> purpose (maybe you missed the '!', in which case I understand why you >>> were surprised). >> >> Ok, I see now that this code was just setting a fake mask in case >> of PIO mode, which doesn't apply here. That in turn means that >> your first patch is wrong though: >> >> For a device that is not DMA capable (neither SDMA nor ADMA >> claimed to be supported), we should not call dma_set_mask_and_coherent() >> because that is likely to fail as well. It's an ugly hack to >> just override the mask in that case, and I'd say it requires >> a comment explaining what is going on. > > Yeah, I'm not too sure what is the point of setting the fake mask to > be honest, but you are definitely right that it is a contradiction to > call a DMA function on a device that is not DMA-capable. Ah, I finally got it - we are just setting it to the *address* of host->dma_mask so the device's DMA mask does not end up being a NULL pointer. That actually changes things a bit. DMA-capable devices are clearly expected to set the mask themselves, but the only one to do it is host/mtk-sd.c. And dma_set_mask() is only called in dw_mmc and sdhci-acpi's enable_dma callback. This means most DMA-capable devices (including Tegra, but not only) are simply left with no DMA setup at all. Probably we can detect when the host did not do any DMA setup in the probe function and attempt some sane defaults depending on what the hardware says it is capable of? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask 2016-03-04 6:43 ` Alexandre Courbot @ 2016-03-04 8:38 ` Arnd Bergmann 0 siblings, 0 replies; 11+ messages in thread From: Arnd Bergmann @ 2016-03-04 8:38 UTC (permalink / raw) To: Alexandre Courbot Cc: Alexandre Courbot, Ulf Hansson, Stephen Warren, Thierry Reding, linux-mmc, linux-tegra@vger.kernel.org, Linux Kernel Mailing List On Friday 04 March 2016 15:43:56 Alexandre Courbot wrote: > > > > Yeah, I'm not too sure what is the point of setting the fake mask to > > be honest, but you are definitely right that it is a contradiction to > > call a DMA function on a device that is not DMA-capable. > > Ah, I finally got it - we are just setting it to the *address* of > host->dma_mask so the device's DMA mask does not end up being a NULL > pointer. > > That actually changes things a bit. DMA-capable devices are clearly > expected to set the mask themselves, but the only one to do it is > host/mtk-sd.c. And dma_set_mask() is only called in dw_mmc and > sdhci-acpi's enable_dma callback. > > This means most DMA-capable devices (including Tegra, but not only) > are simply left with no DMA setup at all. > > Probably we can detect when the host did not do any DMA setup in the > probe function and attempt some sane defaults depending on what the > hardware says it is capable of? When the host leaves an empty DMA mask, the intended meaning is that the device is not on a DMA capable bus, so if we run into that case, we should instead fix the creation of the device rather than the driver that looks at the data. Arnd ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-03-04 8:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-01 4:32 [PATCH v2 0/2] mmc: sdhci: Set DMA mask if specified Alexandre Courbot 2016-03-01 4:32 ` [PATCH v2 1/2] mmc: sdhci: Set DMA mask Alexandre Courbot 2016-03-01 21:30 ` Arnd Bergmann 2016-03-04 6:09 ` Alexandre Courbot 2016-03-01 4:32 ` [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid " Alexandre Courbot 2016-03-01 21:34 ` Arnd Bergmann 2016-03-02 10:36 ` Alexandre Courbot 2016-03-02 11:25 ` Arnd Bergmann 2016-03-04 6:08 ` Alexandre Courbot 2016-03-04 6:43 ` Alexandre Courbot 2016-03-04 8:38 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).