[PATCH 01/10] ASoC: wm_adsp: Remove the wmfw_add_ctl helper function
The helper function wmfw_add_ctl is only called from one place and that place is a function with only 2 lines of code. Merge the helper function into the work function to simplify the code.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- sound/soc/codecs/wm_adsp.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index d4f0d72cbcc80..404717e30f44d 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -537,15 +537,20 @@ static unsigned int wmfw_convert_flags(unsigned int in, unsigned int len) return out; }
-static int wmfw_add_ctl(struct wm_adsp *dsp, struct wm_coeff_ctl *ctl) +static void wm_adsp_ctl_work(struct work_struct *work) { + struct wm_coeff_ctl *ctl = container_of(work, + struct wm_coeff_ctl, + work); struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; + struct wm_adsp *dsp = container_of(cs_ctl->dsp, + struct wm_adsp, + cs_dsp); struct snd_kcontrol_new *kcontrol; - int ret;
kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL); if (!kcontrol) - return -ENOMEM; + return;
kcontrol->name = ctl->name; kcontrol->info = wm_coeff_info; @@ -571,29 +576,9 @@ static int wmfw_add_ctl(struct wm_adsp *dsp, struct wm_coeff_ctl *ctl) break; }
- ret = snd_soc_add_component_controls(dsp->component, kcontrol, 1); - if (ret < 0) - goto err_kcontrol; + snd_soc_add_component_controls(dsp->component, kcontrol, 1);
kfree(kcontrol); - - return 0; - -err_kcontrol: - kfree(kcontrol); - return ret; -} - -static void wm_adsp_ctl_work(struct work_struct *work) -{ - struct wm_coeff_ctl *ctl = container_of(work, - struct wm_coeff_ctl, - work); - struct wm_adsp *dsp = container_of(ctl->cs_ctl->dsp, - struct wm_adsp, - cs_dsp); - - wmfw_add_ctl(dsp, ctl); }
static int wm_adsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl)
Some of the control functions exposed by the cs_dsp code require the pwr_lock to be held by the caller. Add lockdep_assert_held calls to ensure this is done correctly.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 1a0c6c793f6a7..0d1ba7d8efa47 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -653,6 +653,8 @@ int cs_dsp_coeff_write_acked_control(struct cs_dsp_coeff_ctl *ctl, unsigned int unsigned int reg; int i, ret;
+ lockdep_assert_held(&dsp->pwr_lock); + if (!dsp->running) return -EPERM;
@@ -754,6 +756,8 @@ int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, const void *buf, size_ { int ret = 0;
+ lockdep_assert_held(&ctl->dsp->pwr_lock); + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) ret = -EPERM; else if (buf != ctl->cache) @@ -811,6 +815,8 @@ int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len) { int ret = 0;
+ lockdep_assert_held(&ctl->dsp->pwr_lock); + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) { if (ctl->enabled && ctl->dsp->running) return cs_dsp_coeff_read_ctrl_raw(ctl, buf, len); @@ -1453,6 +1459,8 @@ struct cs_dsp_coeff_ctl *cs_dsp_get_ctl(struct cs_dsp *dsp, const char *name, in { struct cs_dsp_coeff_ctl *pos, *rslt = NULL;
+ lockdep_assert_held(&dsp->pwr_lock); + list_for_each_entry(pos, &dsp->ctl_list, list) { if (!pos->subname) continue; @@ -1548,6 +1556,8 @@ struct cs_dsp_alg_region *cs_dsp_find_alg_region(struct cs_dsp *dsp, { struct cs_dsp_alg_region *alg_region;
+ lockdep_assert_held(&dsp->pwr_lock); + list_for_each_entry(alg_region, &dsp->alg_regions, list) { if (id == alg_region->alg && type == alg_region->type) return alg_region; @@ -2783,6 +2793,8 @@ int cs_dsp_read_raw_data_block(struct cs_dsp *dsp, int mem_type, unsigned int me unsigned int reg; int ret;
+ lockdep_assert_held(&dsp->pwr_lock); + if (!mem) return -EINVAL;
@@ -2836,6 +2848,8 @@ int cs_dsp_write_data_word(struct cs_dsp *dsp, int mem_type, unsigned int mem_ad __be32 val = cpu_to_be32(data & 0x00ffffffu); unsigned int reg;
+ lockdep_assert_held(&dsp->pwr_lock); + if (!mem) return -EINVAL;
The firmware coefficient files contain version information that is currently ignored by the cs_dsp code. This information specifies which version of the firmware the coefficient were generated for. Add a check into the code which prints a warning in the case the coefficient and firmware differ in version, in many cases this will be ok but it is not always, so best to let the user know there is a potential issue.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 49 +++++++++++++++++++++++++--------- include/linux/firmware/cirrus/cs_dsp.h | 2 ++ 2 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 0d1ba7d8efa47..0da454a8498d0 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1569,7 +1569,7 @@ EXPORT_SYMBOL_GPL(cs_dsp_find_alg_region);
static struct cs_dsp_alg_region *cs_dsp_create_region(struct cs_dsp *dsp, int type, __be32 id, - __be32 base) + __be32 ver, __be32 base) { struct cs_dsp_alg_region *alg_region;
@@ -1579,6 +1579,7 @@ static struct cs_dsp_alg_region *cs_dsp_create_region(struct cs_dsp *dsp,
alg_region->type = type; alg_region->alg = be32_to_cpu(id); + alg_region->ver = be32_to_cpu(ver); alg_region->base = be32_to_cpu(base);
list_add_tail(&alg_region->list, &dsp->alg_regions); @@ -1628,14 +1629,14 @@ static void cs_dsp_parse_wmfw_v3_id_header(struct cs_dsp *dsp, nalgs); }
-static int cs_dsp_create_regions(struct cs_dsp *dsp, __be32 id, int nregions, - const int *type, __be32 *base) +static int cs_dsp_create_regions(struct cs_dsp *dsp, __be32 id, __be32 ver, + int nregions, const int *type, __be32 *base) { struct cs_dsp_alg_region *alg_region; int i;
for (i = 0; i < nregions; i++) { - alg_region = cs_dsp_create_region(dsp, type[i], id, base[i]); + alg_region = cs_dsp_create_region(dsp, type[i], id, ver, base[i]); if (IS_ERR(alg_region)) return PTR_ERR(alg_region); } @@ -1670,12 +1671,14 @@ static int cs_dsp_adsp1_setup_algs(struct cs_dsp *dsp) cs_dsp_parse_wmfw_id_header(dsp, &adsp1_id.fw, n_algs);
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP1_ZM, - adsp1_id.fw.id, adsp1_id.zm); + adsp1_id.fw.id, adsp1_id.fw.ver, + adsp1_id.zm); if (IS_ERR(alg_region)) return PTR_ERR(alg_region);
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP1_DM, - adsp1_id.fw.id, adsp1_id.dm); + adsp1_id.fw.id, adsp1_id.fw.ver, + adsp1_id.dm); if (IS_ERR(alg_region)) return PTR_ERR(alg_region);
@@ -1698,6 +1701,7 @@ static int cs_dsp_adsp1_setup_algs(struct cs_dsp *dsp)
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP1_DM, adsp1_alg[i].alg.id, + adsp1_alg[i].alg.ver, adsp1_alg[i].dm); if (IS_ERR(alg_region)) { ret = PTR_ERR(alg_region); @@ -1719,6 +1723,7 @@ static int cs_dsp_adsp1_setup_algs(struct cs_dsp *dsp)
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP1_ZM, adsp1_alg[i].alg.id, + adsp1_alg[i].alg.ver, adsp1_alg[i].zm); if (IS_ERR(alg_region)) { ret = PTR_ERR(alg_region); @@ -1771,17 +1776,20 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp) cs_dsp_parse_wmfw_id_header(dsp, &adsp2_id.fw, n_algs);
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_XM, - adsp2_id.fw.id, adsp2_id.xm); + adsp2_id.fw.id, adsp2_id.fw.ver, + adsp2_id.xm); if (IS_ERR(alg_region)) return PTR_ERR(alg_region);
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_YM, - adsp2_id.fw.id, adsp2_id.ym); + adsp2_id.fw.id, adsp2_id.fw.ver, + adsp2_id.ym); if (IS_ERR(alg_region)) return PTR_ERR(alg_region);
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_ZM, - adsp2_id.fw.id, adsp2_id.zm); + adsp2_id.fw.id, adsp2_id.fw.ver, + adsp2_id.zm); if (IS_ERR(alg_region)) return PTR_ERR(alg_region);
@@ -1806,6 +1814,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp)
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_XM, adsp2_alg[i].alg.id, + adsp2_alg[i].alg.ver, adsp2_alg[i].xm); if (IS_ERR(alg_region)) { ret = PTR_ERR(alg_region); @@ -1827,6 +1836,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp)
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_YM, adsp2_alg[i].alg.id, + adsp2_alg[i].alg.ver, adsp2_alg[i].ym); if (IS_ERR(alg_region)) { ret = PTR_ERR(alg_region); @@ -1848,6 +1858,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp)
alg_region = cs_dsp_create_region(dsp, WMFW_ADSP2_ZM, adsp2_alg[i].alg.id, + adsp2_alg[i].alg.ver, adsp2_alg[i].zm); if (IS_ERR(alg_region)) { ret = PTR_ERR(alg_region); @@ -1873,7 +1884,7 @@ static int cs_dsp_adsp2_setup_algs(struct cs_dsp *dsp) return ret; }
-static int cs_dsp_halo_create_regions(struct cs_dsp *dsp, __be32 id, +static int cs_dsp_halo_create_regions(struct cs_dsp *dsp, __be32 id, __be32 ver, __be32 xm_base, __be32 ym_base) { static const int types[] = { @@ -1882,7 +1893,7 @@ static int cs_dsp_halo_create_regions(struct cs_dsp *dsp, __be32 id, }; __be32 bases[] = { xm_base, xm_base, ym_base, ym_base };
- return cs_dsp_create_regions(dsp, id, ARRAY_SIZE(types), types, bases); + return cs_dsp_create_regions(dsp, id, ver, ARRAY_SIZE(types), types, bases); }
static int cs_dsp_halo_setup_algs(struct cs_dsp *dsp) @@ -1910,7 +1921,7 @@ static int cs_dsp_halo_setup_algs(struct cs_dsp *dsp)
cs_dsp_parse_wmfw_v3_id_header(dsp, &halo_id.fw, n_algs);
- ret = cs_dsp_halo_create_regions(dsp, halo_id.fw.id, + ret = cs_dsp_halo_create_regions(dsp, halo_id.fw.id, halo_id.fw.ver, halo_id.xm_base, halo_id.ym_base); if (ret) return ret; @@ -1934,6 +1945,7 @@ static int cs_dsp_halo_setup_algs(struct cs_dsp *dsp) be32_to_cpu(halo_alg[i].ym_base));
ret = cs_dsp_halo_create_regions(dsp, halo_alg[i].alg.id, + halo_alg[i].alg.ver, halo_alg[i].xm_base, halo_alg[i].ym_base); if (ret) @@ -1955,7 +1967,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware const struct cs_dsp_region *mem; struct cs_dsp_alg_region *alg_region; const char *region_name; - int ret, pos, blocks, type, offset, reg; + int ret, pos, blocks, type, offset, reg, version; struct cs_dsp_buf *buf;
if (!firmware) @@ -1999,6 +2011,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware
type = le16_to_cpu(blk->type); offset = le16_to_cpu(blk->offset); + version = le32_to_cpu(blk->ver) >> 8;
cs_dsp_dbg(dsp, "%s.%d: %x v%d.%d.%d\n", file, blocks, le32_to_cpu(blk->id), @@ -2056,6 +2069,16 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware alg_region = cs_dsp_find_alg_region(dsp, type, le32_to_cpu(blk->id)); if (alg_region) { + if (version != alg_region->ver) + cs_dsp_warn(dsp, + "Algorithm coefficient version %d.%d.%d but expected %d.%d.%d\n", + (version >> 16) & 0xFF, + (version >> 8) & 0xFF, + version & 0xFF, + (alg_region->ver >> 16) & 0xFF, + (alg_region->ver >> 8) & 0xFF, + alg_region->ver & 0xFF); + reg = alg_region->base; reg = dsp->ops->region_to_reg(mem, reg); reg += offset; diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h index 3a54b1afc48fc..ce54705e2becf 100644 --- a/include/linux/firmware/cirrus/cs_dsp.h +++ b/include/linux/firmware/cirrus/cs_dsp.h @@ -54,12 +54,14 @@ struct cs_dsp_region { * struct cs_dsp_alg_region - Describes a logical algorithm region in DSP address space * @list: List node for internal use * @alg: Algorithm id + * @ver: Expected algorithm version * @type: Memory region type * @base: Address of region */ struct cs_dsp_alg_region { struct list_head list; unsigned int alg; + unsigned int ver; int type; unsigned int base; };
On Tue, Nov 16, 2021 at 04:16:02PM +0000, Charles Keepax wrote:
The firmware coefficient files contain version information that is currently ignored by the cs_dsp code. This information specifies which version of the firmware the coefficient were generated for. Add a check into the code which prints a warning in the case the coefficient and firmware differ in version, in many cases this will be ok but it is not always, so best to let the user know there is a potential issue.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Simon Trimmer simont@opensource.cirrus.com
This has Simon's signoff after yours but no other indication of his involvement?
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Tuesday, November 16, 2021 6:08 PM To: Charles Keepax ckeepax@opensource.cirrus.com Cc: lgirdwood@gmail.com; patches@opensource.cirrus.com; alsa-devel@alsa- project.org Subject: Re: [PATCH 03/10] firmware: cs_dsp: Add version checks on
coefficient
loading
On Tue, Nov 16, 2021 at 04:16:02PM +0000, Charles Keepax wrote:
The firmware coefficient files contain version information that is currently ignored by the cs_dsp code. This information specifies which version of the firmware the coefficient were generated for. Add a check into the code which prints a warning in the case the coefficient and firmware differ in version, in many cases this will be ok but it is not always, so best to let the user know there is a potential issue.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Simon Trimmer simont@opensource.cirrus.com
This has Simon's signoff after yours but no other indication of his involvement?
I have been working with Charles on most of these patches over the last few months and I'd fixed some internal review comments on this one before we shared it. If it helps I can certainly ack the chain?
On Tue, Nov 16, 2021 at 06:44:06PM -0000, Simon Trimmer wrote:
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Simon Trimmer simont@opensource.cirrus.com
This has Simon's signoff after yours but no other indication of his involvement?
I have been working with Charles on most of these patches over the last few months and I'd fixed some internal review comments on this one before we shared it. If it helps I can certainly ack the chain?
Yeah apologies I am never quite sure what to do in this situation. We internally have someone add their sign-off when they make a change to a patch. I could change this to a Co-authored-by: if that is the preferred upstream approach?
Thanks, Charles
On Wed, Nov 17, 2021 at 11:00:46AM +0000, Charles Keepax wrote:
On Tue, Nov 16, 2021 at 06:44:06PM -0000, Simon Trimmer wrote:
I have been working with Charles on most of these patches over the last few months and I'd fixed some internal review comments on this one before we shared it. If it helps I can certainly ack the chain?
Yeah apologies I am never quite sure what to do in this situation. We internally have someone add their sign-off when they make a change to a patch. I could change this to a Co-authored-by: if that is the preferred upstream approach?
Yes, and if nothing else the signoff of the person sending the mail should be the last one in the chain in the email.
The code already has a post_run callback, add a matching pre_run callback to the client_ops that is called before execution is started. This callback provides a convenient place for the client code to set DSP controls or hardware that requires configuration before the DSP core actually starts execution. Note that placing this callback before cs_dsp_coeff_sync_controls is important to ensure that any control values are then correctly synced out to the chip.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 6 ++++++ include/linux/firmware/cirrus/cs_dsp.h | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 0da454a8498d0..ef7afadea42d1 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -2627,6 +2627,12 @@ int cs_dsp_run(struct cs_dsp *dsp) goto err; }
+ if (dsp->client_ops->pre_run) { + ret = dsp->client_ops->pre_run(dsp); + if (ret) + goto err; + } + /* Sync set controls */ ret = cs_dsp_coeff_sync_controls(dsp); if (ret != 0) diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h index ce54705e2becf..0bf849baeaa5a 100644 --- a/include/linux/firmware/cirrus/cs_dsp.h +++ b/include/linux/firmware/cirrus/cs_dsp.h @@ -187,7 +187,8 @@ struct cs_dsp { * struct cs_dsp_client_ops - client callbacks * @control_add: Called under the pwr_lock when a control is created * @control_remove: Called under the pwr_lock when a control is destroyed - * @post_run: Called under the pwr_lock by cs_dsp_run() + * @pre_run: Called under the pwr_lock by cs_dsp_run() before the core is started + * @post_run: Called under the pwr_lock by cs_dsp_run() after the core is started * @post_stop: Called under the pwr_lock by cs_dsp_stop() * @watchdog_expired: Called when a watchdog expiry is detected * @@ -197,6 +198,7 @@ struct cs_dsp { struct cs_dsp_client_ops { int (*control_add)(struct cs_dsp_coeff_ctl *ctl); void (*control_remove)(struct cs_dsp_coeff_ctl *ctl); + int (*pre_run)(struct cs_dsp *dsp); int (*post_run)(struct cs_dsp *dsp); void (*post_stop)(struct cs_dsp *dsp); void (*watchdog_expired)(struct cs_dsp *dsp);
The coefficient file contains various info strings, and the equivalent strings are printed from the WMFW file as it is loaded. Add support for printing these from the coefficient file as well.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index ef7afadea42d1..3d21574f3a443 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1968,6 +1968,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware struct cs_dsp_alg_region *alg_region; const char *region_name; int ret, pos, blocks, type, offset, reg, version; + char *text = NULL; struct cs_dsp_buf *buf;
if (!firmware) @@ -2025,6 +2026,8 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware region_name = "Unknown"; switch (type) { case (WMFW_NAME_TEXT << 8): + text = kzalloc(le32_to_cpu(blk->len) + 1, GFP_KERNEL); + break; case (WMFW_INFO_TEXT << 8): case (WMFW_METADATA << 8): break; @@ -2094,6 +2097,13 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware break; }
+ if (text) { + memcpy(text, blk->data, le32_to_cpu(blk->len)); + cs_dsp_info(dsp, "%s: %s\n", dsp->fw_name, text); + kfree(text); + text = NULL; + } + if (reg) { if (le32_to_cpu(blk->len) > firmware->size - pos - sizeof(*blk)) { @@ -2144,6 +2154,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware out_fw: regmap_async_complete(regmap); cs_dsp_buf_free(&buf_list); + kfree(text); return ret; }
Add support for the revision 2 coefficient file, this format is identical to revision 1 and was simply added by accident to some firmware. However unfortunately many firmwares have leaked into production using this and as such driver support really needs to be added for it.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 3d21574f3a443..62ba4ebbf11f5 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1990,6 +1990,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware
switch (be32_to_cpu(hdr->rev) & 0xff) { case 1: + case 2: break; default: cs_dsp_err(dsp, "%s: Unsupported coefficient file format %d\n",
Add a NULL check to the cs_dsp_coeff_write/read_ctrl functions. This is a major convenience for users of the cs_dsp library as it allows the call to cs_dsp_get_ctl to be inlined with the call to read/write the control itself.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 62ba4ebbf11f5..9eecd16265375 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -758,6 +758,9 @@ int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, const void *buf, size_
lockdep_assert_held(&ctl->dsp->pwr_lock);
+ if (!ctl) + return -ENOENT; + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) ret = -EPERM; else if (buf != ctl->cache) @@ -817,6 +820,9 @@ int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len)
lockdep_assert_held(&ctl->dsp->pwr_lock);
+ if (!ctl) + return -ENOENT; + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) { if (ctl->enabled && ctl->dsp->running) return cs_dsp_coeff_read_ctrl_raw(ctl, buf, len);
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 4 ++-- include/linux/firmware/cirrus/cs_dsp.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 9eecd16265375..d1bcade2efe23 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -746,7 +746,7 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, * cs_dsp_coeff_write_ctrl() - Writes the given buffer to the given coefficient control * @ctl: pointer to coefficient control * @buf: the buffer to write to the given control - * @len: the length of the buffer + * @len: the length of the buffer in bytes * * Must be called with pwr_lock held. * @@ -808,7 +808,7 @@ static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, void *buf, s * cs_dsp_coeff_read_ctrl() - Reads the given coefficient control into the given buffer * @ctl: pointer to coefficient control * @buf: the buffer to store to the given control - * @len: the length of the buffer + * @len: the length of the buffer in bytes * * Must be called with pwr_lock held. * diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h index 0bf849baeaa5a..1ad1b173417a0 100644 --- a/include/linux/firmware/cirrus/cs_dsp.h +++ b/include/linux/firmware/cirrus/cs_dsp.h @@ -76,8 +76,8 @@ struct cs_dsp_alg_region { * @enabled: Flag indicating whether control is enabled * @list: List node for internal use * @cache: Cached value of the control - * @offset: Offset of control within alg_region - * @len: Length of the cached value + * @offset: Offset of control within alg_region in words + * @len: Length of the cached value in bytes * @set: Flag indicating the value has been written by the user * @flags: Bitfield of WMFW_CTL_FLAG_ control flags defined in wmfw.h * @type: One of the WMFW_CTL_TYPE_ control types defined in wmfw.h
Provide a mechanism to access only part of a control through the cs_dsp interface.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 44 +++++++++++++++++++++------------- include/linux/firmware/cirrus/cs_dsp.h | 6 +++-- sound/soc/codecs/wm_adsp.c | 14 +++++------ 3 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index d1bcade2efe23..5fe08de91ecd3 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -616,7 +616,8 @@ static void cs_dsp_halo_show_fw_status(struct cs_dsp *dsp) offs[0], offs[1], offs[2], offs[3]); }
-static int cs_dsp_coeff_base_reg(struct cs_dsp_coeff_ctl *ctl, unsigned int *reg) +static int cs_dsp_coeff_base_reg(struct cs_dsp_coeff_ctl *ctl, unsigned int *reg, + unsigned int off) { const struct cs_dsp_alg_region *alg_region = &ctl->alg_region; struct cs_dsp *dsp = ctl->dsp; @@ -629,7 +630,7 @@ static int cs_dsp_coeff_base_reg(struct cs_dsp_coeff_ctl *ctl, unsigned int *reg return -EINVAL; }
- *reg = dsp->ops->region_to_reg(mem, ctl->alg_region.base + ctl->offset); + *reg = dsp->ops->region_to_reg(mem, ctl->alg_region.base + ctl->offset + off);
return 0; } @@ -658,7 +659,7 @@ int cs_dsp_coeff_write_acked_control(struct cs_dsp_coeff_ctl *ctl, unsigned int if (!dsp->running) return -EPERM;
- ret = cs_dsp_coeff_base_reg(ctl, ®); + ret = cs_dsp_coeff_base_reg(ctl, ®, 0); if (ret) return ret;
@@ -712,14 +713,14 @@ int cs_dsp_coeff_write_acked_control(struct cs_dsp_coeff_ctl *ctl, unsigned int EXPORT_SYMBOL_GPL(cs_dsp_coeff_write_acked_control);
static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, - const void *buf, size_t len) + unsigned int off, const void *buf, size_t len) { struct cs_dsp *dsp = ctl->dsp; void *scratch; int ret; unsigned int reg;
- ret = cs_dsp_coeff_base_reg(ctl, ®); + ret = cs_dsp_coeff_base_reg(ctl, ®, off); if (ret) return ret;
@@ -745,6 +746,7 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, /** * cs_dsp_coeff_write_ctrl() - Writes the given buffer to the given coefficient control * @ctl: pointer to coefficient control + * @off: word offset at which data should be written * @buf: the buffer to write to the given control * @len: the length of the buffer in bytes * @@ -752,7 +754,8 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, * * Return: Zero for success, a negative number on error. */ -int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, const void *buf, size_t len) +int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, + unsigned int off, const void *buf, size_t len) { int ret = 0;
@@ -761,27 +764,31 @@ int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, const void *buf, size_ if (!ctl) return -ENOENT;
+ if (len + off * sizeof(u32) > ctl->len) + return -EINVAL; + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) ret = -EPERM; else if (buf != ctl->cache) - memcpy(ctl->cache, buf, len); + memcpy(ctl->cache + off * sizeof(u32), buf, len);
ctl->set = 1; if (ctl->enabled && ctl->dsp->running) - ret = cs_dsp_coeff_write_ctrl_raw(ctl, buf, len); + ret = cs_dsp_coeff_write_ctrl_raw(ctl, off, buf, len);
return ret; } EXPORT_SYMBOL_GPL(cs_dsp_coeff_write_ctrl);
-static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len) +static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, + unsigned int off, void *buf, size_t len) { struct cs_dsp *dsp = ctl->dsp; void *scratch; int ret; unsigned int reg;
- ret = cs_dsp_coeff_base_reg(ctl, ®); + ret = cs_dsp_coeff_base_reg(ctl, ®, off); if (ret) return ret;
@@ -807,6 +814,7 @@ static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, void *buf, s /** * cs_dsp_coeff_read_ctrl() - Reads the given coefficient control into the given buffer * @ctl: pointer to coefficient control + * @off: word offset at which data should be read * @buf: the buffer to store to the given control * @len: the length of the buffer in bytes * @@ -814,7 +822,8 @@ static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, void *buf, s * * Return: Zero for success, a negative number on error. */ -int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len) +int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, + unsigned int off, void *buf, size_t len) { int ret = 0;
@@ -823,17 +832,20 @@ int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len) if (!ctl) return -ENOENT;
+ if (len + off * sizeof(u32) > ctl->len) + return -EINVAL; + if (ctl->flags & WMFW_CTL_FLAG_VOLATILE) { if (ctl->enabled && ctl->dsp->running) - return cs_dsp_coeff_read_ctrl_raw(ctl, buf, len); + return cs_dsp_coeff_read_ctrl_raw(ctl, off, buf, len); else return -EPERM; } else { if (!ctl->flags && ctl->enabled && ctl->dsp->running) - ret = cs_dsp_coeff_read_ctrl_raw(ctl, ctl->cache, ctl->len); + ret = cs_dsp_coeff_read_ctrl_raw(ctl, 0, ctl->cache, ctl->len);
if (buf != ctl->cache) - memcpy(buf, ctl->cache, len); + memcpy(buf, ctl->cache + off * sizeof(u32), len); }
return ret; @@ -857,7 +869,7 @@ static int cs_dsp_coeff_init_control_caches(struct cs_dsp *dsp) * created so we don't need to do anything. */ if (!ctl->flags || (ctl->flags & WMFW_CTL_FLAG_READABLE)) { - ret = cs_dsp_coeff_read_ctrl_raw(ctl, ctl->cache, ctl->len); + ret = cs_dsp_coeff_read_ctrl_raw(ctl, 0, ctl->cache, ctl->len); if (ret < 0) return ret; } @@ -875,7 +887,7 @@ static int cs_dsp_coeff_sync_controls(struct cs_dsp *dsp) if (!ctl->enabled) continue; if (ctl->set && !(ctl->flags & WMFW_CTL_FLAG_VOLATILE)) { - ret = cs_dsp_coeff_write_ctrl_raw(ctl, ctl->cache, + ret = cs_dsp_coeff_write_ctrl_raw(ctl, 0, ctl->cache, ctl->len); if (ret < 0) return ret; diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h index 1ad1b173417a0..38b4da3ddfe4f 100644 --- a/include/linux/firmware/cirrus/cs_dsp.h +++ b/include/linux/firmware/cirrus/cs_dsp.h @@ -232,8 +232,10 @@ void cs_dsp_init_debugfs(struct cs_dsp *dsp, struct dentry *debugfs_root); void cs_dsp_cleanup_debugfs(struct cs_dsp *dsp);
int cs_dsp_coeff_write_acked_control(struct cs_dsp_coeff_ctl *ctl, unsigned int event_id); -int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, const void *buf, size_t len); -int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, void *buf, size_t len); +int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, + const void *buf, size_t len); +int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, + void *buf, size_t len); struct cs_dsp_coeff_ctl *cs_dsp_get_ctl(struct cs_dsp *dsp, const char *name, int type, unsigned int alg);
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 404717e30f44d..f084b093cff64 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -401,7 +401,7 @@ static int wm_coeff_put(struct snd_kcontrol *kctl, int ret = 0;
mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_write_ctrl(cs_ctl, p, cs_ctl->len); + ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, p, cs_ctl->len); mutex_unlock(&cs_ctl->dsp->pwr_lock);
return ret; @@ -421,7 +421,7 @@ static int wm_coeff_tlv_put(struct snd_kcontrol *kctl, if (copy_from_user(cs_ctl->cache, bytes, size)) ret = -EFAULT; else - ret = cs_dsp_coeff_write_ctrl(cs_ctl, cs_ctl->cache, size); + ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, cs_ctl->cache, size);
mutex_unlock(&cs_ctl->dsp->pwr_lock);
@@ -464,7 +464,7 @@ static int wm_coeff_get(struct snd_kcontrol *kctl, int ret;
mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_read_ctrl(cs_ctl, p, cs_ctl->len); + ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, p, cs_ctl->len); mutex_unlock(&cs_ctl->dsp->pwr_lock);
return ret; @@ -481,7 +481,7 @@ static int wm_coeff_tlv_get(struct snd_kcontrol *kctl,
mutex_lock(&cs_ctl->dsp->pwr_lock);
- ret = cs_dsp_coeff_read_ctrl(cs_ctl, cs_ctl->cache, size); + ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, cs_ctl->cache, size);
if (!ret && copy_to_user(bytes, cs_ctl->cache, size)) ret = -EFAULT; @@ -684,7 +684,7 @@ int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, int type, if (len > cs_ctl->len) return -EINVAL;
- ret = cs_dsp_coeff_write_ctrl(cs_ctl, buf, len); + ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, buf, len); if (ret) return ret;
@@ -723,7 +723,7 @@ int wm_adsp_read_ctl(struct wm_adsp *dsp, const char *name, int type, if (len > cs_ctl->len) return -EINVAL;
- return cs_dsp_coeff_read_ctrl(cs_ctl, buf, len); + return cs_dsp_coeff_read_ctrl(cs_ctl, 0, buf, len); } EXPORT_SYMBOL_GPL(wm_adsp_read_ctl);
@@ -1432,7 +1432,7 @@ static int wm_adsp_buffer_parse_coeff(struct cs_dsp_coeff_ctl *cs_ctl) int ret, i;
for (i = 0; i < 5; ++i) { - ret = cs_dsp_coeff_read_ctrl(cs_ctl, &coeff_v1, sizeof(coeff_v1)); + ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, &coeff_v1, sizeof(coeff_v1)); if (ret < 0) return ret;
Some firmwares contain controls intended to convey firmware state back to the host. Whilst more infrastructure will probably be needed for these in time, as a first step allow creation of the controls, so said firmwares arn't completely rejected.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 1 + include/linux/firmware/cirrus/wmfw.h | 1 + 2 files changed, 2 insertions(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 5fe08de91ecd3..3814cbba0a544 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -1177,6 +1177,7 @@ static int cs_dsp_parse_coeff(struct cs_dsp *dsp, return -EINVAL; break; case WMFW_CTL_TYPE_HOSTEVENT: + case WMFW_CTL_TYPE_FWEVENT: ret = cs_dsp_check_coeff_flags(dsp, &coeff_blk, WMFW_CTL_FLAG_SYS | WMFW_CTL_FLAG_VOLATILE | diff --git a/include/linux/firmware/cirrus/wmfw.h b/include/linux/firmware/cirrus/wmfw.h index a19bf7c6fc8b0..74e5a4f6c13a0 100644 --- a/include/linux/firmware/cirrus/wmfw.h +++ b/include/linux/firmware/cirrus/wmfw.h @@ -29,6 +29,7 @@ #define WMFW_CTL_TYPE_ACKED 0x1000 /* acked control */ #define WMFW_CTL_TYPE_HOSTEVENT 0x1001 /* event control */ #define WMFW_CTL_TYPE_HOST_BUFFER 0x1002 /* host buffer pointer */ +#define WMFW_CTL_TYPE_FWEVENT 0x1004 /* firmware event control */
struct wmfw_header { char magic[4];
participants (3)
-
Charles Keepax -
Mark Brown -
Simon Trimmer