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, 24 Aug 2020 18:33:17 +0200	[thread overview]
Message-ID: <dec6d548-8376-683f-7d07-44f1cfbbf375@intel.com> (raw)
In-Reply-To: <20200820090055.GT1891694@smile.fi.intel.com>

On 2020-08-20 11:00 AM, Andy Shevchenko wrote:
> On Thu, Aug 20, 2020 at 09:30:13AM +0200, Cezary Rojewski wrote:
>> 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;
> 
> Okay, and how 0x3 bytes can be a burst? Does DMA engine support this?
> 

That's a very good question. Early this morning starting digging.

I believe this stems from attempt of sst_dsp_dma_get_channel() to serve 
two masters - MID DMAC and DW DMAC - at the same time.

Intel MID DMAC has been first introduced in version v2.6.36-rc1 and 
subsequently removed in v4.1-rc1 due to lack of any usage throughout its 
whole life. Reference:
	https://cateee.net/lkddb/web-lkddb/INTEL_MID_DMAC.html

files to look for in said kernel versions:
	drivers/dma/intel_mid_dma.c
	drivers/dma/intel_mid_regs.h

As MID DMAC is supposed to handle Langwell PCH which is coupled with 
Atom CPUs my guess is that /atom/ was supposed to make use of said DMAC 
at certain point in time which actually never happened. This is backed 
up by the following:

/haswell/ makes use of DW DMAC but that wasn't always the case - 
solution, which was first introduced in v3.15-rc1 have had the 
sst_dma_new() and friends (wrappers calling dw_dma_probe()) implemented 
only since v3.19-rc1. So, there was a period in time where both 
solutions were not using any DMAC whatsoever. As stated, in v3.19-rc1 
/haswell/ moved to recommended flow where DW DMAC gets involved in image 
loading. The same cannot be said about /atom/ -or- /baytrail/ for that 
matter and that is why I believe MID DMAC got removed, eventually.

Now, one can notice the following when viewing older versions of 
/haswell/ e.g.:
	https://elixir.bootlin.com/linux/v4.0.9/source/sound/soc/intel/sst-firmware.c#L224

notice the comment mentioning why sst_dsp_dma_get_channel() is somewhat 
ambiguous:
	The Intel MID DMA engine driver needs the slave config set but
	Synopsis DMA engine driver safely ignores the slave config

And thus:
	dma_cap_set(DMA_SLAVE, mask);
is flagged only because of MID DMAC while causing no harm to DW. As MID 
no longer exists, so should DMA_SLAVE flagging. This should answer your 
question from:

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.
 >
 >> +	dma_cap_set(DMA_SLAVE, mask);
 >
 > How this helps with mem2mem channel?
 >

In regard to maxburst, (SST_DSP_DMA_MAX_BURST 0x3) this is again mixture 
of MID & DW expectations - MID owns no conversion code, value assigned 
to src_/dst_maxburst is taken directly:

	https://elixir.bootlin.com/linux/v4.0.9/source/drivers/dma/intel_mid_dma.c#L495
vs
	https://elixir.bootlin.com/linux/v4.0.9/source/drivers/dma/dw/core.c#L965
notice convert_burst() (/drivers/dma/dw has seen several improvements 
and code isn't identical in newer kernels but functionality remains the 
same)

combine that knowledge with:
	enum dw_dma_msize {
		DW_DMA_MSIZE_1,
		DW_DMA_MSIZE_4,
		DW_DMA_MSIZE_8,
		DW_DMA_MSIZE_16,

from:
	https://elixir.bootlin.com/linux/v4.0.9/source/drivers/dma/dw/regs.h#L135

and one can safely assume maxburst is mistakenly set for DW - direct 
enum constant is used, ignoring the fact that is it DW code which 
converts provided value behind the scenes to appropriate one. Currently 
we end up with:
fls(0x3) -> 2;
2 - 2 = 0 (conversion method) -> DW_DMA_MSIZE_1

Conclusion: I believe DW_DMA_MSIZE_16 (0x3) is the correct one i.e.:
	src_maxburst = 16;
	dst_maxburst = 16;
should be present within catpt_dma_request_config_chan().

>>
>> 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 see. Can you dump DSDT somewhere? We are interested in
> PSx/PRx/PSE/PSW/PSC/PRE/PRW/ON/OFF (x=0..3) methods.
> 
>> 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.
> 
> If we really need to use these commands directly, utilize at least definitions
> from PCI core, e.g. PCI_D0, PCI_D3hot, PCI_PM_CTRL.
> 

Kudos to you, Andy, for taking time and debugging ACPI tables from our 
BDW machines.
Unfortunately explicit _updatel_pci for d3hot/d0 will have to remain as 
there is no other way to cause explicit power_state change for the device.

Another question though: PCI_PM_CTRL. In order for me to make use of 
this, "pm_cap" member would have to be declared for my device. As this 
is no struct pci_dev, catpt has currently no separate member for that 
purpose. I don't believe you want me to add that field into struct's 
declaration.
Second option is to define constant for pm_cap offset aka 0x80 within 
registers.h and then do the operations as follows:
	catpt_updatel_pci(cdev, CATPT_PM_CAP + PCI_PM_CTRL, ...)

However, in such case I won't be able to make use of current version of 
_updatel_pci() as definition of that macro allows me to skip prefix and 
type implicitly - PMCS (the rest is appended automatically).
Maybe let's leave it within registers.h altogether so I can actually 
keep using said macro?

Czarek