Date: Wed, 16 Jan 2013 16:22:07 +0800 From: Ganbold Tsagaankhuu <ganbold@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, Ganbold Tsagaankhuu <ganbold@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org, "Wojciech A. Koszek" <wkoszek@freebsd.org> Subject: Re: svn commit: r245450 - in head/sys: arm/allwinner arm/conf boot/fdt/dts Message-ID: <CAGtf9xOVZ1YP9=tTkVezE0bbUCe%2Bdtd%2BbNHij93bnxrP_mF0bA@mail.gmail.com> In-Reply-To: <20130116170419.G1725@besplex.bde.org> References: <201301150826.r0F8QGJr044600@svn.freebsd.org> <20130115162116.GI20538@FreeBSD.org> <20130116170419.G1725@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 16, 2013 at 2:43 PM, Bruce Evans <brde@optusnet.com.au> 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 <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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGtf9xOVZ1YP9=tTkVezE0bbUCe%2Bdtd%2BbNHij93bnxrP_mF0bA>