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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@microchip.com 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]