mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Jules Maselbas <jmaselbas@kalray.eu>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Yann Sionneau <ysionneau@kalray.eu>, barebox@lists.infradead.org
Subject: Re: [PATCH v2 3/5] kvx: Implement dma handling primitives
Date: Mon, 17 May 2021 12:56:59 +0200	[thread overview]
Message-ID: <20210517105659.GA23867@tellis.lin.mbt.kalray.eu> (raw)
In-Reply-To: <eca36ac2-d9fb-6b34-bf64-455243398f88@pengutronix.de>

Hi Ahmad,

On Mon, May 17, 2021 at 12:42:21PM +0200, Ahmad Fatoum wrote:
> Hello Jules, Yann,
> 
> On 05.03.21 19:33, Jules Maselbas wrote:
> > From: Yann Sionneau <ysionneau@kalray.eu>
> > 
> > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> 
> [snip]
> 
> > diff --git a/arch/kvx/lib/dma-default.c b/arch/kvx/lib/dma-default.c
> > new file mode 100644
> > index 000000000..2a4144696
> > --- /dev/null
> > +++ b/arch/kvx/lib/dma-default.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// SPDX-FileCopyrightText: 2021 Yann Sionneau <ysionneau@kalray.eu>, Kalray Inc.
> > +
> > +#include <dma.h>
> > +#include <asm/barrier.h>
> > +#include <asm/io.h>
> > +#include <asm/cache.h>
> > +#include <asm/sfr.h>
> > +#include <asm/sys_arch.h>
> > +
> > +/*
> > + * The implementation of arch should follow the following rules:
> > + *		map		for_cpu		for_device	unmap
> > + * TO_DEV	writeback	none		writeback	none
> > + * FROM_DEV	invalidate	invalidate(*)	invalidate	invalidate(*)
> > + * BIDIR	writeback	invalidate	writeback	invalidate
> > + *
> > + * (*) - only necessary if the CPU speculatively prefetches.
> > + *
> > + * (see https://lkml.org/lkml/2018/5/18/979)
> > + */
> > +
> > +void dma_sync_single_for_device(dma_addr_t addr, size_t size,
> > +				enum dma_data_direction dir)
> > +{
> > +	/* dcache is Write-Through: no need to flush to force writeback */
> > +	switch (dir) {
> > +	case DMA_FROM_DEVICE:
> > +		invalidate_dcache_range(addr, addr + size);
> > +		break;
> > +	case DMA_TO_DEVICE:
> > +	case DMA_BIDIRECTIONAL:
> > +		/* allow device to read buffer written by CPU */
> > +		wmb();
> > +		break;
> > +	default:
> > +		BUG();
> > +	}
> > +}
> > +
> > +void dma_sync_single_for_cpu(dma_addr_t addr, size_t size,
> > +				enum dma_data_direction dir)
> > +{
> > +	/* CPU does not speculatively prefetches */
> > +	switch (dir) {
> > +	case DMA_FROM_DEVICE:
> > +		/* invalidate has been done during map/for_device */
> > +	case DMA_TO_DEVICE:
> > +		break;
> > +	case DMA_BIDIRECTIONAL:
> > +		invalidate_dcache_range(addr, addr + size);
> > +		break;
> > +	default:
> > +		BUG();
> > +	}
> > +}
> 
> Both of these work on CPU pointers.
> 
> > +
> > +#define KVX_DDR_ALIAS_OFFSET \
> > +	(KVX_DDR_64BIT_RAM_WINDOW_BA - KVX_DDR_32BIT_RAM_WINDOW_BA)
> > +#define KVX_DDR_ALIAS_WINDOW \
> > +	(KVX_DDR_64BIT_RAM_WINDOW_BA + KVX_DDR_ALIAS_OFFSET)
> > +
> > +/* Local smem is aliased between 0 and 16MB */
> > +#define KVX_SMEM_LOCAL_ALIAS 0x1000000ULL
> > +
> > +dma_addr_t dma_map_single(struct device_d *dev, void *ptr, size_t size,
> > +			  enum dma_data_direction dir)
> > +{
> > +	uintptr_t addr = (uintptr_t) ptr;
> > +
> > +	dma_sync_single_for_device(addr, size, dir);
> 
> So this is correct.
> 
> > +
> > +	/* Local smem alias should never be used for dma */
> > +	if (addr < KVX_SMEM_LOCAL_ALIAS)
> > +		return addr + (1 + kvx_cluster_id()) * KVX_SMEM_LOCAL_ALIAS;
> > +
> > +	if (dev->dma_mask && addr <= dev->dma_mask)
> > +		return addr;
> > +
> > +	if (addr >= KVX_DDR_ALIAS_WINDOW)
> > +		return DMA_ERROR_CODE;
> > +
> > +	addr -= KVX_DDR_ALIAS_OFFSET;
> > +	if (dev->dma_mask && addr > dev->dma_mask)
> > +		return DMA_ERROR_CODE;
> > +
> > +	return addr;
> > +}
> 
> Now you compute a dma_addr_t as CPU pointer - KVX_DDR_ALIAS_OFFSET.
> 
> > +
> > +void dma_unmap_single(struct device_d *dev, dma_addr_t addr, size_t size,
> > +		      enum dma_data_direction dir)
> > +{
> > +	dma_sync_single_for_cpu(addr, size, dir);
> 
> And then that dma_addr_t is passed here without + KVX_DDR_ALIAS_OFFSET
> to get a CPU pointer out.
> So for DMA_BIDIRECTIONAL you'd invalidate the wrong cache region.
Yes, this is bogus but I believe in our case dma_unmap_single is never
called with DMA_BIDIRECTIONAL.

> 
> I stumbled upon this, because I noticed that that kvx whould've failed to
> build starting with v2021.04.0, because following commits conflict with each other:
> 3f975f810bd3 ("dma: move dma_map/unmap_single from ARM to common code") commited on 2021-03-03
> 4808d6f80073 ("kvx: Implement dma handling primitives") commited on 2021-03-05
> 
> Now, dma_map_single() is defined twice for kvx.
Yes,  I noticed this as well when rebasing on barebox v2021.04.

> As dma_(un?)map_single is always called with a dev argument, couldn't you define the
> DMA offsets in the device tree and use the common drivers/dma/map.c implementation
> for these two functions?
Our custom implementation will be removed in favor of the common
implementation and the uses of dma-range.

I had to add a dma-range to the dwc2 device nodes in our device tree, so
when the driver does dma_map/unmap the common implementation will do the
correct remap to the aliased region (so addresses fits in 32bit).

However I do not know how Linux handle a mix of dma-range and iommu.
Since I didn't had much time to test this we are still using v2021.03. 


Thanks,
Jules


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


  reply	other threads:[~2021-05-17 11:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-05 18:33 [PATCH v2 1/5] kvx: Implement setjmp/longjmp/initjmp Jules Maselbas
2021-03-05 18:33 ` [PATCH v2 2/5] kvx: Implement dcache invalidation primitive Jules Maselbas
2021-03-05 18:33 ` [PATCH v2 3/5] kvx: Implement dma handling primitives Jules Maselbas
2021-05-17 10:42   ` Ahmad Fatoum
2021-05-17 10:56     ` Jules Maselbas [this message]
2021-03-05 18:33 ` [PATCH v2 4/5] kvx: Request enough privilege to boot Linux Jules Maselbas
2021-03-05 18:33 ` [PATCH v2 5/5] kvx: lib: dtb: Remove unused variable Jules Maselbas
2021-03-15  8:35 ` [PATCH v2 1/5] kvx: Implement setjmp/longjmp/initjmp Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210517105659.GA23867@tellis.lin.mbt.kalray.eu \
    --to=jmaselbas@kalray.eu \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=ysionneau@kalray.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox