| Message ID | 1258365067-8983-2-git-send-email-kristoffer@gaisler.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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
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)