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

[Heiko Schocher <hs@denx.de>]

Hello Jagan, Daniel,

Am 04.11.2014 22:24, schrieb Daniel Schwierzeck:
> 2014-11-04 21:32 GMT+01:00 Jagan Teki<jagannadh.teki@gmail.com>:
>> On 5 September 2014 11:08, 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>
>>> Cc: Jagannadha Sutradharudu Teki<jagannadh.teki@gmail.com>
>>>
>>> ---
>>> MAKEALL for ar, mips, powerc compiles clean
>>>
>>> - changes for v2:
>>>    - add comment from Daniel Schwierzeck:
>>>      fix compile error from original patch with
>>>      "static inline" rather than "static __maybe_unused"
>>> - changes for v3:
>>>    rebase with d6c1ffc7d23f4fe4ae8c91101861055b8e1501b6
>>> ---
>>>   README                        |   3 ++
>>>   common/cmd_sf.c               |   9 ++--
>>>   drivers/mtd/spi/Makefile      |   1 +
>>>   drivers/mtd/spi/sf_internal.h |  13 ++++++
>>>   drivers/mtd/spi/sf_mtd.c      | 104 ++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/mtd/spi/sf_probe.c    |   5 ++
>>>   6 files changed, 131 insertions(+), 4 deletions(-)
>>>   create mode 100644 drivers/mtd/spi/sf_mtd.c
>>>
>>> diff --git a/README b/README
>>> index 0a0f528..e7be54e 100644
>>> --- a/README
>>> +++ b/README
>>> @@ -2965,6 +2965,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 b4ceb71..7653d7e 100644
>>> --- a/common/cmd_sf.c
>>> +++ b/common/cmd_sf.c
>>> @@ -121,16 +121,17 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>>>                          return -1;
>>>          }
>>>
>>> +       if (flash)
>>> +               spi_flash_free(flash);
>>> +
>>>          new = spi_flash_probe(bus, cs, speed, mode);
>>> +       flash = new;
>>> +
>>
>> Why is this change? I guess it's not related to mtd.
>> And it's better to add new with global flash only it's probed isn't it?

If we have probed a flash, and we probe another flash on maybe another bus/cs,
we should free the old ... or?

And if the new probe failed, we have to update the "flash" pointer in any case ...

>>>          if (!new) {
>>>                  printf("Failed to initialize SPI flash at %u:%u\n", bus, cs);
>>>                  return 1;
>>>          }
>>>
>>> -       if (flash)
>>> -               spi_flash_free(flash);
>>> -       flash = new;
>>> -
>>>          return 0;
>>>   }
>>>
>>> diff --git a/drivers/mtd/spi/Makefile b/drivers/mtd/spi/Makefile
>>> index 9e18fb4..b15d273 100644
>>> --- a/drivers/mtd/spi/Makefile
>>> +++ b/drivers/mtd/spi/Makefile
>>> @@ -12,6 +12,7 @@ endif
>>>
>>>   obj-$(CONFIG_CMD_SF) += sf.o
>>>   obj-$(CONFIG_SPI_FLASH) += sf_params.o sf_probe.o sf_ops.o
>>> +obj-$(CONFIG_SPI_FLASH_MTD) += sf_mtd.o
>>>   obj-$(CONFIG_SPI_FRAM_RAMTRON) += ramtron.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 19d4914..a9f97d1 100644
>>> --- a/drivers/mtd/spi/sf_internal.h
>>> +++ b/drivers/mtd/spi/sf_internal.h
>>> @@ -160,4 +160,17 @@ 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);
>>> +#else
>>> +static inline int spi_flash_mtd_register(struct spi_flash *flash)
>>> +{
>>> +       return 0;
>>> +}
>>> +static inline 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());
>>
>> This is quite not required for sf - like mtd_number is devnum that is maximum
>> number of sf's on the particular board.
>>
>> We're not dealing with this at this point of time, comments?
>
> this is needed if you enable CFI-MTD and SF-MTD at the same time
> (common case on evaluation boards). Because CFI is always initialized
> in board_init_r() (before SF), it occupies MTD numbers 0 to
> CONFIG_SYS_MAX_FLASH_BANKS - 1.

exactly! Thanks for clarifying.

>>> +
>>> +       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;
>>> +
>>> +       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 4d148d1..6c50766 100644
>>> --- a/drivers/mtd/spi/sf_probe.c
>>> +++ b/drivers/mtd/spi/sf_probe.c
>>> @@ -385,6 +385,10 @@ static struct spi_flash *spi_flash_probe_slave(struct spi_slave *spi)
>>>          /* Release spi bus */
>>>          spi_release_bus(spi);
>>>
>>> +       ret = spi_flash_mtd_register(flash);
>>> +       if (ret)
>>> +               goto err_claim_bus;
>>> +
>>
>> Be define this code in macro's and place before spi_release_bus()
>
> the defines are already in sf_internal.h. This technique is much more
> elegant and cleaner than polluting the code with #ifdef's
>
> +#ifdef CONFIG_SPI_FLASH_MTD
> +int spi_flash_mtd_register(struct spi_flash *flash);
> +void spi_flash_mtd_unregister(void);
> +#else
> +static inline int spi_flash_mtd_register(struct spi_flash *flash)
> +{
> +       return 0;
> +}
> +static inline void spi_flash_mtd_unregister(void)
> +{
> +}
> +#endif

Yep.

>>
>>>          return flash;
>>>
>>>   err_read_id:
>>> @@ -416,6 +420,7 @@ struct spi_flash *spi_flash_probe_fdt(const void *blob, int slave_node,
>>>
>>>   void spi_flash_free(struct spi_flash *flash)
>>>   {
>>> +       spi_flash_mtd_unregister();
>>
>> Same ..
>>
>>>          spi_free_slave(flash->spi);
>>>          free(flash);
>>>   }
>>> --
>>> 1.8.3.1
>>>
>>
>> Code looks straight forward and be define the necessary macro's on new
>> code and please

I am here on Daniels side ... do we really want to introduce here macros?

>> update the test log on doc/SPI. Mean while I will look for more options.

What do you mean exactly?

Thanks for the review!

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