[PATCH RFC 0/7] Support for SPI RX Sampling Delay Compensation

[PATCH RFC 0/7] Support for SPI RX Sampling Delay Compensation

[Frieder Schrempf]

This is an attempt to continue the efforts started in [1] to support
SPI devices that are clocked at high speeds and have a significant
delay in the RX path.

This aims to handle the known RX path delay introduced by the SPI
device itself and specified in the datasheet as maximum tCLQV value
by:

1. Applying compensation if the controller driver supports it
2. Reducing the clock frequency to a safe value if the controller
   driver's compensation capabilities are exceeded.

I'm sending this as a new RFC as quite a few things have changed
compared to [1] and a lot of time has passed. Also I'm quite unsure
if the layering for this implementation is correct and wether
it conflicts or overlaps with the PHY tuning series [2] in some
way.

This approach adds a new attribute for SPI devices to track the
RX sampling delay requirements of the device and extends the SPI
NAND drivers for Toshiba and Winbond devices to specify the values
from the datasheets (patch 1-4).

Then it adds calls to a new handler in the SPI controller drivers to
the core and provides a generic helper to calculate the delay cycles
(patch 5-6).

At last it implements the new handler for the FSL QSPI driver in
order to compensate the RX sampling delay, or as a fallback reduce
the clock rate to a value that is safe to use without violating the
RX sampling delay requirement of the device (patch 7).

This does not yet implement the fallback to reduce the clock rate
for controllers that don't have compensation capabilities or that
have not yet implemented the compensation handler. Therefore this
series doesn't cause changes in behavior for controllers other than
FSL QSPI.

Please note that some drivers such as spi-stm32-qspi.c currently
enable a RX sampling delay of one half clock cycle unconditionally
for all devices. While this works in general, as all currently known
devices are in range of sampling the data at this point, when testing
this approach with FSL QSPI it failed at low clock rates (e.g.
100 kHz). This is probably due to the sampling happening too late,
when the data line is alredy changing its state as the delay is
negligible compared to the (rather long) clock period in this case.

This series was tested on i.MX6UL with the following SPI NAND devices:

* Toshiba/Kioxia TC58CVG2S0HRAIJ (133 MHz, tCLQV max. 6 ns)
* Winbond W25M02GV (104 MHz, tCLQV max. 7 ns)
* Winbond W25N02KV (104 MHz, tCLQV max. 7 ns)

The former two chips seem to work fine even without this patchset,
probably due to the actual RX sampling delay being lower than the
maximum value specified in the datasheet. The latter chip fails
reading at 104 MHz without RX sampling delay compensation.

[1] https://lore.kernel.org/all/20231026152316.2729575-1-estl@gmx.net/
[2] https://patchwork.ozlabs.org/project/linux-mtd/cover/20260113141617.1905039-1-s-k6@ti.com/

---
Eberhard Stoll (4):
      spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay
      mtd: spinand: Add support for clock to RX delay setting
      mtd: spinand: winbond: Add RX sampling delay values
      spi: spi-fsl-qspi: Add support for RX data sampling point adjustment

Frieder Schrempf (3):
      mtd: spinand: toshiba: Add RX sampling delay values
      spi: Add RX sampling point adjustment
      spi: spi-mem: Call spi_set_rx_sampling_point() for each op

 drivers/mtd/nand/spi/core.c    |  1 +
 drivers/mtd/nand/spi/toshiba.c | 48 ++++++++++++++++---------
 drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++--------
 drivers/spi/spi-fsl-qspi.c     | 80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-mem.c          |  2 ++
 drivers/spi/spi.c              | 73 ++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h    |  5 +++
 include/linux/spi/spi.h        | 11 ++++++
 8 files changed, 231 insertions(+), 31 deletions(-)
---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20260303-fsl-qspi-rx-sampling-delay-9133e51ce503

Best regards,
-- 
Frieder Schrempf <frieder.schrempf@kontron.de>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay

[Frieder Schrempf]

From: Eberhard Stoll <eberhard.stoll@kontron.de>

Some high speed SPI devices require a delay between the controller
clock and the sampling point of the client device. This 'clock to
receive data delay' value is often referenced as tCLQV in SPI device
data sheets.

Not respecting this can lead to data being sampled too early when
the client doesn't yet reflect the correct level at the data out
pin(s).

There are two ways to handle the situation:

1. Reduce the controller clock to the amount where the sampling
   point requirement of the client is still met, meaning the
   clock period being greater than two times tCLQV.

2. If the host controller supports it, compensate by delaying the
   sampling point to meet the tCLQV requirement of the client.

Add a parameter 'rx_sampling_delay_ns' in struct spi_device to enable
setting the clock to RX data delay for each SPI device, so drivers
can check and act upon this timing requirement.

Signed-off-by: Eberhard Stoll <eberhard.stoll@kontron.de>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 include/linux/spi/spi.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index af7cfee7b8f60..4f8a0c28e1d46 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -181,6 +181,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg,
  * @num_tx_lanes: Number of transmit lanes wired up.
  * @rx_lane_map: Map of peripheral lanes (index) to controller lanes (value).
  * @num_rx_lanes: Number of receive lanes wired up.
+ * @rx_sampling_delay_ns: spi clk to spi rx data delay
  *
  * A @spi_device is used to interchange data between an SPI target device
  * (usually a discrete chip) and CPU memory.
@@ -235,6 +236,8 @@ struct spi_device {
 	struct spi_delay	cs_setup;
 	struct spi_delay	cs_hold;
 	struct spi_delay	cs_inactive;
+	/* Transfer characteristics */
+	u32			rx_sampling_delay_ns; /* clk to rx data delay */
 
 	u8			chip_select[SPI_DEVICE_CS_CNT_MAX];
 	u8			num_chipselect;

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 2/7] mtd: spinand: Add support for clock to RX delay setting

[Frieder Schrempf]

From: Eberhard Stoll <eberhard.stoll@kontron.de>

Add the configuration parameter to set the SPI clock to receive
data delay parameter for SPI NAND devices.

This parameter is often referenced as tCLQV in SPI NAND device
data sheets.

Signed-off-by: Eberhard Stoll <eberhard.stoll@kontron.de>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/spi/core.c | 1 +
 include/linux/mtd/spinand.h | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 8aa3753aaaa1d..782a605018bc3 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1594,6 +1594,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
 		spinand->user_otp = &table[i].user_otp;
 		spinand->read_retries = table[i].read_retries;
 		spinand->set_read_retry = table[i].set_read_retry;
