Skip to contentSkip to conversation

PL01x: baudrate & line control fixes

PATCH New Vikas Manocha 2 participants 17 messages Nov 18, 2014 – Nov 25, 2014

lore
Share & tools
lore

Vikas Manocha

author
#0 lore raw

cover PL01x: baudrate & line control fixes

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

 drivers/serial/serial_pl01x.c |   46 +++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 22 deletions(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 1/5 serial: pl01x: pass pl01x_type to set baudrate

Although we were checking the pl01x type, seems like PL010 type was being passed by mistake.

Signed-off-by Vikas Manocha <[email protected]>
---
 drivers/serial/serial_pl01x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/serial/serial_pl01x.c +1 -1
index 38dda91..1860289 100644
@@ -201,7 +201,7 @@ static void pl01x_serial_init_baud(int baudrate)
 	base_regs = (struct pl01x_regs *)port[CONFIG_CONS_INDEX];
 
 	pl01x_generic_serial_init(base_regs, pl01x_type);
-	pl01x_generic_setbrg(base_regs, TYPE_PL010, clock, baudrate);
+	pl01x_generic_setbrg(base_regs, pl01x_type, clock, baudrate);
 }
 
 /*
Signature · 1 line
1.7.9.5
author
#3 lore raw

PATCH 2/5 serial: pl01x: fix pl011 baud rate configuration

In reply to Vikas Manocha

UART_IBRD, UART_FBRD, and UART_LCR_H form a single 30-bit wide register which is updated on a single write strobe generated by a UART_LCR_H write. So, to internally update the content of UART_IBRD or UART_FBRD, a write to UART_LCR_H must always be performed at the end.

Signed-off-by Vikas Manocha <[email protected]>
---
 drivers/serial/serial_pl01x.c |    6 ++++++
 1 file changed, 6 insertions(+)
drivers/serial/serial_pl01x.c +6 -0
index 1860289..c0531ca 100644
@@ -122,6 +122,7 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
 				int clock, int baudrate)
 {
+	unsigned int lcr;
 	switch (type) {
 	case TYPE_PL010: {
 		unsigned int divisor;

@@ -175,6 +176,11 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
 		writel(divider, &regs->pl011_ibrd);
 		writel(fraction, &regs->pl011_fbrd);
 
+		/* Internal update of baud rate register require line
+		 * control register write */
+		lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
+		writel(lcr, &regs->pl011_lcrh);
+
 		/* Finally, enable the UART */
 		writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE |
 		       UART_PL011_CR_RXE | UART_PL011_CR_RTS, &regs->pl011_cr);
Signature · 1 line
1.7.9.5
#4 lore raw

PATCH 2/5 serial: pl01x: fix pl011 baud rate configuration

Quoted 6 lines · Vikas Manocha

UART_IBRD, UART_FBRD, and UART_LCR_H form a single 30-bit wide register which is updated on a single write strobe generated by a UART_LCR_H write. So, to internally update the content of UART_IBRD or UART_FBRD, a write to UART_LCR_H must always be performed at the end.

Signed-off-by Vikas Manocha <[email protected]>
Acked-by Simon Glass <[email protected]>

but see below.

Quoted 17 lines
---
 drivers/serial/serial_pl01x.c |    6 ++++++
 1 file changed, 6 insertions(+)
