Re: [PATCH RFC 6/7] spi: spi-mem: Call spi_set_rx_sampling_point() for each op

[Miquel Raynal]

Hi Frieder,

> The RX delay introduced by the chip is not design-dependent, only
> chip-dependent. There might be additional delays introduced by the board
> layout, but that's out of scope currently and I don't know if we even
> need this.

We do, for very high speeds, and this is the purpose of the PHY tuning
series from Santhosh.

[...]

>>> 3. By default limit the clock frequency for the READ_ID op to a safe
>>> value that works for all chips and controllers, even if RX sampling
>>> delay compensations is not available.
>> 
>> I am sorry this is not going to work out. There is no such harmonized
>> speed, differences can be an order (or two) of magnitude different, we
>> do not want to pay the penalty of a very slow identification on all
>> designs. We currently do the read several times with different
>> parameters. What if we were trying one more time by cutting the speed by
>> 2 (completely random proposal)?
>
> If we try with reduced clock as a fallback after the other attempts,
> there would be a slight chance of one of the earlier attempts with
> normal clock rate returning some invalid data that matches an existing
> chip ID, no? Isn't this a bit flaky?

Yeah, I understand your (and Mark's) concern.

> Let's say for a worst case scenario a chip has an RX delay of 20ns (the
> highest I've seen in datasheets so far is 8ns). In that case the maximum
> clock we could safely use for reading the ID would be 1/(2*20e-9) =
> 25MHz. Do you think it really makes much of a difference if we read the
> ID (only a handful of bytes) at 25MHz or full speed (e. g. 104 MHz)? I
> mean this should be fast enough either way, no? But maybe I'm misjudging
> this.

I am honestly not a big fan of the global penalty, but I am not totally
opposed either, especially since you just said you only observed 8ns
delays at most. This is 62.5MHz, which is already above what most
designs use, so the penalty would be minimal. What about taking this
approach and see if that fixes most of our use cases?

>>> 4. In PHY mode, check against the DT max frequency (board limitations)
>>> and chip max frequency before switching to this mode.
>> 
>> There is not such thing :-) the max frequency in DT currently would be
>> set to the platform limitation. We need somehow another parameter
>> indicating what is the maximum speed if we can fine tune the timings.
>
> So we can exceed the platform limitations using fine-tuned timings? I
> guess I need to read and understand that tuning RFC series.

Well, this is still under discussion. As of today, the max speed in DT
is the maximum speed. But it is in most cases the maximum speed
*without* tuning, which means if we tune the PHY delays we can increase
the speed further. I already proposed to turn this DT property into an
array, in order to indicate what are the maximum speed*s*, without and
with tuning, if ever possible.

>>> I hope this makes at least a bit of sense. I'm really not yet familiar
>>> with all the different use-cases and limitations.
>> 
>> If I get back to your own issue. Is there any chance we could avoid
>> tweaking the DT for handling it? Would there be a configuration of your
>> controller that would allow reading the ID of all the chips with enough
>> delays? Currently, the spi core doesn't care much about other parameters
>> than the frequency, but there is perhaps room for improvement.
>
> We could use RX delay compensation (one half clock cycle) in the
> controller by default (as currently done by the STM32 driver). But that
> would break if we use a very low clock (for whatever reason) because
> then the RX sampling delay gets neglectable compared to the clock period
> and sampling is triggered at the very end of the clock cycle when the
> data might already be invalid. At least that's what I suspect based on
> my test results.
> And it would also break if the chip actually requires more than one half
> clock cycle of RX sampling delay.
>
> The RX sampling delay setting must match the used clock frequency. In
> most cases there is a lot of room for tolerance before things start to
> fail, but it's not endless and especially at very high clock rates we
> need to take it into account.
>
> So if we don't cap the speed for reading the ID by default, we somehow
> need to know what value to use for the RX sampling delay or we need to
> try different values. And if the controller does not support or
> implement the RX sampling delay compensation, we need to cap the speed
> anyway.

What about:

*SPI controller drivers*
- Enable RX delay compensation in the controller by default, disable it
if the speed is very low, ie. compensation is dangerous
*Core*
- READID at max 65MHz (with the default RX delay compensation)
*Vendor driver*
- Indicate the internal RX delay per chip (maybe we need some heuristics
depending on the speed as well?)
*SPI controller drivers*
- Hook to tune RX based on the fastest reachable speed.

Thanks,
Miquèl