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

[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.goncalves at 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_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.

>> +/*
>> + * 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.

>> +	0x0000, 0x0080, 0x0808, 0x0888, 0x2222, 0x4924, 0x4A52, 0x54AA,
>> +	0x5555, 0xD555, 0xD5D5, 0xDDD5, 0xDDDD, 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