Re: [PATCH 5/5] mtd: spi: Switch to MTD uclass (absolute UCLASS_SPI_FLASH)

On 09/07/20 4:47 pm, Jagan Teki wrote: [...] On 09/07/20 4:47 pm, Jagan Teki wrote:

diff --git a/arch/x86/cpu/apollolake/spl.c b/arch/x86/cpu/apollolake/spl.c index e1ee1e0624..9c80440bbb 100644 --- a/arch/x86/cpu/apollolake/spl.c +++ b/arch/x86/cpu/apollolake/spl.c @@ -10,6 +10,7 @@ #include <image.h> #include <log.h> #include <malloc.h> +#include <mtd.h> #include <spi.h> #include <spl.h> #include <spi_flash.h> @@ -41,7 +42,7 @@ static int rom_load_image(struct spl_image_info

*spl_image,

debug("Reading from mapped SPI %lx, size %lx", spl_pos, spl_size);

if (CONFIG_IS_ENABLED(SPI_FLASH_SUPPORT)) {

• ret = uclass_find_first_device(UCLASS_SPI_FLASH, &dev);
  

• ret = uclass_find_first_device(UCLASS_MTD, &dev);
  

You have to be careful here (and rest of places in this series where uclass_find_first_device() is used) as the first MTD device need not be first SPI FLASH device. RAW Nand and parallel flash drivers also register MTD devices. Eg.: Above change will be wrong, if apollolake has both SPI FLASH and NAND/Parallel NORs and Parallel NOR binds as first UCLASS_MTD driver.

if (ret)
	return log_msg_ret("spi_flash", ret);
if (!dev)

@@ -77,7 +78,7 @@ static int spl_fast_spi_load_image(struct

spl_image_info *spl_image,

struct udevice *dev; int ret;

• ret = uclass_first_device_err(UCLASS_SPI_FLASH, &dev);

• ret = uclass_first_device_err(UCLASS_MTD, &dev); if (ret) return ret;

[...]

diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c index 44cdb3151d..9cab08f49b 100644 --- a/drivers/mtd/spi/sf-uclass.c +++ b/drivers/mtd/spi/sf-uclass.c @@ -7,6 +7,7 @@ #include <dm.h> #include <log.h> #include <malloc.h> +#include <mtd.h> #include <spi.h> #include <spi_flash.h> #include <dm/device-internal.h> @@ -14,22 +15,6 @@

DECLARE_GLOBAL_DATA_PTR;

-int spi_flash_read_dm(struct udevice *dev, u32 offset, size_t len, void *buf) -{

• return log_ret(sf_get_ops(dev)->read(dev, offset, len, buf));

-}

-int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,

•        const void *buf)
  

-{

• return log_ret(sf_get_ops(dev)->write(dev, offset, len, buf));

-}

-int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len) -{

• return log_ret(sf_get_ops(dev)->erase(dev, offset, len));

-}

void spi_flash_free(struct spi_flash *flash) { device_remove(flash->spi->dev, DM_REMOVE_NORMAL); @@ -57,108 +42,78 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs, str = strdup(name); #endif ret = spi_get_bus_and_cs(bus, cs, max_hz, spi_mode,

• 		 "spi_flash_std", str, &bus_dev, &slave);
  

• 		  "spi_flash_std", str, &bus_dev, &slave);
  

Unintended whitespace change?

if (ret) return NULL;

• return dev_get_uclass_priv(slave->dev);

• return dev_get_priv(slave->dev);

}

-static int spi_flash_post_bind(struct udevice *dev) +static int spi_flash_std_read(struct udevice *dev, loff_t from, size_t len,

• 	      u_char *buf)
  

{ -#if defined(CONFIG_NEEDS_MANUAL_RELOC)

• struct dm_spi_flash_ops *ops = sf_get_ops(dev);
• static int reloc_done;
• if (!reloc_done) {

• if (ops->read)
  

• 	ops->read += gd->reloc_off;
  

• if (ops->write)
  

• 	ops->write += gd->reloc_off;
  

• if (ops->erase)
  

• 	ops->erase += gd->reloc_off;
  

• reloc_done++;
  

• }

-#endif

• return 0;

-}

-static int spi_flash_std_read(struct udevice *dev, u32 offset, size_t len,

• 	      void *buf)
  

-{

• struct spi_flash *flash = dev_get_uclass_priv(dev);

• struct spi_flash *flash = dev_get_priv(dev); struct mtd_info *mtd = &flash->mtd; size_t retlen;

• return log_ret(mtd->_read(mtd, offset, len, &retlen, buf));

• return log_ret(mtd->_read(mtd, from, len, &retlen, buf));

}

