[v4,01/13] ASoC: Intel: Add catpt device
diff mbox series

Message ID 20200812205753.29115-2-cezary.rojewski@intel.com
State New
Headers show
Series
  • ASoC: Intel: Catpt - Lynx and Wildcat point
Related show

Commit Message

Cezary Rojewski Aug. 12, 2020, 8:57 p.m. UTC
Declare base structures, registers and device routines for the catpt
solution. Catpt deprecates and is a direct replacement for
sound/soc/intel/haswell. Supports Lynxpoint and Wildcat Point both.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/core.h      | 187 +++++++++++++++
 sound/soc/intel/catpt/device.c    | 376 ++++++++++++++++++++++++++++++
 sound/soc/intel/catpt/registers.h | 191 +++++++++++++++
 3 files changed, 754 insertions(+)
 create mode 100644 sound/soc/intel/catpt/core.h
 create mode 100644 sound/soc/intel/catpt/device.c
 create mode 100644 sound/soc/intel/catpt/registers.h

Comments

Andy Shevchenko Aug. 13, 2020, 6:29 p.m. UTC | #1
On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:
> Declare base structures, registers and device routines for the catpt
> solution. Catpt deprecates and is a direct replacement for
> sound/soc/intel/haswell. Supports Lynxpoint and Wildcat Point both.

Before doing v5 give a chance to review entire series, please!

...

> +static inline bool catpt_resource_overlapping(struct resource *r1,
> +					      struct resource *r2,
> +					      struct resource *ret)
> +{
> +	if (!resource_overlaps(r1, r2))
> +		return false;
> +	ret->start = max(r1->start, r2->start);
> +	ret->end = min(r1->end, r2->end);
> +	return true;
> +}

JFYI, I have just submitted a series [1] that includes this helper [2]
to be available for all.

[1]: https://lore.kernel.org/linux-acpi/20200813175729.15088-1-andriy.shevchenko@linux.intel.com/
[2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@linux.intel.com/

...

> +struct catpt_dev {
> +	struct device *dev;

> +	struct dw_dma_chip *dmac;

Is it possible to use opaque pointer here? It will be better if in the future
(I think unlikely, but still) somebody decides to use this with another DMA
engine.

> +	struct catpt_ipc ipc;
> +
> +	void __iomem *pci_ba;
> +	void __iomem *lpe_ba;
> +	u32 lpe_base;
> +	int irq;
> +
> +	const struct catpt_spec *spec;
> +	struct completion fw_ready;
> +
> +	struct resource dram;
> +	struct resource iram;
> +	struct resource *scratch;
> +
> +	struct catpt_mixer_stream_info mixer;
> +	struct catpt_module_type modules[CATPT_MODULE_COUNT];
> +	struct catpt_ssp_device_format devfmt[CATPT_SSP_COUNT];
> +	struct list_head stream_list;
> +	spinlock_t list_lock;
> +	struct mutex clk_mutex;
> +
> +	struct catpt_dx_context dx_ctx;
> +	void *dxbuf_vaddr;
> +	dma_addr_t dxbuf_paddr;
> +};

...

> +#define CATPT_IPC_ERROR(err) ((err < 0) ? err : -EREMOTEIO)

err -> (err) in all cases of right side.

...

> +struct catpt_stream_runtime {
> +	struct snd_pcm_substream *substream;
> +
> +	struct catpt_stream_template *template;
> +	struct catpt_stream_info info;
> +	struct resource *persistent;
> +	struct snd_dma_buffer pgtbl;
> +

> +	bool allocated:1;
> +	bool prepared:1;

Does this ':1' make any sense?

> +	struct list_head node;
> +};

...

> +#ifdef CONFIG_PM

Perhaps __maybe_unused?

...

> +	ret = catpt_dsp_stall(cdev, true);
> +	if (ret < 0)

I'm wondering if all these ' < 0' all over the code make sense? What do you
expect out of positive returned values if any?

> +		goto exit;

...

> +#ifdef CONFIG_PM_SLEEP

Perhaps __maybe_unused?

...

> +	board = platform_device_register_data(NULL, mach->drv_name,
> +					PLATFORM_DEVID_NONE,
> +					(const void *)mach, sizeof(*mach));
> +	if (!board) {

Here obviously not correct check.

> +		dev_err(cdev->dev, "board register failed\n");
> +		return PTR_ERR(board);
> +	}
> +
> +	ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
> +	if (ret < 0) {
> +		platform_device_unregister(board);

> +		return ret;
> +	}
> +
> +	return 0;

return ret;

...

> +	mutex_init(&cdev->clk_mutex);

+ blank line.

> +	/*
> +	 * Mark both device formats as uninitialized. Once corresponding
> +	 * cpu_dai's pcm is created, proper values are assigned

Please, use period(s) in multi-line comments.

> +	 */

...

> +static int catpt_acpi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct catpt_dev *cdev;
> +	struct resource *res;
> +	int ret;
> +
> +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> +	if (!cdev)
> +		return -ENOMEM;


> +	cdev->spec = device_get_match_data(dev);
> +	if (!cdev->spec)
> +		return -ENODEV;

You may save some cycles if you do this before memory allocations.

> +	catpt_dev_init(cdev, dev);
> +
> +	/* map DSP bar address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!cdev->lpe_ba)
> +		return -EIO;
> +	cdev->lpe_base = res->start;

Why the region is not get requested?

> +	/* map PCI bar address */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -ENODEV;
> +	cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!cdev->pci_ba)
> +		return -EIO;

Ditto.

> +	/* alloc buffer for storing DRAM context during dx transitions */
> +	cdev->dxbuf_vaddr = dma_alloc_coherent(dev, catpt_dram_size(cdev),

dmam_alloc_coherent() ?

> +					       &cdev->dxbuf_paddr, GFP_KERNEL);
> +	if (!cdev->dxbuf_vaddr)
> +		return -ENOMEM;

> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		goto irq_err;
> +	cdev->irq = ret;

But you may return directly if you get IRQ resource before allocation (despite
previous comment).

> +
> +	platform_set_drvdata(pdev, cdev);
> +
> +	ret = devm_request_threaded_irq(dev, cdev->irq, catpt_dsp_irq_handler,
> +					catpt_dsp_irq_thread,
> +					IRQF_SHARED, "AudioDSP", cdev);
> +	if (ret < 0)
> +		goto irq_err;
> +
> +	ret = catpt_probe_components(cdev);

return ...

> +	if (ret < 0)
> +		goto irq_err;
> +	return 0;
> +
> +irq_err:
> +	dma_free_coherent(cdev->dev, catpt_dram_size(cdev),
> +			  cdev->dxbuf_vaddr, cdev->dxbuf_paddr);
> +
> +	return ret;

This will be gone...

> +}

...

> +static const struct acpi_device_id catpt_ids[] = {
> +	{ "INT33C8", (unsigned long)&lpt_desc },
> +	{ "INT3438", (unsigned long)&wpt_desc },

> +	{ },

No need to have comma in terminator line.

> +};

...

> +static struct platform_driver catpt_acpi_driver = {
> +	.probe = catpt_acpi_probe,
> +	.remove = catpt_acpi_remove,
> +	.driver = {
> +		.name = "catpt_adsp",

> +		.acpi_match_table = ACPI_PTR(catpt_ids),

ACPI_PTR() either bogus (when you have depends on ACPI) or mistake that brings
you compiler warning (unused variable).

I highly recommend in new code avoid completely ACPI_PTR() and of_match_ptr()
macros.

> +		.pm = &catpt_dev_pm,
> +	},
> +};

...

> +#include <linux/iopoll.h>

Missed headers:
bits.h (note, the below guarantees to provide this one)
bitops.h
io.h (writel(), readl(), etc)

...

> +/* DSP Shim registers */
> +
> +#define CATPT_SHIM_CS1		0x0

Please, keep all register definitions of the same width, like 0x00 here or so.

...

> +#define CATPT_CS_SFCR(ssp)	BIT(27 + ssp)

In all macros, try to be a little bit defensive, e.g. here ssp -> (ssp).

...

> +#define CATPT_HMDC_HDDA(e, ch)	BIT(8 * e + ch)

...or e -> (e) and ch -> (ch) here.

...

> +#define CATPT_CS_DEFAULT	0x8480040E
> +#define CATPT_IMC_DEFAULT	0x7FFF0003
> +#define CATPT_IMD_DEFAULT	0x7FFF0003
> +#define CATPT_CLKCTL_DEFAULT	0x7FF

These looks like set of bit fields, can we describe them either in comments
or in the values like GENMASK(x, y) | BIT(z) ?

...

> +/* PCI Configuration registers */
> +
> +#define CATPT_PCI_PMCS		0x84

Why?! We have PCI capability and entire infrastructure for that in PCI core.

...

> +#define CATPT_PMCS_PS		GENMASK(1, 0)
> +#define CATPT_PMCS_PS_D3HOT	(0x3 << 0)

Ditto.

...

> +#define CATPT_SSP_SSC0		0x0
> +#define CATPT_SSP_SSC1		0x4
> +#define CATPT_SSP_SSS		0x8
> +#define CATPT_SSP_SSIT		0xC
> +#define CATPT_SSP_SSD		0x10
> +#define CATPT_SSP_SSTO		0x28
> +#define CATPT_SSP_SSPSP		0x2C
> +#define CATPT_SSP_SSTSA		0x30
> +#define CATPT_SSP_SSRSA		0x34
> +#define CATPT_SSP_SSTSS		0x38
> +#define CATPT_SSP_SSC2		0x40
> +#define CATPT_SSP_SSPSP2	0x44

Isn't it PXA2xx register set? Can you use their definitions?

...

> +#define CATPT_SSP_SSC0_DEFAULT		0x0
> +#define CATPT_SSP_SSC1_DEFAULT		0x0
> +#define CATPT_SSP_SSS_DEFAULT		0xF004
> +#define CATPT_SSP_SSIT_DEFAULT		0x0
> +#define CATPT_SSP_SSD_DEFAULT		0xC43893A3
> +#define CATPT_SSP_SSTO_DEFAULT		0x0
> +#define CATPT_SSP_SSPSP_DEFAULT		0x0
> +#define CATPT_SSP_SSTSA_DEFAULT		0x0
> +#define CATPT_SSP_SSRSA_DEFAULT		0x0
> +#define CATPT_SSP_SSTSS_DEFAULT		0x0
> +#define CATPT_SSP_SSC2_DEFAULT		0x0
> +#define CATPT_SSP_SSPSP2_DEFAULT	0x0

