[PATCH 00/24] spi-nand/spi-mem DTR support

[PATCH 00/24] spi-nand/spi-mem DTR support

[Miquel Raynal]

Hello Mark, hello MTD folks,

Here is a (big) series supposed to bring DTR support in SPI-NAND.

I could have split this into two but I eventually preferred showing the
big picture. Once v1 will be over, I can make it two. However when we'll
discuss merging, we'll have to share an immutable tag among the two
subsystems.

Here is the logic:
* patches 1 & 2 add support for spi-mem operations with a specific
  frequency limitation. This is not only related to DTR support, because
  as you can see I could use this to support basic features in the
  Winbond driver.
* patches 3-16 are going through all the easy controller drivers, where
  effectively supporting these per-operation limitation was easy to
  do. In practice, I believe all controllers can, but software is
  sometimes the limiting factor. All controllers without spi-mem support
  will gracefully handle the request (provided that they already care
  about the maximum speed of course), and all the updated controllers in
  this series will also handle the situation correctly. For the others,
  it's an opt-in parameter, so they will simply refuse the operation
  during the checks_op/supports_op() phase.
* patches 17-19 make use of this new possibility to fix Winbond support
* patches 20-24 are actually adding DTR support in SPI-NAND, and making
  this feature useful with Winbond chips.

Thanks,
Miquèl

Miquel Raynal (24):
  spi: spi-mem: Extend spi-mem operations with a per-operation maximum
    frequency
  spi: spi-mem: Add a new controller capability
  spi: amd: Support per spi-mem operation frequency switches
  spi: amlogic-spifc-a1: Support per spi-mem operation frequency
    switches
  spi: cadence-qspi: Support per spi-mem operation frequency switches
  spi: dw: Support per spi-mem operation frequency switches
  spi: fsl-qspi: Support per spi-mem operation frequency switches
  spi: microchip-core-qspi: Support per spi-mem operation frequency
    switches
  spi: mt65xx: Support per spi-mem operation frequency switches
  spi: mxic: Support per spi-mem operation frequency switches
  spi: nxp-fspi: Support per spi-mem operation frequency switches
  spi: rockchip-sfc: Support per spi-mem operation frequency switches
  spi: spi-sn-f-ospi: Support per spi-mem operation frequency switches
  spi: spi-ti-qspi: Support per spi-mem operation frequency switches
  spi: zynq-qspi: Support per spi-mem operation frequency switches
  spi: zynqmp-gqspi: Support per spi-mem operation frequency switches
  mtd: spinand: Create distinct fast and slow read from cache variants
  mtd: spinand: Add an optional frequency to read from cache macros
  mtd: spinand: winbond: Fix the *JW chip definitions
  spi: spi-mem: Reorder SPI_MEM_OP_CMD internals
  spi: spi-mem: Create macros for DTR operation
  mtd: spinand: Add support for read DTR operations
  mtd: spinand: winbond: Add comment about naming
  mtd: spinand: winbond: Add support for DTR operations

 drivers/mtd/nand/spi/alliancememory.c |  4 +-
 drivers/mtd/nand/spi/ato.c            |  4 +-
 drivers/mtd/nand/spi/esmt.c           |  4 +-
 drivers/mtd/nand/spi/foresee.c        |  4 +-
 drivers/mtd/nand/spi/gigadevice.c     | 16 ++++----
 drivers/mtd/nand/spi/macronix.c       |  4 +-
 drivers/mtd/nand/spi/micron.c         |  8 ++--
 drivers/mtd/nand/spi/paragon.c        |  4 +-
 drivers/mtd/nand/spi/toshiba.c        |  4 +-
 drivers/mtd/nand/spi/winbond.c        | 27 +++++++++++--
 drivers/mtd/nand/spi/xtx.c            |  4 +-
 drivers/spi/spi-amd.c                 | 10 ++++-
 drivers/spi/spi-amlogic-spifc-a1.c    |  7 +++-
 drivers/spi/spi-cadence-quadspi.c     |  3 +-
 drivers/spi/spi-dw-core.c             | 10 ++++-
 drivers/spi/spi-fsl-qspi.c            | 12 ++++--
 drivers/spi/spi-mem.c                 | 13 +++++++
 drivers/spi/spi-microchip-core-qspi.c | 26 +++++++++++--
 drivers/spi/spi-mt65xx.c              |  7 +++-
 drivers/spi/spi-mxic.c                |  3 +-
 drivers/spi/spi-nxp-fspi.c            | 12 ++++--
 drivers/spi/spi-rockchip-sfc.c        | 11 ++++--
 drivers/spi/spi-sn-f-ospi.c           |  8 +++-
 drivers/spi/spi-ti-qspi.c             |  7 +++-
 drivers/spi/spi-zynq-qspi.c           | 13 +++++--
 drivers/spi/spi-zynqmp-gqspi.c        | 13 +++++--
 include/linux/mtd/spinand.h           | 56 +++++++++++++++++++++++++--
 include/linux/spi/spi-mem.h           | 56 ++++++++++++++++++++++++++-
 28 files changed, 282 insertions(+), 68 deletions(-)

-- 
2.43.0

[PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Miquel Raynal]

In the spi subsystem, the bus frequency is derived as follows:
- the controller may expose a minimum and maximum operating frequency
- the hardware description, through the spi peripheral properties,
  advise what is the maximum acceptable frequency from a device/wiring
  point of view.
Transfers must be observed at a frequency which fits both (so in
practice, the lowest maximum).

Actually, this second point mixes two information and already takes the
lowest frequency among:
- what the spi device is capable of (what is written in the component
  datasheet)
- what the wiring allows (electromagnetic sensibility, crossovers,
  terminations, antenna effect, etc).

This logic works until spi devices are no longer capable of sustaining
their highest frequency regardless of the operation. Spi memories are
typically subject to such variation. Some devices are capable of
spitting their internally stored data (essentially in read mode) at a
very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using
"fast" commands. However, some of the low-end operations, such as
regular page read-from-cache commands, are more limited and can only be
executed at 54MHz at most. This is currently a problem in the SPI-NAND
subsystem. Another situation, even if not yet supported, will be with
DTR commands, when the data is latched on both edges of the clock. The
same chips as mentioned previously are in this case limited to
80MHz. Yet another example might be continuous reads, which, under
certain circumstances, can also run at most at 104 or 120MHz.

As a matter of fact, the "one frequency per chip" policy is outdated and
more fine grain configuration is needed: we need to allow per-operation
frequency limitations. So far, all datasheets I encountered advertise a
maximum default frequency, which need to be lowered for certain specific
operations. So based on the current infrastructure, we can still expect
firmware (device trees in general) to continued advertising the same
maximum speed which is a mix between the PCB limitations and the chip
maximum capability, and expect per-operation lower frequencies when this
is relevant.

Add a `struct spi_mem_op` member to carry this information. Not
providing this field explicitly from upper layers means that there is no
further constraint and the default spi device maximum speed will be
carried instead. The SPI_MEM_OP() macro is also expanded with an
optional frequency argument, because virtually all operations can be
subject to such a limitation, and this will allow for a smooth and
discrete transition.

For controller drivers which do not implement the spi-mem interface, the
per-transfer speed is also set acordingly to a lower (than the maximum
default) speed, or 0, to comply with the current API.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mem.c       |  8 ++++++++
 include/linux/spi/spi-mem.h | 11 ++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 17b8baf749e6..ab650ae953bb 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
 	struct spi_controller *ctlr = mem->spi->controller;
+	unsigned int xfer_speed = op->max_freq;
 	struct spi_transfer xfers[4] = { };
 	struct spi_message msg;
 	u8 *tmpbuf;
@@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	if (!spi_mem_internal_supports_op(mem, op))
 		return -EOPNOTSUPP;
 
+	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
+		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
+
 	if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) {
 		ret = spi_mem_access_start(mem);
 		if (ret)
@@ -407,6 +411,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	xfers[xferpos].tx_buf = tmpbuf;
 	xfers[xferpos].len = op->cmd.nbytes;
 	xfers[xferpos].tx_nbits = op->cmd.buswidth;
+	xfers[xferpos].speed_hz = xfer_speed;
 	spi_message_add_tail(&xfers[xferpos], &msg);
 	xferpos++;
 	totalxferlen++;
@@ -421,6 +426,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		xfers[xferpos].tx_buf = tmpbuf + 1;
 		xfers[xferpos].len = op->addr.nbytes;
 		xfers[xferpos].tx_nbits = op->addr.buswidth;
+		xfers[xferpos].speed_hz = xfer_speed;
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->addr.nbytes;
@@ -432,6 +438,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		xfers[xferpos].len = op->dummy.nbytes;
 		xfers[xferpos].tx_nbits = op->dummy.buswidth;
 		xfers[xferpos].dummy_data = 1;
+		xfers[xferpos].speed_hz = xfer_speed;
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->dummy.nbytes;
@@ -447,6 +454,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		}
 
 		xfers[xferpos].len = op->data.nbytes;
+		xfers[xferpos].speed_hz = xfer_speed;
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->data.nbytes;
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index f866d5c8ed32..8963f236911b 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -68,6 +68,9 @@ enum spi_mem_data_dir {
 	SPI_MEM_DATA_OUT,
 };
 
+#define SPI_MEM_OP_MAX_FREQ(__freq)				\
+	.max_freq = __freq
+
 /**
  * struct spi_mem_op - describes a SPI memory operation
  * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
@@ -95,6 +98,9 @@ enum spi_mem_data_dir {
  *		 operation does not involve transferring data
  * @data.buf.in: input buffer (must be DMA-able)
  * @data.buf.out: output buffer (must be DMA-able)
+ * @max_freq: frequency limitation wrt this operation. 0 means there is no
+ *	      specific constraint and the highest achievable frequency can be
+ *	      attempted).
  */
 struct spi_mem_op {
 	struct {
@@ -132,14 +138,17 @@ struct spi_mem_op {
 			const void *out;
 		} buf;
 	} data;
+
+	unsigned int max_freq;
 };
 
-#define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
+#define SPI_MEM_OP(__cmd, __addr, __dummy, __data, ...)		\
 	{							\
 		.cmd = __cmd,					\
 		.addr = __addr,					\
 		.dummy = __dummy,				\
 		.data = __data,					\
+		__VA_ARGS__					\
 	}
 
 /**
-- 
2.43.0

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Han Xu]

On 24/10/25 06:14PM, Miquel Raynal wrote:
> In the spi subsystem, the bus frequency is derived as follows:
> - the controller may expose a minimum and maximum operating frequency
> - the hardware description, through the spi peripheral properties,
>   advise what is the maximum acceptable frequency from a device/wiring
>   point of view.
> Transfers must be observed at a frequency which fits both (so in
> practice, the lowest maximum).
> 
> Actually, this second point mixes two information and already takes the
> lowest frequency among:
> - what the spi device is capable of (what is written in the component
>   datasheet)
> - what the wiring allows (electromagnetic sensibility, crossovers,
>   terminations, antenna effect, etc).
> 
> This logic works until spi devices are no longer capable of sustaining
> their highest frequency regardless of the operation. Spi memories are
> typically subject to such variation. Some devices are capable of
> spitting their internally stored data (essentially in read mode) at a
> very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using
> "fast" commands. However, some of the low-end operations, such as
> regular page read-from-cache commands, are more limited and can only be
> executed at 54MHz at most. This is currently a problem in the SPI-NAND
> subsystem. Another situation, even if not yet supported, will be with
> DTR commands, when the data is latched on both edges of the clock. The
> same chips as mentioned previously are in this case limited to
> 80MHz. Yet another example might be continuous reads, which, under
> certain circumstances, can also run at most at 104 or 120MHz.
> 
> As a matter of fact, the "one frequency per chip" policy is outdated and
> more fine grain configuration is needed: we need to allow per-operation
> frequency limitations. So far, all datasheets I encountered advertise a
> maximum default frequency, which need to be lowered for certain specific
> operations. So based on the current infrastructure, we can still expect
> firmware (device trees in general) to continued advertising the same
> maximum speed which is a mix between the PCB limitations and the chip
> maximum capability, and expect per-operation lower frequencies when this
> is relevant.

Hi Miquel,

I met the similar problem when working on the Micron MT35XU01GBBA SPI NOR chip.
The chip can work at 166MHz in SDR mode but 200MHz in DDR mode. I found Read ID
failed on some platforms when using 200MHz as maximum frequency, so I have to
lower the maximum frequency with some performance loss.

I think the patch is useful but the SPI NOR doesn't have such vendor-specific
predefined SPI_MEM_OPS like SPI NAND. Do you have any suggestion on how to handle
this case? Thanks.

> 
> Add a `struct spi_mem_op` member to carry this information. Not
> providing this field explicitly from upper layers means that there is no
> further constraint and the default spi device maximum speed will be
> carried instead. The SPI_MEM_OP() macro is also expanded with an
> optional frequency argument, because virtually all operations can be
> subject to such a limitation, and this will allow for a smooth and
> discrete transition.
> 
> For controller drivers which do not implement the spi-mem interface, the
> per-transfer speed is also set acordingly to a lower (than the maximum
> default) speed, or 0, to comply with the current API.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-mem.c       |  8 ++++++++
>  include/linux/spi/spi-mem.h | 11 ++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 17b8baf749e6..ab650ae953bb 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  {
>  	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
>  	struct spi_controller *ctlr = mem->spi->controller;
> +	unsigned int xfer_speed = op->max_freq;
>  	struct spi_transfer xfers[4] = { };
>  	struct spi_message msg;
>  	u8 *tmpbuf;
> @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	if (!spi_mem_internal_supports_op(mem, op))
>  		return -EOPNOTSUPP;
>  
> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
> +
>  	if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) {
>  		ret = spi_mem_access_start(mem);
>  		if (ret)
> @@ -407,6 +411,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	xfers[xferpos].tx_buf = tmpbuf;
>  	xfers[xferpos].len = op->cmd.nbytes;
>  	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> +	xfers[xferpos].speed_hz = xfer_speed;
>  	spi_message_add_tail(&xfers[xferpos], &msg);
>  	xferpos++;
>  	totalxferlen++;
> @@ -421,6 +426,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		xfers[xferpos].tx_buf = tmpbuf + 1;
>  		xfers[xferpos].len = op->addr.nbytes;
>  		xfers[xferpos].tx_nbits = op->addr.buswidth;
> +		xfers[xferpos].speed_hz = xfer_speed;
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->addr.nbytes;
> @@ -432,6 +438,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		xfers[xferpos].len = op->dummy.nbytes;
>  		xfers[xferpos].tx_nbits = op->dummy.buswidth;
>  		xfers[xferpos].dummy_data = 1;
> +		xfers[xferpos].speed_hz = xfer_speed;
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->dummy.nbytes;
> @@ -447,6 +454,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		}
>  
>  		xfers[xferpos].len = op->data.nbytes;
> +		xfers[xferpos].speed_hz = xfer_speed;
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->data.nbytes;
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index f866d5c8ed32..8963f236911b 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -68,6 +68,9 @@ enum spi_mem_data_dir {
>  	SPI_MEM_DATA_OUT,
>  };
>  
> +#define SPI_MEM_OP_MAX_FREQ(__freq)				\
> +	.max_freq = __freq
> +
>  /**
>   * struct spi_mem_op - describes a SPI memory operation
>   * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> @@ -95,6 +98,9 @@ enum spi_mem_data_dir {
>   *		 operation does not involve transferring data
>   * @data.buf.in: input buffer (must be DMA-able)
>   * @data.buf.out: output buffer (must be DMA-able)
> + * @max_freq: frequency limitation wrt this operation. 0 means there is no
> + *	      specific constraint and the highest achievable frequency can be
> + *	      attempted).
>   */
>  struct spi_mem_op {
>  	struct {
> @@ -132,14 +138,17 @@ struct spi_mem_op {
>  			const void *out;
>  		} buf;
>  	} data;
> +
> +	unsigned int max_freq;
>  };
>  
> -#define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
> +#define SPI_MEM_OP(__cmd, __addr, __dummy, __data, ...)		\
>  	{							\
>  		.cmd = __cmd,					\
>  		.addr = __addr,					\
>  		.dummy = __dummy,				\
>  		.data = __data,					\
> +		__VA_ARGS__					\
>  	}
>  
>  /**
> -- 
> 2.43.0
> 

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Tudor Ambarus]

On 10/30/24 8:52 PM, Han Xu wrote:
> Hi Miquel,

Hi!

> 
> I met the similar problem when working on the Micron MT35XU01GBBA SPI NOR chip.
> The chip can work at 166MHz in SDR mode but 200MHz in DDR mode. I found Read ID
> failed on some platforms when using 200MHz as maximum frequency, so I have to
> lower the maximum frequency with some performance loss.
> 
> I think the patch is useful but the SPI NOR doesn't have such vendor-specific
> predefined SPI_MEM_OPS like SPI NAND. Do you have any suggestion on how to handle
> this case? Thanks.

Why can't we add similar predefined SPI_MEM_OPS in SPI NOR?

Cheers,
ta

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:

cut

> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 17b8baf749e6..ab650ae953bb 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  {
>  	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
>  	struct spi_controller *ctlr = mem->spi->controller;
> +	unsigned int xfer_speed = op->max_freq;

be aware that for controllers that don't support SPIMEM ops, you pass
the frequency from the upper layers, without adjusting it with
spi->max_speed_hz. Was this intentional?


>  	struct spi_transfer xfers[4] = { };
>  	struct spi_message msg;
>  	u8 *tmpbuf;
> @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	if (!spi_mem_internal_supports_op(mem, op))
>  		return -EOPNOTSUPP;
>  
> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;

not a big fan of casting the const out. How about introducing a
spi_mem_adjust_op_freq()? The upper layers will use that were needed,
and you'll still be able to pass a const op to spi_mem_exec_op()

cut

> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index f866d5c8ed32..8963f236911b 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -68,6 +68,9 @@ enum spi_mem_data_dir {
>  	SPI_MEM_DATA_OUT,
>  };
>  
> +#define SPI_MEM_OP_MAX_FREQ(__freq)				\
> +	.max_freq = __freq
> +
>  /**
>   * struct spi_mem_op - describes a SPI memory operation
>   * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> @@ -95,6 +98,9 @@ enum spi_mem_data_dir {
>   *		 operation does not involve transferring data
>   * @data.buf.in: input buffer (must be DMA-able)
>   * @data.buf.out: output buffer (must be DMA-able)
> + * @max_freq: frequency limitation wrt this operation. 0 means there is no
> + *	      specific constraint and the highest achievable frequency can be
> + *	      attempted).

nit: you close a parenthesis without opening one

Looking good,
ta

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Miquel Raynal]

Hello Tudor,

On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>
> cut
>
>> 
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index 17b8baf749e6..ab650ae953bb 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  {
>>  	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
>>  	struct spi_controller *ctlr = mem->spi->controller;
>> +	unsigned int xfer_speed = op->max_freq;
>
> be aware that for controllers that don't support SPIMEM ops, you pass
> the frequency from the upper layers, without adjusting it with
> spi->max_speed_hz. Was this intentional?

That is exactly the opposite of what I wanted to achieve
initially. That's a very good catch.

>>  	struct spi_transfer xfers[4] = { };
>>  	struct spi_message msg;
>>  	u8 *tmpbuf;
>> @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  	if (!spi_mem_internal_supports_op(mem, op))
>>  		return -EOPNOTSUPP;
>>  
>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>
> not a big fan of casting the const out. How about introducing a
> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
> and you'll still be able to pass a const op to spi_mem_exec_op()

I know it is not ideal so to follow your idea I drafted the use of
spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
to call this function everywhere in the core and the drivers to make
sure we never get out of bounds, but here is the problem:

    $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
    42

This approach requires to add a call to spi_mem_adjust_op_freq() before
*every* spi_mem_exec_op(). Yes I can do that but that means to be very
attentive to the fact that these two functions are always called
together. I am not sure it is a good idea.

What about doing the following once in spi_mem_exec_op() instead?

    spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);

I know we still have a cast, but it feels more acceptable than the one I
initially proposed and covers all cases. I would not accept that in a
driver, but here we are in the core, so that sounds acceptable.

Another possibility otherwise would be to drop the const from the
spi_mem_op structure entirely. But I prefer the above function call.

>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index f866d5c8ed32..8963f236911b 100644
>> --- a/include/linux/spi/spi-mem.h
>> +++ b/include/linux/spi/spi-mem.h
>> @@ -68,6 +68,9 @@ enum spi_mem_data_dir {
>>  	SPI_MEM_DATA_OUT,
>>  };
>>  
>> +#define SPI_MEM_OP_MAX_FREQ(__freq)				\
>> +	.max_freq = __freq
>> +
>>  /**
>>   * struct spi_mem_op - describes a SPI memory operation
>>   * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
>> @@ -95,6 +98,9 @@ enum spi_mem_data_dir {
>>   *		 operation does not involve transferring data
>>   * @data.buf.in: input buffer (must be DMA-able)
>>   * @data.buf.out: output buffer (must be DMA-able)
>> + * @max_freq: frequency limitation wrt this operation. 0 means there is no
>> + *	      specific constraint and the highest achievable frequency can be
>> + *	      attempted).
>
> nit: you close a parenthesis without opening one

Corrected!

Thanks for this very useful feedback!
Miquèl

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Tudor Ambarus]

On 12/13/24 10:46 AM, Miquel Raynal wrote:
> Hello Tudor,
> 

Hi!

> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>
>> cut
>>
>>>
>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>> index 17b8baf749e6..ab650ae953bb 100644
>>> --- a/drivers/spi/spi-mem.c
>>> +++ b/drivers/spi/spi-mem.c

cut

>>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>>
>> not a big fan of casting the const out. How about introducing a
>> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
>> and you'll still be able to pass a const op to spi_mem_exec_op()
> 
> I know it is not ideal so to follow your idea I drafted the use of
> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
> to call this function everywhere in the core and the drivers to make
> sure we never get out of bounds, but here is the problem:
> 
>     $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
>     42
> 
> This approach requires to add a call to spi_mem_adjust_op_freq() before
> *every* spi_mem_exec_op(). Yes I can do that but that means to be very
> attentive to the fact that these two functions are always called
> together. I am not sure it is a good idea.
> 
> What about doing the following once in spi_mem_exec_op() instead?
> 
>     spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);
> 
> I know we still have a cast, but it feels more acceptable than the one I
> initially proposed and covers all cases. I would not accept that in a
> driver, but here we are in the core, so that sounds acceptable.
> 
> Another possibility otherwise would be to drop the const from the
> spi_mem_op structure entirely. But I prefer the above function call.

How about introducing a spi_nand_spimem_exec_op() where you call
spi_mem_adjust_op_freq() and spi_mem_exec_op()?

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Miquel Raynal]

On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 12/13/24 10:46 AM, Miquel Raynal wrote:
>> Hello Tudor,
>> 
>
> Hi!
>
>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>> 
>>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>>
>>> cut
>>>
>>>>
>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>>> index 17b8baf749e6..ab650ae953bb 100644
>>>> --- a/drivers/spi/spi-mem.c
>>>> +++ b/drivers/spi/spi-mem.c
>
> cut
>
>>>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>>>
>>> not a big fan of casting the const out. How about introducing a
>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
>>> and you'll still be able to pass a const op to spi_mem_exec_op()
>> 
>> I know it is not ideal so to follow your idea I drafted the use of
>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
>> to call this function everywhere in the core and the drivers to make
>> sure we never get out of bounds, but here is the problem:
>> 
>>     $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
>>     42
>> 
>> This approach requires to add a call to spi_mem_adjust_op_freq() before
>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very
>> attentive to the fact that these two functions are always called
>> together. I am not sure it is a good idea.
>> 
>> What about doing the following once in spi_mem_exec_op() instead?
>> 
>>     spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);
>> 
>> I know we still have a cast, but it feels more acceptable than the one I
>> initially proposed and covers all cases. I would not accept that in a
>> driver, but here we are in the core, so that sounds acceptable.
>> 
>> Another possibility otherwise would be to drop the const from the
>> spi_mem_op structure entirely. But I prefer the above function call.
>
> How about introducing a spi_nand_spimem_exec_op() where you call
> spi_mem_adjust_op_freq() and spi_mem_exec_op()?

That would work to make the cast disappear but TBH would not be totally
relevant as adjusting the frequency is typically something that would
benefit to spi-nor as well (maybe in the future) and therefore would
fully apply to spi memories as a whole, not just spi-nand. We can think
about another naming maybe, but I find like spi_mem_exec_op() is the
right location to do this.

Thanks,
Miquèl

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Tudor Ambarus]

On 12/18/24 9:37 AM, Miquel Raynal wrote:
> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 12/13/24 10:46 AM, Miquel Raynal wrote:
>>> Hello Tudor,
>>>
>>
>> Hi!
>>
>>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>>>
>>>> cut
>>>>
>>>>>
>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>>>> index 17b8baf749e6..ab650ae953bb 100644
>>>>> --- a/drivers/spi/spi-mem.c
>>>>> +++ b/drivers/spi/spi-mem.c
>>
>> cut
>>
>>>>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>>>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>>>>
>>>> not a big fan of casting the const out. How about introducing a
>>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
>>>> and you'll still be able to pass a const op to spi_mem_exec_op()
>>>
>>> I know it is not ideal so to follow your idea I drafted the use of
>>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
>>> to call this function everywhere in the core and the drivers to make
>>> sure we never get out of bounds, but here is the problem:
>>>
>>>     $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
>>>     42
>>>
>>> This approach requires to add a call to spi_mem_adjust_op_freq() before
>>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very
>>> attentive to the fact that these two functions are always called
>>> together. I am not sure it is a good idea.
>>>
>>> What about doing the following once in spi_mem_exec_op() instead?
>>>
>>>     spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);
>>>
>>> I know we still have a cast, but it feels more acceptable than the one I
>>> initially proposed and covers all cases. I would not accept that in a
>>> driver, but here we are in the core, so that sounds acceptable.
>>>
>>> Another possibility otherwise would be to drop the const from the
>>> spi_mem_op structure entirely. But I prefer the above function call.
>>
>> How about introducing a spi_nand_spimem_exec_op() where you call
>> spi_mem_adjust_op_freq() and spi_mem_exec_op()?
> 
> That would work to make the cast disappear but TBH would not be totally
> relevant as adjusting the frequency is typically something that would
> benefit to spi-nor as well (maybe in the future) and therefore would

Right, SPI NOR will benefit of this too.

> fully apply to spi memories as a whole, not just spi-nand. We can think
> about another naming maybe, but I find like spi_mem_exec_op() is the
> right location to do this.
> 

It's not the first time that we adjust spi_mem_op parameters, see:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153

Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it
calls it when using dirmap, but not with a plain spi_mem_exec_op().

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Tudor Ambarus]

On 12/18/24 10:03 AM, Tudor Ambarus wrote:
> 
> 
> On 12/18/24 9:37 AM, Miquel Raynal wrote:
>> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>>> On 12/13/24 10:46 AM, Miquel Raynal wrote:
>>>> Hello Tudor,
>>>>
>>>
>>> Hi!
>>>
>>>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>
>>>>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>>>>
>>>>> cut
>>>>>
>>>>>>
>>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>>>>> index 17b8baf749e6..ab650ae953bb 100644
>>>>>> --- a/drivers/spi/spi-mem.c
>>>>>> +++ b/drivers/spi/spi-mem.c
>>>
>>> cut
>>>
>>>>>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>>>>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>>>>>
>>>>> not a big fan of casting the const out. How about introducing a
>>>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
>>>>> and you'll still be able to pass a const op to spi_mem_exec_op()
>>>>
>>>> I know it is not ideal so to follow your idea I drafted the use of
>>>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
>>>> to call this function everywhere in the core and the drivers to make
>>>> sure we never get out of bounds, but here is the problem:
>>>>
>>>>     $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
>>>>     42
>>>>
>>>> This approach requires to add a call to spi_mem_adjust_op_freq() before
>>>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very
>>>> attentive to the fact that these two functions are always called
>>>> together. I am not sure it is a good idea.
>>>>
>>>> What about doing the following once in spi_mem_exec_op() instead?
>>>>
>>>>     spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);
>>>>
>>>> I know we still have a cast, but it feels more acceptable than the one I
>>>> initially proposed and covers all cases. I would not accept that in a
>>>> driver, but here we are in the core, so that sounds acceptable.
>>>>
>>>> Another possibility otherwise would be to drop the const from the
>>>> spi_mem_op structure entirely. But I prefer the above function call.
>>>
>>> How about introducing a spi_nand_spimem_exec_op() where you call
>>> spi_mem_adjust_op_freq() and spi_mem_exec_op()?
>>
>> That would work to make the cast disappear but TBH would not be totally
>> relevant as adjusting the frequency is typically something that would
>> benefit to spi-nor as well (maybe in the future) and therefore would
> 
> Right, SPI NOR will benefit of this too.
> 
>> fully apply to spi memories as a whole, not just spi-nand. We can think
>> about another naming maybe, but I find like spi_mem_exec_op() is the
>> right location to do this.
>>
> 
> It's not the first time that we adjust spi_mem_op parameters, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153
> 
> Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it
> calls it when using dirmap, but not with a plain spi_mem_exec_op().
> 

I ask because I'm thinking of adding in the SPIMEM core a prepare()
method, and maybe rename exec_op() to exec(). And then introduce a
prepare_exec() method that the upper layers would call? Similar to
clk_prepare_enable.

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Miquel Raynal]

Hello,

On 18/12/2024 at 10:13:39 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 12/18/24 10:03 AM, Tudor Ambarus wrote:
>> 
>> 
>> On 12/18/24 9:37 AM, Miquel Raynal wrote:
>>> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>>> On 12/13/24 10:46 AM, Miquel Raynal wrote:
>>>>> Hello Tudor,
>>>>>
>>>>
>>>> Hi!
>>>>
>>>>> On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>>
>>>>>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>>>>>
>>>>>> cut
>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>>>>>> index 17b8baf749e6..ab650ae953bb 100644
>>>>>>> --- a/drivers/spi/spi-mem.c
>>>>>>> +++ b/drivers/spi/spi-mem.c
>>>>
>>>> cut
>>>>
>>>>>>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>>>>>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>>>>>>
>>>>>> not a big fan of casting the const out. How about introducing a
>>>>>> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
>>>>>> and you'll still be able to pass a const op to spi_mem_exec_op()
>>>>>
>>>>> I know it is not ideal so to follow your idea I drafted the use of
>>>>> spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
>>>>> to call this function everywhere in the core and the drivers to make
>>>>> sure we never get out of bounds, but here is the problem:
>>>>>
>>>>>     $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
>>>>>     42
>>>>>
>>>>> This approach requires to add a call to spi_mem_adjust_op_freq() before
>>>>> *every* spi_mem_exec_op(). Yes I can do that but that means to be very
>>>>> attentive to the fact that these two functions are always called
>>>>> together. I am not sure it is a good idea.
>>>>>
>>>>> What about doing the following once in spi_mem_exec_op() instead?
>>>>>
>>>>>     spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);
>>>>>
>>>>> I know we still have a cast, but it feels more acceptable than the one I
>>>>> initially proposed and covers all cases. I would not accept that in a
>>>>> driver, but here we are in the core, so that sounds acceptable.
>>>>>
>>>>> Another possibility otherwise would be to drop the const from the
>>>>> spi_mem_op structure entirely. But I prefer the above function call.
>>>>
>>>> How about introducing a spi_nand_spimem_exec_op() where you call
>>>> spi_mem_adjust_op_freq() and spi_mem_exec_op()?
>>>
>>> That would work to make the cast disappear but TBH would not be totally
>>> relevant as adjusting the frequency is typically something that would
>>> benefit to spi-nor as well (maybe in the future) and therefore would
>> 
>> Right, SPI NOR will benefit of this too.
>> 
>>> fully apply to spi memories as a whole, not just spi-nand. We can think
>>> about another naming maybe, but I find like spi_mem_exec_op() is the
>>> right location to do this.
>>>
>> 
>> It's not the first time that we adjust spi_mem_op parameters, see:
>> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153
>> 
>> Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it
>> calls it when using dirmap, but not with a plain spi_mem_exec_op().
>> 
>
> I ask because I'm thinking of adding in the SPIMEM core a prepare()
> method, and maybe rename exec_op() to exec(). And then introduce a
> prepare_exec() method that the upper layers would call? Similar to
> clk_prepare_enable.

Do you have something else in mind you would like to put in the prepare
step? I am not at all opposed to it, but I feel like for now the
spi_mem_exec_op() is a fine path for that, especially since there are
very little things to "prepare" (for now).

Do you mind if I keep the cast (not the one from the series, I cleaned
that up to have an adjust_op function as discussed) for now, and if you
ever go the prepare/exec path we would get this converted to use the new
API? I'd like to make progresses on other topics in the spi-nand core
and avoid being blocked by a bigger task that we need to discuss first.

Cheers,
Miquèl

Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Pratyush Yadav]

On Fri, Oct 25 2024, Miquel Raynal wrote:

> In the spi subsystem, the bus frequency is derived as follows:
> - the controller may expose a minimum and maximum operating frequency
> - the hardware description, through the spi peripheral properties,
>   advise what is the maximum acceptable frequency from a device/wiring
>   point of view.
> Transfers must be observed at a frequency which fits both (so in
> practice, the lowest maximum).
>
> Actually, this second point mixes two information and already takes the
> lowest frequency among:
> - what the spi device is capable of (what is written in the component
>   datasheet)
> - what the wiring allows (electromagnetic sensibility, crossovers,
>   terminations, antenna effect, etc).
>
> This logic works until spi devices are no longer capable of sustaining
> their highest frequency regardless of the operation. Spi memories are
> typically subject to such variation. Some devices are capable of
> spitting their internally stored data (essentially in read mode) at a
> very fast rate, typically up to 166MHz on Winbond SPI-NAND chips, using
> "fast" commands. However, some of the low-end operations, such as
> regular page read-from-cache commands, are more limited and can only be
> executed at 54MHz at most. This is currently a problem in the SPI-NAND
> subsystem. Another situation, even if not yet supported, will be with
> DTR commands, when the data is latched on both edges of the clock. The
> same chips as mentioned previously are in this case limited to
> 80MHz. Yet another example might be continuous reads, which, under
> certain circumstances, can also run at most at 104 or 120MHz.

It's been a while so I don't remember the specifics anymore, but IIRC
some NOR flashes had the same issue. Some commands could only run at a
lower frequency.

>
> As a matter of fact, the "one frequency per chip" policy is outdated and
> more fine grain configuration is needed: we need to allow per-operation
> frequency limitations. So far, all datasheets I encountered advertise a
> maximum default frequency, which need to be lowered for certain specific
> operations. So based on the current infrastructure, we can still expect
> firmware (device trees in general) to continued advertising the same
> maximum speed which is a mix between the PCB limitations and the chip
> maximum capability, and expect per-operation lower frequencies when this
> is relevant.
>
> Add a `struct spi_mem_op` member to carry this information. Not
> providing this field explicitly from upper layers means that there is no
> further constraint and the default spi device maximum speed will be
> carried instead. The SPI_MEM_OP() macro is also expanded with an
> optional frequency argument, because virtually all operations can be
> subject to such a limitation, and this will allow for a smooth and
> discrete transition.
>
> For controller drivers which do not implement the spi-mem interface, the
> per-transfer speed is also set acordingly to a lower (than the maximum
> default) speed, or 0, to comply with the current API.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
[...]
>  /**
>   * struct spi_mem_op - describes a SPI memory operation
>   * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> @@ -95,6 +98,9 @@ enum spi_mem_data_dir {
>   *		 operation does not involve transferring data
>   * @data.buf.in: input buffer (must be DMA-able)
>   * @data.buf.out: output buffer (must be DMA-able)
> + * @max_freq: frequency limitation wrt this operation. 0 means there is no
> + *	      specific constraint and the highest achievable frequency can be
> + *	      attempted).
>   */
>  struct spi_mem_op {
>  	struct {
> @@ -132,14 +138,17 @@ struct spi_mem_op {
>  			const void *out;
>  		} buf;
>  	} data;
> +
> +	unsigned int max_freq;

So we limit the maximum frequency to roughly 4.2 GHz. Seems reasonable,
considering the top end NOR flashes do up to 200-300 MHz.

Didn't look too closely at the code but the idea seems good to me.

Acked-by: Pratyush Yadav <pratyush@kernel.org>


-- 
Regards,
Pratyush Yadav

[PATCH 02/24] spi: spi-mem: Add a new controller capability

[Miquel Raynal]

There are spi devices with multiple frequency limitations depending on
the invoked command. We probably do not want to afford running at the
lowest supported frequency all the time, so if we want to get the most
of our hardware, we need to allow per-operation frequency limitations.

Among all the spi-memory controllers, I believe all are capable of
changing the spi frequency on the fly. Some of the drivers do not make
any frequency setup though. And some others will derive a per-chip
pre-scaler value which will be used forever.

Actually changing the frequency on the fly is something new in Linux, so
we need to carefully flag the drivers which do and do not support it. A
controller capability is created for that, and the presence for this
capability will always be checked before accepting such pattern.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mem.c       | 5 +++++
 include/linux/spi/spi-mem.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index ab650ae953bb..102d351c3d04 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -184,6 +184,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 			return false;
 	}
 
