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

[Wojciech A. Koszek]

On Tue, Jan 15, 2013 at 08:26:16AM +0000, Ganbold Tsagaankhuu wrote:
> Author: ganbold (doc committer)
> Date: Tue Jan 15 08:26:16 2013
> New Revision: 245450
> URL: http://svnweb.freebsd.org/changeset/base/245450
> 
> Log:
>   Initial support for Allwinner A10 SoC (Cubieboard)
>   	Add simple console driver
>   	Add interrupt handling and timer codes
>   	Add kernel config file
>   	Add dts file
>   Approved by: gonzo

Ganbold,

Thanks for this commit. Comments below.

> 
> Added: head/sys/arm/allwinner/a10_machdep.c
> ==============================================================================
> --- /dev/null	00:00:00 1970	(empty, because file is newly added)
> +++ head/sys/arm/allwinner/a10_machdep.c	Tue Jan 15 08:26:16 2013	(r245450)
> @@ -0,0 +1,122 @@
> +/*-
> + * Copyright (c) 2012 Ganbold Tsagaankhuu. <ganbold at gmail.com>

Dot '.' after name isn't necessary.

I'd really appreciate having PDF filename of the documentation from which
the code was derived in the comments, e.g.:

	"This file is derived from X.PDF, ver1.0, date 2012/Y/Z"

> + *
> + * from: FreeBSD: //depot/projects/arm/src/sys/arm/ti/ti_machdep.c
> + */
> +

[..]

> +int
> +platform_devmap_init(void)
> +{
> +	int i = 0;
> +
> +	fdt_devmap[i].pd_va =   0xE1C00000;
> +	fdt_devmap[i].pd_pa =   0x01C00000;
> +	fdt_devmap[i].pd_size = 0x00400000;	/* 4 MB */

Do you think you could comment on what these mean (or pages in the PDF where
can I find them) next to these variables?

> 
> 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 */
> +
> +
> +/*
> + * uart related funcs
> + */
> +static u_int32_t
> +uart_getreg(u_int32_t *bas)
> +{
> +	return *((volatile u_int32_t *)(bas)) & 0xff;
> +}
> +
> +static void
> +uart_setreg(u_int32_t *bas, u_int32_t val)
> +{
> +	*((volatile u_int32_t *)(bas)) = (u_int32_t)val;
> +}
> +
> +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');
> +
> +	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?

> +#
> +#
> +options		PHYSADDR=0x40000000
> +
> +makeoptions	KERNPHYSADDR=0x40200000

Two tabs?

> +options		KERNPHYSADDR=0x40200000
> +makeoptions	KERNVIRTADDR=0xc0200000
> +options		KERNVIRTADDR=0xc0200000
> +
> +options		STARTUP_PAGETABLE_ADDR=0x48000000
> +
> +files		"../allwinner/files.a10"



> *** DIFF OUTPUT TRUNCATED AT 1000 LINES ***

-- 
Wojciech A. Koszek
wkoszek at FreeBSD.czest.pl
http://FreeBSD.czest.pl/~wkoszek/