svn commit: r245450 - in head/sys: arm/allwinner arm/conf boot/fdt/dts

[Bruce Evans]

On Tue, 15 Jan 2013, Wojciech A. Koszek wrote:

> On Tue, Jan 15, 2013 at 08:26:16AM +0000, Ganbold Tsagaankhuu wrote:
>> ...
>> Added: head/sys/arm/allwinner/console.c
>> ==============================================================================
>> --- /dev/null	00:00:00 1970	(empty, because file is newly added)
>> +++ head/sys/arm/allwinner/console.c	Tue Jan 15 08:26:16 2013	(r245450)
>> @@ -0,0 +1,146 @@
> [..]
>> +#ifndef	A10_UART_BASE
>> +#define	A10_UART_BASE	0xe1c28000 	/* UART0 */
>> +#endif
>> +
>> +int reg_shift = 2;
>
> Could you make it static and move it below defines?
>
>> +#define UART_DLL        0       /* Out: Divisor Latch Low */
>> +#define UART_DLM        1       /* Out: Divisor Latch High */
>> +#define UART_FCR        2       /* Out: FIFO Control Register */
>> +#define UART_LCR        3       /* Out: Line Control Register */
>> +#define UART_MCR        4       /* Out: Modem Control Register */
>> +#define UART_LSR        5       /* In:  Line Status Register */
>> +#define UART_LSR_THRE   0x20    /* Transmit-hold-register empty */
>> +#define UART_LSR_DR     0x01    /* Receiver data ready */
>> +#define UART_MSR        6       /* In:  Modem Status Register */
>> +#define UART_SCR        7       /* I/O: Scratch Register */

These are ordinary 16450 values which are defined in <ic/ns16550.h>.

This is misformatted with a space instead of a tab after #define.

Since it's a 16450, uart(4) can probably handle the entire console driver,
with fewer bugs (some locking is required in a general console driver).
Of course, it's easier to write a primitive console driver by hacking on
the raw registers than to interface with uart.

>> +
>> +
>> +/*
>> + * uart related funcs
>> + */

Style bugs (extra and missing blank lines, and comment punctuation).

>> +static u_int32_t
>> +uart_getreg(u_int32_t *bas)
>> +{
>> +	return *((volatile u_int32_t *)(bas)) & 0xff;
>> +}

It's better to put the reg_shift complications at the lowest level.

reg_shift could probably be const or #defined so that all the calculations
with it can be done at runtime.

>> +
>> +static void
>> +uart_setreg(u_int32_t *bas, u_int32_t val)
>> +{
>> +	*((volatile u_int32_t *)(bas)) = (u_int32_t)val;
>> +}

Bogus cast.  val already has type u_int32_t.  u_int32_t should be spelled
uint32_t in new code.

>> +
>> +static int
>> +ub_getc(void)
>> +{
>> +	while ((uart_getreg((u_int32_t *)(A10_UART_BASE +
>> +	    (UART_LSR << reg_shift))) & UART_LSR_DR) == 0);
>> +		__asm __volatile("nop");
>> +
>> +	return (uart_getreg((u_int32_t *)A10_UART_BASE) & 0xff);
>> +}
>> +
>> +static void
>> +ub_putc(unsigned char c)
>> +{
>> +	if (c == '\n')
>> +		ub_putc('\r');

The normal console driver does this in upper layers.  Is this driver
only for a very specialized console driver for use at boot time?
I'm not sure if uart(4) works then.

>> +
>> +	while ((uart_getreg((u_int32_t *)(A10_UART_BASE +
>> +	    (UART_LSR << reg_shift))) & UART_LSR_THRE) == 0)
>> +		__asm __volatile("nop");
>> +
>> +	uart_setreg((u_int32_t *)A10_UART_BASE, c);
>> +}
>
> Why aren't bus_* methods used here for accessing memory?

They don't support reg_shift, but should work otherwise.  You do the
shifting somewhere and then use bus-space at the level of uart_getreg()
above.

Bruce