| 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 |
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
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
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.
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?
> 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 --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;