From: Vitaly Bordug <vbordug@ru•mvista.com>
To: pantelis.antoniou@gmail•com
Cc: Kumar Gala <galak@freescale•com>,
linuxppc-embedded list <linuxppc-embedded@ozlabs•org>
Subject: Re: [PATCH] cpm_uart: Made non-console uart work
Date: Wed, 03 Aug 2005 11:16:50 +0400 [thread overview]
Message-ID: <42F06F62.6050509@ru.mvista.com> (raw)
In-Reply-To: <200508030039.16107.pantelis.antoniou@gmail.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 --]
next prev parent reply other threads:[~2005-08-03 7:16 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-08-02 15:24 [PATCH] cpm_uart: Made non-console uart work Vitaly Bordug
2005-08-02 18:35 ` Kumar Gala
2005-08-02 21:26 ` Pantelis Antoniou
2005-08-02 21:39 ` Pantelis Antoniou
2005-08-03 7:16 ` Vitaly Bordug [this message]
2005-08-03 16:14 ` Pantelis Antoniou
2005-08-03 14:50 ` Vitaly Bordug
-- strict thread matches above, loose matches on Subject: below --
2005-09-09 19:21 Murch, Christopher
2005-09-09 19:59 ` Pantelis Antoniou
2005-09-12 13:47 Murch, Christopher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=42F06F62.6050509@ru.mvista.com \
--to=vbordug@ru$(echo .)mvista.com \
--cc=galak@freescale$(echo .)com \
--cc=linuxppc-embedded@ozlabs$(echo .)org \
--cc=pantelis.antoniou@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox