Date: Mon, 29 Jun 2009 15:16:56 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: Piotr =?iso-8859-2?q?Zi=EAcik?= <kosmo@semihalf.com>, freebsd-arm@freebsd.org Cc: Rafal Jaworowski <raj@semihalf.com>, thompsa@freebsd.org, freebsd-usb@freebsd.org Subject: Re: CPU Cache and busdma usage in USB Message-ID: <200906291516.58725.hselasky@c2i.net> In-Reply-To: <200906231035.43096.kosmo@semihalf.com> References: <200906231035.43096.kosmo@semihalf.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 23 June 2009 10:35:42 Piotr Ziêcik wrote:
> --- a/sys/dev/usb/usb_busdma.c
> +++ b/sys/dev/usb/usb_busdma.c
> @@ -658,8 +658,7 @@ usb_pc_cpu_invalidate(struct usb_page_cache *pc)
> /* nothing has been loaded into this page cache! */
> return;
> }
> - bus_dmamap_sync(pc->tag, pc->map,
> - BUS_DMASYNC_POSTWRITE | BUS_DMASYNC_POSTREAD);
> + bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
> }
>
> /*------------------------------------------------------------------------
>* @@ -672,8 +671,7 @@ usb_pc_cpu_flush(struct usb_page_cache *pc)
> /* nothing has been loaded into this page cache! */
> return;
> }
> - bus_dmamap_sync(pc->tag, pc->map,
> - BUS_DMASYNC_PREWRITE | BUS_DMASYNC_PREREAD);
> + bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
> }
Hi,
Let's restart the discussion with the initial patch.
You want to change the flags passed to bus_dmamap_sync() so that the
flush/invalidate mapping gets right. I understand why your patch makes it
work. That's not the problem.
In "src/sys/arm/arm/busdma_machdep.c" there is a function called
"_bus_dmamap_sync_bp()". If you look at that function you see that it only
triggers on the "BUS_DMASYNC_PREWRITE" and "BUS_DMASYNC_POSTREAD" flags. After
your patching only the PREXXXX flags are used, so if bouce pages are used on
ARM and x86 and amd64 +++, then only BUS_DMASYNC_PREWRITE will do anything.
This indicates that your patch is not fully correct.
Grepping through the source code for ARM, I found a line like this:
/*XXX*/ arm9_dcache_wbinv_range, /* dcache_inv_range */
which is not correct. If we are only invalidating, then it is not correct to
do a flush first.
Summed up:
static void
bus_dmamap_sync_buf(void *buf, int len, bus_dmasync_op_t op)
{
char _tmp_cl[arm_dcache_align], _tmp_clend[arm_dcache_align];
if ((op & BUS_DMASYNC_PREWRITE) && !(op & BUS_DMASYNC_PREREAD)) {
cpu_dcache_wb_range((vm_offset_t)buf, len);
cpu_l2cache_wb_range((vm_offset_t)buf, len);
}
if (op & BUS_DMASYNC_PREREAD) {
if (!(op & BUS_DMASYNC_PREWRITE) &&
((((vm_offset_t)(buf) | len) & arm_dcache_align_mask) ==
0)) {
cpu_dcache_inv_range((vm_offset_t)buf, len);
cpu_l2cache_inv_range((vm_offset_t)buf, len);
} else {
Because the USB code specifies both PREREAD and PREWRITE we end up in the
following case, which is not implemented correctly. The function name
indicates write back first, then invalidate, but when looking at the
implementation:
<example>
arm8_cache_purgeID, /* idcache_wbinv_all */
(void *)arm8_cache_purgeID, /* idcache_wbinv_range */
</example>
You see that it only performs purge and no prior flush. This is what needs
fixing! If semihalf could go through the "arm/arm/cpufunc.c" file and fix
those flush and invalidate functions then many people would become happy!
Again, it is not a problem in USB firstly, it is a problem in "arm/xxx".
cpu_dcache_wbinv_range((vm_offset_t)buf, len);
cpu_l2cache_wbinv_range((vm_offset_t)buf, len);
}
}
if (op & BUS_DMASYNC_POSTREAD) {
if ((vm_offset_t)buf & arm_dcache_align_mask) {
memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~
arm_dcache_align_mask),
(vm_offset_t)buf & arm_dcache_align_mask);
}
if (((vm_offset_t)buf + len) & arm_dcache_align_mask) {
memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len),
arm_dcache_align - (((vm_offset_t)(buf) + len) &
arm_dcache_align_mask));
}
cpu_dcache_inv_range((vm_offset_t)buf, len);
cpu_l2cache_inv_range((vm_offset_t)buf, len);
if ((vm_offset_t)buf & arm_dcache_align_mask)
memcpy((void *)((vm_offset_t)buf &
~arm_dcache_align_mask), _tmp_cl,
(vm_offset_t)buf & arm_dcache_align_mask);
if (((vm_offset_t)buf + len) & arm_dcache_align_mask)
memcpy((void *)((vm_offset_t)buf + len), _tmp_clend,
arm_dcache_align - (((vm_offset_t)(buf) + len) &
arm_dcache_align_mask));
}
}
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906291516.58725.hselasky>