Hi James,
On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> A write sequencer is a sequence of register addresses
> and values executed by some Cirrus DSPs following
> power state transitions.
>
> Add support for Cirrus drivers to update or add to a
> write sequencer present in firmware.
>
> Reviewed-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: James Ogletree <jogletre@xxxxxxxxxxxxxxxxxxxxx>
Truly excellent work here; thanks again for continuing to lead a
productive review. I think the overall design of the driver and
the way it is organized are solid now.
Apologies for not getting you feedback sooner; I have some minor
comments below and in a few other spots throughout the series. I
think it is ready after these last few points are addressed.
> ---
> drivers/firmware/cirrus/cs_dsp.c | 263 +++++++++++++++++++++++++
> include/linux/firmware/cirrus/cs_dsp.h | 28 +++
> 2 files changed, 291 insertions(+)
>
> diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
> index 79d4254d1f9b..90cd56d70c49 100644
> --- a/drivers/firmware/cirrus/cs_dsp.c
> +++ b/drivers/firmware/cirrus/cs_dsp.c
> @@ -275,6 +275,12 @@
> #define HALO_MPU_VIO_ERR_SRC_MASK 0x00007fff
> #define HALO_MPU_VIO_ERR_SRC_SHIFT 0
>
> +/*
> + * Write Sequence
> + */
> +#define WSEQ_OP_MAX_WORDS 3
> +#define WSEQ_END_OF_SCRIPT 0xFFFFFF
> +
> struct cs_dsp_ops {
> bool (*validate_version)(struct cs_dsp *dsp, unsigned int version);
> unsigned int (*parse_sizes)(struct cs_dsp *dsp,
> @@ -3339,6 +3345,263 @@ int cs_dsp_chunk_read(struct cs_dsp_chunk *ch, int nbits)
> }
> EXPORT_SYMBOL_NS_GPL(cs_dsp_chunk_read, FW_CS_DSP);
>
> +
> +struct cs_dsp_wseq_op {
> + struct list_head list;
> + u32 address;
> + u32 data;
> + u16 offset;
> + u8 operation;
> +};
> +
> +static int cs_dsp_populate_wseq(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq)
> +{
> + struct cs_dsp_wseq_op *op = NULL;
> + struct cs_dsp_chunk ch;
> + u8 *words;
> + int ret;
> +
> + if (!wseq->ctl) {
> + cs_dsp_err(dsp, "No control for write sequence\n");
> + return -EINVAL;
> + }
> +
> + words = kzalloc(wseq->ctl->len, GFP_KERNEL);
> + if (!words)
> + return -ENOMEM;
> +
> + ret = cs_dsp_coeff_read_ctrl(wseq->ctl, 0, words, wseq->ctl->len);
> + if (ret) {
> + cs_dsp_err(dsp, "Failed to read %s: %d\n", wseq->ctl->subname, ret);
> + goto err_free;
> + }
> +
> + INIT_LIST_HEAD(&wseq->ops);
> +
> + ch = cs_dsp_chunk(words, wseq->ctl->len);
> +
> + while (!cs_dsp_chunk_end(&ch)) {
> + op = devm_kzalloc(dsp->dev, sizeof(*op), GFP_KERNEL);
> + if (!op) {
> + ret = -ENOMEM;
> + goto err_free;
> + }
> +
> + op->offset = cs_dsp_chunk_bytes(&ch);
> + op->operation = cs_dsp_chunk_read(&ch, 8);
> +
> + switch (op->operation) {
> + case CS_DSP_WSEQ_END:
> + op->data = WSEQ_END_OF_SCRIPT;
> + break;
> + case CS_DSP_WSEQ_UNLOCK:
> + op->data = cs_dsp_chunk_read(&ch, 16);
> + break;
> + case CS_DSP_WSEQ_ADDR8:
> + op->address = cs_dsp_chunk_read(&ch, 8);
> + op->data = cs_dsp_chunk_read(&ch, 32);
> + break;
> + case CS_DSP_WSEQ_H16:
> + case CS_DSP_WSEQ_L16:
> + op->address = cs_dsp_chunk_read(&ch, 24);
> + op->data = cs_dsp_chunk_read(&ch, 16);
> + break;
> + case CS_DSP_WSEQ_FULL:
> + op->address = cs_dsp_chunk_read(&ch, 32);
> + op->data = cs_dsp_chunk_read(&ch, 32);
> + break;
> + default:
> + ret = -EINVAL;
> + cs_dsp_err(dsp, "Unsupported op: %X\n", op->operation);
> + goto err_free;
> + }
> +
> + list_add_tail(&op->list, &wseq->ops);
> +
> + if (op->operation == CS_DSP_WSEQ_END)
> + break;
> + }
> +
> + if (op && op->operation != CS_DSP_WSEQ_END) {
> + cs_dsp_err(dsp, "Write sequence missing end terminator\n");
> + ret = -ENOENT;
> + }
> +
> +err_free:
> + kfree(words);
> +
> + return ret;
> +}
> +
> +/**
> + * cs_dsp_wseq_init() - Initialize write sequences contained within the loaded DSP firmware
> + * @dsp: Pointer to DSP structure
> + * @wseqs: List of write sequences to initialize
> + * @num_wseqs: Number of write sequences to initialize
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs)
> +{
> + int i, ret = 0;
> +
> + lockdep_assert_held(&dsp->pwr_lock);
> +
> + for (i = 0; i < num_wseqs; i++) {
> + ret = cs_dsp_populate_wseq(dsp, &wseqs[i]);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(cs_dsp_wseq_init, FW_CS_DSP);
> +
> +static struct cs_dsp_wseq_op *cs_dsp_wseq_find_op(u32 addr, u8 op_code,
> + struct list_head *wseq_ops)
> +{
> + struct cs_dsp_wseq_op *op;
> +
> + list_for_each_entry(op, wseq_ops, list) {
> + if (op->operation == op_code && op->address == addr)
> + return op;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * cs_dsp_wseq_write() - Add or update an entry in a write sequence
> + * @dsp: Pointer to a DSP structure
> + * @wseq: Write sequence to write to
> + * @addr: Address of the register to be written to
> + * @data: Data to be written
> + * @op_code: The type of operation of the new entry
> + * @update: If true, searches for the first entry in the write sequence with
> + * the same address and op_code, and replaces it. If false, creates a new entry
> + * at the tail.
> + *
> + * This function formats register address and value pairs into the format
> + * required for write sequence entries, and either updates or adds the
> + * new entry into the write sequence.
> + *
> + * If update is set to true and no matching entry is found, it will add a new entry.
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> + u32 addr, u32 data, u8 op_code, bool update)
> +{
> + struct cs_dsp_wseq_op *op_end, *op_new = NULL;
> + u32 words[WSEQ_OP_MAX_WORDS];
> + struct cs_dsp_chunk ch;
Nit: 'chunk' is probably a more descriptive variable name; 'ch' is ambiguous
and too easily confused with other common things (e.g. channel).
> + int new_op_size, ret;
> +
> + if (update)
> + op_new = cs_dsp_wseq_find_op(addr, op_code, &wseq->ops);
> +
> + /* If entry to update is not found, treat it as a new operation */
> + if (!op_new) {
> + op_end = cs_dsp_wseq_find_op(0, CS_DSP_WSEQ_END, &wseq->ops);
> + if (!op_end) {
> + cs_dsp_err(dsp, "Missing write sequence list terminator\n");
> + return -EINVAL;
> + }
> +
> + op_new = devm_kzalloc(dsp->dev, sizeof(*op_new), GFP_KERNEL);
> + if (!op_new)
> + return -ENOMEM;
> +
> + op_new->operation = op_code;
> + op_new->address = addr;
> + op_new->offset = op_end->offset;
> + update = false;
> + }
> +
> + op_new->data = data;
> +
> + ch = cs_dsp_chunk(words, sizeof(words));
> + cs_dsp_chunk_write(&ch, 8, op_new->operation);
Nit: maybe a newline here?
> + switch (op_code) {
> + case CS_DSP_WSEQ_FULL:
> + cs_dsp_chunk_write(&ch, 32, op_new->address);
> + cs_dsp_chunk_write(&ch, 32, op_new->data);
> + break;
> + case CS_DSP_WSEQ_L16:
> + case CS_DSP_WSEQ_H16:
> + cs_dsp_chunk_write(&ch, 24, op_new->address);
> + cs_dsp_chunk_write(&ch, 16, op_new->data);
> + break;
> + default:
> + ret = -EINVAL;
> + cs_dsp_err(dsp, "Op code not supported: %X\n", op_code);
> + goto op_new_free;
There is no need to drop down and call devm_kfree() here; it's sufficient
to simply return -EINVAL. The devres core will free any instances of
cs_dsp_wseq_op when the driver is unbound.
I imagine you're calling devm_kfree() to protect against the case where
the driver successfully probes, but the contents of the firmware are found
to be invalid later. In that case, yes, this newly allocated memory will
persist throughout the length of the driver's life.
That's an error condition though; if it happens, the customer will surely
remove the module, correct the .wmfw or .bin file, then insert the module
again. All we need to do here is make sure that memory does not leak after
the module is removed, and device-managed allocation already handles this.
> + }
> +
> + new_op_size = cs_dsp_chunk_bytes(&ch);
> +
> + if (!update) {
> + if (wseq->ctl->len - op_end->offset < new_op_size) {
> + cs_dsp_err(dsp, "Not enough memory in write sequence for entry\n");
> + ret = -ENOMEM;
> + goto op_new_free;
Same here. Maybe you want to return -E2BIG here so the caller can tell the
difference between there not being enough memory available to allocate a
new instance of cs_dsp_wseq_op, vs. one having already been allocated, but
not big enough.
> + }
> +
> + op_end->offset += new_op_size;
> +
> + ret = cs_dsp_coeff_write_ctrl(wseq->ctl, op_end->offset / sizeof(u32),
> + &op_end->data, sizeof(u32));
> + if (ret)
> + goto op_new_free;
And here; just return ret.
> +
> + list_add_tail(&op_new->list, &op_end->list);
> + }
> +
> + ret = cs_dsp_coeff_write_ctrl(wseq->ctl, op_new->offset / sizeof(u32),
> + words, new_op_size);
> + if (ret)
> + goto op_new_free;
> +
> + return 0;
Here too. This clean-up avoids the rather unsightly design pattern of placing
a tear-down path after the function's primary return path.
> +
> +op_new_free:
> + devm_kfree(dsp->dev, op_new);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(cs_dsp_wseq_write, FW_CS_DSP);
> +
> +/**
> + * cs_dsp_wseq_multi_write() - Add or update multiple entries in the write sequence
> + * @dsp: Pointer to a DSP structure
> + * @wseq: Write sequence to write to
> + * @reg_seq: List of address-data pairs
> + * @num_regs: Number of address-data pairs
> + * @op_code: The types of operations of the new entries
> + * @update: If true, searches for the first entry in the write sequence with the same
> + * address and op code, and replaces it. If false, creates a new entry at the tail.
> + *
> + * This function calls cs_dsp_wseq_write() for multiple address-data pairs.
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> + const struct reg_sequence *reg_seq, int num_regs,
> + u8 op_code, bool update)
> +{
> + int ret, i;
> +
> + for (i = 0; i < num_regs; i++) {
> + ret = cs_dsp_wseq_write(dsp, wseq, reg_seq[i].reg,
> + reg_seq[i].def, update, op_code);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
This is absolutely a nit-pick, but in cs_dsp_wseq_init(), you use the pattern
of breaking on error and always returning ret; here you return on error. Both
are functionally equivalent but it would be nice to be consistent in terms of
style.
If you do opt to match cs_dsp_wseq_multi_write() to cs_dsp_wseq_init(), I do
think you'll need to initialize ret to zero here as well to avoid a compiler
warning for the num_regs = 0 case.
> +}
> +EXPORT_SYMBOL_NS_GPL(cs_dsp_wseq_multi_write, FW_CS_DSP);
> +
> MODULE_DESCRIPTION("Cirrus Logic DSP Support");
> MODULE_AUTHOR("Simon Trimmer <simont@xxxxxxxxxxxxxxxxxxxxx>");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
> index 29cd11d5a3cf..cfeab75772f6 100644
> --- a/include/linux/firmware/cirrus/cs_dsp.h
> +++ b/include/linux/firmware/cirrus/cs_dsp.h
> @@ -42,6 +42,16 @@
> #define CS_DSP_ACKED_CTL_MIN_VALUE 0
> #define CS_DSP_ACKED_CTL_MAX_VALUE 0xFFFFFF
>
> +/*
> + * Write sequence operation codes
> + */
> +#define CS_DSP_WSEQ_FULL 0x00
> +#define CS_DSP_WSEQ_ADDR8 0x02
> +#define CS_DSP_WSEQ_L16 0x04
> +#define CS_DSP_WSEQ_H16 0x05
> +#define CS_DSP_WSEQ_UNLOCK 0xFD
> +#define CS_DSP_WSEQ_END 0xFF
> +
> /**
> * struct cs_dsp_region - Describes a logical memory region in DSP address space
> * @type: Memory region type
> @@ -255,6 +265,24 @@ struct cs_dsp_alg_region *cs_dsp_find_alg_region(struct cs_dsp *dsp,
>
> const char *cs_dsp_mem_region_name(unsigned int type);
>
> +/**
> + * struct cs_dsp_wseq - Describes a write sequence
> + * @name: Name of cs_dsp control
> + * @ctl: Write sequence cs_dsp control
> + * @ops: Operations contained within this write sequence
> + */
> +struct cs_dsp_wseq {
> + struct cs_dsp_coeff_ctl *ctl;
> + struct list_head ops;
> +};
> +
> +int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs);
> +int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq, u32 addr, u32 data,
> + u8 op_code, bool update);
> +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> + const struct reg_sequence *reg_seq, int num_regs,
> + u8 op_code, bool update);
> +
> /**
> * struct cs_dsp_chunk - Describes a buffer holding data formatted for the DSP
> * @data: Pointer to underlying buffer memory
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
