Re: [PATCH 2/5] [POWERPC] 8xx: generic 8xx code arch/powerpc port

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 14, 2006 at 04:28:16AM +0300, Vitaly Bordug wrote:
> --- /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.

> +#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.

> +/*
> + * 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..

> --- 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

> +/* 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?

> +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.