Re: [PATCH] cpm_uart: Made non-console uart work

[Vitaly Bordug <vbordug@ru.mvista.com>]

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

Panto, Kumar,

Thank you for review.

My comments:

>On Tuesday 02 August 2005 18:24, Vitaly Bordug wrote:
>  
>
>>Kumar, Pantelis,
>>
>>    
>>
>
>[snip]
>
>Some more comments.
>
>  
>
>>diff --git a/drivers/serial/cpm_uart/cpm_uart.h 
>>    
>>
>b/drivers/serial/cpm_uart/cpm_uart.h
>  
>
>>--- 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
>  
>
>>--- 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 
>>    
>>
><vbordug@ru.mvista.com>	
>  
>
>>  *
>>  * 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. 

>> 	}
>> }
>> 
>>@@ -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.

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

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

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


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