[PATCH 0/4] Add tCLQV parameter to tweak SPI timings

[PATCH 0/4] Add tCLQV parameter to tweak SPI timings

[Eberhard Stoll]

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

Hi All,

this patch series adds parameters to tweak SPI timings for some
(Q)SPI devices. For example it optimizes the support for the operation
of a Winbond W25N02KV SPI NAND chip on NXP i.mx6 ul(l) devices.

The Winbond W25N02KV SPI NAND has the characteristic to introduce a
delay between spi clock and the masters spi receive data (called tCLQV
value in data sheet).

This chip can operate with a maximum SPI clock of 104MHz. Disregarding
the requred tCLQV value of 7ns for this chip reduces the possible spi
clock to approx. 70MHz. To support the full bandwith of this chip, the
tCLQV parameter has to be supported in SPI framework and SPI controller
and the SPI NAND chip needs this parameter in the device configuration
data.

Also other devices can improve their operating SPI clock performance
with this setting if they also has significant tCLQV values and the SPI
controller will support it.

This patch series adds support the tCLQV parameter in:

- SPI framework
- SPI NAND framework
- NXP i.mx6 QSPI controller
- Winbond W25N02KV SPI NAND device


Eberhard Stoll (4):
  spi: Add parameter for clock to rx delay
  mtd: spinand: Add support for clock to rx delay setting
  mtd: spinand: winbond: Add rx sample delay for W25N02KV
  spi: spi-fsl-qspi: Add support for rx data sample point adjustment

 drivers/mtd/nand/spi/core.c    |  2 +
 drivers/mtd/nand/spi/winbond.c |  3 +-
 drivers/spi/spi-fsl-qspi.c     | 80 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h    |  5 +++
 include/linux/spi/spi.h        |  3 ++
 5 files changed, 92 insertions(+), 1 deletion(-)


base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
--
2.25.1

[PATCH 1/4] spi: Add parameter for clock to rx delay

[Eberhard Stoll]

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

For spi devices the master clock output defines the sampling point
for receive data input stream (rising or falling edge). The receive
data stream from the device is delayed in relation to the master
clock output.

For some devices this delay is larger than one half clock period,
which is normally the sampling point for receive data. In this case
receive data is sampled too early and the device fails to operate.
In consequence the spi clock has to be reduced to match the delay
characteristics and this reduces performance and is therefore not
recommended.

Some spi controllers implement a 'clock to receive data delay'
compensation which shifts the receive sampling point. So we need
a property to set this value for each spi device.

Add a parameter 'rx_sample_delay_ns' to enable setting the clock
to rx data delay for each spi device separately.

The 'clock to receive data delay' value is often referenced as tCLQV
in spi device data sheets.

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 7f8b478fdeb3..14622d47f44f 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -166,6 +166,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg,
  * @cs_inactive: delay to be introduced by the controller after CS is
  *	deasserted. If @cs_change_delay is used from @spi_transfer, then the
  *	two delays will be added up.
+ * @rx_sample_delay_ns: spi clk to spi rx data delay
  * @pcpu_statistics: statistics for the spi_device
  *
  * A @spi_device is used to interchange data between an SPI slave
@@ -219,6 +220,8 @@ struct spi_device {
 	struct spi_delay	cs_setup;
 	struct spi_delay	cs_hold;
 	struct spi_delay	cs_inactive;
+	/* Transfer characteristics */
+	u32			rx_sample_delay_ns; /* Clock to RX data delay */

 	/* The statistics */
 	struct spi_statistics __percpu	*pcpu_statistics;
--
2.25.1

[PATCH 2/4] mtd: spinand: Add support for clock to rx delay setting

[Eberhard Stoll]

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 | 2 ++
 include/linux/mtd/spinand.h | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 393ff37f0d23..e546b0d1f76f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1113,6 +1113,8 @@ int spinand_match_and_init(struct spinand_device *spinand,
 					       info->op_variants.update_cache);
 		spinand->op_templates.update_cache = op;

+		spinand->spimem->spi->rx_sample_delay_ns = table[i].rx_sample_delay_ns;
+
 		return 0;
 	}

diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 3e285c09d16d..fe34d5259c97 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -326,6 +326,7 @@ struct spinand_ondie_ecc_conf {
  * @model: model name
  * @devid: device ID
  * @flags: OR-ing of the SPINAND_XXX flags
+ * @rx_sample_delay_ns: clock to rx data delay timing (tCLQV)
  * @memorg: memory organization
  * @eccreq: ECC requirements
  * @eccinfo: on-die ECC info
@@ -343,6 +344,7 @@ struct spinand_info {
 	const char *model;
 	struct spinand_devid devid;
 	u32 flags;
+	u32 rx_sample_delay_ns;
 	struct nand_memory_organization memorg;
 	struct nand_ecc_props eccreq;
 	struct spinand_ecc_info eccinfo;
@@ -378,6 +380,9 @@ struct spinand_info {
 #define SPINAND_SELECT_TARGET(__func)					\
 	.select_target = __func,

+#define SPINAND_RX_SAMPLE_DELAY(__delay)				\
+	.rx_sample_delay_ns = __delay,
+
 #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants,	\
 		     __flags, ...)					\
 	{								\
--
2.25.1

[PATCH 3/4] mtd: spinand: winbond: Add rx sample delay for W25N02KV

[Eberhard Stoll]

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

Add tCLQV (Clock Low to Output Valid) parameter for Winbond W25N02KV
SPI NAND chip.

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

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index f507e3759301..c0b05f5cbf1b 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -168,7 +168,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_SAMPLE_DELAY(7)),
 };

 static int winbond_spinand_init(struct spinand_device *spinand)
--
2.25.1

[PATCH 4/4] spi: spi-fsl-qspi: Add support for rx data sample point adjustment

[Eberhard Stoll]

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

This qspi controller supports shifting the spi rx data sampling point to
compensate line or spi device response delays. It enables fast spi data
transfers even for devices which have a noticeable delay in the rx data
stream.

Add support for the SMPR sampling functionality

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 79bac30e79af..68801e08f997 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -274,6 +274,12 @@ struct fsl_qspi {
 	int selected;
 };

+struct fsl_qspi_chip_data {
+	u32 rx_sample_delay_ns;
+	unsigned long rate;
+	u32 smpr_sampling;
+};
+
 static inline int needs_swap_endian(struct fsl_qspi *q)
 {
 	return q->devtype_data->quirks & QUADSPI_QUIRK_SWAP_ENDIAN;
@@ -522,14 +528,63 @@ static void fsl_qspi_invalidate(struct fsl_qspi *q)
 	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
 }

+static void fsl_qspi_update_smpr_sampling(struct fsl_qspi *q, u32 smpr)
+{
+	void __iomem *base = q->iobase;
+	u32 reg;
+
+	/* Disable the module */
+	qspi_writel(q, QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
+		    base + QUADSPI_MCR);
+
+	reg = qspi_readl(q, base + QUADSPI_SMPR) &
+	      ~(QUADSPI_SMPR_FSPHS_MASK | QUADSPI_SMPR_FSDLY_MASK);
+	qspi_writel(q, reg | smpr, base + QUADSPI_SMPR);
+
+	/* Enable the module */
+	qspi_writel(q, QUADSPI_MCR_RESERVED_MASK | QUADSPI_MCR_END_CFG_MASK,
+		    base + QUADSPI_MCR);
+}
+
+const char *sampling_mode[] = { "N1", "I1", "N2", "I2"};
+
 static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi)
 {
 	unsigned long rate = spi->max_speed_hz;
 	int ret;
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+	const char *sampling_ident = sampling_mode[0];
+
+	if (chip->rx_sample_delay_ns != spi->rx_sample_delay_ns |
+	    chip->rate != rate) {
+		chip->rx_sample_delay_ns = spi->rx_sample_delay_ns;
+		chip->rate = rate;
+
+		chip->smpr_sampling =
+			(2 * spi->rx_sample_delay_ns * (rate >> 10)) / (1000000000 >> 10);
+		dev_dbg(q->dev, "smpr_sampling = %u (delay %u ns)\n",
+			chip->smpr_sampling, spi->rx_sample_delay_ns);
+
+		if (chip->smpr_sampling > 3) {
+			dev_err(q->dev, "rx sample delay for device %s exceeds hw capabilities! Clamp value to maximum setting.\n",
+				dev_name(&spi->dev));
+			chip->smpr_sampling = 3;
+			sampling_ident = "(I2 clamped to max)";
+		} else {
+			sampling_ident = sampling_mode[chip->smpr_sampling];
+		}
+
+		chip->smpr_sampling <<= 5;
+		dev_info(q->dev, "sampling point %s at %lu kHz used for device %s\n",
+			 sampling_ident, rate / 1000, dev_name(&spi->dev));
+		fsl_qspi_update_smpr_sampling(q, chip->smpr_sampling);
+	}

 	if (q->selected == spi_get_chipselect(spi, 0))
 		return;

+	fsl_qspi_update_smpr_sampling(q, chip->smpr_sampling);
+
 	if (needs_4x_clock(q))
 		rate *= 4;

@@ -839,6 +894,28 @@ static const struct spi_controller_mem_ops fsl_qspi_mem_ops = {
 	.get_name = fsl_qspi_get_name,
 };

+static int fsl_qspi_setup(struct spi_device *spi)
+{
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+
+	if (!chip) {
+		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+		if (!chip)
+			return -ENOMEM;
+		spi_set_ctldata(spi, chip);
+	}
+
+	return 0;
+}
+
+static void fsl_qspi_cleanup(struct spi_device *spi)
+{
+	struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
+
+	kfree(chip);
+	spi_set_ctldata(spi, NULL);
+}
+
 static int fsl_qspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -865,6 +942,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)

 	platform_set_drvdata(pdev, q);

