Re: [PATCH 2/5] [POWERPC] 8xx: generic 8xx code arch/powerpc port
flat view
From: Christoph Hellwig <hch@lst.de>
Date: 2006-11-15 18:23:26
On Tue, Nov 14, 2006 at 04:28:16AM +0300, Vitaly Bordug wrote:
quoted hunk ↗ jump to hunk
--- /dev/null +++ b/arch/powerpc/kernel/dma-mapping.c
Can we please give this a more descriptive name? MIPS names the equivalent dma-noncoherent.c which makes a lot of sense to me.
quoted hunk ↗ jump to hunk
+#include <linux/module.h> +#include <linux/signal.h> +#include <linux/sched.h> +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/string.h> +#include <linux/types.h> +#include <linux/ptrace.h> +#include <linux/mman.h> +#include <linux/mm.h> +#include <linux/swap.h> +#include <linux/stddef.h> +#include <linux/vmalloc.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/bootmem.h> +#include <linux/highmem.h> +#include <linux/dma-mapping.h> +#include <linux/hardirq.h> + +#include <asm/pgalloc.h> +#include <asm/prom.h> +#include <asm/io.h> +#include <asm/mmu_context.h> +#include <asm/pgtable.h> +#include <asm/mmu.h> +#include <asm/uaccess.h> +#include <asm/smp.h> +#include <asm/machdep.h>
Do you really need all these headers?
+int map_page(unsigned long va, phys_addr_t pa, int flags);
this one doesn't seem to be used ever. And even if it was a non-static forward-declaration in a .c is not something we'd want.
quoted hunk ↗ jump to hunk
+/* + * VM region handling support. + * + * This should become something generic, handling VM region allocations for + * vmalloc and similar (ioremap, module space, etc). + * + * I envisage vmalloc()'s supporting vm_struct becoming: + * + * struct vm_struct { + * struct vm_region region; + * unsigned long flags; + * struct page **pages; + * unsigned int nr_pages; + * unsigned long phys_addr; + * }; + * + * get_vm_area() would then call vm_region_alloc with an appropriate + * struct vm_region head (eg): + * + * struct vm_region vmalloc_head = { + * .vm_list = LIST_HEAD_INIT(vmalloc_head.vm_list), + * .vm_start = VMALLOC_START, + * .vm_end = VMALLOC_END, + * }; + * + * However, vmalloc_head.vm_start is variable (typically, it is dependent on + * the amount of RAM found at boot time.) I would imagine that get_vm_area() + * would have to initialise this each time prior to calling vm_region_alloc(
Yeah, I suspect you should do that instead of adding another copy of a copy of core VM code.. Btw, is there anything ppc-specific in the noncoherent dma code? It might be a good idea to put it into lib/ otherwise, I'll contribute a dma-coherent.c for simple architectures for there aswell..
quoted hunk ↗ jump to hunk
--- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c@@ -447,3 +447,102 @@ void bad_page_fault(struct pt_regs *regs die("Kernel access of bad area", regs, sig); } + +#ifdef CONFIG_8xx +
Can we put this functions into a separate file instea of an ifdefed section
quoted hunk ↗ jump to hunk
+/* The pgtable.h claims some functions generically exist, but I + * can't find them...... + */ +pte_t *va_to_pte(unsigned long address) +{ + pgd_t *dir; + pmd_t *pmd; + pte_t *pte; + + if (address < TASK_SIZE) + return NULL; + + dir = pgd_offset(&init_mm, address); + if (dir) { + pmd = pmd_offset(dir, address & PAGE_MASK); + if (pmd && pmd_present(*pmd)) { + pte = pte_offset_kernel(pmd, address & PAGE_MASK); + if (pte && pte_present(*pte)) + return(pte); + } + } + return NULL; +}
But wha do you need this for anyway? It's not used anywhere currently, and it's not 8xx-specific at all?
quoted hunk ↗ jump to hunk
+unsigned long va_to_phys(unsigned long address) +{ + pte_t *pte; + + pte = va_to_pte(address); + if (pte) + return(((unsigned long)(pte_val(*pte)) & PAGE_MASK) | (address & ~(PAGE_MASK))); + return (0); +}
Shouldn't you use virt_to_phys instead? Also for the last two I'd prefer if you'd find a way to avoid adding them.