On 16 June 2015 at 14:48, Heiko Schocher denx <h...@denx.de> wrote:
> Hello Jagan,
>
>
> Am 16.06.2015 um 10:52 schrieb Jagan Teki:
>>
>> Hi Heiko,
>>
>> On 16 June 2015 at 14:13, Heiko Schocher denx <h...@denx.de> wrote:
>>>
>>> Hello Jagan,
>>>
>>>
>>> Am 16.06.2015 um 10:04 schrieb Jagan Teki:
>>>>
>>>>
>>>> Hi Heiko,
>>>>
>>>> On 20 May 2015 at 12:16, Heiko Schocher <h...@denx.de> 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@0x0 env
>>>>>> mtdparts variable not set, see 'help mtdparts'
>>>>>> zynq-uboot> mtdparts
>>>>>>
>>>>>> device nor0 <zynq-sf.0>, # 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?
Suppose I need sf-mtd and enabled CONFIG_SPI_FLASH_MTD so
when I did it from sf_cmd the code in sf-mtd.c is never been used for
erase/read/write. Is that true right?
>
>> 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 ...
Agreed but until unless if you have filesystem link to cmd_sf to partitioning.
>
>>
>>>
>>>>> 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 <spi3.1>, # 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 <gpmi-nand>, # 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 <h...@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
>>>>>>>
>>>>>>> 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 <daniel.schwierz...@gmail.com>
>>>>>>> Signed-off-by: Heiko Schocher <h...@denx.de>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.schwierz...@gmail.com
>>>>>>> + *
>>>>>>> + * SPDX-License-Identifier: GPL-2.0+
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <common.h>
>>>>>>> +#include <malloc.h>
>>>>>>> +#include <asm/errno.h>
>>>>>>> +#include <linux/mtd/mtd.h>
>>>>>>> +#include <spi_flash.h>
>>>>>>> +
>>>>>>> +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!
--
Jagan | openedev.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
