2013-07-24 18:37:38

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 0/4] msm_serial fixes and improvements

Here are a couple fixes and improvements to the msm_serial
driver. The first two patches fix a bug on UARTDM devices and
silence sparse warnings. The last two patches improve the baud
code detection and increase the throughput on UARTDM devices.

After this it is rather trivial to add support for newer UARTDM
devices via new compatible fields.

Stephen Boyd (4):
msm_serial: Fix NUL byte output on UARTDM
msm_serial: Fix sparse warnings
msm_serial: Make baud_code detection more dynamic
msm_serial: Send more than 1 character at a time on UARTDM

drivers/tty/serial/msm_serial.c | 180 ++++++++++++++++++++--------------------
drivers/tty/serial/msm_serial.h | 19 ++---
2 files changed, 97 insertions(+), 102 deletions(-)

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation


2013-07-24 18:37:41

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 4/4] msm_serial: Send more than 1 character at a time on UARTDM

UARTDM cores have a TX fifo that can accept more than one
character per register write, but the msm_serial driver currently
only supports 1 character mode. Add support for this mode of operation
to speed up the transmit path on DM devices.

Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 51 ++++++++++++++++++++++++++---------------
1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index cef8f40..97642ec 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -195,10 +195,10 @@ static void handle_rx(struct uart_port *port)
tty_flip_buffer_push(tport);
}

-static void reset_dm_count(struct uart_port *port)
+static void reset_dm_count(struct uart_port *port, int count)
{
wait_for_xmitr(port);
- msm_write(port, 1, UARTDM_NCF_TX);
+ msm_write(port, count, UARTDM_NCF_TX);
msm_read(port, UARTDM_NCF_TX);
}

@@ -206,39 +206,52 @@ static void handle_tx(struct uart_port *port)
{
struct circ_buf *xmit = &port->state->xmit;
struct msm_port *msm_port = UART_TO_MSM(port);
- int sent_tx;
+ unsigned int tx_count, num_chars;
+ unsigned int tf_pointer = 0;
+
+ tx_count = uart_circ_chars_pending(xmit);
+ tx_count = min3(tx_count, (unsigned int)UART_XMIT_SIZE - xmit->tail,
+ port->fifosize);

if (port->x_char) {
if (msm_port->is_uartdm)
- reset_dm_count(port);
+ reset_dm_count(port, tx_count + 1);

msm_write(port, port->x_char,
msm_port->is_uartdm ? UARTDM_TF : UART_TF);
port->icount.tx++;
port->x_char = 0;
+ } else if (tx_count && msm_port->is_uartdm) {
+ reset_dm_count(port, tx_count);
}

- if (msm_port->is_uartdm)
- reset_dm_count(port);
+ while (tf_pointer < tx_count) {
+ int i;
+ char buf[4] = { 0 };
+ unsigned int *bf = (unsigned int *)&buf;

- while (msm_read(port, UART_SR) & UART_SR_TX_READY) {
- if (uart_circ_empty(xmit)) {
- /* disable tx interrupts */
- msm_port->imr &= ~UART_IMR_TXLEV;
- msm_write(port, msm_port->imr, UART_IMR);
+ if (!(msm_read(port, UART_SR) & UART_SR_TX_READY))
break;
- }
- msm_write(port, xmit->buf[xmit->tail],
- msm_port->is_uartdm ? UARTDM_TF : UART_TF);

if (msm_port->is_uartdm)
- reset_dm_count(port);
+ num_chars = min(tx_count - tf_pointer, sizeof(buf));
+ else
+ num_chars = 1;

- xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
- port->icount.tx++;
- sent_tx = 1;
+ for (i = 0; i < num_chars; i++) {
+ buf[i] = xmit->buf[xmit->tail + i];
+ port->icount.tx++;
+ }
+
+ msm_write(port, *bf, msm_port->is_uartdm ? UARTDM_TF : UART_TF);
+ xmit->tail = (xmit->tail + num_chars) & (UART_XMIT_SIZE - 1);
+ tf_pointer += num_chars;
}

+ /* disable tx interrupts if nothing more to send */
+ if (uart_circ_empty(xmit))
+ msm_stop_tx(port);
+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(port);
}
@@ -757,7 +770,7 @@ static void msm_console_putchar(struct uart_port *port, int c)
struct msm_port *msm_port = UART_TO_MSM(port);

if (msm_port->is_uartdm)
- reset_dm_count(port);
+ reset_dm_count(port, 1);

while (!(msm_read(port, UART_SR) & UART_SR_TX_READY))
;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 18:37:40

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 1/4] msm_serial: Fix NUL byte output on UARTDM

