[U-Boot] [PATCH v2] spi/cadence: Adding Cadence SPI driver support for SOCFPGA

Thu Jan 9 17:04:05 CET 2014

Hi Jagan,

On Thu, 2014-01-09 at 20:56 +0530, Jagan Teki wrote:
> HI Chin Liang,
> 
> On Thu, Jan 9, 2014 at 8:06 PM, Chin Liang See <clsee at altera.com> wrote:
> > Hi Jagan,
> >
> > On Wed, 2014-01-08 at 17:43 +0530, Jagan Teki wrote:
> >> Hi Chin Liang See,
> >>
> >> On Thu, Jan 2, 2014 at 8:13 AM, Chin Liang See <clsee at altera.com> wrote:
> >> > To add the Cadence SPI driver support for Altera SOCFPGA. It
> >> > required information such as clocks and timing from platform's
> >> > configuration header file within include/configs folder
> >> >
> >> > Signed-off-by: Chin Liang See <clsee at altera.com>
> >> > Cc: Jagan Teki <jagannadh.teki at gmail.com>
> >> > Cc: Gerhard Sittig <gsi at denx.de>
> >> > ---
> >> > Changes for v2
> >> > - Combine driver into single C file instead of 2
> >> > - Added documentation on the macro used
> >> > - Using structure for registers instead of macro
> >> > ---
> >> >  doc/README.socfpga         |   47 ++
> >> >  drivers/spi/Makefile       |    1 +
> >> >  drivers/spi/cadence_qspi.c | 1018 ++++++++++++++++++++++++++++++++++++++++++++
> >> >  drivers/spi/cadence_qspi.h |  170 ++++++++
> >> >  4 files changed, 1236 insertions(+)
> >> >  create mode 100644 drivers/spi/cadence_qspi.c
> >> >  create mode 100644 drivers/spi/cadence_qspi.h
> >> >
> >> > diff --git a/doc/README.socfpga b/doc/README.socfpga
> >> > index cfcbbfe..242af97 100644
> >> > --- a/doc/README.socfpga
> >> > +++ b/doc/README.socfpga
> >> > @@ -51,3 +51,50 @@ the card
> >> >  #define CONFIG_SOCFPGA_DWMMC_BUS_HZ    50000000
> >> >  -> The clock rate to controller. Do note the controller have a wrapper which
> >> >  divide the clock from PLL by 4.
> >> > +
> >> > +--------------------------------------------
> >> > +cadence_qspi
> >> > +--------------------------------------------
> >> > +Here are macro and detailed configuration required to enable Cadence QSPI
> >> > +controller support within SOCFPGA
> >> > +
> >> > +#define CONFIG_SPI_FLASH
> >> > +-> To enable the SPI flash framework support
> >> > +
> >> > +#define CONFIG_CMD_SF
> >> > +-> To enable the console support for SPI flash
> >> > +
> >> > +#define CONFIG_SF_DEFAULT_SPEED                (50000000)
> >> > +-> To set the target SPI clock frequency in Hz
> >> > +
> >> > +#define CONFIG_SF_DEFAULT_MODE         SPI_MODE_3
> >> > +-> To set the SPI mode (CPOL & CPHA). Normally use mode 3 for serial NOR flash
> >> > +
> >> > +#define CONFIG_SPI_FLASH_QUAD          (1)
> >> > +-> To enable the Quad IO mode for performance boost
> >> > +
> >> > +#define CONFIG_SPI_FLASH_STMICRO
> >> > +#define CONFIG_SPI_FLASH_SPANSION
> >> > +-> To enable the SPI flash support for vendor Micron and Spansion
> >> > +
> >> > +#define CONFIG_CQSPI_BASE              (SOCFPGA_QSPIREGS_ADDRESS)
> >> > +#define CONFIG_CQSPI_AHB_BASE          (SOCFPGA_QSPIDATA_ADDRESS)
> >> > +-> To specify the base address for controller CSR base and AHB data base addr
> >> > +
> >> > +#define CONFIG_CQSPI_REF_CLK           (400000000)
> >> > +-> The clock frequency supplied from PLL to the QSPI controller
> >> > +
> >> > +#define CONFIG_CQSPI_PAGE_SIZE         (256)
> >> > +-> To define the page size of serial flash in bytes
> >> > +
> >> > +#define CONFIG_CQSPI_BLOCK_SIZE                (16)
> >> > +-> To define the block size of serial flash in pages
> >> > +
> >> > +#define CONFIG_CQSPI_DECODER           (0)
> >> > +-> To enable the 4-to-16 decoder which enable up to 16 serial flash devices
> >> > +
> >> > +#define CONFIG_CQSPI_TSHSL_NS          (200)
> >> > +#define CONFIG_CQSPI_TSD2D_NS          (255)
> >> > +#define CONFIG_CQSPI_TCHSH_NS          (20)
> >> > +#define CONFIG_CQSPI_TSLCH_NS          (20)
> >> > +-> Configure the controller based on serial flash device timing characteristic
> >> Do we really require this, because most of the known macros definitions.
> >> Better to not write too many duplicates - Yes there are few macro's
> >> which are specific to cadence
> >> but I don't think those were required.
> >
> > Oh actually this is to address Gerhard's comment. This document will
> > guide user the required macro (and its details) in order to use this
> > Cadence QSPI controller.
> 
> Yes - but I don't see the actual usecase here better to comment
> cadence macros in
> driver file itself because it could be new style for each spi configs,
> this sound may not valid i guess.
> and also send the config patch where this got enable.
> 

