| Message ID | 20240104223643.876292-1-jogletre@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add support for CS40L50 | expand |
On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote: > Introduce support for Cirrus Logic Device CS40L50: a > haptic driver with waveform memory, integrated DSP, > and closed-loop algorithms. > > The MFD component registers and initializes the device. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- > +config MFD_CS40L50_CORE > + tristate > + select MFD_CORE > + select FW_CS_DSP > + select REGMAP_IRQ > + > +config MFD_CS40L50_I2C > + tristate "Cirrus Logic CS40L50 (I2C)" > + select REGMAP_I2C > + select MFD_CS40L50_CORE > + depends on I2C > + help > + Select this to support the Cirrus Logic CS40L50 Haptic > + Driver over I2C. > + > + This driver can be built as a module. If built as a module it will be > + called "cs40l50-i2c". > + > +config MFD_CS40L50_SPI > + tristate "Cirrus Logic CS40L50 (SPI)" > + select REGMAP_SPI > + select MFD_CS40L50_CORE > + depends on SPI > + help > + Select this to support the Cirrus Logic CS40L50 Haptic > + Driver over SPI. > + > + This driver can be built as a module. If built as a module it will be > + called "cs40l50-spi". > + Generally the order in Kconfigs should be alphabetical, probably up around Cirrus Madera stuff would make most sense. > +static int cs40l50_dsp_init(struct cs40l50 *cs40l50) > +{ > + int err; > + > + cs40l50->dsp.num = 1; > + cs40l50->dsp.type = WMFW_HALO; > + cs40l50->dsp.dev = cs40l50->dev; > + cs40l50->dsp.regmap = cs40l50->regmap; > + cs40l50->dsp.base = CS40L50_CORE_BASE; > + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID; > + cs40l50->dsp.mem = cs40l50_dsp_regions; > + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions); > + cs40l50->dsp.no_core_startstop = true; > + > + err = cs_dsp_halo_init(&cs40l50->dsp); > + if (err) > + return err; > + > + return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove, > + &cs40l50->dsp); Hmm... I notice you use this for both dsp_remove and dsp_power_down. Are you sure devm will guarantee those are called in the right order? Its not immediately clear to me that would be have to be the case. > +static irqreturn_t cs40l50_irq_handler(int irq, void *data) > +{ > + struct cs40l50 *cs40l50 = data; > + int err; > + > + mutex_lock(&cs40l50->lock); > + > + if (irq == cs40l50_irqs[0].virq) > + err = cs40l50_process_dsp_queue(cs40l50); > + else > + err = cs40l50_handle_hw_err(cs40l50, irq); Feels kinda weird to assign the same handler to every IRQ and then depending on which IRQ it was call a different function. Would it not be simpler just to assign a different handler? > +static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50) > +{ > + int err; > + > + mutex_lock(&cs40l50->lock); > + > + if (cs40l50->patch) { > + /* Stop core if loading patch file */ > + err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core, > + ARRAY_SIZE(cs40l50_stop_core)); > + if (err) > + goto err_mutex; > + } > + > + err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw", > + cs40l50->bin, "cs40l50.bin", "cs40l50"); > + if (err) > + goto err_mutex; > + > + err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down, > + &cs40l50->dsp); > + if (err) > + goto err_mutex; > + > + if (cs40l50->patch) { > + /* Resume core after loading patch file */ > + err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL, > + CS40L50_CLOCK_ENABLE); This feels like this needs a comment, why are we skipping the normal DSP init and doing it manually (this appears to be the same writes start_core would have done)? I assume its something to do with what you are really doing is you don't want lock_memory to run? > +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50) > +{ > + u32 nwaves; > + int err; > + > + if (cs40l50->bin) { > + /* Log number of effects if wavetable was loaded */ > + err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves); > + if (err) > + return err; > + > + dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves); Kinda nervous about the fact we access all these DSP controls directly through address, rather than using the DSP control accessors, we have the accessors for a reason. They manage things like access permissions etc. and historically, the firmware guys have not been able to guarantee these remain in consistent locations between firmware versions. I guess this is so you can access them even in the case of the ROM firmware, but you could have a meta-data only firmware file that you load in that case to give you the controls. I don't feel the need to NAK the driver based on this but please think about this very carefully it's a strange way to use the DSP controls, and feels likely to cause problems to me. It is also quite hostile to fixing it in the future since as you are not using the controls no one will be checking that things like the access flags in the firmware are set correctly, which is annoying if the decision has to be reversed later since there will likely be a bunch of broken firmwares already in the field. > +int cs40l50_probe(struct cs40l50 *cs40l50) > +{ > + struct device *dev = cs40l50->dev; > + int err; > + > + mutex_init(&cs40l50->lock); > + > + cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(cs40l50->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio), > + "Failed getting reset GPIO\n"); > + > + err = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(cs40l50_supplies), > + cs40l50_supplies); > + if (err) > + return dev_err_probe(dev, err, "Failed getting supplies\n"); > + > + /* Ensure minimum reset pulse width */ > + usleep_range(CS40L50_RESET_PULSE_US, CS40L50_RESET_PULSE_US + 100); > + > + gpiod_set_value_cansleep(cs40l50->reset_gpio, 0); > + > + /* Wait for control port to be ready */ > + usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100); > + > + err = cs40l50_dsp_init(cs40l50); > + if (err) > + return dev_err_probe(dev, err, "Failed to initialize DSP\n"); > + > + cs40l50_pm_runtime_setup(dev); > + > + err = cs40l50_get_model(cs40l50); > + if (err) > + return dev_err_probe(dev, err, "Failed to get part number\n"); > + > + err = cs40l50_irq_init(cs40l50); > + if (err) > + return dev_err_probe(dev, err, "Failed to initialize IRQs\n"); > + > + err = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_FW, > + dev, GFP_KERNEL, cs40l50, cs40l50_request_patch); > + if (err) > + return dev_err_probe(dev, err, "Failed to request %s\n", CS40L50_FW); > + > + err = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs, > + ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL); > + if (err) > + return dev_err_probe(dev, err, "Failed to add sub devices\n"); > + Do you want to add the child devices here? Or after the firmware init has been done? If you do it here then the child devices may well probe and become available before all the setup you have in the DSP loading stuff is done. What happens if one of those drivers tries to do something before init is complete? > +static int cs40l50_runtime_resume(struct device *dev) > +{ > + struct cs40l50 *cs40l50 = dev_get_drvdata(dev); > + int err, i; > + u32 val; > + > + /* Device NAKs when exiting hibernation, so optionally retry here. */ > + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) { > + err = regmap_write(cs40l50->regmap, CS40L50_DSP_QUEUE, > + CS40L50_PREVENT_HIBER); > + if (!err) > + break; > + > + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100); > + } Are you comfortable with the behaviour here? If the chip fails to respond before the TIMEOUT, you will proceed on and do the read loop. Since the read loop is just looking for a zero in the queue, it looks like if for some reason the chip was too slow to respond this function would succeed despite the chip never receiving the PREVENT_HIBER. Would it not be safer to skip the read loop if you fail to send the PREVENT_HIBER? > + > + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) { > + err = regmap_read(cs40l50->regmap, CS40L50_DSP_QUEUE, &val); > + if (!err && val == 0) > + return 0; > + > + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100); > + } I am not sure if the ignoring errors is important here (as it is for the first loop), but if it is a comment should be added to say why, and if it isn't, couldn't this be a regmap_read_poll_timeout instead of a manual loop? > +++ b/drivers/mfd/cs40l50-spi.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * CS40L50 Advanced Haptic Driver with waveform memory, > + * integrated DSP, and closed-loop algorithms > + * > + * Copyright 2023 Cirrus Logic, Inc. > + * > + * Author: James Ogletree <james.ogletree@cirrus.com> > + */ > + > +#include <linux/mfd/cs40l50.h> > +#include <linux/mfd/spi.h> Should be linux/spi/spi.h, make sure your build testing the whole patch. > +#ifndef __CS40L50_H__ > +#define __CS40L50_H__ > + > +#include <linux/firmware/cirrus/cs_dsp.h> > +#include <linux/gpio/consumer.h> > +#include <linux/pm.h> > +#include <linux/regmap.h> > + > +/* Power Supply Configuration */ > +#define CS40L50_BLOCK_ENABLES2 0x201C > +#define CS40L50_ERR_RLS 0x2034 > +#define CS40L50_PWRMGT_CTL 0x2900 > +#define CS40L50_BST_LPMODE_SEL 0x3810 > +#define CS40L50_DCM_LOW_POWER 0x1 > +#define CS40L50_OVERTEMP_WARN 0x4000010 > + > +/* Interrupts */ > +#define CS40L50_IRQ1_INT_1 0xE010 > +#define CS40L50_IRQ1_MASK_1 0xE090 > +#define CS40L50_IRQ1_MASK_2 0xE094 > +#define CS40L50_IRQ1_MASK_20 0xE0DC > +#define CS40L50_IRQ_MASK_2_OVERRIDE 0xFFDF7FFF > +#define CS40L50_IRQ_MASK_20_OVERRIDE 0x15C01000 > +#define CS40L50_IRQ1_INT_1_OFFSET (4 * 0) > +#define CS40L50_IRQ1_INT_2_OFFSET (4 * 1) > +#define CS40L50_IRQ1_INT_8_OFFSET (4 * 7) > +#define CS40L50_IRQ1_INT_9_OFFSET (4 * 8) > +#define CS40L50_IRQ1_INT_10_OFFSET (4 * 9) > +#define CS40L50_IRQ1_INT_18_OFFSET (4 * 17) > +#define CS40L50_GLOBAL_ERR_RLS_SET BIT(11) > +#define CS40L50_GLOBAL_ERR_RLS_CLEAR 0 > +#define CS40L50_AMP_SHORT_MASK BIT(31) > +#define CS40L50_DSP_QUEUE_MASK BIT(21) > +#define CS40L50_TEMP_ERR_MASK BIT(31) > +#define CS40L50_BST_UVP_MASK BIT(6) > +#define CS40L50_BST_SHORT_MASK BIT(7) > +#define CS40L50_BST_ILIMIT_MASK BIT(18) > +#define CS40L50_UVLO_VDDBATT_MASK BIT(16) > +#define CS40L50_GLOBAL_ERROR_MASK BIT(15) > + > +enum cs40l50_irq_list { > + CS40L50_DSP_QUEUE_IRQ, > + CS40L50_GLOBAL_ERROR_IRQ, > + CS40L50_UVLO_VDDBATT_IRQ, > + CS40L50_BST_ILIMIT_IRQ, > + CS40L50_BST_SHORT_IRQ, > + CS40L50_BST_UVP_IRQ, > + CS40L50_TEMP_ERR_IRQ, > + CS40L50_AMP_SHORT_IRQ, > +}; > + > +/* DSP */ > +#define CS40L50_XMEM_PACKED_0 0x2000000 > +#define CS40L50_XMEM_UNPACKED24_0 0x2800000 > +#define CS40L50_SYS_INFO_ID 0x25E0000 > +#define CS40L50_RAM_INIT 0x28021DC > +#define CS40L50_DSP_QUEUE_WT 0x28042C8 > +#define CS40L50_DSP_QUEUE_RD 0x28042CC > +#define CS40L50_POWER_ON_WSEQ 0x2804320 > +#define CS40L50_NUM_WAVES 0x280CB4C > +#define CS40L50_CORE_BASE 0x2B80000 > +#define CS40L50_CCM_CORE_CONTROL 0x2BC1000 > +#define CS40L50_YMEM_PACKED_0 0x2C00000 > +#define CS40L50_YMEM_UNPACKED24_0 0x3400000 > +#define CS40L50_PMEM_0 0x3800000 > +#define CS40L50_MEM_RDY_HW 0x2 > +#define CS40L50_RAM_INIT_FLAG 0x1 > +#define CS40L50_CLOCK_DISABLE 0x80 > +#define CS40L50_CLOCK_ENABLE 0x281 > +#define CS40L50_DSP_POLL_US 1000 > +#define CS40L50_DSP_TIMEOUT_COUNT 100 > +#define CS40L50_RESET_PULSE_US 2200 > +#define CS40L50_CP_READY_US 3100 > +#define CS40L50_AUTOSUSPEND_MS 2000 > +#define CS40L50_PSEQ_SIZE 200 > + > +/* DSP Commands */ > +#define CS40L50_DSP_QUEUE_BASE 0x11004 > +#define CS40L50_DSP_QUEUE_END 0x1101C > +#define CS40L50_DSP_QUEUE 0x11020 > +#define CS40L50_PREVENT_HIBER 0x2000003 > +#define CS40L50_ALLOW_HIBER 0x2000004 > +#define CS40L50_START_I2S 0x3000002 > +#define CS40L50_OWT_PUSH 0x3000008 > +#define CS40L50_STOP_PLAYBACK 0x5000000 > +#define CS40L50_OWT_DELETE 0xD000000 > + > +/* Firmware files */ > +#define CS40L50_FW "cs40l50.wmfw" > +#define CS40L50_WT "cs40l50.bin" > + > +/* Device */ > +#define CS40L50_DEVID 0x0 > +#define CS40L50_REVID 0x4 > +#define CS40L50_DEVID_A 0x40A50 > +#define CS40L50_REVID_B0 0xB0 Admittedly a bit nitpicky but the tabbing here is all over the place, would be nicer to line up the values on these defines. Thanks, Charles
On Thu, Jan 04, 2024 at 10:36:38PM +0000, James Ogletree wrote: > Introduce support for Cirrus Logic Device CS40L50: a > haptic driver with waveform memory, integrated DSP, > and closed-loop algorithms. > > The ASoC driver enables I2S streaming to the device. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- > +#include <linux/mfd/cs40l50.h> > +#include <linux/pm_runtime.h> > +#include <sound/pcm_params.h> > +#include <sound/soc.h> Need to also include bitfield.h for FIELD_PREP etc. > +static int cs40l50_clk_en(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > +{ > + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm); > + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(comp); > + int ret; > + > + switch (event) { > + case SND_SOC_DAPM_POST_PMU: > + ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_STOP_PLAYBACK); > + if (ret) > + return ret; > + > + ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_START_I2S); > + if (ret) > + return ret; > + Feels weird that we don't wait for these two commands to be acknowledged by the DSP before doing the clock swap. Is that intentional? Is the DSP just guaranteed to be so fast it doesn't matter, in which case a comment would be nice. > +static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt) > +{ > + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component); > + > + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) > + return -EINVAL; > + > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { > + case SND_SOC_DAIFMT_NB_NF: > + codec->daifmt = 0; > + break; > + case SND_SOC_DAIFMT_NB_IF: > + codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK; > + break; > + case SND_SOC_DAIFMT_IB_NF: > + codec->daifmt = CS40L50_ASP_BCLK_INV_MASK; > + break; > + case SND_SOC_DAIFMT_IB_IF: > + codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK | CS40L50_ASP_BCLK_INV_MASK; > + break; > + default: > + return -EINVAL; > + } > + > + if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) == SND_SOC_DAIFMT_I2S) > + codec->daifmt |= FIELD_PREP(CS40L50_ASP_FMT_MASK, CS40L50_ASP_FMT_I2S); It feels unlikely the chip supports all formats with no additional settings? Probably should have a switch for the supported formats and return an error. > +static struct snd_soc_dai_driver cs40l50_dai[] = { > + { > + .name = "cs40l50-pcm", > + .id = 0, > + .playback = { > + .stream_name = "ASP Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_48000, > + .formats = CS40L50_FORMATS, > + }, > + .ops = &cs40l50_dai_ops, > + .symmetric_rate = 1, The symmetric_rate feels a bit redundant since we only have playback supported. Thanks, Charles
On Thu, Jan 04, 2024 at 10:36:34PM +0000, James Ogletree wrote: > A write sequencer is a sequence of register addresses > and values executed by some Cirrus DSPs following > power-up or exit from hibernation, used for avoiding > the overhead of bus transactions. > > Add support for Cirrus drivers to update or add to a > write sequencer present in firmware. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- > drivers/firmware/cirrus/cs_dsp.c | 261 +++++++++++++++++++++++++ > include/linux/firmware/cirrus/cs_dsp.h | 28 +++ > 2 files changed, 289 insertions(+) > > diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c > index 79d4254d1f9b..31a999f42e84 100644 > --- a/drivers/firmware/cirrus/cs_dsp.c > +++ b/drivers/firmware/cirrus/cs_dsp.c > @@ -275,6 +275,15 @@ > #define HALO_MPU_VIO_ERR_SRC_MASK 0x00007fff > #define HALO_MPU_VIO_ERR_SRC_SHIFT 0 > > +/* > + * Write Sequencer > + */ > +#define WSEQ_OP_FULL_WORDS 3 > +#define WSEQ_OP_X16_WORDS 2 > +#define WSEQ_OP_END_WORDS 1 > +#define WSEQ_OP_UNLOCK_WORDS 1 > +#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, > @@ -2233,6 +2242,111 @@ static int cs_dsp_create_name(struct cs_dsp *dsp) > return 0; > } > > +struct cs_dsp_wseq_op { > + struct list_head list; > + u32 words[3]; > + 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; > + int i, num_words, ret; > + u32 *words; > + > + if (wseq->size <= 0 || !wseq->reg) > + return -EINVAL; I would be tempted to give this an error message. > + > + words = kcalloc(wseq->size, sizeof(u32), GFP_KERNEL); > + if (!words) > + return -ENOMEM; > + > + INIT_LIST_HEAD(&wseq->ops); > + > + ret = regmap_raw_read(dsp->regmap, wseq->reg, words, > + wseq->size * sizeof(u32)); > + if (ret) > + goto err_free; > + > + ch = cs_dsp_chunk(words, wseq->size * sizeof(u32)); > + > + for (i = 0; i < wseq->size; i += num_words) { Can just drop num_words and i, and just do: while(!cs_dsp_chunk_end(&ch)) { Also allows you to drop the length defines for each OP. > + op = devm_kzalloc(dsp->dev, sizeof(*op), GFP_KERNEL); > + if (!op) { > + ret = -ENOMEM; > + goto err_free; > + } > + > + op->offset = ch.bytes; Use cs_dsp_chunk_bytes, cleaner to not access the internals directly incase we need to refactor them at some point. > + op->operation = cs_dsp_chunk_read(&ch, 8); > + > + switch (op->operation) { > + case CS_DSP_WSEQ_END: > + num_words = WSEQ_OP_END_WORDS; > + break; > + case CS_DSP_WSEQ_UNLOCK: > + num_words = WSEQ_OP_UNLOCK_WORDS; > + op->address = 0; > + op->data = cs_dsp_chunk_read(&ch, 16); > + break; > + case CS_DSP_WSEQ_ADDR8: > + case CS_DSP_WSEQ_H16: > + case CS_DSP_WSEQ_L16: > + num_words = WSEQ_OP_X16_WORDS; > + op->address = cs_dsp_chunk_read(&ch, 24); > + op->data = cs_dsp_chunk_read(&ch, 16); > + break; > + case CS_DSP_WSEQ_FULL: > + num_words = WSEQ_OP_FULL_WORDS; > + 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: %u\n", op->operation); > + goto err_free; > + } > + > + list_add(&op->list, &wseq->ops); > + > + if (op->operation == CS_DSP_WSEQ_END) > + break; > + } > + > + if (op && op->operation != CS_DSP_WSEQ_END) > + ret = -ENOENT; This definitely wants an error message, since this indicates the firmware is in a broken state, or the buffer passed in was not a write sequence. > +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; > + > + for (i = 0; i < num_wseqs; i++) { > + ret = cs_dsp_populate_wseq(dsp, &wseqs[i]); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cs_dsp_wseq_init); > + This location in the middle of the file is a bit weird, would be nicer to keep all the wseq stuff together move this down to the bottom of the file with the other functions. > static int cs_dsp_common_init(struct cs_dsp *dsp) > { > int ret; > @@ -3339,6 +3453,153 @@ int cs_dsp_chunk_read(struct cs_dsp_chunk *ch, int nbits) > } > EXPORT_SYMBOL_NS_GPL(cs_dsp_chunk_read, FW_CS_DSP); > > +static struct cs_dsp_wseq_op *cs_dsp_wseq_find_op(u8 op_code, u32 addr, > + 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 > + * @update: If true, searches for the first entry in the Write Sequencer with > + * the same address and op_code, and replaces it. If false, creates a new entry > + * at the tail. > + * @op_code: The type of operation of the new entry > + * > + * 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. > + * > + * 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, bool update, u8 op_code) Feels weird to have the op_code after the update flag in the order of arguments. addr, data and op_code are all parts of the new entry they should go together. Also be nice for the API to be consistent in the order it uses them, wseq_find_op is op_code, addr. > +{ > + struct cs_dsp_wseq_op *op_end, *op_new; > + struct cs_dsp_chunk ch; > + u32 wseq_bytes; > + int new_op_size, ret; > + > + if (update) { > + op_new = cs_dsp_wseq_find_op(op_code, addr, &wseq->ops); > + if (!op_new) > + return -EINVAL; This could also have an error message. > + } else { I would be tempted to pull the init of op_end up here like: op_end = cs_dsp_wseq_find_op(CS_DSP_WSEQ_END, 0, &wseq->ops); if (!op_end) { cs_dsp_err(dsp, "Missing write sequencer 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; And: op_new->offset = op_end->offset; > + } > + > + op_new->data = data; > + > + ch = cs_dsp_chunk((void *) op_new->words, > + WSEQ_OP_FULL_WORDS * sizeof(u32)); Since this is the only place you use op->words make it a local variable, its only 3 ints on the stack and it saves having 3 redundant ints in every op in the list. > + cs_dsp_chunk_write(&ch, 8, op_new->operation); > + 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; > + goto op_new_free; This also could have an error message, in general I would recommend have error messages for places where handling arguments from the user of the API. It is much more friendly for other developers, since they get immediate feedback if when they do something wrong when using the API. > + } > + With op_end pre-initialised this bit becomes: > + new_op_size = cs_dsp_chunk_bytes(&ch); > + > + wseq_bytes = wseq->size * sizeof(u32); > + > + if (wseq_bytes - op_end->offset < new_op_size) { > + cs_dsp_err(dsp, "Not enough memory in Write Sequencer for entry\n"); > + ret = -ENOMEM; > + goto op_new_free; > + } > + > + ret = regmap_raw_write(dsp->regmap, wseq->reg + op_new->offset, > + op_new->words, new_op_size); > + if (ret) > + goto op_new_free; > + > + if (!update) { Then pull the shift of op_end->offset into here: op_end->offset += new_op_size; > + ret = regmap_write(dsp->regmap, wseq->reg + op_end->offset, > + WSEQ_END_OF_SCRIPT); > + if (ret) > + goto op_new_free; > + > + list_add(&op_new->list, &wseq->ops); > + } > + > + return 0; > + > +op_new_free: > + devm_kfree(dsp->dev, op_new); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(cs_dsp_wseq_write); > + > +/** > + * 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 > + * @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. > + * @op_code: The types of operations of the new entries > + * > + * 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, bool update, u8 op_code) > +{ > + 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; > +} > +EXPORT_SYMBOL_GPL(cs_dsp_wseq_multi_write); > + > 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..d674fc061e9d 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 sequencer 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 > @@ -107,6 +117,18 @@ struct cs_dsp_coeff_ctl { > struct cs_dsp_ops; > struct cs_dsp_client_ops; > > +/** > + * struct cs_dsp_wseq - Describes a write sequence > + * @reg: Address of the head of the write sequence register > + * @size: Size of the write sequence in words The only user that wants the size in words is the loop counter that can be deleted. Is there any reason not to specify the size in bytes? > + * @ops: Operations contained within the write sequence > + */ > +struct cs_dsp_wseq { > + unsigned int reg; > + unsigned int size; > + struct list_head ops; > +}; > + > /** > * struct cs_dsp - Configuration and state of a Cirrus Logic DSP > * @name: The name of the DSP instance > @@ -254,6 +276,12 @@ struct cs_dsp_alg_region *cs_dsp_find_alg_region(struct cs_dsp *dsp, > int type, unsigned int id); > > const char *cs_dsp_mem_region_name(unsigned int type); > +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, > + bool update, u8 op_code); > +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq, > + const struct reg_sequence *reg_seq, > + int num_regs, bool update, u8 op_code); > This is also pretty spaced through the file, leave the defines where they are, but gather the struct and the funcs and move them to the bottom of the file in a group. Keeps all the API together when someone is looking it up. > /** > * struct cs_dsp_chunk - Describes a buffer holding data formatted for the DSP > -- > 2.25.1 > Overall my only other concern is still the register based API rather than control based. I guess there is some precident with the compressed stuff although that is at least taking addresses from the DSP and translating them into register addresses so the host can use them. Richard is off today, but back on Monday let me discuss with him then and we should have a chat too. Thanks, Charles
Hi Charles, Thank you for your excellent review. Anything not replied to will be adopted as-is in the next version. > On Jan 5, 2024, at 8:04 AM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote > >> +static int cs40l50_dsp_init(struct cs40l50 *cs40l50) >> +{ >> + int err; >> + >> + cs40l50->dsp.num = 1; >> + cs40l50->dsp.type = WMFW_HALO; >> + cs40l50->dsp.dev = cs40l50->dev; >> + cs40l50->dsp.regmap = cs40l50->regmap; >> + cs40l50->dsp.base = CS40L50_CORE_BASE; >> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID; >> + cs40l50->dsp.mem = cs40l50_dsp_regions; >> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions); >> + cs40l50->dsp.no_core_startstop = true; >> + >> + err = cs_dsp_halo_init(&cs40l50->dsp); >> + if (err) >> + return err; >> + >> + return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove, >> + &cs40l50->dsp); > > Hmm... I notice you use this for both dsp_remove and > dsp_power_down. Are you sure devm will guarantee those are called > in the right order? Its not immediately clear to me that would be > have to be the case. On my inspection of the devm code, actions are always added to the tail, and played back from head to tail on driver detach. > >> +static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50) >> +{ >> + int err; >> + >> + mutex_lock(&cs40l50->lock); >> + >> + if (cs40l50->patch) { >> + /* Stop core if loading patch file */ >> + err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core, >> + ARRAY_SIZE(cs40l50_stop_core)); >> + if (err) >> + goto err_mutex; >> + } >> + >> + err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw", >> + cs40l50->bin, "cs40l50.bin", "cs40l50"); >> + if (err) >> + goto err_mutex; >> + >> + err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down, >> + &cs40l50->dsp); >> + if (err) >> + goto err_mutex; >> + >> + if (cs40l50->patch) { >> + /* Resume core after loading patch file */ >> + err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL, >> + CS40L50_CLOCK_ENABLE); > > This feels like this needs a comment, why are we skipping the > normal DSP init and doing it manually (this appears to be the > same writes start_core would have done)? I assume its something to > do with what you are really doing is you don't want lock_memory > to run? The dsp struct uses cs_dsp_halo_ao_ops, made for self-booting DSPs, which has none of the ops used in cs_dsp_run(). The manual stop is because it is self-booting (already running you could say) but we need to stop the clock to patch the firmware. Please correct me if that is not right. >> +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50) >> +{ >> + u32 nwaves; >> + int err; >> + >> + if (cs40l50->bin) { >> + /* Log number of effects if wavetable was loaded */ >> + err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves); >> + if (err) >> + return err; >> + >> + dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves); > > Kinda nervous about the fact we access all these DSP controls > directly through address, rather than using the DSP control > accessors, we have the accessors for a reason. They manage things > like access permissions etc. and historically, the firmware > guys have not been able to guarantee these remain in consistent > locations between firmware versions. > > I guess this is so you can access them even in the case of the > ROM firmware, but you could have a meta-data only firmware file > that you load in that case to give you the controls. I don't > feel the need to NAK the driver based on this but please think > about this very carefully it's a strange way to use the DSP > controls, and feels likely to cause problems to me. It is also > quite hostile to fixing it in the future since as you are not > using the controls no one will be checking that things like the > access flags in the firmware are set correctly, which is annoying > if the decision has to be reversed later since there will likely > be a bunch of broken firmwares already in the field. Noting here that we discussed this offline. The driver is going to stay with a static register design for now, but the write sequence interface is being modified to be control based. Best, James