+	if (op->max_freq < mem->spi->max_speed_hz) {
+		if (!spi_mem_controller_is_capable(ctlr, per_op_freq))
+			return false;
+	}
+
 	return spi_mem_check_buswidth(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 8963f236911b..379c048b2eb4 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -306,10 +306,12 @@ struct spi_controller_mem_ops {
  * struct spi_controller_mem_caps - SPI memory controller capabilities
  * @dtr: Supports DTR operations
  * @ecc: Supports operations with error correction
+ * @per_op_freq: Supports per-operation frequency switching
  */
 struct spi_controller_mem_caps {
 	bool dtr;
 	bool ecc;
+	bool per_op_freq;
 };
 
 #define spi_mem_controller_is_capable(ctlr, cap)	\
-- 
2.43.0

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote:
> There are spi devices with multiple frequency limitations depending on
> the invoked command. We probably do not want to afford running at the
> lowest supported frequency all the time, so if we want to get the most
> of our hardware, we need to allow per-operation frequency limitations.

This all makes sense to me.  I'll leave it a little bit to see if anyone
(especially people working with the individual devices) has any thoughts
and assuming no issues apply the SPI bits on a branch.  Does that sound
sensible?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote:
> There are spi devices with multiple frequency limitations depending on
> the invoked command. We probably do not want to afford running at the
> lowest supported frequency all the time, so if we want to get the most
> of our hardware, we need to allow per-operation frequency limitations.

After applying this patch (I bisected the series) my Avenger96 board
started failing to probe the SPI NOR flash it has:

[    3.567876] spi-nor spi0.0: probe with driver spi-nor failed with error -95

Full job:

   https://lava.sirena.org.uk/scheduler/job/925156

I didn't spot anything with the code on a recheck but it's late on a
Friday so I've not looked too hard.  My other boards are all fine though
there's limited coverage.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Miquel Raynal]

Hi Mark,

Thanks a lot for the testing and sorry for being slow.

On 01/11/2024 at 20:17:33 GMT, Mark Brown <broonie@kernel.org> wrote:

> On Fri, Oct 25, 2024 at 06:14:39PM +0200, Miquel Raynal wrote:
>> There are spi devices with multiple frequency limitations depending on
>> the invoked command. We probably do not want to afford running at the
>> lowest supported frequency all the time, so if we want to get the most
>> of our hardware, we need to allow per-operation frequency limitations.
>
> After applying this patch (I bisected the series) my Avenger96 board
> started failing to probe the SPI NOR flash it has:
>
> [    3.567876] spi-nor spi0.0: probe with driver spi-nor failed with
> error -95

This is an EOPNOTSUPP so maybe there is a new check that is breaking
your board. I checked the hardware manual, they talk about a NOR
flash. Looking at the code, I believe I forgot the SPI-NOR case which
currently does not (yet?) use the op->max_freq parameter.

> Full job:
>
>    https://lava.sirena.org.uk/scheduler/job/925156
>
> I didn't spot anything with the code on a recheck but it's late on a
> Friday so I've not looked too hard.  My other boards are all fine though
> there's limited coverage.

Would you mind testing the series with this change on top and tell me if
that fixes it?

--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -184,7 +184,7 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
                        return false;
        }
 
-       if (op->max_freq < mem->spi->max_speed_hz) {
+       if (op->max_freq && op->max_freq < mem->spi->max_speed_hz) {
                if (!spi_mem_controller_is_capable(ctlr, per_op_freq))
                        return false;
        }

I don't know how easy it is for you to make that test with lava, let me
know if you prefer me to send a fixup! patch or even resend the whole
series (but it's a bit big).

Thanks,
Miquèl

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On Thu, Nov 07, 2024 at 11:40:00AM +0100, Miquel Raynal wrote:
> On 01/11/2024 at 20:17:33 GMT, Mark Brown <broonie@kernel.org> wrote:

> > After applying this patch (I bisected the series) my Avenger96 board
> > started failing to probe the SPI NOR flash it has:

> > [    3.567876] spi-nor spi0.0: probe with driver spi-nor failed with
> > error -95

> Would you mind testing the series with this change on top and tell me if
> that fixes it?
> 
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -184,7 +184,7 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>                         return false;
>         }
>  
> -       if (op->max_freq < mem->spi->max_speed_hz) {
> +       if (op->max_freq && op->max_freq < mem->spi->max_speed_hz) {
>                 if (!spi_mem_controller_is_capable(ctlr, per_op_freq))
>                         return false;
>         }

Yes, that seems to have been the issue.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Miquel Raynal]

On 07/11/2024 at 17:15:03 GMT, Mark Brown <broonie@kernel.org> wrote:

> On Thu, Nov 07, 2024 at 11:40:00AM +0100, Miquel Raynal wrote:
>> On 01/11/2024 at 20:17:33 GMT, Mark Brown <broonie@kernel.org> wrote:
>
>> > After applying this patch (I bisected the series) my Avenger96 board
>> > started failing to probe the SPI NOR flash it has:
>
>> > [    3.567876] spi-nor spi0.0: probe with driver spi-nor failed with
>> > error -95
>
>> Would you mind testing the series with this change on top and tell me if
>> that fixes it?
>> 
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -184,7 +184,7 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>>                         return false;
>>         }
>>  
>> -       if (op->max_freq < mem->spi->max_speed_hz) {
>> +       if (op->max_freq && op->max_freq < mem->spi->max_speed_hz) {
>>                 if (!spi_mem_controller_is_capable(ctlr, per_op_freq))
>>                         return false;
>>         }
>
> Yes, that seems to have been the issue.

Great, thanks for testing. I'll soon send a v2, but I guess that's too
late for this merge window.

Regarding how to apply, I believe I'll have more spi-nand patches on top
of that in the next cycle, so either I apply them with your Ack and
share an immutable tag, or you apply it and give me one. Either ways
works fine for me. It's more work to create the branch/tag so I can
handle it (once we settle on the content ofc).

Cheers,
Miquèl

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Fri, Nov 08, 2024 at 09:55:07AM +0100, Miquel Raynal wrote:
> On 07/11/2024 at 17:15:03 GMT, Mark Brown <broonie@kernel.org> wrote:

> > Yes, that seems to have been the issue.

> Great, thanks for testing. I'll soon send a v2, but I guess that's too
> late for this merge window.

It should be fine at least for the core bits, the driver bits it might
be better to hold off for the next release but the core changes
shouldn't actually do anything until the drivers start enabling them so
they ought to be relatively safe.  That'd also make merging easier as
there'd be no cross tree issues.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:

cut

> 
> Among all the spi-memory controllers, I believe all are capable of

nit: SPI memory? or spi memory? but no "-" I think
> changing the spi frequency on the fly. Some of the drivers do not make
> any frequency setup though. And some others will derive a per-chip

nit: s/per-chip/per chip?

> pre-scaler value which will be used forever.

nit: s/pre-scaler/prescaler?

cut

> index 8963f236911b..379c048b2eb4 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -306,10 +306,12 @@ struct spi_controller_mem_ops {
>   * struct spi_controller_mem_caps - SPI memory controller capabilities
>   * @dtr: Supports DTR operations
>   * @ecc: Supports operations with error correction
> + * @per_op_freq: Supports per-operation frequency switching

nit: s/per-operation/per operation?

If you fix the bug that you identified you can add my R-b tag,
regardless if you address these nits or not:

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH 02/24] spi: spi-mem: Add a new controller capability

[Miquel Raynal]

Hi Tudor,

>> Among all the spi-memory controllers, I believe all are capable of
>
> nit: SPI memory? or spi memory? but no "-" I think

...

>> changing the spi frequency on the fly. Some of the drivers do not make
>> any frequency setup though. And some others will derive a per-chip
>
> nit: s/per-chip/per chip?

...

>> pre-scaler value which will be used forever.
>
> nit: s/pre-scaler/prescaler?

...

>> + * @per_op_freq: Supports per-operation frequency switching
>
> nit: s/per-operation/per operation?
>
> If you fix the bug that you identified you can add my R-b tag,
> regardless if you address these nits or not:
>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

No idea why I like '-' so much. I removed them all as advised :-)

Thanks!
Miquèl

[PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

This controller however performed a frequency check, which is also
observed during the ->check_op() phase.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Sanjay R Mehta <sanju.mehta@amd.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-amd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 2245ad54b03a..f58dc6375582 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -368,6 +368,9 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
 	    op->data.buswidth > 1 || op->data.nbytes > AMD_SPI_MAX_DATA)
 		return false;
 
+	if (op->max_freq < AMD_SPI_MIN_HZ)
+		return false;
+
 	return spi_mem_default_supports_op(mem, op);
 }
 
