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: Thu, 20 Aug 2020 09:30:13 +0200	[thread overview]
Message-ID: <237f2343-fd57-8ebf-b8f2-8c2cf5c3c745@intel.com> (raw)
In-Reply-To: <946fdd80-c89d-ee1b-6eef-e752318b55a6@intel.com>

On 2020-08-17 1:12 PM, 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:

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

Actually, wasn't able to find anything _MAXBURST related in dmaengine.h. 
_BUSWIDTH_ have their constants defined there, true, but I'm already 
making use of these and this is dst/src_maxburst we're talking about. 
 From what I've seen in kernel sources, most usages are direct 
assignments: xxx_maxburst = Y;

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

Similar situation occurred here. What we're dealing with is: instance of 
'struct platform_device' type, found on bus: acpi with PCI set as a 
parent device.

Scope found in DSDT:
	\_SB_.PCI0.ADSP
sysfs device path:
	/sys/devices/pci0000:00/INT3438:00
Within the latter _no_ standard utility files will be available e.g.: 
ability to dump PCI config space, bars and such.

I haven't found any functionality to extract "pci_companion" from a 
platform_device. What can be made use of is: PCI_D3hot and PCI_D0 enum 
constants, as pci_set_power_state() does not apply - expects struct 
pci_dev *.

Perhaps got misled by the function naming? catpt_updatel_xxx helpers: 
_xxx denotes specific ADSP device's mmio space. Almost all cases are 
covered by _pci and _shim.