[PATCH] mmc: sdhci-tegra: Set DMA mask

[PATCH] mmc: sdhci-tegra: Set DMA mask

[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>
---
I am pretty sure this one is wrong in some way, but just to get the ball
rolling as the use of bounce buffers is currently quite heavy on Jetson TX1.

Thierry, Stephen, could you confirm that I got the DMA masks correctly? I
am not sure about the actual addressable size on TX1, and also suspect TK1 can
also address more than 32 bits.

Also, I noticed that sdhci_host has a dma_mask member which I thought would do
the trick but actually doesn't seem to be used for anything useful. Could the
MMC maintainers comment on this and let me know if the DMA mask setting should
be moved at the core level instead of being done per-driver?

 drivers/mmc/host/sdhci-tegra.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 83c4bf7bc16c..50a97c5845e8 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[] = {
@@ -380,6 +386,9 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 		return -EINVAL;
 	soc_data = match->data;
 
+	if (soc_data->dma_mask)
+		dma_set_mask(&pdev->dev, soc_data->dma_mask);
+
 	host = sdhci_pltfm_init(pdev, soc_data->pdata, 0);
 	if (IS_ERR(host))
 		return PTR_ERR(host);
-- 
2.7.1

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Arnd Bergmann]

On Wednesday 24 February 2016 18:11:19 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 am pretty sure this one is wrong in some way, but just to get the ball
> rolling as the use of bounce buffers is currently quite heavy on Jetson TX1.
> 
> Thierry, Stephen, could you confirm that I got the DMA masks correctly? I
> am not sure about the actual addressable size on TX1, and also suspect TK1 can
> also address more than 32 bits.
> 
> Also, I noticed that sdhci_host has a dma_mask member which I thought would do
> the trick but actually doesn't seem to be used for anything useful. Could the
> MMC maintainers comment on this and let me know if the DMA mask setting should
> be moved at the core level instead of being done per-driver?

So the question is what the DMA capabilities of the sdhci device are.

Usually I think SDHCI should just support a 64-bit mask, and you can
request that in the driver, but the platform might reject it, e.g.
if the parent bus is lacking a dma-ranges property.

On 32-bit platforms with no RAM above the 4GB boundary, setting a 64-bit
mask typically succeeds, because there is no harm in using it.

You should only set a 34-bit mask in the specific case that:

* SDHCI reports that it supports 64-bit addressing
* The parent bus supports 64-bit addressing and correctly sets up
  its dma-ranges property
* The device is connected incorrectly to the parent bus and
  any access above 0x400000000ull fail to end up in the correct
  memory for this particular device, but not other devices on the
  same bus.

	Arnd

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Alexandre Courbot]

On Wed, Feb 24, 2016 at 9:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 24 February 2016 18:11:19 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 am pretty sure this one is wrong in some way, but just to get the ball
>> rolling as the use of bounce buffers is currently quite heavy on Jetson TX1.
>>
>> Thierry, Stephen, could you confirm that I got the DMA masks correctly? I
>> am not sure about the actual addressable size on TX1, and also suspect TK1 can
>> also address more than 32 bits.
>>
>> Also, I noticed that sdhci_host has a dma_mask member which I thought would do
>> the trick but actually doesn't seem to be used for anything useful. Could the
>> MMC maintainers comment on this and let me know if the DMA mask setting should
>> be moved at the core level instead of being done per-driver?
>
> So the question is what the DMA capabilities of the sdhci device are.
>
> Usually I think SDHCI should just support a 64-bit mask, and you can
> request that in the driver, but the platform might reject it, e.g.
> if the parent bus is lacking a dma-ranges property.
>
> On 32-bit platforms with no RAM above the 4GB boundary, setting a 64-bit
> mask typically succeeds, because there is no harm in using it.
>
> You should only set a 34-bit mask in the specific case that:
>
> * SDHCI reports that it supports 64-bit addressing
> * The parent bus supports 64-bit addressing and correctly sets up
>   its dma-ranges property
> * The device is connected incorrectly to the parent bus and
>   any access above 0x400000000ull fail to end up in the correct
>   memory for this particular device, but not other devices on the
>   same bus.

Thanks for the clarification. The hardware is definitely capable of >
32 bits addressing (how much more exactly, I still have to figure out
as Stephen pointed out). Parent bus is the platform bus (root node in
the DT) so I'm not too sure how the dma-ranges property should be used
in that case (as of today we have no dma-ranges property at all in
Tegra DTs). We could maybe specify it at the root level (I see at
least one arm device doing that), but then we need to make sure every
platform device supports the same range.