@@ -443,7 +446,7 @@ static int amd_spi_exec_mem_op(struct spi_mem *mem,
 
 	amd_spi = spi_controller_get_devdata(mem->spi->controller);
 
-	ret = amd_set_spi_freq(amd_spi, mem->spi->max_speed_hz);
+	ret = amd_set_spi_freq(amd_spi, op->max_freq);
 	if (ret)
 		return ret;
 
@@ -469,6 +472,10 @@ static const struct spi_controller_mem_ops amd_spi_mem_ops = {
 	.supports_op = amd_spi_supports_op,
 };
 
+static const struct spi_controller_mem_caps amd_spi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int amd_spi_host_transfer(struct spi_controller *host,
 				   struct spi_message *msg)
 {
@@ -521,6 +528,7 @@ static int amd_spi_probe(struct platform_device *pdev)
 	host->setup = amd_spi_host_setup;
 	host->transfer_one_message = amd_spi_host_transfer;
 	host->mem_ops = &amd_spi_mem_ops;
+	host->mem_caps = &amd_spi_mem_caps;
 	host->max_transfer_size = amd_spi_max_transfer_size;
 	host->max_message_size = amd_spi_max_transfer_size;
 
-- 
2.43.0

Re: [PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Every ->exec_op() call correctly configures the spi bus speed to the
> maximum allowed frequency for the memory using the constant spi default
> parameter. Since we can now have per-operation constraints, let's use
> the value that comes from the spi-mem operation structure instead. In
> case there is no specific limitation for this operation, the default spi
> device value will be given anyway.
> 
> This controller however performed a frequency check, which is also
> observed during the ->check_op() phase.
> 
> The per-operation frequency capability is thus advertised to the spi-mem
> core.
> 
> Cc: Sanjay R Mehta <sanju.mehta@amd.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-amd.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
> index 2245ad54b03a..f58dc6375582 100644
> --- a/drivers/spi/spi-amd.c
> +++ b/drivers/spi/spi-amd.c
> @@ -368,6 +368,9 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
>  	    op->data.buswidth > 1 || op->data.nbytes > AMD_SPI_MAX_DATA)
>  		return false;
>  
> +	if (op->max_freq < AMD_SPI_MIN_HZ)

How about using mem->spi->controller->min_speed_hz intead?

> +		return false;

I find the check fine here, but I see however that amd_set_spi_freq()
duplicates the same, returning -EINVAL when speed_hz < AMD_SPI_MIN_HZ
> +
>  	return spi_mem_default_supports_op(mem, op);
>  }
>  
> @@ -443,7 +446,7 @@ static int amd_spi_exec_mem_op(struct spi_mem *mem,
>  
>  	amd_spi = spi_controller_get_devdata(mem->spi->controller);
>  
> -	ret = amd_set_spi_freq(amd_spi, mem->spi->max_speed_hz);
> +	ret = amd_set_spi_freq(amd_spi, op->max_freq);
>  	if (ret)
>  		return ret;

however the return code is checked just on this call, and completely
ignored in the 2 other calls. I find the code a bit ugly for the non
SPIMEM case, but maybe something for the amd owner to address.

Looking good,
ta
>  
> @@ -469,6 +472,10 @@ static const struct spi_controller_mem_ops amd_spi_mem_ops = {
>  	.supports_op = amd_spi_supports_op,
>  };
>  
> +static const struct spi_controller_mem_caps amd_spi_mem_caps = {
> +	.per_op_freq = true,
> +};
> +
>  static int amd_spi_host_transfer(struct spi_controller *host,
>  				   struct spi_message *msg)
>  {
> @@ -521,6 +528,7 @@ static int amd_spi_probe(struct platform_device *pdev)
>  	host->setup = amd_spi_host_setup;
>  	host->transfer_one_message = amd_spi_host_transfer;
>  	host->mem_ops = &amd_spi_mem_ops;
> +	host->mem_caps = &amd_spi_mem_caps;
>  	host->max_transfer_size = amd_spi_max_transfer_size;
>  	host->max_message_size = amd_spi_max_transfer_size;
>  

Re: [PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches

[Miquel Raynal]

Hi Tudor,

On 11/11/2024 at 13:36:15 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>> Every ->exec_op() call correctly configures the spi bus speed to the
>> maximum allowed frequency for the memory using the constant spi default
>> parameter. Since we can now have per-operation constraints, let's use
>> the value that comes from the spi-mem operation structure instead. In
>> case there is no specific limitation for this operation, the default spi
>> device value will be given anyway.
>> 
>> This controller however performed a frequency check, which is also
>> observed during the ->check_op() phase.
>> 
>> The per-operation frequency capability is thus advertised to the spi-mem
>> core.
>> 
>> Cc: Sanjay R Mehta <sanju.mehta@amd.com>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/spi/spi-amd.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
>> index 2245ad54b03a..f58dc6375582 100644
>> --- a/drivers/spi/spi-amd.c
>> +++ b/drivers/spi/spi-amd.c
>> @@ -368,6 +368,9 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
>>  	    op->data.buswidth > 1 || op->data.nbytes > AMD_SPI_MAX_DATA)
>>  		return false;
>>  
>> +	if (op->max_freq < AMD_SPI_MIN_HZ)
>
> How about using mem->spi->controller->min_speed_hz intead?

Good idea. I think I used AMD_SPI_MIN_HZ to follow what was done
somewhere else, but that's fine.

>
>> +		return false;
>
> I find the check fine here, but I see however that amd_set_spi_freq()
> duplicates the same, returning -EINVAL when speed_hz < AMD_SPI_MIN_HZ

This one is useless, the spi core already takes care of this check, I'll
drop it in a separate patch.

>> +
>>  	return spi_mem_default_supports_op(mem, op);
>>  }
>>  
>> @@ -443,7 +446,7 @@ static int amd_spi_exec_mem_op(struct spi_mem *mem,
>>  
>>  	amd_spi = spi_controller_get_devdata(mem->spi->controller);
>>  
>> -	ret = amd_set_spi_freq(amd_spi, mem->spi->max_speed_hz);
>> +	ret = amd_set_spi_freq(amd_spi, op->max_freq);
>>  	if (ret)
>>  		return ret;
>
> however the return code is checked just on this call, and completely
> ignored in the 2 other calls. I find the code a bit ugly for the non
> SPIMEM case, but maybe something for the amd owner to address.

Once the above check removed (it does not make much sense there), the
function can return void.

Cheers,
Miquèl

[PATCH 04/24] spi: amlogic-spifc-a1: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-amlogic-spifc-a1.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
index fadf6667cd51..18c9aa2cbc29 100644
--- a/drivers/spi/spi-amlogic-spifc-a1.c
+++ b/drivers/spi/spi-amlogic-spifc-a1.c
@@ -259,7 +259,7 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
 	size_t data_size = op->data.nbytes;
 	int ret;
 
-	ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz);
+	ret = amlogic_spifc_a1_set_freq(spifc, op->max_freq);
 	if (ret)
 		return ret;
 
@@ -320,6 +320,10 @@ static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = {
 	.adjust_op_size = amlogic_spifc_a1_adjust_op_size,
 };
 
+static const struct spi_controller_mem_caps amlogic_spifc_a1_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int amlogic_spifc_a1_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctrl;
@@ -356,6 +360,7 @@ static int amlogic_spifc_a1_probe(struct platform_device *pdev)
 	ctrl->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctrl->auto_runtime_pm = true;
 	ctrl->mem_ops = &amlogic_spifc_a1_mem_ops;
+	ctrl->mem_caps = &amlogic_spifc_a1_mem_caps;
 	ctrl->min_speed_hz = SPIFC_A1_MIN_HZ;
 	ctrl->max_speed_hz = SPIFC_A1_MAX_HZ;
 	ctrl->mode_bits = (SPI_RX_DUAL | SPI_TX_DUAL |
-- 
2.43.0

Re: [PATCH 04/24] spi: amlogic-spifc-a1: Support per spi-mem operation frequency switches

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Every ->exec_op() call correctly configures the spi bus speed to the
> maximum allowed frequency for the memory using the constant spi default
> parameter. Since we can now have per-operation constraints, let's use
> the value that comes from the spi-mem operation structure instead. In
> case there is no specific limitation for this operation, the default spi
> device value will be given anyway.
> 
> The per-operation frequency capability is thus advertised to the spi-mem
> core.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-amlogic-spifc-a1.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
> index fadf6667cd51..18c9aa2cbc29 100644
> --- a/drivers/spi/spi-amlogic-spifc-a1.c
> +++ b/drivers/spi/spi-amlogic-spifc-a1.c
> @@ -259,7 +259,7 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>  	size_t data_size = op->data.nbytes;
>  	int ret;
>  
> -	ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz);
> +	ret = amlogic_spifc_a1_set_freq(spifc, op->max_freq);
>  	if (ret)
>  		return ret;
>  
> @@ -320,6 +320,10 @@ static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = {
>  	.adjust_op_size = amlogic_spifc_a1_adjust_op_size,
>  };

I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ;

Do you want to introduce a struct spi_controller_mem_ops.supports_op and
check that the spimem op freq is not below the controller's minimum freq?

Re: [PATCH 04/24] spi: amlogic-spifc-a1: Support per spi-mem operation frequency switches

[Miquel Raynal]

Hi Tudor,

On 11/11/2024 at 13:42:31 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>> Every ->exec_op() call correctly configures the spi bus speed to the
>> maximum allowed frequency for the memory using the constant spi default
>> parameter. Since we can now have per-operation constraints, let's use
>> the value that comes from the spi-mem operation structure instead. In
>> case there is no specific limitation for this operation, the default spi
>> device value will be given anyway.
>> 
>> The per-operation frequency capability is thus advertised to the spi-mem
>> core.
>
> I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ;
>
> Do you want to introduce a struct spi_controller_mem_ops.supports_op and
> check that the spimem op freq is not below the controller's minimum freq?

I was about to do that but I think we can do better. I am already tuning
the max frequency depending on the op. I can just check in the default
supports_op hook that the operation max frequency is not below the
controller's minimum. So all drivers with this kind of limitation will
be covered.

Thanks for the heads up!
Miquèl

Re: [PATCH 04/24] spi: amlogic-spifc-a1: Support per spi-mem operation frequency switches

[Tudor Ambarus]

On 12/13/24 11:44 AM, Miquel Raynal wrote:
> Hi Tudor,
> 
> On 11/11/2024 at 13:42:31 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>> Every ->exec_op() call correctly configures the spi bus speed to the
>>> maximum allowed frequency for the memory using the constant spi default
>>> parameter. Since we can now have per-operation constraints, let's use
>>> the value that comes from the spi-mem operation structure instead. In
>>> case there is no specific limitation for this operation, the default spi
>>> device value will be given anyway.
>>>
>>> The per-operation frequency capability is thus advertised to the spi-mem
>>> core.
>>
>> I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ;
>>
>> Do you want to introduce a struct spi_controller_mem_ops.supports_op and
>> check that the spimem op freq is not below the controller's minimum freq?
> 
> I was about to do that but I think we can do better. I am already tuning
> the max frequency depending on the op. I can just check in the default
> supports_op hook that the operation max frequency is not below the
> controller's minimum. So all drivers with this kind of limitation will
> be covered.
> 

Sounds good!

[PATCH 05/24] spi: cadence-qspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 05ebb03d319f..d285c7698291 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1407,7 +1407,7 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 	struct cqspi_flash_pdata *f_pdata;
 
 	f_pdata = &cqspi->f_pdata[spi_get_chipselect(mem->spi, 0)];
-	cqspi_configure(f_pdata, mem->spi->max_speed_hz);
+	cqspi_configure(f_pdata, op->max_freq);
 
 	if (op->data.dir == SPI_MEM_DATA_IN && op->data.buf.in) {
 	/*
@@ -1655,6 +1655,7 @@ static const struct spi_controller_mem_ops cqspi_mem_ops = {
 
 static const struct spi_controller_mem_caps cqspi_mem_caps = {
 	.dtr = true,
+	.per_op_freq = true,
 };
 
 static int cqspi_setup_flash(struct cqspi_st *cqspi)
-- 
2.43.0

Re: [PATCH 05/24] spi: cadence-qspi: Support per spi-mem operation frequency switches

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Every ->exec_op() call correctly configures the spi bus speed to the
> maximum allowed frequency for the memory using the constant spi default
> parameter. Since we can now have per-operation constraints, let's use
> the value that comes from the spi-mem operation structure instead. In
> case there is no specific limitation for this operation, the default spi
> device value will be given anyway.
> 
> The per-operation frequency capability is thus advertised to the spi-mem
> core.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

[PATCH 06/24] spi: dw: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-dw-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 431788dd848c..3d49b1dbaed4 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -677,7 +677,7 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	 * operation. Transmit-only mode is suitable for the rest of them.
 	 */
 	cfg.dfs = 8;
-	cfg.freq = clamp(mem->spi->max_speed_hz, 0U, dws->max_mem_freq);
+	cfg.freq = clamp(op->max_freq, 0U, dws->max_mem_freq);
 	if (op->data.dir == SPI_MEM_DATA_IN) {
 		cfg.tmode = DW_SPI_CTRLR0_TMOD_EPROMREAD;
 		cfg.ndf = op->data.nbytes;
@@ -894,6 +894,10 @@ static void dw_spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
 }
 
+static const struct spi_controller_mem_caps dw_spi_mem_caps = {
+	.per_op_freq = true,
+};
+
 int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 {
 	struct spi_controller *host;
@@ -941,8 +945,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 		host->set_cs = dw_spi_set_cs;
 	host->transfer_one = dw_spi_transfer_one;
 	host->handle_err = dw_spi_handle_err;
-	if (dws->mem_ops.exec_op)
+	if (dws->mem_ops.exec_op) {
 		host->mem_ops = &dws->mem_ops;
+		host->mem_caps = &dw_spi_mem_caps;
+	}
 	host->max_speed_hz = dws->max_freq;
 	host->flags = SPI_CONTROLLER_GPIO_SS;
 	host->auto_runtime_pm = true;
-- 
2.43.0

Re: [PATCH 06/24] spi: dw: Support per spi-mem operation frequency switches

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Every ->exec_op() call correctly configures the spi bus speed to the
> maximum allowed frequency for the memory using the constant spi default
> parameter. Since we can now have per-operation constraints, let's use
> the value that comes from the spi-mem operation structure instead. In
> case there is no specific limitation for this operation, the default spi
> device value will be given anyway.
> 
> The per-operation frequency capability is thus advertised to the spi-mem
> core.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-dw-core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 431788dd848c..3d49b1dbaed4 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -677,7 +677,7 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	 * operation. Transmit-only mode is suitable for the rest of them.
>  	 */
>  	cfg.dfs = 8;
> -	cfg.freq = clamp(mem->spi->max_speed_hz, 0U, dws->max_mem_freq);
> +	cfg.freq = clamp(op->max_freq, 0U, dws->max_mem_freq);
>  	if (op->data.dir == SPI_MEM_DATA_IN) {
>  		cfg.tmode = DW_SPI_CTRLR0_TMOD_EPROMREAD;
>  		cfg.ndf = op->data.nbytes;
> @@ -894,6 +894,10 @@ static void dw_spi_hw_init(struct device *dev, struct dw_spi *dws)
>  		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
>  }
>  
> +static const struct spi_controller_mem_caps dw_spi_mem_caps = {
> +	.per_op_freq = true,
> +};
> +
>  int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  {
>  	struct spi_controller *host;
> @@ -941,8 +945,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  		host->set_cs = dw_spi_set_cs;
>  	host->transfer_one = dw_spi_transfer_one;
>  	host->handle_err = dw_spi_handle_err;
> -	if (dws->mem_ops.exec_op)
> +	if (dws->mem_ops.exec_op) {
>  		host->mem_ops = &dws->mem_ops;
> +		host->mem_caps = &dw_spi_mem_caps;

I guess you need a dws->mem_caps, otherwise you overwrite whatever the
layer above specifies.

> +	}
>  	host->max_speed_hz = dws->max_freq;
>  	host->flags = SPI_CONTROLLER_GPIO_SS;
>  	host->auto_runtime_pm = true;

[PATCH 07/24] spi: fsl-qspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Han Xu <han.xu@nxp.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-fsl-qspi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index 79bac30e79af..ce86f44b0e93 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -522,9 +522,10 @@ static void fsl_qspi_invalidate(struct fsl_qspi *q)
 	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
 }
 
-static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi)
+static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi,
+				const struct spi_mem_op *op)
 {
-	unsigned long rate = spi->max_speed_hz;
+	unsigned long rate = op->max_freq;
 	int ret;
 
 	if (q->selected == spi_get_chipselect(spi, 0))
@@ -652,7 +653,7 @@ static int fsl_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	fsl_qspi_readl_poll_tout(q, base + QUADSPI_SR, (QUADSPI_SR_IP_ACC_MASK |
 				 QUADSPI_SR_AHB_ACC_MASK), 10, 1000);
 
-	fsl_qspi_select_mem(q, mem->spi);
+	fsl_qspi_select_mem(q, mem->spi, op);
 
 	if (needs_amba_base_offset(q))
 		addr_offset = q->memmap_phy;
@@ -839,6 +840,10 @@ static const struct spi_controller_mem_ops fsl_qspi_mem_ops = {
 	.get_name = fsl_qspi_get_name,
 };
 
+static const struct spi_controller_mem_caps fsl_qspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int fsl_qspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -923,6 +928,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = 4;
 	ctlr->mem_ops = &fsl_qspi_mem_ops;
+	ctlr->mem_caps = &fsl_qspi_mem_caps;
 
 	fsl_qspi_default_setup(q);
 
-- 
2.43.0

[PATCH 08/24] spi: microchip-core-qspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

This controller however performed a frequency check, which is also
observed during the ->check_op() phase.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Conor Dooley <conor.dooley@microchip.com>
Cc: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-microchip-core-qspi.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-qspi.c b/drivers/spi/spi-microchip-core-qspi.c
index 09f16471c537..eb5c388895cf 100644
--- a/drivers/spi/spi-microchip-core-qspi.c
+++ b/drivers/spi/spi-microchip-core-qspi.c
@@ -265,7 +265,8 @@ static irqreturn_t mchp_coreqspi_isr(int irq, void *dev_id)
 	return ret;
 }
 
-static int mchp_coreqspi_setup_clock(struct mchp_coreqspi *qspi, struct spi_device *spi)
+static int mchp_coreqspi_setup_clock(struct mchp_coreqspi *qspi, struct spi_device *spi,
+				     const struct spi_mem_op *op)
 {
 	unsigned long clk_hz;
 	u32 control, baud_rate_val = 0;
@@ -274,11 +275,11 @@ static int mchp_coreqspi_setup_clock(struct mchp_coreqspi *qspi, struct spi_devi
 	if (!clk_hz)
 		return -EINVAL;
 
-	baud_rate_val = DIV_ROUND_UP(clk_hz, 2 * spi->max_speed_hz);
+	baud_rate_val = DIV_ROUND_UP(clk_hz, 2 * op->max_freq);
 	if (baud_rate_val > MAX_DIVIDER || baud_rate_val < MIN_DIVIDER) {
 		dev_err(&spi->dev,
 			"could not configure the clock for spi clock %d Hz & system clock %ld Hz\n",
-			spi->max_speed_hz, clk_hz);
+			op->max_freq, clk_hz);
 		return -EINVAL;
 	}
 
@@ -399,7 +400,7 @@ static int mchp_coreqspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
 	if (err)
 		goto error;
 
-	err = mchp_coreqspi_setup_clock(qspi, mem->spi);
+	err = mchp_coreqspi_setup_clock(qspi, mem->spi, op);
 	if (err)
 		goto error;
 
@@ -457,6 +458,10 @@ static int mchp_coreqspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
 
 static bool mchp_coreqspi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
+	struct mchp_coreqspi *qspi = spi_controller_get_devdata(mem->spi->controller);
+	unsigned long clk_hz;
+	u32 baud_rate_val;
+
 	if (!spi_mem_default_supports_op(mem, op))
 		return false;
 
@@ -479,6 +484,14 @@ static bool mchp_coreqspi_supports_op(struct spi_mem *mem, const struct spi_mem_
 			return false;
 	}
 
+	clk_hz = clk_get_rate(qspi->clk);
+	if (!clk_hz)
+		return false;
+
+	baud_rate_val = DIV_ROUND_UP(clk_hz, 2 * op->max_freq);
+	if (baud_rate_val > MAX_DIVIDER || baud_rate_val < MIN_DIVIDER)
+		return false;
+
 	return true;
 }
 
@@ -498,6 +511,10 @@ static const struct spi_controller_mem_ops mchp_coreqspi_mem_ops = {
 	.exec_op = mchp_coreqspi_exec_op,
 };
 
+static const struct spi_controller_mem_caps mchp_coreqspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int mchp_coreqspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -540,6 +557,7 @@ static int mchp_coreqspi_probe(struct platform_device *pdev)
 
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctlr->mem_ops = &mchp_coreqspi_mem_ops;
+	ctlr->mem_caps = &mchp_coreqspi_mem_caps;
 	ctlr->setup = mchp_coreqspi_setup_op;
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
 			  SPI_TX_DUAL | SPI_TX_QUAD;
-- 
2.43.0

[PATCH 09/24] spi: mt65xx: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mt65xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 36c2f52cd6b8..8b49d1acd73c 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -952,7 +952,7 @@ static int mtk_spi_mem_exec_op(struct spi_mem *mem,
 
 	mtk_spi_reset(mdata);
 	mtk_spi_hw_init(mem->spi->controller, mem->spi);
-	mtk_spi_prepare_transfer(mem->spi->controller, mem->spi->max_speed_hz);
+	mtk_spi_prepare_transfer(mem->spi->controller, op->max_freq);
 
 	reg_val = readl(mdata->base + SPI_CFG3_IPM_REG);
 	/* opcode byte len */
@@ -1113,6 +1113,10 @@ static const struct spi_controller_mem_ops mtk_spi_mem_ops = {
 	.exec_op = mtk_spi_mem_exec_op,
 };
 
+static const struct spi_controller_mem_caps mtk_spi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int mtk_spi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1151,6 +1155,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
 	if (mdata->dev_comp->ipm_design) {
 		mdata->dev = dev;
 		host->mem_ops = &mtk_spi_mem_ops;
+		host->mem_caps = &mtk_spi_mem_caps;
 		init_completion(&mdata->spimem_done);
 	}
 
-- 
2.43.0

[PATCH 10/24] spi: mxic: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mxic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 6156d691630a..49a5833ca744 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -514,7 +514,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	int i, ret;
 	u8 addr[8], cmd[2];
 
-	ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
+	ret = mxic_spi_set_freq(mxic, op->max_freq);
 	if (ret)
 		return ret;
 
@@ -573,6 +573,7 @@ static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
 static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
 	.dtr = true,
 	.ecc = true,
+	.per_op_freq = true,
 };
 
 static void mxic_spi_set_cs(struct spi_device *spi, bool lvl)
-- 
2.43.0

[PATCH 11/24] spi: nxp-fspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-nxp-fspi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 88397f712a3b..c351daa9d934 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -697,9 +697,10 @@ static void nxp_fspi_dll_calibration(struct nxp_fspi *f)
  * Value for rest of the CS FLSHxxCR0 register would be zero.
  *
  */
-static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi)
+static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi,
+				const struct spi_mem_op *op)
 {
-	unsigned long rate = spi->max_speed_hz;
+	unsigned long rate = op->max_freq;
 	int ret;
 	uint64_t size_kb;
 
@@ -922,7 +923,7 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 				   FSPI_STS0_ARB_IDLE, 1, POLL_TOUT, true);
 	WARN_ON(err);
 
