allow coherent DMA API to work before main page allocator is set up

Hi Dan, Matt,

The following patch changes dma_alloc_coherent() to, in case the main page allocator is not yet up and running, use the bootmem allocator instead.

It also adds a new parameter to m8xx_cpm_hostalloc() to send back the physical address to its caller.

With such modification in place it will be possible for early drivers to rely on the coherent DMA allocator. Special casing such as drivers/serial/cpm_uart/cpm_uart_cpm1.c's cpm_uart_allocbuf() can be removed:

        if (is_con) {
                mem_addr = (u8 *) m8xx_cpm_hostalloc(memsz);
                dma_addr = 0;
        } else
                mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
                                              GFP_KERNEL);

What is your opinion on this approach?

If virt_to_phys() usage is discouraged any different method to walk the pagetables and retrieve the physical address can be used.

TIA

diff -Nur linux-2.6.11.orig/arch/ppc/8xx_io/commproc.c linux-2.6.11/arch/ppc/8xx_io/commproc.c
@@ -39,7 +39,8 @@
 #include <asm/tlbflush.h>
 #include <asm/rheap.h>
 
-extern int get_pteptr(struct mm_struct *mm, unsigned long addr, pte_t **ptep);
+
+phys_addr_t	physaddr;
 
 static void m8xx_cpm_dpinit(void);
 static	uint	host_buffer;	/* One page of host buffer */

@@ -111,11 +112,10 @@
 extern void flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr);
 
 void