+		spinand->spimem->spi->rx_sampling_delay_ns = table[i].rx_sampling_delay_ns;
 
 		/* I/O variants selection with single-spi SDR commands */
 
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6a024cf1c53ac..b76aaf0747962 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -568,6 +568,7 @@ enum spinand_bus_interface {
  * @model: model name
  * @devid: device ID
  * @flags: OR-ing of the SPINAND_XXX flags
+ * @rx_sampling_delay_ns: clock to rx data delay timing (tCLQV)
  * @memorg: memory organization
  * @eccreq: ECC requirements
  * @eccinfo: on-die ECC info
@@ -592,6 +593,7 @@ struct spinand_info {
 	const char *model;
 	struct spinand_devid devid;
 	u32 flags;
+	u32 rx_sampling_delay_ns;
 	struct nand_memory_organization memorg;
 	struct nand_ecc_props eccreq;
 	struct spinand_ecc_info eccinfo;
@@ -643,6 +645,9 @@ struct spinand_info {
 #define SPINAND_CONFIGURE_CHIP(__configure_chip)			\
 	.configure_chip = __configure_chip
 
+#define SPINAND_RX_SAMPLING_DELAY(__delay)				\
+	.rx_sampling_delay_ns = __delay
+
 #define SPINAND_CONT_READ(__set_cont_read)				\
 	.set_cont_read = __set_cont_read
 

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 3/7] mtd: spinand: winbond: Add RX sampling delay values

[Frieder Schrempf]

From: Eberhard Stoll <eberhard.stoll@kontron.de>

Add tCLQV (Clock Low to Output Valid) parameter from datasheets
for Winbond SPI NAND chips. This allows to let the drivers
properly handle high sampling delays at high clock speeds.

Signed-off-by: Eberhard Stoll <eberhard.stoll@kontron.de>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 6dfd0dcc8ee7a..ec223e6d695a7 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -458,7 +458,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
+		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1G-bit densities */
 	SPINAND_INFO("W25N01GV", /* 3.3V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa, 0x21),
@@ -468,7 +469,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
+		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W25N01GW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xba, 0x21),
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
@@ -477,7 +479,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
+		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	SPINAND_INFO("W25N01JW", /* high-speed 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xbc, 0x21),
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
@@ -487,7 +490,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&w25n01jw_ooblayout, NULL),
-		     SPINAND_CONFIGURE_CHIP(w25n0xjw_hs_cfg)),
+		     SPINAND_CONFIGURE_CHIP(w25n0xjw_hs_cfg),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	SPINAND_INFO("W25N01KV", /* 3.3V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xae, 0x21),
 		     NAND_MEMORG(1, 2048, 96, 64, 1024, 20, 1, 1, 1),
@@ -496,7 +500,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25n01kv_ooblayout, w25n02kv_ecc_get_status)),
+		     SPINAND_ECCINFO(&w25n01kv_ooblayout, w25n02kv_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W35N01JW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc, 0x21),
 		     NAND_MEMORG(1, 4096, 128, 64, 512, 10, 1, 1, 1),
@@ -507,7 +512,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     0,
 		     SPINAND_INFO_VENDOR_OPS(&winbond_w35_ops),
 		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
-		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg)),
+		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	SPINAND_INFO("W35N02JW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdf, 0x22),
 		     NAND_MEMORG(1, 4096, 128, 64, 512, 10, 1, 2, 1),
@@ -518,7 +524,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     0,
 		     SPINAND_INFO_VENDOR_OPS(&winbond_w35_ops),
 		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
-		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg)),
+		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	SPINAND_INFO("W35N04JW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdf, 0x23),
 		     NAND_MEMORG(1, 4096, 128, 64, 512, 10, 1, 4, 1),
@@ -529,7 +536,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     0,
 		     SPINAND_INFO_VENDOR_OPS(&winbond_w35_ops),
 		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
-		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg)),
+		     SPINAND_CONFIGURE_CHIP(w35n0xjw_vcr_cfg),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 2G-bit densities */
 	SPINAND_INFO("W25M02GV", /* 2x1G-bit 3.3V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xab, 0x21),
@@ -541,7 +549,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 		     0,
 		     SPINAND_INFO_VENDOR_OPS(&winbond_w25_ops),
 		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
-		     SPINAND_SELECT_TARGET(w25m02gv_select_target)),
+		     SPINAND_SELECT_TARGET(w25m02gv_select_target),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W25N02JW", /* high-speed 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xbf, 0x22),
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 2, 1),
@@ -551,7 +560,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
-		     SPINAND_CONFIGURE_CHIP(w25n0xjw_hs_cfg)),
+		     SPINAND_CONFIGURE_CHIP(w25n0xjw_hs_cfg),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W25N02KV", /* 3.3V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa, 0x22),
 		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
@@ -560,7 +570,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status)),
+		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W25N02KW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xba, 0x22),
 		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
@@ -569,7 +580,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status)),
+		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	/* 4G-bit densities */
 	SPINAND_INFO("W25N04KV", /* 3.3V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa, 0x23),
@@ -579,7 +591,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status)),
+		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(7)),
 	SPINAND_INFO("W25N04KW", /* 1.8V */
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xba, 0x23),
 		     NAND_MEMORG(1, 2048, 128, 64, 4096, 40, 1, 1, 1),
@@ -588,7 +601,8 @@ static const struct spinand_info winbond_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status)),
+		     SPINAND_ECCINFO(&w25n02kv_ooblayout, w25n02kv_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 };
 
 static int winbond_spinand_init(struct spinand_device *spinand)

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 4/7] mtd: spinand: toshiba: Add RX sampling delay values

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Add tCLQV (Clock Low to Output Valid) parameter from datasheets
for Toshiba/Kioxia SPI NAND chips. This allows to let the drivers
properly handle high sampling delays at high clock speeds.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/spi/toshiba.c | 48 +++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c
index ef649162ee680..7679f3acbae07 100644
--- a/drivers/mtd/nand/spi/toshiba.c
+++ b/drivers/mtd/nand/spi/toshiba.c
@@ -118,7 +118,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 3.3V 2Gb (1st generation) */
 	SPINAND_INFO("TC58CVG1S3HRAIG",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xCB),
@@ -129,7 +130,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	/* 3.3V 4Gb (1st generation) */
 	SPINAND_INFO("TC58CVG2S0HRAIG",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xCD),
@@ -140,7 +142,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	/* 1.8V 1Gb (1st generation) */
 	SPINAND_INFO("TC58CYG0S3HRAIG",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xB2),
@@ -151,7 +154,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	/* 1.8V 2Gb (1st generation) */
 	SPINAND_INFO("TC58CYG1S3HRAIG",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xBB),
@@ -162,7 +166,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 	/* 1.8V 4Gb (1st generation) */
 	SPINAND_INFO("TC58CYG2S0HRAIG",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xBD),
@@ -173,7 +178,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(8)),
 
 	/*
 	 * 2nd generation serial nand has HOLD_D which is equivalent to
@@ -189,7 +195,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 3.3V 2Gb (2nd generation) */
 	SPINAND_INFO("TC58CVG1S3HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xEB),
@@ -200,7 +207,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 3.3V 4Gb (2nd generation) */
 	SPINAND_INFO("TC58CVG2S0HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xED),
@@ -211,7 +219,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 3.3V 8Gb (2nd generation) */
 	SPINAND_INFO("TH58CVG3S0HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xE4),
@@ -222,7 +231,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1.8V 1Gb (2nd generation) */
 	SPINAND_INFO("TC58CYG0S3HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xD2),
@@ -233,7 +243,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1.8V 2Gb (2nd generation) */
 	SPINAND_INFO("TC58CYG1S3HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xDB),
@@ -244,7 +255,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1.8V 4Gb (2nd generation) */
 	SPINAND_INFO("TC58CYG2S0HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xDD),
@@ -255,7 +267,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1.8V 8Gb (2nd generation) */
 	SPINAND_INFO("TH58CYG3S0HRAIJ",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xD4),
@@ -266,7 +279,8 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
+		     SPINAND_RX_SAMPLING_DELAY(6)),
 	/* 1.8V 1Gb (1st generation) */
 	SPINAND_INFO("TC58NYG0S3HBAI4",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xA1),
@@ -277,7 +291,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
 	/* 1.8V 4Gb (1st generation) */
 	SPINAND_INFO("TH58NYG2S3HBAI4",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xAC),
@@ -288,7 +302,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
 	/* 1.8V 8Gb (1st generation) */
 	SPINAND_INFO("TH58NYG3S0HBAI6",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xA3),
@@ -299,7 +313,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
 					      &update_cache_x4_variants),
 		     SPINAND_HAS_QE_BIT,
 		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
-				     tx58cxgxsxraix_ecc_get_status)),
+				     tx58cxgxsxraix_ecc_get_status),
 };
 
 static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = {

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 5/7] spi: Add RX sampling point adjustment

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Some SPI devices such as SPI NAND chips specify a clock-to-RX-sampling
delay constraint, which means that for the data read from the device
at a certain clock rate, we need to make sure that the point at
which the data is sampled is correct.

The default is to assume that the data can be sampled one half clock
cycle after the triggering clock edge. For high clock rates, this
can be too early.

To check this we introduce a new core function spi_set_rx_sampling_point()
and a handler set_rx_sampling_point() in the SPI controller.

Controllers implementing set_rx_sampling_point() can calculate the
sampling point delay using the helper spi_calc_rx_sampling_point()
and store the value to set the appropriate registers during transfer.

In case the controller capabilities are not sufficient to compensate
the RX delay, spi_set_rx_sampling_point() returns a reduced clock
rate value that is safe to use.

This commit does not introduce generic logic for controllers that
don't implement set_rx_sampling_point() in order to reduce the clock
rate if the RX sampling delay requirement can not be met.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/spi/spi.c       | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |  8 ++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 61f7bde8c7fbb..b039007ed430f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3988,6 +3988,77 @@ static int spi_set_cs_timing(struct spi_device *spi)
 	return status;
 }
 
+/**
+ * spi_calc_rx_sampling_point - calculate RX sampling delay cycles
+ * @spi: the device that requires specific a RX sampling delay
+ * @freq: pointer to the clock frequency setpoint for the calculation. This gets
+ *        altered to a reduced value if required.
+ * @max_delay_cycles: the upper limit of supported delay cycles
+ * @delay_cycles_per_clock_cycle: the ratio between delay cycles and
+ *				  master clock cycles
+ *
+ * This function takes in the rx_sampling_delay_ns value from the SPI device
+ * and the given clock frequency setpoint and calculates the required sampling
+ * delay cycles to meet the device's spec. It uses the given controller
+ * constraints and if those are exceeded, it adjusts the clock frequency
+ * setpoint to a lower value that is safe to be used.
+ *
+ * Return: calculated number of delay cycles
+ */
+unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
+					u16 max_delay_cycles,
+					u16 delay_cycles_per_clock_cycle)
+{
+	unsigned long long temp;
+	u16 delay_cycles;
+
+	/* if sampling delay is zero, we assume the default sampling point can be used */
+	if (!spi->rx_sampling_delay_ns)
+		return 0;
+
+	temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
+	do_div(temp, 1000000000UL);
+	delay_cycles = temp;
+
+	if (delay_cycles > max_delay_cycles) {
+		/*
+		 * Reduce the clock to the point where the sampling delay requirement
+		 * can be met.
+		 */
+		delay_cycles = max_delay_cycles;
+		temp = (1000000000UL * delay_cycles);
+		do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
+		*freq = temp;
+	}
+
+	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);
+
+	return delay_cycles;
+}
+EXPORT_SYMBOL_GPL(spi_calc_rx_sampling_point);
+
+/**
+ * spi_set_rx_sampling_point - set the RX sampling delay in the controller driver
+ * @spi: the device that requires specific a RX sampling delay
+ * @freq: the clock frequency setpoint for the RX sampling delay calculation
+ *
+ * This function calls the set_rx_sampling_point() handle in the controller
+ * driver it is available. This makes sure that the controller uses the proper
+ * RX sampling point adjustment. This function should be called whenever
+ * the devices rx_sampling_delay_ns or the currently used clock frequency
+ * changes.
+ *
+ * Return: adjusted clock frequency
+ */
+unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
+{
+	if (spi->controller->set_rx_sampling_point)
+		return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
+
+	return freq;
+}
+EXPORT_SYMBOL_GPL(spi_set_rx_sampling_point);
+
 /**
  * spi_setup - setup SPI mode and clock rate
  * @spi: the device whose settings are being modified
@@ -4090,6 +4161,8 @@ int spi_setup(struct spi_device *spi)
 		}
 	}
 
+	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);
+
 	status = spi_set_cs_timing(spi);
 	if (status) {
 		mutex_unlock(&spi->controller->io_mutex);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4f8a0c28e1d46..f5be4f54d1424 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -685,6 +685,9 @@ struct spi_controller {
 	 */
 	int (*set_cs_timing)(struct spi_device *spi);
 
