diff mbox series

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

Message ID 20231026152316.2729575-2-estl@gmx.net (mailing list archive)
State New, archived
Headers show
Series Add tCLQV parameter to tweak SPI timings | expand

Commit Message

Eberhard Stoll Oct. 26, 2023, 3:23 p.m. UTC
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(+)

--
2.25.1

Comments

Miquel Raynal Oct. 26, 2023, 10:56 p.m. UTC | #1
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
Stoll, Eberhard Oct. 27, 2023, 8:38 a.m. UTC | #2
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
Andy Shevchenko Oct. 27, 2023, 11:46 a.m. UTC | #3
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.
Andy Shevchenko Oct. 30, 2023, 8:48 a.m. UTC | #4
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?
Stoll, Eberhard Oct. 30, 2023, 9:28 a.m. UTC | #5
> 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
diff mbox series

Patch

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;