[U-Boot] [PATCH v7] Marvell Kirkwood family SOC support
Wolfgang Denk
wd at denx.de
Tue May 19 00:26:09 CEST 2009
Dear Prafulla Wadaskar,
> +unsigned char get_random_hex(void)
> +{
> + int i;
> + u32 inbuf[16];
> + u8 outbuf[16];
> +
> + /*
> + * in case of 88F6281/88F6192 A0,
> + * Bit7 need to reset to generate random values in KW_REG_UNDOC_0x1470
> + * Soc reg offsets KW_REG_UNDOC_0x1470 and KW_REG_UNDOC_0x1478 are reserved regs and
> + * Does not have names at this moment (no errata available)
> + */
> + writel(readl(KW_REG_UNDOC_0x1478) & ~(1 << 7), KW_REG_UNDOC_0x1478);
> + for (i = 0; i < 16; i++) {
either use a #define'd name instead of the repeated 16, or use
sizeof(inbuf), but don't hard-code the same constant in multiple
places. This is calling for inconsistency.
> --- /dev/null
> +++ b/drivers/serial/kirkwood_serial.c
> @@ -0,0 +1,129 @@
...
...
> +static void kw_uart_putc(u8 c)
> +{
> + while ((p_uart_port->lsr & LSR_THRE) == 0) ;
> + p_uart_port->thr = c;
> + return;
> +}
> +
> +void serial_putc(const char c)
> +{
> + if (c == '\n')
> + kw_uart_putc('\r');
> +
> + kw_uart_putc(c);
> +}
> +
> +int serial_getc(void)
> +{
> + while ((p_uart_port->lsr & LSR_DR) == 0) ;
> + return (p_uart_port->rbr);
> +}
> +
> +int serial_tstc(void)
> +{
> + return ((p_uart_port->lsr & LSR_DR) != 0);
> +}
> +
> +void serial_setbrg(void)
> +{
> + DECLARE_GLOBAL_DATA_PTR;
> +
> + int clock_divisor = (CONFIG_SYS_TCLK / 16) / gd->baudrate;
> +
> + p_uart_port->ier = 0x00;
> + p_uart_port->lcr = LCR_DIVL_EN; /* Access baud rate */
> + p_uart_port->dll = clock_divisor & 0xff; /* 9600 baud */
> + p_uart_port->dlm = (clock_divisor >> 8) & 0xff;
> + p_uart_port->lcr = LCR_8N1; /* 8 data, 1 stop, no parity */
> + /* Clear & enable FIFOs */
> + p_uart_port->fcr = FCR_FIFO_EN | FCR_RXSR | FCR_TXSR;
> + return;
Please use I/O accessors instead of all trhese raw register accesses.
> +/* SOC specific definations */
> +#define INTREG_BASE 0xd0000000
> +#define KW_REGISTER(x) (KW_REGS_PHY_BASE | x)
> +#define KW_OFFSET_REG (INTREG_BASE | 0x20080)
You want to perform a "base + offset" addition, so please write this
as an addition and not as a logical operation! But...
> +/* undocumented registers */
> +#define KW_REG_UNDOC_0x1470 (KW_REGISTER(0x1470))
> +#define KW_REG_UNDOC_0x1478 (KW_REGISTER(0x1478))
> +
> +#define KW_UART0_BASE (KW_REGISTER(0x12000)) /* UArt 0 */
> +#define KW_UART1_BASE (KW_REGISTER(0x13000)) /* UArt 1 */
> +
> +/* Controler environment registers offsets */
> +#define KW_REG_MPP_CONTROL0 (KW_REGISTER(0x10000))
> +#define KW_REG_MPP_CONTROL1 (KW_REGISTER(0x10004))
> +#define KW_REG_MPP_CONTROL2 (KW_REGISTER(0x10008))
> +#define KW_REG_MPP_CONTROL3 (KW_REGISTER(0x1000C))
> +#define KW_REG_MPP_CONTROL4 (KW_REGISTER(0x10010))
> +#define KW_REG_MPP_CONTROL5 (KW_REGISTER(0x10014))
> +#define KW_REG_MPP_CONTROL6 (KW_REGISTER(0x10018))
> +#define KW_REG_MPP_SMPL_AT_RST (KW_REGISTER(0x10030))
> +#define KW_REG_DEVICE_ID (KW_REGISTER(0x10034))
> +#define KW_REG_MPP_OUT_DRV_REG (KW_REGISTER(0x100E0))
> +
> +#define KW_REG_GPP0_DATA_OUT (KW_REGISTER(0x10100))
> +#define KW_REG_GPP0_DATA_OUT_EN (KW_REGISTER(0x10104))
> +#define KW_REG_GPP0_BLINK_EN (KW_REGISTER(0x10108))
> +#define KW_REG_GPP0_DATA_IN_POL (KW_REGISTER(0x1010C))
> +#define KW_REG_GPP0_DATA_IN (KW_REGISTER(0x10110))
> +#define KW_REG_GPP0_INT_CAUSE (KW_REGISTER(0x10114))
> +#define KW_REG_GPP0_INT_MASK (KW_REGISTER(0x10118))
> +#define KW_REG_GPP0_INT_LVL (KW_REGISTER(0x1011c))
> +
> +#define KW_REG_GPP1_DATA_OUT (KW_REGISTER(0x10140))
> +#define KW_REG_GPP1_DATA_OUT_EN (KW_REGISTER(0x10144))
> +#define KW_REG_GPP1_BLINK_EN (KW_REGISTER(0x10148))
> +#define KW_REG_GPP1_DATA_IN_POL (KW_REGISTER(0x1014C))
> +#define KW_REG_GPP1_DATA_IN (KW_REGISTER(0x10150))
> +#define KW_REG_GPP1_INT_CAUSE (KW_REGISTER(0x10154))
> +#define KW_REG_GPP1_INT_MASK (KW_REGISTER(0x10158))
> +#define KW_REG_GPP1_INT_LVL (KW_REGISTER(0x1015c))
> +
> +#define KW_REG_NAND_READ_PARAM (KW_REGISTER(0x10418))
> +#define KW_REG_NAND_WRITE_PARAM (KW_REGISTER(0x1041c))
> +#define KW_REG_NAND_CTRL (KW_REGISTER(0x10470))
> +
> +#define KW_REG_WIN_CTRL(x) (KW_REGISTER((0x20000 + (x * 0x10))))
> +#define KW_REG_WIN_BASE(x) (KW_REGISTER((0x20004 + (x * 0x10))))
> +#define KW_REG_WIN_REMAP_LOW(x) (KW_REGISTER((0x20008 + (x * 0x10))))
> +#define KW_REG_WIN_REMAP_HIGH(x) (KW_REGISTER((0x2000c + (x * 0x10))))
> +
> +#define KW_REG_CPU_CONFIG (KW_REGISTER(0x20100))
> +#define KW_REG_CPU_CTRL_STAT (KW_REGISTER(0x20104))
> +#define KW_REG_CPU_RSTOUTN_MASK (KW_REGISTER(0x20108))
> +#define KW_REG_CPU_SYS_SOFT_RST (KW_REGISTER(0x2010C))
> +#define KW_REG_CPU_AHB_MBUS_CAUSE_INT (KW_REGISTER(0x20110))
> +#define KW_REG_CPU_AHB_MBUS_MASK_INT (KW_REGISTER(0x20114))
> +#define KW_REG_CPU_FTDLL_CONFIG (KW_REGISTER(0x20120))
> +#define KW_REG_CPU_L2_CONFIG (KW_REGISTER(0x20128))
> +#define KW_REG_L2_RAM_TIMING0 (KW_REGISTER(0x20134))
> +#define KW_REG_L2_RAM_TIMING1 (KW_REGISTER(0x20138))
> +
> +#define KW_REG_TMR_CTRL (KW_REGISTER(0x20300))
> +#define KW_REG_TMR_RELOAD (KW_REGISTER(0x20310))
> +#define KW_REG_TMR_VAL (KW_REGISTER(0x20314))
> +
> +#define KW_REG_PCIE_BASE (KW_REGISTER(0x40000))
> +
> +#define KW_EGIGA0_BASE (KW_REGISTER(0x72000))
> +#define KW_EGIGA1_BASE (KW_REGISTER(0x76000))
... please get rid of all these register offsets and use C structs
instead, as has been discussed here a couple of times recently.
...
> +++ b/include/asm-arm/arch-kirkwood/serial.h
> @@ -0,0 +1,89 @@
...
> +/* This structure describes the registers offsets for one UART port/channel */
> +struct kw_uart_port {
> + u8 rbr; /* 0 = 0-3 */
> + u8 pad1[3];
> + u8 ier; /* 1 = 4-7 */
> + u8 pad2[3];
> + u8 fcr; /* 2 = 8-b */
> + u8 pad3[3];
> + u8 lcr; /* 3 = c-f */
> + u8 pad4[3];
> + u8 mcr; /* 4 = 10-13 */
> + u8 pad5[3];
> + u8 lsr; /* 5 = 14-17 */
> + u8 pad6[3];
> + u8 msr; /* 6 =18-1b */
> + u8 pad7[3];
> + u8 scr; /* 7 =1c-1f */
> + u8 pad8[3];
> +};
Come on. Why does this have to be a separate header file? Does this
not look very much the same like a zillion of other UARTs Why cannot
you simply use "include/ns16550.h" instead?
Heck. Why do you need your own serial driver at all? I bet the
standard 16550 driver just works fine for you, too.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Mistakes are often the stepping stones to utter failure.
More information about the U-Boot
mailing list