-	nxp_fspi_select_mem(f, mem->spi);
+	nxp_fspi_select_mem(f, mem->spi, op);
 
 	nxp_fspi_prepare_lut(f, op);
 	/*
@@ -1134,6 +1135,10 @@ static const struct spi_controller_mem_ops nxp_fspi_mem_ops = {
 	.get_name = nxp_fspi_get_name,
 };
 
+static const struct spi_controller_mem_caps nxp_fspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int nxp_fspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -1231,6 +1236,7 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
 	ctlr->mem_ops = &nxp_fspi_mem_ops;
+	ctlr->mem_caps = &nxp_fspi_mem_caps;
 
 	nxp_fspi_default_setup(f);
 
-- 
2.43.0

[PATCH 12/24] spi: rockchip-sfc: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Han Xu <han.xu@nxp.com>
Cc: Haibo Chen <haibo.chen@nxp.com>
Cc: Yogesh Gaur <yogeshgaur.83@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-rockchip-sfc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 0d7fadcd4ed3..1e0257b85f33 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -491,11 +491,11 @@ static int rockchip_sfc_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op
 	u32 len = op->data.nbytes;
 	int ret;
 
-	if (unlikely(mem->spi->max_speed_hz != sfc->frequency)) {
-		ret = clk_set_rate(sfc->clk, mem->spi->max_speed_hz);
+	if (unlikely(op->max_freq != sfc->frequency)) {
+		ret = clk_set_rate(sfc->clk, op->max_freq);
 		if (ret)
 			return ret;
-		sfc->frequency = mem->spi->max_speed_hz;
+		sfc->frequency = op->max_freq;
 		dev_dbg(sfc->dev, "set_freq=%dHz real_freq=%ldHz\n",
 			sfc->frequency, clk_get_rate(sfc->clk));
 	}
@@ -535,6 +535,10 @@ static const struct spi_controller_mem_ops rockchip_sfc_mem_ops = {
 	.adjust_op_size = rockchip_sfc_adjust_op_size,
 };
 
+static const struct spi_controller_mem_caps rockchip_sfc_mem_caps = {
+	.per_op_freq = true,
+};
+
 static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
 {
 	struct rockchip_sfc *sfc = dev_id;
@@ -567,6 +571,7 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 
 	host->flags = SPI_CONTROLLER_HALF_DUPLEX;
 	host->mem_ops = &rockchip_sfc_mem_ops;
+	host->mem_caps = &rockchip_sfc_mem_caps;
 	host->dev.of_node = pdev->dev.of_node;
 	host->mode_bits = SPI_TX_QUAD | SPI_TX_DUAL | SPI_RX_QUAD | SPI_RX_DUAL;
 	host->max_speed_hz = SFC_MAX_SPEED;
-- 
2.43.0

[PATCH 13/24] spi: spi-sn-f-ospi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-sn-f-ospi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-sn-f-ospi.c b/drivers/spi/spi-sn-f-ospi.c
index a7c3b3923b4a..6a999a103208 100644
--- a/drivers/spi/spi-sn-f-ospi.c
+++ b/drivers/spi/spi-sn-f-ospi.c
@@ -335,7 +335,6 @@ static void f_ospi_config_indir_protocol(struct f_ospi *ospi,
 static int f_ospi_indir_prepare_op(struct f_ospi *ospi, struct spi_mem *mem,
 				   const struct spi_mem_op *op)
 {
-	struct spi_device *spi = mem->spi;
 	u32 irq_stat_en;
 	int ret;
 
@@ -343,7 +342,7 @@ static int f_ospi_indir_prepare_op(struct f_ospi *ospi, struct spi_mem *mem,
 	if (ret)
 		return ret;
 
-	f_ospi_config_clk(ospi, spi->max_speed_hz);
+	f_ospi_config_clk(ospi, op->max_freq);
 
 	f_ospi_config_indir_protocol(ospi, mem, op);
 
@@ -577,6 +576,10 @@ static const struct spi_controller_mem_ops f_ospi_mem_ops = {
 	.exec_op = f_ospi_exec_op,
 };
 
+static const struct spi_controller_mem_caps f_ospi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int f_ospi_init(struct f_ospi *ospi)
 {
 	int ret;
@@ -614,6 +617,7 @@ static int f_ospi_probe(struct platform_device *pdev)
 		| SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL
 		| SPI_MODE_0 | SPI_MODE_1 | SPI_LSB_FIRST;
 	ctlr->mem_ops = &f_ospi_mem_ops;
+	ctlr->mem_caps = &f_ospi_mem_caps;
 	ctlr->bus_num = -1;
 	of_property_read_u32(dev->of_node, "num-cs", &num_cs);
 	if (num_cs > OSPI_NUM_CS) {
-- 
2.43.0

[PATCH 14/24] spi: spi-ti-qspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-ti-qspi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 0fe6899e78dd..36a3a650160d 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -623,7 +623,7 @@ static int ti_qspi_exec_mem_op(struct spi_mem *mem,
 	mutex_lock(&qspi->list_lock);
 
 	if (!qspi->mmap_enabled || qspi->current_cs != spi_get_chipselect(mem->spi, 0)) {
-		ti_qspi_setup_clk(qspi, mem->spi->max_speed_hz);
+		ti_qspi_setup_clk(qspi, op->max_freq);
 		ti_qspi_enable_memory_map(mem->spi);
 	}
 	ti_qspi_setup_mmap_read(mem->spi, op->cmd.opcode, op->data.buswidth,
@@ -658,6 +658,10 @@ static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
 	.adjust_op_size = ti_qspi_adjust_op_size,
 };
 
+static const struct spi_controller_mem_caps ti_qspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 static int ti_qspi_start_transfer_one(struct spi_controller *host,
 		struct spi_message *m)
 {
@@ -777,6 +781,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
 	host->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
 				   SPI_BPW_MASK(8);
 	host->mem_ops = &ti_qspi_mem_ops;
+	host->mem_caps = &ti_qspi_mem_caps;
 
 	if (!of_property_read_u32(np, "num-cs", &num_cs))
 		host->num_chipselect = num_cs;
-- 
2.43.0

[PATCH 15/24] spi: zynq-qspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-zynq-qspi.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-zynq-qspi.c b/drivers/spi/spi-zynq-qspi.c
index d6325c6be3d4..ee9555b46d06 100644
--- a/drivers/spi/spi-zynq-qspi.c
+++ b/drivers/spi/spi-zynq-qspi.c
@@ -318,6 +318,7 @@ static void zynq_qspi_chipselect(struct spi_device *spi, bool assert)
  * zynq_qspi_config_op - Configure QSPI controller for specified transfer
  * @xqspi:	Pointer to the zynq_qspi structure
  * @spi:	Pointer to the spi_device structure
+ * @op:		The memory operation to execute
  *
  * Sets the operational mode of QSPI controller for the next QSPI transfer and
  * sets the requested clock frequency.
@@ -331,7 +332,8 @@ static void zynq_qspi_chipselect(struct spi_device *spi, bool assert)
  * controller the driver will set the highest or lowest frequency supported by
  * controller.
  */
