[U-Boot] [PATCH 1/2][RESEND] Add an SPI driver for PPC440EPx

[Stefan Roese]

Hi Steve,

On Monday 08 December 2008, Steven A. Falco wrote:
> This patch adds a SPI driver for the PPC440EPx processor.
>
> Signed-off-by: Steven A. Falco <sfalco at harris.com>
> ---
> Sorry - forgot the subject line, the first time.

Please start the subject with "ppc4xx: " for 4xx related patches 
(e.g.: "ppc4xx: Add PPC4xx SPI driver"). And while looking at this subject, 
there shouldn't be much 440EPx specific stuff in this patch. So please name 
this patch PPC4xx instead of PPC440EPx, perhaps with a comment that 
it's "only" tested on 440EPx.

And it's always a good idea to CC the subsystem maintainer (custodian), in 
this case that's me. ;)

Some further comments below. And I second all of Ben's comments.

>  cpu/ppc4xx/Makefile |    1 +
>  cpu/ppc4xx/spi.c    |  128
> +++++++++++++++++++++++++++++++++++++++++++++++++++ include/4xx_spi.h   |  
> 50 ++++++++++++++++++++
>  3 files changed, 179 insertions(+), 0 deletions(-)
>  create mode 100644 cpu/ppc4xx/spi.c
>  create mode 100644 include/4xx_spi.h
>
> diff --git a/cpu/ppc4xx/Makefile b/cpu/ppc4xx/Makefile
> index 96ab5c6..bdebfb6 100644
> --- a/cpu/ppc4xx/Makefile
> +++ b/cpu/ppc4xx/Makefile
> @@ -54,6 +54,7 @@ COBJS	+= iop480_uart.o
>  COBJS	+= ndfc.o
>  COBJS	+= sdram.o
>  COBJS	+= speed.o
> +COBJS	+= spi.o

Yes, please move this to drivers/spi.

>  COBJS	+= tlb.o
>  COBJS	+= traps.o
>  COBJS	+= usb.o
> diff --git a/cpu/ppc4xx/spi.c b/cpu/ppc4xx/spi.c
> new file mode 100644
> index 0000000..572244b
> --- /dev/null
> +++ b/cpu/ppc4xx/spi.c
> @@ -0,0 +1,128 @@
> +/*
> + * Copyright (C) 2008 Harris Corporation
> + * Author: Steven A. Falco <sfalco at harris.com>
> + *
> + * 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., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <spi.h>
> +#include <malloc.h>
> +
> +#include <asm/io.h>
> +
> +#include <4xx_spi.h>
> +
> +#if defined(CONFIG_HARD_SPI)
> +
> +void spi_init (void )

Nitpick: No space after "void". You should probably run checkpatch over this 
patch to check for those kind of issues.

> +{
> +	volatile u8 *cd = (volatile u8 *) SPI_CDM;
> +	volatile u8 *md = (volatile u8 *) SPI_MODE;

As Ben mentioned, don't use volatile pointer access. Instead use the in_8()... 
accessor functions.

> +
> +	*cd = 0;		/* Default to "go fast" */
> +	*md = SPI_MODE_SPE;	/* Enable port */
> +}
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +			unsigned int max_hz, unsigned int mode)
> +{
> +	volatile u8 *md = (volatile u8 *) SPI_MODE;
> +	volatile u8 *cd = (volatile u8 *) SPI_CDM;
> +	ulong opb = get_OPB_freq();
> +	ulong divisor;
> +	struct spi_slave *s;
> +
> +	if(!spi_cs_is_valid(bus, cs))
> +		return NULL;
> +
> +	divisor = ((opb + (max_hz * 4) - 1) / (max_hz * 4)) - 1;
> +	if(divisor > 255)

Nitpick: Space after "if". Again checkpatch will help here.

> +		return NULL;
> +
> +	*cd = divisor;
> +
> +	if (!(s = malloc(sizeof(struct spi_slave))))
> +		return NULL;
> +
> +	if (mode & SPI_CPHA)
> +		*md &= ~SPI_MODE_SCP;
> +	else
> +		*md |= SPI_MODE_SCP;
> +
> +	if (mode & SPI_CPOL)
> +		*md |= SPI_MODE_CI;
> +	else
> +		*md &= ~SPI_MODE_CI;
> +
> +	s->bus = bus;
> +	s->cs = cs;
> +
> +	return s;
> +}
> +
> +void spi_free_slave(struct spi_slave *slave)
> +{
> +	free(slave);
> +}
> +
> +int spi_claim_bus(struct spi_slave *slave)
> +{
> +	return 0;
> +}
> +
> +void spi_release_bus(struct spi_slave *slave)
> +{
> +}
> +
> +#define GO	*cr = SPI_CR_STR
> +#define TXWAIT	while(*sr & SPI_SR_BSY)
> +#define RXWAIT	while(!(*sr & SPI_SR_RBR))
> +
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> +		const void *dout, void *din, unsigned long flags)
> +{
> +	volatile u8 *cr = (volatile u8 *) SPI_CR;
> +	volatile u8 *sr = (volatile u8 *) SPI_SR;
> +	volatile u8 *tx = (volatile u8 *) SPI_TXD;
> +	volatile u8 *rx = (volatile u8 *) SPI_RXD;
> +
> +	const u8 *txd = dout;
> +	u8 *rxd = din;
> +	int ii;
> +
> +	if (flags & SPI_XFER_BEGIN)
> +		spi_cs_activate(slave);
> +
> +	/* Do a byte at a time */
> +	for (ii = 0; ii < ((bitlen + 7) / 8); ii++) {
> +		TXWAIT;
> +		*tx = *txd++;
> +		GO;
> +		RXWAIT;
> +		*rxd++ = *rx;
> +	}
> +
> +	if (flags & SPI_XFER_END)
> +		spi_cs_deactivate(slave);
> +
> +	return 0;
> +}
> +
> +#endif /* CONFIG_HARD_SPI */
> diff --git a/include/4xx_spi.h b/include/4xx_spi.h
> new file mode 100644
> index 0000000..331de20
> --- /dev/null
> +++ b/include/4xx_spi.h

I would prefer: include/asm-ppc/ppc4xx-spi.h instead.

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================