Actually even if we specify a dma-ranges on the parent DT node, the
DMA range will still be limited to 32 bits because of the following
code in of_dma_configure():

    /*
     * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
     * setup the correct supported mask.
     */
    if (!dev->coherent_dma_mask)
        dev->coherent_dma_mask = DMA_BIT_MASK(32);

    /*
     * Set it to coherent_dma_mask by default if the architecture
     * code has not set it.
     */
    if (!dev->dma_mask)
        dev->dma_mask = &dev->coherent_dma_mask;

    ....
    /* gets dma-ranges into dma_addr and size */
    ....


    *dev->dma_mask = min((*dev->dma_mask),
                 DMA_BIT_MASK(ilog2(dma_addr + size)));

So unless the DMA mask is set on the device before of_dma_configure()
is called, the min() statement will choose the 32 bits mask that has
been previously set. So IIUC in any case, the driver will need to call
dma_set_mask()

Can I have your thoughts on this? Am I missing something?

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Arnd Bergmann]

On Thursday 25 February 2016 18:49:06 Alexandre Courbot wrote:
> On Wed, Feb 24, 2016 at 9:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 24 February 2016 18:11:19 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 am pretty sure this one is wrong in some way, but just to get the ball
> >> rolling as the use of bounce buffers is currently quite heavy on Jetson TX1.
> >>
> >> Thierry, Stephen, could you confirm that I got the DMA masks correctly? I
> >> am not sure about the actual addressable size on TX1, and also suspect TK1 can
> >> also address more than 32 bits.
> >>
> >> Also, I noticed that sdhci_host has a dma_mask member which I thought would do
> >> the trick but actually doesn't seem to be used for anything useful. Could the
> >> MMC maintainers comment on this and let me know if the DMA mask setting should
> >> be moved at the core level instead of being done per-driver?
> >
> > So the question is what the DMA capabilities of the sdhci device are.
> >
> > Usually I think SDHCI should just support a 64-bit mask, and you can
> > request that in the driver, but the platform might reject it, e.g.
> > if the parent bus is lacking a dma-ranges property.
> >
> > On 32-bit platforms with no RAM above the 4GB boundary, setting a 64-bit
> > mask typically succeeds, because there is no harm in using it.
> >
> > You should only set a 34-bit mask in the specific case that:
> >
> > * SDHCI reports that it supports 64-bit addressing
> > * The parent bus supports 64-bit addressing and correctly sets up
> >   its dma-ranges property
> > * The device is connected incorrectly to the parent bus and
> >   any access above 0x400000000ull fail to end up in the correct
> >   memory for this particular device, but not other devices on the
> >   same bus.
> 
> Thanks for the clarification. The hardware is definitely capable of >
> 32 bits addressing (how much more exactly, I still have to figure out
> as Stephen pointed out). Parent bus is the platform bus (root node in
> the DT) so I'm not too sure how the dma-ranges property should be used
> in that case (as of today we have no dma-ranges property at all in
> Tegra DTs).

I meant the actual physical bus. AXI buses that are typically used
can have either 32-bit or 64-bit addressing, but IIRC they wouldn't
have anything inbetween based on the way this protocol works (i.e.
not an actual set of address wires, but a data packet getting sent
around on a point to point link).

> We could maybe specify it at the root level (I see at
> least one arm device doing that), but then we need to make sure every
> platform device supports the same range.

I think there is still a bug in the code that will lead to
dma_set_mask just succeeding, regardless of what the bus supports,
and this should really really be fixed.

I think it will just keep working for platforms that accidentally
don't set the dma-ranges property, but it may be slower as we fall
back to swiotlb.

> Actually even if we specify a dma-ranges on the parent DT node, the
> DMA range will still be limited to 32 bits because of the following
> code in of_dma_configure():
> 
>     /*
>      * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>      * setup the correct supported mask.
>      */
>     if (!dev->coherent_dma_mask)
>         dev->coherent_dma_mask = DMA_BIT_MASK(32);
> 
>     /*
>      * Set it to coherent_dma_mask by default if the architecture
>      * code has not set it.
>      */
>     if (!dev->dma_mask)
>         dev->dma_mask = &dev->coherent_dma_mask;
> 
>     ....
>     /* gets dma-ranges into dma_addr and size */
>     ....
> 
> 
>     *dev->dma_mask = min((*dev->dma_mask),
>                  DMA_BIT_MASK(ilog2(dma_addr + size)));
> 
> So unless the DMA mask is set on the device before of_dma_configure()
> is called, the min() statement will choose the 32 bits mask that has
> been previously set. So IIUC in any case, the driver will need to call
> dma_set_mask()

Yes, the driver definitely has to call dma_set_mask(), the property of
the parent bus is used to make that fail when the bus doesn't support
it.

> Can I have your thoughts on this? Am I missing something?

One point: I think the dma_set_mask() probably should be in the
generic part of the sdhci driver, not the tegra specific portion.