UARTDM serial devices require us to wait for the entire TX fifo
to drain before we can change the contents of the NCF_TX
register. Furthermore, if we write any characters to the TX fifo
within the same clock cycle of changing the NCF_TX register the
NCF_TX register won't latch properly.

To fix these issues we should read back the NCF_TX register to
delay any TX fifo accesses by a clock cycle and we should wait
for the TX fifo to drain (instead of just waiting for the fifo to
be ready to receive more characters). Failure to do so leads to
random NUL bytes interspersed in the output.

Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 14 +++++++++-----
drivers/tty/serial/msm_serial.h | 1 +
2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 2c6cfb3..fa3fc67 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -50,11 +50,14 @@ struct msm_port {
unsigned int old_snap_state;
};

-static inline void wait_for_xmitr(struct uart_port *port, int bits)
+static inline void wait_for_xmitr(struct uart_port *port)
{
- if (!(msm_read(port, UART_SR) & UART_SR_TX_EMPTY))
- while ((msm_read(port, UART_ISR) & bits) != bits)
- cpu_relax();
+ while (!(msm_read(port, UART_SR) & UART_SR_TX_EMPTY)) {
+ if (msm_read(port, UART_ISR) & UART_ISR_TX_READY)
+ break;
+ udelay(1);
+ }
+ msm_write(port, UART_CR_CMD_RESET_TX_READY, UART_CR);
}

static void msm_stop_tx(struct uart_port *port)
@@ -194,8 +197,9 @@ static void handle_rx(struct uart_port *port)

static void reset_dm_count(struct uart_port *port)
{
- wait_for_xmitr(port, UART_ISR_TX_READY);
+ wait_for_xmitr(port);
msm_write(port, 1, UARTDM_NCF_TX);
+ msm_read(port, UARTDM_NCF_TX);
}

static void handle_tx(struct uart_port *port)
diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
index e4acef5..15c186e 100644
--- a/drivers/tty/serial/msm_serial.h
+++ b/drivers/tty/serial/msm_serial.h
@@ -71,6 +71,7 @@
#define UART_CR_CMD_RESET_RFR (14 << 4)
#define UART_CR_CMD_PROTECTION_EN (16 << 4)
#define UART_CR_CMD_STALE_EVENT_ENABLE (80 << 4)
+#define UART_CR_CMD_RESET_TX_READY (3 << 8)
#define UART_CR_TX_DISABLE (1 << 3)
#define UART_CR_TX_ENABLE (1 << 2)
#define UART_CR_RX_DISABLE (1 << 1)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 18:38:48

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 3/4] msm_serial: Make baud_code detection more dynamic

Currently msm_set_baud_rate() assumes the uart clock rate is
1.8432 MHz. This is not always true, and limits our options to
program the baud rate. Instead of assuming the rate and
hard-coding the baud_code based on it, calculate the divider that
we want and try to find the closest baud_code that matches. This
allows us to support uarts with faster clock speeds.

Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 98 ++++++++++++++++++-----------------------
drivers/tty/serial/msm_serial.h | 18 ++------
2 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index cab4105..cef8f40 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -322,70 +322,60 @@ static void msm_break_ctl(struct uart_port *port, int break_ctl)
msm_write(port, UART_CR_CMD_STOP_BREAK, UART_CR);
}

