kernel 2.6.15: cpm_uart driver broken?

kernel 2.6.15: cpm_uart driver broken?

[David Jander]

Hi all,

Situation 1: MPC852T with SMC1 as uart/console, and SCC3/SCC4 as additional 
uarts. SMC1 works fine, but SCC3/4 don't. On transmission attempt, both UARTS 
transmit a byte 0x00 instead of what was intended to be transmitted.\

Situation 2: The same as above, but console on either SCC3 or SCC4. The uart 
being initialized as console works ok, the other two don't.

I'm pretty sure the following is wrong, but I can't seem to fix it either. 
This seems to apply for both PQ and PQ2 type uarts:
from drivers/serial/cpm_uart/cpm_uart_cpm1.c (line 190):

....
	if (is_con) {
		/* was hostalloc but changed cause it blows away the */
		/* large tlb mapping when pinning the kernel area    */
		mem_addr = (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
		dma_addr = 0;
	} else
		mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
					      GFP_KERNEL);

....
	pinfo->dp_addr = dp_offset;
	pinfo->mem_addr = mem_addr;
	pinfo->dma_addr = dma_addr;

	pinfo->rx_buf = mem_addr;
....

AFAICS pinfo->rx_buf is the pointer to a buffer as seen from the CPM's point 
of view, so it should hold a physical adress, not a virtual address. It seems 
to me that it should be more like this (lines marked with ** are changed):

....
	if (is_con) {
		/* was hostalloc but changed cause it blows away the */
		/* large tlb mapping when pinning the kernel area    */
		mem_addr = (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
**		dma_addr = mem_addr;
	} else
		mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
					      GFP_KERNEL);

....
	pinfo->dp_addr = dp_offset;
	pinfo->mem_addr = mem_addr;
	pinfo->dma_addr = dma_addr;

**	pinfo->rx_buf = dma_addr;
....

This does not work either, but I suspect this is a different problem, because 
if I change dma_alloc_coherent() for something using kmalloc() and then 
dma_addr=virt_to_phys(mem_addr), uarts begin to work, but trasmit mixed old 
and new data from the buffers due to the cache getting in the way. At least 
reception seems to work ok then. 
So, why doesn't dma_alloc_coherent() work the way one would expect?
Obviously, changing "if (is_con)" into "if (1)" all three uarts work 
correctly, but I guess we want to save on DP_RAM usage if ever possible.

What else is wrong here?

Greetings,

-- 
David Jander

Re: kernel 2.6.15: cpm_uart driver broken?

[Vitaly Bordug]

On Tue, 18 Apr 2006 14:46:08 +0200
David Jander <david.jander@protonic.nl> wrote:

> 
> Hi all,
> 
> Situation 1: MPC852T with SMC1 as uart/console, and SCC3/SCC4 as additional 
> uarts. SMC1 works fine, but SCC3/4 don't. On transmission attempt, both UARTS 
> transmit a byte 0x00 instead of what was intended to be transmitted.\
> 
> Situation 2: The same as above, but console on either SCC3 or SCC4. The uart 
> being initialized as console works ok, the other two don't.
> 
Well, existing code works just fine on 885ads and 866ads, that use SMC1 and SMC2 as UARTs.

> I'm pretty sure the following is wrong, but I can't seem to fix it either. 
> This seems to apply for both PQ and PQ2 type uarts:
> from drivers/serial/cpm_uart/cpm_uart_cpm1.c (line 190):
That's wrong - cpm_uart_cpm1.c applies to PQ only... 

> 
> ....
> 	if (is_con) {
> 		/* was hostalloc but changed cause it blows away the */
> 		/* large tlb mapping when pinning the kernel area    */
> 		mem_addr = (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
> 		dma_addr = 0;
> 	} else
> 		mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
> 					      GFP_KERNEL);
> 
> ....
> 	pinfo->dp_addr = dp_offset;
> 	pinfo->mem_addr = mem_addr;
> 	pinfo->dma_addr = dma_addr;
> 
> 	pinfo->rx_buf = mem_addr;
> ....
> 
> AFAICS pinfo->rx_buf is the pointer to a buffer as seen from the CPM's point 
> of view, so it should hold a physical adress, not a virtual address. It seems 
> to me that it should be more like this (lines marked with ** are changed):
> 

