subject:"\[U\-Boot\] \[PATCH 3\/7\] S3C24XX\: Add serial driver"

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

2012-09-13 Thread Marek Vasut

Dear José Miguel Gonçalves,

 Hi Marek,
 
 On 09/12/2012 10:01 PM, Marek Vasut wrote:
  Dear José Miguel Gonçalves,
  
  Serial driver for the S3C24XX SoCs.
  
  Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
  ---
  
drivers/serial/Makefile |1 +
drivers/serial/s3c24xx_serial.c |  146
  
  +++ 2 files changed, 147
  insertions(+)
  
create mode 100644 drivers/serial/s3c24xx_serial.c
  
  diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
  index 65d0f23..2cbdaac 100644
  --- a/drivers/serial/Makefile
  +++ b/drivers/serial/Makefile
  @@ -52,6 +52,7 @@ COBJS-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
  
COBJS-$(CONFIG_PXA_SERIAL) += serial_pxa.o
COBJS-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o
  
  +COBJS-$(CONFIG_S3C24XX_SERIAL) += s3c24xx_serial.o
  
  What's the difference between those two drivers ?!
 
 No substantial differences exists. The UART controller block is the same
 in all S3C24XX chips. One difference is the number of UARTs. The more
 recent chips (S3C2416  S3C2450) have 4 instead of the 3 found on the
 old ones. Besides that, the driver that I submitted uses a more precise
 method for baudrate generation.

So we will have two drivers for the same hardware? No way ... Use the original 
one and apply incremental patches onto it to improve it.

  +
  +#ifdef CONFIG_SERIAL0
  +#define UART_NR   S3C24XX_UART0
  +
  +#elif defined(CONFIG_SERIAL1)
  +#define UART_NR   S3C24XX_UART1
  +
  +#elif defined(CONFIG_SERIAL2)
  +#define UART_NR   S3C24XX_UART2
  +
  +#elif defined(CONFIG_SERIAL3)
  +#define UART_NR   S3C24XX_UART3
  +
  +#else
  +#error Bad: you didn't configure serial ...
  
  Error itself is Bad: so remove it
 
 OK.
 
  +#endif
  +
  +#define barrier() asm volatile( ::: memory)
  
  Is that even used ?
 
 Yes. Without it the GCC optimization removes the loop at the end of the
 baurate generation routine.

So it's yet another accessor issue.
[...]

But anyway, there's more. I'd like to teach you how to do things properly. So 
let's focus on the in-tree driver and fix that one. Incrementally and in small 
steps.
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

2012-09-13 Thread José Miguel Gonçalves


Hi Marek,

On 09/13/2012 10:17 AM, Marek Vasut wrote:

Dear José Miguel Gonçalves,


Hi Marek,

On 09/12/2012 10:01 PM, Marek Vasut wrote:

Dear José Miguel Gonçalves,


Serial driver for the S3C24XX SoCs.

Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
---

   drivers/serial/Makefile |1 +
   drivers/serial/s3c24xx_serial.c |  146

+++ 2 files changed, 147
insertions(+)

   create mode 100644 drivers/serial/s3c24xx_serial.c

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 65d0f23..2cbdaac 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -52,6 +52,7 @@ COBJS-$(CONFIG_PL011_SERIAL) += serial_pl01x.o

   COBJS-$(CONFIG_PXA_SERIAL) += serial_pxa.o
   COBJS-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
   COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o

+COBJS-$(CONFIG_S3C24XX_SERIAL) += s3c24xx_serial.o

What's the difference between those two drivers ?!

No substantial differences exists. The UART controller block is the same
in all S3C24XX chips. One difference is the number of UARTs. The more
recent chips (S3C2416  S3C2450) have 4 instead of the 3 found on the
old ones. Besides that, the driver that I submitted uses a more precise
method for baudrate generation.

So we will have two drivers for the same hardware? No way ... Use the original
one and apply incremental patches onto it to improve it.


+
+#ifdef CONFIG_SERIAL0
+#define UART_NRS3C24XX_UART0
+
+#elif defined(CONFIG_SERIAL1)
+#define UART_NRS3C24XX_UART1
+
+#elif defined(CONFIG_SERIAL2)
+#define UART_NRS3C24XX_UART2
+
+#elif defined(CONFIG_SERIAL3)
+#define UART_NRS3C24XX_UART3
+
+#else
+#error Bad: you didn't configure serial ...

Error itself is Bad: so remove it

OK.


+#endif
+
+#define barrier() asm volatile( ::: memory)

Is that even used ?

Yes. Without it the GCC optimization removes the loop at the end of the
baurate generation routine.