+	ctlr->setup = fsl_qspi_setup;
+	ctlr->cleanup = fsl_qspi_cleanup;
+
 	/* find the resources */
 	q->iobase = devm_platform_ioremap_resource_byname(pdev, "QuadSPI");
 	if (IS_ERR(q->iobase)) {
--
2.25.1

Re: [PATCH 4/4] spi: spi-fsl-qspi: Add support for rx data sample point adjustment

[kernel test robot]

Hi Eberhard,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1]

url:    https://github.com/intel-lab-lkp/linux/commits/Eberhard-Stoll/spi-Add-parameter-for-clock-to-rx-delay/20231026-232547
base:   05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
patch link:    https://lore.kernel.org/r/20231026152316.2729575-5-estl%40gmx.net
patch subject: [PATCH 4/4] spi: spi-fsl-qspi: Add support for rx data sample point adjustment
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231027/202310270332.mcbckKCr-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231027/202310270332.mcbckKCr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310270332.mcbckKCr-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/spi/spi-fsl-qspi.c: In function 'fsl_qspi_select_mem':
>> drivers/spi/spi-fsl-qspi.c:558:38: warning: suggest parentheses around comparison in operand of '|' [-Wparentheses]
     558 |         if (chip->rx_sample_delay_ns != spi->rx_sample_delay_ns |
         |             ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~


vim +558 drivers/spi/spi-fsl-qspi.c

   550	
   551	static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi)
   552	{
   553		unsigned long rate = spi->max_speed_hz;
   554		int ret;
   555		struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
   556		const char *sampling_ident = sampling_mode[0];
   557	
 > 558		if (chip->rx_sample_delay_ns != spi->rx_sample_delay_ns |
   559		    chip->rate != rate) {
   560			chip->rx_sample_delay_ns = spi->rx_sample_delay_ns;
   561			chip->rate = rate;
   562	
   563			chip->smpr_sampling =
   564				(2 * spi->rx_sample_delay_ns * (rate >> 10)) / (1000000000 >> 10);
   565			dev_dbg(q->dev, "smpr_sampling = %u (delay %u ns)\n",
   566				chip->smpr_sampling, spi->rx_sample_delay_ns);
   567	
   568			if (chip->smpr_sampling > 3) {
   569				dev_err(q->dev, "rx sample delay for device %s exceeds hw capabilities! Clamp value to maximum setting.\n",
   570					dev_name(&spi->dev));
   571				chip->smpr_sampling = 3;
   572				sampling_ident = "(I2 clamped to max)";
   573			} else {
   574				sampling_ident = sampling_mode[chip->smpr_sampling];
   575			}
   576	
   577			chip->smpr_sampling <<= 5;
   578			dev_info(q->dev, "sampling point %s at %lu kHz used for device %s\n",
   579				 sampling_ident, rate / 1000, dev_name(&spi->dev));
   580			fsl_qspi_update_smpr_sampling(q, chip->smpr_sampling);
   581		}
   582	
   583		if (q->selected == spi_get_chipselect(spi, 0))
   584			return;
   585	
   586		fsl_qspi_update_smpr_sampling(q, chip->smpr_sampling);
   587	
   588		if (needs_4x_clock(q))
   589			rate *= 4;
   590	
   591		fsl_qspi_clk_disable_unprep(q);
   592	
   593		ret = clk_set_rate(q->clk, rate);
   594		if (ret)
   595			return;
   596	
   597		ret = fsl_qspi_clk_prep_enable(q);
   598		if (ret)
   599			return;
   600	
   601		q->selected = spi_get_chipselect(spi, 0);
   602	
   603		fsl_qspi_invalidate(q);
   604	}
   605	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] spi: Add parameter for clock to rx delay

