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);
+}
