Date: Tue, 9 Jan 2024 21:08:40 +0000
From: James Ogletree <James.Ogletree@...rus.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
CC: James Ogletree <jogletre@...nsource.cirrus.com>,
        Fred Treven
	<Fred.Treven@...rus.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        Dmitry Torokhov
	<dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Simon Trimmer <simont@...nsource.cirrus.com>,
        Richard
 Fitzgerald <rf@...nsource.cirrus.com>,
        Lee Jones <lee@...nel.org>, Liam
 Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, Jaroslav
 Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        James Schulman
	<James.Schulman@...rus.com>,
        David Rhodes <David.Rhodes@...rus.com>,
        Sebastian Reichel <sebastian.reichel@...labora.com>,
        Jacky Bai
	<ping.bai@....com>, Jeff LaBundy <jeff@...undy.com>,
        Peng Fan
	<peng.fan@....com>, Weidong Wang <wangweidong.a@...nic.com>,
        Herve Codina
	<herve.codina@...tlin.com>,
        Arnd Bergmann <arnd@...db.de>, Shenghao Ding
	<13916275206@....com>,
        Ryan Lee <ryans.lee@...log.com>,
        Linus Walleij
	<linus.walleij@...aro.org>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Shuming Fan
	<shumingf@...ltek.com>,
        "open list:CIRRUS LOGIC HAPTIC DRIVERS"
	<patches@...nsource.cirrus.com>,
        "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK,
 TOUCHSCREEN)..." <linux-input@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND
 FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>,
        open list
	<linux-kernel@...r.kernel.org>,
        "open list:SOUND - SOC LAYER / DYNAMIC AUDIO
 POWER MANAGEM..." <linux-sound@...r.kernel.org>,
        "moderated list:CIRRUS LOGIC
 AUDIO CODEC DRIVERS" <alsa-devel@...a-project.org>
Subject: Re: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver

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@...nsource.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

Powered by blists - more mailing lists