[U-Boot] [PATCH v2 1/2] ppc4xx: Add 440EPx SPI driver

[Stefan Roese]

On Monday 08 December 2008, Steven A. Falco wrote:
> This patch adds an SPI driver for the 440EPx processor.
>
> Tested on Sequoia.
>
> Signed-off-by: Steven A. Falco <sfalco at harris.com>
> ---
>
> Regarding Ben's comments:
>
> First, thanks for reviewing.  You gave me two different approaches to
> consider, 1) using in/out functions, and 2) using a structure.  So, for
> this version, I've followed the style of the example you gave, using the
> structure approach.

No, you _need_ to use the in_/out_() accessor functions. This is unrelated to 
using plain addresses and offsets vs. a structure. I really welcome the 
change to the structure in this version, so please just add the in_()/out_() 
functions upon accessing the SoC registers.

> This driver may be applicable to other 4xx chips, but after looking at
> the 405gp and 440gp, neither have a SPI hw device.  So, I'm making this
> one exclusive to the 440epx until someone else wants it on another
> processor.  At that point, it can be adapted as needed.

Many other 4xx PPC's have the same SPI controller as 440EPx. Unfortunately you 
looked at the wrong examples. Here a (incomplete) list of some other PPC's 
with the same controller:

405EX, 440EP, 440GR, 440GRx, 460EX, 460GT

But, yes. Your driver seems generic enough to be adapted to other controllers 
when needed.

> Regarding Stefan's comments:
>
> I've renamed the file, and cleaned up the nits.  Given that I've used
> the structure approach, I believe the in/out calls are not applicable.

No, as mentioned above you need to add the in_()/out_() calls as well.

Please find some more comments below.

> Regarding Wolfgang's comments:
>
> It is not for the fun of it.  We are planning a custom board which will
> use the SPI port.  We are entering layout now, and the hardware won't
> appear for at least a few months.  However, the Sequoia board does have
> a connector for the SPI port, and I have connected a device to that
> connector for prototyping purposes.  Other purchasers of the Sequoia may
> well have a similar need.
>
> You are certainly welcome to reject this driver if you feel that is
> insufficient grounds for "usage".  Please let me know, so I don't bother
> the list with it.
>
>  drivers/spi/Makefile      |    1 +
>  drivers/spi/ppc4xx_spi.c  |  119
> +++++++++++++++++++++++++++++++++++++++++++++ lude/asm-ppc/ppc4xx_spi.h |  
> 62 +++++++++++++++++++++++
>  3 files changed, 182 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/spi/ppc4xx_spi.c
>  create mode 100644 include/asm-ppc/ppc4xx_spi.h
>
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 15e0f7a..7dbba5d 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -28,6 +28,7 @@ LIB	:= $(obj)libspi.a
>  COBJS-$(CONFIG_ATMEL_SPI) += atmel_spi.o
>  COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
>  COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
> +COBJS-$(CONFIG_PPC440EPX_SPI) += ppc4xx_spi.o
>  COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
>
>  COBJS	:= $(COBJS-y)
> diff --git a/drivers/spi/ppc4xx_spi.c b/drivers/spi/ppc4xx_spi.c
> new file mode 100644
> index 0000000..156b056
> --- /dev/null
> +++ b/drivers/spi/ppc4xx_spi.c
> @@ -0,0 +1,119 @@
> +/*
> + * 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 <asm/ppc4xx_spi.h>
> +
> +void spi_init (void)
> +{
> +	volatile spi4xx_t *spi = (volatile spi4xx_t *) SPI_BASE_ADDR;
> +
> +	spi->cdm = 0;			/* Default to "go fast" */
> +	spi->mode = 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 spi4xx_t *spi = (volatile spi4xx_t *) SPI_BASE_ADDR;
> +
> +	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)
> +		return NULL;
> +
> +	spi->cdm = divisor;
> +
> +	if (!(s = malloc(sizeof(struct spi_slave))))
> +		return NULL;
> +
> +	if (mode & SPI_CPHA)
> +		spi->mode &= ~SPI_MODE_SCP;
> +	else
> +		spi->mode |= SPI_MODE_SCP;
> +
> +	if (mode & SPI_CPOL)
> +		spi->mode |= SPI_MODE_CI;
> +	else
> +		spi->mode &= ~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	spi->cr = SPI_CR_STR
> +#define TXWAIT	while(spi->sr & SPI_SR_BSY)
> +#define RXWAIT	while(!(spi->sr & SPI_SR_RBR))

I would prefer static inlined functions here.

> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> +		const void *dout, void *din, unsigned long flags)
> +{
> +	volatile spi4xx_t *spi = (volatile spi4xx_t *) SPI_BASE_ADDR;
> +
> +	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;
> +		spi->txd = *txd++;
> +		GO;
> +		RXWAIT;
> +		*rxd++ = spi->rxd;
> +	}
> +
> +	if (flags & SPI_XFER_END)
> +		spi_cs_deactivate(slave);
> +
> +	return 0;
> +}
> diff --git a/include/asm-ppc/ppc4xx_spi.h b/include/asm-ppc/ppc4xx_spi.h
> new file mode 100644
> index 0000000..135c9e6
> --- /dev/null
> +++ b/include/asm-ppc/ppc4xx_spi.h
> @@ -0,0 +1,62 @@
> +/*
> + * 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
> + */
> +
> +#ifndef _4xx_spi_h_
> +#define _4xx_spi_h_
> +
> +#include <asm/types.h>
> +
> +#if defined(CONFIG_440EPX)

And 440GRx please.

> +#define SPI_BASE_ADDR	(CONFIG_SYS_PERIPHERAL_BASE + 0x00000900)
> +#endif
> +
> +/* Bits in the command register */
> +#define SPI_CR_STR	0x01
> +
> +/* Bits in the status register */
> +#define SPI_SR_RBR	0x01
> +#define SPI_SR_BSY	0x02
> +
> +/* Bits in the mode register */
> +#define SPI_MODE_IL	0x01
> +#define SPI_MODE_CI	0x02
> +#define SPI_MODE_RD	0x04
> +#define SPI_MODE_SPE	0x08
> +#define SPI_MODE_SCP	0x10
> +
> +#if defined(CONFIG_440EPX) || \
> +	defined(CONFIG_440GRX)

Please remove this "#if". All other 4xx PPC's with an SPI controller have the 
same register layout. So the only thing that is 440EPx/440GRx specific is the 
base address.

> +typedef struct spi4xx {
> +	u8 mode;	/* mode register */
> +	u8 rxd;		/* receive register */
> +	u8 txd;		/* transmit register */
> +	u8 cr;		/* command register */
> +	u8 sr;		/* status register */
> +	u8 res0;	/* reserved */
> +	u8 cdm;		/* clock divisor */
> +} spi4xx_t;
> +
> +#endif
> +
> +#endif /* _4xx_spi_h_ */

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
=====================================================================