From owner-svn-src-head@FreeBSD.ORG Wed Jan 16 08:22:08 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id F03E5CDC; Wed, 16 Jan 2013 08:22:07 +0000 (UTC) (envelope-from ganbold@gmail.com) Received: from mail-ie0-f182.google.com (mail-ie0-f182.google.com [209.85.223.182]) by mx1.freebsd.org (Postfix) with ESMTP id 8C55525D; Wed, 16 Jan 2013 08:22:07 +0000 (UTC) Received: by mail-ie0-f182.google.com with SMTP id s9so1941148iec.27 for ; Wed, 16 Jan 2013 00:22:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=o6bSh3hbs+tVvlBtmeeMW6KKQjVc/170b7xu670t498=; b=OGkoTlhbOGUTaHxc3ZXyuxaU5gPbu+ohOiVKx8xhGnngN4LQ/GmrGqBYziq8SN5rtG pqNWyRHSJZUgDeKxLyrur/S8wH5O57TlQmAtjKu2JKqupZCaTOhmZVe9qmFrFZhKxFs8 YG5GdzJx2KNLyvI4W9jmXR0WjTupkuZTLgcrzmLlIUKqwzxlvE8hsPYUhjrNLxAGr4yJ pdBJ4V6/SZuDeLEofQzkVAYJwi8UnxiC78q4tav921Fxbon69LD0HW26GgZEC+GvwD0R tmusQcHO7Y6XHpzstP77iExjh4/y5a3RqbJb2o1b8z2vsWip2JtnoM4G9MC3/OdDDok0 kVqg== MIME-Version: 1.0 X-Received: by 10.50.5.133 with SMTP id s5mr212011igs.14.1358324527221; Wed, 16 Jan 2013 00:22:07 -0800 (PST) Received: by 10.64.170.167 with HTTP; Wed, 16 Jan 2013 00:22:07 -0800 (PST) In-Reply-To: <20130116170419.G1725@besplex.bde.org> References: <201301150826.r0F8QGJr044600@svn.freebsd.org> <20130115162116.GI20538@FreeBSD.org> <20130116170419.G1725@besplex.bde.org> Date: Wed, 16 Jan 2013 16:22:07 +0800 Message-ID: Subject: Re: svn commit: r245450 - in head/sys: arm/allwinner arm/conf boot/fdt/dts From: Ganbold Tsagaankhuu To: Bruce Evans Content-Type: text/plain; charset=ISO-8859-1 Cc: svn-src-head@freebsd.org, Ganbold Tsagaankhuu , svn-src-all@freebsd.org, src-committers@freebsd.org, "Wojciech A. Koszek" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jan 2013 08:22:08 -0000 On Wed, Jan 16, 2013 at 2:43 PM, Bruce Evans wrote: > 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. Current ns16550 driver doesn't work yet at boot time for A10. Ganbold > > >>> + >>> + 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