Re: [PATCH v3 2/2] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Thanks Pierre for taking time to review the patch.

On 11/10/2019 18:50, Pierre-Louis Bossart wrote:
> > +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
> > cmd_data,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 dev_addr, u16 reg_addr)
> > +{
> > +ÂÂÂ DECLARE_COMPLETION_ONSTACK(comp);
> > +ÂÂÂ unsigned long flags;
> > +ÂÂÂ u32 val;
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
> > +ÂÂÂ ctrl->comp = ∁
> > +ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> > +ÂÂÂ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SWRM_SPECIAL_CMD_ID, reg_addr);
> > +ÂÂÂ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto err;
> > +
> > +ÂÂÂ ret = wait_for_completion_timeout(ctrl->comp,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msecs_to_jiffies(TIMEOUT_MS));
> > +
> > +ÂÂÂ if (!ret)
> > +ÂÂÂÂÂÂÂ ret = SDW_CMD_IGNORED;
> > +ÂÂÂ else
> > +ÂÂÂÂÂÂÂ ret = SDW_CMD_OK;
>
> It's odd to report CMD_IGNORED on a timeout. CMD_IGNORED is a valid 
> answer that should be retrieved immediately. You probably need to 
> translate the soundwire errors into -ETIMEOUT or something.

In this controller we have no way to know if the command is ignored or 
timedout, so All the commands that did not receive response either due 
to ignore or timeout are currently detected with out any response 
interrupt in given timeout.

> > +err:
> > +ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
> > +ÂÂÂ ctrl->comp = NULL;
> > +ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> > +
> > +ÂÂÂ return ret;
> > +}
> > +
> > +
> > +ÂÂÂ for (i = 0; i < len; i++) {
> > +ÂÂÂÂÂÂÂ ret = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
> > +ÂÂÂÂÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂÂÂÂÂ rval[i] = val & 0xFF;
> > +ÂÂÂ }
> > +
> > +err:
> > +ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
> > +ÂÂÂ ctrl->comp = NULL;
> > +ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> > +
> > +ÂÂÂ return ret;
> > +} > +
>
> [snip]
>
> > +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_id;
> > +ÂÂÂ u32 sts, value;
> > +ÂÂÂ unsigned long flags;
> > +
> > +ÂÂÂ ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> > +ÂÂÂÂÂÂÂ ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> > +ÂÂÂÂÂÂÂ dev_err_ratelimited(ctrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "CMD error, fifo status 0x%x\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ value);
> > +ÂÂÂÂÂÂÂ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> > +ÂÂÂÂÂÂÂ sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
> > +ÂÂÂÂÂÂÂ schedule_work(&ctrl->slave_work);
> > +
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
> > +ÂÂÂÂÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
> > +ÂÂÂÂÂÂÂ if (ctrl->comp)
> > +ÂÂÂÂÂÂÂÂÂÂÂ complete(ctrl->comp);
> > +ÂÂÂÂÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>
>
> Wouldn't it be simpler if you declared the completion structure as part 
> of your controller definitions, as done for the Intel stuff?
I can give that a go!
> [snip]
>
> > +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_stream_runtime *stream)
> > +{
> > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > +ÂÂÂ unsigned long *port_mask;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->port_lock);
>
> is this lock to avoid races between alloc/free? if yes maybe document it?

Yes, port allocation resource is protected across these calls here, I 
can add some notes as you suggested in next version.