These defaults lack of comments.
Cezary Rojewski Aug. 17, 2020, 10:02 a.m. UTC | #2
On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
> On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:

Thanks for good review Andy!


>> +static inline bool catpt_resource_overlapping(struct resource *r1,
>> +					      struct resource *r2,
>> +					      struct resource *ret)
>> +{
>> +	if (!resource_overlaps(r1, r2))
>> +		return false;
>> +	ret->start = max(r1->start, r2->start);
>> +	ret->end = min(r1->end, r2->end);
>> +	return true;
>> +}
> 
> JFYI, I have just submitted a series [1] that includes this helper [2]
> to be available for all.
> 
> [1]: https://lore.kernel.org/linux-acpi/20200813175729.15088-1-andriy.shevchenko@linux.intel.com/
> [2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@linux.intel.com/
> 

Well, I'm happy that catpt somewhat impacted resource-API getting more 
flexble, although it would be nice to get --cc'ed as 
_overlapping/_intersecting got moved into general part of kernel without 
changes, basically.

This raises a dependancy issue, am I right? i.e. until this gets merged, 
catpt will cause compilation errors on Mark's for-next. -or- perhaps you 
want me to leave things as they are for current release while removing 
said function later, once your PR get's merged?

>> +struct catpt_dev {
>> +	struct device *dev;
> 
>> +	struct dw_dma_chip *dmac;
> 
> Is it possible to use opaque pointer here? It will be better if in the future
> (I think unlikely, but still) somebody decides to use this with another DMA
> engine.
> 

Any opaque structure comes at a cost -> requires higher level of 
understanding from developers maintaining given piece of code (that 
includes architecture knowledge too, to get a grasp of why such decision 
was even made) == higher maintaince cost.

One could device ADSP architectures into:
1) LPT/WPT
2) BYT/CHT/BDW
3) cAVS (SKL+)
4) new (which I won't be elaborating here for obvious reasons)

To my knowledge, except for 1), none of them makes use of dmaengine.h 
when loading FW or doing any other action for that matter. As such, I 
don't see any reason to convert something explicit into something 
implicit. Don't believe either of options would be reusing struct 
catpt_dev too. In general, to make that happen you'd have to start with 
conversion of existing HDAudio transport (cAVS+) into dmaengine model 
and then do the same with SoundWire (cAVS+) - haven't seen sdw code in a 
while but still pretty sure it's not dmaengine-friendly.

> 
>> +#define CATPT_IPC_ERROR(err) ((err < 0) ? err : -EREMOTEIO)
> 
> err -> (err) in all cases of right side.
> 

Ack.

> 
>> +struct catpt_stream_runtime {
>> +	struct snd_pcm_substream *substream;
>> +
>> +	struct catpt_stream_template *template;
>> +	struct catpt_stream_info info;
>> +	struct resource *persistent;
>> +	struct snd_dma_buffer pgtbl;
>> +
> 
>> +	bool allocated:1;
>> +	bool prepared:1;
> 
> Does this ':1' make any sense?
> 

In current state it does not, really. Playing internally with segments 
which are not part of this release (as noted in cover-letter) where some 
of these did. Will remove in v5.

>> +	struct list_head node;
>> +};
> 
> ...
> 
>> +#ifdef CONFIG_PM
> 
> Perhaps __maybe_unused?
> 

Sure, removal of #ifdefs is always nice.

> 
>> +	ret = catpt_dsp_stall(cdev, true);
>> +	if (ret < 0)
> 
> I'm wondering if all these ' < 0' all over the code make sense? What do you
> expect out of positive returned values if any?
> 

Isn't this more of a preference? Please note I'm basing many of my 
decisions on code that's around me - /sound/core/ and sound/soc/ *.c.

Except for IPCs, basically all catpt rets retrieved from functions 
called will be returning either 0 (success) or <0 (error). No 
objections, but I don't see much difference either.

>> +		goto exit;

> 
>> +#ifdef CONFIG_PM_SLEEP
> 
> Perhaps __maybe_unused?
> 

Same as above~1, ack.

> 
>> +	board = platform_device_register_data(NULL, mach->drv_name,
>> +					PLATFORM_DEVID_NONE,
>> +					(const void *)mach, sizeof(*mach));
>> +	if (!board) {
> 
> Here obviously not correct check.
> 

Indeed, ack.

>> +		dev_err(cdev->dev, "board register failed\n");
>> +		return PTR_ERR(board);
>> +	}
>> +
>> +	ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
>> +	if (ret < 0) {
>> +		platform_device_unregister(board);
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> return ret;
> 

Similarly, to comment~2 regarding preferences, don't mind the change (in 
fact, I'm a fan) but in the past got messaged to leave things explicit - 
leave last 'if' with return ret, while return 0 marking success outside.

> 
>> +	mutex_init(&cdev->clk_mutex);
> 
> + blank line.
> 

Thought wide comment makes enough distance already.

>> +	/*
>> +	 * Mark both device formats as uninitialized. Once corresponding
>> +	 * cpu_dai's pcm is created, proper values are assigned
> 
> Please, use period(s) in multi-line comments.
> 

Used to: all-but-last sentence with period(s). Will update as requested 
in v5.

> 
>> +static int catpt_acpi_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct catpt_dev *cdev;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
>> +	if (!cdev)
>> +		return -ENOMEM;
> 
> 
>> +	cdev->spec = device_get_match_data(dev);
>> +	if (!cdev->spec)
>> +		return -ENODEV;
> 
> You may save some cycles if you do this before memory allocations.
> 

i.e. define a local for spec, assign and begin the init process only 
once it's found? Isn't that a loss in most cases? Comes down to:

	declare local + later cdev->spec = spec assignment
	vs
	unlikely -ENODEV with memory being unnecessarily allocated

Perhaps I'm unaware of what's going on with device_get_match_data, but I 
believe .probe() won't get called until one of .acpi_match_table ids 
matches device available on the bus. Existing list of ids won't ever get 
changed as there are only two platforms available for 2011-2013 ADSP 
architecture.

>> +	catpt_dev_init(cdev, dev);
>> +
>> +	/* map DSP bar address */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -ENODEV;
>> +	cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
>> +	if (!cdev->lpe_ba)
>> +		return -EIO;
>> +	cdev->lpe_base = res->start;
> 
> Why the region is not get requested?
> 
>> +	/* map PCI bar address */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	if (!res)
>> +		return -ENODEV;
>> +	cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
>> +	if (!cdev->pci_ba)
>> +		return -EIO;
> 
> Ditto.
> 

Comes from catpt_dmac_probe() (dsp.c) making use of 
devm_ioremap_resource(). If you _get_ requested resource there, the 
function called in catpt_dmac_probe() will yielrd -EBUSY.

This is based on existing code:
/sound/soc/intel/common/sst-acpi.c ::sst_acpi_probe() see mmio assignments.
/sound/soc/intel/common/sst-firmate.ce ::dw_probe() see chip->regs 
assignment.

Perhaps you've found even more problems in existing code than I did..

>> +	/* alloc buffer for storing DRAM context during dx transitions */
>> +	cdev->dxbuf_vaddr = dma_alloc_coherent(dev, catpt_dram_size(cdev),
> 
> dmam_alloc_coherent() ?
> 

Nice! Wasn't aware of this helper. Simplifies error-path too.

>> +					       &cdev->dxbuf_paddr, GFP_KERNEL);
>> +	if (!cdev->dxbuf_vaddr)
>> +		return -ENOMEM;
> 
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0)
>> +		goto irq_err;
>> +	cdev->irq = ret;
> 
> But you may return directly if you get IRQ resource before allocation (despite
> previous comment).
> 

Indeed, reordering irq-request and dxbuf allocation would alloc for 
s/goto irq_err/return <err>/
Error-path wouldn't be removed though, as final operation - 
catpt_probe_components - must be verified before leaving scope.

>> +
>> +	platform_set_drvdata(pdev, cdev);
>> +
>> +	ret = devm_request_threaded_irq(dev, cdev->irq, catpt_dsp_irq_handler,
>> +					catpt_dsp_irq_thread,
>> +					IRQF_SHARED, "AudioDSP", cdev);
>> +	if (ret < 0)
>> +		goto irq_err;
>> +
>> +	ret = catpt_probe_components(cdev);
> 
> return ...
> 

With dmam_xxx helper, true.

>> +	if (ret < 0)
>> +		goto irq_err;
>> +	return 0;
>> +
>> +irq_err:
>> +	dma_free_coherent(cdev->dev, catpt_dram_size(cdev),
>> +			  cdev->dxbuf_vaddr, cdev->dxbuf_paddr);
>> +
>> +	return ret;
> 
> This will be gone...
> 

Ditto. Thanks!

>> +}
> 
> ...
> 
>> +static const struct acpi_device_id catpt_ids[] = {
>> +	{ "INT33C8", (unsigned long)&lpt_desc },
>> +	{ "INT3438", (unsigned long)&wpt_desc },
> 
>> +	{ },
> 
> No need to have comma in terminator line.
> 

Well, that's a habbit to add a ',' at the end of each enumeration line 
and I bet it's a good one. No problem removing this one though.

>> +};
> 
> ...
> 
>> +static struct platform_driver catpt_acpi_driver = {
>> +	.probe = catpt_acpi_probe,
>> +	.remove = catpt_acpi_remove,
>> +	.driver = {
>> +		.name = "catpt_adsp",
> 
>> +		.acpi_match_table = ACPI_PTR(catpt_ids),
> 
> ACPI_PTR() either bogus (when you have depends on ACPI) or mistake that brings
> you compiler warning (unused variable).
> 
> I highly recommend in new code avoid completely ACPI_PTR() and of_match_ptr()
> macros.
> 

That's something new for me. Thanks for a good advice.

>> +		.pm = &catpt_dev_pm,
>> +	},
>> +};
> 
> ...
> 
>> +#include <linux/iopoll.h>
> 
> Missed headers:
> bits.h (note, the below guarantees to provide this one)
> bitops.h
> io.h (writel(), readl(), etc)
> 

Removed these as registers.h always gets included with other files which 
already inhering them via nesting.
Will update in v5 as requested.

>> +/* DSP Shim registers */
>> +
>> +#define CATPT_SHIM_CS1		0x0
> 
> Please, keep all register definitions of the same width, like 0x00 here or so.
> 

