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:
- LPT/WPT
- BYT/CHT/BDW
- cAVS (SKL+)
- 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.