Re: [PATCH RFC 09/23] ASoC: qcom: lpass: add support for optional WSA interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] ASoC: qcom: lpass: lpass_wsa_regmap_config uses RXTX
  callbacks and macros
- [Medium] ASoC: qcom: lpass: lpass_platform_pcmops_trigger() enables
  WSA IRQs on stop
- [Medium] ASoC: qcom: lpass: unchecked regmap_read() in
  lpass_platform_wsaif_irq()
- [Medium] ASoC: qcom: lpass: missing wsa_lpaif_map handling in suspend
  and resume
--

commit ee7ef8a5c6b3d75b7ea08d4564f71cd7fbf7abeb
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: lpass: add support for optional WSA interface

This commit adds LPASS CPU, regmap, and platform DMA support for the
optional WSA LPAIF Codec DMA interface used for speaker SoundWire buses.

> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index ea8b6083529c1..d46b72c926b2b 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -983,6 +983,16 @@ static struct regmap_config lpass_va_regmap_config = {
>  	.cache_type = REGCACHE_FLAT,
>  };
>  
> +static struct regmap_config lpass_wsa_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.writeable_reg = lpass_rxtx_regmap_writeable,
> +	.readable_reg = lpass_rxtx_regmap_readable,
> +	.volatile_reg = lpass_rxtx_regmap_volatile,
> +	.cache_type = REGCACHE_FLAT,
> +};
> +
>  static unsigned int of_lpass_cpu_parse_sd_lines(struct device *dev,
>  						struct device_node *node,
>  						const char *name)

[ ... ]

> @@ -1142,6 +1161,18 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  		if (IS_ERR(drvdata->va_lpaif_map))
>  			return PTR_ERR(drvdata->va_lpaif_map);
>  
> +		if (drvdata->wsa_enable) {
> +			lpass_wsa_regmap_config.max_register = LPAIF_CDC_RXTX_WRDMAPER_REG(variant,
> +						variant->wsa_wrdma_channels +
> +						variant->wsa_wrdma_channel_start,
> +						LPASS_CDC_DMA_WSA_TX0);
> +
> +			drvdata->wsa_lpaif_map = devm_regmap_init_mmio(dev, drvdata->wsa_lpaif,
> +								       &lpass_wsa_regmap_config);
> +			if (IS_ERR(drvdata->wsa_lpaif_map))
> +				return PTR_ERR(drvdata->wsa_lpaif_map);
> +		}
> +
>  		ret = of_lpass_cdc_dma_clks_parse(dev, drvdata);
>  		if (ret) {
>  			dev_err(dev, "failed to get cdc dma clocks %d\n", ret);

[Severity: Medium]
Should lpass_wsa_regmap_config use dedicated WSA regmap callbacks instead
of the RXTX ones, and should max_register use LPAIF_CDC_WSA_WRDMAPER_REG()?

__lpass_rxtx_regmap_accessible() and lpass_rxtx_regmap_volatile() check
v->rxtx_* parameters and pass LPASS_CDC_DMA_RX0 / LPASS_CDC_DMA_TX3 to
LPAIF_CDC_RXTX_*_REG(), leaving the newly added v->wsa_irq_ports,
LPAIF_CDC_WSA_RDMA_INTF_REG(), and LPAIF_CDC_WSA_WRDMA_INTF_REG() unused.
Any difference between WSA and RXTX register geometry or channel counts
causes accesses on wsa_lpaif_map to fail with -EIO or return stale
REGCACHE_FLAT entries.  In addition, __lpass_rxtx_regmap_accessible()
omits adding wrdma_channel_start to i when checking
LPAIF_CDC_RXTX_WRDMACURR_REG(), so lpass_platform_pcmops_pointer() reads
of LPAIF_DMACURR_REG on WRDMA capture channels >= rxtx_wrdma_channels also
fail with -EIO:

sound/soc/qcom/lpass-cpu.c:__lpass_rxtx_regmap_accessible() {
    ...
	for (i = 0; i < v->rxtx_wrdma_channels; ++i) {
		if (reg == LPAIF_CDC_RXTX_WRDMACTL_REG(v, i + v->rxtx_wrdma_channel_start,
							LPASS_CDC_DMA_TX3))
			return true;
    ...
		if (rw == LPASS_REG_READ) {
			if (reg == LPAIF_CDC_RXTX_WRDMACURR_REG(v, i, LPASS_CDC_DMA_RX0))
				return true;
		}
    ...
}

> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> index e162627d6f86e..9c879a8090bbf 100644
> --- a/sound/soc/qcom/lpass-platform.c
> +++ b/sound/soc/qcom/lpass-platform.c

[ ... ]

> @@ -838,6 +937,22 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component,
>  			val_mask = LPAIF_IRQ_ALL(ch);
>  			val_irqen = LPAIF_IRQ_ALL(ch);
>  			break;
> +		case LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9:
> +		case LPASS_CDC_DMA_WSA_TX0 ... LPASS_CDC_DMA_WSA_TX8:
> +			ret = regmap_fields_write(dmactl->dyncclk, id, LPAIF_DMACTL_DYNCLK_OFF);
> +			if (ret) {
> +				dev_err(soc_runtime->dev,
> +					"error writing to rdmactl reg field: %d\n", ret);
> +				return ret;
> +			}
> +
> +			reg_irqclr = LPAIF_WSA_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST);
> +			val_irqclr = LPAIF_IRQ_ALL(ch);
> +
> +			reg_irqen = LPAIF_WSA_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST);
> +			val_mask = LPAIF_IRQ_ALL(ch);
> +			val_irqen = LPAIF_IRQ_ALL(ch);
> +			break;
>  		default:
>  			dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id);
>  			return -EINVAL;