+struct msm_baud_map {
+ u16 divisor;
+ u8 code;
+ u8 rxstale;
+};
+
+static const struct msm_baud_map *
+msm_find_best_baud(struct uart_port *port, unsigned int baud)
+{
+ unsigned int i, divisor;
+ const struct msm_baud_map *entry;
+ static const struct msm_baud_map table[] = {
+ { 1536, 0x00, 1 },
+ { 768, 0x11, 1 },
+ { 384, 0x22, 1 },
+ { 192, 0x33, 1 },
+ { 96, 0x44, 1 },
+ { 48, 0x55, 1 },
+ { 32, 0x66, 1 },
+ { 24, 0x77, 1 },
+ { 16, 0x88, 1 },
+ { 12, 0x99, 6 },
+ { 8, 0xaa, 6 },
+ { 6, 0xbb, 6 },
+ { 4, 0xcc, 6 },
+ { 3, 0xdd, 8 },
+ { 2, 0xee, 16 },
+ { 1, 0xff, 31 },
+ };
+
+ divisor = uart_get_divisor(port, baud);
+
+ for (i = 0, entry = table; i < ARRAY_SIZE(table); i++, entry++)
+ if (entry->divisor <= divisor)
+ break;
+
+ return entry; /* Default to smallest divider */
+}
+
static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
{
- unsigned int baud_code, rxstale, watermark;
+ unsigned int rxstale, watermark;
struct msm_port *msm_port = UART_TO_MSM(port);
+ const struct msm_baud_map *entry;

- switch (baud) {
- case 300:
- baud_code = UART_CSR_300;
- rxstale = 1;
- break;
- case 600:
- baud_code = UART_CSR_600;
- rxstale = 1;
- break;
- case 1200:
- baud_code = UART_CSR_1200;
- rxstale = 1;
- break;
- case 2400:
- baud_code = UART_CSR_2400;
- rxstale = 1;
- break;
- case 4800:
- baud_code = UART_CSR_4800;
- rxstale = 1;
- break;
- case 9600:
- baud_code = UART_CSR_9600;
- rxstale = 2;
- break;
- case 14400:
- baud_code = UART_CSR_14400;
- rxstale = 3;
- break;
- case 19200:
- baud_code = UART_CSR_19200;
- rxstale = 4;
- break;
- case 28800:
- baud_code = UART_CSR_28800;
- rxstale = 6;
- break;
- case 38400:
- baud_code = UART_CSR_38400;
- rxstale = 8;
- break;
- case 57600:
- baud_code = UART_CSR_57600;
- rxstale = 16;
- break;
- case 115200:
- default:
- baud_code = UART_CSR_115200;
- baud = 115200;
- rxstale = 31;
- break;
- }
+ entry = msm_find_best_baud(port, baud);

if (msm_port->is_uartdm)
msm_write(port, UART_CR_CMD_RESET_RX, UART_CR);

- msm_write(port, baud_code, UART_CSR);
+ msm_write(port, entry->code, UART_CSR);

/* RX stale watermark */
+ rxstale = entry->rxstale;
watermark = UART_IPR_STALE_LSB & rxstale;
watermark |= UART_IPR_RXSTALE_LAST;
watermark |= UART_IPR_STALE_TIMEOUT_MSB & (rxstale << 2);
diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
index 15c186e..469fda5 100644
--- a/drivers/tty/serial/msm_serial.h
+++ b/drivers/tty/serial/msm_serial.h
@@ -38,19 +38,7 @@
#define UART_MR2_PARITY_MODE_SPACE 0x3
#define UART_MR2_PARITY_MODE 0x3

-#define UART_CSR 0x0008
-#define UART_CSR_115200 0xFF
-#define UART_CSR_57600 0xEE
-#define UART_CSR_38400 0xDD
-#define UART_CSR_28800 0xCC
-#define UART_CSR_19200 0xBB
-#define UART_CSR_14400 0xAA
-#define UART_CSR_9600 0x99
-#define UART_CSR_4800 0x77
-#define UART_CSR_2400 0x55
-#define UART_CSR_1200 0x44
-#define UART_CSR_600 0x33
-#define UART_CSR_300 0x22
+#define UART_CSR 0x0008

#define UART_TF 0x000C
#define UARTDM_TF 0x0070
@@ -152,6 +140,7 @@ static inline void msm_serial_set_mnd_regs_tcxo(struct uart_port *port)
msm_write(port, 0xF1, UART_NREG);
msm_write(port, 0x0F, UART_DREG);
msm_write(port, 0x1A, UART_MNDREG);
+ port->uartclk = 1843200;
}