I also forget how this really needs to interact with swiotlb. I know
we have discussed this a couple of times, but the result currently
is lost to me.
Maybe the answer was that if swiotlb or iommu are enabled, then
dma_set_mask() should always succeed, but the mask should not actually
be updated?

	Arnd

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Alexandre Courbot]

On Thu, Feb 25, 2016 at 11:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Actually even if we specify a dma-ranges on the parent DT node, the
>> DMA range will still be limited to 32 bits because of the following
>> code in of_dma_configure():
>>
>>     /*
>>      * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>>      * setup the correct supported mask.
>>      */
>>     if (!dev->coherent_dma_mask)
>>         dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>
>>     /*
>>      * Set it to coherent_dma_mask by default if the architecture
>>      * code has not set it.
>>      */
>>     if (!dev->dma_mask)
>>         dev->dma_mask = &dev->coherent_dma_mask;
>>
>>     ....
>>     /* gets dma-ranges into dma_addr and size */
>>     ....
>>
>>
>>     *dev->dma_mask = min((*dev->dma_mask),
>>                  DMA_BIT_MASK(ilog2(dma_addr + size)));
>>
>> So unless the DMA mask is set on the device before of_dma_configure()
>> is called, the min() statement will choose the 32 bits mask that has
>> been previously set. So IIUC in any case, the driver will need to call
>> dma_set_mask()
>
> Yes, the driver definitely has to call dma_set_mask(), the property of
> the parent bus is used to make that fail when the bus doesn't support
> it.

And that's where things seem to stop working: the driver's probe
function is invoked by the platform bus, *after* of_dma_configure() is
called. So unless I am missing something there is no way for the
driver to set the DMA mask in such a way that of_dma_configure() can
see it and do the right thing.

In other words, most of the DMA mask logic in of_dma_configure()
doesn't seem to have any effect (and a 32 bits mask will be set), at
least on the platform bus.

>> Can I have your thoughts on this? Am I missing something?
>
> One point: I think the dma_set_mask() probably should be in the
> generic part of the sdhci driver, not the tegra specific portion.

Ok, but then how does the generic part of the driver knows which DMA
mask applies to the device?

> I also forget how this really needs to interact with swiotlb. I know
> we have discussed this a couple of times, but the result currently
> is lost to me.
> Maybe the answer was that if swiotlb or iommu are enabled, then
> dma_set_mask() should always succeed, but the mask should not actually
> be updated?

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Arnd Bergmann]

On Friday 26 February 2016 16:24:34 Alexandre Courbot wrote:
> On Thu, Feb 25, 2016 at 11:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> Actually even if we specify a dma-ranges on the parent DT node, the
> >> DMA range will still be limited to 32 bits because of the following
> >> code in of_dma_configure():
> >>
> >>     /*
> >>      * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
> >>      * setup the correct supported mask.
> >>      */
> >>     if (!dev->coherent_dma_mask)
> >>         dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >>
> >>     /*
> >>      * Set it to coherent_dma_mask by default if the architecture
> >>      * code has not set it.
> >>      */
> >>     if (!dev->dma_mask)
> >>         dev->dma_mask = &dev->coherent_dma_mask;
> >>
> >>     ....
> >>     /* gets dma-ranges into dma_addr and size */
> >>     ....
> >>
> >>
> >>     *dev->dma_mask = min((*dev->dma_mask),
> >>                  DMA_BIT_MASK(ilog2(dma_addr + size)));
> >>
> >> So unless the DMA mask is set on the device before of_dma_configure()
> >> is called, the min() statement will choose the 32 bits mask that has
> >> been previously set. So IIUC in any case, the driver will need to call
> >> dma_set_mask()
> >
> > Yes, the driver definitely has to call dma_set_mask(), the property of
> > the parent bus is used to make that fail when the bus doesn't support
> > it.
> 
> And that's where things seem to stop working: the driver's probe
> function is invoked by the platform bus, *after* of_dma_configure() is
> called. So unless I am missing something there is no way for the
> driver to set the DMA mask in such a way that of_dma_configure() can
> see it and do the right thing.
> 
> In other words, most of the DMA mask logic in of_dma_configure()
> doesn't seem to have any effect (and a 32 bits mask will be set), at
> least on the platform bus.

That is correct: of_dma_configure has to set a 32-bit DMA mask
because that is the default that we expect to see in Linux drivers.

A lot of drivers don't call dma_set_mask() at all, so this is
the most reasonable value that typically works, unless the
device is more limited, or you want the extra performance you
get on devices that actually support a bigger mask.

> >> Can I have your thoughts on this? Am I missing something?
> >
> > One point: I think the dma_set_mask() probably should be in the
> > generic part of the sdhci driver, not the tegra specific portion.
> 
> Ok, but then how does the generic part of the driver knows which DMA
> mask applies to the device?