+	/* set RX sampling point */
+	unsigned int (*set_rx_sampling_point)(struct spi_device *spi, unsigned int freq);
+
 	/*
 	 * Bidirectional bulk transfers
 	 *
@@ -1337,6 +1340,11 @@ extern int spi_setup(struct spi_device *spi);
 extern int spi_async(struct spi_device *spi, struct spi_message *message);
 extern int spi_target_abort(struct spi_device *spi);
 
+unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
+					       u16 max_delay_cycles,
+					       u16 delay_cycles_per_clock_cycle);
+unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq);
+
 static inline size_t
 spi_max_message_size(struct spi_device *spi)
 {

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

With clock rates changing on a per-op basis, we need to make sure
that we meet the RX sampling point delay constraint of the underlying
SPI chip.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/spi/spi-mem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index a09371a075d2e..6b8bee7d6f5e3 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -589,6 +589,8 @@ void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
 {
 	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
 		op->max_freq = mem->spi->max_speed_hz;
+
+	op->max_freq = spi_set_rx_sampling_point(mem->spi, op->max_freq);
 }
 EXPORT_SYMBOL_GPL(spi_mem_adjust_op_freq);
 

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH RFC 7/7] spi: spi-fsl-qspi: Add support for RX data sampling point adjustment

[Frieder Schrempf]

From: Eberhard Stoll <eberhard.stoll@kontron.de>

This QSPI controller supports shifting the RX data sampling point to
compensate line or SPI device response delays. This enables fast SPI
data transfers even for devices which have a significant delay in the
RX data stream.

Implement the set_rx_sampling_point() handler to:

1. check if the sampling point needs to be adjusted
2. prepare a value for the SDRSMP bits in the SMPR register and save it
3. reduce the clock rate if sampling point adjustment is not enough

During operation the SDRSMP bits in the SMPR register get updated
with the calculated value.

Signed-off-by: Eberhard Stoll <eberhard.stoll@kontron.de>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/spi/spi-fsl-qspi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index a223b4bc6e637..7021db144f84b 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -88,6 +88,7 @@
 
 #define QUADSPI_SMPR			0x108
 #define QUADSPI_SMPR_DDRSMP_MASK	GENMASK(18, 16)
+#define QUADSPI_SMPR_SDRSMP(x)		((x) << 5)
 #define QUADSPI_SMPR_FSDLY_MASK		BIT(6)
 #define QUADSPI_SMPR_FSPHS_MASK		BIT(5)
 #define QUADSPI_SMPR_HSENA_MASK		BIT(0)
@@ -290,6 +291,16 @@ struct fsl_qspi {
 	struct device *dev;
 	int selected;
 	u32 memmap_phy;
+	u32 smpr;
+};
+
+#define RX_SAMPLING_MAX_DELAY_CYCLES			3
+#define RX_SAMPLING_DELAY_CYCLES_PER_CLOCK_CYCLE	2
+
+struct fsl_qspi_chip_data {
+	u8 sdrsmp;
+	u32 rx_sampling_delay_ns;
+	unsigned int rate;
 };
 
 static bool needs_swap_endian(struct fsl_qspi *q)
@@ -545,6 +556,27 @@ static void fsl_qspi_invalidate(struct fsl_qspi *q)
 	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
 }
 
+static void fsl_qspi_update_rx_sampling_delay(struct fsl_qspi *q, u32 sdrsmp)
+{
+	void __iomem *base = q->iobase;
+	u32 mcr, smpr = QUADSPI_SMPR_SDRSMP(sdrsmp);
+
+	/* skip if requested value matches cached value */
+	if (q->smpr == smpr)
+		return;
+
+	/* Disable the module */
+	mcr = qspi_readl(q, base + QUADSPI_MCR);
+	qspi_writel(q, mcr | QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
+
+	qspi_writel(q, smpr, base + QUADSPI_SMPR);
+
+	/* Enable the module */
+	qspi_writel(q, mcr & ~QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
+
+	q->smpr = smpr;
+}
+
 static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi,
 				const struct spi_mem_op *op)
 {
@@ -668,6 +700,7 @@ static int fsl_qspi_readl_poll_tout(struct fsl_qspi *q, void __iomem *base,
 static int fsl_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	struct fsl_qspi *q = spi_controller_get_devdata(mem->spi->controller);
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(mem->spi);
 	void __iomem *base = q->iobase;
 	u32 addr_offset = 0;
 	int err = 0;
@@ -679,6 +712,8 @@ 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_update_rx_sampling_delay(q, chip->sdrsmp);
+
 	fsl_qspi_select_mem(q, mem->spi, op);
 
 	if (needs_amba_base_offset(q))
@@ -871,6 +906,28 @@ static const struct spi_controller_mem_caps fsl_qspi_mem_caps = {
 	.per_op_freq = true,
 };
 
+static int fsl_qspi_setup_device(struct spi_device *spi)
+{
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+
+	if (!chip) {
+		chip = kzalloc_obj(*chip, GFP_KERNEL);
+		if (!chip)
+			return -ENOMEM;
+		spi_set_ctldata(spi, chip);
+	}
+
+	return 0;
+}
+
+static void fsl_qspi_cleanup_device(struct spi_device *spi)
+{
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+
+	kfree(chip);
+	spi_set_ctldata(spi, NULL);
+}
+
 static void fsl_qspi_disable(void *data)
 {
 	struct fsl_qspi *q = data;
@@ -891,6 +948,25 @@ static void fsl_qspi_cleanup(void *data)
 	mutex_destroy(&q->lock);
 }
 
+static unsigned int fsl_qspi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
+{
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+
+	/* skip calculation if input clock rate and sampling delay did not change */
+	if (spi->rx_sampling_delay_ns == chip->rx_sampling_delay_ns &&
+	    freq == chip->rate)
+		return freq;
+
+	chip->rate = freq;
+	chip->rx_sampling_delay_ns = spi->rx_sampling_delay_ns;
+
+	chip->sdrsmp = spi_calc_rx_sampling_point(spi, &freq,
+						  RX_SAMPLING_MAX_DELAY_CYCLES,
+						  RX_SAMPLING_DELAY_CYCLES_PER_CLOCK_CYCLE);
+
+	return freq;
+}
+
 static int fsl_qspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -915,6 +991,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, q);
 
+	ctlr->setup = fsl_qspi_setup_device;
+	ctlr->cleanup = fsl_qspi_cleanup_device;
+	ctlr->set_rx_sampling_point = fsl_qspi_set_rx_sampling_point;
+
 	/* find the resources */
 	q->iobase = devm_platform_ioremap_resource_byname(pdev, "QuadSPI");
 	if (IS_ERR(q->iobase))

-- 
2.53.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

Subject: Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point()
 for each op

> +	op->max_freq = spi_set_rx_sampling_point(mem->spi, op->max_freq);

Does spi_set_rx_sampling_point() return an error code? If so, you need to
check for it. If it can fail, silently ignoring the return value may cause
timing violations. Suggest adding error handling or documenting why failure
is acceptable here.

Also, the blank line before this statement is unnecessary. Remove it to keep
the function compact.

---

AI bot review and may be useless.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 7/7] spi: spi-fsl-qspi: Add support for RX data sampling point adjustment

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

AI bot review and may be useless.

> +#define QUADSPI_SMPR_SDRSMP(x)		((x) << 5)

Macro argument 'x' should be parenthesized: ((x) << 5) is fine, but
consider ((x) << 5) for consistency with kernel style.

> +struct fsl_qspi_chip_data {
> +	u8 sdrsmp;
> +	u32 rx_sampling_delay_ns;
> +	unsigned int rate;
> +};

Consider documenting struct members with kernel-doc comments explaining
the purpose of each field.

