Re: [PATCH] spi: Limit the spi device max speed to controller's max speed

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

Hi, Serge, Mark,

On 12/9/20 10:30 PM, Serge Semin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Dec 09, 2020 at 08:25:52PM +0000, Mark Brown wrote:
>> On Wed, Dec 09, 2020 at 11:15:35PM +0300, Serge Semin wrote:
>>> On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote:
>>
>>>> Right, in general we aim to do this sort of fixup on the transfers
>>>> and messages rather than the devices, I guess we might be missing
>>>> validation in some of the flash acceleration paths or was this an issue
>>>> seen through inspection?

We miss validation for the SPI controllers that provide the
spi_controller_mem_ops with its exec_op() method. In this case the SPI
core does not check if the max_speed_hz of spi_device overrides the
max_speed_hz of controller.

This was seen through inspection. There are octal SPI NOR flashes that
can run at 400 MHZ, and I've also seen SPI controllers that are limited
to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet
in mainline).

>>
>>> In case of DW SPI driver we just make sure the SPI-client device
>>> speed set in the max_speed_hz doesn't exceed the controller SPI-bus
>>> clock frequency and clamp it if it does. So the driver is safe in that
>>> matter.
>>
> 
>> Ideally the driver wouldn't have to check though (no harm in doing so of
>> course).

Right. I thought of doing this in the SPI core, rather than doing in (each)
controller driver.

Serge,

I've looked at https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570
That limits the xfer speed hz to the controller's max_speed_hz. SPI
controllers that implement the exec_op() optimized handler are not going
through this path and are not covered by the check in __spi_validate().

> 
> If so then we'd need to have a dedicated speed-related field in the
> spi_mem_op structure, which would be accordingly initialized by the
> SPI-mem core.
> 

We do need a max_speed_hz in the SPIMEM, but probably for other purposes:
NOR flashes, for example, can discover the maximum supported frequency
at run-time, when parsing the jesd216 SFDP tables. One may consider that
the run-time discovered max_speed_hz should have a higher precedence than
what we fill with the spi-max-frequency dt property, because the
manufacturers/jesd216 standard know/s better. Of course, if the
manufacturers put a wrong max_speed_hz in the sfdp table, that can be
updated at runtime with a fixup() hook, on a case by case basis. Other
thing to consider here, is the max_speed_hz supported by the PCB. For
example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but
the PCB only 180 MHZ, then we'll have to synchronize all three. But this
seems like a discussion for other patch.

Let me know if you think that this patch is ok for now. I can update the
commit message if needed.

Cheers,
ta