[PATCH] cpm_uart: Made non-console uart work

Kumar, Pantelis,

This patch makes non-console UART work on both 8xx and 82xx. Various 
issues are resolved:
- removed unnecessary STOP_TX commands in shutdown - no need to 
completely stop CPM TX and reinit BDs each time the port is closing;
- when mem_addr has been allocated via dma_coherent_alloc, virt_to_bus 
on it will not tell the truth in most cases
- SCC UART needs to wait several character times even after all the BDs 
have READY bit cleared
- adds needed board-specific bits for 86xADS

Tested on 8272ADS, 885ADS and 866ADS development boards.
---------------------------------
Signed-off-by:  Vitaly Bordug <redacted>

-- 
Sincerely, 
Vitaly

I want to test this out on the 8560 ADS to see if works there as well  
before pushing upstream.

- kumar

On Aug 2, 2005, at 10:24 AM, Vitaly Bordug wrote:

Kumar, Pantelis,

This patch makes non-console UART work on both 8xx and 82xx. Various
issues are resolved:
- removed unnecessary STOP_TX commands in shutdown - no need to
completely stop CPM TX and reinit BDs each time the port is closing;
- when mem_addr has been allocated via dma_coherent_alloc, virt_to_bus
on it will not tell the truth in most cases
- SCC UART needs to wait several character times even after all the  
BDs
have READY bit cleared
- adds needed board-specific bits for 86xADS

Tested on 8272ADS, 885ADS and 866ADS development boards.
---------------------------------
Signed-off-by:  Vitaly Bordug <redacted>

-- 
Sincerely,
Vitaly


<cpm_uart_fixes.patch>

On Tuesday 02 August 2005 18:24, Vitaly Bordug wrote:

Kumar, Pantelis,

This patch makes non-console UART work on both 8xx and 82xx. Various
issues are resolved:
- removed unnecessary STOP_TX commands in shutdown - no need to
completely stop CPM TX and reinit BDs each time the port is closing;
- when mem_addr has been allocated via dma_coherent_alloc, virt_to_bus
on it will not tell the truth in most cases
- SCC UART needs to wait several character times even after all the BDs
have READY bit cleared
- adds needed board-specific bits for 86xADS

Tested on 8272ADS, 885ADS and 866ADS development boards.
---------------------------------
Signed-off-by:  Vitaly Bordug <redacted>

Vitaly, this patch is clearly in conflict with mine.

Mind taking a look at my patch and include the changes I've made?

You'll only have to change the cpm2cpu & cpu2cpm functions...

I would also argue that the STOP_TX command is needed at shutdown, since
this is the canonical way serial ports operate in linux. Also you might want
to change the protocol running on an SCC port after shutdown.

The rest are fine...

Regards

Pantelis

On Tuesday 02 August 2005 18:24, Vitaly Bordug wrote:

Kumar, Pantelis,

[snip]

Some more comments.

b/drivers/serial/cpm_uart/cpm_uart.h

b/drivers/serial/cpm_uart/cpm_uart_core.c

[off-list ref]	

Why the need to mess with the global SCC transmit enable here? 
It's dubious IMO.

Why is that? Where ever we queue a buffer descriptor with zero length.
If we ever do that we're screwed in more ways than that.

This is a good catch...

dido as above.

 	}
 
+	cpm_line_cr_cmd(line,CPM_CR_RESTART_TX);
 	return 0;
 }
 
+inline void cpm_uart_wait_until_send(struct uart_cpm_port *pinfo)
+{
+	unsigned long orig_jiffies = jiffies;
+	while(1)
+	{
+		schedule_timeout(2);
+		if(time_after(jiffies, orig_jiffies + pinfo->wait_closing))
+			break;
+	}
+}
+

perhaps, more like...

	unsigned long target_jiffies = jiffies + pinfo->wait_closing;

	while (!time_after(jiffies, target_jiffies))
   		schedule();

this looks bogus to me...

pinfo->rx_fifosize);

+	dma_addr = (u8*)(pinfo->dma_addr + L1_CACHE_ALIGN(pinfo->rx_nrfifos * 

pinfo->rx_fifosize));

b/drivers/serial/cpm_uart/cpm_uart_cpm1.c

Panto, Kumar,

Thank you for review.

My comments:

On Tuesday 02 August 2005 18:24, Vitaly Bordug wrote:
 

quoted

Kumar, Pantelis,

   

[snip]

Some more comments.

 

quoted

diff --git a/drivers/serial/cpm_uart/cpm_uart.h 
   

b/drivers/serial/cpm_uart/cpm_uart.h
 

quoted

--- a/drivers/serial/cpm_uart/cpm_uart.h
+++ b/drivers/serial/cpm_uart/cpm_uart.h

