[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/