> +static void fsl_qspi_update_rx_sampling_delay(struct fsl_qspi *q, u32 sdrsmp)
> +{
> +	void __iomem *base = q->iobase;
> +	u32 mcr, smpr = QUADSPI_SMPR_SDRSMP(sdrsmp);
> +
> +	/* skip if requested value matches cached value */
> +	if (q->smpr == smpr)
> +		return;
> +
> +	/* Disable the module */
> +	mcr = qspi_readl(q, base + QUADSPI_MCR);
> +	qspi_writel(q, mcr | QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
> +
> +	qspi_writel(q, smpr, base + QUADSPI_SMPR);
> +
> +	/* Enable the module */
> +	qspi_writel(q, mcr & ~QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
> +
> +	q->smpr = smpr;
> +}

Missing synchronization: if fsl_qspi_exec_op() calls this concurrently
from multiple threads, q->smpr cache may become inconsistent. Consider
protecting with q->lock (already used elsewhere in driver).

> +	struct fsl_qspi_chip_data *chip = spi_get_ctldata(mem->spi);
> +	void __iomem *base = q->iobase;
> +	u32 addr_offset = 0;
> +	int err = 0;
> @@ -679,6 +712,8 @@ 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_update_rx_sampling_delay(q, chip->sdrsmp);

Potential NULL dereference: if fsl_qspi_setup_device() was never called
or failed silently, 'chip' is NULL. Add NULL check or ensure setup is
always called before exec_op.

> +static int fsl_qspi_setup_device(struct spi_device *spi)
> +{
> +	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
> +
> +	if (!chip) {
> +		chip = kzalloc_obj(*chip, GFP_KERNEL);

kzalloc_obj() is non-standard. Use kzalloc(sizeof(*chip), GFP_KERNEL)
or verify kzalloc_obj() is defined in kernel headers.

> +		if (!chip)
> +			return -ENOMEM;
> +		spi_set_ctldata(spi, chip);
> +	}
> +
> +	return 0;
> +}

setup_device() allows re-entry without freeing old chip. If called

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 5/7] spi: Add RX sampling point adjustment

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

> +unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
> +					u16 max_delay_cycles,
> +					u16 delay_cycles_per_clock_cycle)
> +{
> +	unsigned long long temp;
> +	u16 delay_cycles;
> +
> +	/* if sampling delay is zero, we assume the default sampling point can be used */
> +	if (!spi->rx_sampling_delay_ns)
> +		return 0;
> +
> +	temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
> +	do_div(temp, 1000000000UL);
> +	delay_cycles = temp;

Potential overflow: multiplying three u32/u16 values before assigning to
u64. Cast operands to u64 before multiplication to avoid intermediate
overflow.

> +	if (delay_cycles > max_delay_cycles) {
> +		/*
> +		 * Reduce the clock to the point where the sampling delay requirement
> +		 * can be met.
> +		 */
> +		delay_cycles = max_delay_cycles;
> +		temp = (1000000000UL * delay_cycles);
> +		do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
> +		*freq = temp;

Same overflow risk in denominator multiplication. Cast to u64 first.

> +	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);

Line exceeds 80 chars. Break into two lines.

> +unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
> +{
> +	if (spi->controller->set_rx_sampling_point)
> +		return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
> +
> +	return freq;
> +}

Bug: function receives 'freq' parameter but ignores it and passes
'spi->max_speed_hz' to the handler instead. Should pass 'freq' or remove
the parameter.

> +	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);

This call happens after spi_set_cs_timing() check but before the mutex is
unlocked. Verify this is the intended placement and that no locking issues
exist with controller callbacks.

AI bot review and may be useless.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

> + * @rx_sampling_delay_ns: spi clk to spi rx data delay

Comment is too terse. Expand to clarify the unit and purpose:
"clock-to-RX-data delay in nanoseconds; see tCLQV in device datasheets"

> +	/* Transfer characteristics */
> +	u32			rx_sampling_delay_ns; /* clk to rx data delay */

The inline comment duplicates the field name. Remove it or expand the
block comment above to explain when/how drivers should use this field.
Also: is u32 the right type? Consider if negative values or larger
ranges are possible, or if this should be `struct spi_delay` for
consistency with cs_setup, cs_hold, cs_inactive above.

> +	struct spi_delay	cs_inactive;
> +	/* Transfer characteristics */

The comment "Transfer characteristics" is vague. Either name it more
specifically (e.g., "RX sampling configuration") or fold it into the
existing block comment at the top of the struct.

---

AI bot review and may be useless.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 2/7] mtd: spinand: Add support for clock to RX delay setting

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

> +		spinand->spimem->spi->rx_sampling_delay_ns = table[i].rx_sampling_delay_ns;

Line exceeds 80 columns (currently ~85). Break into two lines or shorten
variable names.

Also, this assignment happens unconditionally for every matched device.
Should there be a null check on spinand->spimem or spinand->spimem->spi
before dereferencing? Or is this guaranteed to be initialized at this
point in the probe flow?

> + * @rx_sampling_delay_ns: clock to rx data delay timing (tCLQV)

Good. Clarifies the datasheet reference.

> +	u32 rx_sampling_delay_ns;

Consider whether this field should be initialized to 0 or a sensible
default in the macro. Currently, devices that don't use the
SPINAND_RX_SAMPLING_DELAY() macro will have an uninitialized value
(likely 0 from kzalloc, but worth documenting).

> +#define SPINAND_RX_SAMPLING_DELAY(__delay)				\
> +	.rx_sampling_delay_ns = __delay

Macro looks fine. Consistent with existing SPINAND_* patterns.

---

AI bot review and may be useless.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 4/7] mtd: spinand: toshiba: Add RX sampling delay values

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

AI bot review and may be useless.

> -				     tx58cxgxsxraix_ecc_get_status)),
> +				     tx58cxgxsxraix_ecc_get_status),
> +		     SPINAND_RX_SAMPLING_DELAY(6)),

Inconsistent indentation: use tabs to align with opening paren on
previous line. Should be two tabs + spaces to match column position.

> +		     SPINAND_RX_SAMPLING_DELAY(8)),

Same indentation issue across all added SPINAND_RX_SAMPLING_DELAY()
lines. Verify alignment matches the struct initializer style used
elsewhere in the file.

> -				     tx58cxgxsxraix_ecc_get_status)),
> +				     tx58cxgxsxraix_ecc_get_status),
> +		     SPINAND_RX_SAMPLING_DELAY(6)),

Line 195: trailing comma after SPINAND_RX_SAMPLING_DELAY(6) is correct
for struct init, but confirm this macro expands to a valid struct
member or initializer list entry.

No resource leaks or locking issues detected in error paths. Patch is
primarily data-driven table updates with no dynamic allocation or
synchronization primitives.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 3/7] mtd: spinand: winbond: Add RX sampling delay values

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

AI bot review and may be useless.

The patch looks straightforward—adding RX sampling delay parameters to
Winbond SPI NAND device entries. A few observations:

> -		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
> +		     SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
> +		     SPINAND_RX_SAMPLING_DELAY(6)),

Trailing comma removal on the SPINAND_ECCINFO line is correct for the
macro continuation. Consistent across all hunks.

> +		     SPINAND_RX_SAMPLING_DELAY(6)),

The delay values (6, 7, 8) appear to come from datasheets per the commit
message. No obvious logic errors, but consider adding a brief inline
comment explaining the tCLQV mapping if the values are not self-evident
to future maintainers. For example:

  /* tCLQV from datasheet */
  SPINAND_RX_SAMPLING_DELAY(6)),

This is minor—the commit message already documents the intent. The patch
is mechanically sound: consistent formatting, no resource leaks, no
locking issues. All entries follow the same pattern.

One style note: ensure the macro SPINAND_RX_SAMPLING_DELAY is defined
elsewhere in the patchset (likely in an earlier patch in the series).
If not already reviewed, verify its signature matches the usage here.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

On Tue, Mar 03, 2026 at 04:01:14PM -0500, Frank Li wrote:
> From: Frank Li (AI-BOT) <frank.li@nxp.com>

Please add a human step before sending these reviews.

> > + * @rx_sampling_delay_ns: spi clk to spi rx data delay

> Comment is too terse. Expand to clarify the unit and purpose:
> "clock-to-RX-data delay in nanoseconds; see tCLQV in device datasheets"

I'm not sure restating the unit in the comment as well as the name is
super useful...

> > +	/* Transfer characteristics */
> > +	u32			rx_sampling_delay_ns; /* clk to rx data delay */

> The inline comment duplicates the field name. Remove it or expand the
> block comment above to explain when/how drivers should use this field.
> Also: is u32 the right type? Consider if negative values or larger
> ranges are possible, or if this should be `struct spi_delay` for
> consistency with cs_setup, cs_hold, cs_inactive above.

...especially given the contradictory feedback here, and the comment
about a negative delay is obviously not appropriate.  I'm not convinced
anyone needs an explanation of what to do with the field either.

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay

[Frank Li]

