| Message ID | 20200812205753.29115-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Catpt - Lynx and Wildcat point | expand |
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.
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.
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.
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.
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.
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
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).
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.
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
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
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