-static int zynq_qspi_config_op(struct zynq_qspi *xqspi, struct spi_device *spi)
+static int zynq_qspi_config_op(struct zynq_qspi *xqspi, struct spi_device *spi,
+			       const struct spi_mem_op *op)
 {
 	u32 config_reg, baud_rate_val = 0;
 
@@ -346,7 +348,7 @@ static int zynq_qspi_config_op(struct zynq_qspi *xqspi, struct spi_device *spi)
 	 */
 	while ((baud_rate_val < ZYNQ_QSPI_CONFIG_BAUD_DIV_MAX)  &&
 	       (clk_get_rate(xqspi->refclk) / (2 << baud_rate_val)) >
-		spi->max_speed_hz)
+		op->max_freq)
 		baud_rate_val++;
 
 	config_reg = zynq_qspi_read(xqspi, ZYNQ_QSPI_CONFIG_OFFSET);
@@ -534,7 +536,7 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem,
 		op->dummy.buswidth, op->data.buswidth);
 
 	zynq_qspi_chipselect(mem->spi, true);
-	zynq_qspi_config_op(xqspi, mem->spi);
+	zynq_qspi_config_op(xqspi, mem->spi, op);
 
 	if (op->cmd.opcode) {
 		reinit_completion(&xqspi->data_completion);
@@ -620,6 +622,10 @@ static const struct spi_controller_mem_ops zynq_qspi_mem_ops = {
 	.exec_op = zynq_qspi_exec_mem_op,
 };
 
+static const struct spi_controller_mem_caps zynq_qspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 /**
  * zynq_qspi_probe - Probe method for the QSPI driver
  * @pdev:	Pointer to the platform_device structure
@@ -706,6 +712,7 @@ static int zynq_qspi_probe(struct platform_device *pdev)
 	ctlr->mode_bits =  SPI_RX_DUAL | SPI_RX_QUAD |
 			    SPI_TX_DUAL | SPI_TX_QUAD;
 	ctlr->mem_ops = &zynq_qspi_mem_ops;
+	ctlr->mem_caps = &zynq_qspi_mem_caps;
 	ctlr->setup = zynq_qspi_setup_op;
 	ctlr->max_speed_hz = clk_get_rate(xqspi->refclk) / 2;
 	ctlr->dev.of_node = np;
-- 
2.43.0

[PATCH 16/24] spi: zynqmp-gqspi: Support per spi-mem operation frequency switches

[Miquel Raynal]

Every ->exec_op() call correctly configures the spi bus speed to the
maximum allowed frequency for the memory using the constant spi default
parameter. Since we can now have per-operation constraints, let's use
the value that comes from the spi-mem operation structure instead. In
case there is no specific limitation for this operation, the default spi
device value will be given anyway.

The per-operation frequency capability is thus advertised to the spi-mem
core.

Cc: Michal Simek <michal.simek@amd.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 99524a3c9f38..96542da6e237 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -535,7 +535,7 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
  * zynqmp_qspi_config_op - Configure QSPI controller for specified
  *				transfer
  * @xqspi:	Pointer to the zynqmp_qspi structure
- * @qspi:	Pointer to the spi_device structure
+ * @op:		The memory operation to execute
  *
  * Sets the operational mode of QSPI controller for the next QSPI transfer and
  * sets the requested clock frequency.
@@ -553,12 +553,12 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
  *	frequency supported by controller.
  */
 static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi,
-				 struct spi_device *qspi)
+				 const struct spi_mem_op *op)
 {
 	ulong clk_rate;
 	u32 config_reg, req_speed_hz, baud_rate_val = 0;
 
-	req_speed_hz = qspi->max_speed_hz;
+	req_speed_hz = op->max_freq;
 
 	if (xqspi->speed_hz != req_speed_hz) {
 		xqspi->speed_hz = req_speed_hz;
@@ -1059,7 +1059,7 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 		op->dummy.buswidth, op->data.buswidth);
 
 	mutex_lock(&xqspi->op_lock);
-	zynqmp_qspi_config_op(xqspi, mem->spi);
+	zynqmp_qspi_config_op(xqspi, op);
 	zynqmp_qspi_chipselect(mem->spi, false);
 	genfifoentry |= xqspi->genfifocs;
 	genfifoentry |= xqspi->genfifobus;
@@ -1206,6 +1206,10 @@ static const struct spi_controller_mem_ops zynqmp_qspi_mem_ops = {
 	.exec_op = zynqmp_qspi_exec_op,
 };
 
+static const struct spi_controller_mem_caps zynqmp_qspi_mem_caps = {
+	.per_op_freq = true,
+};
+
 /**
  * zynqmp_qspi_probe - Probe method for the QSPI driver
  * @pdev:	Pointer to the platform_device structure
@@ -1323,6 +1327,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctlr->mem_ops = &zynqmp_qspi_mem_ops;
+	ctlr->mem_caps = &zynqmp_qspi_mem_caps;
 	ctlr->setup = zynqmp_qspi_setup_op;
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctlr->dev.of_node = np;
-- 
2.43.0

[PATCH 17/24] mtd: spinand: Create distinct fast and slow read from cache variants

[Miquel Raynal]

So far, the SPINAND_PAGE_READ_FROM_CACHE_OP macro was taking a first
argument, "fast", which was inducing the possibility to support higher
bus frequencies than with the normal (slower) read from cache
alternative. In practice, without frequency change on the bus, this was
likely without effect, besides perhaps allowing another variant of the
same command, that could run at the default highest speed. If we want to
support this fully, we need to add a frequency parameter to the slowest
command. But before we do that, let's drop the "fast" boolean from the
macro and duplicate it, this will further help supporting having
different frequencies allowed for each variant.

The change is also of course propagated to all users. It has the nice
effect to have all macros aligned on the same pattern.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/alliancememory.c |  4 ++--
 drivers/mtd/nand/spi/ato.c            |  4 ++--
 drivers/mtd/nand/spi/esmt.c           |  4 ++--
 drivers/mtd/nand/spi/foresee.c        |  4 ++--
 drivers/mtd/nand/spi/gigadevice.c     | 16 ++++++++--------
 drivers/mtd/nand/spi/macronix.c       |  4 ++--
 drivers/mtd/nand/spi/micron.c         |  8 ++++----
 drivers/mtd/nand/spi/paragon.c        |  4 ++--
 drivers/mtd/nand/spi/toshiba.c        |  4 ++--
 drivers/mtd/nand/spi/winbond.c        |  4 ++--
 drivers/mtd/nand/spi/xtx.c            |  4 ++--
 include/linux/mtd/spinand.h           | 20 ++++++++++++++++----
 12 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/drivers/mtd/nand/spi/alliancememory.c b/drivers/mtd/nand/spi/alliancememory.c
index 7936ea546b03..6046c73f8424 100644
--- a/drivers/mtd/nand/spi/alliancememory.c
+++ b/drivers/mtd/nand/spi/alliancememory.c
@@ -21,8 +21,8 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 			   SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/ato.c b/drivers/mtd/nand/spi/ato.c
index 82b377c06812..bb5298911137 100644
--- a/drivers/mtd/nand/spi/ato.c
+++ b/drivers/mtd/nand/spi/ato.c
@@ -15,8 +15,8 @@
 
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/esmt.c b/drivers/mtd/nand/spi/esmt.c
index 4597a82de23a..323a20901fc9 100644
--- a/drivers/mtd/nand/spi/esmt.c
+++ b/drivers/mtd/nand/spi/esmt.c
@@ -15,8 +15,8 @@
 static SPINAND_OP_VARIANTS(read_cache_variants,
 			   SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 			   SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-			   SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-			   SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+			   SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+			   SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 			   SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/foresee.c b/drivers/mtd/nand/spi/foresee.c
index e0d2d9257045..cb5c9e0517ea 100644
--- a/drivers/mtd/nand/spi/foresee.c
+++ b/drivers/mtd/nand/spi/foresee.c
@@ -14,8 +14,8 @@
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
index 6023cba748bb..d620bb02a20a 100644
--- a/drivers/mtd/nand/spi/gigadevice.c
+++ b/drivers/mtd/nand/spi/gigadevice.c
@@ -28,32 +28,32 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(read_cache_variants_f,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP_3A(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP_3A(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP_3A(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP_3A(false, 0, 0, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP_3A(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP_3A(0, 0, NULL, 0));
 
 static SPINAND_OP_VARIANTS(read_cache_variants_1gq5,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(read_cache_variants_2gq5,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 4, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
index 3f9e9c572854..53e3ec38ef9f 100644
--- a/drivers/mtd/nand/spi/macronix.c
+++ b/drivers/mtd/nand/spi/macronix.c
@@ -15,8 +15,8 @@
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 12601bc4227a..ad0bb9755a09 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -33,8 +33,8 @@ static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(x4_write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
@@ -48,8 +48,8 @@ static SPINAND_OP_VARIANTS(x4_update_cache_variants,
 static SPINAND_OP_VARIANTS(x4_read_cache_variants,
 			   SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 			   SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-			   SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-			   SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+			   SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+			   SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(x1_write_cache_variants,
 			   SPINAND_PROG_LOAD(true, 0, NULL, 0));
diff --git a/drivers/mtd/nand/spi/paragon.c b/drivers/mtd/nand/spi/paragon.c
index 519ade513c1f..6e7cc6995380 100644
--- a/drivers/mtd/nand/spi/paragon.c
+++ b/drivers/mtd/nand/spi/paragon.c
@@ -26,8 +26,8 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c
index bbbcaa87c0bc..2e2106b2705f 100644
--- a/drivers/mtd/nand/spi/toshiba.c
+++ b/drivers/mtd/nand/spi/toshiba.c
@@ -17,8 +17,8 @@
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_x4_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index e472d0f692c2..329377bf3717 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -22,8 +22,8 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/drivers/mtd/nand/spi/xtx.c b/drivers/mtd/nand/spi/xtx.c
index 66a4255bdf06..3f539ca0de86 100644
--- a/drivers/mtd/nand/spi/xtx.c
+++ b/drivers/mtd/nand/spi/xtx.c
@@ -27,8 +27,8 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
-		SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0));
 
 static SPINAND_OP_VARIANTS(write_cache_variants,
 		SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 5c19ead60499..3730cdf914f8 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -62,14 +62,26 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_NO_DATA)
 
-#define SPINAND_PAGE_READ_FROM_CACHE_OP(fast, addr, ndummy, buf, len)	\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(fast ? 0x0b : 0x03, 1),		\
+#define SPINAND_PAGE_READ_FROM_CACHE_OP(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x03, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 1),				\
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 1))
 
-#define SPINAND_PAGE_READ_FROM_CACHE_OP_3A(fast, addr, ndummy, buf, len) \
-	SPI_MEM_OP(SPI_MEM_OP_CMD(fast ? 0x0b : 0x03, 1),		\
+#define SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0b, 1),			\
+			 SPI_MEM_OP_ADDR(2, addr, 1),			\
+			 SPI_MEM_OP_DUMMY(ndummy, 1),			\
+			 SPI_MEM_OP_DATA_IN(len, buf, 1))
+
+#define SPINAND_PAGE_READ_FROM_CACHE_OP_3A(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x03, 1),				\
+		   SPI_MEM_OP_ADDR(3, addr, 1),				\
+		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
+		   SPI_MEM_OP_DATA_IN(len, buf, 1))
+
+#define SPINAND_PAGE_READ_FROM_CACHE_FAST_OP_3A(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0b, 1),				\
 		   SPI_MEM_OP_ADDR(3, addr, 1),				\
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 1))
-- 
2.43.0

Re: [PATCH 17/24] mtd: spinand: Create distinct fast and slow read from cache variants

[Tudor Ambarus]

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

[PATCH 18/24] mtd: spinand: Add an optional frequency to read from cache macros

[Miquel Raynal]

While the SPINAND_PAGE_READ_FROM_CACHE_FAST_OP macro is supposed to be
able to run at the highest supported frequency, it is not the case of
the regular read from cache, which may be limited in terms of maximum
frequency. Add an optional argument to this macro, which will be used to
set the maximum frequency, if any.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mtd/spinand.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 3730cdf914f8..6064029c5e05 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -62,11 +62,12 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_NO_DATA)
 
-#define SPINAND_PAGE_READ_FROM_CACHE_OP(addr, ndummy, buf, len) \
+#define SPINAND_PAGE_READ_FROM_CACHE_OP(addr, ndummy, buf, len, ...) \
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x03, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 1),				\
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
-		   SPI_MEM_OP_DATA_IN(len, buf, 1))
+		   SPI_MEM_OP_DATA_IN(len, buf, 1),			\
+		   __VA_OPT__(SPI_MEM_OP_MAX_FREQ(__VA_ARGS__)))
 
 #define SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(addr, ndummy, buf, len) \
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0b, 1),			\
-- 
2.43.0

Re: [PATCH 18/24] mtd: spinand: Add an optional frequency to read from cache macros

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> While the SPINAND_PAGE_READ_FROM_CACHE_FAST_OP macro is supposed to be
> able to run at the highest supported frequency, it is not the case of

what do you mean by highest supported frequency? Is it the max freq
between the ones supported by the controller, pcb and flash?

> the regular read from cache, which may be limited in terms of maximum
> frequency. Add an optional argument to this macro, which will be used to
> set the maximum frequency, if any.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/mtd/spinand.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 3730cdf914f8..6064029c5e05 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -62,11 +62,12 @@
>  		   SPI_MEM_OP_NO_DUMMY,					\
>  		   SPI_MEM_OP_NO_DATA)
>  
> -#define SPINAND_PAGE_READ_FROM_CACHE_OP(addr, ndummy, buf, len) \
> +#define SPINAND_PAGE_READ_FROM_CACHE_OP(addr, ndummy, buf, len, ...) \
>  	SPI_MEM_OP(SPI_MEM_OP_CMD(0x03, 1),				\
>  		   SPI_MEM_OP_ADDR(2, addr, 1),				\
>  		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
> -		   SPI_MEM_OP_DATA_IN(len, buf, 1))
> +		   SPI_MEM_OP_DATA_IN(len, buf, 1),			\
> +		   __VA_OPT__(SPI_MEM_OP_MAX_FREQ(__VA_ARGS__)))
>  
>  #define SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(addr, ndummy, buf, len) \
>  	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0b, 1),			\

Re: [PATCH 18/24] mtd: spinand: Add an optional frequency to read from cache macros

[Miquel Raynal]

On 11/11/2024 at 14:17:36 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>> While the SPINAND_PAGE_READ_FROM_CACHE_FAST_OP macro is supposed to be
>> able to run at the highest supported frequency, it is not the case of
>
> what do you mean by highest supported frequency? Is it the max freq
> between the ones supported by the controller, pcb and flash?

I am really talking about flash limitations here, I will clarify.

Miquèl

[PATCH 19/24] mtd: spinand: winbond: Fix the *JW chip definitions

[Miquel Raynal]

W25N01JW and W25N02JW use a different technology with higher frequencies
supported (up to 166MHz). There is one drawback though, the slowest
READ_FROM_CACHE command cannot run above 54MHz. Because of that, we need
to set a limit for these chips on the basic READ_FROM_CACHE variant.

Duplicating this list is not a problem because these chips have DTR
support, and the list of supported variants will diverge from all the
other chips when adding support for it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/winbond.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 329377bf3717..686e872fe0ff 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/mtd/spinand.h>
+#include <linux/units.h>
 
 #define SPINAND_MFR_WINBOND		0xEF
 
@@ -17,6 +18,14 @@
 
 #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
 
+static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
+		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0, 54 * HZ_PER_MHZ));
+
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
@@ -177,7 +186,7 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xbc, 0x21),
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
 		     NAND_ECCREQ(1, 512),
-		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_dtr_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
@@ -197,7 +206,7 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xbf, 0x22),
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 2, 1),
 		     NAND_ECCREQ(1, 512),
-		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_dtr_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-- 
2.43.0

Re: [PATCH 19/24] mtd: spinand: winbond: Fix the *JW chip definitions

[Tudor Ambarus]

do you want the linux stable teams scripts to queue this patch to stable?

if not, maybe update the commit subject with s/Fix/update? Or use
Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present

Re: [PATCH 19/24] mtd: spinand: winbond: Fix the *JW chip definitions

[Tudor Ambarus]

On 11/11/24 2:27 PM, Tudor Ambarus wrote:
> do you want the linux stable teams scripts to queue this patch to stable?
> 
> if not, maybe update the commit subject with s/Fix/update? Or use
> Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present

If this is a fix, would be good to separate it from the series and send
it as an individual patch with stable tag.

Re: [PATCH 19/24] mtd: spinand: winbond: Fix the *JW chip definitions

[Miquel Raynal]

On 18/12/2024 at 08:16:39 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 11/11/24 2:27 PM, Tudor Ambarus wrote:
>> do you want the linux stable teams scripts to queue this patch to stable?
>> 
>> if not, maybe update the commit subject with s/Fix/update? Or use
>> Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present
>
> If this is a fix, would be good to separate it from the series and send
> it as an individual patch with stable tag.

I actually ignored the "noautosel" thing :) It looks like I didn't
answer, but I decided it was not worth backporting it and used your
trick.

Thanks!
Miquèl

[PATCH 20/24] spi: spi-mem: Reorder SPI_MEM_OP_CMD internals

[Miquel Raynal]

Follow the order used by all the other similar macros:
- nbytes
- value/buffer
- buswidth
- other fields

There is no functional change.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/spi/spi-mem.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 379c048b2eb4..318ea7b193cc 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -15,9 +15,9 @@
 
 #define SPI_MEM_OP_CMD(__opcode, __buswidth)			\
 	{							\
-		.buswidth = __buswidth,				\
-		.opcode = __opcode,				\
 		.nbytes = 1,					\
+		.opcode = __opcode,				\
+		.buswidth = __buswidth,				\
 	}
 
 #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)		\
-- 
2.43.0

Re: [PATCH 20/24] spi: spi-mem: Reorder SPI_MEM_OP_CMD internals

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Follow the order used by all the other similar macros:
> - nbytes
> - value/buffer
> - buswidth
> - other fields
> 
> There is no functional change.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/spi/spi-mem.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 379c048b2eb4..318ea7b193cc 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -15,9 +15,9 @@
>  
>  #define SPI_MEM_OP_CMD(__opcode, __buswidth)			\
>  	{							\
> -		.buswidth = __buswidth,				\
> -		.opcode = __opcode,				\
>  		.nbytes = 1,					\
> +		.opcode = __opcode,				\
> +		.buswidth = __buswidth,				\
>  	}
>  

I don't mind, but shouldn't we follow the order used at struct declaration?

struct spi_mem_op {
	struct {
		u8 nbytes;
		u8 buswidth;
		u8 dtr : 1;
		u8 __pad : 7;
		u16 opcode;
	} cmd;

Re: [PATCH 20/24] spi: spi-mem: Reorder SPI_MEM_OP_CMD internals

[Miquel Raynal]

>>  #define SPI_MEM_OP_CMD(__opcode, __buswidth)			\
>>  	{							\
>> -		.buswidth = __buswidth,				\
>> -		.opcode = __opcode,				\
>>  		.nbytes = 1,					\
>> +		.opcode = __opcode,				\
>> +		.buswidth = __buswidth,				\
>>  	}
>>  
>
> I don't mind, but shouldn't we follow the order used at struct declaration?
>
> struct spi_mem_op {
> 	struct {
> 		u8 nbytes;
> 		u8 buswidth;
> 		u8 dtr : 1;
> 		u8 __pad : 7;
> 		u16 opcode;
> 	} cmd;

Sure, let's do that. I also corrected the other macros at the same time (v2).

[PATCH 21/24] spi: spi-mem: Create macros for DTR operation

[Miquel Raynal]

We do have macros for defining command, address, dummy and data
cycles. We also have a .dtr flag that implies sampling the bus on both
edges, but there are currently no macros enabling it. We might make use
of such macros, so let's create:
- SPI_MEM_DTR_OP_CMD
- SPI_MEM_DTR_OP_ADDR
- SPI_MEM_DTR_OP_DUMMY
- SPI_MEM_DTR_OP_DATA_OUT
- SPI_MEM_DTR_OP_DATA_OUT

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/spi/spi-mem.h | 41 +++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 318ea7b193cc..d332ac5ce971 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -20,6 +20,14 @@
 		.buswidth = __buswidth,				\
 	}
 
+#define SPI_MEM_DTR_OP_CMD(__opcode, __buswidth)		\
+	{							\
+		.nbytes = 1,					\
+		.opcode = __opcode,				\
+		.buswidth = __buswidth,				\
+		.dtr = true,					\
+	}
+
 #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)		\
 	{							\
 		.nbytes = __nbytes,				\
@@ -27,6 +35,14 @@
 		.buswidth = __buswidth,				\
 	}
 
+#define SPI_MEM_DTR_OP_ADDR(__nbytes, __val, __buswidth)	\
+	{							\
+		.nbytes = __nbytes,				\
+		.val = __val,					\
+		.buswidth = __buswidth,				\
+		.dtr = true,					\
+	}
+
 #define SPI_MEM_OP_NO_ADDR	{ }
 
 #define SPI_MEM_OP_DUMMY(__nbytes, __buswidth)			\
@@ -35,6 +51,13 @@
 		.buswidth = __buswidth,				\
 	}
 
+#define SPI_MEM_DTR_OP_DUMMY(__nbytes, __buswidth)		\
+	{							\
+		.nbytes = __nbytes,				\
+		.buswidth = __buswidth,				\
+		.dtr = true,					\
+	}
+
 #define SPI_MEM_OP_NO_DUMMY	{ }
 
 #define SPI_MEM_OP_DATA_IN(__nbytes, __buf, __buswidth)		\
@@ -45,6 +68,15 @@
 		.buswidth = __buswidth,				\
 	}
 
+#define SPI_MEM_DTR_OP_DATA_IN(__nbytes, __buf, __buswidth)	\
+	{							\
+		.dir = SPI_MEM_DATA_IN,				\
+		.nbytes = __nbytes,				\
+		.buf.in = __buf,				\
+		.buswidth = __buswidth,				\
+		.dtr = true,					\
+	}
+
 #define SPI_MEM_OP_DATA_OUT(__nbytes, __buf, __buswidth)	\
 	{							\
 		.dir = SPI_MEM_DATA_OUT,			\
@@ -53,6 +85,15 @@
 		.buswidth = __buswidth,				\
 	}
 
+#define SPI_MEM_DTR_OP_DATA_OUT(__nbytes, __buf, __buswidth)	\
+	{							\
+		.dir = SPI_MEM_DATA_OUT,			\
+		.nbytes = __nbytes,				\
+		.buf.out = __buf,				\
+		.buswidth = __buswidth,				\
+		.dtr = true,					\
+	}
+
 #define SPI_MEM_OP_NO_DATA	{ }
 
 /**
-- 
2.43.0

[PATCH 22/24] mtd: spinand: Add support for read DTR operations

[Miquel Raynal]

Advanced SPI-NAND chips are capable of reading data much faster by
leveraging DTR support. This support extends to dual and quad
configurations.

Create macros defining all possible read from cache DTR variants:
- SPINAND_PAGE_READ_FROM_CACHE_DTR_OP
- SPINAND_PAGE_READ_FROM_CACHE_X2_DTR_OP
- SPINAND_PAGE_READ_FROM_CACHE_X4_DTR_OP
- SPINAND_PAGE_READ_FROM_CACHE_DUALIO_DTR_OP
- SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mtd/spinand.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6064029c5e05..72e9af266494 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -87,6 +87,13 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 1))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_DTR_OP(addr, ndummy, buf, len, freq) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0d, 1),				\
+		   SPI_MEM_DTR_OP_ADDR(2, addr, 1),			\
+		   SPI_MEM_DTR_OP_DUMMY(ndummy, 1),			\
+		   SPI_MEM_DTR_OP_DATA_IN(len, buf, 1),			\
+		   SPI_MEM_OP_MAX_FREQ(freq))
+
 #define SPINAND_PAGE_READ_FROM_CACHE_X2_OP(addr, ndummy, buf, len)	\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x3b, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 1),				\
@@ -99,6 +106,13 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 2))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_X2_DTR_OP(addr, ndummy, buf, len, freq) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x3d, 1),				\
+		   SPI_MEM_DTR_OP_ADDR(2, addr, 1),			\
+		   SPI_MEM_DTR_OP_DUMMY(ndummy, 1),			\
+		   SPI_MEM_DTR_OP_DATA_IN(len, buf, 2),			\
+		   SPI_MEM_OP_MAX_FREQ(freq))
+
 #define SPINAND_PAGE_READ_FROM_CACHE_X4_OP(addr, ndummy, buf, len)	\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x6b, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 1),				\
@@ -111,6 +125,13 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 4))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_X4_DTR_OP(addr, ndummy, buf, len, freq) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0x6d, 1),				\
+		   SPI_MEM_DTR_OP_ADDR(2, addr, 1),			\
+		   SPI_MEM_DTR_OP_DUMMY(ndummy, 1),			\
+		   SPI_MEM_DTR_OP_DATA_IN(len, buf, 4),			\
+		   SPI_MEM_OP_MAX_FREQ(freq))
+
 #define SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(addr, ndummy, buf, len)	\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0xbb, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 2),				\
@@ -123,6 +144,13 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 2),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 2))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_DUALIO_DTR_OP(addr, ndummy, buf, len, freq) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0xbd, 1),				\
+		   SPI_MEM_DTR_OP_ADDR(2, addr, 2),			\
+		   SPI_MEM_DTR_OP_DUMMY(ndummy, 2),			\
+		   SPI_MEM_DTR_OP_DATA_IN(len, buf, 2),			\
+		   SPI_MEM_OP_MAX_FREQ(freq))
+
 #define SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(addr, ndummy, buf, len)	\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0xeb, 1),				\
 		   SPI_MEM_OP_ADDR(2, addr, 4),				\
@@ -135,6 +163,13 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 4),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 4))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP(addr, ndummy, buf, len, freq) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD(0xed, 1),				\
+		   SPI_MEM_DTR_OP_ADDR(2, addr, 4),			\
+		   SPI_MEM_DTR_OP_DUMMY(ndummy, 4),			\
+		   SPI_MEM_DTR_OP_DATA_IN(len, buf, 4),			\
+		   SPI_MEM_OP_MAX_FREQ(freq))
+
 #define SPINAND_PROG_EXEC_OP(addr)					\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1),				\
 		   SPI_MEM_OP_ADDR(3, addr, 1),				\
-- 
2.43.0

Re: [PATCH 22/24] mtd: spinand: Add support for read DTR operations

[Tudor Ambarus]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> +	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0d, 1),	

do we want some names to these hex values?

Re: [PATCH 22/24] mtd: spinand: Add support for read DTR operations

[Miquel Raynal]

Hi Tudor,

On 11/11/2024 at 14:35:23 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>> +	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0d, 1),	
>
> do we want some names to these hex values?

I honestly don't think we do because it would be totally redundant with
the macro name, ie.

+#define SPINAND_PAGE_READ_FROM_CACHE_DTR_OP(addr, ndummy, buf, len, freq) \
+       SPI_MEM_OP(SPI_MEM_OP_CMD(0x0d, 1),                             \
+                  SPI_MEM_DTR_OP_ADDR(2, addr, 1),                     \
+                  SPI_MEM_DTR_OP_DUMMY(ndummy, 1),                     \
+                  SPI_MEM_DTR_OP_DATA_IN(len, buf, 1),                 \
+                  SPI_MEM_OP_MAX_FREQ(freq))

is IMHO better than

+#define SPINAND_PAGE_READ_FROM_CACHE_DTR_OPCODE 0x0d
+#define SPINAND_PAGE_READ_FROM_CACHE_DTR_OP(addr, ndummy, buf, len, freq) \
+       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINAND_PAGE_READ_FROM_CACHE_DTR_OPCODE, 1), \
+                  SPI_MEM_DTR_OP_ADDR(2, addr, 1),                     \
+                  SPI_MEM_DTR_OP_DUMMY(ndummy, 1),                     \
+                  SPI_MEM_DTR_OP_DATA_IN(len, buf, 1),                 \
+                  SPI_MEM_OP_MAX_FREQ(freq))

Re: [PATCH 22/24] mtd: spinand: Add support for read DTR operations

[Tudor Ambarus]

On 12/13/24 12:08 PM, Miquel Raynal wrote:
> Hi Tudor,
> 
> On 11/11/2024 at 14:35:23 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>>> +	SPI_MEM_OP(SPI_MEM_OP_CMD(0x0d, 1),	
>>
>> do we want some names to these hex values?
> 
> I honestly don't think we do because it would be totally redundant with
> the macro name, ie.

I agree. Thanks,
ta

[PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Miquel Raynal]

Make the link between the core macros and the datasheet.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/winbond.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 686e872fe0ff..9e2562805d23 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -18,6 +18,11 @@
 
 #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
 
+/*
+ * "X2" in the core is equivalent to "dual output" in the datasheets,
+ * "X4" in the core is equivalent to "quad output" in the datasheets.
+ */
+
 static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
-- 
2.43.0

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Tudor Ambarus]