-m8xx_cpm_reset(uint bootpage)
+m8xx_cpm_reset(void)
 {
 	volatile immap_t	 *imp;
 	volatile cpm8xx_t	*commproc;
-	pte_t *pte;
 
 	imp = (immap_t *)IMAP_ADDR;
 	commproc = (cpm8xx_t *)&imp->im_cpm;

@@ -143,17 +143,6 @@
 	/* Reclaim the DP memory for our use. */
 	m8xx_cpm_dpinit();
 
-	/* get the PTE for the bootpage */
-	if (!get_pteptr(&init_mm, bootpage, &pte))
-	       panic("get_pteptr failed\n");
-																							
-	/* and make it uncachable */
-	pte_val(*pte) |= _PAGE_NO_CACHE;
-	_tlbie(bootpage);
-
-	host_buffer = bootpage;
-	host_end = host_buffer + PAGE_SIZE;
-
 	/* Tell everyone where the comm processor resides.
 	*/
 	cpmp = (cpm8xx_t *)commproc;

@@ -165,7 +154,6 @@
 static void
 alloc_host_memory(void)
 {
-	dma_addr_t	physaddr;
 
 	/* Set the host page for allocation.
 	*/

@@ -325,7 +313,7 @@
  * UART "fifos" and the like.
  */
 uint
-m8xx_cpm_hostalloc(uint size)
+m8xx_cpm_hostalloc(uint size, phys_addr_t *handle)
 {
 	uint	retloc;
 

@@ -336,7 +324,9 @@
 		return(0);
 
 	retloc = host_buffer;
+	*handle = physaddr;
 	host_buffer += size;
+	physaddr += size;
 
 	return(retloc);
 }

diff -Nur linux-2.6.11.orig/arch/ppc/syslib/m8xx_setup.c linux-2.6.11/arch/ppc/syslib/m8xx_setup.c
@@ -57,7 +57,7 @@
 extern void m8xx_ide_init(void);
 
 extern unsigned long find_available_memory(void);
-extern void m8xx_cpm_reset(uint cpm_page);
+extern void m8xx_cpm_reset();
 extern void m8xx_wdt_handler_install(bd_t *bp);
 extern void rpxfb_alloc_pages(void);
 extern void cpm_interrupt_init(void);

@@ -70,13 +70,9 @@
 void __init
 m8xx_setup_arch(void)
 {
-	int	cpm_page;
-
-	cpm_page = (int) alloc_bootmem_pages(PAGE_SIZE);
-
 	/* Reset the Communication Processor Module.
 	*/
-	m8xx_cpm_reset(cpm_page);
+	m8xx_cpm_reset();
 
 #ifdef CONFIG_FB_RPX
 	rpxfb_alloc_pages();

diff -Nur linux-2.6.11.orig/include/asm-ppc/commproc.h linux-2.6.11/include/asm-ppc/commproc.h
@@ -79,7 +79,7 @@
 extern void *cpm_dpram_addr(uint offset);
 extern void cpm_setbrg(uint brg, uint rate);
 
-extern uint m8xx_cpm_hostalloc(uint size);
+extern uint m8xx_cpm_hostalloc(uint size, phys_addr_t *handle);
 extern int  m8xx_cpm_hostfree(uint start);
 extern void m8xx_cpm_hostdump(void);
 

I'm concerned about this ... Why did the drivers change in 2.6 such that they now call dma_alloc_coherent() prior to the VM set up?

Anyone calling m8xx_cpm_hostalloc() is likely to be immediately calling dma_alloc_coherent(). There is some design problem here if we can't properly use these interfaces and get the mapping we need. The TLB pinning option won't work if we can't do this.

This is bogus and should not be necessary. The original serial driver was able to work without doing this, I spent lots of time so stuff like this wasn't necessary, and it's sad to see us take such a step backward.

As you can tell, I don't like it and we shouldn't need it :-) The first call to m8xx_cpm_hostalloc() should be able to call dma_consistent_alloc() to get a consistent page. If this isn't the case, we have to fix the drivers so they can do this. The only change that should be necessary is to return the physical address from m8xx_cpm_hostalloc(), if for some reason we see some need to get rid of the immensely useful iopa() function.

Thanks.

	-- Dan

Well, as far as I can see, it is expected that console_init() gets called before the main page allocator.

The startup sequence, on both v2.4 and v2.6, is:

setup_arch()
...

	console_init()
		console_8xx_init()
			register_console()
				release_console_sem() 
...
	mem_init()

register_console calls the driver ->setup method, and release_console_sem() is going to ->write pending data from the log buffer to the device.

As you well know, ->setup calls m8xx_cpm_hostalloc().

I must be missing something very obvious here, can you explain how did you arrange things in v2.4 such that the console's ->setup method is called _after_ mem_init() ?

Well, if you got this right in v2.4 then it must be possible to get it right in v2.6 too...

Thanks!

The driver used to use the serial port configuration as left by the boot rom until very late into the initialization. As I have said many times in the past, the serial driver went through three different levels of configuration. There was an early configuration that was used for kgdb, a later one used by the serial kernel printk, and finally a full configuration by the serial driver.

It's sad that "moving forward" is done at a cost of throwing away features that were important. The hack to allocate from boot memory is a quick fix, but this further ensures features like pinned TLB entries won't work.

Thanks.

	-- Dan

Can someone with 8xx using the cpm_uart driver please test the attached patch? It should avoid corruption of the pinned 8Mbyte CONFIG_PIN_TLB entry, as noted by Dan.

I have missed your statements about that in the past.

OK, the v2.4 driver is using the dual port RAM:

        /* Allocate space for two FIFOs.  We can't allocate from host
         * memory yet because vm allocator isn't initialized
         * during this early console init.
         */
        dp_addr = m8xx_cpm_dpalloc(2*EARLY_BUF_SIZE);
        mem_addr = (uint)(&cpmp->cp_dpmem[dp_addr]);

Now that I think of it, no v2.6 driver should be calling dma_alloc_coherent() before mem_init(). I was seeing the problem because we're still using the old uart driver, which does that, and our timer setup routine also.

Actually, the quick fix to use bootmem allocator ensures that the pinned TLB is _preserved_ (by using a pagetableentry from the consistent DMA address space to mark as uncached and invalidate, instead of a pte from the kernel direct mapped virtual space which blows away the 8Mbyte entry - current v2.6 state).

Why do you say that "further ensures that features like pinned TLB entries won't work" ?

If there is really a problem with using the bootmem allocator for noncacheable purposes (which I can't see), it should be pretty easy to modify cpm_uart to use dpram instead of host bootmem RAM, right?

@@ -57,7 +57,7 @@
 extern void m8xx_ide_init(void);
 
 extern unsigned long find_available_memory(void);
-extern void m8xx_cpm_reset(uint cpm_page);
+extern void m8xx_cpm_reset();
 extern void m8xx_wdt_handler_install(bd_t *bp);
 extern void rpxfb_alloc_pages(void);
 extern void cpm_interrupt_init(void);

@@ -70,13 +70,9 @@
 void __init
 m8xx_setup_arch(void)
 {
-	int	cpm_page;
-
-	cpm_page = (int) alloc_bootmem_pages(PAGE_SIZE);
-
 	/* Reset the Communication Processor Module.
 	*/
-	m8xx_cpm_reset(cpm_page);
+	m8xx_cpm_reset();
 
 #ifdef CONFIG_FB_RPX
 	rpxfb_alloc_pages();

@@ -39,8 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/rheap.h>
 
-extern int get_pteptr(struct mm_struct *mm, unsigned long addr, pte_t **ptep);
-
 static void m8xx_cpm_dpinit(void);
 static	uint	host_buffer;	/* One page of host buffer */
 static	uint	host_end;	/* end + 1 */

@@ -111,11 +109,10 @@
 extern void flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr);
 
 void
-m8xx_cpm_reset(uint bootpage)
+m8xx_cpm_reset(void)
 {
 	volatile immap_t	 *imp;
 	volatile cpm8xx_t	*commproc;
-	pte_t *pte;
 
 	imp = (immap_t *)IMAP_ADDR;
 	commproc = (cpm8xx_t *)&imp->im_cpm;

@@ -143,17 +140,6 @@
 	/* Reclaim the DP memory for our use. */
 	m8xx_cpm_dpinit();
 
-	/* get the PTE for the bootpage */
-	if (!get_pteptr(&init_mm, bootpage, &pte))
-	       panic("get_pteptr failed\n");
-																							
-	/* and make it uncachable */
-	pte_val(*pte) |= _PAGE_NO_CACHE;
-	_tlbie(bootpage);
-
-	host_buffer = bootpage;
-	host_end = host_buffer + PAGE_SIZE;
-
 	/* Tell everyone where the comm processor resides.
 	*/
 	cpmp = (cpm8xx_t *)commproc;

You shouldn't have these discussions on the weekend :)

I'm the culprit of the code in question.

This is the second take of the code. The hack with the 'if' was not actually in the first edition, since I had the hostalloc function working properly before initialization by using the remainder of the bootpage, as a backup memory area, for when the mm is not yet ready.

Somewhere along the way, it fell through the cracks.

IMHO that's the proper way to fix it.

There were some objection on that however.

Regards

Pantelis

Sorry. I try not to, but then people think I'm not interested :-) I was working on my race car data acquisition telemetry project, anyway.

I'm not trying to blame anyone. The new driver is better in many ways, we just have to fix up this one little thing so it works in all possible VM configurations.

I also have to look at the new consistent mapping functions, as those also seem to have changed. Maybe this will work fine, or can be made to work with minor changes.

Thanks.

	-- Dan

The attached patch allows the PSC ports on the MPC5200 to work with the devfs file system.

I've never posted a patch before, somebody let me know if I should do anything differently.

Mark Chambers

@@ -806,11 +806,7 @@
 	memset(&rs_driver, 0, sizeof(rs_driver));
 	rs_driver.magic = TTY_DRIVER_MAGIC;
 	rs_driver.driver_name = "serial";
-#ifdef CONFIG_DEVFS_FS
-	rs_driver.name = "tts/%d";
-#else
 	rs_driver.name = "ttyS";
-#endif
 	rs_driver.major = TTY_MAJOR;
 	rs_driver.minor_start = 64;
 	rs_driver.num = MPC5xxx_PSC_NPORTS;

@@ -841,11 +837,7 @@
 	rs_driver.hangup = gs_hangup;
 
 	rs_callout_driver = rs_driver;
-#ifdef CONFIG_DEVFS_FS
-	rs_callout_driver.name = "cua/%d";
-#else
 	rs_callout_driver.name = "cua";
-#endif
 	rs_callout_driver.major = TTYAUX_MAJOR;
 	rs_callout_driver.subtype = SERIAL_TYPE_CALLOUT;