Oh ok, I can move that into this c file to describe the macro specific
to this driver.

> >
> >> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> >> > index ed4ecd7..b8d56ea 100644
> >> > --- a/drivers/spi/Makefile
> >> > +++ b/drivers/spi/Makefile
> >> > @@ -15,6 +15,7 @@ obj-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o
> >> >  obj-$(CONFIG_ATMEL_SPI) += atmel_spi.o
> >> >  obj-$(CONFIG_BFIN_SPI) += bfin_spi.o
> >> >  obj-$(CONFIG_BFIN_SPI6XX) += bfin_spi6xx.o
> >> > +obj-$(CONFIG_CADENCE_QSPI) += cadence_qspi.o
> >> >  obj-$(CONFIG_CF_SPI) += cf_spi.o
> >> >  obj-$(CONFIG_CF_QSPI) += cf_qspi.o
> >> >  obj-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> >> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> >> > new file mode 100644
> >> > index 0000000..4712b45
> >> > --- /dev/null
> >> > +++ b/drivers/spi/cadence_qspi.c
> >> > @@ -0,0 +1,1018 @@
> >> > +/*
> >> > + * (C) Copyright 2014 Altera Corporation <www.altera.com>
> >> > + *
> >> > + * SPDX-License-Identifier:    GPL-2.0+
> >> > + */
> >> > +
> >> > +#include <common.h>
> >> > +#include <asm/io.h>
> >> > +#include <asm/errno.h>
> >> > +#include <malloc.h>
> >> > +#include <spi.h>
> >> > +#include "cadence_qspi.h"
> >>
> >> Try to move the stuff from header here.!
> >
> > Actually I was doing this initially but it bloated this c file a lot.
> > Hopefully its ok that we separate out to a header file. But if you think
> > this is required, I can make the v3.
> 
> My big concern here was about the driver code, I am not satisfied the
> way that the code
> is organized(means no.of functions) w.r.t different modes/commands.
> As this is a bootloader code I want to see the functionality
> spi_xfer() as minimum/optimize as possible.
> 

Oops just realize forget to address this comments. Actually, the
additional code within spi_xfer is due to the controller hardware
behavior. There are various operating mode within the controller where
different mode required for bulk data transfer and status/control
transfer.

> I am open to discuss more with next level patches as well if you have
> more questions.
> 

Let send out v3 for the documentation fix. Thanks again for the
comments.

Chin Liang