[Severity: Medium]
In the SNDRV_PCM_TRIGGER_STOP branch of lpass_platform_pcmops_trigger(),
doesn't setting val_irqen = LPAIF_IRQ_ALL(ch) keep WSA channel interrupts
enabled in LPAIF_WSA_IRQEN_REG instead of disabling them?

reg_irqclr and val_irqclr are also never written in the stop branch.  Once
lpass_platform_pcmops_close() clears drvdata->wsa_substream[data->dma_ch],
lpass_platform_wsaif_irq() ignores any pending interrupt on that channel
without clearing LPAIF_WSA_IRQCLEAR_REG while returning IRQ_HANDLED, which
can cause an interrupt storm.

[ ... ]

> @@ -1124,6 +1245,31 @@ static irqreturn_t lpass_platform_vaif_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t lpass_platform_wsaif_irq(int irq, void *data)
> +{
> +	struct lpass_data *drvdata = data;
> +	const struct lpass_variant *v = drvdata->variant;
> +	unsigned int irqs;
> +	irqreturn_t rv;
> +	int chan;
> +
> +	rv = regmap_read(drvdata->wsa_lpaif_map,
> +			 LPAIF_WSA_IRQSTAT_REG(v, LPAIF_IRQ_PORT_HOST), &irqs);
> +
> +	/* Handle per channel interrupts */
> +	for (chan = 0; chan < LPASS_MAX_CDC_DMA_CHANNELS; chan++) {
> +		if (irqs & LPAIF_IRQ_ALL(chan) && drvdata->wsa_substream[chan]) {
> +			rv = lpass_dma_interrupt_handler(
> +						drvdata->wsa_substream[chan],
> +						drvdata, chan, irqs);
> +			if (rv != IRQ_HANDLED)
> +				return rv;
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
What happens in lpass_platform_wsaif_irq() if regmap_read() fails?

Unlike lpass_platform_lpaif_irq() and lpass_platform_hdmiif_irq(), rv is
not checked before reading the uninitialized stack variable irqs, which
can dispatch spurious period, xrun, or bus-error events on active streams.

[ ... ]

> @@ -1359,6 +1513,34 @@ int asoc_qcom_lpass_platform_register(struct platform_device *pdev)
>  				"error initializing va dmactl fields: %d\n", ret);
>  			return ret;
>  		}
> +
> +		if (drvdata->wsa_enable) {
> +			ret = regmap_write(drvdata->wsa_lpaif_map,
> +				LPAIF_WSA_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST), 0x0);
> +			if (ret) {
> +				dev_err(&pdev->dev, "error writing to wsa irqen reg: %d\n", ret);
> +				return ret;
> +			}
> +			drvdata->wsaif_irq = platform_get_irq_byname(pdev, "lpass-irq-wsaif");
> +			if (drvdata->wsaif_irq < 0)
> +				return -ENODEV;
> +
> +			ret = devm_request_irq(&pdev->dev, drvdata->wsaif_irq,
> +					       lpass_platform_wsaif_irq, 0,
> +					       "lpass-irq-wsaif", drvdata);
> +			if (ret) {
> +				dev_err(&pdev->dev, "wsa irq request failed: %d\n", ret);
> +				return ret;
> +			}
> +
> +			ret = lpass_platform_alloc_wsa_dmactl_fields(&pdev->dev,
> +								     drvdata->wsa_lpaif_map);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					"error initializing wsa dmactl fields: %d\n", ret);
> +				return ret;
> +			}
> +		}
>  	}

[Severity: Medium]
Should lpass_platform_pcmops_suspend() and lpass_platform_pcmops_resume()
also handle drvdata->wsa_lpaif_map when drvdata->wsa_enable is set?

lpass_wsa_regmap_config uses REGCACHE_FLAT, but suspend and resume only
manage hdmiif_map and lpaif_map.  If LPASS loses hardware state across
system suspend, cached WSA DMA registers are not synced on resume, and
subsequent regmap_update_bits() or regmap_fields_write() calls see the
pre-suspend values in the flat cache and skip writing to hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=9