[U-Boot] [PATCH v3] spi/cadence: Adding Cadence SPI driver support for SOCFPGA
Gerhard Sittig
gsi at denx.de
Thu Feb 13 23:23:08 CET 2014
Yes, I'm late to respond. :(
On Fri, Jan 10, 2014 at 11:39 -0600, Chin Liang See 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
s/To add/Add/?
s/It required/It requires/?
> ---
> drivers/spi/Makefile | 1 +
> drivers/spi/cadence_qspi.c | 1018 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/spi/cadence_qspi.h | 196 +++++++++
> 3 files changed, 1215 insertions(+)
> create mode 100644 drivers/spi/cadence_qspi.c
> create mode 100644 drivers/spi/cadence_qspi.h
>
> 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
Is this a driver without a user, aka dead code? There is no
CADENCE_QSPI in U-Boot master, neither does your patch introduce
it anywhere. Am I missing a branch you build on top of?
> +static void cadence_qspi_apb_read_fifo_data(void *dest,
> + const void *src_ahb_addr, unsigned int bytes)
> +{
> + unsigned int temp;
> + int remaining = bytes;
> + unsigned int *dest_ptr = (unsigned int *)dest;
> + unsigned int *src_ptr = (unsigned int *)src_ahb_addr;
> +
> + while (remaining > 0) {
> + if (remaining >= CQSPI_FIFO_WIDTH) {
> + *dest_ptr = readl(src_ptr);
> + remaining -= CQSPI_FIFO_WIDTH;
> + } else {
> + /* dangling bytes */
> + temp = readl(src_ptr);
> + memcpy(dest_ptr, &temp, remaining);
> + break;
> + }
> + dest_ptr++;
> + }
> +
> + return;
> +}
These pointer casts look suspicious. The code assumes that the
"void *" pointers are aligned like integer items would be. This
may work for the port address, but I'd rather not do this for the
memory addresses.
> +/* Return 1 if idle, otherwise return 0 (busy). */
> +static unsigned int cadence_qspi_wait_idle(void)
> +{
> + unsigned int start, count = 0;
> + /* timeout in unit of ms */
> + unsigned int timeout = 5000;
> +
> + start = get_timer(0);
> + for ( ; get_timer(start) < timeout ; ) {
> + if ((readl(&cadence_qspi_base->cfg) >>
> + CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
> + count++;
> + else
> + count = 0;
> + /*
> + * Ensure the QSPI controller is in true idle state after
> + * reading back the same idle status consecutively
> + */
> + if (count >= CQSPI_POLL_IDLE_RETRY)
> + return 1;
> + }
> +
> + /* Timeout, still in busy mode. */
> + printf("QSPI: QSPI is still busy after poll for %d times.\n",
> + CQSPI_REG_RETRY);
> + return 0;
> +}
There are several style issues here.
A for() loop without a setup clause and without a re-iteration
clause would actually be a while() in disguise. At the very least
I'd suggest /* EMPTY */ comments to show that nothing was omitted
by chance, but by will.
There are whitespace issues in the indentation of continued
lines. The if() looks like it had multi line branches without
braces, and makes readers stop and think what's wrong.
The idle flag test looks a little obfuscated. I guess the more
popular idiom is to test for "readl(&cfg) & (1 << flagpos)", or
use the BIT() macro.
I could not quite get how the counter and the timer interact
here. Is my interpretation correct that there is a timeout, and
you succeed if you can see three consecutive set flags within
that time span?
> +static void cadence_qspi_apb_readdata_capture(unsigned int bypass,
> + unsigned int delay)
> +{
> + unsigned int reg;
> + cadence_qspi_apb_controller_disable();
> +
> + reg = readl(&cadence_qspi_base->rddatacap);
> +
> + if (bypass)
> + reg |= (1 << CQSPI_REG_RD_DATA_CAPTURE_BYPASS_LSB);
> + else
> + reg &= ~(1 << CQSPI_REG_RD_DATA_CAPTURE_BYPASS_LSB);
> +
> + reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
> + << CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
> +
> + reg |= ((delay & CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK)
> + << CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
> +
> + writel(reg, &cadence_qspi_base->rddatacap);
> +
> + cadence_qspi_apb_controller_enable();
> + return;
> +}
This could be a candidate for clrsetbits(), but I guess the "if
(bypass)" would complicate this operation. So the above form may
be the most appropriate one.
> +static void cadence_qspi_apb_chipselect(unsigned int chip_select,
> + unsigned int decoder_enable)
> +{
> + unsigned int reg;
> +
> + cadence_qspi_apb_controller_disable();
> +
> + debug("%s : chipselect %d decode %d\n", __func__, chip_select,
> + decoder_enable);
> +
> + reg = readl(&cadence_qspi_base->cfg);
> + /* docoder */
> + if (decoder_enable)
> + reg |= CQSPI_REG_CONFIG_DECODE_MASK;
> + else {
> + reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
> + /* Convert CS if without decoder.
> + * CS0 to 4b'1110
> + * CS1 to 4b'1101
> + * CS2 to 4b'1011
> + * CS3 to 4b'0111
> + */
> + chip_select = 0xF & ~(1 << chip_select);
> + }
style nits: braces around if() arms, and mult line comments,
indentation of continuation lines
> +
> + reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
> + << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
> + reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
> + << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
> + writel(reg, &cadence_qspi_base->cfg);
> +
> + cadence_qspi_apb_controller_enable();
> + return;
> +}
> +/* calibration sequence to determine the read data capture delay register */
> +int spi_calibration(struct spi_slave *slave)
> +{
> + struct cadence_qspi_slave *cadence_qspi = to_cadence_qspi_slave(slave);
> + u8 opcode_rdid = 0x9F;
Is there a symbolic name for this magic value, for those of us
who don't know SPI NOR command opcodes by heart. :)
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +#if (CONFIG_CQSPI_DECODER == 1)
> + if (((cs >= 0) && (cs < CQSPI_DECODER_MAX_CS)) && ((bus >= 0) &&
> + (bus < CQSPI_DECODER_MAX_CS))) {
> + return 1;
> + }
> +#else
> + if (((cs >= 0) && (cs < CQSPI_NO_DECODER_MAX_CS)) &&
> + ((bus >= 0) && (bus < CQSPI_NO_DECODER_MAX_CS))) {
> + return 1;
> + }
> +#endif
> + printf("QSPI: Invalid bus or cs. Bus %d cs %d\n", bus, cs);
> + return 0;
> +}
It took me a while to determine how those two cases are similar
and where exactly the difference is ...
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
> + void *data_in, unsigned long flags)
> +{
> +[ ... ]
> + switch (mode) {
> + case CQSPI_STIG_READ:
> + err = cadence_qspi_apb_command_read(
> + cadence_qspi->cmd_len, cmd_buf,
> + data_bytes, data_in);
> +
> + break;
> + case CQSPI_STIG_WRITE:
> + err = cadence_qspi_apb_command_write(
> + cadence_qspi->cmd_len, cmd_buf,
> + data_bytes, data_out);
> + break;
> + case CQSPI_INDIRECT_READ:
> + err = cadence_qspi_apb_indirect_read_setup(
> + QSPI_AHB_BASE,
> + cadence_qspi->cmd_len, cmd_buf);
> + if (!err) {
> + err = cadence_qspi_apb_indirect_read_execute
> + (ahbbase, data_bytes, data_in);
> + }
> + break;
> + case CQSPI_INDIRECT_WRITE:
> + err = cadence_qspi_apb_indirect_write_setup
> + (QSPI_AHB_BASE,
> + cadence_qspi->cmd_len, cmd_buf);
> + if (!err) {
> + err = cadence_qspi_apb_indirect_write_execute
> + (ahbbase, data_bytes, data_out);
> + }
> + break;
> + default:
> + err = -1;
> + break;
> + }
funny indentation here, for most of the 'break' instructions, and
for continuation lines (and braces around single statements?)
> --- /dev/null
> +++ b/drivers/spi/cadence_qspi.h
> [ ... ]
> +
> +/*
> + * Macro required for this driver
> + *
> + * #define CONFIG_CQSPI_BASE (SOCFPGA_QSPIREGS_ADDRESS)
> + * #define CONFIG_CQSPI_AHB_BASE (SOCFPGA_QSPIDATA_ADDRESS)
> + * -> To specify 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 controller based on serial flash device timing characteristic
> + */
parens around single numbers are useless, parens in C
preprocessor macros usually are only appropriate around complex
(multi word) macro RHS, and parameters which may resolve into
multiple words
> +/* Controller's configuration and status register */
> +#define CQSPI_REG_CONFIG_CLK_POL_LSB 1
> +#define CQSPI_REG_CONFIG_CLK_PHA_LSB 2
> +#define CQSPI_REG_CONFIG_ENABLE_MASK (1 << 0)
> +#define CQSPI_REG_CONFIG_DIRECT_MASK (1 << 7)
> +#define CQSPI_REG_CONFIG_DECODE_MASK (1 << 9)
> +#define CQSPI_REG_CONFIG_XIP_IMM_MASK (1 << 18)
> +#define CQSPI_REG_CONFIG_CHIPSELECT_LSB 10
> +#define CQSPI_REG_CONFIG_BAUD_LSB 19
> +#define CQSPI_REG_CONFIG_IDLE_LSB 31
Here you seem to mix styles of declaring flag bit positions, and
flag mask values. Can you check those (and the other decls that
I did not cite for brevity), and potentially get a single style?
See if the BIT() macro can furhter improve readability.
virtually yours
Gerhard Sittig
--
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
More information about the U-Boot
mailing list