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: Tue, 25 Aug 2020 16:16:15 +0300	[thread overview]
Message-ID: <20200825131615.GG1891694@smile.fi.intel.com> (raw)
In-Reply-To: <dec6d548-8376-683f-7d07-44f1cfbbf375@intel.com>

On Mon, Aug 24, 2020 at 06:33:17PM +0200, Cezary Rojewski wrote:
> 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.

Yes, I have removed it because it's a copy cat of Synopsys DesignWare. At Intel
at that time Vinod, who was developing that driver, had probably no idea that
he is reinventing the wheel. This theory is supported by (internal)
documentation for Medfield which has no words Synopsys, DesignWare in it.

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

Correct.

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

Yes!

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

I see, thanks for checking this.

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

Basically what you do with accessing PCI configuration space via these methods
(catpt_update_pci(), etc) is something repetitive / similar to what xHCI DbC
support code does. I recommend to spend some time to look for similarities here
(catpt) and there (PCI core, xHCI DbC, etc) and, if we were lucky, derive
common helpers for traverse the capability list in more generalized way.

-- 
With Best Regards,
Andy Shevchenko