So it's yet another accessor issue.
[...]

But anyway, there's more. I'd like to teach you how to do things properly. So
let's focus on the in-tree driver and fix that one. Incrementally and in small
steps.


OK, I will figure out the best way to do this. If I have any doubts I'll 
be back to you...


Best regards,
José Gonçalves
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

2012-09-12 Thread José Miguel Gonçalves

Serial driver for the S3C24XX SoCs.

Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
---
 drivers/serial/Makefile |1 +
 drivers/serial/s3c24xx_serial.c |  146 +++
 2 files changed, 147 insertions(+)
 create mode 100644 drivers/serial/s3c24xx_serial.c

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 65d0f23..2cbdaac 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -52,6 +52,7 @@ COBJS-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
 COBJS-$(CONFIG_PXA_SERIAL) += serial_pxa.o
 COBJS-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
 COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o
+COBJS-$(CONFIG_S3C24XX_SERIAL) += s3c24xx_serial.o
 COBJS-$(CONFIG_S3C44B0_SERIAL) += serial_s3c44b0.o
 COBJS-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
 COBJS-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
diff --git a/drivers/serial/s3c24xx_serial.c b/drivers/serial/s3c24xx_serial.c
new file mode 100644
index 000..11f13a5
--- /dev/null
+++ b/drivers/serial/s3c24xx_serial.c
@@ -0,0 +1,146 @@
+/*
+ * (C) Copyright 2012 INOV - INESC Inovacao
+ * Jose Goncalves jose.goncal...@inov.pt
+ *
+ * Based on drivers/serial/s3c64xx.c
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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 the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include common.h
+#include asm/arch/s3c24xx_cpu.h
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#ifdef CONFIG_SERIAL0
+#define UART_NRS3C24XX_UART0
+
+#elif defined(CONFIG_SERIAL1)
+#define UART_NRS3C24XX_UART1
+
+#elif defined(CONFIG_SERIAL2)
+#define UART_NRS3C24XX_UART2
+
+#elif defined(CONFIG_SERIAL3)
+#define UART_NRS3C24XX_UART3
+
+#else
+#error Bad: you didn't configure serial ...
+#endif
+
+#define barrier() asm volatile( ::: memory)
+
+/*
+ * The coefficient, used to calculate the baudrate on S3C24XX UARTs is
+ * calculated as C = UBRDIV * 16 + number_of_set_bits_in_UDIVSLOT
+ * however, section 2.1.10 of the S3C2416 User's Manual doesn't recommend
+ * using 1 for 1, 3 for 2, ... (2^n - 1) for n, instead, they suggest using
+ * these constants:
+ */
+static const int udivslot[] = {
+   0x, 0x0080, 0x0808, 0x0888, 0x, 0x4924, 0x4A52, 0x54AA,
+   0x, 0xD555, 0xD5D5, 0xDDD5, 0x, 0xDFDD, 0xDFDF, 0xFFDF,
+};
+
+void serial_setbrg(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+   u32 pclk;
+   u32 baudrate;
+   int i;
+
+   pclk = get_PCLK();
+   baudrate = gd-baudrate;
+
+   uart-ubrdiv = (pclk / baudrate / 16) - 1;
+   uart-udivslot = udivslot[(pclk / baudrate) % 16];
+
+   for (i = 0; i  100; i++)
+   barrier();
+}
+
+/*
+ * Initialise the serial port with the given baudrate. The settings
+ * are always 8 data bits, no parity, 1 stop bit, no start bits.
+ */
+int serial_init(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+
+   /* FIFO enable, Tx/Rx FIFO clear */
+   uart-ufcon = 0x07;
+   uart-umcon = 0x00;
+   /* Normal mode, No parity, 1 stop bit, 8 data bits */
+   uart-ulcon = 0x03;
+   /* Polling mode */
+   uart-ucon = 0x005;
+
+   serial_setbrg();
+
+   return 0;
+}
+
+/*
+ * Read a single byte from the serial port.
+ */
+int serial_getc(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+
+   /* Wait for character to arrive */
+   while (!(uart-utrstat  0x1)) ;
+
+   return uart-urxh  0xff;
+}
+
+/*
+ * Output a single byte to the serial port.
+ */
+void serial_putc(const char c)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+
+   /* Wait for room in the TX FIFO */
+   while (!(uart-utrstat  0x2)) ;
+
+   uart-utxh = c;
+
+   /* If \n, also do \r */
+   if (c == '\n')
+   serial_putc('\r');
+}
+
+/*
+ * Test whether a character is in the RX buffer.
+ */
+int serial_tstc(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+
+   return uart-utrstat  0x1;
+}
+
+/*
+ * Output a string to the serial port.
+ */
+void serial_puts(const char *s)
+{
+   while (*s)
+   serial_putc(*s++);
+}
-- 
1.7.9.5

