Patchwork [2/2] Added leon3_dma_ops and LEON specific mmu_inval_dma_area

login
register
mail settings
Submitter Kristoffer Glembo
Date Nov. 16, 2009, 9:51 a.m.
Message ID <1258365067-8983-2-git-send-email-kristoffer@gaisler.com>
Download mbox | patch
Permalink /patch/38491/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Kristoffer Glembo - Nov. 16, 2009, 9:51 a.m.
---
 arch/sparc/kernel/ioport.c |   75 +++++++++++++++++++++++++++++++++++---------
 1 files changed, 60 insertions(+), 15 deletions(-)
David Miller - Nov. 16, 2009, 10:55 a.m.
From: Kristoffer Glembo <kristoffer@gaisler.com>
Date: Mon, 16 Nov 2009 10:51:07 +0100

> @@ -524,7 +536,16 @@ static dma_addr_t pci32_map_page(struct device *dev, struct page *page,
>  				 struct dma_attrs *attrs)
>  {
>  	/* IIep is write-through, not flushing. */
> -	return page_to_phys(page) + offset;
> +	return virt_to_phys(page_address(page)) + offset;
> +}

What's wrong with page_to_phys()?

page_to_phys() is what must be used, because it is legal to DMA to or
from a highmem page, and for those page_address(page) does not
necessaily evaluate fully.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kristoffer Glembo - Nov. 16, 2009, 11:06 a.m.
David Miller wrote:
> From: Kristoffer Glembo <kristoffer@gaisler.com>
> Date: Mon, 16 Nov 2009 10:51:07 +0100
> 
>> @@ -524,7 +536,16 @@ static dma_addr_t pci32_map_page(struct device *dev, struct page *page,
>>  				 struct dma_attrs *attrs)
>>  {
>>  	/* IIep is write-through, not flushing. */
>> -	return page_to_phys(page) + offset;
>> +	return virt_to_phys(page_address(page)) + offset;
>> +}
> 
> What's wrong with page_to_phys()?
> 
> page_to_phys() is what must be used, because it is legal to DMA to or
> from a highmem page, and for those page_address(page) does not
> necessaily evaluate fully.
> 
> 

After pulling the latest sparc-next dma_map_single began returning
bogus addresses. I just saw that page_address was being used everywhere
else in ioport.c (including sbus_map_page) and switched to that since
page_to_phys was returning incorrect addresses when passed SKB pages.

So if page_address is wrong I will have to investigate what it is that
really is broken.

I will clean up the code according to your other feedback as well.

Thanks,
Kristoffer


--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 16, 2009, 11:21 a.m.
From: Kristoffer Glembo <kristoffer@gaisler.com>
Date: Mon, 16 Nov 2009 12:06:55 +0100

> After pulling the latest sparc-next dma_map_single began returning
> bogus addresses. I just saw that page_address was being used
> everywhere
> else in ioport.c (including sbus_map_page) and switched to that since
> page_to_phys was returning incorrect addresses when passed SKB pages.
> 
> So if page_address is wrong I will have to investigate what it is that
> really is broken.

You really need to investigate why dma_map_single() is not
returning sane addresses.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kristoffer Glembo - Nov. 18, 2009, 8:45 a.m.
David Miller wrote:

> 
> You really need to investigate why dma_map_single() is not
> returning sane addresses.

I haven't reached a conclusion yet since I'm not so familiar with
the Linux VM subsystem and have not had much time to look at it.

But I noticed that pci_map_single does not work in later kernels.
I guess it stopped working with the patch "sparc: Use
asm-generic/pci-dma-compat" since before the that the physical address
was obtained through virt_to_phys() and after that it goes through
page_to_phys(virt_to_page()) which does not work. Using
page_address(virt_to_page()) works but I understand that this does not
always evaluate properly.

Using page_to_phys(virt_to_page()) results in an address lacking the
phys_base offset. My patch to page_to_phys (adding phys_base) several
weeks ago was done because of this but I thought that the problem then
was that I was using virt_to_page() on an address obtained through
kmalloc which I have read is not really allowed?
The page_to_phys(virt_to_page()) does not even work for memory allocated
with __get_free_page() currently.

I will continue investigating this when I have the time. I just wanted
to give you this feedback. Maybe you understand what is going on ...

Best regards,
Kristoffer






--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 18, 2009, 9:32 a.m.
From: Kristoffer Glembo <kristoffer@gaisler.com>
Date: Wed, 18 Nov 2009 09:45:41 +0100

> But I noticed that pci_map_single does not work in later kernels.
> I guess it stopped working with the patch "sparc: Use
> asm-generic/pci-dma-compat" since before the that the physical address
> was obtained through virt_to_phys() and after that it goes through
> page_to_phys(virt_to_page()) which does not work. Using
> page_address(virt_to_page()) works but I understand that this does not
> always evaluate properly.