Ack.

> 
>> +#define CATPT_CS_SFCR(ssp)	BIT(27 + ssp)
> 
> In all macros, try to be a little bit defensive, e.g. here ssp -> (ssp).
> 
> ...
> 
>> +#define CATPT_HMDC_HDDA(e, ch)	BIT(8 * e + ch)
> 
> ...or e -> (e) and ch -> (ch) here.
> 

Agreed, will update in v5.

>> +#define CATPT_CS_DEFAULT	0x8480040E
>> +#define CATPT_IMC_DEFAULT	0x7FFF0003
>> +#define CATPT_IMD_DEFAULT	0x7FFF0003
>> +#define CATPT_CLKCTL_DEFAULT	0x7FF
> 
> These looks like set of bit fields, can we describe them either in comments
> or in the values like GENMASK(x, y) | BIT(z) ?
> 

Let's go with the latter. As explained below, I don't have much info in 
regard to re-setting registers to their defaults. This knowldge might 
come in time (and a ton of testing) but certainly, won't be part of this 
release.

One issue might arise when describing the "reserved" regions as some 
bits should not be modified by sw normally, but are part of "recommended 
sequence" anyway. I'll see if there are any among '1's.

>> +/* PCI Configuration registers */
>> +
>> +#define CATPT_PCI_PMCS		0x84
> 
> Why?! We have PCI capability and entire infrastructure for that in PCI core.
> 
> ...
> 
>> +#define CATPT_PMCS_PS		GENMASK(1, 0)
>> +#define CATPT_PMCS_PS_D3HOT	(0x3 << 0)
> 
> Ditto.
> 

No need for astonishment : ) Wasn't aware of this, in fact, I count on 
more experienced kernel developers - like you Andy - to help others in 
learning about such improvements. Certainly, this isn't knowledge one is 
going to inherit from developing drivers in Windows environment.

Ack.

> 
>> +#define CATPT_SSP_SSC0		0x0
>> +#define CATPT_SSP_SSC1		0x4
>> +#define CATPT_SSP_SSS		0x8
>> +#define CATPT_SSP_SSIT		0xC
>> +#define CATPT_SSP_SSD		0x10
>> +#define CATPT_SSP_SSTO		0x28
>> +#define CATPT_SSP_SSPSP		0x2C
>> +#define CATPT_SSP_SSTSA		0x30
>> +#define CATPT_SSP_SSRSA		0x34
>> +#define CATPT_SSP_SSTSS		0x38
>> +#define CATPT_SSP_SSC2		0x40
>> +#define CATPT_SSP_SSPSP2	0x44
> 
> Isn't it PXA2xx register set? Can you use their definitions?
> 

Could you be more specific? Wasn't able to find anything useful in /include.

> 
>> +#define CATPT_SSP_SSC0_DEFAULT		0x0
>> +#define CATPT_SSP_SSC1_DEFAULT		0x0
>> +#define CATPT_SSP_SSS_DEFAULT		0xF004
>> +#define CATPT_SSP_SSIT_DEFAULT		0x0
>> +#define CATPT_SSP_SSD_DEFAULT		0xC43893A3
>> +#define CATPT_SSP_SSTO_DEFAULT		0x0
>> +#define CATPT_SSP_SSPSP_DEFAULT		0x0
>> +#define CATPT_SSP_SSTSA_DEFAULT		0x0
>> +#define CATPT_SSP_SSRSA_DEFAULT		0x0
>> +#define CATPT_SSP_SSTSS_DEFAULT		0x0
>> +#define CATPT_SSP_SSC2_DEFAULT		0x0
>> +#define CATPT_SSP_SSPSP2_DEFAULT	0x0
> 
> These defaults lack of comments.
> 

Because there aren't any available to choose from. While these are part 
of "recommended sequence", the only comment attached is:
     bring hw to their defaults as hw won't reset itself

catpt is an effort of sw and fw guys, no hw input is included as I've 
found only one guy still @ intel but he is busy with different projects 
and honestly, even if he would agree, him digging now why was this 
needed might take weeks. That's 2011 ADSP architecture, not some 
cutting-edge stuff.
Andy Shevchenko Aug. 18, 2020, 10:07 a.m. UTC | #3
On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
> On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
> > On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:
> 
> Thanks for good review Andy!

You're welcome!

...

> > JFYI, I have just submitted a series [1] that includes this helper [2]
> > to be available for all.
> > 
> > [1]: https://lore.kernel.org/linux-acpi/20200813175729.15088-1-andriy.shevchenko@linux.intel.com/
> > [2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@linux.intel.com/
> > 
> 
> Well, I'm happy that catpt somewhat impacted resource-API getting more
> flexble, although it would be nice to get --cc'ed as
> _overlapping/_intersecting got moved into general part of kernel without
> changes, basically.
> 
> This raises a dependancy issue, am I right? i.e. until this gets merged,
> catpt will cause compilation errors on Mark's for-next. -or- perhaps you
> want me to leave things as they are for current release while removing said
> function later, once your PR get's merged?

I hope Rafael accepts the v2 soon and thus may provide an immutable branch to
be consumed by others. That's the way how usually we solve cross-subsystem
series.

In any case we still have time for better review of the rest of the series.

...

> > > +struct catpt_dev {
> > > +	struct device *dev;
> > 
> > > +	struct dw_dma_chip *dmac;
> > 
> > Is it possible to use opaque pointer here? It will be better if in the future
> > (I think unlikely, but still) somebody decides to use this with another DMA
> > engine.
> 
> Any opaque structure comes at a cost -> requires higher level of
> understanding from developers maintaining given piece of code (that includes
> architecture knowledge too, to get a grasp of why such decision was even
> made) == higher maintaince cost.
> 
> One could device ADSP architectures into:
> 1) LPT/WPT
> 2) BYT/CHT/BDW
> 3) cAVS (SKL+)
> 4) new (which I won't be elaborating here for obvious reasons)
> 
> To my knowledge, except for 1), none of them makes use of dmaengine.h when
> loading FW or doing any other action for that matter. As such, I don't see
> any reason to convert something explicit into something implicit. Don't
> believe either of options would be reusing struct catpt_dev too. In general,
> to make that happen you'd have to start with conversion of existing HDAudio
> transport (cAVS+) into dmaengine model and then do the same with SoundWire
> (cAVS+) - haven't seen sdw code in a while but still pretty sure it's not
> dmaengine-friendly.


Some documentation says that Audio is using iDMA 32-bit in (some?) BSW/CHT,
some documentation says otherwise (Synopsys DesignWare). Can you somewhere
clarify what the actual state of affairs? I remember even some (android?) ASoC
code used to have different type of DMA engines because of that.

...

> > > +	if (ret < 0)
> > 
> > I'm wondering if all these ' < 0' all over the code make sense? What do you
> > expect out of positive returned values if any?
> > 
> 
> Isn't this more of a preference? Please note I'm basing many of my decisions
> on code that's around me - /sound/core/ and sound/soc/ *.c.
> 
> Except for IPCs, basically all catpt rets retrieved from functions called
> will be returning either 0 (success) or <0 (error). No objections, but I
> don't see much difference either.

In case some will return positive code you may hide the (potential) issue.
I prefer be explicit over implicit, means use ' < 0' only where it makes sense.

> > > +		goto exit;

...

> > > +	ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
> > > +	if (ret < 0) {
> > > +		platform_device_unregister(board);
> > 
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > 
> > return ret;
> > 
> 
> Similarly, to comment~2 regarding preferences, don't mind the change (in
> fact, I'm a fan) but in the past got messaged to leave things explicit -
> leave last 'if' with return ret, while return 0 marking success outside.

Actually you may simplify by calling devm_add_action_or_reset() instead.

...

> > > +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> > > +	if (!cdev)
> > > +		return -ENOMEM;
> > 
> > 
> > > +	cdev->spec = device_get_match_data(dev);
> > > +	if (!cdev->spec)
> > > +		return -ENODEV;
> > 
> > You may save some cycles if you do this before memory allocations.
> > 
> 
> i.e. define a local for spec, assign and begin the init process only once
> it's found? Isn't that a loss in most cases? Comes down to:
> 
> 	declare local + later cdev->spec = spec assignment
> 	vs
> 	unlikely -ENODEV with memory being unnecessarily allocated
> 
> Perhaps I'm unaware of what's going on with device_get_match_data, but I
> believe .probe() won't get called until one of .acpi_match_table ids matches
> device available on the bus. Existing list of ids won't ever get changed as
> there are only two platforms available for 2011-2013 ADSP architecture.

Up to you but I consider cleaner code if we don't do heavier operation ahead
when lighter ones can fail.

...

> > > +	/* map DSP bar address */
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res)
> > > +		return -ENODEV;
> > > +	cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
> > > +	if (!cdev->lpe_ba)
> > > +		return -EIO;
> > > +	cdev->lpe_base = res->start;
> > 
> > Why the region is not get requested?
> > 
> > > +	/* map PCI bar address */
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +	if (!res)
> > > +		return -ENODEV;
> > > +	cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
> > > +	if (!cdev->pci_ba)
> > > +		return -EIO;
> > 
> > Ditto.
> > 
> 
> Comes from catpt_dmac_probe() (dsp.c) making use of devm_ioremap_resource().
> If you _get_ requested resource there, the function called in
> catpt_dmac_probe() will yielrd -EBUSY.
> 
> This is based on existing code:
> /sound/soc/intel/common/sst-acpi.c ::sst_acpi_probe() see mmio assignments.
> /sound/soc/intel/common/sst-firmate.ce ::dw_probe() see chip->regs
> assignment.
> 
> Perhaps you've found even more problems in existing code than I did..

Hmm... But isn't catpt_dmac_probe(), actually what is in the DMA engine driver
beneath, should take care of the requesting *and* remapping resource?

...

> > > +		.acpi_match_table = ACPI_PTR(catpt_ids),
> > 
> > ACPI_PTR() either bogus (when you have depends on ACPI) or mistake that brings
> > you compiler warning (unused variable).
> > 
> > I highly recommend in new code avoid completely ACPI_PTR() and of_match_ptr()
> > macros.
> > 
> 
> That's something new for me. Thanks for a good advice.

Basically of_match_ptr / ACPI_PTR should go together with ugly ifdeffery,
otherwise neither of them should be present. If the driver can be compiled but
won't be functional w/o OF / ACPI dependency, then we do something like

	depends on ACPI || COMPILE_TEST