-static int spi_flash_std_write(struct udevice *dev, u32 offset, size_t len,

• 	       const void *buf)
  

+static int spi_flash_std_write(struct udevice *dev, loff_t to, size_t len,

• 	       const u_char *buf)
  

{

• struct spi_flash *flash = dev_get_uclass_priv(dev);

• struct spi_flash *flash = dev_get_priv(dev); struct mtd_info *mtd = &flash->mtd; size_t retlen;

• return mtd->_write(mtd, offset, len, &retlen, buf);

• return mtd->_write(mtd, to, len, &retlen, buf);

}

-static int spi_flash_std_erase(struct udevice *dev, u32 offset, size_t len) +static int spi_flash_std_erase(struct udevice *dev, struct erase_info *instr) {

• struct spi_flash *flash = dev_get_uclass_priv(dev);

• struct spi_flash *flash = dev_get_priv(dev); struct mtd_info *mtd = &flash->mtd;

• struct erase_info instr;

• if (offset % mtd->erasesize || len % mtd->erasesize) {

• printf("SF: Erase offset/length not multiple of erase size\n");
  

• if (instr->addr % mtd->erasesize || instr->len % mtd->erasesize) {

• dev_err(dev, "Erase offset/length not multiple of erasesize\n");
  

  return -EINVAL; }

Should this check be moved to mtd_derase() wrapper?

• memset(&instr, 0, sizeof(instr));
• instr.addr = offset;
• instr.len = len;
• return mtd->_erase(mtd, &instr);

• return mtd->_erase(mtd, instr);

}

static int spi_flash_std_probe(struct udevice *dev) {

• struct spi_flash *flash = dev_get_priv(dev); struct spi_slave *slave = dev_get_parent_priv(dev);

• struct spi_flash *flash; int ret;

• if (!slave) {

• printf("SF: Failed to set up slave\n");
  

• return -ENODEV;
  

• }

• flash = dev_get_uclass_priv(dev); flash->dev = dev; flash->spi = slave;

• /* Claim spi bus */ ret = spi_claim_bus(slave); if (ret) {

• debug("SF: Failed to claim SPI bus: %d\n", ret);
  

• dev_err(dev, "failed to claim bus (ret=%d)\n", ret);
  

  return ret; }

  ret = spi_nor_scan(flash);

• if (ret)

• goto err_read_id;
  

• if (ret) {

• dev_err(dev, "failed to scan spinor (ret=%d)\n", ret);
  

• goto err_claim_bus;
  

• }

• if (CONFIG_IS_ENABLED(SPI_FLASH_MTD))

• if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) ret = spi_flash_mtd_register(flash);

-err_read_id: +err_claim_bus: spi_release_bus(slave); return ret; }

static int spi_flash_std_remove(struct udevice *dev) {

• if (CONFIG_IS_ENABLED(SPI_FLASH_MTD))

• if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) spi_flash_mtd_unregister();

  return 0;

@@ -190,7 +145,7 @@ static int spi_flash_std_bind(struct udevice *dev) return 0; }

