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.
Attachment:
signature.asc
Description: PGP signature
