[U-Boot] [PATCH v8] Marvell Kirkwood family SOC support

Wolfgang Denk wd at denx.de
Tue May 19 23:58:37 CEST 2009


Dear Prafulla Wadaskar,

In message <1242763678-13724-1-git-send-email-prafulla at 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 <prafulla at 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: wd at denx.de
"One lawyer can steal more than a hundred men with guns."
- The Godfather


More information about the U-Boot mailing list