"page_address(virt_to_page())" returns a virtual address, whereas
page_to_phys(virt_to_page()) returns a physical one.

> Using page_to_phys(virt_to_page()) results in an address lacking the
> phys_base offset.

If it's not adding in phys_base, then it's not returning a legal
physical address.  Any idea why we don't add phys_base here?  I feel
like we've discussed this exactly recently, like in the past few
months.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kristoffer Glembo - Nov. 23, 2009, 12:31 p.m.
Hi,


David Miller wrote:
> From: Kristoffer Glembo <kristoffer@gaisler.com>
> Date: Wed, 18 Nov 2009 09:45:41 +0100
> 
>> But I noticed that pci_map_single does not work in later kernels.
>> I guess it stopped working with the patch "sparc: Use
>> asm-generic/pci-dma-compat" since before the that the physical address
>> was obtained through virt_to_phys() and after that it goes through
>> page_to_phys(virt_to_page()) which does not work. Using
>> page_address(virt_to_page()) works but I understand that this does not
>> always evaluate properly.
> 
> "page_address(virt_to_page())" returns a virtual address, whereas
> page_to_phys(virt_to_page()) returns a physical one.
> 

Yeah, I meant virt_to_phys(page_address(virt_to_page())) ..


>> Using page_to_phys(virt_to_page()) results in an address lacking the
>> phys_base offset.
> 
> If it's not adding in phys_base, then it's not returning a legal
> physical address.  Any idea why we don't add phys_base here?  I feel
> like we've discussed this exactly recently, like in the past few
> months.


We did yes. You said that mem_map is not offset at phys_base and then of
course we don't need to add it in page_to_phys. Is mem_map not offset
because typically sp_banks[].base_addr = 0? In our case we pass in one
sp_banks entry which has base_addr = 0x40000000.


Best regards,
Kristoffer



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 14, 2009, 4:36 a.m.
From: Kristoffer Glembo <kristoffer@gaisler.com>
Date: Mon, 23 Nov 2009 13:31:32 +0100

> David Miller wrote:
>> If it's not adding in phys_base, then it's not returning a legal
>> physical address.  Any idea why we don't add phys_base here?  I feel
>> like we've discussed this exactly recently, like in the past few
>> months.
>
> We did yes. You said that mem_map is not offset at phys_base and
> then of course we don't need to add it in page_to_phys. Is mem_map
> not offset because typically sp_banks[].base_addr = 0? In our case
> we pass in one sp_banks entry which has base_addr = 0x40000000.

I'm still trying to find time to address this, just FYI.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kristoffer Glembo - Dec. 14, 2009, 7:57 a.m.
David Miller wrote:

>> We did yes. You said that mem_map is not offset at phys_base and
>> then of course we don't need to add it in page_to_phys. Is mem_map
>> not offset because typically sp_banks[].base_addr = 0? In our case
>> we pass in one sp_banks entry which has base_addr = 0x40000000.
> 
> I'm still trying to find time to address this, just FYI.
> 
> 

Thanks. I have been working on non Linux stuff for a while
but in the meantime I have actually used the patch where
phys_base is added in page_to_phys.

I hope to continue Linux development after Christmas.

Happy holidays :)

/Kristoffer
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 3c8c44f..312b7b4 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -50,10 +50,15 @@ 
 #include <asm/io-unit.h>
 #include <asm/leon.h>
 
-#ifdef CONFIG_SPARC_LEON
-#define mmu_inval_dma_area(p, l) leon_flush_dcache_all()
-#else
+#ifndef CONFIG_SPARC_LEON
 #define mmu_inval_dma_area(p, l)	/* Anton pulled it out for 2.4.0-xx */
