Hi, Thanks for your comment.
2013/8/9 Jagan Teki <[email protected]>: > Hi, > > > On 08-08-2013 15:02, Nobuhiro Iwamatsu wrote: >> >> This patch adds a driver for Renesas SoC's Queued SPI bus. >> This supports with 8 bits per transfer to use with SPI flash. >> >> Signed-off-by: Kouei Abe <[email protected]> >> Signed-off-by: Nobuhiro Iwamatsu <[email protected]> >> --- >> v2: Change "SH QSPI" to "SH QSPI (Queued SPI)". >> Remove magic number. >> >> drivers/spi/Makefile | 1 + >> drivers/spi/sh_qspi.c | 232 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/spi/sh_qspi.h | 52 +++++++++++ > > > Please move the structure decelerations on to sh_qspi.c OK. I will move to .c . > FYI: I am trying to prepare the spi driver code to more readable > > << header file inclusion >> > << Register bit masks >> > > << MISC macro definitions >> > > << controller reg structure >> > > << controller private slave structure >> > > << inline func defination >> > > << spi_xfer_sub() >> > > << spi_setup_slave_sub >> > > << spi_cs_is_valid >> > > << spi_cs_activate >> > > << spi_cs_deactivate >> > > << spi_init >> > > << spi_setup_slave >> > { > spi_setup_slave_sub() > } > > << spi_free_slave >> > > << spi_claim_bus >> > > << spi_release_bus >> > > << spi_xfer code >> > { > spi_xfer_sub() > } > > I am just trying to do the above format atleast from the drivers which are > pushing now onwards. > > Please see the reff driver > http://git.denx.de/?p=u-boot/u-boot-spi.git;a=commitdiff;h=1465d055f9d7a81edacf30c9d20a1b51dfcbfa8d > > Let me know your views. > > >> 3 files changed, 285 insertions(+) >> create mode 100644 drivers/spi/sh_qspi.c >> create mode 100644 drivers/spi/sh_qspi.h >> >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index 019132e..f71c089 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -33,6 +33,7 @@ COBJS-$(CONFIG_OC_TINY_SPI) += oc_tiny_spi.o >> COBJS-$(CONFIG_OMAP3_SPI) += omap3_spi.o >> COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o >> COBJS-$(CONFIG_SH_SPI) += sh_spi.o >> +COBJS-$(CONFIG_SH_QSPI) += sh_qspi.o >> COBJS-$(CONFIG_FSL_ESPI) += fsl_espi.o >> COBJS-$(CONFIG_FDT_SPI) += fdt_spi.o >> COBJS-$(CONFIG_TEGRA20_SFLASH) += tegra20_sflash.o >> diff --git a/drivers/spi/sh_qspi.c b/drivers/spi/sh_qspi.c >> new file mode 100644 >> index 0000000..25ce60a >> --- /dev/null >> +++ b/drivers/spi/sh_qspi.c >> @@ -0,0 +1,232 @@ >> +/* >> + * drivers/spi/sh_qspi.c > > Not required, i guess. Removed. > > >> + * SH QSPI (Queued SPI) driver >> + * >> + * Copyright (C) 2013 Renesas Electronics Corporation >> + * Copyright (C) 2013 Nobuhiro Iwamatsu >> <[email protected]> >> + * >> + * SPDX-License-Identifier: GPL-2.0 >> + */ >> + >> +#include <common.h> >> +#include <malloc.h> >> +#include <spi.h> >> +#include <asm/io.h> >> +#include "sh_qspi.h" >> + >> +static void sh_qspi_writeb(unsigned char data, unsigned char *reg) >> +{ >> + writeb(data, reg); >> +} >> + >> +static void sh_qspi_writew(unsigned short data, unsigned short *reg) >> +{ >> + writew(data, reg); >> +} >> + >> +static void sh_qspi_writel(unsigned long data, unsigned long *reg) >> +{ >> + writel(data, reg); >> +} >> + >> +static unsigned char sh_qspi_readb(unsigned char *reg) >> +{ >> + return readb(reg); >> +} >> + >> +static unsigned short sh_qspi_readw(unsigned short *reg) >> +{ >> + return readw(reg); >> +} >> + >> +void spi_init(void) >> +{ >> +} > > > Can you use read* and write* directly instead of sh_qspi_write* > sh_qspi_read* > > Do you have any specific reason for using these? No reason. I will change to use read* and write*. > > >> + >> +/* SPCR */ >> +#define SPCR_MSTR 0x08 >> +#define SPCR_SPE 0x40 >> +/* SPSR */ >> +#define SPSR_SPRFF 0x80 >> +#define SPSR_SPTEF 0x20 >> +/* SPPCR */ >> +#define SPPCR_IO3FV 0x04 >> +#define SPPCR_IO2FV 0x02 >> +#define SPPCR_IO1FV 0x01 >> +/* SPBDCR */ >> +#define SPBDCR_RXBC0 (1 << 0) >> +/* SPCMD */ >> +#define SPCMD_SCKDEN (1 << 15) >> +#define SPCMD_SLNDEN (1 << 14) >> +#define SPCMD_SPNDEN (1 << 13) >> +#define SPCMD_SSLKP (1 << 7) >> +#define SPCMD_BRDV0 (1 << 2) >> +#define SPCMD_INIT1 \ >> + (SPCMD_SCKDEN|SPCMD_SLNDEN|SPCMD_SPNDEN|SPCMD_SSLKP|SPCMD_BRDV0) >> +#define SPCMD_INIT2 (SPCMD_SPNDEN|SPCMD_SSLKP|SPCMD_BRDV0) >> +/* SPBFCR */ >> +#define SPBFCR_TXRST (1 << 7) >> +#define SPBFCR_RXRST (1 << 6) >> + >> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, >> + unsigned int max_hz, unsigned int mode) >> +{ >> + struct sh_qspi *ss; >> + >> + if (!spi_cs_is_valid(bus, cs)) >> + return NULL; >> + >> + ss = spi_alloc_slave(struct sh_qspi, bus, cs); >> + if (!ss) >> + return NULL; >> + >> + ss->regs = (struct sh_qspi_regs *)CONFIG_SH_QSPI_BASE; >> + >> + /* QSPI initialize */ >> + sh_qspi_writeb(SPCR_MSTR, &ss->regs->spcr); >> + sh_qspi_writeb(0x00, &ss->regs->sslp); >> + sh_qspi_writeb(SPPCR_IO3FV|SPPCR_IO2FV, &ss->regs->sppcr); >> + >> + /* Set bit rate. See 58.3.8 Quad Serial Peripheral Interface */ >> + sh_qspi_writeb(0x01, &ss->regs->spbr); >> + >> + sh_qspi_writeb(0x00, &ss->regs->spdcr); >> + sh_qspi_writeb(0x00, &ss->regs->spckd); >> + sh_qspi_writeb(0x00, &ss->regs->sslnd); >> + sh_qspi_writeb(0x00, &ss->regs->spnd); >> + sh_qspi_writew(SPCMD_INIT1, &ss->regs->spcmd0); >> + sh_qspi_writew(SPCMD_INIT2, &ss->regs->spcmd0); >> + sh_qspi_writeb(SPBFCR_TXRST|SPBFCR_RXRST, &ss->regs->spbfcr); >> + sh_qspi_writeb(0x00, &ss->regs->spbfcr); >> + sh_qspi_writeb(0x00, &ss->regs->spscr); >> + sh_qspi_writeb(SPCR_SPE|SPCR_MSTR, &ss->regs->spcr); >> + >> + return &ss->slave; >> +} >> + >> +void spi_free_slave(struct spi_slave *slave) >> +{ >> + struct sh_qspi *spi = to_sh_qspi(slave); >> + >> + free(spi); >> +} >> + >> +int spi_claim_bus(struct spi_slave *slave) >> +{ >> + return 0; >> +} >> + >> +void spi_release_bus(struct spi_slave *slave) >> +{ >> +} >> + >> +static int sh_qspi_xfer(struct sh_qspi *ss, unsigned char *tdata, >> + unsigned char *rdata, unsigned long flags) >> +{ >> + while (!(sh_qspi_readb(&ss->regs->spsr) & SPSR_SPTEF)) { >> + if (ctrlc()) >> + return 1; >> + udelay(10); >> + } >> + >> + sh_qspi_writeb(*tdata, (unsigned char *)(&ss->regs->spdr)); >> + >> + while ((sh_qspi_readw(&ss->regs->spbdcr) != SPBDCR_RXBC0)) { >> + int i = 100; >> + >> + if (ctrlc()) >> + return 1; >> + while (i--) >> + ; >> + } >> + >> + while (!(sh_qspi_readb(&ss->regs->spsr) & SPSR_SPRFF)) { >> + if (ctrlc()) >> + return 1; >> + udelay(10); >> + } >> + >> + *rdata = sh_qspi_readb((unsigned char *)(&ss->regs->spdr)); >> + >> + return 0; >> +} >> + >> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void >> *dout, >> + void *din, unsigned long flags) >> +{ >> + struct sh_qspi *ss = to_sh_qspi(slave); >> + unsigned int nbyte; >> + int ret = 0; >> + unsigned char *tdata, *rdata, dtdata = 0, drdata; >> + >> + if (dout == NULL && din == NULL) { >> + if (flags & SPI_XFER_END) >> + sh_qspi_writeb(SPCR_MSTR, &ss->regs->spcr); >> + return 0; >> + } >> + >> + if (bitlen % 8) { >> + printf("%s: bitlen is not 8bit alined %d", __func__, >> bitlen); >> + return 1; >> + } >> + >> + nbyte = bitlen / 8; >> + >> + if (flags & SPI_XFER_BEGIN) { >> + sh_qspi_writeb(SPCR_MSTR, &ss->regs->spcr); >> + >> + sh_qspi_writew(SPCMD_INIT1, &ss->regs->spcmd0); >> + >> + if (flags & SPI_XFER_END) >> + sh_qspi_writel(nbyte, &ss->regs->spbmul0); >> + else >> + /* Set 1048576 byte */ >> + sh_qspi_writel(0x100000, &ss->regs->spbmul0); >> + sh_qspi_writeb(SPBFCR_TXRST|SPBFCR_RXRST, >> &ss->regs->spbfcr); >> + sh_qspi_writeb(0x00, &ss->regs->spbfcr); >> + sh_qspi_writeb(0x00, &ss->regs->spscr); >> + sh_qspi_writeb(SPCR_SPE|SPCR_MSTR, &ss->regs->spcr); >> + } >> + >> + if (dout != NULL) >> + tdata = (unsigned char *)dout; >> + else >> + tdata = &dtdata; >> + >> + if (din != NULL) >> + rdata = din; >> + else >> + rdata = &drdata; >> + >> + while (nbyte > 0) { >> + ret = sh_qspi_xfer(ss, tdata, rdata, flags); >> + if (ret) >> + break; >> + >> + if (dout != NULL) >> + tdata++; >> + >> + if (din != NULL) >> + rdata++; >> + >> + nbyte--; >> + } >> + >> + if (flags & SPI_XFER_END) >> + sh_qspi_writeb(SPCR_MSTR, &ss->regs->spcr); >> + >> + return ret; >> +} >> + >> +int spi_cs_is_valid(unsigned int bus, unsigned int cs) >> +{ >> + return 1; >> +} >> + >> +void spi_cs_activate(struct spi_slave *slave) >> +{ >> +} >> + >> +void spi_cs_deactivate(struct spi_slave *slave) >> +{ >> +} > > > I will give few more comments, please fix these if you agree with the driver > format i suggested. > if not please let me know your views. I agreed the driver format which you propose. I will update your point and driver format, and re-send. > > -- > Thanks, > Jagan Best regards, Nobuhiro -- Nobuhiro Iwamatsu _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
