Message-ID: <638496dd-ec60-4e53-bad7-eb657f67d580@csgroup.eu>
Date: Mon, 3 Nov 2025 17:33:34 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "A. Sverdlin" <alexander.sverdlin@...mens.com>,
 linux-kernel@...r.kernel.org
Cc: Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Michael Walle <mwalle@...nel.org>, Hui Wang <hui.wang@...onical.com>,
 TRINH THAI Florent <florent.trinh-thai@...soprasteria.com>
Subject: Re: [PATCH] eeprom: at25: convert to spi-mem API

Hi,

Le 03/07/2025 à 00:28, A. Sverdlin a écrit :
> From: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> 
> Replace the RAW SPI accesses with spi-mem API. The latter will fall back to
> RAW SPI accesses if spi-mem callbacks are not implemented by a controller
> driver.

With this patch (kernel v6.17.1) our powerpc boards are totally 
unstable, we get multiple random Oops due to bad memory accesses.

With this commit reverted the board is stable again.

The SPI driver is:

CONFIG_SPI=y
CONFIG_SPI_MASTER=y
CONFIG_SPI_MEM=y
CONFIG_SPI_FSL_LIB=y
CONFIG_SPI_FSL_CPM=y
CONFIG_SPI_FSL_SPI=y

How can we further investigate the issue ?

Thanks
Christophe