On 10/25/24 5:15 PM, Miquel Raynal wrote:
> Make the link between the core macros and the datasheet.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
> index 686e872fe0ff..9e2562805d23 100644
> --- a/drivers/mtd/nand/spi/winbond.c
> +++ b/drivers/mtd/nand/spi/winbond.c
> @@ -18,6 +18,11 @@
>  
>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>  
> +/*
> + * "X2" in the core is equivalent to "dual output" in the datasheets,
> + * "X4" in the core is equivalent to "quad output" in the datasheets.
> + */

doesn't help great for an outsider like me. Is quad referring to cmd,
addr or data? Or maybe of all? I need to read the code anyway.
> +
>  static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Tudor Ambarus]

On 11/11/24 2:38 PM, Tudor Ambarus wrote:
> 
> 
> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>> Make the link between the core macros and the datasheet.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>> index 686e872fe0ff..9e2562805d23 100644
>> --- a/drivers/mtd/nand/spi/winbond.c
>> +++ b/drivers/mtd/nand/spi/winbond.c
>> @@ -18,6 +18,11 @@
>>  
>>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>>  
>> +/*
>> + * "X2" in the core is equivalent to "dual output" in the datasheets,
>> + * "X4" in the core is equivalent to "quad output" in the datasheets.
>> + */
> 
> doesn't help great for an outsider like me. Is quad referring to cmd,
> addr or data? Or maybe of all? I need to read the code anyway.

more straight forward would be to use the X-Y-Z terminology in the
comment, where X,Y and Z are the number of IO lines for cmd, address and
data.

>> +
>>  static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
>>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Miquel Raynal]

On 11/11/2024 at 14:38:53 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>> Make the link between the core macros and the datasheet.
>> 
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>> index 686e872fe0ff..9e2562805d23 100644
>> --- a/drivers/mtd/nand/spi/winbond.c
>> +++ b/drivers/mtd/nand/spi/winbond.c
>> @@ -18,6 +18,11 @@
>>  
>>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>>  
>> +/*
>> + * "X2" in the core is equivalent to "dual output" in the datasheets,
>> + * "X4" in the core is equivalent to "quad output" in the datasheets.
>> + */
>
> doesn't help great for an outsider like me. Is quad referring to cmd,
> addr or data? Or maybe of all? I need to read the code anyway.

