| Message ID | 20250225131843.113752-2-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe08b7d5085a9774abc30c26d5aebc5b9cdd6091 |
| Headers | show |
| Series | ALSA: cs35l56: Fix for races when soft-resetting on SPI | expand |
On Tue, Feb 25, 2025 at 01:18:42PM +0000, Richard Fitzgerald wrote: > The async writes are a 12-year-old workaround for inefficiency of > synchronous writes in the SPI subsystem. The SPI subsystem has since > been changed to avoid the overheads, so this workaround should not be > necessary. That wasn't really the issue, it was that on systems with low numbers of slower cores (especially single core but there were some dual core systems too) there was a surprisingly low threashold where it was useful to overlap the marshalling of data to send to the device with transmitting already marshalled data. SMP adds locking requirements which loose a lot of the gains, and it's controller dependent where the cuttoff points for interrupts and/or DMA are.
On 25/02/2025 3:15 pm, Mark Brown wrote: > On Tue, Feb 25, 2025 at 01:18:42PM +0000, Richard Fitzgerald wrote: > >> The async writes are a 12-year-old workaround for inefficiency of >> synchronous writes in the SPI subsystem. The SPI subsystem has since >> been changed to avoid the overheads, so this workaround should not be >> necessary. > > That wasn't really the issue, it was that on systems with low numbers of > slower cores (especially single core but there were some dual core > systems too) there was a surprisingly low threashold where it was useful > to overlap the marshalling of data to send to the device with > transmitting already marshalled data. SMP adds locking requirements > which loose a lot of the gains, and it's controller dependent where the > cuttoff points for interrupts and/or DMA are. Ok. It was a very long time ago. Do you want a V2 with a commit message that doesn't libel the SPI framework? FWIW switching to sync write reduced the download of small amp firmware by 200ms on a 1.67MHz SPI bus.
On Tue, Feb 25, 2025 at 04:22:49PM +0000, Richard Fitzgerald wrote: > Ok. It was a very long time ago. > Do you want a V2 with a commit message that doesn't libel the SPI > framework? No, it's fine - it's in CI already. > FWIW switching to sync write reduced the download of small amp firmware > by 200ms on a 1.67MHz SPI bus. Yeah, with modern multi-core systems that's unsurprising especially with a controller that does small writes synchronously - the cost of locking on modern CPUs tends to blow out the overlapping of the formatting with the I/O. It's part of why maple tree caches don't do async writes during sync.
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 5365e9a43000..42433c19eb30 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1609,8 +1609,8 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, goto out_fw; } - ret = regmap_raw_write_async(regmap, reg, buf->buf, - le32_to_cpu(region->len)); + ret = regmap_raw_write(regmap, reg, buf->buf, + le32_to_cpu(region->len)); if (ret != 0) { cs_dsp_err(dsp, "%s.%d: Failed to write %d bytes at %d in %s: %d\n", @@ -1625,12 +1625,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, regions++; } - ret = regmap_async_complete(regmap); - if (ret != 0) { - cs_dsp_err(dsp, "Failed to complete async write: %d\n", ret); - goto out_fw; - } - if (pos > firmware->size) cs_dsp_warn(dsp, "%s.%d: %zu bytes at end of file\n", file, regions, pos - firmware->size); @@ -1638,7 +1632,6 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware, cs_dsp_debugfs_save_wmfwname(dsp, file); out_fw: - regmap_async_complete(regmap); cs_dsp_buf_free(&buf_list); if (ret == -EOVERFLOW) @@ -2326,8 +2319,8 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware cs_dsp_dbg(dsp, "%s.%d: Writing %d bytes at %x\n", file, blocks, le32_to_cpu(blk->len), reg); - ret = regmap_raw_write_async(regmap, reg, buf->buf, - le32_to_cpu(blk->len)); + ret = regmap_raw_write(regmap, reg, buf->buf, + le32_to_cpu(blk->len)); if (ret != 0) { cs_dsp_err(dsp, "%s.%d: Failed to write to %x in %s: %d\n", @@ -2339,10 +2332,6 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware blocks++; } - ret = regmap_async_complete(regmap); - if (ret != 0) - cs_dsp_err(dsp, "Failed to complete async write: %d\n", ret); - if (pos > firmware->size) cs_dsp_warn(dsp, "%s.%d: %zu bytes at end of file\n", file, blocks, pos - firmware->size); @@ -2350,7 +2339,6 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware cs_dsp_debugfs_save_binname(dsp, file); out_fw: - regmap_async_complete(regmap); cs_dsp_buf_free(&buf_list); if (ret == -EOVERFLOW) @@ -2561,8 +2549,8 @@ static int cs_dsp_adsp2_enable_core(struct cs_dsp *dsp) { int ret; - ret = regmap_update_bits_async(dsp->regmap, dsp->base + ADSP2_CONTROL, - ADSP2_SYS_ENA, ADSP2_SYS_ENA); + ret = regmap_update_bits(dsp->regmap, dsp->base + ADSP2_CONTROL, + ADSP2_SYS_ENA, ADSP2_SYS_ENA); if (ret != 0) return ret;
Change calls to async regmap write functions to use the normal blocking writes so that the cs35l56 driver can use spi_bus_lock() to gain exclusive access to the SPI bus. As this is part of a fix, it makes only the minimal change to swap the functions to the blocking equivalents. There's no need to risk reworking the buffer allocation logic that is now partially redundant. The async writes are a 12-year-old workaround for inefficiency of synchronous writes in the SPI subsystem. The SPI subsystem has since been changed to avoid the overheads, so this workaround should not be necessary. The cs35l56 driver needs to use spi_bus_lock() prevent bus activity while it is soft-resetting the cs35l56. But spi_bus_lock() is incompatible with spi_async() calls, which will fail with -EBUSY. Fixes: 8a731fd37f8b ("ASoC: cs35l56: Move utility functions to shared file") Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- drivers/firmware/cirrus/cs_dsp.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)