Dear Prafulla Wadaskar,
In message <1242763678-13724-1-git-send-email-prafu...@marvell.com> you wrote:
>
> Kirkwood family controllers are highly integrated SOCs
> based on Feroceon-88FR131/Sheeva-88SV131 cpu core.
...
> +/*
> + * Window Size
> + * Used with the Base register to set the address window size and location.
> + * Must be programmed from LSB to MSB as sequence of 1âs followed by
> + * sequence of 0âs. The number of 1âs specifies the size of the window in
> + * 64 KByte granularity (e.g., a value of 0x00FF specifies 256 = 16 MByte).
> + * NOTE: A value of 0x0 specifies 64-KByte size.
> + */
You have a number of strange special characters here. Please try and
restrict yourself to plain ASCII text in normal C comments.
> + for (i = 0; i < (sizeval / 0x10000); i++) {
> + j |= (1 << i);
> + }
No curly braces for single line statements, please.
> +}
> +
> +/* prepares data to be loaded in win_Ctrl register */
> +#define KWCPU_WIN_CTRL_DATA(size, target, attr, en) (en | (target << 4)
> \
> + | (attr << 8) | (kw_winctrl_calcsize(size) << 16))
> +
> +/*
> + * kw_config_adr_windows - Configure address Windows
> + *
> + * There are 7 address windows supported by Kirkwood Soc to addess different
> + * devices. Each window can be configured for size, BAR and remap addr
> + * Below configuration is standard for most of the cases
> + *
> + * Reference Documentation:
> + * Mbus-L to Mbus Bridge Registers Configuration.
> + * (Sec 25.1 and 25.3 of Datasheet)
> + */
> +int kw_config_adr_windows(void)
> +{
> + struct kwwin_registers *winregs = (struct kwwin_registers
> *)KW_CPU_WIN_BASE;
Line too long.
> +/*
> + * kw_config_gpio - GPIO configuration
> + */
> +void kw_config_gpio(u32 gpp0_oe_val, u32 gpp1_oe_val, u32 gpp0_oe, u32
> gpp1_oe)
> +{
> + struct kwgpio_registers *gpio0reg = (struct kwgpio_registers
> *)KW_GPIO0_BASE;
> + struct kwgpio_registers *gpio1reg = (struct kwgpio_registers
> *)KW_GPIO1_BASE;
More too long lines. Pleasse check everywhere.
> + /* Init GPIOS to default values as per board requirement */
> + writel(gpp0_oe_val, (u32)&gpio0reg->dout);
> + writel(gpp1_oe_val, (u32)&gpio1reg->dout);
> + writel(gpp0_oe, (u32)&gpio0reg->oe);
> + writel(gpp1_oe, (u32)&gpio1reg->oe);
Why are you using these casts here? The whole purpose of using a C
struct to access device registers is to enable type checking by the C
compiler, but you sabotage this with these casts. Please don't do
that.
Why are you using these casts here? The whole purpose of using a C
struct to access device registers is to enable type checking by the C
compiler, but you sabotage this with these casts. Please don't do
that. This comment applies to the whole patch.
...
> + cntmrctrl = readl(CNTMR_CTRL_REG);
> + cntmrctrl |= CTCR_ARM_TIMER_EN(UBOOT_CNTR); /* enable cnt\timer */
Are you sure you want to have a TAB character in this comment? What's
"cnt imer" ? :-)
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 966df9a..dd5f332 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -27,6 +27,9 @@
> #ifdef CONFIG_NS87308
> #include <ns87308.h>
> #endif
> +#ifdef CONFIG_KIRKWOOD
> +#include <asm/arch/kirkwood.h>
> +#endif
What exactly is this needed for?
> #if defined (CONFIG_SERIAL_MULTI)
> #include <serial.h>
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 1350f3e..7ffa47d 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -28,6 +28,7 @@ LIB := $(obj)libspi.a
> COBJS-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o
> COBJS-$(CONFIG_ATMEL_SPI) += atmel_spi.o
> COBJS-$(CONFIG_BFIN_SPI) += bfin_spi.o
> +COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
> COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
> COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
> COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
> diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
> new file mode 100644
> index 0000000..e0c4f0a
> --- /dev/null
> +++ b/drivers/spi/kirkwood_spi.c
> @@ -0,0 +1,169 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafu...@marvell.com>
> + *
> + * Derived from drivers/spi/mpc8xxx_spi.c
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <spi.h>
> +#include <asm/arch/kirkwood.h>
> +#include <asm/arch/spi.h>
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> + unsigned int max_hz, unsigned int mode)
> +{
> + struct spi_slave *slave;
> + u32 data;
> + u32 *mppreg = (u32 *)KW_MPP_BASE;
> +
> + if (!spi_cs_is_valid(bus, cs))
> + return NULL;
> +
> + slave = malloc(sizeof(struct spi_slave));
> + if (!slave)
> + return NULL;
> +
> + slave->bus = bus;
> + slave->cs = cs;
> +
> + writel(0x00000002, KW_REG_SPI_CTRL);
> + /* program spi clock prescaller using max_hz */
> + data = ((CONFIG_SYS_TCLK / 2) / max_hz) & 0x0000000f;
> + debug("data = 0x%08x \n", data);
> + writel(0x00000210 | data, KW_REG_SPI_CONFIG);
> + writel(0x00000001, KW_REG_SPI_IRQ_CAUSE);
> + writel(0x00000000, KW_REG_SPI_IRQ_MASK);
What does these magic constants mean?
> + /* program mpp registers to select SPI_CSn */
> + if (cs)
> + writel((readl((u32)&mppreg[0]) & 0x0fffffff) |
> + 0x20000000, (u32)&mppreg[0]);
> + else
> + writel((readl((u32)&mppreg[0]) & 0xfffffff0) |
> + 0x00000002, (u32)&mppreg[0]);
Ot these?
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
> + void *din, unsigned long flags)
...
> + for (tm = 0, isread = 0; tm < KW_SPI_TIMEOUT; ++tm) {
> + if (readl(KW_REG_SPI_IRQ_CAUSE)) {
> + isread = 1;
> + tmpdin = readl(KW_REG_SPI_DATA_IN);
> + debug
> + ("*** spi_xfer: din %08X ... %08x read\n",
> + din, tmpdin);
Indentation by TABs only, please.
> +#define INTREG_BASE 0xd0000000
> +#define KW_REGISTER(x) (KW_REGS_PHY_BASE + x)
> +#define KW_OFFSET_REG (INTREG_BASE + 0x20080)
> +
> +/* 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))
> +#define KW_UART1_BASE (KW_REGISTER(0x13000))
> +#define KW_MPP_BASE (KW_REGISTER(0x10000))
> +#define KW_GPIO0_BASE (KW_REGISTER(0x10100))
> +#define KW_GPIO1_BASE (KW_REGISTER(0x10140))
> +#define KW_CPU_WIN_BASE (KW_REGISTER(0x20000))
> +#define KW_CPU_REG_BASE (KW_REGISTER(0x20100))
> +#define KW_TIMER_BASE (KW_REGISTER(0x20300))
> +#define KW_REG_PCIE_BASE (KW_REGISTER(0x40000))
> +#define KW_EGIGA0_BASE (KW_REGISTER(0x72000))
> +#define KW_EGIGA1_BASE (KW_REGISTER(0x76000))
Use a C struct?
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: w...@denx.de
"One lawyer can steal more than a hundred men with guns."
- The Godfather
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