[Miquel Raynal]

Hello,

estl@gmx.net wrote on Thu, 26 Oct 2023 17:23:02 +0200:

> From: Eberhard Stoll <eberhard.stoll@kontron.de>
> 
> For spi devices the master clock output defines the sampling point
> for receive data input stream (rising or falling edge). The receive
> data stream from the device is delayed in relation to the master
> clock output.
> 
> For some devices this delay is larger than one half clock period,

Can you be more specific? I am wondering how big the need is.

> which is normally the sampling point for receive data. In this case
> receive data is sampled too early and the device fails to operate.
> In consequence the spi clock has to be reduced to match the delay
> characteristics and this reduces performance and is therefore not
> recommended.
> 
> Some spi controllers implement a 'clock to receive data delay'
> compensation which shifts the receive sampling point. So we need
> a property to set this value for each spi device.

What if the spi controller does not support this feature? Shall we add
a capability? Shall we refuse to probe if the controller is not capable
of sampling at the right moment?

> Add a parameter 'rx_sample_delay_ns' to enable setting the clock
> to rx data delay for each spi device separately.
> 
> The 'clock to receive data delay' value is often referenced as tCLQV
> in spi device data sheets.
> 
> 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 7f8b478fdeb3..14622d47f44f 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -166,6 +166,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg,
>   * @cs_inactive: delay to be introduced by the controller after CS is
>   *	deasserted. If @cs_change_delay is used from @spi_transfer, then the
>   *	two delays will be added up.
> + * @rx_sample_delay_ns: spi clk to spi rx data delay
>   * @pcpu_statistics: statistics for the spi_device
>   *
>   * A @spi_device is used to interchange data between an SPI slave
> @@ -219,6 +220,8 @@ struct spi_device {
>  	struct spi_delay	cs_setup;
>  	struct spi_delay	cs_hold;
>  	struct spi_delay	cs_inactive;
> +	/* Transfer characteristics */
> +	u32			rx_sample_delay_ns; /* Clock to RX data delay */
> 
>  	/* The statistics */
>  	struct spi_statistics __percpu	*pcpu_statistics;
> --
> 2.25.1
> 


Thanks,
Miquèl

Re: [PATCH 4/4] spi: spi-fsl-qspi: Add support for rx data sample point adjustment

[Frieder Schrempf]

On 26.10.23 22:03, kernel test robot wrote:
> Hi Eberhard,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Eberhard-Stoll/spi-Add-parameter-for-clock-to-rx-delay/20231026-232547
> base:   05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
> patch link:    https://lore.kernel.org/r/20231026152316.2729575-5-estl%40gmx.net
> patch subject: [PATCH 4/4] spi: spi-fsl-qspi: Add support for rx data sample point adjustment
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231027/202310270332.mcbckKCr-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231027/202310270332.mcbckKCr-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310270332.mcbckKCr-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/spi/spi-fsl-qspi.c: In function 'fsl_qspi_select_mem':
>>> drivers/spi/spi-fsl-qspi.c:558:38: warning: suggest parentheses around comparison in operand of '|' [-Wparentheses]
>      558 |         if (chip->rx_sample_delay_ns != spi->rx_sample_delay_ns |
>          |             ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~

IIRC, when I prepared the patches for sending "checkpatch.pl --strict"
suggested to remove the parentheses here. Seems a bit inconsistent...

AW: [PATCH 1/4] spi: Add parameter for clock to rx delay

[Stoll, Eberhard]

Hello,

> > For spi devices the master clock output defines the sampling point
> > for receive data input stream (rising or falling edge). The receive
> > data stream from the device is delayed in relation to the master
> > clock output.
> >
> > For some devices this delay is larger than one half clock period,
> 
> Can you be more specific? I am wondering how big the need is.

In our case it's a QSPI NAND chip (Winbond W25N02KV). This device
can operate at 104MHz SPI clock. But it also has a tCLQV value of 7ns.
The tCLQV value limits the SPI clock speed for this device to 2x7ns
(if it is not adjustable in the SPI controller) which is approximately
70MHz.

Without the ability to set the tCLQV value, the SPI clock has to be
limited to 70MHz in device tree for this bus.

In our case the Winbond W25N02KV chip is a replacement of an
older chip. The older chip can operate at 104MHz and does not
have the tCLQV restrictions as this new one.
The new chip is mostly is better than the data sheet and meet the
timing requirements for 104MHz. But on higher temperatures
devices fail.

In device tree QSPI NAND chips are configured by a compatible
property of 'spi-nand'. The mtd layer detects the real device
and fetches the properties of this device from the appropriate
driver.

So for our case (boards containing the old and new chip) we well
have to reduce the SPI clock for the entire QSPI bus to 70MHz, even
for the elder chips which can operate well also with 104MHz. 

> 
> > which is normally the sampling point for receive data. In this case
> > receive data is sampled too early and the device fails to operate.
> > In consequence the spi clock has to be reduced to match the delay
> > characteristics and this reduces performance and is therefore not
> > recommended.
> >
> > Some spi controllers implement a 'clock to receive data delay'
> > compensation which shifts the receive sampling point. So we need
> > a property to set this value for each spi device.
> 
> What if the spi controller does not support this feature? Shall we add
> a capability? Shall we refuse to probe if the controller is not capable
> of sampling at the right moment?
> 

Refuse to probe is not necessary IMHO. The device can operate well
even with controllers which do not implement the tCLQV functionality.
The SPI clock has simply to be reduced and all works well. In this case
not the maximum SPI clock frequency of the device limits the SPI bus
clock, but the tCLQV value!

IMHO it's the responsibility of the writer of the device tree configuration.

For SPI controllers which do not support this setting, the SPI framework
could check whether the max SPI clock frequency of the device or the
tCLQV value limits the SPI bus speed and adjust it appropriately.

For our case this seems a little bit of overkill.

With 'discoverable' devices on the SPI bus like SPI NAND chips, the
'max_speed_hz' in 'struct spi_device' is no more really device specific,
but more like chip select specific. The real chips 'max_speed_hz' data
sheet value could then e.g. be propagated from the discovered chips SPI
device driver to the frameworks  'chip select specific'  'max_speed_hz'
property. We could introduce a 'probe_speed_hz' setting and maybe
many other things ...

... but IMHO this would be far too much of overkill (at least currently) ...

Thanks,
Eberhard

Re: AW: [PATCH 1/4] spi: Add parameter for clock to rx delay

[Andy Shevchenko]

On Fri, Oct 27, 2023 at 08:38:54AM +0000, Stoll, Eberhard wrote:

...

> > Can you be more specific? I am wondering how big the need is.
> 
> In our case it's a QSPI NAND chip (Winbond W25N02KV). This device
> can operate at 104MHz SPI clock. But it also has a tCLQV value of 7ns.
> The tCLQV value limits the SPI clock speed for this device to 2x7ns
> (if it is not adjustable in the SPI controller) which is approximately
> 70MHz.
> 
> Without the ability to set the tCLQV value, the SPI clock has to be
> limited to 70MHz in device tree for this bus.
> 
> In our case the Winbond W25N02KV chip is a replacement of an
> older chip. The older chip can operate at 104MHz and does not
> have the tCLQV restrictions as this new one.
> The new chip is mostly is better than the data sheet and meet the
> timing requirements for 104MHz. But on higher temperatures
> devices fail.
> 
> In device tree QSPI NAND chips are configured by a compatible
> property of 'spi-nand'. The mtd layer detects the real device
> and fetches the properties of this device from the appropriate
> driver.
> 
> So for our case (boards containing the old and new chip) we well
> have to reduce the SPI clock for the entire QSPI bus to 70MHz, even
> for the elder chips which can operate well also with 104MHz. 

So, to me sounds like device tree source issue. I.e. you need to provide
different DT(b)s depending on the platform (and how it should be).
The cleanest solution (as I see not the first time people I trying quirks like
this to be part of the subsystems / drivers) is to make DT core (OF) to have
conditionals or boot-time modifications allowed.

This, what you are doing, does not scale and smells like an ugly hack.

-- 
With Best Regards,
Andy Shevchenko

Re: AW: [PATCH 1/4] spi: Add parameter for clock to rx delay

[Andy Shevchenko]

On Fri, Oct 27, 2023 at 04:45:25PM +0100, Mark Brown wrote:
> On Fri, Oct 27, 2023 at 02:46:42PM +0300, Andy Shevchenko wrote:
> 
> > So, to me sounds like device tree source issue. I.e. you need to provide
> > different DT(b)s depending on the platform (and how it should be).
> > The cleanest solution (as I see not the first time people I trying quirks like
> > this to be part of the subsystems / drivers) is to make DT core (OF) to have
> > conditionals or boot-time modifications allowed.
> 
> > This, what you are doing, does not scale and smells like an ugly hack.
> 
> No, this seems like an entirely reasonable thing to have - it's just a
> property of the device, we don't need to add a DT property for it, and
> the maximum speed that the device can run at is going to vary depending
> on the ability of the controller to control the sampling point.
> 
> As people have been saying there's a particularly clear case for this
> with SPI flash which is probed at runtime and is readily substituted at
> the hardware level.

So, then the question is what does DT _actually_ describes?
If we have an autoprobe of something that doesn't work on platform A and works
on platform B, shouldn't these platforms have to have distinguishable DTs?

-- 
With Best Regards,
Andy Shevchenko

AW: AW: [PATCH 1/4] spi: Add parameter for clock to rx delay

[Stoll, Eberhard]

> So, then the question is what does DT _actually_ describes?
> If we have an autoprobe of something that doesn't work on platform A and
> works
> on platform B, shouldn't these platforms have to have distinguishable DTs?

Yes it's one possibility to deal with it.
But I think the first choice should be to improve the autoprobe function to work
properly on all platforms. This offers the most advantage for all. If this doesn't
work, the DT approach should be the fallback choice.

Improving the autoprobe function in this way seems realistic. So it's currently the
way we should go.

Kind regards
Eberhard