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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: 
http://ozlabs.org/pipermail/linuxppc-embedded/attachments/20050803/9990c46a/attachment.htm
 

Reply via email to