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

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Andy Shevchenko <andriy.shevchenko@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: Mon, 17 Aug 2020 13:12:01 +0200	[thread overview]
Message-ID: <946fdd80-c89d-ee1b-6eef-e752318b55a6@intel.com> (raw)
In-Reply-To: <20200813185129.GB1891694@smile.fi.intel.com>

On 2020-08-13 8:51 PM, Andy Shevchenko wrote:
> 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.

Thanks for your input Andy!

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

Agree. Don't have much expertise with DMA-engine and some stuff is based 
on existing stuff:
/sound/soc/intel/common/sst-firmware.c ::sst_dma_transfer_complete()

That's not somebody - that's CI. I don't mind ftracer, but our's CI 
logging is currently dmesg/ event traces -based.

Anyway, with log removed, catpt_dma_transfer_complete() ceases to exist too.

> 
>> +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).
> 

Good point. Yeah, I've tried to find generic-helper before even adding 
this, but with no result.

> 
>> +#define CATPT_DMA_DEVID		1 /* dma engine used */
> 
> Not sure I understand what exactly this means.
> 

Well, you may choose either engine 0 or 1 for loading images. Reference 
solution which I'm basing catpt on - Windows driver equivalent - makes 
use of engine 1. Goal of this implementation is to align closely to 
stable Windows solution wherever possible to reduce maintainance cost.

> 
>> +#define CATPT_DMA_MAXBURST	0x3
> 
> We have DMA engine definitions for that, please avoid magic numbers.
> 

As with most of the dma stuff, based on existing:
/sound/soc/intel/common/sst-firmware.c SST_DSP_DMA_MAX_BURST

Ack.

>> +#define CATPT_DMA_DSP_ADDR_MASK	0xFFF00000
> 
> GENMASK()
> 

Ditto as above. Ack.

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

I'm as surpriced as you : )
Remnant of pm_runtime + single platform_device (for adsp and dw both) 
debug. Removed in v5.

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

Yes. This is based on stable Windows solution equivalent and get's 
updated even in failure case to disable access to HOST memory in demand 
more.

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

Well, I've just replaced these with dev_dbg (from trace events). Not 
many usages and caused all reg-related prints to be available in dmesg.

>> +	if (old == new)
>> +		return;
>> +
>> +	catpt_updatel_pci(cdev, VDRTCTL0, mask, new);
> 
>> +	udelay(60);
> 
> Delays should be commented.
> 

Similarly to previous review (comment regarding hw support), I'm afraid 
I don't have good comments for most occurences available.

I'll try to add something meaningful for at least some of these.

>> +
>> +	/*
>> +	 * 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()
> 

Ack.

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

Ack.

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

Was called out in the past not to combine everything in one-line like if 
I'm to hide something from reviewer.

No problem with combining these together in v5.

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

As this causes early return, I agree.

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

list_first_entry (part of list_for_each_entry) expects list to be 
non-empty. ->streal_list may be empty when invoking 
catpt_dsp_update_lpclock().

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

Thanks to you now I know the correct answer: yes.
Ack for all of these. Good advice Andy, again!