I also don't like these terms. IIRC "output" is referring to the data cycles,
otherwise it means address (dummy) and data cycles.

In single, dual or quad mode the naming is unclear but "okay". But octal
DDR modes can require the opcode to be sent in octal mode as well, which
is new. If we support that, I'll take care of using a more
understandable naming for all macros like Xy-Xy-Xy, X being the
buswidth, y being S (sdr) or D (ddr) and the three members being
Command-Address-Data. I might even be tempted to include dummy cycles as
well, because it is important to be clear if eg. in octal mode "1" means
"1 cycle" or "8 cycles".

>> +
>>  static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
>>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

Thanks,
Miquèl

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Tudor Ambarus]

On 12/13/24 12:25 PM, Miquel Raynal wrote:
> On 11/11/2024 at 14:38:53 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>>> Make the link between the core macros and the datasheet.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>>> index 686e872fe0ff..9e2562805d23 100644
>>> --- a/drivers/mtd/nand/spi/winbond.c
>>> +++ b/drivers/mtd/nand/spi/winbond.c
>>> @@ -18,6 +18,11 @@
>>>  
>>>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>>>  
>>> +/*
>>> + * "X2" in the core is equivalent to "dual output" in the datasheets,
>>> + * "X4" in the core is equivalent to "quad output" in the datasheets.
>>> + */
>>
>> doesn't help great for an outsider like me. Is quad referring to cmd,
>> addr or data? Or maybe of all? I need to read the code anyway.
> 
> I also don't like these terms. IIRC "output" is referring to the data cycles,
> otherwise it means address (dummy) and data cycles.
> 
> In single, dual or quad mode the naming is unclear but "okay". But octal
> DDR modes can require the opcode to be sent in octal mode as well, which
> is new. If we support that, I'll take care of using a more
> understandable naming for all macros like Xy-Xy-Xy, X being the
> buswidth, y being S (sdr) or D (ddr) and the three members being

8d-8d-8d is common and covered by few standards, yes.

> Command-Address-Data. I might even be tempted to include dummy cycles as
> well, because it is important to be clear if eg. in octal mode "1" means
> "1 cycle" or "8 cycles".
I find the info about dummy cycles useful. I wonder if such terminology
is already specified in a standard. If not, maybe we can put the dummy
cycles after the mode, in parenthesis? I would refrain custom terminology.

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Miquel Raynal]

On 18/12/2024 at 08:14:36 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 12/13/24 12:25 PM, Miquel Raynal wrote:
>> On 11/11/2024 at 14:38:53 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>> 
>>> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>>>> Make the link between the core macros and the datasheet.
>>>>
>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>> ---
>>>>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>>>> index 686e872fe0ff..9e2562805d23 100644
>>>> --- a/drivers/mtd/nand/spi/winbond.c
>>>> +++ b/drivers/mtd/nand/spi/winbond.c
>>>> @@ -18,6 +18,11 @@
>>>>  
>>>>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>>>>  
>>>> +/*
>>>> + * "X2" in the core is equivalent to "dual output" in the datasheets,
>>>> + * "X4" in the core is equivalent to "quad output" in the datasheets.
>>>> + */
>>>
>>> doesn't help great for an outsider like me. Is quad referring to cmd,
>>> addr or data? Or maybe of all? I need to read the code anyway.
>> 
>> I also don't like these terms. IIRC "output" is referring to the data cycles,
>> otherwise it means address (dummy) and data cycles.
>> 
>> In single, dual or quad mode the naming is unclear but "okay". But octal
>> DDR modes can require the opcode to be sent in octal mode as well, which
>> is new. If we support that, I'll take care of using a more
>> understandable naming for all macros like Xy-Xy-Xy, X being the
>> buswidth, y being S (sdr) or D (ddr) and the three members being
>
> 8d-8d-8d is common and covered by few standards, yes.
>
>> Command-Address-Data. I might even be tempted to include dummy cycles as
>> well, because it is important to be clear if eg. in octal mode "1" means
>> "1 cycle" or "8 cycles".
> I find the info about dummy cycles useful. I wonder if such terminology
> is already specified in a standard. If not, maybe we can put the dummy
> cycles after the mode, in parenthesis? I would refrain custom terminology.

I see you concern, but would you mind giving an example of what you have
in mind?

Thanks,
Miquèl

Re: [PATCH 23/24] mtd: spinand: winbond: Add comment about naming

[Tudor Ambarus]

On 12/18/24 9:33 AM, Miquel Raynal wrote:
> On 18/12/2024 at 08:14:36 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> On 12/13/24 12:25 PM, Miquel Raynal wrote:
>>> On 11/11/2024 at 14:38:53 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>>> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>>>>> Make the link between the core macros and the datasheet.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>>>>> index 686e872fe0ff..9e2562805d23 100644
>>>>> --- a/drivers/mtd/nand/spi/winbond.c
>>>>> +++ b/drivers/mtd/nand/spi/winbond.c
>>>>> @@ -18,6 +18,11 @@
>>>>>  
>>>>>  #define W25N04KV_STATUS_ECC_5_8_BITFLIPS	(3 << 4)
>>>>>  
>>>>> +/*
>>>>> + * "X2" in the core is equivalent to "dual output" in the datasheets,
>>>>> + * "X4" in the core is equivalent to "quad output" in the datasheets.
>>>>> + */
>>>>
>>>> doesn't help great for an outsider like me. Is quad referring to cmd,
>>>> addr or data? Or maybe of all? I need to read the code anyway.
>>>
>>> I also don't like these terms. IIRC "output" is referring to the data cycles,
>>> otherwise it means address (dummy) and data cycles.
>>>
>>> In single, dual or quad mode the naming is unclear but "okay". But octal
>>> DDR modes can require the opcode to be sent in octal mode as well, which
>>> is new. If we support that, I'll take care of using a more
>>> understandable naming for all macros like Xy-Xy-Xy, X being the
>>> buswidth, y being S (sdr) or D (ddr) and the three members being
>>
>> 8d-8d-8d is common and covered by few standards, yes.
>>
>>> Command-Address-Data. I might even be tempted to include dummy cycles as
>>> well, because it is important to be clear if eg. in octal mode "1" means
>>> "1 cycle" or "8 cycles".
>> I find the info about dummy cycles useful. I wonder if such terminology
>> is already specified in a standard. If not, maybe we can put the dummy
>> cycles after the mode, in parenthesis? I would refrain custom terminology.
> 
> I see you concern, but would you mind giving an example of what you have
> in mind?
> 

on a second thought, the number of dummy cycles can vary depending on
frequency, so better to stick just to the
	Xy-Xy-Xy, X = {1,2,4,8}, y = {S, D} terminology.

[PATCH 24/24] mtd: spinand: winbond: Add support for DTR operations

[Miquel Raynal]

W25N01JW and W25N02JW support many DTR read modes in single, dual and
quad configurations.

DTR modes however cannot be used at 166MHz, as the bus frequency in
this case must be lowered to 80MHz.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/winbond.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 9e2562805d23..77897a52b149 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -24,10 +24,15 @@
  */
 
 static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
+		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP(0, 8, NULL, 0, 80 * HZ_PER_MHZ),
+		SPINAND_PAGE_READ_FROM_CACHE_X4_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_DTR_OP(0, 4, NULL, 0, 80 * HZ_PER_MHZ),
+		SPINAND_PAGE_READ_FROM_CACHE_X2_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
 		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
 		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0, 54 * HZ_PER_MHZ));
 
-- 
2.43.0

Re: [PATCH 24/24] mtd: spinand: winbond: Add support for DTR operations

[Tudor Ambarus]

On 10/25/24 5:15 PM, Miquel Raynal wrote:
> W25N01JW and W25N02JW support many DTR read modes in single, dual and
> quad configurations.
> 
> DTR modes however cannot be used at 166MHz, as the bus frequency in
> this case must be lowered to 80MHz.

ha, what's the benefit then? Aren't we better of with non dtr modes
then? 80 MHz * 2 < 166 MHz?

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/spi/winbond.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
> index 9e2562805d23..77897a52b149 100644
> --- a/drivers/mtd/nand/spi/winbond.c
> +++ b/drivers/mtd/nand/spi/winbond.c
> @@ -24,10 +24,15 @@
>   */
>  
>  static SPINAND_OP_VARIANTS(read_cache_dtr_variants,
> +		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP(0, 8, NULL, 0, 80 * HZ_PER_MHZ),
> +		SPINAND_PAGE_READ_FROM_CACHE_X4_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
>  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_DTR_OP(0, 4, NULL, 0, 80 * HZ_PER_MHZ),
> +		SPINAND_PAGE_READ_FROM_CACHE_X2_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
>  		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_DTR_OP(0, 2, NULL, 0, 80 * HZ_PER_MHZ),
>  		SPINAND_PAGE_READ_FROM_CACHE_FAST_OP(0, 1, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_OP(0, 1, NULL, 0, 54 * HZ_PER_MHZ));
>  

Re: [PATCH 24/24] mtd: spinand: winbond: Add support for DTR operations

[Miquel Raynal]

Hello Tudor,

On 11/11/2024 at 14:40:59 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>> W25N01JW and W25N02JW support many DTR read modes in single, dual and
>> quad configurations.
>> 
>> DTR modes however cannot be used at 166MHz, as the bus frequency in
>> this case must be lowered to 80MHz.
>
> ha, what's the benefit then? Aren't we better of with non dtr modes
> then? 80 MHz * 2 < 166 MHz?

This is actually a good question, and you are right DDR in this case
does not bring better throughputs, it can even make it a little bit
slower. I think however we should expect some gains at the PCB design
step, which may be way simpler as routing 8 166MHz lines might
apparently be challenging. It is common to have PCB limitations on the
frequency, so enabling DDR can be a great way to keep signal integrity
while definitely improving the performances. However, you're right, I
should probably move these definitions lower in the table to make sure
we run at 166MHz if that's possible.

Thanks,
Miquèl

Re: [PATCH 24/24] mtd: spinand: winbond: Add support for DTR operations

[Miquel Raynal]

Hello Tudor,

On 23/12/2024 at 19:22:12 +01, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello Tudor,
>
> On 11/11/2024 at 14:40:59 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>> On 10/25/24 5:15 PM, Miquel Raynal wrote:
>>> W25N01JW and W25N02JW support many DTR read modes in single, dual and
>>> quad configurations.
>>> 
>>> DTR modes however cannot be used at 166MHz, as the bus frequency in
>>> this case must be lowered to 80MHz.
>>
>> ha, what's the benefit then? Aren't we better of with non dtr modes
>> then? 80 MHz * 2 < 166 MHz?
>
> This is actually a good question, and you are right DDR in this case
> does not bring better throughputs, it can even make it a little bit
> slower. I think however we should expect some gains at the PCB design
> step, which may be way simpler as routing 8 166MHz lines might
> apparently be challenging. It is common to have PCB limitations on the
> frequency, so enabling DDR can be a great way to keep signal integrity
> while definitely improving the performances. However, you're right, I
> should probably move these definitions lower in the table to make sure
> we run at 166MHz if that's possible.

Actually this is probably not a good solution. This stable is parsed
once from top to bottom. The elements order decide whether we'll use a
variant or another, not by deciding which one is the fastest, but by
taking the first one that fits. But with DTR operations it no longer
works so well. If I list items like that:

SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP(0, 8, NULL, 0, 80 * HZ_PER_MHZ),
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0 /* 166 MHz */),

It is likely that I will not run at the fastest possible throughput, ie
about 160MHz instead of 166MHz. But if I do the reverse:

SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0 /* 166 MHz */),
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_DTR_OP(0, 8, NULL, 0, 80 * HZ_PER_MHZ),

I will only run at the fastest throughput if the PCB lines are so well
designed that they can support 166MHz. If they only support 150MHz (or
anything lower) it would have been better to pick the DTR op.

My conclusion is: the current logic needs to be improved, so I'm
drafting a slightly more complex variant picker which will evaluate the
theoretical length of an op based on the speed, parallel lines, DTR
capability, etc. This way the order in this table will no longer matter.

I will soon respin a series because I've enhanced a lot of things
inside.

Cheers,
Miquèl

Re: (subset) [PATCH 00/24] spi-nand/spi-mem DTR support

[Mark Brown]

On Fri, 25 Oct 2024 18:14:37 +0200, Miquel Raynal wrote:
> Here is a (big) series supposed to bring DTR support in SPI-NAND.
> 
> I could have split this into two but I eventually preferred showing the
> big picture. Once v1 will be over, I can make it two. However when we'll
> discuss merging, we'll have to share an immutable tag among the two
> subsystems.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency
        commit: 0fefeade90e74bc8f40ab0e460f483565c492e28
[02/24] spi: spi-mem: Add a new controller capability
        commit: 1248c9b8d54120950fda10fbeb98fb8932b4d45c
[03/24] spi: amd: Support per spi-mem operation frequency switches
        commit: d0e5faccb229b1dacc4c9fa11f6df33bb1fdabd8
[04/24] spi: amlogic-spifc-a1: Support per spi-mem operation frequency switches
        commit: 5baa189789e8894c58eacc7803e3c163c1d0fc0a
[05/24] spi: cadence-qspi: Support per spi-mem operation frequency switches
        commit: 06e9f5a1f6ba774d8942a168d3ec5ed5a008fbcb
[06/24] spi: dw: Support per spi-mem operation frequency switches
        commit: eee7bc9e7ade6f7ac17d9ec02887cd5509ba9427
[07/24] spi: fsl-qspi: Support per spi-mem operation frequency switches
        commit: 2438db5253eb17a7c0ccb15aea4252a150dda057
[08/24] spi: microchip-core-qspi: Support per spi-mem operation frequency switches
        commit: 13529647743d906ed3cf991f1d77727e7ff1fb6f
[09/24] spi: mt65xx: Support per spi-mem operation frequency switches
        commit: 13fd04b53053bbfa741a0f2a781837ab80e485f6
[10/24] spi: mxic: Support per spi-mem operation frequency switches
        commit: 67707cb094f134f5b3931eefbedbb9ca7e3209d0
[11/24] spi: nxp-fspi: Support per spi-mem operation frequency switches
        commit: 26851cf65ffca2d3a8d529a125e54cf0084d69e7
[12/24] spi: rockchip-sfc: Support per spi-mem operation frequency switches
        commit: d3f35dd3ad968256ed1080e3ea2022f947861cff
[13/24] spi: spi-sn-f-ospi: Support per spi-mem operation frequency switches
        commit: 1a206344218cc15ad8f321e3abab3f3d36ab639f
[14/24] spi: spi-ti-qspi: Support per spi-mem operation frequency switches
        commit: b2fac3192919dd07e7ce30558e34abd7e07dde77
[15/24] spi: zynq-qspi: Support per spi-mem operation frequency switches
        commit: 9a68f6c8d6cfddeac7c5874528ed04e50a1cb579
[16/24] spi: zynqmp-gqspi: Support per spi-mem operation frequency switches
        commit: 30eb2e6e78225f92f04a2325c6fd77fe8f5b4aab
[20/24] spi: spi-mem: Reorder SPI_MEM_OP_CMD internals
        (no commit info)
[21/24] spi: spi-mem: Create macros for DTR operation
        commit: f0006897a96c736623ddeb9b68c3880eb5cdebe7

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark