[U-Boot] [PATCH v2] EXYNOS: SPL: Add a custom spi copy function

[Wolfgang Denk]

Dear Simon Glass,

In message <1368285471-11039-1-git-send-email-sjg at chromium.org> you wrote:
> From: Rajeshwari Shinde <rajeshwari.s at 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, &regs->pkt_cnt);
> +	/* set FB_CLK_SEL */
> +	writel(SPI_FB_DELAY_180, &regs->fb_clk);
> +	/* set CH_WIDTH and BUS_WIDTH as word */
> +	setbits_le32(&regs->mode_cfg, SPI_MODE_CH_WIDTH_WORD |
> +					SPI_MODE_BUS_WIDTH_WORD);
> +	clrbits_le32(&regs->ch_cfg, SPI_CH_CPOL_L); /* CPOL: active high */
> +
> +	/* clear rx and tx channel if set priveously */
> +	clrbits_le32(&regs->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(&regs->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

-- 
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 at denx.de
Love all, trust a few.                          - William Shakespeare