Re: [PATCH 08/10] ASoC: cs35l56: Add driver for Cirrus Logic CS35L56

Richard Fitzgerald
2023-02-21T17:19:21Z

On 21/02/2023 16:45, Pierre-Louis Bossart wrote:
>
>> +static int cs35l56_sdw_interrupt(struct sdw_slave *peripheral,
>> + struct sdw_slave_intr_status *status)
>> +{
>> + struct cs35l56_private *cs35l56 = dev_get_drvdata(&peripheral->dev);
>> +
>> + /* SoundWire core holds our pm_runtime when calling this function. */
>> +
>> + dev_dbg(cs35l56->dev, "int control_port=%#x\n", status->control_port);
>> +
>> + if ((status->control_port & SDW_SCP_INT1_IMPL_DEF) == 0)
>> + return 0;
>> +
>> + /* Prevent host controller suspending before we handle the interrupt */
>> + pm_runtime_get_noresume(cs35l56->dev);
>
> can this happen that the manager suspends in this function?
>
> Or is this needed because of the queued work which the manager has no
> knowledge of?
>

Because you issue a Bus-Reset when you suspend and clock-stop, if we
didn't take our pm_runtime there is a small window of time where we
could be reset before we've handled the interrupt. It's unlikely to
happen but better to be safe than to rely on autosuspend delays.

>> +
>> + /*
>> + * Mask and clear until it has been handled. The read of GEN_INT_STAT_1
>> + * is required as per the SoundWire spec for interrupt status bits
>> + * to clear. GEN_INT_MASK_1 masks the _inputs_ to GEN_INT_STAT1.
>> + * None of the interrupts are time-critical so use the
>> + * power-efficient queue.
>> + */
>> + sdw_write_no_pm(peripheral, CS35L56_SDW_GEN_INT_MASK_1, 0);
>> + sdw_read_no_pm(peripheral, CS35L56_SDW_GEN_INT_STAT_1);
>> + sdw_write_no_pm(peripheral, CS35L56_SDW_GEN_INT_STAT_1, 0xFF);
>> + queue_work(system_power_efficient_wq, &cs35l56->sdw_irq_work);
>> +
>> + return 0;
>> +}
>
>> +static int __maybe_unused cs35l56_sdw_handle_unattach(struct cs35l56_private *cs35l56)
>> +{
>> + struct sdw_slave *peripheral = cs35l56->sdw_peripheral;
>> +
>> + if (peripheral->unattach_request) {
>> + /* Cannot access registers until master re-attaches. */
>
> not sure what the comment means, the manager does not attach. did you
> mean resume the bus?
>

If the manager has forced us to reset we can't access the registers
until the manager has recovered its state.

>> + dev_dbg(cs35l56->dev, "Wait for initialization_complete\n");
>> + if (!wait_for_completion_timeout(&peripheral->initialization_complete,
>> + msecs_to_jiffies(5000))) {
>> + dev_err(cs35l56->dev, "initialization_complete timed out\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + peripheral->unattach_request = 0;
>> +
>> + /*
>> + * Don't call regcache_mark_dirty(), we can't be sure that the
>> + * Manager really did issue a Bus Reset.
>> + */
>> + }
>> +
>> + return 0;
>> +}
> ...
>
>> +static void cs35l56_dsp_work(struct work_struct *work)
>> +{
>> + struct cs35l56_private *cs35l56 = container_of(work,
>> + struct cs35l56_private,
>> + dsp_work);
>> + unsigned int reg;
>> + unsigned int val;
>> + int ret = 0;
>> +
>> + if (!wait_for_completion_timeout(&cs35l56->init_completion,
>> + msecs_to_jiffies(5000))) {
>> + dev_err(cs35l56->dev, "%s: init_completion timed out\n", __func__);
>> + goto complete;
>> + }
>> +
>> + if (!cs35l56->init_done || cs35l56->removing)
>> + goto complete;
>> +
>> + cs35l56->dsp.part = devm_kasprintf(cs35l56->dev, GFP_KERNEL, "cs35l56%s-%02x",
>> + cs35l56->secured ? "s" : "", cs35l56->rev);
>> +
>> + if (!cs35l56->dsp.part)
>> + goto complete;
>> +
>> + pm_runtime_get_sync(cs35l56->dev);
>
> test that this is successful?
>

Could do. Wasn't really expecting it to fail unless the hardware is
already broken.

>> +
>> + /*
>> + * Disable SoundWire interrupts to prevent race with IRQ work.
>> + * Setting sdw_irq_no_unmask prevents the handler re-enabling
>> + * the SoundWire interrupt.
>> + */
>> + if (cs35l56->sdw_peripheral) {
>> + cs35l56->sdw_irq_no_unmask = true;
>> + cancel_work_sync(&cs35l56->sdw_irq_work);
>> + sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1, 0);
>> + sdw_read_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_STAT_1);
>> + sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_STAT_1, 0xFF);
>> + }
>> +
>> + ret = cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_SHUTDOWN);
>> + if (ret) {
>> + dev_dbg(cs35l56->dev, "%s: CS35L56_MBOX_CMD_SHUTDOWN ret %d\n", __func__, ret);
>> + goto err;
>> + }
>> +
>> + if (cs35l56->rev < CS35L56_REVID_B0)
>> + reg = CS35L56_DSP1_PM_CUR_STATE_A1;
>> + else
>> + reg = CS35L56_DSP1_PM_CUR_STATE;
>> +
>> + ret = regmap_read_poll_timeout(cs35l56->regmap, reg,
>> + val, (val == CS35L56_HALO_STATE_SHUTDOWN),
>> + CS35L56_HALO_STATE_POLL_US,
>> + CS35L56_HALO_STATE_TIMEOUT_US);
>> + if (ret < 0)
>> + dev_err(cs35l56->dev, "Failed to poll PM_CUR_STATE to 1 is %d (ret %d)\n",
>> + val, ret);
>> +
>> + /* Use wm_adsp to load and apply the firmware patch and coefficient files */
>> + ret = wm_adsp_power_up(&cs35l56->dsp);
>> + if (ret) {
>> + dev_dbg(cs35l56->dev, "%s: wm_adsp_power_up ret %d\n", __func__, ret);
>> + goto err;
>> + }
>> +
>> + if (cs35l56->removing)
>> + goto err;
>> +
>> + mutex_lock(&cs35l56->irq_lock);
>> +
>> + init_completion(&cs35l56->init_completion);
>> +
>> + cs35l56_system_reset(cs35l56);
>> +
>> + if (cs35l56->sdw_peripheral) {
>> + if (!wait_for_completion_timeout(&cs35l56->init_completion,
>> + msecs_to_jiffies(5000))) {
>> + dev_err(cs35l56->dev, "%s: init_completion timed out (SDW)\n", __func__);
>
> shouldn't do the same routine as for a regular pm_runtime resume,
> including re-synching regmaps?
>

Not sure it would help. It's not the same as runtime_resume because
we've changed the firmware and rebooted it (the firmware is retained
in a runtime_suspend). We need to do some of the first-time init()
code again, which we don't need to do in runtime_resume.

Also would create a circular dependency between this driver and the
cs35l56-sdw driver. (We _could_ call our dev->pm->runtime_resume pointer
but that's a bit ugly)

>
>> + goto err_unlock;
>> + }
>> + } else {
>> + if (cs35l56_init(cs35l56))
>> + goto err_unlock;
>> + }
>> +
>> + cs35l56->fw_patched = true;
>> +
>> +err_unlock:
>> + mutex_unlock(&cs35l56->irq_lock);
>> +err:
>> + pm_runtime_mark_last_busy(cs35l56->dev);
>> + pm_runtime_put_autosuspend(cs35l56->dev);
>> +
>> + /* Re-enable SoundWire interrupts */
>> + if (cs35l56->sdw_peripheral) {
>> + cs35l56->sdw_irq_no_unmask = false;
>> + sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1,
>> + CS35L56_SDW_INT_MASK_CODEC_IRQ);
>> + }
>> +
>> +complete:
>> + complete_all(&cs35l56->dsp_ready_completion);
>> +}