| Message ID | 1368285471-11039-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
Hi Rajeshwari, On Sat, May 11, 2013 at 9:17 AM, Simon Glass <sjg@chromium.org> wrote: > From: Rajeshwari Shinde <rajeshwari.s@samsung.com> > > This CL implements a custom spi_copy funtion to copy u-boot from SF to > RAM. This is faster then iROM spi_copy funtion as this runs spi at 50Mhz > and also in WORD mode of operation. > > Changed a printf in pimux.c to debug just to avoid the the compilation > error in SPL. > Removed the enum for boot mode from spl_boot.c as it was already define in spl.h > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > Signed-off-by: Tom Wai-Hong Tam <waihong@chromium.org> > Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com> > Rebased on top of MMC series: > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Changes in v2: > - Rebase on top of MMC series > - Fix new checkpatch warnings > Acked-by: Simon Glass <sjg@chromium.org> For all your SPI patches (listed below): Tested-by: Simon Glass <sjg@chromium.org> I have applied the MMC series and all of your SPI patches, and tested on snow. Everything works correctly I think. Original SPI speed is very slow: SMDK5250 # time sf read 40000000 0 400000 time: 1 minutes, 9.906 seconds, 69906 ticks With your SPI patches and my 50MHz device tree patch I get: SMDK5250 # time sf read 40000000 0 400000 time: 1.427 seconds, 1427 ticks which is a dramatic improvement. If I turn on the caches (remove CONFIG_SYS_DCACHE_OFF from include/configs/exynos5-dt.h) then it goes at full sped: SMDK5250 # time sf read 40000000 0 400000 time: 0.699 seconds, 699 ticks Note that we are working on some further SPI code improvements locally and may send patches for that, but it will be after the merge window. For references, here are the patches in my tree when I tested (listed in the reverse order they were applied). 660d9fd (HEAD, ws/snow, snow) exynos: dts: Use 50MHz SPI flash speed on snow 0295f39 EXYNOS: SPL: Add a custom spi copy function e712583 EXYNOS: SPI: Support word transfers aac5c2d EXYNOS: SPI: Minimise access to SPI FIFO level 99f2680 EXYNOS: SPI: Support a delay after deactivate cbe66cb EXYNOS: Export timer_get_us() to get microsecond timer 893d152 EXYNOS: SPI: Support SPI_PREAMBLE mode f4cce8b SPI: Add support for preamble bytes f91072a exynos: Enable mmc for snow aa1b39f COMMON: MMC: Command to support EMMC booting and to resize EMMC boot partition a6358a3 SMDK5250: Enable EMMC booting e37cc9f MMC: APIs to support resize of EMMC boot partition 919c7f6 SMDK5250: Initialise and Enable DWMMC, support FDT and non-FDT 2277040 EXYNOS5: DWMMC: Initialise the local variable to avoid unwanted results. 206af80 EXYNOS5: DWMMC: Added FDT support for DWMMC 0e8e41c DWMMC: Initialise dwmci and resolve EMMC read write issues 087075c EXYNOS5: FDT: Add DWMMC device node data 0f2db62 FDT: Add compatible string for DWMMC 6e4e37c EXYNOS5: I2C: Add FDT and non-FDT support for I2C > arch/arm/cpu/armv7/exynos/pinmux.c | 2 +- > arch/arm/include/asm/arch-exynos/spi.h | 2 + > arch/arm/include/asm/arch-exynos/spl.h | 1 + > board/samsung/smdk5250/spl_boot.c | 132 +++++++++++++++++++++++++++++---- > include/configs/exynos5250-dt.h | 3 + > spl/Makefile | 4 + > 6 files changed, 129 insertions(+), 15 deletions(-) > Regards, Simon
Dear Simon Glass, In message <1368285471-11039-1-git-send-email-sjg@chromium.org> you wrote: > From: Rajeshwari Shinde <rajeshwari.s@samsung.com> > > This CL implements a custom spi_copy funtion to copy u-boot from SF to What is a "CL" ? > Changed a printf in pimux.c to debug just to avoid the the compilation s/the the/the/ ?? > Removed the enum for boot mode from spl_boot.c as it was already define in spl.h s/define/defined/ Line length in commit messages should be not more than 72 characters. > +/** > + * Copy uboot from spi flash to RAM > + * > + * @parma uboot_size size of u-boot to copy > + * @param uboot_addr address of u-boot to copy > + */ Incorrect multiline comment style. > +static void exynos_spi_copy(unsigned int uboot_size, unsigned int uboot_addr) > +{ > + int upto, todo; > + int i; > + struct exynos_spi *regs = (struct exynos_spi *)CONFIG_ENV_SPI_BASE; > + > + set_spi_clk(PERIPH_ID_SPI1, 50000000); /* set spi clock to 50Mhz */ > + /* set the spi1 GPIO */ > + exynos_pinmux_config(PERIPH_ID_SPI1, PINMUX_FLAG_NONE); > + > + /* set pktcnt and enable it */ > + writel(4 | SPI_PACKET_CNT_EN, ®s->pkt_cnt); > + /* set FB_CLK_SEL */ > + writel(SPI_FB_DELAY_180, ®s->fb_clk); > + /* set CH_WIDTH and BUS_WIDTH as word */ > + setbits_le32(®s->mode_cfg, SPI_MODE_CH_WIDTH_WORD | > + SPI_MODE_BUS_WIDTH_WORD); > + clrbits_le32(®s->ch_cfg, SPI_CH_CPOL_L); /* CPOL: active high */ > + > + /* clear rx and tx channel if set priveously */ > + clrbits_le32(®s->ch_cfg, SPI_RX_CH_ON | SPI_TX_CH_ON); > + > + typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst); We do not allow typedefs or variable declarations etc. right in the middle of the code. I'm surprised that checkpatch does not complain here about not to add new typedefs ?? > + /* waiting for TX done */ > + while (!(readl(®s->spi_sts) & SPI_ST_TX_DONE)) > + ; Potentially infinite loop. This (and similar code) should always have a timeout and appropriate error handling. > + /* let us our own function to copy u-boot from SF */ Please fix the wording of this comment. > diff --git a/spl/Makefile b/spl/Makefile > index b5a8de7..5e7816a 100644 > --- a/spl/Makefile > +++ b/spl/Makefile > @@ -94,6 +94,10 @@ LIBS-y += arch/$(ARCH)/cpu/tegra-common/libcputegra-common.o > LIBS-y += $(CPUDIR)/tegra-common/libtegra-common.o > endif > > +ifneq ($(CONFIG_EXYNOS4)$(CONFIG_EXYNOS5),) > +LIBS-y += $(CPUDIR)/s5p-common/libs5p-common.o > +endif This should be done without the "ifneq". Best regards, Wolfgang Denk
Hi Wolfgang Denk, Thank you for review comments. On Sun, May 12, 2013 at 2:01 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <1368285471-11039-1-git-send-email-sjg@chromium.org> you wrote: >> From: Rajeshwari Shinde <rajeshwari.s@samsung.com> >> >> This CL implements a custom spi_copy funtion to copy u-boot from SF to > > What is a "CL" ? > >> Changed a printf in pimux.c to debug just to avoid the the compilation > > s/the the/the/ ?? > >> Removed the enum for boot mode from spl_boot.c as it was already define in spl.h > > s/define/defined/ > > Line length in commit messages should be not more than 72 characters. Will correct these issues and resubmit the patch set soon. > > >> +/** >> + * Copy uboot from spi flash to RAM >> + * >> + * @parma uboot_size size of u-boot to copy >> + * @param uboot_addr address of u-boot to copy >> + */ > > Incorrect multiline comment style. Will correct these > >> +static void exynos_spi_copy(unsigned int uboot_size, unsigned int uboot_addr) >> +{ >> + int upto, todo; >> + int i; >> + struct exynos_spi *regs = (struct exynos_spi *)CONFIG_ENV_SPI_BASE; >> + >> + set_spi_clk(PERIPH_ID_SPI1, 50000000); /* set spi clock to 50Mhz */ >> + /* set the spi1 GPIO */ >> + exynos_pinmux_config(PERIPH_ID_SPI1, PINMUX_FLAG_NONE); >> + >> + /* set pktcnt and enable it */ >> + writel(4 | SPI_PACKET_CNT_EN, ®s->pkt_cnt); >> + /* set FB_CLK_SEL */ >> + writel(SPI_FB_DELAY_180, ®s->fb_clk); >> + /* set CH_WIDTH and BUS_WIDTH as word */ >> + setbits_le32(®s->mode_cfg, SPI_MODE_CH_WIDTH_WORD | >> + SPI_MODE_BUS_WIDTH_WORD); >> + clrbits_le32(®s->ch_cfg, SPI_CH_CPOL_L); /* CPOL: active high */ >> + >> + /* clear rx and tx channel if set priveously */ >> + clrbits_le32(®s->ch_cfg, SPI_RX_CH_ON | SPI_TX_CH_ON); >> + >> + typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst); > > We do not allow typedefs or variable declarations etc. right in the > middle of the code. > > I'm surprised that checkpatch does not complain here about not to add > new typedefs ?? Basically typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst); was removed in my original patch. Will check and remove this from the version of patch set I submit. > >> + /* waiting for TX done */ >> + while (!(readl(®s->spi_sts) & SPI_ST_TX_DONE)) >> + ; > > Potentially infinite loop. This (and similar code) should always have > a timeout and appropriate error handling. Will add a timeout check here > >> + /* let us our own function to copy u-boot from SF */ > > Please fix the wording of this comment. Will correct this. > >> diff --git a/spl/Makefile b/spl/Makefile >> index b5a8de7..5e7816a 100644 >> --- a/spl/Makefile >> +++ b/spl/Makefile >> @@ -94,6 +94,10 @@ LIBS-y += arch/$(ARCH)/cpu/tegra-common/libcputegra-common.o >> LIBS-y += $(CPUDIR)/tegra-common/libtegra-common.o >> endif >> >> +ifneq ($(CONFIG_EXYNOS4)$(CONFIG_EXYNOS5),) >> +LIBS-y += $(CPUDIR)/s5p-common/libs5p-common.o >> +endif > > This should be done without the "ifneq". This is specific to samsung and currently used only by EXYNOS hence it was added with a ifneq > > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > Love all, trust a few. - William Shakespeare > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
diff --git a/arch/arm/cpu/armv7/exynos/pinmux.c b/arch/arm/cpu/armv7/exynos/pinmux.c index bd499b4..c484a86 100644 --- a/arch/arm/cpu/armv7/exynos/pinmux.c +++ b/arch/arm/cpu/armv7/exynos/pinmux.c @@ -427,7 +427,7 @@ static int exynos4_pinmux_config(int peripheral, int flags) case PERIPH_ID_SDMMC1: case PERIPH_ID_SDMMC3: case PERIPH_ID_SDMMC4: - printf("SDMMC device %d not implemented\n", peripheral); + debug("SDMMC device %d not implemented\n", peripheral); return -1; default: debug("%s: invalid peripheral %d", __func__, peripheral); diff --git a/arch/arm/include/asm/arch-exynos/spi.h b/arch/arm/include/asm/arch-exynos/spi.h index e67ad27..3430ac1 100644 --- a/arch/arm/include/asm/arch-exynos/spi.h +++ b/arch/arm/include/asm/arch-exynos/spi.h @@ -43,6 +43,8 @@ struct exynos_spi { #define SPI_TIMEOUT_MS 10 +#define SF_READ_DATA_CMD 0x3 + /* SPI_CHCFG */ #define SPI_CH_HS_EN (1 << 6) #define SPI_CH_RST (1 << 5) diff --git a/arch/arm/include/asm/arch-exynos/spl.h b/arch/arm/include/asm/arch-exynos/spl.h index 46b25a6..59bb7e0 100644 --- a/arch/arm/include/asm/arch-exynos/spl.h +++ b/arch/arm/include/asm/arch-exynos/spl.h @@ -32,6 +32,7 @@ enum boot_mode { * pin values are the same across Exynos4 and Exynos5. */ BOOT_MODE_MMC = 4, + BOOT_MODE_EMMC = 8, /* EMMC4.4 */ BOOT_MODE_SERIAL = 20, /* Boot based on Operating Mode pin settings */ BOOT_MODE_OM = 32, diff --git a/board/samsung/smdk5250/spl_boot.c b/board/samsung/smdk5250/spl_boot.c index 98f2286..8699d1d 100644 --- a/board/samsung/smdk5250/spl_boot.c +++ b/board/samsung/smdk5250/spl_boot.c @@ -22,6 +22,12 @@ #include<common.h> #include<config.h> +#include <spi.h> +#include <asm/arch/clk.h> +#include <asm/arch/spi.h> +#include <asm/arch/pinmux.h> +#include <asm/arch/periph.h> +#include <asm/arch/spl.h> #include <asm/arch-exynos/dmc.h> #include <asm/arch/clock.h> @@ -48,15 +54,6 @@ u32 irom_ptr_table[] = { [USB_INDEX] = 0x02020070, /* iROM Function Pointer-USB boot*/ }; -enum boot_mode { - BOOT_MODE_MMC = 4, - BOOT_MODE_SERIAL = 20, - BOOT_MODE_EMMC = 8, /* EMMC4.4 */ - /* Boot based on Operating Mode pin settings */ - BOOT_MODE_OM = 32, - BOOT_MODE_USB, /* Boot using USB download */ -}; - void *get_irom_func(int index) { return (void *)*(u32 *)irom_ptr_table[index]; @@ -76,6 +73,115 @@ static int config_branch_prediction(int set_cr_z) return cr & CR_Z; } +static void spi_rx_tx(struct exynos_spi *regs, int todo, + void *dinp, void const *doutp, int i) +{ + uint *rxp = (uint *)(dinp + (i * (32 * 1024))); + int rx_lvl, tx_lvl; + uint out_bytes, in_bytes; + + in_bytes = todo; + out_bytes = todo; + setbits_le32(®s->ch_cfg, SPI_CH_RST); + clrbits_le32(®s->ch_cfg, SPI_CH_RST); + writel(((todo * 8) / 32) | SPI_PACKET_CNT_EN, ®s->pkt_cnt); + + while (in_bytes) { + uint32_t spi_sts; + int temp; + + spi_sts = readl(®s->spi_sts); + rx_lvl = ((spi_sts >> 15) & 0x7f); + tx_lvl = ((spi_sts >> 6) & 0x7f); + while (tx_lvl < 32 && out_bytes) { + temp = 0xffffffff; + writel(temp, ®s->tx_data); + out_bytes -= 4; + tx_lvl += 4; + } + while (rx_lvl >= 4 && in_bytes) { + temp = readl(®s->rx_data); + if (rxp) + *rxp++ = temp; + in_bytes -= 4; + rx_lvl -= 4; + } + } +} + +/** + * Copy uboot from spi flash to RAM + * + * @parma uboot_size size of u-boot to copy + * @param uboot_addr address of u-boot to copy + */ +static void exynos_spi_copy(unsigned int uboot_size, unsigned int uboot_addr) +{ + int upto, todo; + int i; + struct exynos_spi *regs = (struct exynos_spi *)CONFIG_ENV_SPI_BASE; + + set_spi_clk(PERIPH_ID_SPI1, 50000000); /* set spi clock to 50Mhz */ + /* set the spi1 GPIO */ + exynos_pinmux_config(PERIPH_ID_SPI1, PINMUX_FLAG_NONE); + + /* set pktcnt and enable it */ + writel(4 | SPI_PACKET_CNT_EN, ®s->pkt_cnt); + /* set FB_CLK_SEL */ + writel(SPI_FB_DELAY_180, ®s->fb_clk); + /* set CH_WIDTH and BUS_WIDTH as word */ + setbits_le32(®s->mode_cfg, SPI_MODE_CH_WIDTH_WORD | + SPI_MODE_BUS_WIDTH_WORD); + clrbits_le32(®s->ch_cfg, SPI_CH_CPOL_L); /* CPOL: active high */ + + /* clear rx and tx channel if set priveously */ + clrbits_le32(®s->ch_cfg, SPI_RX_CH_ON | SPI_TX_CH_ON); + + typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst); + setbits_le32(®s->swap_cfg, SPI_RX_SWAP_EN | + SPI_RX_BYTE_SWAP | + SPI_RX_HWORD_SWAP); + + /* do a soft reset */ + setbits_le32(®s->ch_cfg, SPI_CH_RST); + clrbits_le32(®s->ch_cfg, SPI_CH_RST); + + /* now set rx and tx channel ON */ + setbits_le32(®s->ch_cfg, SPI_RX_CH_ON | SPI_TX_CH_ON | SPI_CH_HS_EN); + clrbits_le32(®s->cs_reg, SPI_SLAVE_SIG_INACT); /* CS low */ + + /* Send read instruction (0x3h) followed by a 24 bit addr */ + writel((SF_READ_DATA_CMD << 24) | SPI_FLASH_UBOOT_POS, ®s->tx_data); + + /* waiting for TX done */ + while (!(readl(®s->spi_sts) & SPI_ST_TX_DONE)) + ; + + for (upto = 0, i = 0; upto < uboot_size; upto += todo, i++) { + todo = min(uboot_size - upto, (1 << 15)); + spi_rx_tx(regs, todo, (void *)(uboot_addr), + (void *)(SPI_FLASH_UBOOT_POS), i); + } + + setbits_le32(®s->cs_reg, SPI_SLAVE_SIG_INACT);/* make the CS high */ + + /* + * Let put controller mode to BYTE as + * SPI driver does not support WORD mode yet + */ + clrbits_le32(®s->mode_cfg, SPI_MODE_CH_WIDTH_WORD | + SPI_MODE_BUS_WIDTH_WORD); + writel(0, ®s->swap_cfg); + + /* + * Flush spi tx, rx fifos and reset the SPI controller + * and clear rx/tx channel + */ + clrsetbits_le32(®s->ch_cfg, SPI_CH_HS_EN, SPI_CH_RST); + clrbits_le32(®s->ch_cfg, SPI_CH_RST); + clrbits_le32(®s->ch_cfg, SPI_TX_CH_ON | SPI_RX_CH_ON); +} + /* * Copy U-boot from mmc to RAM: * COPY_BL2_FNPTR_ADDR: Address in iRAM, which Contains @@ -86,8 +192,7 @@ void copy_uboot_to_ram(void) int is_cr_z_set; unsigned int sec_boot_check; enum boot_mode bootmode = BOOT_MODE_OM; - - u32 (*spi_copy)(u32 offset, u32 nblock, u32 dst); + struct spl_machine_param *param = spl_get_machine_params(); u32 (*copy_bl2)(u32 offset, u32 nblock, u32 dst); u32 (*copy_bl2_from_emmc)(u32 nblock, u32 dst); void (*end_bootop_from_emmc)(void); @@ -103,9 +208,8 @@ void copy_uboot_to_ram(void) switch (bootmode) { case BOOT_MODE_SERIAL: - spi_copy = get_irom_func(SPI_INDEX); - spi_copy(SPI_FLASH_UBOOT_POS, CONFIG_BL2_SIZE, - CONFIG_SYS_TEXT_BASE); + /* let us our own function to copy u-boot from SF */ + exynos_spi_copy(param->uboot_size, CONFIG_SYS_TEXT_BASE); break; case BOOT_MODE_MMC: copy_bl2 = get_irom_func(MMC_INDEX); diff --git a/include/configs/exynos5250-dt.h b/include/configs/exynos5250-dt.h index 5262e41..e9b0767 100644 --- a/include/configs/exynos5250-dt.h +++ b/include/configs/exynos5250-dt.h @@ -152,6 +152,8 @@ /* MMC SPL */ #define CONFIG_SPL +#define CONFIG_SPL_GPIO_SUPPORT + #define COPY_BL2_FNPTR_ADDR 0x02020030 /* specific .lds file */ @@ -305,6 +307,7 @@ #define CONFIG_ENV_SPI_MODE SPI_MODE_0 #define CONFIG_ENV_SECT_SIZE CONFIG_ENV_SIZE #define CONFIG_ENV_SPI_BUS 1 +#define CONFIG_ENV_SPI_BASE 0x12D30000 #define CONFIG_ENV_SPI_MAX_HZ 50000000 #endif diff --git a/spl/Makefile b/spl/Makefile index b5a8de7..5e7816a 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -94,6 +94,10 @@ LIBS-y += arch/$(ARCH)/cpu/tegra-common/libcputegra-common.o LIBS-y += $(CPUDIR)/tegra-common/libtegra-common.o endif +ifneq ($(CONFIG_EXYNOS4)$(CONFIG_EXYNOS5),) +LIBS-y += $(CPUDIR)/s5p-common/libs5p-common.o +endif + # Add GCC lib ifeq ("$(USE_PRIVATE_LIBGCC)", "yes") PLATFORM_LIBGCC = $(SPLTREE)/arch/$(ARCH)/lib/libgcc.o