> ....
> 	if (is_con) {
> 		/* was hostalloc but changed cause it blows away the */
> 		/* large tlb mapping when pinning the kernel area    */
> 		mem_addr = (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
> **		dma_addr = mem_addr;
> 	} else
> 		mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
> 					      GFP_KERNEL);
> 
> ....
> 	pinfo->dp_addr = dp_offset;
> 	pinfo->mem_addr = mem_addr;
> 	pinfo->dma_addr = dma_addr;
> 
> **	pinfo->rx_buf = dma_addr;
> ....
> 
> This does not work either, but I suspect this is a different problem, because 
> if I change dma_alloc_coherent() for something using kmalloc() and then 
> dma_addr=virt_to_phys(mem_addr), uarts begin to work, but trasmit mixed old 
> and new data from the buffers due to the cache getting in the way. At least 
> reception seems to work ok then. 
> So, why doesn't dma_alloc_coherent() work the way one would expect?
> Obviously, changing "if (is_con)" into "if (1)" all three uarts work 
> correctly, but I guess we want to save on DP_RAM usage if ever possible.
> 
> What else is wrong here?
> 
Well, since it works on other boards, and kmalloc stuff seems to sorta work as well, then I guess reason is screwed DMA thing for your board. Check/alter CONFIG_CONSISTENT_* things and see if it will help.

And, try to just replace dma_alloc_.. with kmalloc (not changing rx_buf etc.) and reproduce the same behavior you had. BTW, rx_buf and tx_buf are never used actually in the code, hereby there is no difference what values they contain...


-- 
Sincerely, 
Vitaly

Re: kernel 2.6.15: cpm_uart driver broken?

[David Jander]

On Tuesday 18 April 2006 15:22, Vitaly Bordug wrote:
>[..]
> Well, existing code works just fine on 885ads and 866ads, that use SMC1 and
> SMC2 as UARTs.

Interesting.

> > I'm pretty sure the following is wrong, but I can't seem to fix it
> > either. This seems to apply for both PQ and PQ2 type uarts:
> > from drivers/serial/cpm_uart/cpm_uart_cpm1.c (line 190):
>
> That's wrong - cpm_uart_cpm1.c applies to PQ only...

I know. cpm_uart_cpm2.c contains the same code, so it should be as broken as 
the cpm_uart_cpm1.c.

>[...]
> Well, since it works on other boards, and kmalloc stuff seems to sorta work
> as well, then I guess reason is screwed DMA thing for your board.
> Check/alter CONFIG_CONSISTENT_* things and see if it will help.

