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
