From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 05 Nov 2014 13:26:37 +0100 Subject: [U-Boot] [PATCH v3 1/3] mtd, spi: add MTD layer driver In-Reply-To: References: <1409895536-18092-1-git-send-email-hs@denx.de> <1409895536-18092-2-git-send-email-hs@denx.de> Message-ID: <545A177D.2070300@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, Daniel, Am 04.11.2014 22:24, schrieb Daniel Schwierzeck: > 2014-11-04 21:32 GMT+01:00 Jagan Teki: >> On 5 September 2014 11:08, 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 >>> Cc: Jagannadha Sutradharudu Teki >>> >>> --- >>> 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 >>> +#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()); >> >> 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