lkml.org 
[lkml]   [2016]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] mmc: sdhci-tegra: Set DMA mask
Date
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

\
 
 \ /
  Last update: 2016-02-25 16:21    [W:0.060 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site