___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

2012-09-12 Thread Marek Vasut

Dear José Miguel Gonçalves,

 Serial driver for the S3C24XX SoCs.
 
 Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
 ---
  drivers/serial/Makefile |1 +
  drivers/serial/s3c24xx_serial.c |  146
 +++ 2 files changed, 147 insertions(+)
  create mode 100644 drivers/serial/s3c24xx_serial.c
 
 diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
 index 65d0f23..2cbdaac 100644
 --- a/drivers/serial/Makefile
 +++ b/drivers/serial/Makefile
 @@ -52,6 +52,7 @@ COBJS-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
  COBJS-$(CONFIG_PXA_SERIAL) += serial_pxa.o
  COBJS-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
  COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o
 +COBJS-$(CONFIG_S3C24XX_SERIAL) += s3c24xx_serial.o

What's the difference between those two drivers ?!

  COBJS-$(CONFIG_S3C44B0_SERIAL) += serial_s3c44b0.o
  COBJS-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
  COBJS-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
 diff --git a/drivers/serial/s3c24xx_serial.c
 b/drivers/serial/s3c24xx_serial.c new file mode 100644
 index 000..11f13a5
 --- /dev/null
 +++ b/drivers/serial/s3c24xx_serial.c
 @@ -0,0 +1,146 @@
 +/*
 + * (C) Copyright 2012 INOV - INESC Inovacao
 + * Jose Goncalves jose.goncal...@inov.pt
 + *
 + * Based on drivers/serial/s3c64xx.c
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * 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 the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include common.h
 +#include asm/arch/s3c24xx_cpu.h
 +
 +DECLARE_GLOBAL_DATA_PTR;
 +
 +#ifdef CONFIG_SERIAL0
 +#define UART_NR  S3C24XX_UART0
 +
 +#elif defined(CONFIG_SERIAL1)
 +#define UART_NR  S3C24XX_UART1
 +
 +#elif defined(CONFIG_SERIAL2)
 +#define UART_NR  S3C24XX_UART2
 +
 +#elif defined(CONFIG_SERIAL3)
 +#define UART_NR  S3C24XX_UART3
 +
 +#else
 +#error Bad: you didn't configure serial ...

Error itself is Bad: so remove it

 +#endif
 +
 +#define barrier() asm volatile( ::: memory)

Is that even used ?

 +/*
 + * The coefficient, used to calculate the baudrate on S3C24XX UARTs is
 + * calculated as C = UBRDIV * 16 + number_of_set_bits_in_UDIVSLOT
 + * however, section 2.1.10 of the S3C2416 User's Manual doesn't recommend
 + * using 1 for 1, 3 for 2, ... (2^n - 1) for n, instead, they suggest
 using + * these constants:
 + */
 +static const int udivslot[] = {

const int const ... const array const members

 + 0x, 0x0080, 0x0808, 0x0888, 0x, 0x4924, 0x4A52, 0x54AA,
 + 0x, 0xD555, 0xD5D5, 0xDDD5, 0x, 0xDFDD, 0xDFDF, 0xFFDF,
 +};
 +
 +void serial_setbrg(void)
 +{
 + s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
 + u32 pclk;
 + u32 baudrate;
 + int i;
 +
 + pclk = get_PCLK();
 + baudrate = gd-baudrate;
 +
 + uart-ubrdiv = (pclk / baudrate / 16) - 1;
 + uart-udivslot = udivslot[(pclk / baudrate) % 16];
 +
 + for (i = 0; i  100; i++)
 + barrier();
 +}
 +
 +/*
 + * Initialise the serial port with the given baudrate. The settings
 + * are always 8 data bits, no parity, 1 stop bit, no start bits.
 + */
 +int serial_init(void)
 +{
 + s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
 +
 + /* FIFO enable, Tx/Rx FIFO clear */
 + uart-ufcon = 0x07;
 + uart-umcon = 0x00;

Magic numbers, fix

 + /* Normal mode, No parity, 1 stop bit, 8 data bits */
 + uart-ulcon = 0x03;
 + /* Polling mode */
 + uart-ucon = 0x005;
 +
 + serial_setbrg();
 +
 + return 0;
 +}
 +
 +/*
 + * Read a single byte from the serial port.
 + */
 +int serial_getc(void)
 +{
 + s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
 +
 + /* Wait for character to arrive */
 + while (!(uart-utrstat  0x1)) ;
 +
 + return uart-urxh  0xff;
 +}
 +
 +/*
 + * Output a single byte to the serial port.
 + */
 +void serial_putc(const char c)
 +{
 + s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
 +
 + /* Wait for room in the TX FIFO */
 + while (!(uart-utrstat  0x2)) ;
 +
 + uart-utxh = c;
 +
 + /* If \n, also do \r */
 + if (c == '\n')
 + serial_putc('\r');
 +}
 +
 +/*
 + * Test whether a character is in the RX buffer.
 + */
 +int serial_tstc(void)
 +{
 + s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
 +
 +

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

2012-09-12 Thread José Miguel Gonçalves


Hi Marek,

On 09/12/2012 10:01 PM, Marek Vasut wrote:

Dear José Miguel Gonçalves,


Serial driver for the S3C24XX SoCs.

Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
---
  drivers/serial/Makefile |1 +
  drivers/serial/s3c24xx_serial.c |  146
+++ 2 files changed, 147 insertions(+)
  create mode 100644 drivers/serial/s3c24xx_serial.c

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 65d0f23..2cbdaac 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -52,6 +52,7 @@ COBJS-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
  COBJS-$(CONFIG_PXA_SERIAL) += serial_pxa.o
  COBJS-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
  COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o
+COBJS-$(CONFIG_S3C24XX_SERIAL) += s3c24xx_serial.o

What's the difference between those two drivers ?!


No substantial differences exists. The UART controller block is the same 
in all S3C24XX chips. One difference is the number of UARTs. The more 
recent chips (S3C2416  S3C2450) have 4 instead of the 3 found on the 
old ones. Besides that, the driver that I submitted uses a more precise 
method for baudrate generation.



+
+#ifdef CONFIG_SERIAL0
+#define UART_NRS3C24XX_UART0
+
+#elif defined(CONFIG_SERIAL1)
+#define UART_NRS3C24XX_UART1
+
+#elif defined(CONFIG_SERIAL2)
+#define UART_NRS3C24XX_UART2
+
+#elif defined(CONFIG_SERIAL3)
+#define UART_NRS3C24XX_UART3
+
+#else
+#error Bad: you didn't configure serial ...

Error itself is Bad: so remove it


OK.




+#endif
+
+#define barrier() asm volatile( ::: memory)

Is that even used ?


Yes. Without it the GCC optimization removes the loop at the end of the 
baurate generation routine.



+/*
+ * The coefficient, used to calculate the baudrate on S3C24XX UARTs is
+ * calculated as C = UBRDIV * 16 + number_of_set_bits_in_UDIVSLOT
+ * however, section 2.1.10 of the S3C2416 User's Manual doesn't recommend
+ * using 1 for 1, 3 for 2, ... (2^n - 1) for n, instead, they suggest
using + * these constants:
+ */
+static const int udivslot[] = {

const int const ... const array const members


It don't see the need for that because you have a constant array when 
their members are constant and;


static const int udivslot[];
static int const udivslot[];

are both (correct) forms to declare an array of constant integers.


+   0x, 0x0080, 0x0808, 0x0888, 0x, 0x4924, 0x4A52, 0x54AA,
+   0x, 0xD555, 0xD5D5, 0xDDD5, 0x, 0xDFDD, 0xDFDF, 0xFFDF,
+};
+
+void serial_setbrg(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+   u32 pclk;
+   u32 baudrate;
+   int i;
+
+   pclk = get_PCLK();
+   baudrate = gd-baudrate;
+
+   uart-ubrdiv = (pclk / baudrate / 16) - 1;
+   uart-udivslot = udivslot[(pclk / baudrate) % 16];
+
+   for (i = 0; i  100; i++)
+   barrier();
+}
+
+/*
+ * Initialise the serial port with the given baudrate. The settings
+ * are always 8 data bits, no parity, 1 stop bit, no start bits.
+ */
+int serial_init(void)
+{
+   s3c24xx_uart *const uart = s3c24xx_get_base_uart(UART_NR);
+
+   /* FIFO enable, Tx/Rx FIFO clear */
+   uart-ufcon = 0x07;
+   uart-umcon = 0x00;

Magic numbers, fix


OK.


Can you implement it as a serial multi (CONFIG_SERIAL_MULTI) right away ?
thanks.



I will look at that option and change the driver to support it.

Best regards,
José Gonçalves
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

[U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

Re: [U-Boot] [PATCH 3/7] S3C24XX: Add serial driver

5 matches

Site Navigation

Mail list logo

Footer information