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

[Miquel Raynal]

Hello,

>>> For example on Winbond W25N04KV this leads to detection failures. So we
>>> should maybe introduce some kind of reduced clock setpoint for the
>>> initial detection, that is safe to be used without RX sampling delay
>>> compensation.
>> 
>> That should be the DT max frequency, no?
>
> Hm, I've seen it the other way round. In my perspective the DT max
> frequency describes the maximum clock frequency supported by the chip as
> given in the datasheet (assuming there are no other limitations
> introduced by board design, etc.).

I believe that what is in the datasheet should instead be statically
described in the SPI NAND vendor file.

The max SPI frequency DT property is here to tell at which speed to use
the device, and this depends mostly on the routing.

> The RX sampling delay is a timing parameter specified in the datasheet
> and it is relevant in order to interface the chip at maximum frequency
> within spec.

If it's not design dependent, then we should handle it "statically".

> Your approach would mean, that we need to manually calculate the maximum
> supported clock frequency without RX sampling delay compensation and use
> this value in the DT. Then let the driver increase the clock if RX
> sampling delay compensation is available.
>
> This would have the advantage, that even before initial detection we
> would use the correct clock by default. But it has the downside that the
> clock value in DT won't match the datasheet value.

It never does. Winbond chips can run at 166MHz. I know no design capable
of that natively (ie. without finer tuning, like what Santhosh
proposes).

> In my approach we assume the driver is able to use the maximum clock
> from the DT (same as in datasheet) by using RX sampling delay
> compensation and only if it turns out the compensation is not possible
> we will lower the clock dynamically.
>
> What if we do the following?
>
> 1. Add a parameter in the SPI NAND chip driver that contains the maximum
> supported clock frequency as given in the datasheet as this is clearly
> the best place to keep this device-specific parameter.

I've so far avoided doing it, but yes, this is something we can do. It
is going to be simple enough to implement as all the infrastructure is
already there. You can provide variants with speed limitations (look
into winbond.c). I was writing "0" in the fields which didn't need a
limitation that is something else than the maximum speed the chip can
sustain, as anyway the DT property *will* tell us what is this speed, if
we are ever able to reach it.

> 2. Allow to leave spi-max-frequency in DT unset for SPI NANDs in case
> there are no board-specific limitations and therefore the maximum
> frequency supported by the combination of chip and controller can be
> used.

In many cases, the limitations are board specific. Anyway, this is
already the case, you may just not put any value in this property.

> 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)?

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

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

Thanks,
Miquèl