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: Wed, 19 Aug 2020 15:46:30 +0200	[thread overview]
Message-ID: <9a733e30-8d44-edf7-1bae-5b6f935628d2@intel.com> (raw)
In-Reply-To: <20200818115050.GI1891694@smile.fi.intel.com>

On 2020-08-18 1:50 PM, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 01:12:01PM +0200, Cezary Rojewski wrote:
>> 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!
> 
> You're welcome!
> 

>>>> +#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.
> 
> Please, give extended comment here.
> 

Sure, ack.

>>>> +	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.
> 
> I guess this deserves a comment.
> 

Ditto.

>>>> +	return (status == DMA_COMPLETE) ? 0 : -EPROTO;
> 
> ...
> 
>>>> +	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.
> 
> you also may consider to use u32_replace_bits() or so.
> 

I'll check bitfields.h too, sure.

>>>> +	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().
> 
> I didn't get this. Can you point out where is exactly problematic place?
> 

list_for_each_entry makes use of list_first_entry when initializing 
'pos' index variable. Documentation for list_first_entry reads: "Note, 
that list is expected to be not empty" so I'm validating list's status 
before moving on to the loop as stream_list may be empty when 
catpt_dsp_update_lpclock() gets called.