/*
@@ -163,6 +152,7 @@ static inline void msm_serial_set_mnd_regs_tcxoby4(struct uart_port *port)
msm_write(port, 0xF6, UART_NREG);
msm_write(port, 0x0F, UART_DREG);
msm_write(port, 0x0A, UART_MNDREG);
+ port->uartclk = 1843200;
}

static inline
@@ -170,7 +160,7 @@ void msm_serial_set_mnd_regs_from_uartclk(struct uart_port *port)
{
if (port->uartclk == 19200000)
msm_serial_set_mnd_regs_tcxo(port);
- else
+ else if (port->uartclk == 4800000)
msm_serial_set_mnd_regs_tcxoby4(port);
}

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 18:39:04

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 2/4] msm_serial: Fix sparse warnings

drivers/tty/serial/msm_serial.c:302:6: warning: symbol 'msm_set_mctrl' was
not declared. Should it be static?
drivers/tty/serial/msm_serial.c:597:17: warning: incorrect type in argument 2
(different address spaces)
drivers/tty/serial/msm_serial.c:597:17: expected void volatile [noderef] <asn:2>*addr
drivers/tty/serial/msm_serial.c:597:17: got unsigned int *
drivers/tty/serial/msm_serial.c:608:33: warning: incorrect type in argument 1
(different address spaces)
drivers/tty/serial/msm_serial.c:608:33: expected void volatile [noderef] <asn:2>*addr
drivers/tty/serial/msm_serial.c:608:33: got unsigned int *gsbi_base
drivers/tty/serial/msm_serial.c:648:37: warning: incorrect type in assignment
(different address spaces)
drivers/tty/serial/msm_serial.c:648:37: expected unsigned int *gsbi_base
drivers/tty/serial/msm_serial.c:648:37: got void [noderef] <asn:2>*

Mark the ioremapped memory as __iomem and use writel instead of
iowrite because we're not dealing with PCI devices. Also, mark
msm_set_mctrl() static because it isn't used outside this file.

Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index fa3fc67..cab4105 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -45,7 +45,7 @@ struct msm_port {
struct clk *clk;
struct clk *pclk;
unsigned int imr;
- unsigned int *gsbi_base;
+ void __iomem *gsbi_base;
int is_uartdm;
unsigned int old_snap_state;
};
@@ -299,7 +299,7 @@ static void msm_reset(struct uart_port *port)
msm_write(port, UART_CR_CMD_SET_RFR, UART_CR);
}

-void msm_set_mctrl(struct uart_port *port, unsigned int mctrl)
+static void msm_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
unsigned int mr;
mr = msm_read(port, UART_MR1);
@@ -593,12 +593,10 @@ static void msm_release_port(struct uart_port *port)
port->membase = NULL;

if (msm_port->gsbi_base) {
- iowrite32(GSBI_PROTOCOL_IDLE, msm_port->gsbi_base +
- GSBI_CONTROL);
-
- gsbi_resource = platform_get_resource(pdev,
- IORESOURCE_MEM, 1);
+ writel_relaxed(GSBI_PROTOCOL_IDLE,
+ msm_port->gsbi_base + GSBI_CONTROL);

+ gsbi_resource = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (unlikely(!gsbi_resource))
return;

@@ -670,10 +668,9 @@ static void msm_config_port(struct uart_port *port, int flags)
if (ret)
return;
}
-
if (msm_port->is_uartdm)
- iowrite32(GSBI_PROTOCOL_UART, msm_port->gsbi_base +
- GSBI_CONTROL);
+ writel_relaxed(GSBI_PROTOCOL_UART,
+ msm_port->gsbi_base + GSBI_CONTROL);
}

static int msm_verify_port(struct uart_port *port, struct serial_struct *ser)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 20:29:48

by David Brown

[permalink] [raw]
Subject: Re: [PATCH 1/4] msm_serial: Fix NUL byte output on UARTDM

On Wed, Jul 24, 2013 at 11:37:28AM -0700, Stephen Boyd wrote:
>UARTDM serial devices require us to wait for the entire TX fifo
>to drain before we can change the contents of the NCF_TX
>register. Furthermore, if we write any characters to the TX fifo
>within the same clock cycle of changing the NCF_TX register the
>NCF_TX register won't latch properly.
>
>To fix these issues we should read back the NCF_TX register to
>delay any TX fifo accesses by a clock cycle and we should wait
>for the TX fifo to drain (instead of just waiting for the fifo to
>be ready to receive more characters). Failure to do so leads to
>random NUL bytes interspersed in the output.
>
>Signed-off-by: Stephen Boyd <[email protected]>
>---
> drivers/tty/serial/msm_serial.c | 14 +++++++++-----
> drivers/tty/serial/msm_serial.h | 1 +
> 2 files changed, 10 insertions(+), 5 deletions(-)

Acked-by: David Brown <[email protected]>

--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 20:30:10

by David Brown

[permalink] [raw]
Subject: Re: [PATCH 2/4] msm_serial: Fix sparse warnings

On Wed, Jul 24, 2013 at 11:37:29AM -0700, Stephen Boyd wrote:
>drivers/tty/serial/msm_serial.c:302:6: warning: symbol 'msm_set_mctrl' was
>not declared. Should it be static?
>drivers/tty/serial/msm_serial.c:597:17: warning: incorrect type in argument 2
>(different address spaces)
>drivers/tty/serial/msm_serial.c:597:17: expected void volatile [noderef] <asn:2>*addr
>drivers/tty/serial/msm_serial.c:597:17: got unsigned int *
>drivers/tty/serial/msm_serial.c:608:33: warning: incorrect type in argument 1
>(different address spaces)
>drivers/tty/serial/msm_serial.c:608:33: expected void volatile [noderef] <asn:2>*addr
>drivers/tty/serial/msm_serial.c:608:33: got unsigned int *gsbi_base
>drivers/tty/serial/msm_serial.c:648:37: warning: incorrect type in assignment
>(different address spaces)
>drivers/tty/serial/msm_serial.c:648:37: expected unsigned int *gsbi_base
>drivers/tty/serial/msm_serial.c:648:37: got void [noderef] <asn:2>*
>
>Mark the ioremapped memory as __iomem and use writel instead of
>iowrite because we're not dealing with PCI devices. Also, mark
>msm_set_mctrl() static because it isn't used outside this file.
>
>Signed-off-by: Stephen Boyd <[email protected]>
>---
> drivers/tty/serial/msm_serial.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)

Acked-by: David Brown <[email protected]>

--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 20:30:33

by David Brown

[permalink] [raw]
Subject: Re: [PATCH 3/4] msm_serial: Make baud_code detection more dynamic

On Wed, Jul 24, 2013 at 11:37:30AM -0700, Stephen Boyd wrote:
>Currently msm_set_baud_rate() assumes the uart clock rate is
>1.8432 MHz. This is not always true, and limits our options to
>program the baud rate. Instead of assuming the rate and
>hard-coding the baud_code based on it, calculate the divider that
>we want and try to find the closest baud_code that matches. This
>allows us to support uarts with faster clock speeds.
>
>Signed-off-by: Stephen Boyd <[email protected]>
>---
> drivers/tty/serial/msm_serial.c | 98 ++++++++++++++++++-----------------------
> drivers/tty/serial/msm_serial.h | 18 ++------
> 2 files changed, 48 insertions(+), 68 deletions(-)

Acked-by: David Brown <[email protected]>

--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 20:31:11

by David Brown

[permalink] [raw]
Subject: Re: [PATCH 4/4] msm_serial: Send more than 1 character at a time on UARTDM

On Wed, Jul 24, 2013 at 11:37:31AM -0700, Stephen Boyd wrote:
>UARTDM cores have a TX fifo that can accept more than one
>character per register write, but the msm_serial driver currently
>only supports 1 character mode. Add support for this mode of operation
>to speed up the transmit path on DM devices.
>
>Signed-off-by: Stephen Boyd <[email protected]>
>---
> drivers/tty/serial/msm_serial.c | 51 ++++++++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 19 deletions(-)

Acked-by: David Brown <[email protected]>

--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-24 20:31:55

by David Brown

[permalink] [raw]
Subject: Re: [PATCH 0/4] msm_serial fixes and improvements

On Wed, Jul 24, 2013 at 11:37:27AM -0700, Stephen Boyd wrote:
>Here are a couple fixes and improvements to the msm_serial
>driver. The first two patches fix a bug on UARTDM devices and
>silence sparse warnings. The last two patches improve the baud
>code detection and increase the throughput on UARTDM devices.
>
>After this it is rather trivial to add support for newer UARTDM
>devices via new compatible fields.
>
>Stephen Boyd (4):
> msm_serial: Fix NUL byte output on UARTDM
> msm_serial: Fix sparse warnings
> msm_serial: Make baud_code detection more dynamic
> msm_serial: Send more than 1 character at a time on UARTDM
>
> drivers/tty/serial/msm_serial.c | 180 ++++++++++++++++++++--------------------
> drivers/tty/serial/msm_serial.h | 19 ++---
> 2 files changed, 97 insertions(+), 102 deletions(-)

Thanks for these patches. I'm glad to finally see these issues fixed.

David

--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-07-26 04:30:53

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 3/4] msm_serial: Make baud_code detection more dynamic

On Wed, Jul 24, 2013 at 10:37 AM, Stephen Boyd <[email protected]> wrote:
> [snip]
> + unsigned int i, divisor;
> + const struct msm_baud_map *entry;
> + static const struct msm_baud_map table[] = {
> + { 1536, 0x00, 1 },
> + { 768, 0x11, 1 },
> + { 384, 0x22, 1 },
> + { 192, 0x33, 1 },
> + { 96, 0x44, 1 },
> + { 48, 0x55, 1 },
> + { 32, 0x66, 1 },
> + { 24, 0x77, 1 },
> + { 16, 0x88, 1 },
> + { 12, 0x99, 6 },
> + { 8, 0xaa, 6 },
> + { 6, 0xbb, 6 },
> + { 4, 0xcc, 6 },
> + { 3, 0xdd, 8 },
> + { 2, 0xee, 16 },
> + { 1, 0xff, 31 },
> + };
> +
> + divisor = uart_get_divisor(port, baud);
> +
> + for (i = 0, entry = table; i < ARRAY_SIZE(table); i++, entry++)
> + if (entry->divisor <= divisor)
> + break;
> +
> + return entry; /* Default to smallest divider */

