Hello,
[email protected] wrote on Thu, 26 Oct 2023 17:23:02 +0200:
> From: Eberhard Stoll <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Frieder Schrempf <[email protected]>
> ---
> 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.
--
With Best Regards,
Andy Shevchenko
Hello,
> > > 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.
We don't talk about device tree modifications on boot time!
Currently the SPI NAND chips are not fully configured in the device
tree data. Today a QSPI NAND is represented by an abstract 'compatible' entry
of 'spi-nand' in device tree. Which can be seen as something like a 'spi-nand'
bus with autodetection of the connected chips.
Therefore there is no way to reference a QSPI NAND chip directly, it's
auto-detected by the framework. There is also currently no possibility to
set the tCLQV parameter for a single SPI CS line.
Some parameters for the SPI NAND chips are already provided only by
the fitting chip driver (e.g. flash layout, banks, variants of the command
set of the device, ...). With this patchset it's now possible to provide also
the tCLQV data for this chip.
IMHO a autodetect system does not make much sense if you have to provide
parts of the chip configuration also in device tree. The framework should
detect the chip and fetch the operation parameters either from the chip
itself or from a chip driver which provides the required configuration settings.
Best Regards,
Eberhard
Hi Eberhard,
[email protected] wrote on Fri, 27 Oct 2023 12:41:23 +0000:
> Hello,
>
> > > > 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.
>
> We don't talk about device tree modifications on boot time!
>
> Currently the SPI NAND chips are not fully configured in the device
> tree data. Today a QSPI NAND is represented by an abstract 'compatible' entry
> of 'spi-nand' in device tree. Which can be seen as something like a 'spi-nand'
> bus with autodetection of the connected chips.
>
> Therefore there is no way to reference a QSPI NAND chip directly, it's
> auto-detected by the framework. There is also currently no possibility to
> set the tCLQV parameter for a single SPI CS line.
>
> Some parameters for the SPI NAND chips are already provided only by
> the fitting chip driver (e.g. flash layout, banks, variants of the command
> set of the device, ...). With this patchset it's now possible to provide also
> the tCLQV data for this chip.
>
> IMHO a autodetect system does not make much sense if you have to provide
> parts of the chip configuration also in device tree. The framework should
> detect the chip and fetch the operation parameters either from the chip
> itself or from a chip driver which provides the required configuration settings.
Yes, if the information is discoverable, we should propagate it to the
spi layer so that the relevant action is taken, from the most desirable
to the less desirable:
- adapting the sampling point
- lowering the bus frequency
- refusing the probe if none of these solutions are possible
Can you update your patchset with this in mind?
Thanks,
Miquèl
Hi Miquel,
On 27.10.23 14:46, Miquel Raynal wrote:
> Hi Eberhard,
>
> [email protected] wrote on Fri, 27 Oct 2023 12:41:23 +0000:
>
>> Hello,
>>
>>>>> 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.
>>
>> We don't talk about device tree modifications on boot time!
>>
>> Currently the SPI NAND chips are not fully configured in the device
>> tree data. Today a QSPI NAND is represented by an abstract 'compatible' entry
>> of 'spi-nand' in device tree. Which can be seen as something like a 'spi-nand'
>> bus with autodetection of the connected chips.
>>
>> Therefore there is no way to reference a QSPI NAND chip directly, it's
>> auto-detected by the framework. There is also currently no possibility to
>> set the tCLQV parameter for a single SPI CS line.
>>
>> Some parameters for the SPI NAND chips are already provided only by
>> the fitting chip driver (e.g. flash layout, banks, variants of the command
>> set of the device, ...). With this patchset it's now possible to provide also
>> the tCLQV data for this chip.
>>
>> IMHO a autodetect system does not make much sense if you have to provide
>> parts of the chip configuration also in device tree. The framework should
>> detect the chip and fetch the operation parameters either from the chip
>> itself or from a chip driver which provides the required configuration settings.
>
> Yes, if the information is discoverable, we should propagate it to the
> spi layer so that the relevant action is taken, from the most desirable
> to the less desirable:
> - adapting the sampling point
> - lowering the bus frequency
> - refusing the probe if none of these solutions are possible
This approach sounds reasonable and I guess we can try to come up with
an implementation. But it will probably take a while as this will get
quite a bit more complicated and we need to dive further into the SPI
driver framework.
And we need to keep in mind that this might cause perf degradation for
some users. I don't think there are cases where the clock can't be
limited and probe needs to be refused, but maybe there are some special
cases with whatever SPI controllers are around that I can't think of
right now.
Personally I think this is acceptable as the alternative is to continue
to use the SPI NAND chips out-of-spec and take the risk of instability
and data failures.
At some point someone probably should check all of those SPI NAND
datasheets and see what tCLQV values are documented there. We already
know that some Winbond and Toshiba chips have relevant tCLQV values
specified.
Thanks
Frieder
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.
On Fri, Oct 27, 2023 at 02:46:46PM +0200, Miquel Raynal wrote:
> Yes, if the information is discoverable, we should propagate it to the
> spi layer so that the relevant action is taken, from the most desirable
> to the less desirable:
> - adapting the sampling point
> - lowering the bus frequency
> - refusing the probe if none of these solutions are possible
> Can you update your patchset with this in mind?
Yes, this sounds exactly like what I'd expect the SPI core to be doing.
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
> 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