-static const struct dm_spi_flash_ops spi_flash_std_ops = { +static const struct mtd_ops spi_flash_std_ops = { .read = spi_flash_std_read, .write = spi_flash_std_write, .erase = spi_flash_std_erase, @@ -203,7 +158,7 @@ static const struct udevice_id spi_flash_std_ids[] = {

U_BOOT_DRIVER(spi_flash_std) = { .name = "spi_flash_std",

• .id = UCLASS_SPI_FLASH,

• .id = UCLASS_MTD, .of_match = spi_flash_std_ids, .probe = spi_flash_std_probe, .bind = spi_flash_std_bind,

@@ -211,10 +166,3 @@ U_BOOT_DRIVER(spi_flash_std) = { .priv_auto_alloc_size = sizeof(struct spi_flash), .ops = &spi_flash_std_ops, };

-UCLASS_DRIVER(spi_flash) = {

• .id = UCLASS_SPI_FLASH,
• .name = "spi_flash",
• .post_bind = spi_flash_post_bind,
• .per_device_auto_alloc_size = sizeof(struct spi_flash),

-}; diff --git a/drivers/mtd/spi/sf_dataflash.c b/drivers/mtd/spi/sf_dataflash.c index 27d847d421..5c73b54cbc 100644 --- a/drivers/mtd/spi/sf_dataflash.c +++ b/drivers/mtd/spi/sf_dataflash.c @@ -12,6 +12,7 @@ #include <fdtdec.h> #include <flash.h> #include <log.h> +#include <mtd.h> #include <spi.h> #include <spi_flash.h> #include <div64.h> @@ -117,7 +118,7 @@ static int dataflash_waitready(struct spi_slave *spi) }

/* Erase pages of flash */ -static int spi_dataflash_erase(struct udevice *dev, u32 offset, size_t len) +static int spi_dataflash_erase(struct udevice *dev, struct erase_info *instr) { struct dataflash *dataflash; struct spi_flash *spi_flash; @@ -125,6 +126,8 @@ static int spi_dataflash_erase(struct udevice *dev, u32 offset, size_t len) unsigned blocksize; uint8_t *command; uint32_t rem;

• loff_t offset = instr->addr;

• size_t len = instr->len; int status;

  dataflash = dev_get_priv(dev);

@@ -136,7 +139,7 @@ static int spi_dataflash_erase(struct udevice *dev, u32 offset, size_t len) memset(dataflash->command, 0 , sizeof(dataflash->command)); command = dataflash->command;

• debug("%s: erase addr=0x%x len 0x%x\n", dev->name, offset, len);

• debug("%s: erase addr=0x%llx len 0x%x\n", dev->name, offset, len);

  div_u64_rem(len, spi_flash->page_size, &rem); if (rem) {

@@ -146,7 +149,7 @@ static int spi_dataflash_erase(struct udevice *dev, u32 offset, size_t len) } div_u64_rem(offset, spi_flash->page_size, &rem); if (rem) {

• printf("%s: offset(0x%x) isn't the multiple of page size(0x%x)\n",
  

• printf("%s: offset(0x%llx) isn't the multiple of page size(0x%x)\n",
    dev->name, offset, spi_flash->page_size);
  

  return -EINVAL; }

@@ -210,8 +213,8 @@ static int spi_dataflash_erase(struct udevice *dev, u32 offset, size_t len)

• len : Amount to read
• buf : Buffer containing the data

*/ -static int spi_dataflash_read(struct udevice *dev, u32 offset, size_t len,

• 	      void *buf)
  

+static int spi_dataflash_read(struct udevice *dev, loff_t offset, size_t len,

• 	      u_char *buf)
  

{ struct dataflash *dataflash; struct spi_flash *spi_flash; @@ -227,7 +230,7 @@ static int spi_dataflash_read(struct udevice *dev, u32 offset, size_t len, memset(dataflash->command, 0 , sizeof(dataflash->command)); command = dataflash->command;

• debug("%s: erase addr=0x%x len 0x%x\n", dev->name, offset, len);

• debug("%s: erase addr=0x%llx len 0x%x\n", dev->name, offset, len); debug("READ: (%x) %x %x %x\n", command[0], command[1], command[2], command[3]);

@@ -266,8 +269,8 @@ static int spi_dataflash_read(struct udevice *dev, u32 offset, size_t len,

• len : Amount to write
• buf : Buffer containing the data

*/ -int spi_dataflash_write(struct udevice *dev, u32 offset, size_t len,

• 	const void *buf)
  

+int spi_dataflash_write(struct udevice *dev, loff_t offset, size_t len,

• 	const u_char *buf)
  

{ struct dataflash *dataflash; struct spi_flash *spi_flash; @@ -285,7 +288,7 @@ int spi_dataflash_write(struct udevice *dev, u32 offset, size_t len, memset(dataflash->command, 0 , sizeof(dataflash->command)); command = dataflash->command;

• debug("%s: write 0x%x..0x%x\n", dev->name, offset, (offset + len));

• debug("%s: write 0x%llx..0x%llx\n", dev->name, offset, (offset + len));

  pageaddr = ((unsigned)offset / spi_flash->page_size); to = ((unsigned)offset % spi_flash->page_size);

@@ -676,7 +679,7 @@ err_jedec_probe: return status; }

-static const struct dm_spi_flash_ops spi_dataflash_ops = { +static const struct mtd_ops spi_dataflash_ops = { .read = spi_dataflash_read, .write = spi_dataflash_write, .erase = spi_dataflash_erase, @@ -690,7 +693,7 @@ static const struct udevice_id spi_dataflash_ids[] = {

U_BOOT_DRIVER(spi_dataflash) = { .name = "spi_dataflash",

• .id = UCLASS_SPI_FLASH,

• .id = UCLASS_MTD, .of_match = spi_dataflash_ids, .probe = spi_dataflash_probe, .priv_auto_alloc_size = sizeof(struct dataflash),

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 7837d459f1..1f56f7f48a 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -99,7 +99,6 @@ enum uclass_id { UCLASS_SMEM, /* Shared memory interface */ UCLASS_SOUND, /* Playing simple sounds */ UCLASS_SPI, /* SPI bus */

• UCLASS_SPI_FLASH, /* SPI flash */ UCLASS_SPI_GENERIC, /* Generic SPI flash target */ UCLASS_SPMI, /* System Power Management Interface bus */ UCLASS_SYSCON, /* System configuration device */

diff --git a/include/spi_flash.h b/include/spi_flash.h index 24759944eb..f4852fc5c1 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -10,6 +10,7 @@ #define _SPI_FLASH_H_

#include <dm.h> /* Because we dereference struct udevice here */ +#include <mtd.h> #include <linux/types.h> #include <linux/mtd/spi-nor.h>

@@ -27,70 +28,24 @@ # define CONFIG_ENV_SPI_MODE CONFIG_SF_DEFAULT_MODE #endif

-struct spi_slave;

-struct dm_spi_flash_ops {

• int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
• int (*write)(struct udevice *dev, u32 offset, size_t len,

•      const void *buf);
  

• int (*erase)(struct udevice *dev, u32 offset, size_t len);

-};

-/* Access the serial operations for a device */ -#define sf_get_ops(dev) ((struct dm_spi_flash_ops *)(dev)->driver->ops)

#if CONFIG_IS_ENABLED(DM_SPI_FLASH) -/**

• • spi_flash_read_dm() - Read data from SPI flash
• • @dev: SPI flash device
• • @offset: Offset into device in bytes to read from
• • @len: Number of bytes to read
• • @buf: Buffer to put the data that is read
• • @return 0 if OK, -ve on error
• */

-int spi_flash_read_dm(struct udevice *dev, u32 offset, size_t len, void *buf);

-/**

• • spi_flash_write_dm() - Write data to SPI flash
• • @dev: SPI flash device
• • @offset: Offset into device in bytes to write to
• • @len: Number of bytes to write
• • @buf: Buffer containing bytes to write
• • @return 0 if OK, -ve on error
• */

-int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,

•        const void *buf);
  

-/**

• • spi_flash_erase_dm() - Erase blocks of the SPI flash
• • Note that @len must be a muiltiple of the flash sector size.
• • @dev: SPI flash device
• • @offset: Offset into device in bytes to start erasing
• • @len: Number of bytes to erase
• • @return 0 if OK, -ve on error
• */

-int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len);

static inline int spi_flash_read(struct spi_flash *flash, u32 offset, size_t len, void *buf) {

• return spi_flash_read_dm(flash->dev, offset, len, buf);

• return mtd_dread(flash->dev, offset, len, buf);

}

static inline int spi_flash_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf) {

• return spi_flash_write_dm(flash->dev, offset, len, buf);

• return mtd_dwrite(flash->dev, offset, len, buf);

}

static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, size_t len) {

• return spi_flash_erase_dm(flash->dev, offset, len);

• return mtd_derase(flash->dev, offset, len);

}

struct sandbox_state; diff --git a/test/dm/sf.c b/test/dm/sf.c index 9e7dead684..a675923dc5 100644 --- a/test/dm/sf.c +++ b/test/dm/sf.c @@ -31,23 +31,23 @@ static int dm_test_spi_flash(struct unit_test_state *uts)

src = map_sysmem(0x20000, full_size); ut_assertok(os_write_file("spi.bin", src, full_size));

• ut_assertok(uclass_first_device_err(UCLASS_SPI_FLASH, &dev));

• ut_assertok(uclass_first_device_err(UCLASS_MTD, &dev));

  dst = map_sysmem(0x20000 + full_size, full_size);

• ut_assertok(spi_flash_read_dm(dev, 0, size, dst));

• ut_assertok(mtd_dread(dev, 0, size, dst)); ut_asserteq_mem(src, dst, size);

  /* Erase */

• ut_assertok(spi_flash_erase_dm(dev, 0, size));
• ut_assertok(spi_flash_read_dm(dev, 0, size, dst));

• ut_assertok(mtd_derase(dev, 0, size));

• ut_assertok(mtd_dread(dev, 0, size, dst)); for (i = 0; i < size; i++) ut_asserteq(dst[i], 0xff);

  /* Write some new data */ for (i = 0; i < size; i++) src[i] = i;

• ut_assertok(spi_flash_write_dm(dev, 0, size, src));
• ut_assertok(spi_flash_read_dm(dev, 0, size, dst));

• ut_assertok(mtd_dwrite(dev, 0, size, src));

• ut_assertok(mtd_dread(dev, 0, size, dst)); ut_asserteq_mem(src, dst, size);

  /* Check mapping */

-- 2.25.1