@@ -40,6 +40,8 @@

#define TX_NUM_FIFO	4
#define TX_BUF_SIZE	32

+#define SCC_WAIT_CLOSING 100
+
struct uart_cpm_port {
	struct uart_port	port;
	u16			rx_nrfifos;	

@@ -67,6 +69,8 @@ struct uart_cpm_port {

	int			 bits;
	/* Keep track of 'odd' SMC2 wirings */
	int			is_portb;
+	/* wait on close if needed */
+	int 			wait_closing;
};

extern int cpm_uart_port_map[UART_NR];

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c 
   

b/drivers/serial/cpm_uart/cpm_uart_core.c
 

quoted

--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c

@@ -12,6 +12,7 @@
 * 
 *  Copyright (C) 2004 Freescale Semiconductor, Inc.
 *            (C) 2004 Intracom, S.A.
+ * 	      (C) 2005 MontaVista Software, Inc. by Vitaly Bordug 
   

[off-list ref]	
 

quoted

 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by

@@ -143,10 +144,13 @@ static void cpm_uart_start_tx(struct uar

	}

	if (cpm_uart_tx_pump(port) != 0) {
-		if (IS_SMC(pinfo))
+		if (IS_SMC(pinfo)) {
			smcp->smc_smcm |= SMCM_TX;
-		else
+			smcp->smc_smcmr |= SMCMR_TEN;
+		} else {
			sccp->scc_sccm |= UART_SCCM_TX;
+			pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENT;
+		}
   

Why the need to mess with the global SCC transmit enable here? 
It's dubious IMO.

 

But without it (at least my boards) will have TX disabled. Just look - 
we have enabled this bit in pinfo->sccp->scc_gsmrl within ..._init_scc. 
Then all will
be fine until shutdown which will clear it and related in sccp->scc_sccm 
as well. The latter will be restored in start_tx, but scc_gsmrl will 
not. This results in the only first successful transmission. 

quoted

	}
}

@@ -265,13 +269,15 @@ static void cpm_uart_int_rx(struct uart_

		}		/* End while (i--) */

		/* This BD is ready to be used again. Clear status. get next */
-		bdp->cbd_sc &= ~(BD_SC_BR | BD_SC_FR | BD_SC_PR | BD_SC_OV);
+		bdp->cbd_sc &= ~(BD_SC_BR | BD_SC_FR | BD_SC_PR | BD_SC_OV | BD_SC_ID);
		bdp->cbd_sc |= BD_SC_EMPTY;

-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = pinfo->rx_bd_base;
-		else
-			bdp++;
+		if (bdp->cbd_datlen) {
+			if (bdp->cbd_sc & BD_SC_WRAP)
+				bdp = pinfo->rx_bd_base;
+			else
+				bdp++;
+		}
   

Why is that? Where ever we queue a buffer descriptor with zero length.
If we ever do that we're screwed in more ways than that.

 

I'm inclined to agree.

quoted

	} /* End for (;;) */

	/* Write back buffer pointer */

@@ -336,22 +342,22 @@ static irqreturn_t cpm_uart_int(int irq,

	if (IS_SMC(pinfo)) {
		events = smcp->smc_smce;
+		smcp->smc_smce = events;
		if (events & SMCM_BRKE)
			uart_handle_break(port);
		if (events & SMCM_RX)
			cpm_uart_int_rx(port, regs);
		if (events & SMCM_TX)
			cpm_uart_int_tx(port, regs);
-		smcp->smc_smce = events;
	} else {
		events = sccp->scc_scce;
+		sccp->scc_scce = events;
		if (events & UART_SCCM_BRKE)
			uart_handle_break(port);
		if (events & UART_SCCM_RX)
			cpm_uart_int_rx(port, regs);
		if (events & UART_SCCM_TX)
			cpm_uart_int_tx(port, regs);
-		sccp->scc_scce = events;
   

This is a good catch...

 

quoted

	}
	return (events) ? IRQ_HANDLED : IRQ_NONE;
}

