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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

On 12/10/20 5:33 PM, Mark Brown wrote:
> On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
>> On 12/9/20 10:30 PM, Serge Semin 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).
> 
> Right, that's the hole :/
> 
>>>> 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.
> 
> Yes, we should just make sure things are OK in the core as much as we
> can so there's less work for driver authors.
> 
>>> 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.
> 
> The potential for board issues suggests that we should be taking the
> minimum of what the board DT and runtime discovery say - if the board
> limits things more than the parts we should assume that there's a system
> integration limitation there.
> 
>> Let me know if you think that this patch is ok for now. I can update the
>> commit message if needed.
> 
> It does work for now but it'd be nicer if we were doing this through
> recording the decision on the transfer.
> 

Ok, we can drop the patch, as nobody complained about this until now. I can
work on a better approach. Are you saying that we should calibrate/adjust the
maximum supported frequency on each operation/command? Most of the commands
can work at the same frequency. I know an exception: on SPI NOR flashes, the
jesd216 standard specifies that the READ SFDP command should be run at 50MHz,
even though the rest of the commands/opcodes can run at a higher frequency.
It is common that flashes can run this command at 50+ MHz, and nobody bothered
about adjusting the frequency at run-time until now. That being said, maybe we
can calibrate/adjust a generic max frequency for most of the commands and
treat the exceptions on a per operation basis.

Cheers,
ta