I did not touch de defaults. What are the rules to change them? What do I have 
to look out for?
CONFIG_CONSISTENT_START = 0xff100000 and I suspect a problem here if I was 
using MTD (currently disabled) since flash is at physical adress 
0xfe000000-0xffffffff on my board. Isn't MTD going to io_remap that area to 
that same virtual adress? How's that supposed to work then?
I changed CONFIG_CONSISTENT_START to 0xfd100000 and now the system freezes 
when I write to the serial port :-(

> And, try to just replace dma_alloc_.. with kmalloc (not changing rx_buf
> etc.) and reproduce the same behavior you had. BTW, rx_buf and tx_buf are
> never used actually in the code, hereby there is no difference what values
> they contain...

Argh. Code looks obviously broken, but since it isn't used, it just works. 
That is not good! We should fix that either way then.

Thanks,

-- 
David Jander

Re: kernel 2.6.15: cpm_uart driver broken?

[Vitaly Bordug]

On Tue, 18 Apr 2006 16:05:40 +0200
David Jander <david.jander@protonic.nl> wrote:

> On Tuesday 18 April 2006 15:22, Vitaly Bordug wrote:
> >[..]
> > Well, existing code works just fine on 885ads and 866ads, that use SMC1 and
> > SMC2 as UARTs.
> 
> Interesting.
> 

And for PQ2 I can add 8260 , PQ2fads and 8272ads that had no issues with current driver
> > > I'm pretty sure the following is wrong, but I can't seem to fix it
> > > either. This seems to apply for both PQ and PQ2 type uarts:
> > > from drivers/serial/cpm_uart/cpm_uart_cpm1.c (line 190):
> >
> > That's wrong - cpm_uart_cpm1.c applies to PQ only...
> 
> I know. cpm_uart_cpm2.c contains the same code, so it should be as broken as 
> the cpm_uart_cpm1.c.
pq2 are cache-coherent, so there are no such troubles in there. And allocation routine is different of course.
> 
> >[...]
> > Well, since it works on other boards, and kmalloc stuff seems to sorta work
> > as well, then I guess reason is screwed DMA thing for your board.
> > Check/alter CONFIG_CONSISTENT_* things and see if it will help.
> 
> I did not touch de defaults. What are the rules to change them? What do I have 
> to look out for?
> CONFIG_CONSISTENT_START = 0xff100000 and I suspect a problem here if I was 
> using MTD (currently disabled) since flash is at physical adress 
> 0xfe000000-0xffffffff on my board. Isn't MTD going to io_remap that area to 
> that same virtual adress? How's that supposed to work then?
> I changed CONFIG_CONSISTENT_START to 0xfd100000 and now the system freezes 
> when I write to the serial port :-(
> 

Unfortunately, I know nothing where the correct thing should be pulled from...
But the problem lies in dma stuff from what I can say.

> > And, try to just replace dma_alloc_.. with kmalloc (not changing rx_buf
> > etc.) and reproduce the same behavior you had. BTW, rx_buf and tx_buf are
> > never used actually in the code, hereby there is no difference what values
> > they contain...
> 
> Argh. Code looks obviously broken, but since it isn't used, it just works. 
> That is not good! We should fix that either way then.

The overhaul is on the way, I'll put the cleanup of the unused stuff together with other changes.

-- 
Sincerely, 
Vitaly

kernel 2.6.15: cpm_uart driver broken?

[Kenneth Poole]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

For David Jander and Vitaly Bordug:

Regarding DMA allocation for CPM uarts, we had a similar issue with our
MPC857T and MPC885 boards. I think the real problem is that
bus_to_virt() and virt_to_bus() don't work on 8xx platforms for
addresses allocated using dma_alloc_coherent(). We had a previous
discussion Pantelis Antoniou and he agreed that the use of bus_to_virt()
and virt_to_bus() should be deprecated. This is also recommended in the
whitepaper series that discussed porting device drivers from 2.4 to 2.6.

What we did was use dma_alloc_coherent() in cpm_uart_cpm1.c, saving
dma_addr in the pinfo structure. The in cpm_uart_core.c, we use dma_addr
as the base address for the cbd_bufaddr values in each of the
descriptors. The software, when accessing the DMA buffers, uses mem_addr
as the base, applying the same offset computed previously for the dma
addresses. This technique is used in other drivers all over 2.6.

This avoids the configuration dependencies on the conversion functions.

Ken Poole
kpoole@bos.mrv.com


[-- Attachment #2: Type: text/html, Size: 5214 bytes --]

Re: kernel 2.6.15: cpm_uart driver broken?

[David Jander]

Hi Kenneth,

On Tuesday 18 April 2006 21:19, Kenneth Poole wrote:
> Regarding DMA allocation for CPM uarts, we had a similar issue with our
> MPC857T and MPC885 boards. I think the real problem is that
> bus_to_virt() and virt_to_bus() don't work on 8xx platforms for
> addresses allocated using dma_alloc_coherent(). We had a previous
> discussion Pantelis Antoniou and he agreed that the use of bus_to_virt()
> and virt_to_bus() should be deprecated. This is also recommended in the
> whitepaper series that discussed porting device drivers from 2.4 to 2.6.

You are right! I hadn't noticed the (implicit) use of virt_to_bus() in 
cpm_uart_core.c. In the case that "((unsigned long)addr >= CPM_ADDR)", the 
address isn't translated, that's why the uart keeps sending lots of 0xff's 
instead of real data: There is empty flash at that adress (after 0xff100000)! 
This also explains why the system hangs when I set CONFIG_CONSISTENT_START to 
0xfd100000, because then the CPM will access unadressed space.

Vitaly: Is it true that your ADS boards both have 
IMMAP_ADDR < CONFIG_CONSISTENT_START ?
That would explain why those do work.
On the PQ2's dma_alloc_coherent(), behaves differently, so they might also 
work.

In fact, bus_to_virt(), which is also used, does nothing more than adding 
KERNELBASE to the address, so a!=bus_to_virt(virt_to_bus(a)) if 'a' is 
obtained by anything other than kmalloc(). So if 'a' is obtained from 
dma_alloc_coherent(), will bus_to_virt(virt_to_bus(a)) at least yield a 
correctly mapped virtual address?
I am asking, because line 263 of cpm_uart_core.c does exactly this, and it 
smells ;-)

> What we did was use dma_alloc_coherent() in cpm_uart_cpm1.c, saving
> dma_addr in the pinfo structure. The in cpm_uart_core.c, we use dma_addr
> as the base address for the cbd_bufaddr values in each of the
> descriptors. The software, when accessing the DMA buffers, uses mem_addr
> as the base, applying the same offset computed previously for the dma
> addresses. This technique is used in other drivers all over 2.6.

Sounds like the correct thing to do, but do you (by any chance) have a patch 
for this change, to share?

Thanks a lot to everyone for their comments and help.

Greetings,

-- 
David Jander

Re: kernel 2.6.15: cpm_uart driver broken?

[Vitaly Bordug]

On Wed, 19 Apr 2006 11:28:49 +0200
David Jander <david.jander@protonic.nl> wrote:

> 
> Hi Kenneth,
> 
> On Tuesday 18 April 2006 21:19, Kenneth Poole wrote:
> > Regarding DMA allocation for CPM uarts, we had a similar issue with our
> > MPC857T and MPC885 boards. I think the real problem is that
> > bus_to_virt() and virt_to_bus() don't work on 8xx platforms for
> > addresses allocated using dma_alloc_coherent(). We had a previous
> > discussion Pantelis Antoniou and he agreed that the use of bus_to_virt()
> > and virt_to_bus() should be deprecated. This is also recommended in the
> > whitepaper series that discussed porting device drivers from 2.4 to 2.6.
> 
> You are right! I hadn't noticed the (implicit) use of virt_to_bus() in 
> cpm_uart_core.c. In the case that "((unsigned long)addr >= CPM_ADDR)", the 
> address isn't translated, that's why the uart keeps sending lots of 0xff's 
> instead of real data: There is empty flash at that adress (after 0xff100000)! 
> This also explains why the system hangs when I set CONFIG_CONSISTENT_START to 
> 0xfd100000, because then the CPM will access unadressed space.
> 
> Vitaly: Is it true that your ADS boards both have 
> IMMAP_ADDR < CONFIG_CONSISTENT_START ?
> That would explain why those do work.
> On the PQ2's dma_alloc_coherent(), behaves differently, so they might also 
> work.
> 
Right. 

> In fact, bus_to_virt(), which is also used, does nothing more than adding 
> KERNELBASE to the address, so a!=bus_to_virt(virt_to_bus(a)) if 'a' is 
> obtained by anything other than kmalloc(). So if 'a' is obtained from 
> dma_alloc_coherent(), will bus_to_virt(virt_to_bus(a)) at least yield a 
> correctly mapped virtual address?
> I am asking, because line 263 of cpm_uart_core.c does exactly this, and it 
> smells ;-)
> 

Well I was aware about no-good there, but since it works fine with my HW, and no other complaints, there were no reason to overhaul.

> > What we did was use dma_alloc_coherent() in cpm_uart_cpm1.c, saving
> > dma_addr in the pinfo structure. The in cpm_uart_core.c, we use dma_addr
> > as the base address for the cbd_bufaddr values in each of the
> > descriptors. The software, when accessing the DMA buffers, uses mem_addr
> > as the base, applying the same offset computed previously for the dma
> > addresses. This technique is used in other drivers all over 2.6.
> 
> Sounds like the correct thing to do, but do you (by any chance) have a patch 
> for this change, to share?
> 

If there is any code to reference, I'd appreciate it (and merge to the driver). 



-- 
Sincerely, 
Vitaly

RE: kernel 2.6.15: cpm_uart driver broken?

[Kenneth Poole]

>If there is any code to reference, I'd appreciate it (and merge to the
>driver).=20