Shouldn't matter, but you're not defaulting to the smallest divider.
Your are defaulting to an undefined value, as `entry` will be off the
array once i == ARRAY_SIZE().

Regards,
Bjorn

2013-07-26 17:05:28

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 3/4] msm_serial: Make baud_code detection more dynamic

On 07/25, Bjorn Andersson wrote:
> On Wed, Jul 24, 2013 at 10:37 AM, Stephen Boyd <[email protected]> wrote:
> > [snip]
> > + unsigned int i, divisor;
> > + const struct msm_baud_map *entry;
> > + static const struct msm_baud_map table[] = {
> > + { 1536, 0x00, 1 },
> > + { 768, 0x11, 1 },
> > + { 384, 0x22, 1 },
> > + { 192, 0x33, 1 },
> > + { 96, 0x44, 1 },
> > + { 48, 0x55, 1 },
> > + { 32, 0x66, 1 },
> > + { 24, 0x77, 1 },
> > + { 16, 0x88, 1 },
> > + { 12, 0x99, 6 },
> > + { 8, 0xaa, 6 },
> > + { 6, 0xbb, 6 },
> > + { 4, 0xcc, 6 },
> > + { 3, 0xdd, 8 },
> > + { 2, 0xee, 16 },
> > + { 1, 0xff, 31 },
> > + };
> > +
> > + divisor = uart_get_divisor(port, baud);
> > +
> > + for (i = 0, entry = table; i < ARRAY_SIZE(table); i++, entry++)
> > + if (entry->divisor <= divisor)
> > + break;
> > +
> > + return entry; /* Default to smallest divider */
>
> Shouldn't matter, but you're not defaulting to the smallest divider.
> Your are defaulting to an undefined value, as `entry` will be off the
> array once i == ARRAY_SIZE().
>