If dma_set_mask() succeeds when passed a 64-bit mask, the driver
can pass high addresses into dma_map_*() and put the result into
the 64-bit DMA registers of the device. That is all the driver
needs to know here.

When the bus is more limited than the device, we either have
an swiotlb/iommu that will use bounce buffers to map dma_map_* work
anyway (using low DMA addresses for high memory), or we don't have
an swiotlb and the dma_set_mask() operation has to fail.

	Arnd

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Alexandre Courbot]

On 02/26/2016 08:31 PM, Arnd Bergmann wrote:
> On Friday 26 February 2016 16:24:34 Alexandre Courbot wrote:
>> On Thu, Feb 25, 2016 at 11:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> Actually even if we specify a dma-ranges on the parent DT node, the
>>>> DMA range will still be limited to 32 bits because of the following
>>>> code in of_dma_configure():
>>>>
>>>>      /*
>>>>       * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>>>>       * setup the correct supported mask.
>>>>       */
>>>>      if (!dev->coherent_dma_mask)
>>>>          dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>>>
>>>>      /*
>>>>       * Set it to coherent_dma_mask by default if the architecture
>>>>       * code has not set it.
>>>>       */
>>>>      if (!dev->dma_mask)
>>>>          dev->dma_mask = &dev->coherent_dma_mask;
>>>>
>>>>      ....
>>>>      /* gets dma-ranges into dma_addr and size */
>>>>      ....
>>>>
>>>>
>>>>      *dev->dma_mask = min((*dev->dma_mask),
>>>>                   DMA_BIT_MASK(ilog2(dma_addr + size)));
>>>>
>>>> So unless the DMA mask is set on the device before of_dma_configure()
>>>> is called, the min() statement will choose the 32 bits mask that has
>>>> been previously set. So IIUC in any case, the driver will need to call
>>>> dma_set_mask()
>>>
>>> Yes, the driver definitely has to call dma_set_mask(), the property of
>>> the parent bus is used to make that fail when the bus doesn't support
>>> it.
>>
>> And that's where things seem to stop working: the driver's probe
>> function is invoked by the platform bus, *after* of_dma_configure() is
>> called. So unless I am missing something there is no way for the
>> driver to set the DMA mask in such a way that of_dma_configure() can
>> see it and do the right thing.
>>
>> In other words, most of the DMA mask logic in of_dma_configure()
>> doesn't seem to have any effect (and a 32 bits mask will be set), at
>> least on the platform bus.
>
> That is correct: of_dma_configure has to set a 32-bit DMA mask
> because that is the default that we expect to see in Linux drivers.
>
> A lot of drivers don't call dma_set_mask() at all, so this is
> the most reasonable value that typically works, unless the
> device is more limited, or you want the extra performance you
> get on devices that actually support a bigger mask.
>
>>>> Can I have your thoughts on this? Am I missing something?
>>>
>>> One point: I think the dma_set_mask() probably should be in the
>>> generic part of the sdhci driver, not the tegra specific portion.
>>
>> Ok, but then how does the generic part of the driver knows which DMA
>> mask applies to the device?
>
> If dma_set_mask() succeeds when passed a 64-bit mask, the driver
> can pass high addresses into dma_map_*() and put the result into
> the 64-bit DMA registers of the device. That is all the driver
> needs to know here.
>
> When the bus is more limited than the device, we either have
> an swiotlb/iommu that will use bounce buffers to map dma_map_* work
> anyway (using low DMA addresses for high memory), or we don't have
> an swiotlb and the dma_set_mask() operation has to fail.

Ok, I think I understand. I was expecting of_dma_configure() to do more 
than it actually needs to and be the final arbiter of a device's DMA 
mask. That's obviously not the case - thanks for the thorough explanation.

Let me send a v2 of this to see if I got it properly.

Re: [PATCH] mmc: sdhci-tegra: Set DMA mask

[Stephen Warren]

On 02/24/2016 02:11 AM, 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 am pretty sure this one is wrong in some way, but just to get the ball
> rolling as the use of bounce buffers is currently quite heavy on Jetson TX1.
>
> Thierry, Stephen, could you confirm that I got the DMA masks correctly? I
> am not sure about the actual addressable size on TX1, and also suspect TK1 can
> also address more than 32 bits.

I don't actually know what the HW capabilities are. You had best track 
down one of NVIDIA's HW designers/integrators; hopefully the can provide 
all kinds of gory details.

One thing I will say: in U-Boot, we deliberately clip usable RAM size to 
2GiB so that the PA of all RAM fits into 32 bits, specifically because 
of IO controllers that can only address 32 bits. It is possible this was 
only required on earlier chips and T210 fixed it though; I don't know. 
Equally, I suppose the restriction might only apply to a subset of IO 
controllers (USB would be another one relevant to U-Boot).