[v9,1/5] firmware: cs_dsp: Add write sequencer interface

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@opensource.cirrus.com>
Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 drivers/firmware/cirrus/cs_dsp.c       | 263 +++++++++++++++++++++++++
 include/linux/firmware/cirrus/cs_dsp.h |  28 +++
 2 files changed, 291 insertions(+)

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@opensource.cirrus.com>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>

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@opensource.cirrus.com>");
>  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

Hi Charles,

On Mon, Mar 11, 2024 at 10:14:17AM +0000, Charles Keepax wrote:
> On Sun, Mar 10, 2024 at 02:12:16PM -0500, Jeff LaBundy wrote:
> > On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> > > +	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.
> > 
> 
> I disagree here. This is the write function, there is no guarantee
> this is even called at probe time. This is generic code going
> into the DSP library and will presumably get re-used for different
> purposes and on different parts in the future. Freeing on the error
> path is much safer here.

Agreed that this won't be called during probe; all I mean to say is
that I don't see any issue in hanging on to a bit of memory while the
device is in an error state, before the module is unloaded and devres
unrolls all of the device-managed resources.

It's also perfectly fine to leave this as-is; the existing method is
functionally correct, and I understand the perspective of having the
generic library clean up immediately. If that's the intent however,
the same error handling needs to be applied in cs_dsp_populate_wseq();
currently only cs_dsp_wseq_write() calls devm_kfree() on error. Since
L50 uses asynchronous FW loading, neither are called until after the
device probes.

> 
> > > +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.
> 
> I would be inclined to make cs_dsp_wseq_init function like this
> one personally, rather than the other way around. Generally best
> to return if there is no clean up to do.

Makes sense to me.

> 
> Thanks,
> Charles

Kind regards,
Jeff LaBundy

On Thu, Mar 14, 2024 at 09:40:34AM -0500, Jeff LaBundy wrote:
> On Mon, Mar 11, 2024 at 10:14:17AM +0000, Charles Keepax wrote:
> > On Sun, Mar 10, 2024 at 02:12:16PM -0500, Jeff LaBundy wrote:
> > > On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> > I disagree here. This is the write function, there is no guarantee
> > this is even called at probe time. This is generic code going
> > into the DSP library and will presumably get re-used for different
> > purposes and on different parts in the future. Freeing on the error
> > path is much safer here.
> 
> Agreed that this won't be called during probe; all I mean to say is
> that I don't see any issue in hanging on to a bit of memory while the
> device is in an error state, before the module is unloaded and devres
> unrolls all of the device-managed resources.

I think that's the problem the assumption that all users will
completely bork the device if this operation fails. Likely but
far from certain.

> It's also perfectly fine to leave this as-is; the existing method is
> functionally correct, and I understand the perspective of having the
> generic library clean up immediately. If that's the intent however,
> the same error handling needs to be applied in cs_dsp_populate_wseq();
> currently only cs_dsp_wseq_write() calls devm_kfree() on error. Since
> L50 uses asynchronous FW loading, neither are called until after the
> device probes.
> 

Hmm... yes I guess in my head I wasn't thinking so much about
populate being called at runtime, but I am inclined to agree. We
should probably update populate to also free on the error path.

Thanks,
Charles