[U-Boot] [PATCH 0/5] 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(-)
Although we were checking the pl01x type, seems like PL010 type was being passed by mistake.
Signed-off-by: Vikas Manocha vikas.manocha@st.com --- drivers/serial/serial_pl01x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 38dda91..1860289 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -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); }
/*
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
Although we were checking the pl01x type, seems like PL010 type was being passed by mistake.
Signed-off-by: Vikas Manocha vikas.manocha@st.com
Acked-by: Simon Glass sjg@chromium.org
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 vikas.manocha@st.com --- drivers/serial/serial_pl01x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 1860289..c0531ca 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -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, ®s->pl011_ibrd); writel(fraction, ®s->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, ®s->pl011_lcrh); + /* Finally, enable the UART */ writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr);
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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 vikas.manocha@st.com
Acked-by: Simon Glass sjg@chromium.org
but see below.
drivers/serial/serial_pl01x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 1860289..c0531ca 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -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, ®s->pl011_ibrd); writel(fraction, ®s->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 ... */
lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;writel(lcr, ®s->pl011_lcrh);/* Finally, enable the UART */ writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr);--
Regards, Simon
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 vikas.manocha@st.com --- drivers/serial/serial_pl01x.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index c0531ca..3155840 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -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, ®s->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, ®s->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, ®s->fr);
writel(lcr, ®s->pl011_rlcr); - /* lcrh needs to be set again for change to be effective */ - writel(lcr, ®s->pl011_lcrh); -#endif - break; } - default: - return -EINVAL; - } - +#endif + writel(lcr, ®s->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, ®s->pl011_ibrd); writel(fraction, ®s->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, ®s->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, ®s->pl011_cr);
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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 vikas.manocha@st.com
Seems cleaner to me.
Acked-by: Simon Glass sjg@chromium.org
pl010 & pl011 have different control register offsets, setting it as per the pl01x type.
Signed-off-by: Vikas Manocha vikas.manocha@st.com --- drivers/serial/serial_pl01x.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 3155840..758684f 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -82,13 +82,14 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs, } #endif
- /* First, disable everything */ - writel(0, ®s->pl010_cr); - switch (type) { case TYPE_PL010: + /* disable everything */ + writel(0, ®s->pl010_cr); break; case TYPE_PL011: + /* disable everything */ + writel(0, ®s->pl011_cr); break; default: return -EINVAL;
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
pl010 & pl011 have different control register offsets, setting it as per the pl01x type.
Signed-off-by: Vikas Manocha vikas.manocha@st.com
Acked-by: Simon Glass sjg@chromium.org
Signed-off-by: Vikas Manocha vikas.manocha@st.com --- drivers/serial/serial_pl01x.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 758684f..3fc1db5 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -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, ®s->pl010_cr); + break; + case TYPE_PL011: #ifdef CONFIG_PL011_SERIAL_FLUSH_ON_INIT - if (type == TYPE_PL011) { /* Empty RX fifo if necessary */ if (readl(®s->pl011_cr) & UART_PL011_CR_UARTEN) { while (!(readl(®s->fr) & UART_PL01x_FR_RXFE)) readl(®s->dr); } - } #endif - - switch (type) { - case TYPE_PL010: - /* disable everything */ - writel(0, ®s->pl010_cr); - break; - case TYPE_PL011: /* disable everything */ writel(0, ®s->pl011_cr); break;
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
Signed-off-by: Vikas Manocha vikas.manocha@st.com
Acked-by: Simon Glass sjg@chromium.org
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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
Thanks Simon,
On 11/17/2014 09:30 PM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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.
Regards, Simon
Rgds, Vikas
Hi Vikas,
On 18 November 2014 at 11:59, vikasm vikas.manocha@st.com wrote:
Thanks Simon,
On 11/17/2014 09:30 PM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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
Hi Simon,
On 11/24/2014 07:51 AM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 at 11:59, vikasm vikas.manocha@st.com wrote:
Thanks Simon,
On 11/17/2014 09:30 PM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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.
Regards, Simon
Hi Vikas,
On 24 November 2014 at 17:40, vikasm vikas.manocha@st.com wrote:
Hi Simon,
On 11/24/2014 07:51 AM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 at 11:59, vikasm vikas.manocha@st.com wrote:
Thanks Simon,
On 11/17/2014 09:30 PM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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.
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
Hello Simon,
On 11/24/2014 07:51 PM, Simon Glass wrote:
Hi Vikas,
On 24 November 2014 at 17:40, vikasm vikas.manocha@st.com wrote:
Hi Simon,
On 11/24/2014 07:51 AM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 at 11:59, vikasm vikas.manocha@st.com wrote:
Thanks Simon,
On 11/17/2014 09:30 PM, Simon Glass wrote:
Hi Vikas,
On 18 November 2014 00:17, Vikas Manocha vikas.manocha@st.com wrote:
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(-)
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.
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.
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....
When Stephen ACKs my Raspberry Pi driver rmodel series then I'll pull this series in to DM.
ok.
Regards, Simon
Regards, Vikas
participants (3)
-
Simon Glass -
Vikas Manocha -
vikasm