Messages in this thread | | | Date | Wed, 22 Feb 2023 11:16:59 +0000 | Subject | Re: [PATCH 08/10] ASoC: cs35l56: Add driver for Cirrus Logic CS35L56 | From | Richard Fitzgerald <> |
| |
On 21/02/2023 17:58, Pierre-Louis Bossart wrote: > > > On 2/21/23 12:18, Richard Fitzgerald wrote: >> 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. > > What I meant is that the pm_runtime refcount should be increased prior > to the SoundWire core using the interrupt_callback. > > So the only window I see is in the second part of in the interrupt > handling, before the workqueue is scheduled.
Yes, sorry I didn't explain well. It's because we defer the handling to a workqueue job. It's to prevent the (small) chance of runtime-suspending and clock-stopping before the work queue function has had time to process the interrupt.
It only matters because the Intel controller issues a Bus-Reset when it comes out of clock stop.
>> >>>> + >>>> + /* >>>> + * 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. > > "until the manager restarted the bus and re-enumerated devices" ? >
I'll re-word the comment. And also change to "manager".
>> >>>> + 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. > > it's not supposed to happen indeed, but our CI caught a couple of issues > over the last two years. Better add a check. >
No harm in checking for error.
>> >>>> + >>>> + /* >>>> + * 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) > > I wasn't suggesting using a pm_runtime suspend/resume cycle but rather > use a common helper called from here and from the pm_runtime_resume. > > It's a suggestion only. >
It's on my to-do list to see whether this is useful.
Effectively we're re-initializing and the code we want to re-use is what cs35l56_init() does after it issues SYSTEM_RESET. So cs35l56_init() is acting as the common code.
We're going to get an update_status() callback when it re-enumerates. The update_status() calls cs35l56_init() during the first enumeration. So we are re-using that same code path to call again cs35l56_init() and repeat the post-SYSTEM_RESET handling.
>> >>> >>>> + 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); >>>> +}
| |