> > +
> > +ÂÂÂ list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +ÂÂÂÂÂÂÂ if (m_rt->direction == SDW_DATA_DIR_RX)
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->dout_port_mask;
> > +ÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->din_port_mask;
> > +
> > +ÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &m_rt->port_list, port_node)
> > +ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(p_rt->num - 1, port_mask);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mutex_unlock(&ctrl->port_lock);
> > +}
> > +
> > +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_stream_runtime *stream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_pcm_hw_params *params,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int direction)
> > +{
> > +ÂÂÂ struct sdw_port_config pconfig[QCOM_SDW_MAX_PORTS];
> > +ÂÂÂ struct sdw_stream_config sconfig;
> > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > +ÂÂÂ struct sdw_slave_runtime *s_rt;
> > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > +ÂÂÂ unsigned long *port_mask;
> > +ÂÂÂ int i, maxport, pn, nports = 0, ret = 0;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->port_lock);
> > +ÂÂÂ list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +ÂÂÂÂÂÂÂ if (m_rt->direction == SDW_DATA_DIR_RX) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ maxport = ctrl->num_dout_ports;
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->dout_port_mask;
> > +ÂÂÂÂÂÂÂ } else {
> > +ÂÂÂÂÂÂÂÂÂÂÂ maxport = ctrl->num_din_ports;
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->din_port_mask;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Port numbers start from 1 - 14*/
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pn = find_first_zero_bit(port_mask, maxport);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pn > (maxport - 1)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "All ports busy\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EBUSY;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_bit(pn, port_mask);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pconfig[nports].num = pn + 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pconfig[nports].ch_mask = p_rt->ch_mask;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nports++;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (direction == SNDRV_PCM_STREAM_CAPTURE)
> > +ÂÂÂÂÂÂÂ sconfig.direction = SDW_DATA_DIR_TX;
> > +ÂÂÂ else
> > +ÂÂÂÂÂÂÂ sconfig.direction = SDW_DATA_DIR_RX;
> > +
> > +ÂÂÂ sconfig.ch_count = 1;
> > +ÂÂÂ sconfig.frame_rate = params_rate(params);
> > +ÂÂÂ sconfig.type = stream->type;
> > +ÂÂÂ sconfig.bps = 1;
>
> Should probably add a note that hw_params is ignored since it's PDM 
> content, so only 1ch 1bit data.

Okay Sure!
> > +ÂÂÂ sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nports, stream);
> > +err:
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ for (i = 0; i < nports; i++)
> > +ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(pconfig[i].num - 1, port_mask);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mutex_unlock(&ctrl->port_lock);
> > +
> > +ÂÂÂ return ret;
> > +}
> > +
>
> [snip]
>
> > +static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> > +ÂÂÂ struct sdw_stream_runtime *sruntime = ctrl->sruntime[dai->id];
> > +
> > +ÂÂÂ qcom_swrm_stream_free_ports(ctrl, sruntime);
> > +ÂÂÂ sdw_stream_remove_master(&ctrl->bus, sruntime);
> > +ÂÂÂ sdw_deprepare_stream(sruntime);
> > +ÂÂÂ sdw_disable_stream(sruntime);
>
> Should is be the reverse order? Removing ports/master before disabling 
> doesn't seem too good.

Good point!  Will fix it in next version.

> > +
> > +ÂÂÂ return 0;
> > +}
> > +
>
> > +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +ÂÂÂ int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> > +ÂÂÂ struct snd_soc_dai_driver *dais;
> > +ÂÂÂ struct snd_soc_pcm_stream *stream;
> > +ÂÂÂ struct device *dev = ctrl->dev;
> > +ÂÂÂ int i;
> > +
> > +ÂÂÂ /* PDM dais are only tested for now */
> > +ÂÂÂ dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
> > +ÂÂÂ if (!dais)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ for (i = 0; i < num_dais; i++) {
> > +ÂÂÂÂÂÂÂ dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW Pin%d", i);
> > +ÂÂÂÂÂÂÂ if (!dais[i].name)
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂ if (i < ctrl->num_dout_ports) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ stream = &dais[i].playback;
> > +ÂÂÂÂÂÂÂÂÂÂÂ stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "SDW Tx%d", i);
> > +ÂÂÂÂÂÂÂ } else {
> > +ÂÂÂÂÂÂÂÂÂÂÂ stream = &dais[i].capture;
> > +ÂÂÂÂÂÂÂÂÂÂÂ stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "SDW Rx%d", i);
> > +ÂÂÂÂÂÂÂ }
>
> For the Intel stuff, we removed the stream_name assignment since it 
> conflicted with the DAI widgets added by the topology. Since the code 
> looks inspired by the Intel DAI handling, you should look into this.

Yes, this code was inspired by Intel's DAI handling, I will take a look 
a look at latest code and update accordingly.

Thanks,
srini
>