Yes because the if condition will always be true. Perhaps that's
too confusing? We could add a subtraction by 1 to make it more
obvious.

for (i = 0; entry = table; i < ARRAY_SIZE(table) - 1; i++, entry++)
if (entry->divisor <= divisor)
break;

return entry; /* Default to smallest divider*/

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2013-07-29 09:32:39

by Ivan T. Ivanov

[permalink] [raw]
Subject: Re: [PATCH 0/4] msm_serial fixes and improvements


Hi,

On Wed, 2013-07-24 at 11:37 -0700, Stephen Boyd wrote:
> Here are a couple fixes and improvements to the msm_serial
> driver. The first two patches fix a bug on UARTDM devices and
> silence sparse warnings. The last two patches improve the baud
> code detection and increase the throughput on UARTDM devices.
>
> After this it is rather trivial to add support for newer UARTDM
> devices via new compatible fields.
>
> Stephen Boyd (4):
> msm_serial: Fix NUL byte output on UARTDM
> msm_serial: Fix sparse warnings
> msm_serial: Make baud_code detection more dynamic
> msm_serial: Send more than 1 character at a time on UARTDM
>

Thanks, It is working on 8074 based DragonBoard.

Tested-by: Ivan T. Ivanov <[email protected]>


> drivers/tty/serial/msm_serial.c | 180 ++++++++++++++++++++--------------------
> drivers/tty/serial/msm_serial.h | 19 ++---
> 2 files changed, 97 insertions(+), 102 deletions(-)
>