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

[Rajeshwari Birje]

Hi Wolfgang Denk,

Thank you for review comments.

On Sun, May 12, 2013 at 2:01 AM, Wolfgang Denk <wd at denx.de> wrote:
> 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.
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, &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 ??
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(&regs->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 at denx.de
> Love all, trust a few.                          - William Shakespeare
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-- 
Regards,
Rajeshwari Shinde