From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher denx Date: Tue, 16 Jun 2015 11:18:01 +0200 Subject: [U-Boot] [PATCH v6 1/4] mtd, spi: add MTD layer driver In-Reply-To: References: <1430113328-29184-1-git-send-email-hs@denx.de> <1430113328-29184-2-git-send-email-hs@denx.de> <555C2DC3.6030400@denx.de> <557FE1C6.6010701@denx.de> Message-ID: <557FE9C9.3050001@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Jagan, Am 16.06.2015 um 10:52 schrieb Jagan Teki: > Hi Heiko, > > On 16 June 2015 at 14:13, Heiko Schocher denx wrote: >> Hello Jagan, >> >> >> Am 16.06.2015 um 10:04 schrieb Jagan Teki: >>> >>> Hi Heiko, >>> >>> On 20 May 2015 at 12:16, Heiko Schocher wrote: >>>> >>>> Hello Jagan, >>>> >>>> Am 19.05.2015 22:09, schrieb Jagan Teki: >>>>> >>>>> >>>>> Hi Heiko, >>>>> >>>>> I have tested this sf-mtd stuff, please see below and enabled prints >>>>> in all the func calls. >>>> >>>> >>>> >>>> Thanks for testing! >>>> >>>> >>>>> zynq-uboot> mtdparts add nor0 0x10000 at 0x0 env >>>>> mtdparts variable not set, see 'help mtdparts' >>>>> zynq-uboot> mtdparts >>>>> >>>>> device nor0 , # parts = 1 >>>>> #: name size offset mask_flags >>>>> 0: env 0x00010000 0x00000000 0 >>>>> >>>>> active partition: nor0,0 - (env) 0x00010000 @ 0x00000000 >>>>> >>>>> defaults: >>>>> mtdids : nor0=zynq-sf.0 >>>>> mtdparts: none >>>>> zynq-uboot> sf erase env 0x10000 >>>>> spi_flash_erase >>>>> spi_flash_cmd_erase_ops >>>>> SF: erase d8 0 0 0 (0) >>>>> SF: 65536 bytes @ 0x0 Erased: OK >>>>> zynq-uboot> mw.b 0x100 0x44 0x10000 >>>>> zynq-uboot> sf write 0x100 env >>>>> device 0 offset 0x0, size 0x10000 >>>>> spi_flash_cmd_write_ops >>>>> SF: 65536 bytes @ 0x0 Written: OK >>>>> zynq-uboot> sf read 0x40000 env >>>>> device 0 offset 0x0, size 0x10000 >>>>> spi_flash_cmd_read_ops >>>>> off = 0x10000, len = 0 >>>>> SF: 65536 bytes @ 0x0 Read: OK >>>>> zynq-uboot> cmp.b 0x100 0x40000 0x10000 >>>>> Total of 65536 byte(s) were the same >>>> >>>> >>>> >>>> Looks good ... >>>> >>>>> I wonder none of the sf_mtd_info._* getting called, why? >>>> >>>> >>>> >>>> Hmm.. good question .. you use the "sf ..." commands, they do not >>>> use the mtd interface, right? >>> >>> >>> I'm fine with the testing, but mtd code in sf seems used only for in UBI >>> only. >> >> >> a fast "grep mtd_read" in the u-boot source shows, that yaffs2 >> also uses mtd_read ... I did no yaffs2 test, but I think, yaffs2 can >> be used also with spi flashes now ... if this is wise, I don;t know ... >> >> I tested with UBI on a spi flash, and that works ... in the same >> fashion it currently does on nand for example ... >> >>> I wouldn't see this is a better approach where mtd code is considered as >>> to >>> be unknown thing for sf. >> >> >> I do not understand you here complete ... >> >> drivers/mtd/spi/sf_mtd.c >> >> adds just spi flash specific functions to integrate spi flashes >> into mtd ... and mtd users can then read/write/erase >> with the mtd_* functions ... >> >> Maybe someone has time to convert common/sf.c to use them? >> >> So, the final question is, can this patches go into mainline? > > The only point I'm concerned here is If I need to use sf with mtd support > without using ubifs or any flash filesystem, the code in sf_mtd.c ops becomes > unused. Why you want to enable mtd support for sf, if you not use it? do not define CONFIG_SPI_FLASH_MTD in this case? > This seems to be a code size increasing factor which is obviously not a good > point for bootloader atleast for u-boot, what do you think? > > I agreed your concerned for someone may add support to common/cmd_sf.c > in future, but I'm bit worried to go this. Why? Its the same state as it is in the nand subsystem ... bye, Heiko > >> >>>> I tested this functions with using UBI on the SPI NOR on the >>>> aristainetos and aristianetos2 boards... I added for example in >>>> >>>> diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c >>>> index 0b9cb62..6063ed7 100644 >>>> --- a/drivers/mtd/spi/sf_mtd.c >>>> +++ b/drivers/mtd/spi/sf_mtd.c >>>> @@ -39,6 +40,7 @@ static int spi_flash_mtd_read(struct mtd_info *mtd, >>>> loff_t >>>> from, size_t len, >>>> struct spi_flash *flash = mtd->priv; >>>> int err; >>>> >>>> +printf("%s ****\n", __func__); >>>> err = spi_flash_read(flash, from, len, buf); >>>> if (!err) >>>> *retlen = len; >>>> >>>> and see: >>>> >>>> => sf probe >>>> SF: Detected N25Q128A with page size 256 Bytes, erase size 64 KiB, total >>>> 16 >>>> MiB >>>> => mtdparts >>>> >>>> device nor0 , # parts = 4 >>>> #: name size offset mask_flags >>>> 0: u-boot 0x000d0000 0x00000000 0 >>>> 1: env 0x00010000 0x000d0000 0 >>>> 2: env-red 0x00010000 0x000e0000 0 >>>> 3: rescue-system 0x00f10000 0x000f0000 0 >>>> >>>> device nand0 , # parts = 1 >>>> #: name size offset mask_flags >>>> 0: ubi 0x40000000 0x00000000 0 >>>> >>>> active partition: nor0,0 - (u-boot) 0x000d0000 @ 0x00000000 >>>> >>>> defaults: >>>> mtdids : none >>>> mtdparts: none >>>> => ubi part rescue-system >>>> UBI: default fastmap pool size: 10 >>>> UBI: default fastmap WL pool size: 25 >>>> UBI: attaching mtd2 to ubi0 >>>> UBI DBG gen (pid 1): sizeof(struct ubi_ainf_peb) 48 >>>> UBI DBG gen (pid 1): sizeof(struct ubi_wl_entry) 20 >>>> UBI DBG gen (pid 1): min_io_size 1 >>>> UBI DBG gen (pid 1): max_write_size 256 >>>> UBI DBG gen (pid 1): hdrs_min_io_size 1 >>>> UBI DBG gen (pid 1): ec_hdr_alsize 64 >>>> UBI DBG gen (pid 1): vid_hdr_alsize 64 >>>> UBI DBG gen (pid 1): vid_hdr_offset 64 >>>> UBI DBG gen (pid 1): vid_hdr_aloffset 64 >>>> UBI DBG gen (pid 1): vid_hdr_shift 0 >>>> UBI DBG gen (pid 1): leb_start 128 >>>> UBI DBG gen (pid 1): max_erroneous 24 >>>> UBI DBG gen (pid 1): process PEB 0 >>>> UBI DBG bld (pid 1): scan PEB 0 >>>> UBI DBG io (pid 1): read EC header from PEB 0 >>>> UBI DBG io (pid 1): read 64 bytes from PEB 0:0 >>>> spi_flash_mtd_read **** >>>> UBI DBG io (pid 1): read VID header from PEB 0 >>>> UBI DBG io (pid 1): read 64 bytes from PEB 0:64 >>>> spi_flash_mtd_read **** >>>> [...] >>>> >>>> UBI uses the MTD layer ... the sf command not ... >>>> Hope this helps? >>>> >>>> bye, >>>> Heiko >>>> >>>> >>>>> On 27 April 2015 at 11:12, Heiko Schocher wrote: >>>>>> >>>>>> >>>>>> From: Daniel Schwierzeck >>>>>> >>>>>> add MTD layer driver for spi, original patch from: >>>>>> >>>>>> >>>>>> http://git.denx.de/?p=u-boot/u-boot-mips.git;a=commitdiff;h=bb246819cdc90493dd7089eaa51b9e639765cced >>>>>> >>>>>> changes from Heiko Schocher against this patch: >>>>>> - remove compile error if not defining CONFIG_SPI_FLASH_MTD: >>>>>> >>>>>> LD drivers/mtd/spi/built-in.o >>>>>> drivers/mtd/spi/sf_probe.o: In function `spi_flash_mtd_unregister': >>>>>> /home/hs/abb/imx6/u-boot/drivers/mtd/spi/sf_internal.h:168: multiple >>>>>> definition of `spi_flash_mtd_unregister' >>>>>> >>>>>> >>>>>> drivers/mtd/spi/sf_params.o:/home/hs/abb/imx6/u-boot/drivers/mtd/spi/sf_internal.h:168: >>>>>> first defined here >>>>>> drivers/mtd/spi/sf_ops.o: In function `spi_flash_mtd_unregister': >>>>>> /home/hs/abb/imx6/u-boot/drivers/mtd/spi/sf_internal.h:168: multiple >>>>>> definition of `spi_flash_mtd_unregister' >>>>>> >>>>>> >>>>>> drivers/mtd/spi/sf_params.o:/home/hs/abb/imx6/u-boot/drivers/mtd/spi/sf_internal.h:168: >>>>>> first defined here >>>>>> make[1]: *** [drivers/mtd/spi/built-in.o] Fehler 1 >>>>>> make: *** [drivers/mtd/spi] Fehler 2 >>>>>> >>>>>> - add a README entry. >>>>>> - add correct writebufsize, to fit with Linux v3.14 >>>>>> MTD, UBI/UBIFS sync. >>>>>> >>>>>> Signed-off-by: Daniel Schwierzeck >>>>>> Signed-off-by: Heiko Schocher >>>>>> >>>>>> --- >>>>>> >>>>>> Changes in v6: None >>>>>> Changes in v2: >>>>>> - add comment from Daniel Schwierzeck: >>>>>> fix compile error from original patch with >>>>>> "static inline" rather than "static __maybe_unused" >>>>>> Series-changes: 3 >>>>>> - rebase with d6c1ffc7d23f4fe4ae8c91101861055b8e1501b6 >>>>>> Series-changes: 4 >>>>>> - rebased against 385a08a60f042061b004642d6b9bb6cfb794ad5a >>>>>> Series-changes: 5 >>>>>> - no changes >>>>>> Series-changes: 6 >>>>>> - add comments from Jagan Teki: >>>>>> move code, which checks if flash pointer is used >>>>>> into a new patch. >>>>>> - use #ifdef in Code >>>>>> - call mtd register before the spi_release_bus >>>>>> >>>>>> README | 3 ++ >>>>>> common/cmd_sf.c | 2 - >>>>>> drivers/mtd/spi/Makefile | 1 + >>>>>> drivers/mtd/spi/sf_internal.h | 5 ++ >>>>>> drivers/mtd/spi/sf_mtd.c | 104 >>>>>> ++++++++++++++++++++++++++++++++++++++++++ >>>>>> drivers/mtd/spi/sf_probe.c | 10 ++-- >>>>>> 6 files changed, 119 insertions(+), 6 deletions(-) >>>>>> create mode 100644 drivers/mtd/spi/sf_mtd.c >>>>>> >>>>>> diff --git a/README b/README >>>>>> index fc1fd52..36f6fc9 100644 >>>>>> --- a/README >>>>>> +++ b/README >>>>>> @@ -3097,6 +3097,9 @@ CBFS (Coreboot Filesystem) support >>>>>> operation will not execute. The only way to exit this >>>>>> hardware-protected mode is to drive W#/VPP HIGH. >>>>>> >>>>>> + CONFIG_SPI_FLASH_MTD >>>>>> + add MTD translation layer driver. >>>>>> + >>>>>> - SystemACE Support: >>>>>> CONFIG_SYSTEMACE >>>>>> >>>>>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c >>>>>> index 6aabf39..0250011 100644 >>>>>> --- a/common/cmd_sf.c >>>>>> +++ b/common/cmd_sf.c >>>>>> @@ -139,8 +139,6 @@ static int do_spi_flash_probe(int argc, char * >>>>>> const >>>>>> argv[]) >>>>>> return 1; >>>>>> } >>>>>> >>>>>> - if (flash) >>>>>> - spi_flash_free(flash); >>>>>> flash = new; >>>>>> #endif >>>>>> >>>>>> diff --git a/drivers/mtd/spi/Makefile b/drivers/mtd/spi/Makefile >>>>>> index c61b784e..f8580cd 100644 >>>>>> --- a/drivers/mtd/spi/Makefile >>>>>> +++ b/drivers/mtd/spi/Makefile >>>>>> @@ -17,5 +17,6 @@ obj-$(CONFIG_SPI_FLASH) += sf_probe.o >>>>>> #endif >>>>>> obj-$(CONFIG_CMD_SF) += sf.o >>>>>> obj-$(CONFIG_SPI_FLASH) += sf_ops.o sf_params.o >>>>>> +obj-$(CONFIG_SPI_FLASH_MTD) += sf_mtd.o >>>>>> obj-$(CONFIG_SPI_FLASH_SANDBOX) += sandbox.o >>>>>> obj-$(CONFIG_SPI_M95XXX) += eeprom_m95xxx.o >>>>>> diff --git a/drivers/mtd/spi/sf_internal.h >>>>>> b/drivers/mtd/spi/sf_internal.h >>>>>> index 785f7a9..8a2eb6e 100644 >>>>>> --- a/drivers/mtd/spi/sf_internal.h >>>>>> +++ b/drivers/mtd/spi/sf_internal.h >>>>>> @@ -221,4 +221,9 @@ int spi_flash_read_common(struct spi_flash *flash, >>>>>> const u8 *cmd, >>>>>> int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset, >>>>>> size_t len, void *data); >>>>>> >>>>>> +#ifdef CONFIG_SPI_FLASH_MTD >>>>>> +int spi_flash_mtd_register(struct spi_flash *flash); >>>>>> +void spi_flash_mtd_unregister(void); >>>>>> +#endif >>>>>> + >>>>>> #endif /* _SF_INTERNAL_H_ */ >>>>>> diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c >>>>>> new file mode 100644 >>>>>> index 0000000..0b9cb62 >>>>>> --- /dev/null >>>>>> +++ b/drivers/mtd/spi/sf_mtd.c >>>>>> @@ -0,0 +1,104 @@ >>>>>> +/* >>>>>> + * Copyright (C) 2012-2014 Daniel Schwierzeck, >>>>>> daniel.schwierzeck at gmail.com >>>>>> + * >>>>>> + * SPDX-License-Identifier: GPL-2.0+ >>>>>> + */ >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +static struct mtd_info sf_mtd_info; >>>>>> +static char sf_mtd_name[8]; >>>>>> + >>>>>> +static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info >>>>>> *instr) >>>>>> +{ >>>>>> + struct spi_flash *flash = mtd->priv; >>>>>> + int err; >>>>>> + >>>>>> + instr->state = MTD_ERASING; >>>>>> + >>>>>> + err = spi_flash_erase(flash, instr->addr, instr->len); >>>>>> + if (err) { >>>>>> + instr->state = MTD_ERASE_FAILED; >>>>>> + instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN; >>>>>> + return -EIO; >>>>>> + } >>>>>> + >>>>>> + instr->state = MTD_ERASE_DONE; >>>>>> + mtd_erase_callback(instr); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int spi_flash_mtd_read(struct mtd_info *mtd, loff_t from, >>>>>> size_t >>>>>> len, >>>>>> + size_t *retlen, u_char *buf) >>>>>> +{ >>>>>> + struct spi_flash *flash = mtd->priv; >>>>>> + int err; >>>>>> + >>>>>> + err = spi_flash_read(flash, from, len, buf); >>>>>> + if (!err) >>>>>> + *retlen = len; >>>>>> + >>>>>> + return err; >>>>>> +} >>>>>> + >>>>>> +static int spi_flash_mtd_write(struct mtd_info *mtd, loff_t to, size_t >>>>>> len, >>>>>> + size_t *retlen, const u_char *buf) >>>>>> +{ >>>>>> + struct spi_flash *flash = mtd->priv; >>>>>> + int err; >>>>>> + >>>>>> + err = spi_flash_write(flash, to, len, buf); >>>>>> + if (!err) >>>>>> + *retlen = len; >>>>>> + >>>>>> + return err; >>>>>> +} >>>>>> + >>>>>> +static void spi_flash_mtd_sync(struct mtd_info *mtd) >>>>>> +{ >>>>>> +} >>>>>> + >>>>>> +static int spi_flash_mtd_number(void) >>>>>> +{ >>>>>> +#ifdef CONFIG_SYS_MAX_FLASH_BANKS >>>>>> + return CONFIG_SYS_MAX_FLASH_BANKS; >>>>>> +#else >>>>>> + return 0; >>>>>> +#endif >>>>>> +} >>>>>> + >>>>>> +int spi_flash_mtd_register(struct spi_flash *flash) >>>>>> +{ >>>>>> + memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); >>>>>> + sprintf(sf_mtd_name, "nor%d", spi_flash_mtd_number()); >>>>>> + >>>>>> + sf_mtd_info.name = sf_mtd_name; >>>>>> + sf_mtd_info.type = MTD_NORFLASH; >>>>>> + sf_mtd_info.flags = MTD_CAP_NORFLASH; >>>>>> + sf_mtd_info.writesize = 1; >>>>>> + sf_mtd_info.writebufsize = flash->page_size; >>>>>> + >>>>>> + sf_mtd_info._erase = spi_flash_mtd_erase; >>>>>> + sf_mtd_info._read = spi_flash_mtd_read; >>>>>> + sf_mtd_info._write = spi_flash_mtd_write; >>>>>> + sf_mtd_info._sync = spi_flash_mtd_sync; >>>>> >>>>> >>>>> >>>>> Even if I remove this every thing as usual, like "sf erase" will call >>>>> from cmd_sf >>>>> there it calls mtd_arg_off_size, to detected off and size from >>>>> partition and after >>>>> sf_flash will call erase ops from sf_ops.c. >>>>> >>>>> As it's a mtd call, I thought sf_mtd_into._erase will intern calls >>>>> erase ops from sf_ops.c >>>>> What is this behavior could you please help me? >>>>> >>>>>> + >>>>>> + sf_mtd_info.size = flash->size; >>>>>> + sf_mtd_info.priv = flash; >>>>>> + >>>>>> + /* Only uniform flash devices for now */ >>>>>> + sf_mtd_info.numeraseregions = 0; >>>>>> + sf_mtd_info.erasesize = flash->sector_size; >>>>>> + >>>>>> + return add_mtd_device(&sf_mtd_info); >>>>>> +} >>>>>> + >>>>>> +void spi_flash_mtd_unregister(void) >>>>>> +{ >>>>>> + del_mtd_device(&sf_mtd_info); >>>>>> +} >>>>>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c >>>>>> index d19138d..2342972 100644 >>>>>> --- a/drivers/mtd/spi/sf_probe.c >>>>>> +++ b/drivers/mtd/spi/sf_probe.c >>>>>> @@ -397,10 +397,9 @@ int spi_flash_probe_slave(struct spi_slave *spi, >>>>>> struct spi_flash *flash) >>>>>> if (spi_enable_wp_pin(flash)) >>>>>> puts("Enable WP pin failed\n"); >>>>>> >>>>>> - /* Release spi bus */ >>>>>> - spi_release_bus(spi); >>>>>> - >>>>>> - return 0; >>>>>> +#ifdef CONFIG_SPI_FLASH_MTD >>>>>> + ret = spi_flash_mtd_register(flash); >>>>>> +#endif >>>>>> >>>>>> err_read_id: >>>>>> spi_release_bus(spi); >>>>>> @@ -450,6 +449,9 @@ struct spi_flash *spi_flash_probe_fdt(const void >>>>>> *blob, int slave_node, >>>>>> >>>>>> void spi_flash_free(struct spi_flash *flash) >>>>>> { >>>>>> +#ifdef CONFIG_SPI_FLASH_MTD >>>>>> + spi_flash_mtd_unregister(); >>>>>> +#endif >>>>>> spi_free_slave(flash->spi); >>>>>> free(flash); >>>>>> } >>>>>> -- >>>>>> 2.1.0 > > thanks! > -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany