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