On Wed, Jan 05, 2011 at 05:09:45PM +0100, Daniel Hellstrom wrote:
> From: Kristoffer Glembo <kristoffer@xxxxxxxxxxx>
>
> Added leon3_dma_ops and mmu_inval_dma_area.
Hi Kristoffer.
This patch mixes cleanup with added functionality.
I would have preferred that you divided this into
several smaller patches.
I am not familiar with the area that your patch touches - but
I have tried to give some feedback below.
Sam
> ---
> arch/sparc/include/asm/dma-mapping.h | 13 +---
> arch/sparc/kernel/ioport.c | 139 +++++++++++++++++++++++++---------
> 2 files changed, 103 insertions(+), 49 deletions(-)
>
> diff --git a/arch/sparc/include/asm/dma-mapping.h b/arch/sparc/include/asm/dma-mapping.h
> index 8c0e4f7..fadc653 100644
> --- a/arch/sparc/include/asm/dma-mapping.h
> +++ b/arch/sparc/include/asm/dma-mapping.h
> @@ -51,17 +51,6 @@ static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
> return (dma_addr == DMA_ERROR_CODE);
> }
>
> -static inline int dma_set_mask(struct device *dev, u64 mask)
> -{
> -#ifdef CONFIG_PCI
> - if (dev->bus == &pci_bus_type) {
> - if (!dev->dma_mask || !dma_supported(dev, mask))
> - return -EINVAL;
> - *dev->dma_mask = mask;
> - return 0;
> - }
> -#endif
> - return -EINVAL;
> -}
> +extern int dma_set_mask(struct device *dev, u64 dma_mask);
So dma_set_mask() is moved to ioport.c
>
> +int dma_set_mask(struct device *dev, u64 dma_mask)
> +{
> +#ifdef CONFIG_PCI
> + if (dev->bus == &pci_bus_type)
> + return pci_set_dma_mask(to_pci_dev(dev), dma_mask);
> +#endif
> + return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(dma_set_mask);
And it is now an exported symbol - looks OK.
But the new implementation differs from the old implementation.
Grepping for pci_set_dma_mask() turned up this implmentation:
static inline int pci_set_dma_mask(struct pci_dev *dev, u64 mask)
{
return dma_set_mask(&dev->dev, mask);
}
It looks like an infinite loop??
Note: This change alone warrant an independent patch.
> #endif
> diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
> index 41f7e4e..7359369 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
This is better than before as we no longer has a double definition.
But why do we call mmu_inval_dma_area() wich is empty in SPARC32 build
from SBUS code? LEON does not support SBUS only AMBA + PCI IIRC.
Hmm - looking at Kconfig LEON supports SBUS too. Is that correct?
>
> static struct resource *_sparc_find_resource(struct resource *r,
> @@ -254,7 +259,7 @@ static void *sbus_alloc_coherent(struct device *dev, size_t len,
> dma_addr_t *dma_addrp, gfp_t gfp)
> {
> struct platform_device *op = to_platform_device(dev);
> - unsigned long len_total = (len + PAGE_SIZE-1) & PAGE_MASK;
> + unsigned long len_total = PAGE_ALIGN(len);
PAGE_ALIGN is much better than the old version.
But again - please do such cleanup in a separate step.
If we are going to revert your functional change we do not
want to lose such cleanups.
> unsigned long va;
> struct resource *res;
> int order;
> @@ -287,15 +292,19 @@ static void *sbus_alloc_coherent(struct device *dev, size_t len,
> * 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
I cannot follow why you need this ugly ifdef here.
Some comments please.
> res->name = op->dev.of_node->name;
>
> return (void *)(unsigned long)res->start;
>
> -err_noiommu:
> - release_resource(res);
> err_nova:
> free_pages(va, order);
> err_nomem:
> @@ -321,7 +330,7 @@ static void sbus_free_coherent(struct device *dev, size_t n, void *p,
> return;
> }
>
> - n = (n + PAGE_SIZE-1) & PAGE_MASK;
> + n = PAGE_ALIGN(n);
Another patch..
> if ((res->end-res->start)+1 != n) {
> printk("sbus_free_consistent: region 0x%lx asked 0x%zx\n",
> (long)((res->end-res->start)+1), n);
> @@ -333,7 +342,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
Why? comemnts needed again.
Or better find another way to do this.
Looking a bit around...
Is it correct understood that BTFIXUPSET_CALL() is used
to patch in another function here for for example sun4c?
So mmu_unmap_dma_area() becomes a call to sun4c_unmap_dma_area()?
>
> __free_pages(pgv, get_order(n));
> }
> @@ -408,9 +422,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 +433,7 @@ arch_initcall(sparc_register_ioport);
>
> #endif /* CONFIG_SBUS */
>
> -#ifdef CONFIG_PCI
> +#if defined(CONFIG_PCI) || defined(CONFIG_SPARC_LEON)
Why do we need PCI functions when PCI is not defined?
>
> /* Allocate and map kernel buffer using consistent mode DMA for a device.
> * hwdev should be valid struct pci_dev pointer for PCI devices.
> @@ -430,7 +441,7 @@ arch_initcall(sparc_register_ioport);
> static void *pci32_alloc_coherent(struct device *dev, size_t len,
> dma_addr_t *pba, gfp_t gfp)
> {
> - unsigned long len_total = (len + PAGE_SIZE-1) & PAGE_MASK;
> + unsigned long len_total = PAGE_ALIGN(len);
> unsigned long va;
> struct resource *res;
> int order;
> @@ -463,10 +474,6 @@ static void *pci32_alloc_coherent(struct device *dev, size_t len,
> return NULL;
> }
> mmu_inval_dma_area(va, len_total);
> -#if 0
> -/* P3 */ printk("pci_alloc_consistent: kva %lx uncva %lx phys %lx size %lx\n",
> - (long)va, (long)res->start, (long)virt_to_phys(va), len_total);
> -#endif
unrealted cleanup...
> sparc_mapiorange(0, virt_to_phys(va), res->start, len_total);
>
> *pba = virt_to_phys(va); /* equals virt_to_bus (R.I.P.) for us. */
> @@ -498,7 +505,7 @@ static void pci32_free_coherent(struct device *dev, size_t n, void *p,
> return;
> }
>
> - n = (n + PAGE_SIZE-1) & PAGE_MASK;
> + n = PAGE_ALIGN(n);
Another patch.
> if ((res->end-res->start)+1 != n) {
> printk("pci_free_consistent: region 0x%lx asked 0x%lx\n",
> (long)((res->end-res->start)+1), (long)n);
> @@ -515,6 +522,15 @@ static void pci32_free_coherent(struct device *dev, size_t n, void *p,
> free_pages(pgp, get_order(n));
> }
>
> +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),
> + PAGE_ALIGN(size));
> +}
Looks like ordinary SPARC32 gained an unmap_page - correct?
Looks like a separate chage to me.
> +
> /*
> * Same as pci_map_single, but with pages.
> */
> @@ -551,8 +567,7 @@ static int pci32_map_sg(struct device *device, struct scatterlist *sgl,
>
> /* IIep is write-through, not flushing. */
> for_each_sg(sgl, sg, nents, n) {
> - BUG_ON(page_address(sg_page(sg)) == NULL);
Why is this BUG_ON() removed?
> - sg->dma_address = virt_to_phys(sg_virt(sg));
> + sg->dma_address = sg_phys(sg);
Why - is this better or what?
> sg->dma_length = sg->length;
> }
> return nents;
> @@ -571,10 +586,8 @@ static void pci32_unmap_sg(struct device *dev, struct scatterlist *sgl,
>
> if (dir != PCI_DMA_TODEVICE) {
> for_each_sg(sgl, sg, nents, n) {
> - BUG_ON(page_address(sg_page(sg)) == NULL);
> - mmu_inval_dma_area(
> - (unsigned long) page_address(sg_page(sg)),
> - (sg->length + PAGE_SIZE-1) & PAGE_MASK);
> + mmu_inval_dma_area((unsigned long)sg_virt(sg),
> + PAGE_ALIGN(sg->length));
> }
Partly same type of comments like above.
> }
> }
> @@ -594,7 +607,7 @@ static void pci32_sync_single_for_cpu(struct device *dev, dma_addr_t ba,
> {
> if (dir != PCI_DMA_TODEVICE) {
> mmu_inval_dma_area((unsigned long)phys_to_virt(ba),
> - (size + PAGE_SIZE-1) & PAGE_MASK);
> + PAGE_ALIGN(size));
Another patch...
Same goes for additional spots in this file - I did not comment these.
> +/* LEON3 unmapping functions
> + *
> + * We can only invalidate the whole cache so unmap_page and unmap_sg do the
> + * same thing
> + */
> +static void leon3_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(0, 0);
> +}
> +
> +static void leon3_unmap_sg(struct device *dev, struct scatterlist *sgl,
> + int nents, enum dma_data_direction dir,
> + struct dma_attrs *attrs)
> +{
> + if (dir != PCI_DMA_TODEVICE)
> + mmu_inval_dma_area(0, 0);
> +}
> +
> 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 +689,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 = leon3_unmap_page,
> + .map_sg = pci32_map_sg,
> + .unmap_sg = leon3_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
This just looks wrong to me...
Why we need to export only is SBUS is defined - but
we define leon3_dma_ops if LEON is defined.
I think this need to be revisited - and then see if you
can hide more of the ifdef in dma-mapping.h
>
> /*
> * Return whether the given PCI device DMA address mask can be
> @@ -676,6 +730,17 @@ int dma_supported(struct device *dev, u64 mask)
> }
> EXPORT_SYMBOL(dma_supported);
>
> +int dma_set_mask(struct device *dev, u64 dma_mask)
> +{
> +#ifdef CONFIG_PCI
> + if (dev->bus == &pci_bus_type)
> + return pci_set_dma_mask(to_pci_dev(dev), dma_mask);
> +#endif
> + return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(dma_set_mask);
> +
> +
> #ifdef CONFIG_PROC_FS
>
> static int sparc_io_proc_show(struct seq_file *m, void *v)
> @@ -717,7 +782,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;
Minor nit but unrelated change.
Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