> 
> On Tue, Mar 03, 2026 at 04:01:14PM -0500, Frank Li wrote:
> > From: Frank Li (AI-BOT) <frank.li@nxp.com>
> 
> Please add a human step before sending these reviews.
> 
> > > + * @rx_sampling_delay_ns: spi clk to spi rx data delay
> 
> > Comment is too terse. Expand to clarify the unit and purpose:
> > "clock-to-RX-data delay in nanoseconds; see tCLQV in device datasheets"
> 
> I'm not sure restating the unit in the comment as well as the name is
> super useful...
> 
> > > +	/* Transfer characteristics */
> > > +	u32			rx_sampling_delay_ns; /* clk to rx data delay
> */
> 
> > The inline comment duplicates the field name. Remove it or expand the
> > block comment above to explain when/how drivers should use this field.
> > Also: is u32 the right type? Consider if negative values or larger
> > ranges are possible, or if this should be `struct spi_delay` for
> > consistency with cs_setup, cs_hold, cs_inactive above.
> 
> ...especially given the contradictory feedback here, and the comment
> about a negative delay is obviously not appropriate.  I'm not convinced
> anyone needs an explanation of what to do with the field either.

Sorry, I have not careful review AI bot generated result before sent
Out. Please discard it

Frank

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Miquel Raynal]

Hi Frieder,

First, thanks for the series! I believe it will conflict with the SPI
tuning series as you mentioned, but we should be able to make them live
side by side.

On 03/03/2026 at 17:29:27 +01, Frieder Schrempf <frieder@fris.de> wrote:

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> With clock rates changing on a per-op basis, we need to make sure
> that we meet the RX sampling point delay constraint of the underlying
> SPI chip.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/spi/spi-mem.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index a09371a075d2e..6b8bee7d6f5e3 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -589,6 +589,8 @@ void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
>  {
>  	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>  		op->max_freq = mem->spi->max_speed_hz;
> +
> +	op->max_freq = spi_set_rx_sampling_point(mem->spi,
> op->max_freq);

How can this work with the above 2 lines? Maybe there will be the need
to use a "min of the max" frequencies?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 2/7] mtd: spinand: Add support for clock to RX delay setting

[Miquel Raynal]

> +#define SPINAND_RX_SAMPLING_DELAY(__delay)				\
> +	.rx_sampling_delay_ns = __delay

For this macro I would be in favour of suffixing it with the unit,
ie: _NS.

Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 4/7] mtd: spinand: toshiba: Add RX sampling delay values

[Miquel Raynal]

> @@ -277,7 +291,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>  					      &update_cache_variants),
>  		     0,
>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
> -				     tx58cxgxsxraix_ecc_get_status)),
> +				     tx58cxgxsxraix_ecc_get_status),
>  	/* 1.8V 4Gb (1st generation) */
>  	SPINAND_INFO("TH58NYG2S3HBAI4",
>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xAC),
> @@ -288,7 +302,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>  					      &update_cache_x4_variants),
>  		     SPINAND_HAS_QE_BIT,
>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
> -				     tx58cxgxsxraix_ecc_get_status)),
> +				     tx58cxgxsxraix_ecc_get_status),
>  	/* 1.8V 8Gb (1st generation) */
>  	SPINAND_INFO("TH58NYG3S0HBAI6",
>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xA3),
> @@ -299,7 +313,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>  					      &update_cache_x4_variants),
>  		     SPINAND_HAS_QE_BIT,
>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
> -				     tx58cxgxsxraix_ecc_get_status)),
> +				     tx58cxgxsxraix_ecc_get_status),
>  };

Are these 3 changes legitimate?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 5/7] spi: Add RX sampling point adjustment

[Miquel Raynal]

Hello,

+ Santhosh

On 03/03/2026 at 17:29:26 +01, Frieder Schrempf <frieder@fris.de> wrote:

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Some SPI devices such as SPI NAND chips specify a clock-to-RX-sampling
> delay constraint, which means that for the data read from the device
> at a certain clock rate, we need to make sure that the point at
> which the data is sampled is correct.
>
> The default is to assume that the data can be sampled one half clock
> cycle after the triggering clock edge. For high clock rates, this
> can be too early.
>
> To check this we introduce a new core function spi_set_rx_sampling_point()
> and a handler set_rx_sampling_point() in the SPI controller.
>
> Controllers implementing set_rx_sampling_point() can calculate the
> sampling point delay using the helper spi_calc_rx_sampling_point()
> and store the value to set the appropriate registers during transfer.
>
> In case the controller capabilities are not sufficient to compensate
> the RX delay, spi_set_rx_sampling_point() returns a reduced clock
> rate value that is safe to use.
>
> This commit does not introduce generic logic for controllers that
> don't implement set_rx_sampling_point() in order to reduce the clock
> rate if the RX sampling delay requirement can not be met.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/spi/spi.c       | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |  8 ++++++
>  2 files changed, 81 insertions(+)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 61f7bde8c7fbb..b039007ed430f 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -3988,6 +3988,77 @@ static int spi_set_cs_timing(struct spi_device *spi)
>  	return status;
>  }
>  
> +/**
> + * spi_calc_rx_sampling_point - calculate RX sampling delay cycles
> + * @spi: the device that requires specific a RX sampling delay
> + * @freq: pointer to the clock frequency setpoint for the calculation. This gets
> + *        altered to a reduced value if required.
> + * @max_delay_cycles: the upper limit of supported delay cycles
> + * @delay_cycles_per_clock_cycle: the ratio between delay cycles and
> + *				  master clock cycles
> + *
> + * This function takes in the rx_sampling_delay_ns value from the SPI device
> + * and the given clock frequency setpoint and calculates the required sampling
> + * delay cycles to meet the device's spec. It uses the given controller
> + * constraints and if those are exceeded, it adjusts the clock frequency
> + * setpoint to a lower value that is safe to be used.
> + *
> + * Return: calculated number of delay cycles
> + */
> +unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
> +					u16 max_delay_cycles,
> +					u16 delay_cycles_per_clock_cycle)
> +{
> +	unsigned long long temp;
> +	u16 delay_cycles;
> +
> +	/* if sampling delay is zero, we assume the default sampling point can be used */
> +	if (!spi->rx_sampling_delay_ns)
> +		return 0;
> +
> +	temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
> +	do_div(temp, 1000000000UL);
> +	delay_cycles = temp;
> +
> +	if (delay_cycles > max_delay_cycles) {
> +		/*
> +		 * Reduce the clock to the point where the sampling delay requirement
> +		 * can be met.
> +		 */
> +		delay_cycles = max_delay_cycles;
> +		temp = (1000000000UL * delay_cycles);
> +		do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
> +		*freq = temp;

This is silently modifying the spi_device structure, I don't like this much.

> +	}
> +
> +	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);
> +
> +	return delay_cycles;
> +}
> +EXPORT_SYMBOL_GPL(spi_calc_rx_sampling_point);
> +
> +/**
> + * spi_set_rx_sampling_point - set the RX sampling delay in the controller driver
> + * @spi: the device that requires specific a RX sampling delay
> + * @freq: the clock frequency setpoint for the RX sampling delay calculation
> + *
> + * This function calls the set_rx_sampling_point() handle in the controller
> + * driver it is available. This makes sure that the controller uses the proper
> + * RX sampling point adjustment. This function should be called whenever
> + * the devices rx_sampling_delay_ns or the currently used clock frequency
> + * changes.
> + *
> + * Return: adjusted clock frequency
> + */
> +unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
> +{
> +	if (spi->controller->set_rx_sampling_point)
> +		return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
> +
> +	return freq;
> +}
> +EXPORT_SYMBOL_GPL(spi_set_rx_sampling_point);
> +
>  /**
>   * spi_setup - setup SPI mode and clock rate
>   * @spi: the device whose settings are being modified
> @@ -4090,6 +4161,8 @@ int spi_setup(struct spi_device *spi)
>  		}
>  	}
>  
> +	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);

I believe we need a clearer yet stronger logic around the setting of
max_speed_hz.

The "maximum speed" parameter is reaching its limit. This is clearly
what needs to be discussed with Santhosh. The SPI tuning series is
playing with this value as well.

Cheers,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Frieder Schrempf]

Hi Miquèl,

On 09.03.26 16:09, Miquel Raynal wrote:
> Hi Frieder,
> 
> First, thanks for the series! I believe it will conflict with the SPI
> tuning series as you mentioned, but we should be able to make them live
> side by side.

Ok, good! Thanks for your feedback!

> 
> On 03/03/2026 at 17:29:27 +01, Frieder Schrempf <frieder@fris.de> wrote:
> 
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> With clock rates changing on a per-op basis, we need to make sure
>> that we meet the RX sampling point delay constraint of the underlying
>> SPI chip.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  drivers/spi/spi-mem.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index a09371a075d2e..6b8bee7d6f5e3 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -589,6 +589,8 @@ void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
>>  {
>>  	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>>  		op->max_freq = mem->spi->max_speed_hz;
>> +
>> +	op->max_freq = spi_set_rx_sampling_point(mem->spi,
>> op->max_freq);
> 
> How can this work with the above 2 lines? Maybe there will be the need
> to use a "min of the max" frequencies?

The sampling point delay is coupled to the clock frequency. So if the
clock changes per-op, we also need to change the sampling delay per-op.

In general, if we want to avoid switching the parameters back and forth
in cases of alternating ops with different max frequencies, we should
maybe do some "min of the max" calculation and use the resulting
frequency for all ops.

Is that what you mean?

We could even set a threshold to decide between using a common "min of
the max" frequency or do the switching per-op.

One other possibility would be to somehow cache the per-op frequencies
and calculated sampling delay values so they can be reused when
switching without much overhead.

There is one more issue that is not yet covered by this series: Before
spinand_id_detect() we don't know yet what RX sampling delay value the
chip requires, but we already use read-status and read-id operations at
maximum chip clock.

For example on Winbond W25N04KV this leads to detection failures. So we
should maybe introduce some kind of reduced clock setpoint for the
initial detection, that is safe to be used without RX sampling delay
compensation.

Thanks
Frieder

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 4/7] mtd: spinand: toshiba: Add RX sampling delay values

[Frieder Schrempf]

On 09.03.26 16:12, Miquel Raynal wrote:
> 
>> @@ -277,7 +291,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>>  					      &update_cache_variants),
>>  		     0,
>>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
>> -				     tx58cxgxsxraix_ecc_get_status)),
>> +				     tx58cxgxsxraix_ecc_get_status),
>>  	/* 1.8V 4Gb (1st generation) */
>>  	SPINAND_INFO("TH58NYG2S3HBAI4",
>>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xAC),
>> @@ -288,7 +302,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>>  					      &update_cache_x4_variants),
>>  		     SPINAND_HAS_QE_BIT,
>>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
>> -				     tx58cxgxsxraix_ecc_get_status)),
>> +				     tx58cxgxsxraix_ecc_get_status),
>>  	/* 1.8V 8Gb (1st generation) */
>>  	SPINAND_INFO("TH58NYG3S0HBAI6",
>>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xA3),
>> @@ -299,7 +313,7 @@ static const struct spinand_info toshiba_spinand_table[] = {
>>  					      &update_cache_x4_variants),
>>  		     SPINAND_HAS_QE_BIT,
>>  		     SPINAND_ECCINFO(&tx58cxgxsxraix_ooblayout,
>> -				     tx58cxgxsxraix_ecc_get_status)),
>> +				     tx58cxgxsxraix_ecc_get_status),
>>  };
> 
> Are these 3 changes legitimate?

