Messages in this thread | | | Date | Thu, 10 Dec 2020 15:33:51 +0000 | From | Mark Brown <> | Subject | Re: [PATCH] spi: Limit the spi device max speed to controller's max speed |
| |
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. [unhandled content-type:application/pgp-signature] | |