to give a hint to the user.

...

> > > +#include <linux/iopoll.h>
> > 
> > Missed headers:
> > bits.h (note, the below guarantees to provide this one)
> > bitops.h
> > io.h (writel(), readl(), etc)
> > 
> 
> Removed these as registers.h always gets included with other files which
> already inhering them via nesting.
> Will update in v5 as requested.

The rule of thumb is to include headers which we are direct users of.
This is listed in Submit Patches Checklist document AFAIR.

...

> > > +#define CATPT_SSP_SSC0		0x0
> > > +#define CATPT_SSP_SSC1		0x4
> > > +#define CATPT_SSP_SSS		0x8
> > > +#define CATPT_SSP_SSIT		0xC
> > > +#define CATPT_SSP_SSD		0x10
> > > +#define CATPT_SSP_SSTO		0x28
> > > +#define CATPT_SSP_SSPSP		0x2C
> > > +#define CATPT_SSP_SSTSA		0x30
> > > +#define CATPT_SSP_SSRSA		0x34
> > > +#define CATPT_SSP_SSTSS		0x38
> > > +#define CATPT_SSP_SSC2		0x40
> > > +#define CATPT_SSP_SSPSP2	0x44
> > 
> > Isn't it PXA2xx register set? Can you use their definitions?
> > 
> 
> Could you be more specific? Wasn't able to find anything useful in /include.

include/linux/pxa2xx_ssp.h

...

> > These defaults lack of comments.
> > 
> 
> Because there aren't any available to choose from. While these are part of
> "recommended sequence", the only comment attached is:
>     bring hw to their defaults as hw won't reset itself
> 
> catpt is an effort of sw and fw guys, no hw input is included as I've found
> only one guy still @ intel but he is busy with different projects and
> honestly, even if he would agree, him digging now why was this needed might
> take weeks. That's 2011 ADSP architecture, not some cutting-edge stuff.

I understand, but try your best to leave at least a little trail of these...
Sometimes, btw, so called Production Kernel repository (patches there) may give
a hint. Lately, during AtomISP v2 resurrection, it appears that Intel Aero
platform has support there and a lot of quirks available somewhere.
Cezary Rojewski Aug. 19, 2020, 1:26 p.m. UTC | #4
On 2020-08-18 12:07 PM, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
>> On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
>>> On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:
>>
>> Thanks for good review Andy!
> 
> You're welcome!
> 

>>>> +struct catpt_dev {
>>>> +	struct device *dev;
>>>
>>>> +	struct dw_dma_chip *dmac;
>>>
>>> Is it possible to use opaque pointer here? It will be better if in the future
>>> (I think unlikely, but still) somebody decides to use this with another DMA
>>> engine.
>>
>> Any opaque structure comes at a cost -> requires higher level of
>> understanding from developers maintaining given piece of code (that includes
>> architecture knowledge too, to get a grasp of why such decision was even
>> made) == higher maintaince cost.
>>
>> One could device ADSP architectures into:
>> 1) LPT/WPT
>> 2) BYT/CHT/BDW
>> 3) cAVS (SKL+)
>> 4) new (which I won't be elaborating here for obvious reasons)
>>
>> To my knowledge, except for 1), none of them makes use of dmaengine.h when
>> loading FW or doing any other action for that matter. As such, I don't see
>> any reason to convert something explicit into something implicit. Don't
>> believe either of options would be reusing struct catpt_dev too. In general,
>> to make that happen you'd have to start with conversion of existing HDAudio
>> transport (cAVS+) into dmaengine model and then do the same with SoundWire
>> (cAVS+) - haven't seen sdw code in a while but still pretty sure it's not
>> dmaengine-friendly.
> 
> 
> Some documentation says that Audio is using iDMA 32-bit in (some?) BSW/CHT,
> some documentation says otherwise (Synopsys DesignWare). Can you somewhere
> clarify what the actual state of affairs? I remember even some (android?) ASoC
> code used to have different type of DMA engines because of that.

Well, we are supporting Android for HDA-based platforms only. At least 
that's true for Androids my team is covering. LPT/WPT and BYT/CHT/BSW 
architectures are not part of that coverage (and I'm not aware of any 
support for these on Android, probably some hard-maintainance with no 
possibility of changes). As HDA DMAs are made use of during image 
loading in cAVS+, there is no need for enlisting DW DMAC.

BYT/CHT/BSW support moved to /sound/soc/intel/atom (away from 
/sound/soc/intel/baytrail in case of BYT) mostly with support available 
in SOF too. Support for that architecture in either of the solutions is 
not within my area of expertise but I don't believe any DMAC is enlisted 
there either.

>>>> +	if (ret < 0)
>>>
>>> I'm wondering if all these ' < 0' all over the code make sense? What do you
>>> expect out of positive returned values if any?
>>>
>>
>> Isn't this more of a preference? Please note I'm basing many of my decisions
>> on code that's around me - /sound/core/ and sound/soc/ *.c.
>>
>> Except for IPCs, basically all catpt rets retrieved from functions called
>> will be returning either 0 (success) or <0 (error). No objections, but I
>> don't see much difference either.
> 
> In case some will return positive code you may hide the (potential) issue.
> I prefer be explicit over implicit, means use ' < 0' only where it makes sense.
> 

Ack.

>>>> +	ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
>>>> +	if (ret < 0) {
>>>> +		platform_device_unregister(board);
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>
>>> return ret;
>>>
>>
>> Similarly, to comment~2 regarding preferences, don't mind the change (in
>> fact, I'm a fan) but in the past got messaged to leave things explicit -
>> leave last 'if' with return ret, while return 0 marking success outside.
> 
> Actually you may simplify by calling devm_add_action_or_reset() instead.
> 

Indeed, that simplifies things. Ack.

>>>> +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
>>>> +	if (!cdev)
>>>> +		return -ENOMEM;
>>>
>>>
>>>> +	cdev->spec = device_get_match_data(dev);
>>>> +	if (!cdev->spec)
>>>> +		return -ENODEV;
>>>
>>> You may save some cycles if you do this before memory allocations.
>>>
>>
>> i.e. define a local for spec, assign and begin the init process only once
>> it's found? Isn't that a loss in most cases? Comes down to:
>>
>> 	declare local + later cdev->spec = spec assignment
>> 	vs
>> 	unlikely -ENODEV with memory being unnecessarily allocated
>>
>> Perhaps I'm unaware of what's going on with device_get_match_data, but I
>> believe .probe() won't get called until one of .acpi_match_table ids matches
>> device available on the bus. Existing list of ids won't ever get changed as
>> there are only two platforms available for 2011-2013 ADSP architecture.
> 
> Up to you but I consider cleaner code if we don't do heavier operation ahead
> when lighter ones can fail.
> 

And this is a very good approach. Thought device_get_match_data is 
'heavier' than the devm_kzalloc.

>>>> +	/* map DSP bar address */
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res)
>>>> +		return -ENODEV;
>>>> +	cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
>>>> +	if (!cdev->lpe_ba)
>>>> +		return -EIO;
>>>> +	cdev->lpe_base = res->start;
>>>
>>> Why the region is not get requested?
>>>
>>>> +	/* map PCI bar address */
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +	if (!res)
>>>> +		return -ENODEV;
>>>> +	cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
>>>> +	if (!cdev->pci_ba)
>>>> +		return -EIO;
>>>
>>> Ditto.
>>>
>>
>> Comes from catpt_dmac_probe() (dsp.c) making use of devm_ioremap_resource().
>> If you _get_ requested resource there, the function called in
>> catpt_dmac_probe() will yielrd -EBUSY.
>>
>> This is based on existing code:
>> /sound/soc/intel/common/sst-acpi.c ::sst_acpi_probe() see mmio assignments.
>> /sound/soc/intel/common/sst-firmate.ce ::dw_probe() see chip->regs
>> assignment.
>>
>> Perhaps you've found even more problems in existing code than I did..
> 
> Hmm... But isn't catpt_dmac_probe(), actually what is in the DMA engine driver
> beneath, should take care of the requesting *and* remapping resource?
> 
> ...
> 

One could say ADSP subsystem in LPT/WPT is made of following modules:
- dsp (shim) space
- 2x dma (engine 0 & 1)
- 2x ssp (with 1 tasked with BT-paths and 0 for non-BT-paths)

Recommended sequences in dsp.c (_power_up/ _power_down) will be 
operating only on SHIM and SSP spaces. DMA space is actually only 
accessed when dumping memory during device coredump. Because of that 
though, I cannot say "adsp will never access DMA space".

-

I did some testing today and indeed very simple approach suffices:
- devm_platform_get_and_ioremap_resource for DSP bar
- devm_platform_ioremap_resource for PCI bar
- instead of doing devm_ioremap_resource() separately for dmac in 
catpt_dmac_probe(), just do:

dmac->regs = cdev->lpe_ba + cdev->spec->host_dma_offset[<engine id>]

>>>> +		.acpi_match_table = ACPI_PTR(catpt_ids),
>>>
>>> ACPI_PTR() either bogus (when you have depends on ACPI) or mistake that brings
>>> you compiler warning (unused variable).
>>>
>>> I highly recommend in new code avoid completely ACPI_PTR() and of_match_ptr()
>>> macros.
>>>
>>
>> That's something new for me. Thanks for a good advice.
> 
> Basically of_match_ptr / ACPI_PTR should go together with ugly ifdeffery,
> otherwise neither of them should be present. If the driver can be compiled but
> won't be functional w/o OF / ACPI dependency, then we do something like
> 
> 	depends on ACPI || COMPILE_TEST
> 
> to give a hint to the user.
> 

Ack(s) all the way.

>>>> +#include <linux/iopoll.h>
>>>
>>> Missed headers:
>>> bits.h (note, the below guarantees to provide this one)
>>> bitops.h
>>> io.h (writel(), readl(), etc)
>>>
>>
>> Removed these as registers.h always gets included with other files which
>> already inhering them via nesting.
>> Will update in v5 as requested.
> 
> The rule of thumb is to include headers which we are direct users of.
> This is listed in Submit Patches Checklist document AFAIR.
> 

Thanks for info! Ack.