+#else
+static inline void mmu_inval_dma_area(unsigned long va, unsigned long len)
+{
+	if (!sparc_leon3_snooping_enabled()) {
+		leon_flush_dcache_all();
+	}
+}
 #endif
 
 static struct resource *_sparc_find_resource(struct resource *r,
@@ -281,21 +286,26 @@  static void *sbus_alloc_coherent(struct device *dev, size_t len,
 		goto err_nova;
 	}
 	mmu_inval_dma_area(va, len_total);
+
 	// XXX The mmu_map_dma_area does this for us below, see comments.
 	// sparc_mapiorange(0, virt_to_phys(va), res->start, len_total);
 	/*
 	 * XXX That's where sdev would be used. Currently we load
 	 * all iommu tables with the same translations.
 	 */
-	if (mmu_map_dma_area(dev, dma_addrp, va, res->start, len_total) != 0)
-		goto err_noiommu;
-
+#ifdef CONFIG_SPARC_LEON
+	sparc_mapiorange(0, virt_to_phys(va), res->start, len_total);	
+	*dma_addrp = virt_to_phys(va);
+#else
+	if (mmu_map_dma_area(dev, dma_addrp, va, res->start, len_total) != 0) {
+		release_resource(res);
+		goto err_nova;
+	}
+#endif
 	res->name = op->node->name;
 
 	return (void *)(unsigned long)res->start;
 
-err_noiommu:
-	release_resource(res);
 err_nova:
 	free_pages(va, order);
 err_nomem:
@@ -333,7 +343,12 @@  static void sbus_free_coherent(struct device *dev, size_t n, void *p,
 
 	/* mmu_inval_dma_area(va, n); */ /* it's consistent, isn't it */
 	pgv = virt_to_page(p);
+
+#ifdef CONFIG_SPARC_LEON
+	sparc_unmapiorange((unsigned long)p, n);
+#else
 	mmu_unmap_dma_area(dev, ba, n);
+#endif
 
 	__free_pages(pgv, get_order(n));
 }
@@ -408,9 +423,6 @@  struct dma_map_ops sbus_dma_ops = {
 	.sync_sg_for_device	= sbus_sync_sg_for_device,
 };
 
-struct dma_map_ops *dma_ops = &sbus_dma_ops;
-EXPORT_SYMBOL(dma_ops);
-
 static int __init sparc_register_ioport(void)
 {
 	register_proc_sparc_ioport();
@@ -422,7 +434,7 @@  arch_initcall(sparc_register_ioport);
 
 #endif /* CONFIG_SBUS */
 
-#ifdef CONFIG_PCI
+#if defined(CONFIG_PCI) || defined(CONFIG_SPARC_LEON)
 
 /* Allocate and map kernel buffer using consistent mode DMA for a device.
  * hwdev should be valid struct pci_dev pointer for PCI devices.
@@ -524,7 +536,16 @@  static dma_addr_t pci32_map_page(struct device *dev, struct page *page,
 				 struct dma_attrs *attrs)
 {
 	/* IIep is write-through, not flushing. */
-	return page_to_phys(page) + offset;
+	return virt_to_phys(page_address(page)) + offset;
+}
+
+static void pci32_unmap_page(struct device *dev, dma_addr_t ba, size_t size,
+			     enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	if (dir != PCI_DMA_TODEVICE) {
+		mmu_inval_dma_area((unsigned long)phys_to_virt(ba),
+		    (size + PAGE_SIZE-1) & PAGE_MASK);
+	}
 }
 
 /* Map a set of buffers described by scatterlist in streaming
@@ -649,6 +670,7 @@  struct dma_map_ops pci32_dma_ops = {
 	.alloc_coherent		= pci32_alloc_coherent,
 	.free_coherent		= pci32_free_coherent,
 	.map_page		= pci32_map_page,
+	.unmap_page		= pci32_unmap_page,
 	.map_sg			= pci32_map_sg,
 	.unmap_sg		= pci32_unmap_sg,
 	.sync_single_for_cpu	= pci32_sync_single_for_cpu,
@@ -658,7 +680,30 @@  struct dma_map_ops pci32_dma_ops = {
 };
 EXPORT_SYMBOL(pci32_dma_ops);
 
-#endif /* CONFIG_PCI */
+struct dma_map_ops leon3_dma_ops = {
+	.alloc_coherent		= sbus_alloc_coherent,
+	.free_coherent		= sbus_free_coherent,
+	.map_page		= pci32_map_page,
+	.unmap_page		= pci32_unmap_page,
+	.map_sg			= pci32_map_sg,
+	.unmap_sg		= pci32_unmap_sg,
+	.sync_single_for_cpu	= pci32_sync_single_for_cpu,
+	.sync_single_for_device	= pci32_sync_single_for_device,
+	.sync_sg_for_cpu	= pci32_sync_sg_for_cpu,
+	.sync_sg_for_device	= pci32_sync_sg_for_device,
+};
+
+#endif /* CONFIG_PCI || CONFIG_SPARC_LEON */
+
+#ifdef CONFIG_SPARC_LEON
+struct dma_map_ops *dma_ops = &leon3_dma_ops;
+#else
+struct dma_map_ops *dma_ops = &sbus_dma_ops;
+#endif
+
+#ifdef CONFIG_SBUS
+EXPORT_SYMBOL(dma_ops);
+#endif
 
 /*
  * Return whether the given PCI device DMA address mask can be
@@ -728,7 +773,7 @@  static const struct file_operations sparc_io_proc_fops = {
 static struct resource *_sparc_find_resource(struct resource *root,
 					     unsigned long hit)
 {
-        struct resource *tmp;
+	struct resource *tmp;
 
 	for (tmp = root->child; tmp != 0; tmp = tmp->sibling) {
 		if (tmp->start <= hit && tmp->end >= hit)