From owner-svn-src-all@FreeBSD.ORG Wed Jan 16 06:43:19 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id D2E35C63; Wed, 16 Jan 2013 06:43:19 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 71F7ACD7; Wed, 16 Jan 2013 06:43:18 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r0G6h98N031494 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 16 Jan 2013 17:43:10 +1100 Date: Wed, 16 Jan 2013 17:43:09 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Wojciech A. Koszek" Subject: Re: svn commit: r245450 - in head/sys: arm/allwinner arm/conf boot/fdt/dts In-Reply-To: <20130115162116.GI20538@FreeBSD.org> Message-ID: <20130116170419.G1725@besplex.bde.org> References: <201301150826.r0F8QGJr044600@svn.freebsd.org> <20130115162116.GI20538@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=R7tbgqtX c=1 sm=1 a=T_vc9_tLY8QA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=0w7YBWHZSsMA:10 a=rSZ73qvps9zlUWiUUE0A:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, Ganbold Tsagaankhuu , svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jan 2013 06:43:19 -0000 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 . 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