No, they are not. This is a last-minute fixup gone wrong.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 5/7] spi: Add RX sampling point adjustment

[Frieder Schrempf]

On 09.03.26 16:25, Miquel Raynal wrote:
> Hello,
> 
> + Santhosh
> 
> On 03/03/2026 at 17:29:26 +01, Frieder Schrempf <frieder@fris.de> wrote:
> 
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Some SPI devices such as SPI NAND chips specify a clock-to-RX-sampling
>> delay constraint, which means that for the data read from the device
>> at a certain clock rate, we need to make sure that the point at
>> which the data is sampled is correct.
>>
>> The default is to assume that the data can be sampled one half clock
>> cycle after the triggering clock edge. For high clock rates, this
>> can be too early.
>>
>> To check this we introduce a new core function spi_set_rx_sampling_point()
>> and a handler set_rx_sampling_point() in the SPI controller.
>>
>> Controllers implementing set_rx_sampling_point() can calculate the
>> sampling point delay using the helper spi_calc_rx_sampling_point()
>> and store the value to set the appropriate registers during transfer.
>>
>> In case the controller capabilities are not sufficient to compensate
>> the RX delay, spi_set_rx_sampling_point() returns a reduced clock
>> rate value that is safe to use.
>>
>> This commit does not introduce generic logic for controllers that
>> don't implement set_rx_sampling_point() in order to reduce the clock
>> rate if the RX sampling delay requirement can not be met.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  drivers/spi/spi.c       | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/spi/spi.h |  8 ++++++
>>  2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 61f7bde8c7fbb..b039007ed430f 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -3988,6 +3988,77 @@ static int spi_set_cs_timing(struct spi_device *spi)
>>  	return status;
>>  }
>>  
>> +/**
>> + * spi_calc_rx_sampling_point - calculate RX sampling delay cycles
>> + * @spi: the device that requires specific a RX sampling delay
>> + * @freq: pointer to the clock frequency setpoint for the calculation. This gets
>> + *        altered to a reduced value if required.
>> + * @max_delay_cycles: the upper limit of supported delay cycles
>> + * @delay_cycles_per_clock_cycle: the ratio between delay cycles and
>> + *				  master clock cycles
>> + *
>> + * This function takes in the rx_sampling_delay_ns value from the SPI device
>> + * and the given clock frequency setpoint and calculates the required sampling
>> + * delay cycles to meet the device's spec. It uses the given controller
>> + * constraints and if those are exceeded, it adjusts the clock frequency
>> + * setpoint to a lower value that is safe to be used.
>> + *
>> + * Return: calculated number of delay cycles
>> + */
>> +unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
>> +					u16 max_delay_cycles,
>> +					u16 delay_cycles_per_clock_cycle)
>> +{
>> +	unsigned long long temp;
>> +	u16 delay_cycles;
>> +
>> +	/* if sampling delay is zero, we assume the default sampling point can be used */
>> +	if (!spi->rx_sampling_delay_ns)
>> +		return 0;
>> +
>> +	temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
>> +	do_div(temp, 1000000000UL);
>> +	delay_cycles = temp;
>> +
>> +	if (delay_cycles > max_delay_cycles) {
>> +		/*
>> +		 * Reduce the clock to the point where the sampling delay requirement
>> +		 * can be met.
>> +		 */
>> +		delay_cycles = max_delay_cycles;
>> +		temp = (1000000000UL * delay_cycles);
>> +		do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
>> +		*freq = temp;
> 
> This is silently modifying the spi_device structure, I don't like this much.

Right, I agree. I think we will likely need some struct type to hold the
frequency and sampling delay values anyway. So maybe this function could
then return a value of such type.

> 
>> +	}
>> +
>> +	dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);
>> +
>> +	return delay_cycles;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_calc_rx_sampling_point);
>> +
>> +/**
>> + * spi_set_rx_sampling_point - set the RX sampling delay in the controller driver
>> + * @spi: the device that requires specific a RX sampling delay
>> + * @freq: the clock frequency setpoint for the RX sampling delay calculation
>> + *
>> + * This function calls the set_rx_sampling_point() handle in the controller
>> + * driver it is available. This makes sure that the controller uses the proper
>> + * RX sampling point adjustment. This function should be called whenever
>> + * the devices rx_sampling_delay_ns or the currently used clock frequency
>> + * changes.
>> + *
>> + * Return: adjusted clock frequency
>> + */
>> +unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
>> +{
>> +	if (spi->controller->set_rx_sampling_point)
>> +		return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
>> +
>> +	return freq;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_set_rx_sampling_point);
>> +
>>  /**
>>   * spi_setup - setup SPI mode and clock rate
>>   * @spi: the device whose settings are being modified
>> @@ -4090,6 +4161,8 @@ int spi_setup(struct spi_device *spi)
>>  		}
>>  	}
>>  
>> +	spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);
> 
> I believe we need a clearer yet stronger logic around the setting of
> max_speed_hz.
> 
> The "maximum speed" parameter is reaching its limit. This is clearly
> what needs to be discussed with Santhosh. The SPI tuning series is
> playing with this value as well.

I agree. I'm still missing parts of the bigger picture here, so I'm
currently not really sure how this might look like. But I'm open to any
discussions and suggestions.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Miquel Raynal]

Hi Frieder,

> The sampling point delay is coupled to the clock frequency. So if the
> clock changes per-op, we also need to change the sampling delay per-op.
>
> In general, if we want to avoid switching the parameters back and forth
> in cases of alternating ops with different max frequencies, we should
> maybe do some "min of the max" calculation and use the resulting
> frequency for all ops.
>
> Is that what you mean?

Not exactly, I am not afraid by the time it would take to switch, I am
afraid by the likelihood that both the PHY tuning series and your series
might want to force a different maximum frequency. For instance with TI
designs, when entering PHY mode, the frequency is 166MHz, period. You
cannot lower it because by design, you bypass the SPI divisors.

> We could even set a threshold to decide between using a common "min of
> the max" frequency or do the switching per-op.
>
> One other possibility would be to somehow cache the per-op frequencies
> and calculated sampling delay values so they can be reused when
> switching without much overhead.
>
> There is one more issue that is not yet covered by this series: Before
> spinand_id_detect() we don't know yet what RX sampling delay value the
> chip requires, but we already use read-status and read-id operations at
> maximum chip clock.

Not exactly "maximum chip clock" as in Santhosh's work, but indeed we
run these at the frequency given in the DT as bein the maximum
frequency. If that's not correct, you must lower it in the DT. That is
why I am in favour of having two maximum frequencies: the standard one,
that just works, which is the one for non optimized settings (the one we
actually use today) and another one, when tuning the bus timings.

> For example on Winbond W25N04KV this leads to detection failures. So we
> should maybe introduce some kind of reduced clock setpoint for the
> initial detection, that is safe to be used without RX sampling delay
> compensation.

That should be the DT max frequency, no?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Frieder Schrempf]

Hi Miquèl,

On 31.03.26 11:23, Miquel Raynal wrote:
> Hi Frieder,
> 
>> The sampling point delay is coupled to the clock frequency. So if the
>> clock changes per-op, we also need to change the sampling delay per-op.
>>
>> In general, if we want to avoid switching the parameters back and forth
>> in cases of alternating ops with different max frequencies, we should
>> maybe do some "min of the max" calculation and use the resulting
>> frequency for all ops.
>>
>> Is that what you mean?
> 
> Not exactly, I am not afraid by the time it would take to switch, I am
> afraid by the likelihood that both the PHY tuning series and your series
> might want to force a different maximum frequency. For instance with TI
> designs, when entering PHY mode, the frequency is 166MHz, period. You
> cannot lower it because by design, you bypass the SPI divisors.

Ah, ok. I didn't know the PHY mode bypasses the usual clock setup.

> 
>> We could even set a threshold to decide between using a common "min of
>> the max" frequency or do the switching per-op.
>>
>> One other possibility would be to somehow cache the per-op frequencies
>> and calculated sampling delay values so they can be reused when
>> switching without much overhead.
>>
>> There is one more issue that is not yet covered by this series: Before
>> spinand_id_detect() we don't know yet what RX sampling delay value the
>> chip requires, but we already use read-status and read-id operations at
>> maximum chip clock.
> 
> Not exactly "maximum chip clock" as in Santhosh's work, but indeed we
> run these at the frequency given in the DT as bein the maximum
> frequency. If that's not correct, you must lower it in the DT. That is
> why I am in favour of having two maximum frequencies: the standard one,
> that just works, which is the one for non optimized settings (the one we
> actually use today) and another one, when tuning the bus timings.
> 
>> For example on Winbond W25N04KV this leads to detection failures. So we
>> should maybe introduce some kind of reduced clock setpoint for the
>> initial detection, that is safe to be used without RX sampling delay
>> compensation.
> 
> That should be the DT max frequency, no?

Hm, I've seen it the other way round. In my perspective the DT max
frequency describes the maximum clock frequency supported by the chip as
given in the datasheet (assuming there are no other limitations
introduced by board design, etc.).

The RX sampling delay is a timing parameter specified in the datasheet
and it is relevant in order to interface the chip at maximum frequency
within spec.

Your approach would mean, that we need to manually calculate the maximum
supported clock frequency without RX sampling delay compensation and use
this value in the DT. Then let the driver increase the clock if RX
sampling delay compensation is available.

This would have the advantage, that even before initial detection we
would use the correct clock by default. But it has the downside that the
clock value in DT won't match the datasheet value.

In my approach we assume the driver is able to use the maximum clock
from the DT (same as in datasheet) by using RX sampling delay
compensation and only if it turns out the compensation is not possible
we will lower the clock dynamically.

What if we do the following?

1. Add a parameter in the SPI NAND chip driver that contains the maximum
supported clock frequency as given in the datasheet as this is clearly
the best place to keep this device-specific parameter.

2. Allow to leave spi-max-frequency in DT unset for SPI NANDs in case
there are no board-specific limitations and therefore the maximum
frequency supported by the combination of chip and controller can be used.

3. By default limit the clock frequency for the READ_ID op to a safe
value that works for all chips and controllers, even if RX sampling
delay compensations is not available.

4. In PHY mode, check against the DT max frequency (board limitations)
and chip max frequency before switching to this mode.

I hope this makes at least a bit of sense. I'm really not yet familiar
with all the different use-cases and limitations.

Best regards
Frieder

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Miquel Raynal]

Hello,

>>> For example on Winbond W25N04KV this leads to detection failures. So we
>>> should maybe introduce some kind of reduced clock setpoint for the
>>> initial detection, that is safe to be used without RX sampling delay
>>> compensation.
>> 
>> That should be the DT max frequency, no?
>
> Hm, I've seen it the other way round. In my perspective the DT max
> frequency describes the maximum clock frequency supported by the chip as
> given in the datasheet (assuming there are no other limitations
> introduced by board design, etc.).

I believe that what is in the datasheet should instead be statically
described in the SPI NAND vendor file.

The max SPI frequency DT property is here to tell at which speed to use
the device, and this depends mostly on the routing.

> The RX sampling delay is a timing parameter specified in the datasheet
> and it is relevant in order to interface the chip at maximum frequency
> within spec.

If it's not design dependent, then we should handle it "statically".

> Your approach would mean, that we need to manually calculate the maximum
> supported clock frequency without RX sampling delay compensation and use
> this value in the DT. Then let the driver increase the clock if RX
> sampling delay compensation is available.
>
> This would have the advantage, that even before initial detection we
> would use the correct clock by default. But it has the downside that the
> clock value in DT won't match the datasheet value.

It never does. Winbond chips can run at 166MHz. I know no design capable
of that natively (ie. without finer tuning, like what Santhosh
proposes).

> In my approach we assume the driver is able to use the maximum clock
> from the DT (same as in datasheet) by using RX sampling delay
> compensation and only if it turns out the compensation is not possible
> we will lower the clock dynamically.
>
> What if we do the following?
>
> 1. Add a parameter in the SPI NAND chip driver that contains the maximum
> supported clock frequency as given in the datasheet as this is clearly
> the best place to keep this device-specific parameter.

I've so far avoided doing it, but yes, this is something we can do. It
is going to be simple enough to implement as all the infrastructure is
already there. You can provide variants with speed limitations (look
into winbond.c). I was writing "0" in the fields which didn't need a
limitation that is something else than the maximum speed the chip can
sustain, as anyway the DT property *will* tell us what is this speed, if
we are ever able to reach it.

> 2. Allow to leave spi-max-frequency in DT unset for SPI NANDs in case
> there are no board-specific limitations and therefore the maximum
> frequency supported by the combination of chip and controller can be
> used.

In many cases, the limitations are board specific. Anyway, this is
already the case, you may just not put any value in this property.

> 3. By default limit the clock frequency for the READ_ID op to a safe
> value that works for all chips and controllers, even if RX sampling
> delay compensations is not available.

I am sorry this is not going to work out. There is no such harmonized
speed, differences can be an order (or two) of magnitude different, we
do not want to pay the penalty of a very slow identification on all
designs. We currently do the read several times with different
parameters. What if we were trying one more time by cutting the speed by
2 (completely random proposal)?

> 4. In PHY mode, check against the DT max frequency (board limitations)
> and chip max frequency before switching to this mode.

There is not such thing :-) the max frequency in DT currently would be
set to the platform limitation. We need somehow another parameter
indicating what is the maximum speed if we can fine tune the timings.

> I hope this makes at least a bit of sense. I'm really not yet familiar
> with all the different use-cases and limitations.

If I get back to your own issue. Is there any chance we could avoid
tweaking the DT for handling it? Would there be a configuration of your
controller that would allow reading the ID of all the chips with enough
delays? Currently, the spi core doesn't care much about other parameters
than the frequency, but there is perhaps room for improvement.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Frieder Schrempf]

On 31.03.26 17:26, Miquel Raynal wrote:
> Hello,
> 
>>>> For example on Winbond W25N04KV this leads to detection failures. So we
>>>> should maybe introduce some kind of reduced clock setpoint for the
>>>> initial detection, that is safe to be used without RX sampling delay
>>>> compensation.
>>>
>>> That should be the DT max frequency, no?
>>
>> Hm, I've seen it the other way round. In my perspective the DT max
>> frequency describes the maximum clock frequency supported by the chip as
>> given in the datasheet (assuming there are no other limitations
>> introduced by board design, etc.).
> 
> I believe that what is in the datasheet should instead be statically
> described in the SPI NAND vendor file.
> 
> The max SPI frequency DT property is here to tell at which speed to use
> the device, and this depends mostly on the routing.

Ok, agree.

> 
>> The RX sampling delay is a timing parameter specified in the datasheet
>> and it is relevant in order to interface the chip at maximum frequency
>> within spec.
> 
> If it's not design dependent, then we should handle it "statically".

The RX delay introduced by the chip is not design-dependent, only
chip-dependent. There might be additional delays introduced by the board
layout, but that's out of scope currently and I don't know if we even
need this.

> 
>> Your approach would mean, that we need to manually calculate the maximum
>> supported clock frequency without RX sampling delay compensation and use
>> this value in the DT. Then let the driver increase the clock if RX
>> sampling delay compensation is available.
>>
>> This would have the advantage, that even before initial detection we
>> would use the correct clock by default. But it has the downside that the
>> clock value in DT won't match the datasheet value.
> 
> It never does. Winbond chips can run at 166MHz. I know no design capable
> of that natively (ie. without finer tuning, like what Santhosh
> proposes).

Ok, right.

> 
>> In my approach we assume the driver is able to use the maximum clock
>> from the DT (same as in datasheet) by using RX sampling delay
>> compensation and only if it turns out the compensation is not possible
>> we will lower the clock dynamically.
>>
>> What if we do the following?
>>
>> 1. Add a parameter in the SPI NAND chip driver that contains the maximum
>> supported clock frequency as given in the datasheet as this is clearly
>> the best place to keep this device-specific parameter.
> 
> I've so far avoided doing it, but yes, this is something we can do. It
> is going to be simple enough to implement as all the infrastructure is
> already there. You can provide variants with speed limitations (look
> into winbond.c). I was writing "0" in the fields which didn't need a
> limitation that is something else than the maximum speed the chip can
> sustain, as anyway the DT property *will* tell us what is this speed, if
> we are ever able to reach it.

Ok.

> 
>> 2. Allow to leave spi-max-frequency in DT unset for SPI NANDs in case
>> there are no board-specific limitations and therefore the maximum
>> frequency supported by the combination of chip and controller can be
>> used.
> 
> In many cases, the limitations are board specific. Anyway, this is
> already the case, you may just not put any value in this property.

Ok.

> 
>> 3. By default limit the clock frequency for the READ_ID op to a safe
>> value that works for all chips and controllers, even if RX sampling
>> delay compensations is not available.
> 
> I am sorry this is not going to work out. There is no such harmonized
> speed, differences can be an order (or two) of magnitude different, we
> do not want to pay the penalty of a very slow identification on all
> designs. We currently do the read several times with different
> parameters. What if we were trying one more time by cutting the speed by
> 2 (completely random proposal)?

If we try with reduced clock as a fallback after the other attempts,
there would be a slight chance of one of the earlier attempts with
normal clock rate returning some invalid data that matches an existing
chip ID, no? Isn't this a bit flaky?

Let's say for a worst case scenario a chip has an RX delay of 20ns (the
highest I've seen in datasheets so far is 8ns). In that case the maximum
clock we could safely use for reading the ID would be 1/(2*20e-9) =
25MHz. Do you think it really makes much of a difference if we read the
ID (only a handful of bytes) at 25MHz or full speed (e. g. 104 MHz)? I
mean this should be fast enough either way, no? But maybe I'm misjudging
this.

> 
>> 4. In PHY mode, check against the DT max frequency (board limitations)
>> and chip max frequency before switching to this mode.
> 
> There is not such thing :-) the max frequency in DT currently would be
> set to the platform limitation. We need somehow another parameter
> indicating what is the maximum speed if we can fine tune the timings.

So we can exceed the platform limitations using fine-tuned timings? I
guess I need to read and understand that tuning RFC series.

> 
>> I hope this makes at least a bit of sense. I'm really not yet familiar
>> with all the different use-cases and limitations.
> 
> If I get back to your own issue. Is there any chance we could avoid
> tweaking the DT for handling it? Would there be a configuration of your
> controller that would allow reading the ID of all the chips with enough
> delays? Currently, the spi core doesn't care much about other parameters
> than the frequency, but there is perhaps room for improvement.

We could use RX delay compensation (one half clock cycle) in the
controller by default (as currently done by the STM32 driver). But that
would break if we use a very low clock (for whatever reason) because
then the RX sampling delay gets neglectable compared to the clock period
and sampling is triggered at the very end of the clock cycle when the
data might already be invalid. At least that's what I suspect based on
my test results.
And it would also break if the chip actually requires more than one half
clock cycle of RX sampling delay.

The RX sampling delay setting must match the used clock frequency. In
most cases there is a lot of room for tolerance before things start to
fail, but it's not endless and especially at very high clock rates we
need to take it into account.

So if we don't cap the speed for reading the ID by default, we somehow
need to know what value to use for the RX sampling delay or we need to
try different values. And if the controller does not support or
implement the RX sampling delay compensation, we need to cap the speed
anyway.


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 868 bytes --]

On Tue, Mar 31, 2026 at 07:57:21PM +0200, Frieder Schrempf wrote:
> On 31.03.26 17:26, Miquel Raynal wrote:

> > I am sorry this is not going to work out. There is no such harmonized
> > speed, differences can be an order (or two) of magnitude different, we
> > do not want to pay the penalty of a very slow identification on all
> > designs. We currently do the read several times with different
> > parameters. What if we were trying one more time by cutting the speed by
> > 2 (completely random proposal)?

> If we try with reduced clock as a fallback after the other attempts,
> there would be a slight chance of one of the earlier attempts with
> normal clock rate returning some invalid data that matches an existing
> chip ID, no? Isn't this a bit flaky?

I'd worry about single bit errors causing confusion between related
parts from a single manufacturer...

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Miquel Raynal]

Hi Frieder,

> The RX delay introduced by the chip is not design-dependent, only
> chip-dependent. There might be additional delays introduced by the board
> layout, but that's out of scope currently and I don't know if we even
> need this.

We do, for very high speeds, and this is the purpose of the PHY tuning
series from Santhosh.

[...]

>>> 3. By default limit the clock frequency for the READ_ID op to a safe
>>> value that works for all chips and controllers, even if RX sampling
>>> delay compensations is not available.
>> 
>> I am sorry this is not going to work out. There is no such harmonized
>> speed, differences can be an order (or two) of magnitude different, we
>> do not want to pay the penalty of a very slow identification on all
>> designs. We currently do the read several times with different
>> parameters. What if we were trying one more time by cutting the speed by
>> 2 (completely random proposal)?
>
> If we try with reduced clock as a fallback after the other attempts,
> there would be a slight chance of one of the earlier attempts with
> normal clock rate returning some invalid data that matches an existing
> chip ID, no? Isn't this a bit flaky?

Yeah, I understand your (and Mark's) concern.

> Let's say for a worst case scenario a chip has an RX delay of 20ns (the
> highest I've seen in datasheets so far is 8ns). In that case the maximum
> clock we could safely use for reading the ID would be 1/(2*20e-9) =
> 25MHz. Do you think it really makes much of a difference if we read the
> ID (only a handful of bytes) at 25MHz or full speed (e. g. 104 MHz)? I
> mean this should be fast enough either way, no? But maybe I'm misjudging
> this.

I am honestly not a big fan of the global penalty, but I am not totally
opposed either, especially since you just said you only observed 8ns
delays at most. This is 62.5MHz, which is already above what most
designs use, so the penalty would be minimal. What about taking this
approach and see if that fixes most of our use cases?

>>> 4. In PHY mode, check against the DT max frequency (board limitations)
>>> and chip max frequency before switching to this mode.
>> 
>> There is not such thing :-) the max frequency in DT currently would be
>> set to the platform limitation. We need somehow another parameter
>> indicating what is the maximum speed if we can fine tune the timings.
>
> So we can exceed the platform limitations using fine-tuned timings? I
> guess I need to read and understand that tuning RFC series.

Well, this is still under discussion. As of today, the max speed in DT
is the maximum speed. But it is in most cases the maximum speed
*without* tuning, which means if we tune the PHY delays we can increase
the speed further. I already proposed to turn this DT property into an
array, in order to indicate what are the maximum speed*s*, without and
with tuning, if ever possible.

>>> I hope this makes at least a bit of sense. I'm really not yet familiar
>>> with all the different use-cases and limitations.
>> 
>> If I get back to your own issue. Is there any chance we could avoid
>> tweaking the DT for handling it? Would there be a configuration of your
>> controller that would allow reading the ID of all the chips with enough
>> delays? Currently, the spi core doesn't care much about other parameters
>> than the frequency, but there is perhaps room for improvement.
>
> We could use RX delay compensation (one half clock cycle) in the
> controller by default (as currently done by the STM32 driver). But that
> would break if we use a very low clock (for whatever reason) because
> then the RX sampling delay gets neglectable compared to the clock period
> and sampling is triggered at the very end of the clock cycle when the
> data might already be invalid. At least that's what I suspect based on
> my test results.
> And it would also break if the chip actually requires more than one half
> clock cycle of RX sampling delay.
>
> The RX sampling delay setting must match the used clock frequency. In
> most cases there is a lot of room for tolerance before things start to
> fail, but it's not endless and especially at very high clock rates we
> need to take it into account.
>
> So if we don't cap the speed for reading the ID by default, we somehow
> need to know what value to use for the RX sampling delay or we need to
> try different values. And if the controller does not support or
> implement the RX sampling delay compensation, we need to cap the speed
> anyway.

What about:

*SPI controller drivers*
- Enable RX delay compensation in the controller by default, disable it
if the speed is very low, ie. compensation is dangerous
*Core*
- READID at max 65MHz (with the default RX delay compensation)
*Vendor driver*
- Indicate the internal RX delay per chip (maybe we need some heuristics
depending on the speed as well?)
*SPI controller drivers*
- Hook to tune RX based on the fastest reachable speed.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Michael Walle]

[-- Attachment #1.1: Type: text/plain, Size: 1159 bytes --]

On Wed Apr 1, 2026 at 10:32 AM CEST, Miquel Raynal wrote:
>> Let's say for a worst case scenario a chip has an RX delay of 20ns (the
>> highest I've seen in datasheets so far is 8ns). In that case the maximum
>> clock we could safely use for reading the ID would be 1/(2*20e-9) =
>> 25MHz. Do you think it really makes much of a difference if we read the
>> ID (only a handful of bytes) at 25MHz or full speed (e. g. 104 MHz)? I
>> mean this should be fast enough either way, no? But maybe I'm misjudging
>> this.
>
> I am honestly not a big fan of the global penalty, but I am not totally
> opposed either, especially since you just said you only observed 8ns
> delays at most. This is 62.5MHz, which is already above what most
> designs use, so the penalty would be minimal. What about taking this
> approach and see if that fixes most of our use cases?

What are the actual numbers we are talking about here? I mean, at
least for SPI NOR, we only read the ID *once*. And it takes about 56
bits (command + id length of 6). That is about 2us at 25MHz. I'd
guess the setup and the software handling takes far longer than
that.

-michael

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/