Ok, here's a patch, below. It includes changes for DMA buffer allocation
as discussed. This is untested, because the CPM uart driver for our
platforms has many other modifications that don't apply.

It also includes a fix for sending x_char in cpm_uart_tx_pump(). This
allows the actual x_char to be sent, instead of the next regular
character.

-------------------------------------------------------------
--- cpm_uart_core.c.orig	2006-04-19 10:24:47.000000000 -0400
+++ cpm_uart_core.c	2006-04-19 10:51:43.000000000 -0400
@@ -71,20 +71,6 @@
=20
 /**************************************************************/
=20
-static inline unsigned long cpu2cpm_addr(void *addr)
-{
-	if ((unsigned long)addr >=3D CPM_ADDR)
-		return (unsigned long)addr;
-	return virt_to_bus(addr);
-}
-
-static inline void *cpm2cpu_addr(unsigned long addr)
-{
-	if (addr >=3D CPM_ADDR)
-		return (void *)addr;
-	return bus_to_virt(addr);
-}
-
 /*
  * Check, if transmit buffers are processed
 */
@@ -261,7 +247,7 @@
 		}
=20
 		/* get pointer */
-		cp =3D cpm2cpu_addr(bdp->cbd_bufaddr);
+		cp =3D (unsigned char *)pinfo->mem_addr +
(bdp->cbd_bufaddr - pinfo->dma_addr);
=20
 		/* loop through the buffer */
 		while (i-- > 0) {
@@ -606,11 +592,12 @@
 		/* Pick next descriptor and fill from buffer */
 		bdp =3D pinfo->tx_cur;
=20
-		p =3D cpm2cpu_addr(bdp->cbd_bufaddr);
+		p =3D (unsigned char *)pinfo->mem_addr + (bdp->cbd_bufaddr
- pinfo->dma_addr);
=20
-		*p++ =3D xmit->buf[xmit->tail];
+		*p =3D port->x_char;
 		bdp->cbd_datlen =3D 1;
 		bdp->cbd_sc |=3D BD_SC_READY;
+		__asm__("eieio");
 		/* Get next BD. */
 		if (bdp->cbd_sc & BD_SC_WRAP)
 			bdp =3D pinfo->tx_bd_base;
@@ -633,7 +620,7 @@
=20
 	while (!(bdp->cbd_sc & BD_SC_READY) && (xmit->tail !=3D
xmit->head)) {
 		count =3D 0;
-		p =3D cpm2cpu_addr(bdp->cbd_bufaddr);
+		p =3D (unsigned char *)pinfo->mem_addr + (bdp->cbd_bufaddr
- pinfo->dma_addr);
 		while (count < pinfo->tx_fifosize) {
 			*p++ =3D xmit->buf[xmit->tail];
 			xmit->tail =3D (xmit->tail + 1) & (UART_XMIT_SIZE
- 1);
@@ -670,39 +657,37 @@
 static void cpm_uart_initbd(struct uart_cpm_port *pinfo)
 {
 	int i;
-	u8 *mem_addr;
+	dma_addr_t dma_addr;
 	volatile cbd_t *bdp;
=20
 	pr_debug("CPM uart[%d]:initbd\n", pinfo->port.line);
=20
 	/* Set the physical address of the host memory
-	 * buffers in the buffer descriptors, and the
-	 * virtual address for us to work with.
+	 * buffers in the buffer descriptors.
 	 */
-	mem_addr =3D pinfo->mem_addr;
+	dma_addr =3D pinfo->dma_addr;
 	bdp =3D pinfo->rx_cur =3D pinfo->rx_bd_base;
 	for (i =3D 0; i < (pinfo->rx_nrfifos - 1); i++, bdp++) {
-		bdp->cbd_bufaddr =3D cpu2cpm_addr(mem_addr);
+		bdp->cbd_bufaddr =3D dma_addr;
 		bdp->cbd_sc =3D BD_SC_EMPTY | BD_SC_INTRPT;
-		mem_addr +=3D pinfo->rx_fifosize;
+		dma_addr +=3D pinfo->rx_fifosize;
 	}
-
-	bdp->cbd_bufaddr =3D cpu2cpm_addr(mem_addr);
+=09
+	bdp->cbd_bufaddr =3D dma_addr;
 	bdp->cbd_sc =3D BD_SC_WRAP | BD_SC_EMPTY | BD_SC_INTRPT;
=20
 	/* Set the physical address of the host memory
-	 * buffers in the buffer descriptors, and the
-	 * virtual address for us to work with.
+	 * buffers in the buffer descriptors.
 	 */
-	mem_addr =3D pinfo->mem_addr + L1_CACHE_ALIGN(pinfo->rx_nrfifos *
pinfo->rx_fifosize);
+	dma_addr =3D pinfo->dma_addr + L1_CACHE_ALIGN(pinfo->rx_nrfifos *
pinfo->rx_fifosize);
 	bdp =3D pinfo->tx_cur =3D pinfo->tx_bd_base;
 	for (i =3D 0; i < (pinfo->tx_nrfifos - 1); i++, bdp++) {
-		bdp->cbd_bufaddr =3D cpu2cpm_addr(mem_addr);
+		bdp->cbd_bufaddr =3D dma_addr;
 		bdp->cbd_sc =3D BD_SC_INTRPT;
-		mem_addr +=3D pinfo->tx_fifosize;
+		dma_addr +=3D pinfo->tx_fifosize;
 	}
-
-	bdp->cbd_bufaddr =3D cpu2cpm_addr(mem_addr);
+=09
+	bdp->cbd_bufaddr =3D dma_addr;
 	bdp->cbd_sc =3D BD_SC_WRAP | BD_SC_INTRPT;
 }
=20
@@ -1032,7 +1017,7 @@
 		 * If the buffer address is in the CPM DPRAM, don't
 		 * convert it.
 		 */
-		cp =3D cpm2cpu_addr(bdp->cbd_bufaddr);
+		cp =3D (unsigned char *)pinfo->mem_addr +
(bdp->cbd_bufaddr - pinfo->dma_addr);
=20
 		*cp =3D *s;
=20
@@ -1049,7 +1034,7 @@
 			while ((bdp->cbd_sc & BD_SC_READY) !=3D 0)
 				;
=20
-			cp =3D cpm2cpu_addr(bdp->cbd_bufaddr);
+			cp =3D (unsigned char *)pinfo->mem_addr +
(bdp->cbd_bufaddr - pinfo->dma_addr);
=20
 			*cp =3D 13;
 			bdp->cbd_datlen =3D 1;
--- cpm_uart_cpm1.c.orig	2006-04-19 10:26:46.000000000 -0400
+++ cpm_uart_cpm1.c	2006-04-19 10:32:05.000000000 -0400
@@ -191,7 +191,7 @@
 		/* was hostalloc but changed cause it blows away the */
 		/* large tlb mapping when pinning the kernel area    */
 		mem_addr =3D (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
-		dma_addr =3D 0;
+		dma_addr =3D (dma_addr_t)mem_addr;
 	} else
 		mem_addr =3D dma_alloc_coherent(NULL, memsz, &dma_addr,
 					      GFP_KERNEL);

----------------------------------------------------------

Ken Poole
kpoole@bos.mrv.com
=20

Re: kernel 2.6.15: cpm_uart driver broken?

[Dan Malek]

On Apr 19, 2006, at 3:24 PM, Kenneth Poole wrote:

>
>  		/* get pointer */
> -		cp = cpm2cpu_addr(bdp->cbd_bufaddr);
> +		cp = (unsigned char *)pinfo->mem_addr +
> (bdp->cbd_bufaddr - pinfo->dma_addr);


Ummm, no.  Keep the cpm2cpu_addr() and pass it
some driver data structure pointer so it does the computes,
or better, keep the phys/virt addresses in a handy data
structure you can easily access and work with offsets within
the different address spaces.  The test of addr >= CPM_ADDR
is critically important to early boot and kgdb support
and can't be removed.


> -		bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr);
> +		bdp->cbd_bufaddr = dma_addr;

This kind of initialization is broken, too.  You have to test
that memory address and return the proper space.

Thanks.

	-- Dan

RE: kernel 2.6.15: cpm_uart driver broken?

[Kenneth Poole]

Hello Dan:

>
>From: Dan Malek [mailto:dan@embeddedalley.com]=20
>Sent: Wednesday, April 19, 2006 3:42 PM
>To: Kenneth Poole
>Cc: Vitaly Bordug; David Jander; linuxppc-embedded@ozlabs.org
>Subject: Re: kernel 2.6.15: cpm_uart driver broken?


>On Apr 19, 2006, at 3:24 PM, Kenneth Poole wrote:

>>
>>  		/* get pointer */
>> -		cp =3D cpm2cpu_addr(bdp->cbd_bufaddr);
>> +		cp =3D (unsigned char *)pinfo->mem_addr +
>> (bdp->cbd_bufaddr - pinfo->dma_addr);


>Ummm, no.  Keep the cpm2cpu_addr() and pass it
>some driver data structure pointer so it does the computes,
>

We did this to solve the problem mentioned earlier in this thread which
is that bus_to_virt() and virt_to_bus() don't work for memory allocated
with dma_alloc_coherent(). By saving the return value from
dma_alloc_coherent() in dma_addr, we avoid having to do those
conversions all of the time.

>or better, keep the phys/virt addresses in a handy data
>structure you can easily access and work with offsets within
>the different address spaces.

That's what we're doing by using dma_addr. We just compute the offset in
the physical address space and apply that offset in the virtual address
space. Some drivers keep both sets of pointers around for each buffer
allocated, and that would work as well.

>  The test of addr >=3D CPM_ADDR
>is critically important to early boot and kgdb support
>and can't be removed.

I assumed that this test was simply to exclude console ports from
conversion. But in cpm_uart_allocbuf() there already is a test (is_con).
For the console, we simply save the unconverted dma_addr in the port
structure once and use it normally. Our console port works just fine.
Testing addr >=3D CPM_ADDR forces very specific ordering onto the IMMR =
and
DMA address spaces, and I thought our approach was more general.=20

>> -		bdp->cbd_bufaddr =3D cpu2cpm_addr(mem_addr);
>> +		bdp->cbd_bufaddr =3D dma_addr;

>This kind of initialization is broken, too.  You have to test
>that memory address and return the proper space.

>Thanks.

>	-- Dan

Re: kernel 2.6.15: cpm_uart driver broken?

[Dan Malek]

On Apr 19, 2006, at 4:40 PM, Kenneth Poole wrote:

> .....  we avoid having to do those
> conversions all of the time.

You are still doing a very similar conversion, only with
different addresses and offsets, and more compute
operations.

> I assumed that this test was simply to exclude console ports from
> conversion.

Bad assumption.  It's done to be able to use buffers allocated out
of the CPM memory or before the VM is sufficiently configured to
relocate the buffers.   This is needed for early debug prints, xmon,
and kgdb.

These modifications further remove features that have been
part of past kernels, and it has to stop.

Thanks.

	-- Dan

Re: kernel 2.6.15: cpm_uart driver broken?

[David Jander]

Hi,

On Wednesday 19 April 2006 23:14, Dan Malek wrote:
> > I assumed that this test was simply to exclude console ports from
> > conversion.
>
> Bad assumption.  It's done to be able to use buffers allocated out
> of the CPM memory or before the VM is sufficiently configured to
> relocate the buffers.   This is needed for early debug prints, xmon,
> and kgdb.

Yes, but it's broken. In order to work as expected it had to be something 
like:

if((addr >= CPM_ADDR) && (addr <= CPM_ADDR_END))....

Otherwise you are also, in some configurations, excluding dma allocated memory 
from mapping, since in those cases (my case for instance) 
CONFIG_CONSISTENT_START > CPM_ADDR_END. (Assuming CPM_ADDR_END is something 
like CPM_ADDR+sizeof(DPRAM)).

What about the following patch?
This patch adds another member to "struct uart_cpm_port" to store a 
dma_offset. That offset is computed in cpm_uart_cpm?.c and passed to the 
cpu2cpm_addr() and cpm2cpu_addr() functions as a second argument.
One thing that still smells IMHO, is the check in cpm2cpu_addr() to see if we 
originally come from a dma-type address, although it should work under all 
situations I can think of. Any ideas?

=======================================
--- drivers/serial/cpm_uart/cpm_uart.h  (revision 513)
+++ drivers/serial/cpm_uart/cpm_uart.h  (working copy)
@@ -64,6 +64,7 @@
        uint                     dp_addr;
        void                    *mem_addr;
        dma_addr_t               dma_addr;
+       unsigned long    dma_offset;
        /* helpers */
        int                      baud;
        int                      bits;
--- drivers/serial/cpm_uart/cpm_uart_cpm1.c     (revision 513)
+++ drivers/serial/cpm_uart/cpm_uart_cpm1.c     (working copy)
@@ -191,11 +191,11 @@
                /* was hostalloc but changed cause it blows away the */
                /* large tlb mapping when pinning the kernel area    */
                mem_addr = (u8 *) cpm_dpram_addr(cpm_dpalloc(memsz, 8));
-               dma_addr = 0;
+               dma_addr = (dma_addr_t)mem_addr;
        } else
                mem_addr = dma_alloc_coherent(NULL, memsz, &dma_addr,
                                              GFP_KERNEL);
@@ -206,6 +206,7 @@
        pinfo->dp_addr = dp_offset;
        pinfo->mem_addr = mem_addr;
        pinfo->dma_addr = dma_addr;
+       pinfo->dma_offset = (dma_addr_t)((unsigned long)dma_addr - (unsigned 
long)mem_addr);

        pinfo->rx_buf = mem_addr;
        pinfo->tx_buf = pinfo->rx_buf + L1_CACHE_ALIGN(pinfo->rx_nrfifos
--- drivers/serial/cpm_uart/cpm_uart_core.c     (revision 518)
+++ drivers/serial/cpm_uart/cpm_uart_core.c     (working copy)
@@ -71,17 +71,19 @@

 /**************************************************************/

-static inline unsigned long cpu2cpm_addr(void *addr)
+static inline unsigned long cpu2cpm_addr(void *addr, unsigned long offset)
 {
-       if ((unsigned long)addr >= CPM_ADDR)
-               return (unsigned long)addr;
+       if (((unsigned long)addr >= CPM_ADDR)
+               || ((unsigned long)addr >= CONFIG_CONSISTENT_START))
+               return (unsigned long)addr + offset;
        return virt_to_bus(addr);
 }

-static inline void *cpm2cpu_addr(unsigned long addr)
+static inline void *cpm2cpu_addr(unsigned long addr, unsigned long offset)
 {
-       if (addr >= CPM_ADDR)
-               return (void *)addr;
+       if (((unsigned long)(addr - offset) >= CPM_ADDR)
+               || ((unsigned long)(addr - offset) >= 
CONFIG_CONSISTENT_START))
+               return (void *)(addr - offset);
        return bus_to_virt(addr);
 }
@@ -261,7 +262,7 @@
                }

                /* get pointer */
-               cp = cpm2cpu_addr(bdp->cbd_bufaddr);
+               cp = cpm2cpu_addr(bdp->cbd_bufaddr, pinfo->dma_offset);

                /* loop through the buffer */
                while (i-- > 0) {
@@ -615,7 +605,7 @@
                /* Pick next descriptor and fill from buffer */
                bdp = pinfo->tx_cur;

-               p = cpm2cpu_addr(bdp->cbd_bufaddr);
+               p = cpm2cpu_addr(bdp->cbd_bufaddr, pinfo->dma_offset);

                *p++ = xmit->buf[xmit->tail];
                bdp->cbd_datlen = 1;
@@ -642,7 +632,7 @@

        while (!(bdp->cbd_sc & BD_SC_READY) && (xmit->tail != xmit->head)) {
                count = 0;
-               p = cpm2cpu_addr(bdp->cbd_bufaddr);
+               p = cpm2cpu_addr(bdp->cbd_bufaddr, pinfo->dma_offset);
                while (count < pinfo->tx_fifosize) {
                        *p++ = xmit->buf[xmit->tail];
                        xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
@@ -680,6 +670,7 @@
 {
        int i;
        u8 *mem_addr;
+       unsigned long dma_offset;
        volatile cbd_t *bdp;

        pr_debug("CPM uart[%d]:initbd\n", pinfo->port.line);
@@ -689,14 +680,15 @@
         * virtual address for us to work with.
         */
        mem_addr = pinfo->mem_addr;
+       dma_offset = pinfo->dma_offset;
        bdp = pinfo->rx_cur = pinfo->rx_bd_base;
        for (i = 0; i < (pinfo->rx_nrfifos - 1); i++, bdp++) {
-               bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr);
+               bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr, dma_offset);
                bdp->cbd_sc = BD_SC_EMPTY | BD_SC_INTRPT;
                mem_addr += pinfo->rx_fifosize;
        }

-       bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr);
+       bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr, dma_offset);
        bdp->cbd_sc = BD_SC_WRAP | BD_SC_EMPTY | BD_SC_INTRPT;

        /* Set the physical address of the host memory
@@ -706,12 +698,12 @@
        mem_addr = pinfo->mem_addr + L1_CACHE_ALIGN(pinfo->rx_nrfifos * 
pinfo->rx_fifosize);
        bdp = pinfo->tx_cur = pinfo->tx_bd_base;
        for (i = 0; i < (pinfo->tx_nrfifos - 1); i++, bdp++) {
-               bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr);
+               bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr, dma_offset);
                bdp->cbd_sc = BD_SC_INTRPT;
                mem_addr += pinfo->tx_fifosize;
        }

-       bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr);
+       bdp->cbd_bufaddr = cpu2cpm_addr(mem_addr, dma_offset);
        bdp->cbd_sc = BD_SC_WRAP | BD_SC_INTRPT;
 }

@@ -1041,7 +1033,7 @@
                 * If the buffer address is in the CPM DPRAM, don't
                 * convert it.
                 */
-               cp = cpm2cpu_addr(bdp->cbd_bufaddr);
+               cp = cpm2cpu_addr(bdp->cbd_bufaddr, pinfo->dma_offset);

                *cp = *s;

@@ -1058,7 +1050,7 @@
                        while ((bdp->cbd_sc & BD_SC_READY) != 0)
                                ;

-                       cp = cpm2cpu_addr(bdp->cbd_bufaddr);
+                       cp = cpm2cpu_addr(bdp->cbd_bufaddr, 
pinfo->dma_offset);

                        *cp = 13;
                        bdp->cbd_datlen = 1;
======================================================

cpm_uart_cpm2.c may need the same changes as cpm_uart_cpm1.c.

> These modifications further remove features that have been
> part of past kernels, and it has to stop.

Hmmm. What do you mean? The driver is broken, we want to fix it.

Greetings,

-- 
David Jander
Protonic Holland.
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag

Re: kernel 2.6.15: cpm_uart driver broken?

[raul.moreno]

Hi,

I've found this thread, but I haven't seen the end of the discussion. I=
 am
using this kernel version, and I am also having problems with the cpm_u=
art.
Only the console (SMC1) is working, the other devices working as uart f=
ail.
I've realised that the problem is in the DMA. The system crash when the=
 Rx
and Tx buffers are allocated in external memory (in case of allocating =
in
the DPRAM of my mpc866, it works properly) and I don't know why. It is
supposed other devices (as fec) use the same functions
("dma_alloc_coherent") and they work.

Moreover I don't find any documentation about the parameters
"CONFIG_CONSISTENT_START" and "CONFIG_CONSISTENT_SIZE". Does these
parameters affect to the driver behaviour?

Could anyone help me?

Kind regards,

Ra=FAl Moreno
***********Internet Email Confidentiality Footer*************
This email and any files transmitted with it are confidential and inten=
ded
solely for the use of the organization or individual to whom they are
addressed.  It is expressly forbidden to retransmit or copy email and/o=
r
this  attached files without our permission .  If you are not the
addressee indicated in this message (or responsible for delivery of the=

message to such person), you may not copy or deliver this message
to anyone. In such case, you should destroy this message and kindly
notify the sender by reply email. Please advise immediately if you or
your employer does not consent to Internet email for messages of this
kind.  Opinions, conclusions and other information in this message that=

do not relate to the official business of my firm shall be understood a=
s
neither given nor endorsed by it.=