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, 27 Aug 2020 12:06:55 +0200	[thread overview]
Message-ID: <e5745369-732a-d70c-20be-2d8c3665e472@intel.com> (raw)
In-Reply-To: <20200825131615.GG1891694@smile.fi.intel.com>

On 2020-08-25 3:16 PM, Andy Shevchenko wrote:
> On Mon, Aug 24, 2020 at 06:33:17PM +0200, Cezary Rojewski wrote:
>> On 2020-08-20 11:00 AM, Andy Shevchenko wrote:

...

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

I wouldn't call direct-access a repetitive procedure, i.e. had procedure 
for enumerating PCI capabilities list been implemented individually by 
every PCI device type, then one can describe that as repetitiveness. 
Here, we are dealing with no procedure at all, just a writel & readl.

About xHCI, I believe you meant: xhci_find_next_ext_cap()
	https://elixir.bootlin.com/linux/latest/source/drivers/usb/host/xhci-ext-caps.h#L97

in case of PCI that's: pci_find_next_capability(), __pci_find_next_cap() 
and friends. pci_find_next_capability() is pci_dev dependent while most 
of the rest pci_bus instead. We fail both dependencies in catpt case.

xhci_find_next_ext_cap search method seems xHCI-specific, notice the 
0x10 offset for HCCPARAMS1 and then the left-shift-by-2. PCI doesn't do 
that when enumerating capabilities, instead it checks Capabilities 
List-bit within Status reg and then begins iterating given the start pos 
- Capability Pointer, usually 0x34. Abstracting these (if even possible) 
would end up with 80% code gluing two different worlds with 20% left 
doing the actual job. Fact that those two are separated increases code 
readability.

While catpt is of platform_device type located on acpi bus, beneath 
there's a (incomplete?) description of PCI device.

PCI config
catpt_acpi_probe00000000: 9cb68086 00100006 04010003 00000000
catpt_acpi_probe00000010: fe000000 fe100000 00000000 00000000
catpt_acpi_probe00000020: 00000000 00000000 00000000 00000000
catpt_acpi_probe00000030: 00000000 00000080 00000000 00000100

PCI base + 0x80
catpt_acpi_probe00000000: 40030001 0000000b 00000000 00000000
catpt_acpi_probe00000010: 00000000 00000000 00000000 00000000
catpt_acpi_probe00000020: fffffffd 00000000 80000fff 00000000
catpt_acpi_probe00000030: 00000000 00000000 00000000 00000000

Capabilities List-bit is set, start pos from Capabilitiy Pointer equals 
0x80. What we have here is singular list of capabilities - PM as the 
only element. Following is the important DWORD (_PM_CTRL) - 0xb tells us 
that device is currently in D3hot. For LPT/WPT ADSP basically all other 
PM bits are hardwired to 0 or not supported.

So, quite frankly, had the BIOS offered correct ADSP device description, 
we wouldn't be dealing with ACPI device/ACPI bus at all. This has been 
corrected from SKL+ ADSP onward. To answer the immediate question: no, 
device of id 0x9cb6 won't be present within /sys/bus/pci/devices/ (cat 
'./device' for id for every entry). Even converted catpt driver from 
acpi to pci just to make sure.

I don't mind adding new constant within register.h for transparency:
	#define CATPT_PCI_PMCAPID 0x80
	#define CATPT_PCI_PMCS (CATPT_PCI_PMCAPID + PCI_PM_CTRL)

Current status for PM catpt_updatel_pci:
	catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D3hot)
	catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D0)
which looks very good to me.