| Message ID | 20200629164103.1.Ied8e8ad8bbb2df7f947e3bc5ea1c315e041785a2@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d40f0b6f2e21f2400ae8b1b120d11877d9ffd8ec |
| Headers | show |
| Series | spi: Avoid setting the chip select if we don't need to | expand |
On Mon, 2020-06-29 at 16:41 -0700, Douglas Anderson wrote: > [External] > > On some SPI controllers (like spi-geni-qcom) setting the chip select > is a heavy operation. For instance on spi-geni-qcom, with the current > code, is was measured as taking upwards of 20 us. Even on SPI > controllers that aren't as heavy, setting the chip select is at least > something like a MMIO operation over some peripheral bus which isn't > as fast as a RAM access. > > While it would be good to find ways to mitigate problems like this in > the drivers for those SPI controllers, it can also be noted that the > SPI framework could also help out. Specifically, in some situations, > we can see the SPI framework calling the driver's set_cs() with the > same parameter several times in a row. This is specifically observed > when looking at the way the Chrome OS EC SPI driver (cros_ec_spi) > works but other drivers likely trip it to some extent. > > Let's solve this by caching the chip select state in the core and only > calling into the controller if there was a change. We check not only > the "enable" state but also the chip select mode (active high or > active low) since controllers may care about both the mode and the > enable flag in their callback. I think checkpatch suggested I be added here, since I touched some parts of the delay/timings code. > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/spi/spi.c | 11 +++++++++++ > include/linux/spi/spi.h | 4 ++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 6fa56590bba2..d4ba723a30da 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -778,6 +778,17 @@ static void spi_set_cs(struct spi_device *spi, bool > enable) > { > bool enable1 = enable; > > + /* > + * Avoid calling into the driver (or doing delays) if the chip > select > + * isn't actually changing from the last time this was called. > + */ > + if ((spi->controller->last_cs_enable == enable) && > + (spi->controller->last_cs_mode_high == (spi->mode & > SPI_CS_HIGH))) > + return; > + > + spi->controller->last_cs_enable = enable; > + spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; > + I don't feel like this is the best approach for the SPI CS handling, because it's pretty difficult to guess the last CS state, and whether this return would cause other weirder issues [like not toggling CS when it should]. Maybe a question is: when should this CS be toggled [or not]? Is it between 2 calls of spi_transfer_one_message() or between 2 spi_transfers? Or, is "xfer->cs_change == 1" where it shouldn't be? I think, there are some ways to not toggle CS between some of these, or if there aren't, some controls could be added/proposed to avoid toggling CS, vs doing caching. > if (!spi->controller->set_cs_timing) { > if (enable1) > spi_delay_exec(&spi->controller->cs_setup, NULL); > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index b4917df79637..0e67a9a3a1d3 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -368,6 +368,8 @@ static inline void spi_unregister_driver(struct > spi_driver *sdrv) > * @cur_msg_prepared: spi_prepare_message was called for the currently > * in-flight message > * @cur_msg_mapped: message has been mapped for DMA > + * @last_cs_enable: was enable true on the last call to set_cs. > + * @last_cs_mode_high: was (mode & SPI_CS_HIGH) true on the last call to > set_cs. > * @xfer_completion: used by core transfer_one_message() > * @busy: message pump is busy > * @running: message pump is running > @@ -604,6 +606,8 @@ struct spi_controller { > bool auto_runtime_pm; > bool cur_msg_prepared; > bool cur_msg_mapped; > + bool last_cs_enable; > + bool last_cs_mode_high; > bool fallback; > struct completion xfer_completion; > size_t max_dma_len;
On Wed, Jul 01, 2020 at 06:26:24AM +0000, Ardelean, Alexandru wrote: > On Mon, 2020-06-29 at 16:41 -0700, Douglas Anderson wrote: > > + spi->controller->last_cs_enable = enable; > > + spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; > I don't feel like this is the best approach for the SPI CS handling, > because it's pretty difficult to guess the last CS state, and whether this > return would cause other weirder issues [like not toggling CS when it > should]. There's no guesswork involved here - the only thing that should be setting the chip select is the SPI core so other than at startup we always know the state of the chip select. > Maybe a question is: when should this CS be toggled [or not]? > Is it between 2 calls of spi_transfer_one_message() or between 2 > spi_transfers? > Or, is "xfer->cs_change == 1" where it shouldn't be? This is well documented, it's asserted while a message is being transferred unless changed by cs_change in which case we do whatever the opposite of the default action is.
On Mon, 29 Jun 2020 16:41:06 -0700, Douglas Anderson wrote: > On some SPI controllers (like spi-geni-qcom) setting the chip select > is a heavy operation. For instance on spi-geni-qcom, with the current > code, is was measured as taking upwards of 20 us. Even on SPI > controllers that aren't as heavy, setting the chip select is at least > something like a MMIO operation over some peripheral bus which isn't > as fast as a RAM access. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: Avoid setting the chip select if we don't need to commit: d40f0b6f2e21f2400ae8b1b120d11877d9ffd8ec All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 6fa56590bba2..d4ba723a30da 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -778,6 +778,17 @@ static void spi_set_cs(struct spi_device *spi, bool enable) { bool enable1 = enable; + /* + * Avoid calling into the driver (or doing delays) if the chip select + * isn't actually changing from the last time this was called. + */ + if ((spi->controller->last_cs_enable == enable) && + (spi->controller->last_cs_mode_high == (spi->mode & SPI_CS_HIGH))) + return; + + spi->controller->last_cs_enable = enable; + spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH; + if (!spi->controller->set_cs_timing) { if (enable1) spi_delay_exec(&spi->controller->cs_setup, NULL); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index b4917df79637..0e67a9a3a1d3 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -368,6 +368,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * @cur_msg_prepared: spi_prepare_message was called for the currently * in-flight message * @cur_msg_mapped: message has been mapped for DMA + * @last_cs_enable: was enable true on the last call to set_cs. + * @last_cs_mode_high: was (mode & SPI_CS_HIGH) true on the last call to set_cs. * @xfer_completion: used by core transfer_one_message() * @busy: message pump is busy * @running: message pump is running @@ -604,6 +606,8 @@ struct spi_controller { bool auto_runtime_pm; bool cur_msg_prepared; bool cur_msg_mapped; + bool last_cs_enable; + bool last_cs_mode_high; bool fallback; struct completion xfer_completion; size_t max_dma_len;
On some SPI controllers (like spi-geni-qcom) setting the chip select is a heavy operation. For instance on spi-geni-qcom, with the current code, is was measured as taking upwards of 20 us. Even on SPI controllers that aren't as heavy, setting the chip select is at least something like a MMIO operation over some peripheral bus which isn't as fast as a RAM access. While it would be good to find ways to mitigate problems like this in the drivers for those SPI controllers, it can also be noted that the SPI framework could also help out. Specifically, in some situations, we can see the SPI framework calling the driver's set_cs() with the same parameter several times in a row. This is specifically observed when looking at the way the Chrome OS EC SPI driver (cros_ec_spi) works but other drivers likely trip it to some extent. Let's solve this by caching the chip select state in the core and only calling into the controller if there was a change. We check not only the "enable" state but also the chip select mode (active high or active low) since controllers may care about both the mode and the enable flag in their callback. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/spi/spi.c | 11 +++++++++++ include/linux/spi/spi.h | 4 ++++ 2 files changed, 15 insertions(+)