drivers/serial/serial_pl01x.c +1 -0
index 1860289..c0531ca 100644
@@ -122,6 +122,7 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
                                int clock, int baudrate)
 {
+       unsigned int lcr;
        switch (type) {
        case TYPE_PL010: {
                unsigned int divisor;

drivers/serial/serial_pl01x.c static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type, · line 180

Quoted 5 lines
drivers/serial/serial_pl01x.c +2 -0
@@ -175,6 +176,11 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
                writel(divider, &regs->pl011_ibrd);
                writel(fraction, &regs->pl011_fbrd);
 
+               /* Internal update of baud rate register require line
+                * control register write */

You may as well fix the comment style at the same time:

/*
* Internal update...
* control ...
*/

drivers/serial/serial_pl01x.c static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,

Quoted 7 lines
drivers/serial/serial_pl01x.c +3 -1
+               lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
+               writel(lcr, &regs->pl011_lcrh);
+
                /* Finally, enable the UART */
                writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE |
                       UART_PL011_CR_RXE | UART_PL011_CR_RTS, &regs->pl011_cr);
--

Regards,
Simon

author
#5 lore raw

PATCH 3/5 serial: pl01x: move all line control at same place

In reply to Vikas Manocha

Receive line control uses same setting as transmit line control, also one lcrh write is effective for both baud rate & receive line control internal update.

Signed-off-by Vikas Manocha <[email protected]>
---
 drivers/serial/serial_pl01x.c |   40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)
drivers/serial/serial_pl01x.c +19 -21
index c0531ca..3155840 100644
@@ -72,8 +72,6 @@ static int pl01x_tstc(struct pl01x_regs *regs)
 static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 				     enum pl01x_type type)
 {
-	unsigned int lcr;
-
 #ifdef CONFIG_PL011_SERIAL_FLUSH_ON_INIT
 	if (type == TYPE_PL011) {
 		/* Empty RX fifo if necessary */

@@ -87,15 +85,26 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 	/* First, disable everything */
 	writel(0, &regs->pl010_cr);
 
-	/* Set the UART to be 8 bits, 1 stop bit, no parity, fifo enabled */
-	lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
-	writel(lcr, &regs->pl011_lcrh);
-
 	switch (type) {
 	case TYPE_PL010:
 		break;
-	case TYPE_PL011: {
+	case TYPE_PL011:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int set_line_control(struct pl01x_regs *regs)
+{
+	unsigned int lcr;
+	/* Internal update of baud rate register require line
+	 * control register write */
+	lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
 #ifdef CONFIG_PL011_SERIAL_RLCR
+	{
 		int i;
 
 		/*

@@ -107,22 +116,15 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 			writel(lcr, &regs->fr);
 
 		writel(lcr, &regs->pl011_rlcr);
-		/* lcrh needs to be set again for change to be effective */
-		writel(lcr, &regs->pl011_lcrh);
-#endif
-		break;
 	}
-	default:
-		return -EINVAL;
-	}
-
+#endif
+	writel(lcr, &regs->pl011_lcrh);
 	return 0;
 }
 
 static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
 				int clock, int baudrate)
 {
-	unsigned int lcr;
 	switch (type) {
 	case TYPE_PL010: {
 		unsigned int divisor;

@@ -176,11 +178,7 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
 		writel(divider, &regs->pl011_ibrd);
 		writel(fraction, &regs->pl011_fbrd);
 
-		/* Internal update of baud rate register require line
-		 * control register write */
-		lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
-		writel(lcr, &regs->pl011_lcrh);
-
+		set_line_control(regs);
 		/* Finally, enable the UART */
 		writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE |
 		       UART_PL011_CR_RXE | UART_PL011_CR_RTS, &regs->pl011_cr);
Signature · 1 line
1.7.9.5
author
#7 lore raw

PATCH 4/5 serial: pl01x: disable as per type of pl01x

In reply to Vikas Manocha

pl010 & pl011 have different control register offsets, setting it as per the pl01x type.

Signed-off-by Vikas Manocha <[email protected]>
---
 drivers/serial/serial_pl01x.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/serial/serial_pl01x.c +4 -3
index 3155840..758684f 100644
@@ -82,13 +82,14 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 	}
 #endif
 
-	/* First, disable everything */
-	writel(0, &regs->pl010_cr);
-
 	switch (type) {
 	case TYPE_PL010:
+		/* disable everything */
+		writel(0, &regs->pl010_cr);
 		break;
 	case TYPE_PL011:
+		/* disable everything */
+		writel(0, &regs->pl011_cr);
 		break;
 	default:
 		return -EINVAL;
Signature · 1 line
1.7.9.5
author
#9 lore raw

PATCH 5/5 serial: pl01x: avoid pl01x type check two times

In reply to Vikas Manocha
Signed-off-by Vikas Manocha <[email protected]>
---
 drivers/serial/serial_pl01x.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/serial/serial_pl01x.c +6 -9
index 758684f..3fc1db5 100644
@@ -72,22 +72,19 @@ static int pl01x_tstc(struct pl01x_regs *regs)
 static int pl01x_generic_serial_init(struct pl01x_regs *regs,
 				     enum pl01x_type type)
 {
+	switch (type) {
+	case TYPE_PL010:
+		/* disable everything */
+		writel(0, &regs->pl010_cr);
+		break;
+	case TYPE_PL011:
 #ifdef CONFIG_PL011_SERIAL_FLUSH_ON_INIT
-	if (type == TYPE_PL011) {
 		/* Empty RX fifo if necessary */
 		if (readl(&regs->pl011_cr) & UART_PL011_CR_UARTEN) {
 			while (!(readl(&regs->fr) & UART_PL01x_FR_RXFE))
 				readl(&regs->dr);
 		}
-	}
 #endif
-
-	switch (type) {
-	case TYPE_PL010:
-		/* disable everything */
-		writel(0, &regs->pl010_cr);
-		break;
-	case TYPE_PL011:
 		/* disable everything */
 		writel(0, &regs->pl011_cr);
 		break;
Signature · 1 line
1.7.9.5
#11 lore raw

cover PL01x: baudrate & line control fixes

In reply to Vikas Manocha

Hi Vikas,

Quoted 10 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

Quoted 2 lines
 drivers/serial/serial_pl01x.c |   46 +++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

Regards,
Simon

author
#12 lore raw

cover PL01x: baudrate & line control fixes

Thanks Simon,

Quoted 13 lines · Simon Glass

Hi Vikas,

Quoted 10 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

Quoted 6 lines
Quoted 2 lines
  drivers/serial/serial_pl01x.c |   46 +++++++++++++++++++++--------------------
  1 file changed, 24 insertions(+), 22 deletions(-)

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

I was able to test it only for pl011, don't have any board with pl010.

Quoted 3 lines

Regards,
Simon

Rgds,
Vikas

#13 lore raw

cover PL01x: baudrate & line control fixes

Hi Vikas,

Quoted 23 lines · vikasm

Thanks Simon,

Quoted 19 lines · Simon Glass

Hi Vikas,

Quoted 14 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

  drivers/serial/serial_pl01x.c |   46
+++++++++++++++++++++--------------------
  1 file changed, 24 insertions(+), 22 deletions(-)
Quoted 7 lines
Quoted 4 lines

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

I was able to test it only for pl011, don't have any board with pl010.

Sadly I was wrong, I only have pl011 also. It makes me wonder how it worked so nicely for me without your patches.

Regards,
Simon

author
#14 lore raw

cover PL01x: baudrate & line control fixes

Hi Simon,

Quoted 23 lines · Simon Glass

Hi Vikas,

Quoted 20 lines · vikasm

Thanks Simon,

Quoted 16 lines · Simon Glass

Hi Vikas,

Quoted 13 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

   drivers/serial/serial_pl01x.c |   46
+++++++++++++++++++++--------------------
   1 file changed, 24 insertions(+), 22 deletions(-)
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

I was able to test it only for pl011, don't have any board with pl010.

Sadly I was wrong, I only have pl011 also. It makes me wonder how it worked so nicely for me without your patches.

Please find below the possible explanation for all patches:

serial: pl01x: pass pl01x_type to set baudrate
- This patch only affect if DM is not defined.
If you are using driver model for pl011, pl011 will work without this patch.

serial: pl01x: fix pl011 baud rate configuration
- If pl011 is already configured to correct baudrate or LCR register is being written later.

serial: pl01x: move all line control at same place
- this patch does not affect the functionality.

serial: pl01x: disable as per type of pl01x
- pl011 might work without disabling it properly at start. pl010 control register(disable bit) location is reserved in case of pl011.

serial: pl01x: avoid pl01x type check two times
- does not affect the functionality.

Quoted 3 lines

Regards,
Simon

#15 lore raw

cover PL01x: baudrate & line control fixes

Hi Vikas,

Quoted 56 lines · vikasm

Hi Simon,

Quoted 34 lines · Simon Glass

Hi Vikas,

Quoted 30 lines · vikasm

Thanks Simon,

Quoted 23 lines · Simon Glass

Hi Vikas,

Quoted 11 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

Quoted 3 lines
   drivers/serial/serial_pl01x.c |   46
+++++++++++++++++++++--------------------
   1 file changed, 24 insertions(+), 22 deletions(-)

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

Quoted 4 lines
Quoted 1 line

I was able to test it only for pl011, don't have any board with pl010.

Sadly I was wrong, I only have pl011 also. It makes me wonder how it worked so nicely for me without your patches.

Please find below the possible explanation for all patches:

serial: pl01x: pass pl01x_type to set baudrate
- This patch only affect if DM is not defined.
If you are using driver model for pl011, pl011 will work without this patch.

serial: pl01x: fix pl011 baud rate configuration
- If pl011 is already configured to correct baudrate or LCR register is being written later.

serial: pl01x: move all line control at same place
- this patch does not affect the functionality.

Quoted 6 lines

serial: pl01x: disable as per type of pl01x
- pl011 might work without disabling it properly at start. pl010 control register(disable bit) location is reserved in case of pl011.

serial: pl01x: avoid pl01x type check two times
- does not affect the functionality.

I didn't mean for you to go to that much trouble, but thank you.

Raspberry Pi is running without driver model (I tested it both ways) so I guess I'm just lucky (or actually unlucky since I might have noticed these problems).

When Stephen ACKs my Raspberry Pi driver rmodel series then I'll pull this series in to DM.

Regards,
Simon

author
#16 lore raw

cover PL01x: baudrate & line control fixes

Hello Simon,

Quoted 51 lines · Simon Glass

Hi Vikas,

Quoted 48 lines · vikasm

Hi Simon,

Quoted 23 lines · Simon Glass

Hi Vikas,

Quoted 20 lines · vikasm

Thanks Simon,

Quoted 16 lines · Simon Glass

Hi Vikas,

Quoted 13 lines · Vikas Manocha

This patchset fixes the pl01x driver esp for pl011 baudrate & line control.

Vikas Manocha (5):
serial: pl01x: pass pl01x_type to set baudrate
serial: pl01x: fix pl011 baud rate configuration
serial: pl01x: move all line control at same place
serial: pl01x: disable as per type of pl01x
serial: pl01x: avoid pl01x type check two times

drivers/serial/serial_pl01x.c | 46
+++++++++++++++++++++--------------------

    1 file changed, 24 insertions(+), 22 deletions(-)
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Great to see this series. If you have not tested on p1010 I can do this in a week or so (am travelling). At least a few of these looks like bug fixes to my recent refactor so I could bring them through the DM tree if no one else picks them up.

I was able to test it only for pl011, don't have any board with pl010.

Sadly I was wrong, I only have pl011 also. It makes me wonder how it worked so nicely for me without your patches.

Please find below the possible explanation for all patches:

serial: pl01x: pass pl01x_type to set baudrate
- This patch only affect if DM is not defined.
If you are using driver model for pl011, pl011 will work without this patch.

serial: pl01x: fix pl011 baud rate configuration
- If pl011 is already configured to correct baudrate or LCR register is being written later.

serial: pl01x: move all line control at same place
- this patch does not affect the functionality.

Quoted 7 lines
Quoted 6 lines

serial: pl01x: disable as per type of pl01x
- pl011 might work without disabling it properly at start. pl010 control register(disable bit) location is reserved in case of pl011.

serial: pl01x: avoid pl01x type check two times
- does not affect the functionality.

I didn't mean for you to go to that much trouble, but thank you.

Not a problem, i was also curious to know it.

Quoted 4 lines

Raspberry Pi is running without driver model (I tested it both ways) so I guess I'm just lucky (or actually unlucky since I might have noticed these problems).

just guessing... (don't want to bother you....don't reply if is not the case)
could it be possible uart is already initialized before u-boot like in xloader if it exists for Raspberry....

Quoted 3 lines

When Stephen ACKs my Raspberry Pi driver rmodel series then I'll pull this series in to DM.

ok.

Quoted 3 lines

Regards,
Simon

Regards,
Vikas