@@ -360,6 +366,7 @@ static int cpm_uart_startup(struct uart_

{
	int retval;
	struct uart_cpm_port *pinfo = (struct uart_cpm_port *)port;
+	int line = pinfo - cpm_uart_ports;

	pr_debug("CPM uart[%d]:startup\n", port->line);

@@ -374,18 +381,30 @@ static int cpm_uart_startup(struct uart_

		pinfo->smcp->smc_smcmr |= SMCMR_REN;
	} else {
		pinfo->sccp->scc_sccm |= UART_SCCM_RX;
+		pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENR;
   

dido as above.
 

Yeah, this is superfluous as it has been done above.

quoted

	}

+	cpm_line_cr_cmd(line,CPM_CR_RESTART_TX);
	return 0;
}

+inline void cpm_uart_wait_until_send(struct uart_cpm_port *pinfo)
+{
+	unsigned long orig_jiffies = jiffies;
+	while(1)
+	{
+		schedule_timeout(2);
+		if(time_after(jiffies, orig_jiffies + pinfo->wait_closing))
+			break;
+	}
+}
+
   

perhaps, more like...

unsigned long target_jiffies = jiffies + pinfo->wait_closing;

while (!time_after(jiffies, target_jiffies))
  		schedule();

 

No objections, I'll try this one.

quoted

/*
 * Shutdown the uart
 */
static void cpm_uart_shutdown(struct uart_port *port)
{
	struct uart_cpm_port *pinfo = (struct uart_cpm_port *)port;
-	int line = pinfo - cpm_uart_ports;

	pr_debug("CPM uart[%d]:shutdown\n", port->line);

@@ -394,6 +413,12 @@ static void cpm_uart_shutdown(struct uar

	/* If the port is not the console, disable Rx and Tx. */
	if (!(pinfo->flags & FLAG_CONSOLE)) {
+		/* Wait for all the BDs marked sent */
+		while(!cpm_uart_tx_empty(port))
+			schedule_timeout(2);
+		if(pinfo->wait_closing)
+			cpm_uart_wait_until_send(pinfo);
+
		/* Stop uarts */
		if (IS_SMC(pinfo)) {
			volatile smc_t *smcp = pinfo->smcp;

@@ -405,9 +430,6 @@ static void cpm_uart_shutdown(struct uar

			sccp->scc_sccm &= ~(UART_SCCM_TX | UART_SCCM_RX);
		}

-		/* Shut them really down and reinit buffer descriptors */
-		cpm_line_cr_cmd(line, CPM_CR_STOP_TX);
-		cpm_uart_initbd(pinfo);
	}
}

@@ -569,7 +591,10 @@ static int cpm_uart_tx_pump(struct uart_

		/* Pick next descriptor and fill from buffer */
		bdp = pinfo->tx_cur;

-		p = bus_to_virt(bdp->cbd_bufaddr);
+		if (pinfo->dma_addr)
+			p=(u8*)((ulong)(pinfo->mem_addr) + bdp->cbd_bufaddr - pinfo->dma_addr);
+		else
+			p = bus_to_virt(bdp->cbd_bufaddr);
   

this looks bogus to me...

 

Well, all the stuff works on 8272 even without this and likewise stuff, 
but don't on 866ADS, where bus_to_virt returns value not equal to where 
we allocated DMA. I didn't dig too deep to track why this happens, since 
if we're using DMA, we should remember addresses upon allocation and 
avoid using bus_to_virt.




-- 
Sincerely, 
Vitaly

On Wednesday 03 August 2005 10:16, you wrote:

Panto, Kumar,

Thank you for review.

[major snipage]

Well, all the stuff works on 8272 even without this and likewise stuff,
but don't on 866ADS, where bus_to_virt returns value not equal to where
we allocated DMA. I didn't dig too deep to track why this happens, since
if we're using DMA, we should remember addresses upon allocation and
avoid using bus_to_virt.

Well, this is weird cause I've tested my latest patch on 8xx and had no 
problems...

I'll try to test your patch, some time later today.

However, what is important now is to get the fs_enet driver posted.
Please test the latest patch I've send you on a linus tree and report if you
have any problems...

Regards

Pantelis

quoted

Well, all the stuff works on 8272 even without this and likewise stuff,
but don't on 866ADS, where bus_to_virt returns value not equal to where
we allocated DMA. I didn't dig too deep to track why this happens, since
if we're using DMA, we should remember addresses upon allocation and
avoid using bus_to_virt.
   

Well, this is weird cause I've tested my latest patch on 8xx and had no 
problems...

 

Since currently I have no ability to reproduce this issue, I'm inclined 
to drop those hunks. BTW, why those bus_to_virt and virt_to_bus are 
used? Isn't it more clear to use pure addresses if they are dma-allocated?

I'll try to test your patch, some time later today.

However, what is important now is to get the fs_enet driver posted.
Please test the latest patch I've send you on a linus tree and report if you
have any problems...

 

I'll try to test it today/tomorrow as time permits...  But do you think 
it can proceed with those mii issues? Though they are almost harmless, 
the driver will complain that no PHY found but all seem to work fine 
subsequently. As far as Andy's phy will likely to be in rc14, maybe we 
can try to push this upstream (as what we have currently for 82xx and 
especially for 8xx is IMO worse than even incomplete implementation). 
I'll try to  investigate mii troubles, but it's hard to estimate efforts...

Regards

Pantelis


 

-- 
Sincerely, 
Vitaly