[U-Boot] [PATCH v6 1/4] mtd, spi: add MTD layer driver

[Heiko Schocher denx <hs@denx.de>]

Hello Jagan,

Am 16.06.2015 um 10:04 schrieb Jagan Teki:
> Hi Heiko,
>
> On 20 May 2015 at 12:16, Heiko Schocher <hs@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 at 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?

thanks!

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 <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 <hs@denx.de> wrote:
>>>>
>>>> From: Daniel Schwierzeck <daniel.schwierzeck@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.schwierzeck@gmail.com>
>>>> Signed-off-by: Heiko Schocher <hs@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.schwierzeck at 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!
>

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany