Hi Stefan, On 2026-09-22T06:09:29, Heiko Schocher <[email protected]> wrote: > mtd: spi: add driver for the Microchip 23LCV1024 SPI SRAM > > The 23LCV1024 is a 1 Mbit serial SRAM with battery backup, sitting on an > SPI bus and used as non volatile scratch storage. Bind it to the > spi_flash uclass so that the sf command reaches it, and register it as an > MTD device of type MTD_RAM, so mtd command can be used.
Is UCLASS_SPI_FLASH really the right home for a device with no flash semantics? The uclass allocates a struct spi_nor for every instance and every user of it (sf command, bootdev, MTD helpers) assumes NOR behaviour. It works because you shim the fields in, but it feels wrong. Have you considered driving it purely through UCLASS_MTD, or adding a small UCLASS_SPI_RAM with the same ops? What do you think? > > The device powers up in byte operation, where every command carries > exactly one data byte. Select sequential operation once during probe, as > Linux does in drivers/mtd/devices/mchp23k256.c, so that a read or a write > of any length is served by a single command. > > A RAM needs no erase before a write, but a caller that asks for one > expects the area to read back the way an erased flash would, so both > erase operations fill it with 0xff, which is what the sandbox flash > emulation leaves behind as well. Leaving the operation out is no option > either, because the uclass calls it without checking for it first. > > Signed-off-by: Stefan Bigler <[email protected]> > Signed-off-by: Heiko Schocher <[email protected]> > > configs/sandbox_defconfig | 1 + > drivers/mtd/spi/Kconfig | 11 ++ > drivers/mtd/spi/Makefile | 1 + > drivers/mtd/spi/spi_ram.c | 280 > ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 293 insertions(+) > diff --git a/drivers/mtd/spi/spi_ram.c b/drivers/mtd/spi/spi_ram.c > @@ -0,0 +1,280 @@ > +#include <dm.h> > +#include <dm/device_compat.h> > +#include <log.h> > +#include <malloc.h> > +#include <spi.h> > +#include <spi_flash.h> > +#include <vsprintf.h> > +#include <linux/bitops.h> > +#include <linux/kernel.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/spi-nor.h> > +#include <linux/sizes.h> The linux/mtd/spi-nor.h include is redundant — spi_flash.h already pulls it in — and it advertises that this driver is a NOR driver, which it is not. Please drop it. > diff --git a/drivers/mtd/spi/spi_ram.c b/drivers/mtd/spi/spi_ram.c > @@ -0,0 +1,280 @@ > +static int spi_ram_erase(struct udevice *dev, u32 offset, size_t len) > +{ > + size_t step = min_t(size_t, len, SPI_RAM_ERASE_STEP); > + u8 *buf; > + int ret = 0; > + > + buf = malloc(step); I wish we could avoid malloc() here? > + if (!buf) > + return -ENOMEM; > + > + memset(buf, SPI_RAM_ERASE_BYTE, step); > + > + while (len) { > + step = min_t(size_t, len, SPI_RAM_ERASE_STEP); > + > + ret = spi_ram_write(dev, offset, step, buf); > + if (ret) > + break; > + > + offset += step; > + len -= step; > + } > + > + free(buf); > + > + return ret; > +} Two things. If len is 0 we still malloc(0) - please return early. And each loop iteration calls spi_ram_write(), which claims and releases the bus for every 1 KB chunk. Since you have already put the device into sequential mode at probe, one claim/release around the whole loop (or a helper that emits one WRITE command and streams the fill) would be much cheaper and match what the data sheet allows. > diff --git a/drivers/mtd/spi/spi_ram.c b/drivers/mtd/spi/spi_ram.c > @@ -0,0 +1,280 @@ > + mtd->writesize = 1; > + mtd->erasesize = 1; > + mtd->_erase = spi_ram_mtd_erase; > + mtd->_read = spi_ram_mtd_read; > + mtd->_write = spi_ram_mtd_write; > + > + flash->name = data->name; > + flash->size = data->size; > + flash->erase_size = mtd->erasesize; > + flash->sector_size = mtd->erasesize; Setting sector_size to 1 makes the sf update path in cmd/sf.c drop to a one-byte read/erase/write loop over the whole region (see spi_flash_update() and spi_flash_update_block()) - 128 KB of byte-at-a-time round trips. Please pick something more useful (the 32-byte page, or a larger convenience value) for sector_size, even if erasesize stays at 1. > diff --git a/drivers/mtd/spi/spi_ram.c b/drivers/mtd/spi/spi_ram.c > @@ -0,0 +1,280 @@ > + sprintf(flash->mtd_name, "ram%d", dev_seq(dev)); Please use snprintf() with sizeof(flash->mtd_name) - the buffer is MTD_NAME_SIZE(MTD_DEV_TYPE_NOR), which only just happens to fit 'ram' because it is also three characters, and any future change to that macro or DM_MAX_SEQ_STR will bite silently. > diff --git a/drivers/mtd/spi/spi_ram.c b/drivers/mtd/spi/spi_ram.c > @@ -0,0 +1,280 @@ > + * Copyright (C) 2019 S. Bigler Securiton AG > + * Copyright (C) 2023 Heiko Schocher, DENX Software Engineering, > [email protected]. Nit: please drop the trailing full stop after the email address on the 2023 line - do you have an email for the Bigler person? Regards, Simon