> 
> Notable advantages:
> - read function now allocates a bounce buffer for SPI DMA compatibility,
>    similar to write function;
> - the driver can now be used in conjunction with SPI controller drivers
>    providing spi-mem API only, e.g. spi-nxp-fspi.
> - during the initial probe the driver polls busy/ready status bit for 25ms
>    instead of giving up instantly and hoping that the FW didn't write the
>    EEPROM
> 
> Notes:
> - mutex_lock() has been dropped from fm25_aux_read() because the latter is
>    only being called in probe phase and therefore cannot race with
>    at25_ee_read() or at25_ee_write()
> 
> Quick 4KB block size test with CY15B102Q 256KB F-RAM over spi_omap2_mcspi
> driver (no spi-mem ops provided, fallback to raw SPI inside spi-mem):
> 
> OP	| throughput, KB/s	| change
> --------+-----------------------+-------
> write	| 1717.847 -> 1656.684	| -3.6%
> read	| 1115.868 -> 1059.367	| -5.1%
> 
> The lower throughtput probably comes from the 3 messages per SPI transfer
> inside spi-mem instead of hand-crafted 2 messages per transfer in the
> former at25 code. However, if the raw SPI access is not preserved, then
> the driver doesn't grow from the lines-of-code perspective and subjectively
> could be considered even a bit simpler.
> 
> Higher performance impact on the read operation could be explained by the
> newly introduced bounce buffer in read operation. I didn't find any
> explanation or guarantee, why would a bounce buffer be not needed on the
> read side, so I assume it's a pure luck that nobody read EEPROM into
> some variable on stack on an architecture where kernel stack would be
> not DMA-able.
> 
> Cc: Michael Walle <mwalle@...nel.org>
> Cc: Hui Wang <hui.wang@...onical.com>
> Link: https://lore.kernel.org/all/28ab8b72afee1af59b628f7389f0d7f5@kernel.org/
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> ---
>   drivers/misc/eeprom/Kconfig |   1 +
>   drivers/misc/eeprom/at25.c  | 321 ++++++++++++++++++------------------
>   2 files changed, 162 insertions(+), 160 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index cb1c4b8e7fd37..0bef5b93bd6dc 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -37,6 +37,7 @@ config EEPROM_AT25
>   	depends on SPI && SYSFS
>   	select NVMEM
>   	select NVMEM_SYSFS
> +	select SPI_MEM
>   	help
>   	  Enable this driver to get read/write support to most SPI EEPROMs
>   	  and Cypress FRAMs,
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 595ceb9a71261..20611320e7152 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -7,8 +7,10 @@
>    */
>   
>   #include <linux/bits.h>
> +#include <linux/cleanup.h>
>   #include <linux/delay.h>
>   #include <linux/device.h>
> +#include <linux/iopoll.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/property.h>
> @@ -17,6 +19,7 @@
>   
>   #include <linux/spi/eeprom.h>
>   #include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
>   
>   #include <linux/nvmem-provider.h>
>   
> @@ -35,13 +38,12 @@
>   
>   struct at25_data {
>   	struct spi_eeprom	chip;
> -	struct spi_device	*spi;
> +	struct spi_mem		*spimem;
>   	struct mutex		lock;
>   	unsigned		addrlen;
>   	struct nvmem_config	nvmem_config;
>   	struct nvmem_device	*nvmem;
>   	u8 sernum[FM25_SN_LEN];
> -	u8 command[EE_MAXADDRLEN + 1];
>   };
>   
>   #define	AT25_WREN	0x06		/* latch the write enable */
> @@ -74,20 +76,29 @@ struct at25_data {
>   
>   #define	io_limit	PAGE_SIZE	/* bytes */
>   
> +/* Handle the address MSB as part of instruction byte */
> +static u8 at25_instr(struct at25_data *at25, u8 instr, unsigned int off)
> +{
> +	if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR))
> +		return instr;
> +	if (off < BIT(at25->addrlen * 8))
> +		return instr;
> +	return instr | AT25_INSTR_BIT3;
> +}
> +
>   static int at25_ee_read(void *priv, unsigned int offset,
>   			void *val, size_t count)
>   {
> +	u8 *bounce __free(kfree) = kmalloc(min(count, io_limit), GFP_KERNEL);
>   	struct at25_data *at25 = priv;
>   	char *buf = val;
> -	size_t max_chunk = spi_max_transfer_size(at25->spi);
>   	unsigned int msg_offset = offset;
>   	size_t bytes_left = count;
>   	size_t segment;
> -	u8			*cp;
> -	ssize_t			status;
> -	struct spi_transfer	t[2];
> -	struct spi_message	m;
> -	u8			instr;
> +	int status;
> +
> +	if (!bounce)
> +		return -ENOMEM;
>   
>   	if (unlikely(offset >= at25->chip.byte_len))
>   		return -EINVAL;
> @@ -97,87 +108,67 @@ static int at25_ee_read(void *priv, unsigned int offset,
>   		return -EINVAL;
>   
>   	do {
> -		segment = min(bytes_left, max_chunk);
> -		cp = at25->command;
> +		struct spi_mem_op op;
>   
> -		instr = AT25_READ;
> -		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> -			if (msg_offset >= BIT(at25->addrlen * 8))
> -				instr |= AT25_INSTR_BIT3;
> +		segment = min(bytes_left, io_limit);
>   
> -		mutex_lock(&at25->lock);
> +		op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(at25_instr(at25, AT25_READ,
> +									     msg_offset), 1),
> +						   SPI_MEM_OP_ADDR(at25->addrlen, msg_offset, 1),
> +						   SPI_MEM_OP_NO_DUMMY,
> +						   SPI_MEM_OP_DATA_IN(segment, bounce, 1));
>   
> -		*cp++ = instr;
> -
> -		/* 8/16/24-bit address is written MSB first */
> -		switch (at25->addrlen) {
> -		default:	/* case 3 */
> -			*cp++ = msg_offset >> 16;
> -			fallthrough;
> -		case 2:
> -			*cp++ = msg_offset >> 8;
> -			fallthrough;
> -		case 1:
> -		case 0:	/* can't happen: for better code generation */
> -			*cp++ = msg_offset >> 0;
> -		}
> -
> -		spi_message_init(&m);
> -		memset(t, 0, sizeof(t));
> -
> -		t[0].tx_buf = at25->command;
> -		t[0].len = at25->addrlen + 1;
> -		spi_message_add_tail(&t[0], &m);
> -
> -		t[1].rx_buf = buf;
> -		t[1].len = segment;
> -		spi_message_add_tail(&t[1], &m);
> -
> -		status = spi_sync(at25->spi, &m);
> +		status = spi_mem_adjust_op_size(at25->spimem, &op);
> +		if (status)
> +			return status;
> +		segment = op.data.nbytes;
>   
> +		mutex_lock(&at25->lock);
> +		status = spi_mem_exec_op(at25->spimem, &op);
>   		mutex_unlock(&at25->lock);
> -
>   		if (status)
>   			return status;
> +		memcpy(buf, bounce, segment);
>   
>   		msg_offset += segment;
>   		buf += segment;
>   		bytes_left -= segment;
>   	} while (bytes_left > 0);
>   
> -	dev_dbg(&at25->spi->dev, "read %zu bytes at %d\n",
> +	dev_dbg(&at25->spimem->spi->dev, "read %zu bytes at %d\n",
>   		count, offset);
>   	return 0;
>   }
>   
> -/* Read extra registers as ID or serial number */
> +/*
> + * Read extra registers as ID or serial number
> + *
> + * Allow for the callers to provide @buf on stack (not necessary DMA-capable)
> + * by allocating a bounce buffer internally.
> + */
>   static int fm25_aux_read(struct at25_data *at25, u8 *buf, uint8_t command,
>   			 int len)
>   {
> +	u8 *bounce __free(kfree) = kmalloc(len, GFP_KERNEL);
> +	struct spi_mem_op op;
>   	int status;
> -	struct spi_transfer t[2];
> -	struct spi_message m;
> -
> -	spi_message_init(&m);
> -	memset(t, 0, sizeof(t));
>   
> -	t[0].tx_buf = at25->command;
> -	t[0].len = 1;
> -	spi_message_add_tail(&t[0], &m);
> -
> -	t[1].rx_buf = buf;
> -	t[1].len = len;
> -	spi_message_add_tail(&t[1], &m);
> +	if (!bounce)
> +		return -ENOMEM;
>   
> -	mutex_lock(&at25->lock);
> +	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(command, 1),
> +					   SPI_MEM_OP_NO_ADDR,
> +					   SPI_MEM_OP_NO_DUMMY,
> +					   SPI_MEM_OP_DATA_IN(len, bounce, 1));
>   
> -	at25->command[0] = command;
> +	status = spi_mem_exec_op(at25->spimem, &op);
> +	dev_dbg(&at25->spimem->spi->dev, "read %d aux bytes --> %d\n", len, status);
> +	if (status)
> +		return status;
>   
> -	status = spi_sync(at25->spi, &m);
> -	dev_dbg(&at25->spi->dev, "read %d aux bytes --> %d\n", len, status);
> +	memcpy(buf, bounce, len);
>   
> -	mutex_unlock(&at25->lock);
> -	return status;
> +	return 0;
>   }
>   
>   static ssize_t sernum_show(struct device *dev, struct device_attribute *attr, char *buf)
> @@ -195,14 +186,47 @@ static struct attribute *sernum_attrs[] = {
>   };
>   ATTRIBUTE_GROUPS(sernum);
>   
> +/*
> + * Poll Read Status Register with timeout
> + *
> + * Return:
> + * 0, if the chip is ready
> + * [positive] Status Register value as-is, if the chip is busy
> + * [negative] error code in case of read failure
> + */
> +static int at25_wait_ready(struct at25_data *at25)
> +{
> +	u8 *bounce __free(kfree) = kmalloc(1, GFP_KERNEL);
> +	struct spi_mem_op op;
> +	int status;
> +
> +	if (!bounce)
> +		return -ENOMEM;
> +
> +	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_RDSR, 1),
> +					   SPI_MEM_OP_NO_ADDR,
> +					   SPI_MEM_OP_NO_DUMMY,
> +					   SPI_MEM_OP_DATA_IN(1, bounce, 1));
> +
> +	read_poll_timeout(spi_mem_exec_op, status,
> +			  status || !(bounce[0] & AT25_SR_nRDY), false,
> +			  USEC_PER_MSEC, USEC_PER_MSEC * EE_TIMEOUT,
> +			  at25->spimem, &op);
> +	if (status < 0)
> +		return status;
> +	if (!(bounce[0] & AT25_SR_nRDY))
> +		return 0;
> +
> +	return bounce[0];
> +}
> +
>   static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>   {
> +	u8 *bounce __free(kfree) = kmalloc(min(count, io_limit), GFP_KERNEL);
>   	struct at25_data *at25 = priv;
> -	size_t maxsz = spi_max_transfer_size(at25->spi);
>   	const char *buf = val;
> -	int			status = 0;
> -	unsigned		buf_size;
> -	u8			*bounce;
> +	unsigned int buf_size;
> +	int status;
>   
>   	if (unlikely(off >= at25->chip.byte_len))
>   		return -EFBIG;
> @@ -211,11 +235,8 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>   	if (unlikely(!count))
>   		return -EINVAL;
>   
> -	/* Temp buffer starts with command and address */
>   	buf_size = at25->chip.page_size;
> -	if (buf_size > io_limit)
> -		buf_size = io_limit;
> -	bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL);
> +
>   	if (!bounce)
>   		return -ENOMEM;
>   
> @@ -223,85 +244,64 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>   	 * For write, rollover is within the page ... so we write at
>   	 * most one page, then manually roll over to the next page.
>   	 */
> -	mutex_lock(&at25->lock);
> +	guard(mutex)(&at25->lock);
>   	do {
> -		unsigned long	timeout, retries;
> -		unsigned	segment;
> -		unsigned	offset = off;
> -		u8		*cp = bounce;
> -		int		sr;
> -		u8		instr;
> -
> -		*cp = AT25_WREN;
> -		status = spi_write(at25->spi, cp, 1);
> -		if (status < 0) {
> -			dev_dbg(&at25->spi->dev, "WREN --> %d\n", status);
> -			break;
> -		}
> -
> -		instr = AT25_WRITE;
> -		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> -			if (offset >= BIT(at25->addrlen * 8))
> -				instr |= AT25_INSTR_BIT3;
> -		*cp++ = instr;
> +		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_WREN, 1),
> +						  SPI_MEM_OP_NO_ADDR,
> +						  SPI_MEM_OP_NO_DUMMY,
> +						  SPI_MEM_OP_NO_DATA);
> +		unsigned int segment;
>   
> -		/* 8/16/24-bit address is written MSB first */
> -		switch (at25->addrlen) {
> -		default:	/* case 3 */
> -			*cp++ = offset >> 16;
> -			fallthrough;
> -		case 2:
> -			*cp++ = offset >> 8;
> -			fallthrough;
> -		case 1:
> -		case 0:	/* can't happen: for better code generation */
> -			*cp++ = offset >> 0;
> +		status = spi_mem_exec_op(at25->spimem, &op);
> +		if (status < 0) {
> +			dev_dbg(&at25->spimem->spi->dev, "WREN --> %d\n", status);
> +			return status;
>   		}
>   
>   		/* Write as much of a page as we can */
> -		segment = buf_size - (offset % buf_size);
> +		segment = buf_size - (off % buf_size);
>   		if (segment > count)
>   			segment = count;
> -		if (segment > maxsz)
> -			segment = maxsz;
> -		memcpy(cp, buf, segment);
> -		status = spi_write(at25->spi, bounce,
> -				segment + at25->addrlen + 1);
> -		dev_dbg(&at25->spi->dev, "write %u bytes at %u --> %d\n",
> -			segment, offset, status);
> -		if (status < 0)
> -			break;
> +		if (segment > io_limit)
> +			segment = io_limit;
> +
> +		op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(at25_instr(at25, AT25_WRITE, off),
> +								  1),
> +						   SPI_MEM_OP_ADDR(at25->addrlen, off, 1),
> +						   SPI_MEM_OP_NO_DUMMY,
> +						   SPI_MEM_OP_DATA_OUT(segment, bounce, 1));
> +
> +		status = spi_mem_adjust_op_size(at25->spimem, &op);
> +		if (status)
> +			return status;
> +		segment = op.data.nbytes;
> +
> +		memcpy(bounce, buf, segment);
> +
> +		status = spi_mem_exec_op(at25->spimem, &op);
> +		dev_dbg(&at25->spimem->spi->dev, "write %u bytes at %u --> %d\n",
> +			segment, off, status);
> +		if (status)
> +			return status;
>   
>   		/*
>   		 * REVISIT this should detect (or prevent) failed writes
>   		 * to read-only sections of the EEPROM...
>   		 */
>   
> -		/* Wait for non-busy status */
> -		timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
> -		retries = 0;
> -		do {
> -
> -			sr = spi_w8r8(at25->spi, AT25_RDSR);
> -			if (sr < 0 || (sr & AT25_SR_nRDY)) {
> -				dev_dbg(&at25->spi->dev,
> -					"rdsr --> %d (%02x)\n", sr, sr);
> -				/* at HZ=100, this is sloooow */
> -				msleep(1);
> -				continue;
> -			}
> -			if (!(sr & AT25_SR_nRDY))
> -				break;
> -		} while (retries++ < 3 || time_before_eq(jiffies, timeout));
> -
> -		if ((sr < 0) || (sr & AT25_SR_nRDY)) {
> -			dev_err(&at25->spi->dev,
> +		status = at25_wait_ready(at25);
> +		if (status < 0) {
> +			dev_err_probe(&at25->spimem->spi->dev, status,
> +				      "Read Status Redister command failed\n");
> +			return status;
> +		}
> +		if (status) {
> +			dev_dbg(&at25->spimem->spi->dev,
> +				"Status %02x\n", status);
> +			dev_err(&at25->spimem->spi->dev,
>   				"write %u bytes offset %u, timeout after %u msecs\n",
> -				segment, offset,
> -				jiffies_to_msecs(jiffies -
> -					(timeout - EE_TIMEOUT)));
> -			status = -ETIMEDOUT;
> -			break;
> +				segment, off, EE_TIMEOUT);
> +			return -ETIMEDOUT;
>   		}
>   
>   		off += segment;
> @@ -310,9 +310,6 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>   
>   	} while (count > 0);
>   
> -	mutex_unlock(&at25->lock);
> -
> -	kfree(bounce);
>   	return status;
>   }
>   
> @@ -429,31 +426,33 @@ static const struct spi_device_id at25_spi_ids[] = {
>   };
>   MODULE_DEVICE_TABLE(spi, at25_spi_ids);
>   
> -static int at25_probe(struct spi_device *spi)
> +static int at25_probe(struct spi_mem *mem)
>   {
> -	struct at25_data	*at25 = NULL;
> -	int			err;
> -	int			sr;
> +	struct spi_device *spi = mem->spi;
>   	struct spi_eeprom *pdata;
> +	struct at25_data *at25;
>   	bool is_fram;
> +	int err;
> +
> +	at25 = devm_kzalloc(&spi->dev, sizeof(*at25), GFP_KERNEL);
> +	if (!at25)
> +		return -ENOMEM;
> +
> +	at25->spimem = mem;
>   
>   	/*
>   	 * Ping the chip ... the status register is pretty portable,
> -	 * unlike probing manufacturer IDs. We do expect that system
> -	 * firmware didn't write it in the past few milliseconds!
> +	 * unlike probing manufacturer IDs.
>   	 */
> -	sr = spi_w8r8(spi, AT25_RDSR);
> -	if (sr < 0 || sr & AT25_SR_nRDY) {
> -		dev_dbg(&spi->dev, "rdsr --> %d (%02x)\n", sr, sr);
> +	err = at25_wait_ready(at25);
> +	if (err < 0)
> +		return dev_err_probe(&spi->dev, err, "Read Status Register command failed\n");
> +	if (err) {
> +		dev_err(&spi->dev, "Not ready (%02x)\n", err);
>   		return -ENXIO;
>   	}
>   
> -	at25 = devm_kzalloc(&spi->dev, sizeof(*at25), GFP_KERNEL);
> -	if (!at25)
> -		return -ENOMEM;
> -
>   	mutex_init(&at25->lock);
> -	at25->spi = spi;
>   	spi_set_drvdata(spi, at25);
>   
>   	is_fram = fwnode_device_is_compatible(dev_fwnode(&spi->dev), "cypress,fm25");
> @@ -514,17 +513,19 @@ static int at25_probe(struct spi_device *spi)
>   
>   /*-------------------------------------------------------------------------*/
>   
> -static struct spi_driver at25_driver = {
> -	.driver = {
> -		.name		= "at25",
> -		.of_match_table = at25_of_match,
> -		.dev_groups	= sernum_groups,
> +static struct spi_mem_driver at25_driver = {
> +	.spidrv = {
> +		.driver = {
> +			.name		= "at25",
> +			.of_match_table = at25_of_match,
> +			.dev_groups	= sernum_groups,
> +		},
> +		.id_table	= at25_spi_ids,
>   	},
>   	.probe		= at25_probe,
> -	.id_table	= at25_spi_ids,
>   };
>   
> -module_spi_driver(at25_driver);
> +module_spi_mem_driver(at25_driver);
>   
>   MODULE_DESCRIPTION("Driver for most SPI EEPROMs");
>   MODULE_AUTHOR("David Brownell");

Powered by blists - more mailing lists