Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
	harshapriya.n@intel.com, marcin.barlik@intel.com,
	zwisler@google.com, lgirdwood@gmail.com, tiwai@suse.com,
	filip.proborszcz@intel.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
	cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
	ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations
Date: Thu, 13 Aug 2020 21:51:29 +0300	[thread overview]
Message-ID: <20200813185129.GB1891694@smile.fi.intel.com> (raw)
In-Reply-To: <20200812205753.29115-3-cezary.rojewski@intel.com>

On Wed, Aug 12, 2020 at 10:57:42PM +0200, Cezary Rojewski wrote:
> Implement dsp lifecycle functions such as core RESET and STALL,
> SRAM power control and LP clock selection. This also adds functions for
> handling transport over DW DMA controller.

...

> +static void catpt_dma_transfer_complete(void *arg)
> +{
> +	struct catpt_dev *cdev = arg;
> +

> +	dev_dbg(cdev->dev, "%s\n", __func__);

Noise. Somebody should provide either trace events, of get used to function tracer / perf.

> +}

...

> +static bool catpt_dma_filter(struct dma_chan *chan, void *param)
> +{

> +	return chan->device->dev == (struct device *)param;

Redundant casting, and please, consider to swap left and right side (in this
case easily to find these and perhaps provide a generic helper function).

> +}

...

> +#define CATPT_DMA_DEVID		1 /* dma engine used */

Not sure I understand what exactly this means.

...

> +#define CATPT_DMA_MAXBURST	0x3

We have DMA engine definitions for that, please avoid magic numbers.

...

> +#define CATPT_DMA_DSP_ADDR_MASK	0xFFF00000

GENMASK()

...

> +	dma_cap_set(DMA_SLAVE, mask);

How this helps with mem2mem channel?

...

> +	chan = dma_request_channel(mask, catpt_dma_filter, cdev->dev);
> +	if (!chan) {
> +		dev_err(cdev->dev, "request channel failed\n");

> +		dump_stack();

Huh?!

> +		return ERR_PTR(-EPROBE_DEFER);
> +	}

...

> +	status = dma_wait_for_async_tx(desc);

> +	catpt_updatel_shim(cdev, HMDC,
> +			   CATPT_HMDC_HDDA(CATPT_DMA_DEVID, chan->chan_id), 0);

Update even in erroneous case?

> +	return (status == DMA_COMPLETE) ? 0 : -EPROTO;

...

> +static void catpt_dsp_set_srampge(struct catpt_dev *cdev, struct resource *sram,
> +				  unsigned long mask, unsigned long new)
> +{
> +	unsigned long old;
> +	u32 off = sram->start;
> +	u32 b = __ffs(mask);
> +
> +	old = catpt_readl_pci(cdev, VDRTCTL0) & mask;

> +	dev_dbg(cdev->dev, "SRAMPGE [0x%08lx] 0x%08lx -> 0x%08lx",
> +		mask, old, new);

trace event / point?

> +	if (old == new)
> +		return;
> +
> +	catpt_updatel_pci(cdev, VDRTCTL0, mask, new);

> +	udelay(60);

Delays should be commented.

> +
> +	/*
> +	 * dummy read as the very first access after block enable
> +	 * to prevent byte loss in future operations
> +	 */
> +	for_each_clear_bit_from(b, &new, fls(mask)) {

fls_long()

> +		u8 buf[4];
> +
> +		/* newly enabled: new bit=0 while old bit=1 */
> +		if (test_bit(b, &old)) {
> +			dev_dbg(cdev->dev, "sanitize block %ld: off 0x%08x\n",

> +				(b - __ffs(mask)), off);

Unneeded parentheses.

> +			memcpy_fromio(buf, cdev->lpe_ba + off, sizeof(buf));
> +		}
> +		off += CATPT_MEMBLOCK_SIZE;
> +	}
> +}

...

> +	/* offset value given mask's start and invert it as ON=b0 */

> +	new <<= __ffs(mask);
> +	new = ~(new) & mask;

Unneeded parentheses.
And perhaps in one line it will be better to understand:

	new = ~(new << __ffs(mask)) & mask;

...

> +	if (waiti) {
> +		/* wait for DSP to signal WAIT state */
> +		ret = catpt_readl_poll_shim(cdev, ISD,
> +					    reg, (reg & CATPT_ISD_DCPWM),
> +					    500, 10000);
> +		if (ret < 0) {

> +			dev_warn(cdev->dev, "await WAITI timeout\n");

Shouldn't be an error level?

> +			mutex_unlock(&cdev->clk_mutex);
> +			return ret;
> +		}
> +	}

...

> +int catpt_dsp_update_lpclock(struct catpt_dev *cdev)
> +{
> +	struct catpt_stream_runtime *stream;

> +	bool lp;
> +
> +	if (list_empty(&cdev->stream_list))
> +		return catpt_dsp_select_lpclock(cdev, true, true);
> +
> +	lp = true;
> +	list_for_each_entry(stream, &cdev->stream_list, node) {
> +		if (stream->prepared) {
> +			lp = false;
> +			break;
> +		}
> +	}
> +
> +	return catpt_dsp_select_lpclock(cdev, lp, true);

Seems too much duplication.

	struct catpt_stream_runtime *stream;

	list_for_each_entry(stream, &cdev->stream_list, node) {
		if (stream->prepared)
			return catpt_dsp_select_lpclock(cdev, false, true);
	}

	return catpt_dsp_select_lpclock(cdev, true, true);

Better?

> +}

...

> +	/* set D3 */
> +	catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, CATPT_PMCS_PS_D3HOT);
> +	udelay(50);

Don't we have PCI core function for this?

...

> +	/* set D0 */
> +	catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, 0);
> +	udelay(100);

Ditto.

...

> +	/* set D3 */
> +	catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, CATPT_PMCS_PS_D3HOT);
> +	udelay(50);

Ditto.

...

> +	/* set D0 */
> +	catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, 0);

Ditto.

-- 
With Best Regards,
Andy Shevchenko