>>>> +#define CATPT_SSP_SSC0		0x0
>>>> +#define CATPT_SSP_SSC1		0x4
>>>> +#define CATPT_SSP_SSS		0x8
>>>> +#define CATPT_SSP_SSIT		0xC
>>>> +#define CATPT_SSP_SSD		0x10 and
>>>> +#define CATPT_SSP_SSTO		0x28
>>>> +#define CATPT_SSP_SSPSP		0x2C
>>>> +#define CATPT_SSP_SSTSA		0x30
>>>> +#define CATPT_SSP_SSRSA		0x34
>>>> +#define CATPT_SSP_SSTSS		0x38
>>>> +#define CATPT_SSP_SSC2		0x40
>>>> +#define CATPT_SSP_SSPSP2	0x44
>>>
>>> Isn't it PXA2xx register set? Can you use their definitions?
>>>
>>
>> Could you be more specific? Wasn't able to find anything useful in /include.
> 
> include/linux/pxa2xx_ssp.h
> 

Did some reconnaissance and it while this registers are shared, LPT/WPT 
are equipped with a newer version of said ssp device with some old 
functionalities have been removed too. So the question comes down to: do 
I re-use PXA2XX registers due to historical background (inheritance) 
-or- leave it explicit as is. Honestly, I don't really mind either of 
these. Got surprised by short register names in mentioned header though.

>>> These defaults lack of comments.
>>>
>>
>> Because there aren't any available to choose from. While these are part of
>> "recommended sequence", the only comment attached is:
>>      bring hw to their defaults as hw won't reset itself
>>
>> catpt is an effort of sw and fw guys, no hw input is included as I've found
>> only one guy still @ intel but he is busy with different projects and
>> honestly, even if he would agree, him digging now why was this needed might
>> take weeks. That's 2011 ADSP architecture, not some cutting-edge stuff.
> 
> I understand, but try your best to leave at least a little trail of these...
> Sometimes, btw, so called Production Kernel repository (patches there) may give
> a hint. Lately, during AtomISP v2 resurrection, it appears that Intel Aero
> platform has support there and a lot of quirks available somewhere.
> 

I'll see what I can do.
Andy Shevchenko Aug. 19, 2020, 1:43 p.m. UTC | #5
On Wed, Aug 19, 2020 at 03:26:04PM +0200, Cezary Rojewski wrote:
> On 2020-08-18 12:07 PM, Andy Shevchenko wrote:
> > On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
> > > On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
> > > > On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:

...

> And this is a very good approach. Thought device_get_match_data is 'heavier'
> than the devm_kzalloc.

Memory allocation in 90% cases are heavier then the rest (because of page fault
exceptions).

...

> I did some testing today and indeed very simple approach suffices:
> - devm_platform_get_and_ioremap_resource for DSP bar
> - devm_platform_ioremap_resource for PCI bar
> - instead of doing devm_ioremap_resource() separately for dmac in
> catpt_dmac_probe(), just do:
> 
> dmac->regs = cdev->lpe_ba + cdev->spec->host_dma_offset[<engine id>]

Yes, something like this. Basically what you have is particular case of MFD
(see drivers/mfd for other uses).

...

> > > > > +#define CATPT_SSP_SSC0		0x0
> > > > > +#define CATPT_SSP_SSC1		0x4
> > > > > +#define CATPT_SSP_SSS		0x8
> > > > > +#define CATPT_SSP_SSIT		0xC
> > > > > +#define CATPT_SSP_SSD		0x10 and
> > > > > +#define CATPT_SSP_SSTO		0x28
> > > > > +#define CATPT_SSP_SSPSP		0x2C
> > > > > +#define CATPT_SSP_SSTSA		0x30
> > > > > +#define CATPT_SSP_SSRSA		0x34
> > > > > +#define CATPT_SSP_SSTSS		0x38
> > > > > +#define CATPT_SSP_SSC2		0x40
> > > > > +#define CATPT_SSP_SSPSP2	0x44
> > > > 
> > > > Isn't it PXA2xx register set? Can you use their definitions?
> > > 
> > > Could you be more specific? Wasn't able to find anything useful in /include.
> > 
> > include/linux/pxa2xx_ssp.h
> 
> Did some reconnaissance and it while this registers are shared, LPT/WPT are
> equipped with a newer version of said ssp device with some old
> functionalities have been removed too. So the question comes down to: do I
> re-use PXA2XX registers due to historical background (inheritance) -or-
> leave it explicit as is. Honestly, I don't really mind either of these. Got
> surprised by short register names in mentioned header though.

Short names are for historical reasons, but you may change them over the
kernel, if you wish (I think you won't spend time on this).

My vision is to extend that header to cover changes and use it in your code.
It might, though, require some cleanups to be done against pxa2xx_ssp.h.
Cezary Rojewski Aug. 25, 2020, 9:32 a.m. UTC | #6
On 2020-08-19 3:43 PM, Andy Shevchenko wrote:
> On Wed, Aug 19, 2020 at 03:26:04PM +0200, Cezary Rojewski wrote:
>> On 2020-08-18 12:07 PM, Andy Shevchenko wrote:
>>> On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
>>>> On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
>>>>> On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:

>>>>>> +#define CATPT_SSP_SSC0		0x0
>>>>>> +#define CATPT_SSP_SSC1		0x4
>>>>>> +#define CATPT_SSP_SSS		0x8
>>>>>> +#define CATPT_SSP_SSIT		0xC
>>>>>> +#define CATPT_SSP_SSD		0x10 and
>>>>>> +#define CATPT_SSP_SSTO		0x28
>>>>>> +#define CATPT_SSP_SSPSP		0x2C
>>>>>> +#define CATPT_SSP_SSTSA		0x30
>>>>>> +#define CATPT_SSP_SSRSA		0x34
>>>>>> +#define CATPT_SSP_SSTSS		0x38
>>>>>> +#define CATPT_SSP_SSC2		0x40
>>>>>> +#define CATPT_SSP_SSPSP2	0x44
>>>>>
>>>>> Isn't it PXA2xx register set? Can you use their definitions?
>>>>
>>>> Could you be more specific? Wasn't able to find anything useful in /include.
>>>
>>> include/linux/pxa2xx_ssp.h
>>
>> Did some reconnaissance and it while this registers are shared, LPT/WPT are
>> equipped with a newer version of said ssp device with some old
>> functionalities have been removed too. So the question comes down to: do I
>> re-use PXA2XX registers due to historical background (inheritance) -or-
>> leave it explicit as is. Honestly, I don't really mind either of these. Got
>> surprised by short register names in mentioned header though.
> 
> Short names are for historical reasons, but you may change them over the
> kernel, if you wish (I think you won't spend time on this).
> 
> My vision is to extend that header to cover changes and use it in your code.
> It might, though, require some cleanups to be done against pxa2xx_ssp.h.
> 

Conclusion from checking pxa2_ssp.h registers:

- SSPSP2 is missing (0x44)
- SSC2 vs SSACDD (0x40) both same offset but different purpose so 
probably new define would have to be added

As situation is similar to the resource-API case below are the options:
a) ship catpt with existing ssp reg set, update pxa2_ssp.h in following 
series and then re-use them for catpt
b) update pxa2_ssp.h first, await merge, ship catpt only afterward

I vote for option a) given the maturity driver is reaching plus I'd 
rather be done with sound/soc/intel/ sanitization sooner than later.

Czarek
Andy Shevchenko Aug. 25, 2020, 1:18 p.m. UTC | #7
On Tue, Aug 25, 2020 at 11:32:57AM +0200, Cezary Rojewski wrote:
> On 2020-08-19 3:43 PM, Andy Shevchenko wrote:
> > On Wed, Aug 19, 2020 at 03:26:04PM +0200, Cezary Rojewski wrote:

...

> > My vision is to extend that header to cover changes and use it in your code.
> > It might, though, require some cleanups to be done against pxa2xx_ssp.h.
> 
> Conclusion from checking pxa2_ssp.h registers:
> 
> - SSPSP2 is missing (0x44)
> - SSC2 vs SSACDD (0x40) both same offset but different purpose so probably
> new define would have to be added
> 
> As situation is similar to the resource-API case below are the options:
> a) ship catpt with existing ssp reg set, update pxa2_ssp.h in following
> series and then re-use them for catpt
> b) update pxa2_ssp.h first, await merge, ship catpt only afterward
> 
> I vote for option a) given the maturity driver is reaching plus I'd rather
> be done with sound/soc/intel/ sanitization sooner than later.

Luckily we have Mark to maintain both SPI and ASoC, which means you may prepend
your series with PXA2xx header update and have his Ack for it. He can create an
immutable branch and apply it to SPI tree afterwards, or other way around. So I
definitely vote for b).
Andy Shevchenko Aug. 25, 2020, 1:19 p.m. UTC | #8
On Tue, Aug 25, 2020 at 04:18:21PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 25, 2020 at 11:32:57AM +0200, Cezary Rojewski wrote:
> > On 2020-08-19 3:43 PM, Andy Shevchenko wrote:
> > > On Wed, Aug 19, 2020 at 03:26:04PM +0200, Cezary Rojewski wrote:

...

> > > My vision is to extend that header to cover changes and use it in your code.
> > > It might, though, require some cleanups to be done against pxa2xx_ssp.h.
> > 
> > Conclusion from checking pxa2_ssp.h registers:
> > 
> > - SSPSP2 is missing (0x44)
> > - SSC2 vs SSACDD (0x40) both same offset but different purpose so probably
> > new define would have to be added
> > 
> > As situation is similar to the resource-API case below are the options:
> > a) ship catpt with existing ssp reg set, update pxa2_ssp.h in following
> > series and then re-use them for catpt
> > b) update pxa2_ssp.h first, await merge, ship catpt only afterward
> > 
> > I vote for option a) given the maturity driver is reaching plus I'd rather
> > be done with sound/soc/intel/ sanitization sooner than later.
> 
> Luckily we have Mark to maintain both SPI and ASoC, which means you may prepend
> your series with PXA2xx header update and have his Ack for it. He can create an
> immutable branch and apply it to SPI tree afterwards, or other way around. So I
> definitely vote for b).

Let's say "for b+)" mean the fast track of the changes to both subsystems.
Cezary Rojewski Aug. 25, 2020, 8:43 p.m. UTC | #9
On 2020-08-17 12:02 PM, Cezary Rojewski wrote:
> On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
>> On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:

...

