Hello Simon,
Thanks for the review!
On 28.09.26 16:45, Simon Glass wrote:
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?
At start of this, we had such an UCLASS_SPI_RAM approach, but we had a
lot of duplicated code... I think again about this.
Hmm... I tend to say, I would prefer UCLASS_MTD, which is the lower
impact I think, we only use the "sf command" and have only the mtd
read/write functions... and driver drivers/mtd/nand/spi makes
it the same way.
With UCLASS_SPI_RAM I do not see what we win, and the impact is bigger.
Let me think about it.
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.
Will do.
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?
I look for it, when preparing v2.
+ 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.
Yes, that sounds better.
Hmm.. can we drop the erase function at all, as we do not need it?
Ah, I see, we already have
mtd->flags and MTD_NO_ERASE from linux.
I will drop erase function from spi ram driver, and if I see it
correct I can get rid with this step of the malloc() too!
I just need to add in mtd_erase() the MTD_NO_ERASE handling (will add
this in v2 as a seperate patch.
Oh, I see in linux, that mtd_erase() there only checks:
if (!mtd->erasesize || !master->_erase)
return -ENOTSUPP;
This linux check is missing currently in U-Boots implementation.
Do you agree, that we can simply add this check?
But why not checking the flag MTD_NO_ERASE too?
What do you prefer, the NULL check from linux and/or check the MTD_NO_ERASE
flag?
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.
Yes, on the other side, there is no real sector size, you can write
byte per byte... and in case writting exact one byte, we have with
other settings for writesize/erasize overhead...
I think, the writesize setting should stay at 1, and erasesize gets 0
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.
Good hint, thanks!
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
Will do, sorry.
2023 line - do you have an email for the Bigler person?
Yes, I add it in v2
bye,
Heiko
Regards,
Simon
--
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: [email protected]
Geschäftsführer : Stefano Babic
