Message-ID: <2d55b8c9-e7f9-6b2e-aad8-5cc902d69000@linux.intel.com>
Date:   Tue, 21 Feb 2023 11:45:36 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>, broonie@...nel.org,
        cezary.rojewski@...el.com, peter.ujfalusi@...ux.intel.com,
        yung-chuan.liao@...ux.intel.com, kai.vehmanen@...ux.intel.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH 08/10] ASoC: cs35l56: Add driver for Cirrus Logic CS35L56


> +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?

> +
> +	/*
> +	 * 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?

> +		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?

> +
> +	/*
> +	 * 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?


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

Powered by blists - more mailing lists