>>> +#define CATPT_CS_DEFAULT    0x8480040E
>>> +#define CATPT_IMC_DEFAULT    0x7FFF0003
>>> +#define CATPT_IMD_DEFAULT    0x7FFF0003
>>> +#define CATPT_CLKCTL_DEFAULT    0x7FF
>>
>> These looks like set of bit fields, can we describe them either in 
>> comments
>> or in the values like GENMASK(x, y) | BIT(z) ?
>>
> 
> Let's go with the latter. As explained below, I don't have much info in 
> regard to re-setting registers to their defaults. This knowldge might 
> come in time (and a ton of testing) but certainly, won't be part of this 
> release.
> 
> One issue might arise when describing the "reserved" regions as some 
> bits should not be modified by sw normally, but are part of "recommended 
> sequence" anyway. I'll see if there are any among '1's.
> 

In regard to your comment related to defaults, I've provided WPT ADSP 
Cspec within previously shared location:

	\\10.237.149.136\AudioDspShare\crojewsk\acpi\bdw-y
Note: Chapter 50.7 describes the register map.

What I've said in the last paragraph proved true - many (in some cases 
most..) bits are of 'Reserved' type. Because of spaghetti generated when 
attempting to mask this, I'd stick with existing, explicit default 
values which are frankly more readable.
I've added single comment above each _DEFAULT block instead:
	/* defaults to reset SSP|SHIM registers to after each power cycle */

Thanks once again for your input during 'catpt' upstream process.
Please note I'll remove the Cspec from linked location as soon as I read 
your response to this e-mail (probably tomorrow morning).

Czarek

Patch
diff mbox series

diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
new file mode 100644
index 000000000000..aa231ba9eaa9
--- /dev/null
+++ b/sound/soc/intel/catpt/core.h
@@ -0,0 +1,187 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ *
+ * Author: Cezary Rojewski <cezary.rojewski@intel.com>
+ */
+
+#ifndef __SND_SOC_INTEL_CATPT_CORE_H
+#define __SND_SOC_INTEL_CATPT_CORE_H
+
+#include <linux/dma/dw.h>
+#include <linux/irqreturn.h>
+#include "messages.h"
+#include "registers.h"
+
+void catpt_sram_init(struct resource *sram, u32 start, u32 size);
+void catpt_sram_free(struct resource *sram);
+struct resource *
+catpt_request_region(struct resource *root, resource_size_t size);
+
+static inline bool catpt_resource_overlapping(struct resource *r1,
+					      struct resource *r2,
+					      struct resource *ret)
+{
+	if (!resource_overlaps(r1, r2))
+		return false;
+	ret->start = max(r1->start, r2->start);
+	ret->end = min(r1->end, r2->end);
+	return true;
+}
+
+struct catpt_ipc_msg {
+	union {
+		u32 header;
+		union catpt_global_msg rsp;
+	};
+	void *data;
+	size_t size;
+};
+
+struct catpt_ipc {
+	struct device *dev;
+
+	struct catpt_ipc_msg rx;
+	struct catpt_fw_ready config;
+	u32 default_timeout;
+	bool ready;
+
+	spinlock_t lock;
+	struct mutex mutex;
+	struct completion done_completion;
+	struct completion busy_completion;
+};
+
+void catpt_ipc_init(struct catpt_ipc *ipc, struct device *dev);
+
+struct catpt_module_type {
+	bool loaded;
+	u32 entry_point;
+	u32 persistent_size;
+	u32 scratch_size;
+	/* DRAM, initial module state */
+	u32 state_offset;
+	u32 state_size;
+
+	struct list_head node;
+};
+
+struct catpt_spec {
+	struct snd_soc_acpi_mach *machines;
+	u8 core_id;
+	u32 host_dram_offset;
+	u32 host_iram_offset;
+	u32 host_shim_offset;
+	u32 host_dma_offset[CATPT_DMA_COUNT];
+	u32 host_ssp_offset[CATPT_SSP_COUNT];
+	u32 dram_mask;
+	u32 iram_mask;
+	void (*pll_shutdown)(struct catpt_dev *cdev, bool enable);
+	int (*power_up)(struct catpt_dev *cdev);
+	int (*power_down)(struct catpt_dev *cdev);
+};
+
+struct catpt_dev {
+	struct device *dev;
+	struct dw_dma_chip *dmac;
+	struct catpt_ipc ipc;
+
+	void __iomem *pci_ba;
+	void __iomem *lpe_ba;
+	u32 lpe_base;
+	int irq;
+
+	const struct catpt_spec *spec;
+	struct completion fw_ready;
+
+	struct resource dram;
+	struct resource iram;
+	struct resource *scratch;
+
+	struct catpt_mixer_stream_info mixer;
+	struct catpt_module_type modules[CATPT_MODULE_COUNT];
+	struct catpt_ssp_device_format devfmt[CATPT_SSP_COUNT];
+	struct list_head stream_list;
+	spinlock_t list_lock;
+	struct mutex clk_mutex;
+
+	struct catpt_dx_context dx_ctx;
+	void *dxbuf_vaddr;
+	dma_addr_t dxbuf_paddr;
+};
+
+int catpt_dmac_probe(struct catpt_dev *cdev);
+void catpt_dmac_remove(struct catpt_dev *cdev);
+struct dma_chan *catpt_dma_request_config_chan(struct catpt_dev *cdev);
+int catpt_dma_memcpy_todsp(struct catpt_dev *cdev, struct dma_chan *chan,
+			   dma_addr_t dst_addr, dma_addr_t src_addr,
+			   size_t size);
+int catpt_dma_memcpy_fromdsp(struct catpt_dev *cdev, struct dma_chan *chan,
+			     dma_addr_t dst_addr, dma_addr_t src_addr,
+			     size_t size);
+
+void lpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable);
+void wpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable);
+int lpt_dsp_power_up(struct catpt_dev *cdev);
+int lpt_dsp_power_down(struct catpt_dev *cdev);
+int wpt_dsp_power_up(struct catpt_dev *cdev);
+int wpt_dsp_power_down(struct catpt_dev *cdev);
+int catpt_dsp_stall(struct catpt_dev *cdev, bool stall);
+void catpt_dsp_update_srampge(struct catpt_dev *cdev, struct resource *sram,
+			      unsigned long mask);
+int catpt_dsp_update_lpclock(struct catpt_dev *cdev);
+irqreturn_t catpt_dsp_irq_handler(int irq, void *dev_id);
+irqreturn_t catpt_dsp_irq_thread(int irq, void *dev_id);
+
+/*
+ * IPC handlers may return positive values which denote successful
+ * HOST <-> DSP communication yet failure to process specific request.
+ * Use below macro to convert returned non-zero values appropriately
+ */
+#define CATPT_IPC_ERROR(err) ((err < 0) ? err : -EREMOTEIO)
+
+int catpt_dsp_send_msg_timeout(struct catpt_dev *cdev,
+			       struct catpt_ipc_msg request,
+			       struct catpt_ipc_msg *reply, int timeout);
+int catpt_dsp_send_msg(struct catpt_dev *cdev, struct catpt_ipc_msg request,
+		       struct catpt_ipc_msg *reply);
+
+int catpt_first_boot_firmware(struct catpt_dev *cdev);
+int catpt_boot_firmware(struct catpt_dev *cdev, bool restore);
+int catpt_store_streams_context(struct catpt_dev *cdev, struct dma_chan *chan);
+int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan);
+int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan);
+int catpt_coredump(struct catpt_dev *cdev);
+
+int catpt_sysfs_create(struct catpt_dev *cdev);
+void catpt_sysfs_remove(struct catpt_dev *cdev);
+
+#include <sound/memalloc.h>
+#include <uapi/sound/asound.h>
+
+struct snd_pcm_substream;
+struct catpt_stream_template;
+
+struct catpt_stream_runtime {
+	struct snd_pcm_substream *substream;
+
+	struct catpt_stream_template *template;
+	struct catpt_stream_info info;
+	struct resource *persistent;
+	struct snd_dma_buffer pgtbl;
+
+	bool allocated:1;
+	bool prepared:1;
+
+	struct list_head node;
+};
+
+int catpt_register_plat_component(struct catpt_dev *cdev);
+void catpt_stream_update_position(struct catpt_dev *cdev,
+				  struct catpt_stream_runtime *stream,
+				  struct catpt_notify_position *pos);
+struct catpt_stream_runtime *
+catpt_stream_find(struct catpt_dev *cdev, u8 stream_hw_id);
+int catpt_arm_stream_templates(struct catpt_dev *cdev);
+
+#endif
diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
new file mode 100644
index 000000000000..a4a498ba3456
--- /dev/null
+++ b/sound/soc/intel/catpt/device.c
@@ -0,0 +1,376 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Cezary Rojewski <cezary.rojewski@intel.com>
+//
+// Special thanks to:
+//    Marcin Barlik <marcin.barlik@intel.com>
+//    Piotr Papierkowski <piotr.papierkowski@intel.com>
+//
+// for sharing LPT-LP and WTP-LP AudioDSP architecture expertise and
+// helping backtrack its historical background
+//
+
+#include <linux/acpi.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <sound/soc.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
+#include "core.h"
+#include "registers.h"
+
+#define CREATE_TRACE_POINTS
+#include "trace.h"
+
+#ifdef CONFIG_PM
+static int catpt_suspend(struct device *dev)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(dev);
+	struct dma_chan *chan;
+	int ret;
+
+	chan = catpt_dma_request_config_chan(cdev);
+	if (IS_ERR(chan))
+		return PTR_ERR(chan);
+
+	memset(&cdev->dx_ctx, 0, sizeof(cdev->dx_ctx));
+	ret = catpt_ipc_enter_dxstate(cdev, CATPT_DX_STATE_D3, &cdev->dx_ctx);
+	if (ret) {
+		ret = CATPT_IPC_ERROR(ret);
+		goto exit;
+	}
+
+	ret = catpt_dsp_stall(cdev, true);
+	if (ret < 0)
+		goto exit;
+
+	ret = catpt_store_memdumps(cdev, chan);
+	if (ret < 0) {
+		dev_err(cdev->dev, "store memdumps failed: %d\n", ret);
+		goto exit;
+	}
+
+	ret = catpt_store_module_states(cdev, chan);
+	if (ret < 0) {
+		dev_err(cdev->dev, "store module states failed: %d\n", ret);
+		goto exit;
+	}
+
+	ret = catpt_store_streams_context(cdev, chan);
+	if (ret < 0) {
+		dev_err(cdev->dev, "store streams ctx failed: %d\n", ret);
+		goto exit;
+	}
+exit:
+	dma_release_channel(chan);
+	if (ret < 0)
+		return ret;
+	return cdev->spec->power_down(cdev);
+}
+
+static int catpt_resume(struct device *dev)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(dev);
+	int ret, i;
+
+	ret = cdev->spec->power_up(cdev);
+	if (ret < 0)
+		return ret;
+
+	if (!module_is_live(dev->driver->owner)) {
+		dev_info(dev, "module unloading, skipping fw boot\n");
+		return 0;
+	}
+
+	ret = catpt_boot_firmware(cdev, true);
+	if (ret < 0) {
+		dev_err(cdev->dev, "boot firmware failed: %d\n", ret);
+		return ret;
+	}
+
+	/* reconfigure SSP devices after dx transition */
+	for (i = 0; i < CATPT_SSP_COUNT; i++) {
+		if (cdev->devfmt[i].iface == UINT_MAX)
+			continue;
+
+		ret = catpt_ipc_set_device_format(cdev, &cdev->devfmt[i]);
+		if (ret)
+			return CATPT_IPC_ERROR(ret);
+	}
+
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int catpt_runtime_suspend(struct device *dev)
+{
+	return catpt_suspend(dev);
+}
+
+static int catpt_runtime_resume(struct device *dev)
+{
+	return catpt_resume(dev);
+}
+#endif
+
+static const struct dev_pm_ops catpt_dev_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(catpt_suspend, catpt_resume)
+	SET_RUNTIME_PM_OPS(catpt_runtime_suspend, catpt_runtime_resume, NULL)
+};
+
+/* machine board owned by CATPT is removed with this hook */
+static void board_pdev_unregister(void *data)
+{
+	platform_device_unregister(data);
+}
+
+static int catpt_register_board(struct catpt_dev *cdev)
+{
+	const struct catpt_spec *spec = cdev->spec;
+	struct snd_soc_acpi_mach *mach;
+	struct platform_device *board;
+	int ret;
+
+	mach = snd_soc_acpi_find_machine(spec->machines);
+	if (!mach) {
+		dev_info(cdev->dev, "no machines present\n");
+		return 0;
+	}
+
+	mach->mach_params.platform = "catpt-platform";
+	board = platform_device_register_data(NULL, mach->drv_name,
+					PLATFORM_DEVID_NONE,
+					(const void *)mach, sizeof(*mach));
+	if (!board) {
+		dev_err(cdev->dev, "board register failed\n");
+		return PTR_ERR(board);
+	}
+
+	ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
+	if (ret < 0) {
+		platform_device_unregister(board);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int catpt_probe_components(struct catpt_dev *cdev)
+{
+	int ret;
+
+	ret = cdev->spec->power_up(cdev);
+	if (ret < 0)
+		return ret;
+
+	ret = catpt_dmac_probe(cdev);
+	if (ret < 0) {
+		dev_err(cdev->dev, "DMAC probe failed: %d\n", ret);
+		goto dmac_err;
+	}
+
+	ret = catpt_first_boot_firmware(cdev);
+	if (ret < 0) {
+		dev_err(cdev->dev, "first fw boot failed: %d\n", ret);
+		goto boot_fw_err;
+	}
+
+	ret = catpt_register_plat_component(cdev);
+	if (ret < 0) {
+		dev_err(cdev->dev, "register plat comp failed: %d\n", ret);
+		goto boot_fw_err;
+	}
+
+	ret = catpt_register_board(cdev);
+	if (ret < 0) {
+		dev_err(cdev->dev, "register board failed: %d\n", ret);
+		goto board_err;
+	}
+
+	ret = catpt_sysfs_create(cdev);
+	if (ret < 0)
+		goto board_err;
+
+	/* reflect actual ADSP state in pm_runtime */
+	pm_runtime_set_active(cdev->dev);
+
+	pm_runtime_set_autosuspend_delay(cdev->dev, 2000);
+	pm_runtime_use_autosuspend(cdev->dev);
+	pm_runtime_mark_last_busy(cdev->dev);
+	pm_runtime_enable(cdev->dev);
+	return 0;
+
+board_err:
+	snd_soc_unregister_component(cdev->dev);
+boot_fw_err:
+	catpt_dmac_remove(cdev);
+dmac_err:
+	cdev->spec->power_down(cdev);
+
+	return ret;
+}
+
+static void catpt_dev_init(struct catpt_dev *cdev, struct device *dev)
+{
+	cdev->dev = dev;
+	init_completion(&cdev->fw_ready);
+	INIT_LIST_HEAD(&cdev->stream_list);
+	spin_lock_init(&cdev->list_lock);
+	mutex_init(&cdev->clk_mutex);
+	/*
+	 * Mark both device formats as uninitialized. Once corresponding
+	 * cpu_dai's pcm is created, proper values are assigned
+	 */
+	cdev->devfmt[CATPT_SSP_IFACE_0].iface = UINT_MAX;
+	cdev->devfmt[CATPT_SSP_IFACE_1].iface = UINT_MAX;
+
+	catpt_ipc_init(&cdev->ipc, dev);
+
+	catpt_sram_init(&cdev->dram, cdev->spec->host_dram_offset,
+			catpt_dram_size(cdev));
+	catpt_sram_init(&cdev->iram, cdev->spec->host_iram_offset,
+			catpt_iram_size(cdev));
+}
+
+static int catpt_acpi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct catpt_dev *cdev;
+	struct resource *res;
+	int ret;
+
+	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
+	if (!cdev)
+		return -ENOMEM;
+
+	cdev->spec = device_get_match_data(dev);
+	if (!cdev->spec)
+		return -ENODEV;
+
+	catpt_dev_init(cdev, dev);
+
+	/* map DSP bar address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
+	if (!cdev->lpe_ba)
+		return -EIO;
+	cdev->lpe_base = res->start;
+
+	/* map PCI bar address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -ENODEV;
+	cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
+	if (!cdev->pci_ba)
+		return -EIO;
+
+	/* alloc buffer for storing DRAM context during dx transitions */
+	cdev->dxbuf_vaddr = dma_alloc_coherent(dev, catpt_dram_size(cdev),
+					       &cdev->dxbuf_paddr, GFP_KERNEL);
+	if (!cdev->dxbuf_vaddr)
+		return -ENOMEM;
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		goto irq_err;
+	cdev->irq = ret;
+
+	platform_set_drvdata(pdev, cdev);
+
+	ret = devm_request_threaded_irq(dev, cdev->irq, catpt_dsp_irq_handler,
+					catpt_dsp_irq_thread,
+					IRQF_SHARED, "AudioDSP", cdev);
+	if (ret < 0)
+		goto irq_err;
+
+	ret = catpt_probe_components(cdev);
+	if (ret < 0)
+		goto irq_err;
+	return 0;
+
+irq_err:
+	dma_free_coherent(cdev->dev, catpt_dram_size(cdev),
+			  cdev->dxbuf_vaddr, cdev->dxbuf_paddr);
+
+	return ret;
+}
+
+static int catpt_acpi_remove(struct platform_device *pdev)
+{
+	struct catpt_dev *cdev = platform_get_drvdata(pdev);
+
+	pm_runtime_disable(cdev->dev);
+
+	snd_soc_unregister_component(cdev->dev);
+	catpt_dmac_remove(cdev);
+	cdev->spec->power_down(cdev);
+
+	dma_free_coherent(cdev->dev, catpt_dram_size(cdev),
+			  cdev->dxbuf_vaddr, cdev->dxbuf_paddr);
+	catpt_sram_free(&cdev->iram);
+	catpt_sram_free(&cdev->dram);
+
+	catpt_sysfs_remove(cdev);
+
+	return 0;
+}
+
+static struct catpt_spec lpt_desc = {
+	.machines = snd_soc_acpi_intel_haswell_machines,
+	.core_id = 0x01,
+	.host_dram_offset = 0x000000,
+	.host_iram_offset = 0x080000,
+	.host_shim_offset = 0x0E7000,
+	.host_dma_offset = { 0x0F0000, 0x0F8000 },
+	.host_ssp_offset = { 0x0E8000, 0x0E9000 },
+	.dram_mask = LPT_VDRTCTL0_DSRAMPGE_MASK,
+	.iram_mask = LPT_VDRTCTL0_ISRAMPGE_MASK,
+	.pll_shutdown = lpt_dsp_pll_shutdown,
+	.power_up = lpt_dsp_power_up,
+	.power_down = lpt_dsp_power_down,
+};
+
+static struct catpt_spec wpt_desc = {
+	.machines = snd_soc_acpi_intel_broadwell_machines,
+	.core_id = 0x02,
+	.host_dram_offset = 0x000000,
+	.host_iram_offset = 0x0A0000,
+	.host_shim_offset = 0x0FB000,
+	.host_dma_offset = { 0x0FE000, 0x0FF000 },
+	.host_ssp_offset = { 0x0FC000, 0x0FD000 },
+	.dram_mask = WPT_VDRTCTL0_DSRAMPGE_MASK,
+	.iram_mask = WPT_VDRTCTL0_ISRAMPGE_MASK,
+	.pll_shutdown = wpt_dsp_pll_shutdown,
+	.power_up = wpt_dsp_power_up,
+	.power_down = wpt_dsp_power_down,
+};
+
+static const struct acpi_device_id catpt_ids[] = {
+	{ "INT33C8", (unsigned long)&lpt_desc },
+	{ "INT3438", (unsigned long)&wpt_desc },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, catpt_ids);
+
+static struct platform_driver catpt_acpi_driver = {
+	.probe = catpt_acpi_probe,
+	.remove = catpt_acpi_remove,
+	.driver = {
+		.name = "catpt_adsp",
+		.acpi_match_table = ACPI_PTR(catpt_ids),
+		.pm = &catpt_dev_pm,
+	},
+};
+module_platform_driver(catpt_acpi_driver);
+
+MODULE_AUTHOR("Cezary Rojewski <cezary.rojewski@intel.com>");
+MODULE_DESCRIPTION("Intel LPT/WPT AudioDSP driver");
+MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/intel/catpt/registers.h b/sound/soc/intel/catpt/registers.h
new file mode 100644
index 000000000000..108bec961183
--- /dev/null
+++ b/sound/soc/intel/catpt/registers.h
@@ -0,0 +1,191 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ *
+ * Author: Cezary Rojewski <cezary.rojewski@intel.com>
+ */
+
+#ifndef __SND_SOC_INTEL_CATPT_REGS_H
+#define __SND_SOC_INTEL_CATPT_REGS_H
+
+#include <linux/iopoll.h>
+
+#define CATPT_SHIM_REGS_SIZE	4096
+#define CATPT_DMA_REGS_SIZE	1024
+#define CATPT_DMA_COUNT		2
+#define CATPT_SSP_REGS_SIZE	512
+
+/* DSP Shim registers */
+
+#define CATPT_SHIM_CS1		0x0
+#define CATPT_SHIM_ISC		0x18
+#define CATPT_SHIM_ISD		0x20
+#define CATPT_SHIM_IMC		0x28
+#define CATPT_SHIM_IMD		0x30
+#define CATPT_SHIM_IPCC		0x38
+#define CATPT_SHIM_IPCD		0x40
+#define CATPT_SHIM_CLKCTL	0x78
+#define CATPT_SHIM_CS2		0x80
+#define CATPT_SHIM_LTRC		0xE0
+#define CATPT_SHIM_HMDC		0xE8
+
+#define CATPT_CS_LPCS		BIT(31)
+#define CATPT_CS_SFCR(ssp)	BIT(27 + ssp)
+#define CATPT_CS_S1IOCS		BIT(23)
+#define CATPT_CS_S0IOCS		BIT(21)
+#define CATPT_CS_PCE		BIT(15)
+#define CATPT_CS_SDPM(ssp)	BIT(11 + ssp)
+#define CATPT_CS_STALL		BIT(10)
+#define CATPT_CS_DCS		GENMASK(6, 4)
+/* b100 DSP core & audio fabric high clock */
+#define CATPT_CS_DCS_HIGH	(0x4 << 4)
+#define CATPT_CS_SBCS(ssp)	BIT(2 + ssp)
+#define CATPT_CS_RST		BIT(1)
+
+#define CATPT_ISC_IPCDB		BIT(1)
+#define CATPT_ISC_IPCCD		BIT(0)
+#define CATPT_ISD_DCPWM		BIT(31)
+#define CATPT_ISD_IPCCB		BIT(1)
+#define CATPT_ISD_IPCDD		BIT(0)
+
+#define CATPT_IMC_IPCDB		BIT(1)
+#define CATPT_IMC_IPCCD		BIT(0)
+#define CATPT_IMD_IPCCB		BIT(1)
+#define CATPT_IMD_IPCDD		BIT(0)
+
+#define CATPT_IPCC_BUSY		BIT(31)
+#define CATPT_IPCC_DONE		BIT(30)
+#define CATPT_IPCD_BUSY		BIT(31)
+#define CATPT_IPCD_DONE		BIT(30)
+
+#define CATPT_CLKCTL_CFCIP	BIT(31)
+#define CATPT_CLKCTL_SMOS	GENMASK(25, 24)
+
+#define CATPT_HMDC_HDDA(e, ch)	BIT(8 * e + ch)
+
+#define CATPT_CS_DEFAULT	0x8480040E
+#define CATPT_ISC_DEFAULT	0x0
+#define CATPT_ISD_DEFAULT	0x0
+#define CATPT_IMC_DEFAULT	0x7FFF0003
+#define CATPT_IMD_DEFAULT	0x7FFF0003
+#define CATPT_IPCC_DEFAULT	0x0
+#define CATPT_IPCD_DEFAULT	0x0
+#define CATPT_CLKCTL_DEFAULT	0x7FF
+#define CATPT_CS2_DEFAULT	0x0
+#define CATPT_LTRC_DEFAULT	0x0
+#define CATPT_HMDC_DEFAULT	0x0
+
+/* PCI Configuration registers */
+
+#define CATPT_PCI_PMCS		0x84
+#define CATPT_PCI_VDRTCTL0	0xA0
+#define CATPT_PCI_VDRTCTL2	0xA8
+
+#define CATPT_PMCS_PS		GENMASK(1, 0)
+#define CATPT_PMCS_PS_D3HOT	(0x3 << 0)
+
+#define CATPT_VDRTCTL2_DTCGE	BIT(10)
+#define CATPT_VDRTCTL2_DCLCGE	BIT(1)
+#define CATPT_VDRTCTL2_CGEALL	0xF7F
+
+/* LPT PCI Configuration bits */
+
+#define LPT_VDRTCTL0_DSRAMPGE(b)	BIT(16 + b)
+#define LPT_VDRTCTL0_DSRAMPGE_MASK	GENMASK(31, 16)
+#define LPT_VDRTCTL0_ISRAMPGE(b)	BIT(6 + b)
+#define LPT_VDRTCTL0_ISRAMPGE_MASK	GENMASK(15, 6)
+#define LPT_VDRTCTL0_D3SRAMPGD		BIT(2)
+#define LPT_VDRTCTL0_D3PGD		BIT(1)
+#define LPT_VDRTCTL0_APLLSE		BIT(0)
+
+/* WPT PCI Configuration bits */
+
+#define WPT_VDRTCTL0_DSRAMPGE(b)	BIT(12 + b)
+#define WPT_VDRTCTL0_DSRAMPGE_MASK	GENMASK(31, 12)
+#define WPT_VDRTCTL0_ISRAMPGE(b)	BIT(2 + b)
+#define WPT_VDRTCTL0_ISRAMPGE_MASK	GENMASK(11, 2)
+#define WPT_VDRTCTL0_D3SRAMPGD		BIT(1)
+#define WPT_VDRTCTL0_D3PGD		BIT(0)
+
+#define WPT_VDRTCTL2_APLLSE		BIT(31)
+
+/* SSP Interface registers */
+
+#define CATPT_SSP_SSC0		0x0
+#define CATPT_SSP_SSC1		0x4
+#define CATPT_SSP_SSS		0x8
+#define CATPT_SSP_SSIT		0xC
+#define CATPT_SSP_SSD		0x10
+#define CATPT_SSP_SSTO		0x28
+#define CATPT_SSP_SSPSP		0x2C
+#define CATPT_SSP_SSTSA		0x30
+#define CATPT_SSP_SSRSA		0x34
+#define CATPT_SSP_SSTSS		0x38
+#define CATPT_SSP_SSC2		0x40
+#define CATPT_SSP_SSPSP2	0x44
+
+#define CATPT_SSP_SSC0_DEFAULT		0x0
+#define CATPT_SSP_SSC1_DEFAULT		0x0
+#define CATPT_SSP_SSS_DEFAULT		0xF004
+#define CATPT_SSP_SSIT_DEFAULT		0x0
+#define CATPT_SSP_SSD_DEFAULT		0xC43893A3
+#define CATPT_SSP_SSTO_DEFAULT		0x0
+#define CATPT_SSP_SSPSP_DEFAULT		0x0
+#define CATPT_SSP_SSTSA_DEFAULT		0x0
+#define CATPT_SSP_SSRSA_DEFAULT		0x0
+#define CATPT_SSP_SSTSS_DEFAULT		0x0
+#define CATPT_SSP_SSC2_DEFAULT		0x0
+#define CATPT_SSP_SSPSP2_DEFAULT	0x0
+
+/* Physically the same block, access address differs between host and dsp */
+#define CATPT_DSP_DRAM_OFFSET		0x400000
+#define catpt_to_host_offset(offset)	(offset & ~(CATPT_DSP_DRAM_OFFSET))
+#define catpt_to_dsp_offset(offset)	(offset | CATPT_DSP_DRAM_OFFSET)
+
+#define CATPT_MEMBLOCK_SIZE	0x8000
+#define catpt_num_dram(cdev)	(hweight_long((cdev)->spec->dram_mask))
+#define catpt_num_iram(cdev)	(hweight_long((cdev)->spec->iram_mask))
+#define catpt_dram_size(cdev)	(catpt_num_dram(cdev) * CATPT_MEMBLOCK_SIZE)
+#define catpt_iram_size(cdev)	(catpt_num_iram(cdev) * CATPT_MEMBLOCK_SIZE)
+
+/* registry I/O helpers */
+
+#define catpt_shim_addr(cdev) \
+	((cdev)->lpe_ba + (cdev)->spec->host_shim_offset)
+#define catpt_dma_addr(cdev, dma) \
+	((cdev)->lpe_ba + (cdev)->spec->host_dma_offset[dma])
+#define catpt_ssp_addr(cdev, ssp) \
+	((cdev)->lpe_ba + (cdev)->spec->host_ssp_offset[ssp])
+#define catpt_inbox_addr(cdev) \
+	((cdev)->lpe_ba + (cdev)->ipc.config.inbox_offset)
+#define catpt_outbox_addr(cdev) \
+	((cdev)->lpe_ba + (cdev)->ipc.config.outbox_offset)
+
+#define catpt_writel_ssp(cdev, ssp, reg, val) \
+	writel(val, catpt_ssp_addr(cdev, ssp) + CATPT_SSP_##reg)
+
+#define catpt_readl_shim(cdev, reg) \
+	readl(catpt_shim_addr(cdev) + CATPT_SHIM_##reg)
+#define catpt_writel_shim(cdev, reg, val) \
+	writel(val, catpt_shim_addr(cdev) + CATPT_SHIM_##reg)
+#define catpt_updatel_shim(cdev, reg, mask, val) \
+	catpt_writel_shim(cdev, reg, \
+			(catpt_readl_shim(cdev, reg) & ~(mask)) | val)
+
+#define catpt_readl_poll_shim(cdev, reg, val, cond, delay_us, timeout_us) \
+	readl_poll_timeout(catpt_shim_addr(cdev) + CATPT_SHIM_##reg, \
+			   val, cond, delay_us, timeout_us)
+
+#define catpt_readl_pci(cdev, reg) \
+	readl(cdev->pci_ba + CATPT_PCI_##reg)
+#define catpt_writel_pci(cdev, reg, val) \
+	writel(val, cdev->pci_ba + CATPT_PCI_##reg)
+#define catpt_updatel_pci(cdev, reg, mask, val) \
+	catpt_writel_pci(cdev, reg, \
+			(catpt_readl_pci(cdev, reg) & ~(mask)) | val)
+
+#define catpt_readl_poll_pci(cdev, reg, val, cond, delay_us, timeout_us) \
+	readl_poll_timeout((cdev)->pci_ba + CATPT_PCI_##reg, \
+			   val, cond, delay_us, timeout_us)
+
+#endif